Open
Conversation
87fcb49 to
1576211
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The changes on the
fix_tags_filtersbranch add support for filtering by multiple tags (?tag=a&tag=b). Our contribution on top:Regression test (
test_site_csv_exports.py): switching frommulti_to_dicttoReuseSearch.as_request_parser()for the/api/1/site/reuses.csvendpoint silently dropped thedatasetfilter, becauseReuseSearch.filtersdoesn't declare it whileReuseApiParser.parse_filterssupports it.Fix (
site/api.py): replacedReuseApiParser.parse_filterswithReuse.apply_sort_filters()— the same pattern already used by the dataservices CSV endpoint.apply_sort_filtersdiscovers its filters automatically from model field definitions, so there's no risk of desynchronization between two separately maintained filter lists. As a bonus, the generic parser always returns a list for tag values (viaaction="append"), avoiding themulti_to_dictissue of returning either a string or a list depending on the number of values.Cleanup to do
This bug reveals a broader issue: two independent filtering systems doing the same thing.
The generic system (
api_fields.py/@generate_fields): declares filters once on model fields viafilterable={}, auto-generates the parser andapply_sort_filters(). Used by v2 endpoints and v1 list endpoints.The manual
ApiParsers (ReuseApiParser,DatasetApiParser,DataserviceApiParser,OrgApiParser): redeclare the same filters by hand inparse_filtersmethods with chains ofif args.get("xxx"). Used bymongo_search()(fallback when the search service is down) and CSV exports.Both systems support the same filters for the same models, but they're not synchronized — adding a filter in one requires remembering to add it in the other. That's the root cause of the bug we just fixed.
What remains to align:
datasets.csv,resources.csvandorganizations.csvstill useDatasetApiParser.parse_filters/OrgApiParser.parse_filters+multi_to_dict. They should useModel.apply_sort_filters()like dataservices and reuses do now.mongo_search(): each Search class (ReuseSearch,DataserviceSearch,DatasetSearch,OrgSearch) has amongo_search()that callsApiParser.parse_filters+ sort + pagination.apply_sort_filtersalready does all of that.mongo_searchcould be rewritten to use it, which would make theApiParsers removable.ApiParsers themselves: once CSV exports andmongo_searchare migrated,ReuseApiParser.parse_filters,DataserviceApiParser.parse_filters,DatasetApiParser.parse_filtersandOrgApiParser.parse_filtersbecome dead code and can be deleted.