-
Notifications
You must be signed in to change notification settings - Fork 541
fix: inconsistent filtering on 'many to many' fields #2961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: inconsistent filtering on 'many to many' fields #2961
Conversation
…k-and-eventually-other-object-filters-do-as-well
WalkthroughAdded explicit ModelMultipleChoiceFilter declarations for many relationship fields in backend filter classes and removed those fields from their Meta.fields to tighten the public filter surface and avoid duplicate/implicit filters. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/core/views.py (2)
3151-3154: AppliedControl filters: good, but verify query param format from frontendThe new
folderandreference_controlModelMultipleChoiceFilters plus explicitMeta.fieldsentries forcategory/csf_function/priority/effort/control_impactwork correctly with the existingNullableChoiceFilters and should fix multi‑select behavior on these relations.One integration detail to double‑check:
ModelMultipleChoiceFilterexpects multiple values as repeated query params (?folder=id1&folder=id2), not a single comma‑separated string. If the frontend is still sending CSVs, you may want to normalize GET parameters similarly to_process_request_dataor switch relevant lookups to anin-style filter class.Also applies to: 3248-3254
5136-5160: Parent folder filter on FolderFilter is correct but could be surfaced in Meta
parent_folder = ModelMultipleChoiceFilter(queryset=Folder.objects.all())is appropriate for hierarchical navigation. If you rely onMeta.fieldsfor documentation or schema generation, consider adding"parent_folder"there as well; otherwise the explicit filter alone is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/views.py (4)
backend/iam/models.py (1)
Folder(68-316)backend/core/models.py (4)
Perimeter(1861-1909)RiskMatrix(1478-1567)ReferenceControl(1405-1475)RiskAssessment(4338-4742)backend/ebios_rm/models.py (5)
risk_matrix(352-353)risk_matrix(951-952)risk_matrix(1030-1031)ebios_rm_study(947-948)EbiosRMStudy(57-296)backend/resilience/models.py (1)
risk_matrix(214-215)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
backend/core/views.py (7)
941-983: Explicit folder/asset_class filters inAssetFilterlook correctDefining
folderandasset_classasModelMultipleChoiceFilters is consistent with how other relationship filters are modeled and avoids relying on implicitMeta.fieldsbehavior. No issues spotted.
1900-1918: RiskAssessment relationship filters are well‑scopedAdding
folder,perimeter,risk_matrix, andebios_rm_studyas explicitModelMultipleChoiceFilters aligns the API with typical usage (multi‑select on these relations) and keepsMeta.fieldsrestricted to scalar fields. Implementation looks sound.
4361-4419: Newrisk_assessmentfilter on RiskScenario is consistent and non‑breakingThe added
risk_assessment = ModelMultipleChoiceFilter(queryset=RiskAssessment.objects.all())provides a direct multi‑select filter while keeping the existingfolder/perimeteraliases intact. No logic or RBAC issues evident.
4714-4735: RiskAcceptance filters match common use casesAdding
folderandapproverasModelMultipleChoiceFilters mirrors how the viewset already exposes these relations (e.g.,waitingand state transitions). Filtering semantics are clear and should behave as expected.
4914-4952: ValidationFlowFilterSet gains useful relational filtersThe new
folder,requester, andapproverModelMultipleChoiceFilters integrate cleanly with the existinglinked_modelsfilter and ValidationFlowViewSet. This should make complex filtering significantly easier without changing existing query behavior.
6469-6492: FrameworkFilter: explicit folder filter + tighter Meta fieldsAdding
folder = ModelMultipleChoiceFilter(queryset=Folder.objects.all())and narrowingMeta.fieldsto["provider"]is a clean separation: relations are handled explicitly, simple scalars via Meta. This should help avoid ambiguous auto‑generated filters.
10401-10420: TaskTemplateFilter: folder filter aligns with task RBAC and calendar logicThe new
folderModelMultipleChoiceFiltermatches howTaskTemplateViewSetalready scopes and reports tasks. Using it viafilterset_class = TaskTemplateFilteris consistent, and the implementation doesn’t conflict with the existingfilterset_fieldslist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! LGTM. To merge once CI is green
…k-and-eventually-other-object-filters-do-as-well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/core/views.py (1)
1671-1688: Bug:RiskMatrixViewSet.probabilitywill crash due to list–union misuseThe code initializes
optionsas an empty list and then attempts to use it with the|operator:options = [] ... options = options | choices.items()In Python,
list | ...raisesTypeErrorbecause the bitwise OR operator is not defined for lists. This endpoint will crash at runtime.The suggested fix changes the accumulation to use dicts, matching the pattern in the
risk()endpoint:- undefined = {-1: "--"} - options = [] + undefined = {-1: "--"} + options = undefined for matrix in RiskMatrix.objects.filter(id__in=viewable_matrices): _choices = { i: name for i, name in enumerate( x["name"] for x in matrix.json_definition["probability"] ) } - choices = undefined | _choices - options = options | choices.items() - return Response( - [{k: v for k, v in zip(("value", "label"), o)} for o in options] - ) + options = options | _choices + res = [{"value": k, "label": v} for k, v in options.items()] + return Response(res)This resolves the TypeError and aligns with the working
risk()method implementation.
🧹 Nitpick comments (2)
backend/core/views.py (2)
3172-3202: AppliedControlFilterSet: folder/reference_control filters are correct; consider trimming Meta for choice fieldsThe explicit
folderandreference_controlModelMultipleChoiceFilters are appropriate for these FKs and match the pattern used elsewhere; excluding them fromMeta.fieldsprevents extra implicit lookup variants from being generated.
category,csf_function,priority,effort, andcontrol_impactare now handled viaNullableChoiceFilterbut also remain inMeta.fieldswith only"exact". That’s harmless, but you could optionally drop those keys fromMeta.fieldsand rely solely on the explicit filters to make the filter surface easier to reason about.Also applies to: 3269-3276
10423-10443: TaskTemplateFilter: folder filter is fine;filterset_fieldson the ViewSet is now redundant
folder = ModelMultipleChoiceFilter(queryset=Folder.objects.all())is correct for the FK and matches the pattern used for other resources.Note that
TaskTemplateViewSetsets bothfilterset_class = TaskTemplateFilterandfilterset_fields = [...]. In DRF, the explicitfilterset_classwins, sofilterset_fieldsis effectively unused for filter generation and can be confusing to future readers. Consider droppingfilterset_fieldsthere (or the explicitfilterset_class) to have a single source of truth.Also applies to: 10470-10478
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/views.py (4)
backend/iam/models.py (2)
Folder(68-316)User(496-784)backend/core/models.py (4)
Perimeter(1861-1909)RiskMatrix(1478-1567)ReferenceControl(1405-1475)RiskAssessment(4346-4750)backend/ebios_rm/models.py (5)
risk_matrix(352-353)risk_matrix(951-952)risk_matrix(1030-1031)ebios_rm_study(947-948)EbiosRMStudy(57-296)backend/resilience/models.py (1)
risk_matrix(214-215)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
backend/core/views.py (7)
963-1004: AssetFilter: explicit multi-select filters forfolderandasset_classlook goodDefining
folderandasset_classasModelMultipleChoiceFilters while omitting them fromMeta.fieldsaligns with the goal of consistent, explicit multi-select behaviour and avoids extra implicit lookups. Just ensure the frontend is sending these as multi-valued params in the waydjango-filterexpects (repeated params vs comma-separated), since that drives howModelMultipleChoiceFilterparses values.
1921-1939: RiskAssessmentFilterSet: new foreign-key multi-select filters are consistent and scopedThe added
folder,perimeter,risk_matrix, andebios_rm_studyModelMultipleChoiceFilters are correct for the underlying FK fields, and removing these keys fromMeta.fieldskeeps the exposed lookups to this explicit surface (no extra__in/range variants). Please double-check that existing clients are not relying on any of the removed implicit lookups (e.g.risk_matrix__in), and update API docs accordingly if they were.
4382-4442: RiskScenarioFilter: addingrisk_assessmentmulti-select filter is coherent with aliasesThe new
risk_assessment = ModelMultipleChoiceFilter(...)is appropriate for the FK and complements the existingfolder/perimeteralias filters. Withrisk_assessmentremoved fromMeta.fields, only this explicit filter and your alias UUID filters are exposed, which should help avoid confusing implicit lookups. Verify that UI code is updated to userisk_assessment=rather than any olderrisk_assessment__inpatterns.
4735-4757: RiskAcceptanceFilterSet: explicitfolder/approverfilters are correctUsing
ModelMultipleChoiceFilterforfolderandapprovermatches the underlying relations and brings RiskAcceptance filtering in line with other resources. Since these fields are no longer inMeta.fields, only the intended exact multi-select behaviour is available. Just confirm that any clients relying on the previous single-value behaviour (if any) have been adjusted.
4935-4975: ValidationFlowFilterSet: multi-select filters for folder/requester/approver are consistentThe new
folder,requester, andapproverModelMultipleChoiceFilters correctly reflect their FK types and make the filtering surface explicit. Keeping these out ofMeta.fieldsavoids extra implicit lookups that might not be supported by the UI. This is consistent with the rest of the PR.
5157-5182: FolderFilter:parent_foldermulti-select filter is appropriateAdding
parent_folder = ModelMultipleChoiceFilter(Folder.objects.all())is the right way to expose hierarchical filtering on folders and pairs well with the existingownedandcontent_typefilters. Excludingparent_folderfromMeta.fieldskeeps the API surface limited to exactly this filter. You may later want a RBAC-scoped queryset here, but that matches existing patterns elsewhere.
6491-6514: FrameworkFilter: explicit folder filter plus Meta.fields reduction matches intentDefining
folderas aModelMultipleChoiceFilterwhile trimmingMeta.fieldsdown to["provider"]avoids implicit framework-folder lookups and makes the two supported filters (folder,provider) very clear. This should also keep the behaviour consistent withFrameworkViewSet.filterset_class = FrameworkFilter.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.