DOCSP-57250: Reflect dataset limitation around movie year (no data 2016+)#80
DOCSP-57250: Reflect dataset limitation around movie year (no data 2016+)#80cbullinger merged 7 commits intodevelopmentfrom
Conversation
dacharyc
left a comment
dacharyc
left a comment
There was a problem hiding this comment.
Few comments re: the JS/Express server implementation, but I'll pause here based on our convo on Zoom re: making the year detection dynamic.
- Remove hardcoded year bounds (1800-2030) from all three backends - Backend aggregations now only validate year field is numeric (: number) - Add fetchYearBounds() on client to dynamically detect min/max years from data - Move dataset warning above Year field label in FilterBar - Update tests to reflect removed year bound validation Backends modified: Java Spring, Express, Python FastAPI Client modified: FilterBar.tsx, api.ts
The Atlas CLI local deployment creates a replica set (myLocalRs1) that advertises hostname 'mylocalrs1'. Without directConnection=true, the MongoDB driver attempts server discovery and fails to resolve this hostname in CI, causing all tests to fail with: MongoServerSelectionError: getaddrinfo EAI_AGAIN mylocalrs1
dacharyc
left a comment
dacharyc
left a comment
There was a problem hiding this comment.
Changes look good with some minor tweaks, but I did spot an unrelated issue during testing that we probably want to cut a ticket for.
| loadGenres(); | ||
| }, []); | ||
|
|
||
| // Fetch year bounds from the API on mount |
There was a problem hiding this comment.
🎉
Love this approach - seems much cleaner.
| if genre: | ||
| filter_dict["genres"] = {"$regex": genre, "$options": "i"} | ||
| if year: | ||
| if isinstance(year, int): |
There was a problem hiding this comment.
Seems unrelated to this change, but if I try to filter only based on year, the FastAPI server is throwing an error. Looks like it's expecting a minimum IMDB rating when we try to apply a filter, and since there is no default value coming from the client, it's blowing up.
I suspect this is not the desired behavior, but since it's unrelated to this change, maybe we want to cut a separate ticket to address it?
There was a problem hiding this comment.
weird, I'm not getting any error when only filtering by year with the FastAPI server... 🤔 but I'll cut a ticket to look into it!
There was a problem hiding this comment.
Ticket cut: https://jira.mongodb.org/browse/DOCSP-57393
- Quote all references to $GITHUB_STEP_SUMMARY to prevent bash syntax errors when the variable is empty or unset - Add guard at script start to gracefully skip summary generation if GITHUB_STEP_SUMMARY is not set This fixes the 'Generate Test Summary' step failure in CI where unquoted variable references caused 'echo >> ' syntax errors with set -e enabled.
Create index on comments.movie_id after data restore to fix the /reportingByComments aggregation timeout. Without this index, the $lookup operation performs a full collection scan (41K comments x 21K movies) causing 5+ minute response times in CI vs ~700ms locally.
The previous commit added an index creation step that requires mongosh, but mongosh is not installed on the Ubuntu runner by default. This adds a step to download and install mongosh-2.3.8-linux-x64.
Co-authored-by: Dachary <dachary.carey@mongodb.com>
|
Unrelated, but had to also make some updates to the Express Tests workflow so it'd pass without timing out or returning no test summary |
2 participants
Add tests to all three backends around year validation