feat: Duplicate applied controls properly in mapping #2569
Conversation
|
Warning Rate limit exceeded@monsieurswag has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe perform_create workflow in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API
participant View as perform_create
participant DB
participant Service as computations
Client->>API: POST create ComplianceAssessment
API->>View: validate & save serializer
View->>DB: save -> new_audit
Note right of View: subsequent operations use new_audit
alt baseline framework differs
View->>Service: compute_requirement_assessments_results(new_audit, baseline)
Service-->>View: results
else same framework
View->>Service: compute_requirement_assessments_results(new_audit)
Service-->>View: results
end
rect rgba(230,245,255,0.6)
Note over View,DB: AppliedControl duplication path (conditional)
View->>DB: find candidate applied controls by folder relations
View->>DB: create/update duplicated AppliedControl entries
View->>DB: duplicate related objects (assets,evidences,labels,exceptions,objectives)
View-->>View: cache duplicated_applied_controls map
end
View->>DB: bulk attach applied controls to assessments (use duplicates when flagged)
View->>DB: prefetch new_audit.requirement_assessments
View->>DB: create AppliedControl from suggestions (on new_audit)
View-->>API: return new_audit with assessments & relations
API-->>Client: 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/views.py (1)
5415-5423: Potential performance concern with individual savesThe code creates AppliedControl instances one by one with individual database operations. For audits with many controls, this could be slow. Consider batching these operations.
- duplicated_applied_control_data = { - field.name: getattr(applied_control, field.name) - for field in applied_control._meta.fields - if field.name - not in ["id", "pk", "created_at", "updated_at"] - } - duplicated_applied_control_data["folder"] = ( - new_audit.folder - ) - - duplicated_applied_control, created = ( - AppliedControl.objects.update_or_create( - defaults=duplicated_applied_control_data, - name=applied_control.name, - folder=new_audit.folder, - ) - ) + # Consider collecting all controls to create and using bulk_create + # after checking for existing ones in a single query + duplicated_applied_control_data = { + field.name: getattr(applied_control, field.name) + for field in applied_control._meta.fields + if field.name + not in ["id", "pk", "created_at", "updated_at"] + } + duplicated_applied_control_data["folder"] = ( + new_audit.folder + ) + + duplicated_applied_control, created = ( + AppliedControl.objects.update_or_create( + defaults=duplicated_applied_control_data, + name=applied_control.name, + folder=new_audit.folder, + ) + )Note: The current approach is functionally correct but could benefit from optimization if performance becomes an issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/views.py (3)
backend/core/models.py (16)
ComplianceAssessment(4271-5046)save(1572-1575)save(2258-2260)save(2861-2865)save(3075-3082)save(3377-3379)save(3512-3515)save(3583-3585)save(4193-4219)save(4342-4348)save(5232-5238)save(5673-5686)create_requirement_assessments(4350-4435)AppliedControl(2877-3234)compute_requirement_assessments_results(4917-5000)applied_controls(5729-5730)backend/iam/models.py (2)
get_parent_folders(127-131)created(1000-1001)backend/core/helpers.py (1)
duplicate_related_objects(1552-1651)
⏰ 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: build (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
backend/core/views.py (7)
5333-5335: Good refactoring for clarityThe renaming from
instancetonew_auditimproves code readability by making it clear we're working with the newly created audit throughout the method.
5342-5345: Consider adding type hintWhile the variable name is clear, adding a type hint would make the expected type explicit.
- new_audit: ComplianceAssessment = serializer.save() + new_audit: ComplianceAssessment = serializer.save()Wait, the type hint is already there. This looks good!
5450-5452: Good caching strategyUsing a dictionary to cache duplicated controls avoids recreating the same control multiple times when it's referenced by multiple assessments. This is an efficient approach.
5467-5468: Consistent naming maintainedThe change from
instance.requirement_assessmentstonew_audit.requirement_assessmentsmaintains consistency with the refactored variable naming.
5336-5340: Verify ancestor/descendant logic for control duplicationget_parent_folders yields baseline.folder's ancestors (backend/iam/models.py:127–131). The current condition in backend/core/views.py:5336–5340 duplicates controls when new_audit.folder != baseline.folder AND new_audit.folder is not an ancestor of baseline — it will still duplicate when new_audit.folder is a descendant of baseline or in a different subtree. Confirm whether duplication should also be suppressed for descendant folders; if so, update the check to test both directions (e.g., ensure neither folder is an ancestor of the other).
5456-5462: Verify M2M handling for applied_controls (backend/core/views.py:5456–5462)
- Confirm applied_controls is a direct ManyToManyField (no explicit through). If it uses a through model, .add(*(ac.id for ac in ...)) will not create through entries—create through-model instances instead.
- Migrations reference on_delete=CASCADE and SET_NULL (backend/core/migrations/0012_alter_appliedcontrol_updated_at_and_more.py, 0017_requirementassessment_mapping_inference_and_more.py, 0052_securityexception_appliedcontrol_security_exceptions_and_more.py); verify duplication/deletion logic won't drop or orphan AppliedControl rows.
- Repo search for the model definition returned no results in the last run—locate the applied_controls field (or run: rg -n "applied_controls\s*=") and re-run verification.
5408-5462: Verify duplication helper: missing M2M copying & uniqueness checks
- I inspected duplicate_related_objects (backend/core/helpers.py): it copies non‑PK fields and sets folder, links existing objects, or creates new ones via model_class.objects.create(...). It does not duplicate the related object’s own ManyToMany fields or nested relations.
- Risk: if related models (Asset, Evidence, FilteringLabel, SecurityException, Objective) have M2M fields, unique constraints, or non‑nullable FKs, create(...) may lose relationships or raise IntegrityError — confirm model constraints and update the helper to copy M2M relations and handle uniqueness/errors.
- Confirm the AppliedControl fields passed (assets, evidences, filtering_labels, security_exceptions, objectives) are ManyToMany (so .add(...) is correct) and that folder parent/subfolder comparisons use comparable Folder instances/IDs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/core/views.py (3)
5341-5347: Duplication condition semantics + dict key typing.
- Verify intent: for child-of-baseline folders, duplicate_applied_controls becomes True (you’ll duplicate); is that what you want, or should children reuse parent controls? Please confirm.
- Use UUID-typed keys for the cache.
- duplicated_applied_controls: dict[str, AppliedControl] = {} + duplicated_applied_controls: dict[UUID, AppliedControl] = {}
5357-5361: Guard mapping mapping_set usage and prefer new_audit.framework.If mapping_inference is missing for any computed assessment, this will KeyError later. Also, prefer using new_audit.framework over serializer.validated_data for resilience.
Example (outside the selected lines):
# prefer object fields and guard DoesNotExist try: mapping_set = RequirementMappingSet.objects.select_related( "source_framework", "target_framework" ).get( target_framework=new_audit.framework, source_framework=baseline.framework, ) except RequirementMappingSet.DoesNotExist: raise ValidationError({"error": "missingFrameworkMapping"})
5392-5398: Carry source RA through M2M ops to enable evidence duplication when needed.Include baseline_ra in the tuple so we can apply duplicate_related_objects later.
- m2m_operations.append( - ( - requirement_assessment, - baseline_ra.evidences.all(), - baseline_ra.applied_controls.all(), - ) - ) + m2m_operations.append( + ( + requirement_assessment, + baseline_ra, # keep handle for evidence duplication policy + baseline_ra.evidences.all(), + baseline_ra.applied_controls.all(), + ) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/views.py (3)
backend/core/models.py (13)
save(1572-1575)save(2258-2260)save(2861-2865)save(3075-3082)save(3377-3379)save(3512-3515)save(3583-3585)save(4193-4219)save(4342-4348)save(5232-5238)save(5673-5686)AppliedControl(2877-3234)applied_controls(5729-5730)backend/iam/models.py (2)
get_parent_folders(127-131)created(1000-1001)backend/core/helpers.py (1)
duplicate_related_objects(1552-1651)
⏰ 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). (8)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
🔇 Additional comments (2)
backend/core/views.py (2)
5333-5338: Good switch to new_audit and early RA creation.Saving to new_audit and creating RAs first is correct; copying show_documentation_score for same-framework baselines is also right.
5467-5468: LGTM: efficient prefetch for suggestions.Prefetching requirement__reference_controls off new_audit RAs is appropriate.
| for assessment, evidences, applied_controls in m2m_operations: | ||
| assessment.evidences.add(*[ev.id for ev in evidences]) | ||
| assessment.applied_controls.add(*[ac.id for ac in controls]) | ||
|
|
||
| if duplicate_applied_controls: | ||
| new_applied_controls = [] | ||
| for applied_control in applied_controls: | ||
| if ( | ||
| duplicated_applied_control | ||
| := duplicated_applied_controls.get(applied_control.id) | ||
| ) is None: | ||
| duplicated_applied_control_data = { | ||
| field.name: getattr(applied_control, field.name) | ||
| for field in applied_control._meta.fields | ||
| if field.name | ||
| not in ["id", "pk", "created_at", "updated_at"] | ||
| } | ||
| duplicated_applied_control_data["folder"] = ( | ||
| new_audit.folder | ||
| ) | ||
|
|
||
| duplicated_applied_control, created = ( | ||
| AppliedControl.objects.update_or_create( | ||
| defaults=duplicated_applied_control_data, | ||
| name=applied_control.name, | ||
| folder=new_audit.folder, | ||
| ) | ||
| ) | ||
| if created: | ||
| for field_name in [ | ||
| "assets", | ||
| "evidences", | ||
| "filtering_labels", | ||
| "security_exceptions", | ||
| "objectives", | ||
| ]: | ||
| duplicate_related_objects( | ||
| applied_control, | ||
| duplicated_applied_control, | ||
| new_audit.folder, | ||
| field_name, | ||
| ) | ||
|
|
||
| duplicated_applied_control.owner.set( | ||
| applied_control.owner.all() | ||
| ) | ||
| duplicated_applied_control.save() | ||
|
|
||
| duplicated_applied_controls[applied_control.id] = ( | ||
| duplicated_applied_control | ||
| ) | ||
| new_applied_controls.append(duplicated_applied_control) | ||
| assessment.applied_controls.add( | ||
| *(ac.id for ac in new_applied_controls) | ||
| ) | ||
| else: | ||
| assessment.applied_controls.add( | ||
| *(ac.id for ac in applied_controls) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid cross-folder evidence links; safer owner updates; simpler M2M adds.
- Linking baseline evidences by ID can break folder-permission boundaries when duplicating across folders. Duplicate or link per helper policy instead.
- Don’t overwrite owners on an already existing duplicate; union them to avoid clobbering.
- Prefer passing model instances to add().
- for assessment, evidences, applied_controls in m2m_operations:
- assessment.evidences.add(*[ev.id for ev in evidences])
+ for assessment, baseline_ra, evidences, applied_controls in m2m_operations:
+ # Evidence: avoid cross-folder links when duplicating controls
+ if duplicate_applied_controls:
+ duplicate_related_objects(
+ baseline_ra, assessment, new_audit.folder, "evidences"
+ )
+ else:
+ assessment.evidences.add(*evidences)
@@
- duplicated_applied_control.owner.set(
- applied_control.owner.all()
- )
+ if created:
+ duplicated_applied_control.owner.set(
+ applied_control.owner.all()
+ )
+ else:
+ duplicated_applied_control.owner.add(
+ *applied_control.owner.all()
+ )
@@
- assessment.applied_controls.add(
- *(ac.id for ac in new_applied_controls)
- )
+ assessment.applied_controls.add(*new_applied_controls)
else:
- assessment.applied_controls.add(
- *(ac.id for ac in applied_controls)
- )
+ assessment.applied_controls.add(*applied_controls)Additionally, update_or_create keyed on (name, folder) may raise MultipleObjectsReturned if that pair isn’t unique. If uniqueness isn’t guaranteed, consider strengthening the lookup (e.g., include ref_id when present) or handling collisions.
🏁 Script executed:
#!/bin/bash
# Check for uniqueness constraints on AppliedControl (folder/name or similar)
rg -n -C3 'class\s+AppliedControl\b' backend/core/models.py
rg -n -C2 'unique_together|UniqueConstraint|unique\s*=' backend/core/models.pyLength of output: 1637
Avoid cross-folder evidence links; don't overwrite owners; handle update_or_create collisions.
- Don’t link baseline evidences by ID when duplicating across folders — duplicate them instead (use duplicate_related_objects) rather than assessment.evidences.add(*[ev.id for ev in evidences]). (backend/core/views.py ~5405-5462)
- When a duplicated AppliedControl already exists, union owners instead of overwriting: call .owner.set(...) only when created; otherwise .owner.add(*applied_control.owner.all()).
- Pass model instances to .add() (assessment.applied_controls.add(*new_applied_controls)) instead of IDs.
- Critical: update_or_create(..., name=applied_control.name, folder=new_audit.folder) can raise MultipleObjectsReturned — AppliedControl.name is non-unique and there’s no (name, folder) uniqueness constraint in backend/core/models.py (class AppliedControl ~line 2877). Strengthen the lookup (e.g., include ref_id), handle MultipleObjectsReturned, or add an explicit unique constraint + migration.
🤖 Prompt for AI Agents
In backend/core/views.py around lines 5405-5462: when duplicating assessments
you currently link evidences by ID, overwrite owners, pass IDs into M2M.add, and
call update_or_create with a non-unique lookup which can raise
MultipleObjectsReturned. Fix by duplicating evidence objects via
duplicate_related_objects and add those duplicated instances to
assessment.evidences (not original IDs); when creating/looking up duplicated
AppliedControl, strengthen the lookup (e.g., include a stable unique field like
ref_id or another identifying field) or wrap update_or_create in a try/except to
catch MultipleObjectsReturned and disambiguate (or add a DB unique constraint +
migration beforehand); only call duplicated_applied_control.owner.set(...) when
a new AppliedControl was created, otherwise merge owners with
duplicated_applied_control.owner.add(*applied_control.owner.all()); and pass
model instances (not IDs) into
assessment.applied_controls.add(*new_applied_controls).
1 participant
Summary by CodeRabbit
New Features
Refactor