perf: improve audit creation time with bulk mode #1437
Conversation
WalkthroughThis pull request focuses on optimizing database operations in the compliance assessment creation process. The changes primarily target the Changes
Sequence DiagramsequenceDiagram
participant View as ComplianceAssessmentViewSet
participant Model as ComplianceAssessment
participant DB as Database
View->>Model: create_requirement_assessments()
Model->>DB: Fetch requirements with select_related
Model->>DB: Prefetch baseline assessments
Model->>DB: Bulk create RequirementAssessments
Model->>DB: Update with baseline data
Model-->>View: Return created assessments
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (4)
backend/core/views.py (3)
3420-3427: Consider combining nested transaction blocks.
Lines 3420 and 3427 both initiatewith transaction.atomic():. If there's no need for partial commits or rollback granularity, a singletransaction.atomic()may suffice and simplify the logic.
3431-3492: Add fallback for missing source IDs & consider explicit imports.
- If a future logic error causes an ID mismatch,
baseline_assessments[source_id]could raise aKeyError. Consider a safe fallback or an explicit check to improve robustness.- Also, static analysis flags that
RequirementAssessmentmight come from a star import. To avoid lint errors (F405) and improve clarity, prefer an explicit import statement:- from core.models import * + from core.models import RequirementAssessment, ...🧰 Tools
🪛 Ruff (0.8.2)
3455-3455:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
3485-3485:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
3493-3501: Evaluate bulk creation of suggested applied controls.
Callingcreate_applied_controls_from_suggestions()for each requirement assessment may trigger multiple queries. If the dataset is large, consider grouping suggestions and performing a bulk creation or factoring out some logic to reduce repetitive operations.backend/core/models.py (1)
2782-2853: Great optimization of database operations!The implementation efficiently reduces database queries through:
- Single query fetching with
select_related()- Bulk operations for creation and updates
- Efficient handling of many-to-many relationships
Consider these improvements for better maintainability and observability:
- def create_requirement_assessments( - self, baseline: Self | None = None - ) -> list["RequirementAssessment"]: + def create_requirement_assessments( + self, baseline: Self | None = None + ) -> list["RequirementAssessment"]: + """Create requirement assessments for the compliance assessment. + + Args: + baseline: Optional baseline compliance assessment to copy data from. + + Returns: + List of created RequirementAssessment objects. + + Raises: + DatabaseError: If database operations fail. + """ + logger = get_logger(__name__) + logger.info( + "Creating requirement assessments", + compliance_assessment_id=self.id, + baseline_id=baseline.id if baseline else None + ) + try: # Fetch all requirements in a single query requirements = RequirementNode.objects.filter( framework=self.framework ).select_related() # If there's a baseline, prefetch all related baseline assessments baseline_assessments = {} if baseline and baseline.framework == self.framework: baseline_assessments = { ra.requirement_id: ra for ra in RequirementAssessment.objects.filter( compliance_assessment=baseline, requirement__in=requirements ).prefetch_related("evidences", "applied_controls") } # Create all RequirementAssessment objects in bulk requirement_assessments = [ RequirementAssessment( compliance_assessment=self, requirement=requirement, folder_id=self.folder.id, answer=transform_question_to_answer(requirement.question) if requirement.question else {}, ) for requirement in requirements ] # Bulk create all assessments created_assessments = RequirementAssessment.objects.bulk_create( requirement_assessments ) if baseline_assessments: updates = [] m2m_operations = [] for assessment in created_assessments: baseline_assessment = baseline_assessments.get( assessment.requirement_id ) if baseline_assessment: # Update scalar fields assessment.result = baseline_assessment.result assessment.status = baseline_assessment.status assessment.score = baseline_assessment.score assessment.is_scored = baseline_assessment.is_scored assessment.observation = baseline_assessment.observation updates.append(assessment) # Store M2M operations for later m2m_operations.append( ( assessment, baseline_assessment.evidences.all(), baseline_assessment.applied_controls.all(), ) ) # Bulk update scalar fields if updates: RequirementAssessment.objects.bulk_update( updates, ["result", "status", "score", "is_scored", "observation"] ) # Handle M2M relationships for assessment, evidences, controls in m2m_operations: assessment.evidences.set(evidences) assessment.applied_controls.set(controls) + logger.info( + "Successfully created requirement assessments", + count=len(created_assessments) + ) return created_assessments + except Exception as e: + logger.error( + "Failed to create requirement assessments", + error=str(e), + compliance_assessment_id=self.id, + baseline_id=baseline.id if baseline else None + ) + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/models.py(1 hunks)backend/core/views.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
3455-3455: RequirementAssessment may be undefined, or defined from star imports
(F405)
3485-3485: RequirementAssessment may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: ruff (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/core/views.py (1)
3428-3430: Creation workflow looks good.
Saving the serializer and directly callinginstance.create_requirement_assessments(baseline)is clear and concise. No immediate issues spotted here.
ab-smith
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/iam/models.py (3)
682-684: Consider extracting permission code generation into a helper.
The dynamic generation ofpermission_codenamesis a good approach. However, separating out this logic into a dedicated helper function might keep the main method cleaner.🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
699-702: Guard against potentially huge datasets.
Here, all objects of the given type are fetched in one go. For very large tables, this can be memory-intensive. Consider filtering objects by folder or subfolders before collecting them in memory.🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
703-723: Consider optimizing nested loops for large role assignments.
The current approach iterates over role assignments, perimeters, and objects. For large data sets, this could degrade performance. Evaluate whether partial pre-filtering or additional indexing might help, or if a more direct SQL-based approach is preferable.🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
backend/core/helpers.py (2)
891-891: Run automatic formatting to comply with linters.
The pipeline logs indicate that the file needs formatting. Runruff formator equivalent to align with style requirements.🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
894-900: Avoid potential name collisions from star imports.
Static analysis warns that classes likeAppliedControlmay be undefined or overshadowed. It’s safer to import explicitly to improve readability and avoid naming conflicts.🧰 Tools
🪛 Ruff (0.8.2)
897-897:
AppliedControlmay be undefined, or defined from star imports(F405)
897-897:
ComplianceAssessmentmay be undefined, or defined from star imports(F405)
897-897:
RiskAssessmentmay be undefined, or defined from star imports(F405)
898-898:
RiskScenariomay be undefined, or defined from star imports(F405)
898-898:
Threatmay be undefined, or defined from star imports(F405)
898-898:
RiskAcceptancemay be undefined, or defined from star imports(F405)
898-898:
Evidencemay be undefined, or defined from star imports(F405)
898-898:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/helpers.py(2 hunks)backend/iam/models.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/helpers.py
897-897: AppliedControl may be undefined, or defined from star imports
(F405)
897-897: ComplianceAssessment may be undefined, or defined from star imports
(F405)
897-897: RiskAssessment may be undefined, or defined from star imports
(F405)
898-898: RiskScenario may be undefined, or defined from star imports
(F405)
898-898: Threat may be undefined, or defined from star imports
(F405)
898-898: RiskAcceptance may be undefined, or defined from star imports
(F405)
898-898: Evidence may be undefined, or defined from star imports
(F405)
898-898: RequirementAssessment may be undefined, or defined from star imports
(F405)
903-903: AppliedControl may be undefined, or defined from star imports
(F405)
903-903: AppliedControl may be undefined, or defined from star imports
(F405)
904-904: ComplianceAssessment may be undefined, or defined from star imports
(F405)
904-904: ComplianceAssessment may be undefined, or defined from star imports
(F405)
926-926: RiskAssessment may be undefined, or defined from star imports
(F405)
927-927: RiskScenario may be undefined, or defined from star imports
(F405)
928-928: Threat may be undefined, or defined from star imports
(F405)
928-928: Threat may be undefined, or defined from star imports
(F405)
929-929: RiskAcceptance may be undefined, or defined from star imports
(F405)
933-933: ComplianceAssessment may be undefined, or defined from star imports
(F405)
935-935: Evidence may be undefined, or defined from star imports
(F405)
936-936: RequirementAssessment may be undefined, or defined from star imports
(F405)
937-937: RequirementAssessment may be undefined, or defined from star imports
(F405)
🪛 GitHub Actions: Backend Linters
backend/core/helpers.py
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
backend/iam/models.py
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test (3.12)
🔇 Additional comments (4)
backend/iam/models.py (2)
725-740: Double-check published object inheritance logic.
When granting view permissions for published objects, the code ascends parent folders. Confirm that all required parent folder checks are correct. Edge cases (e.g., newly marked published objects in a wide hierarchy) might be missed if intermediate references are incomplete.🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
743-747: Validate partitioning logic for duplicate permissions.
Objects may receive multiple permissions (e.g., both “view” and “delete”). Ensure that these IDs are intentionally allowed in multiple lists or that duplicates do not cause inconsistent results downstream.🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
backend/core/helpers.py (2)
928-929: Validate threat filtering logic.
Filtering threats only ifrisk_scenarios__isnull=Falseand IDs inaccessible_objects[Threat]might skip newly created threats without scenarios. Confirm if you intend to omit those from the metric count.🧰 Tools
🪛 Ruff (0.8.2)
928-928:
Threatmay be undefined, or defined from star imports(F405)
928-928:
Threatmay be undefined, or defined from star imports(F405)
929-929:
RiskAcceptancemay be undefined, or defined from star imports(F405)
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
936-939: Confirm that partial or in-progress states are not overlooked in “non_compliant_items”.
Currently, “non_compliant_items” counts only items explicitly flagged as "non_compliant." If partial or incomplete states must also be flagged, adjust the filters accordingly.🧰 Tools
🪛 Ruff (0.8.2)
936-936:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
937-937:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
backend/core/helpers.py
Outdated
| # Efficiently compute the average progress from the method calls | ||
| progress_sum = sum(comp.progress() for comp in viewable_compliance) | ||
| progress_count = viewable_compliance.count() | ||
| progress_avg = math.ceil(progress_sum / progress_count) if progress_count else 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce repeated queries in progress calculation.
If comp.progress() triggers multiple DB queries inside a loop, consider an approach using annotate() or batch-processing to compute progress in fewer queries.
🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] File requires formatting. Run 'ruff format' to fix formatting issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
frontend/src/routes/(app)/(internal)/analytics/+page.server.ts (6)
80-84: Defensive check forresultsproperty.
ordAppliedControlsRes.resultsis assumed to exist. Consider verifying thatordAppliedControlsRes.resultsis defined and is an array, to avoid runtime errors in edge cases with absent or malformed responses.
85-89: Mirror defensive checks for measures.
Similarly, ensuremeasuresToReviewRes.resultsis guarded before mapping. This improves stability if the server response doesn’t match expectations.
90-94: Add checks for acceptances to review.
As with the prior arrays, consider verifyingacceptancesToReviewRes.resultsis available to prevent errors.
95-127: Consider performance optimization for nested fetch calls.
Inside the map overprojectsRes.results, each project spawns new fetch calls for compliance assessments and then multiple fetches for donut data and global score. This can become expensive if there are many projects. Investigate bundling these queries or using a bulk endpoint to reduce repeated round trips.
129-157: Aggregation logic is straightforward but watch out for large data sets.
The nested loops that accumulate donut items and compute percentages are clear. For very large data sets, consider more efficient aggregation and dedicated back-end endpoints to reduce overall load.
162-177: Avoid returning large objects when partially unused.
A wide array of data is returned, including numerous status buckets, metrics, counters, and projects. Validate whether the frontend truly needs all these properties at once or if lazy loading might suffice.backend/iam/models.py (1)
737-755: Enhance readability of published objects handling.The published objects handling logic could be more readable by extracting the parent folder traversal into a separate method.
Consider refactoring for better readability:
+ def _get_parent_folders(folder: Folder) -> List[Folder]: + """Get all parent folders of a given folder.""" + parent_folders = [] + current_folder = folder + while current_folder: + if current_folder != folder: + parent_folders.append(current_folder) + current_folder = current_folder.parent_folder + return parent_folders # Handle published objects for view access when not in ENCLAVE folders if hasattr(object_type, "is_published"): for folder in accessible_folders: if folder.content_type != Folder.ContentType.ENCLAVE: - current_folder = folder - target_folders = [] - while current_folder: - if current_folder != folder: - target_folders.append(current_folder) - current_folder = current_folder.parent_folder + target_folders = _get_parent_folders(folder)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/core/helpers.py(2 hunks)backend/iam/models.py(1 hunks)frontend/src/routes/(app)/(internal)/analytics/+page.server.ts(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/helpers.py
899-899: AppliedControl may be undefined, or defined from star imports
(F405)
900-900: ComplianceAssessment may be undefined, or defined from star imports
(F405)
901-901: RiskAssessment may be undefined, or defined from star imports
(F405)
902-902: RiskScenario may be undefined, or defined from star imports
(F405)
903-903: Threat may be undefined, or defined from star imports
(F405)
904-904: RiskAcceptance may be undefined, or defined from star imports
(F405)
905-905: Evidence may be undefined, or defined from star imports
(F405)
906-906: RequirementAssessment may be undefined, or defined from star imports
(F405)
911-911: AppliedControl may be undefined, or defined from star imports
(F405)
912-912: AppliedControl may be undefined, or defined from star imports
(F405)
914-914: ComplianceAssessment may be undefined, or defined from star imports
(F405)
915-915: ComplianceAssessment may be undefined, or defined from star imports
(F405)
938-938: RiskAssessment may be undefined, or defined from star imports
(F405)
939-939: RiskScenario may be undefined, or defined from star imports
(F405)
940-940: Threat may be undefined, or defined from star imports
(F405)
941-941: Threat may be undefined, or defined from star imports
(F405)
945-945: RiskAcceptance may be undefined, or defined from star imports
(F405)
951-951: ComplianceAssessment may be undefined, or defined from star imports
(F405)
955-955: Evidence may be undefined, or defined from star imports
(F405)
956-956: RequirementAssessment may be undefined, or defined from star imports
(F405)
957-957: RequirementAssessment may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
frontend/src/routes/(app)/(internal)/analytics/+page.server.ts (2)
50-50: Add error checks for fetch responses.
Promise.all(urls.map((url) => fetch(url)))might yield failing responses. In production, you may want to verifyresponse.okor handle HTTP error codes for each response to ensure robust error handling.
51-67: Ensure JSON parsing is safe.
Destructuring JSON responses further down could fail (e.g., if a response isn’t valid JSON). Consider guarding with try/catch or verifying response content types.backend/iam/models.py (4)
682-687: LGTM! Efficient permission codename generation.The dynamic generation of permission codenames is clean and efficient, using list comprehension to create the required permission patterns.
689-696: Great optimization using select_related and prefetch_related!The query optimization using
select_relatedandprefetch_relatedreduces the number of database queries by fetching related data in a single query.
698-702: Well-structured data storage using appropriate data structures.Good choice of using a set for accessible folders (constant-time lookups) and defaultdict for permission mapping (automatic initialization).
707-713: Consider adding error handling for folder retrieval.While the preloading of objects is efficient, the folder retrieval might fail silently if
get_folderreturns None.Add error handling to manage cases where folder retrieval fails:
- folder_for_object = {obj.id: Folder.get_folder(obj) for obj in all_objects} + folder_for_object = {} + for obj in all_objects: + folder = Folder.get_folder(obj) + if folder is None: + logger.warning(f"Could not get folder for object {obj.id}") + continue + folder_for_object[obj.id] = folderbackend/core/helpers.py (2)
891-908: Consider using bulk_create for better performance.The current implementation fetches accessible objects for each model separately. Consider using
bulk_createto reduce the number of database queries.🧰 Tools
🪛 Ruff (0.8.2)
899-899:
AppliedControlmay be undefined, or defined from star imports(F405)
900-900:
ComplianceAssessmentmay be undefined, or defined from star imports(F405)
901-901:
RiskAssessmentmay be undefined, or defined from star imports(F405)
902-902:
RiskScenariomay be undefined, or defined from star imports(F405)
903-903:
Threatmay be undefined, or defined from star imports(F405)
904-904:
RiskAcceptancemay be undefined, or defined from star imports(F405)
905-905:
Evidencemay be undefined, or defined from star imports(F405)
906-906:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
918-924: 🛠️ Refactor suggestionOptimize progress calculation to reduce database queries.
The current implementation calls
progress()for each compliance assessment, which could lead to N+1 query problems.Consider using database aggregation:
- progress_sum = sum(comp.progress() for comp in viewable_compliance) - progress_count = viewable_compliance.count() - progress_avg = math.ceil(progress_sum / progress_count) if progress_count else 0 + from django.db.models import Avg + progress_avg = math.ceil( + viewable_compliance.annotate( + progress=models.F('completed_requirements') * 100.0 / models.F('total_requirements') + ).aggregate(avg_progress=Avg('progress'))['avg_progress'] or 0 + )Likely invalid or redundant comment.
| const urls = [ | ||
| `${BASE_API_URL}/applied-controls/per_status/`, | ||
| `${BASE_API_URL}/risk-assessments/per_status/`, | ||
| `${BASE_API_URL}/compliance-assessments/per_status/`, | ||
| `${BASE_API_URL}/risk-scenarios/per_status/`, | ||
| `${BASE_API_URL}/applied-controls/todo/`, | ||
| `${BASE_API_URL}/get_counters/`, | ||
| `${BASE_API_URL}/get_metrics/`, | ||
| `${BASE_API_URL}/risk-matrices/used/`, | ||
| `${BASE_API_URL}/frameworks/used/`, | ||
| `${BASE_API_URL}/risk-scenarios/count_per_level/`, | ||
| `${BASE_API_URL}/threats/threats_count/`, | ||
| `${BASE_API_URL}/applied-controls/to_review/`, | ||
| `${BASE_API_URL}/risk-acceptances/to_review/`, | ||
| `${BASE_API_URL}/risk-assessments/`, | ||
| `${BASE_API_URL}/projects/` | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good approach to batch requests but consider resilience.
The array of target URLs is neatly consolidated for concurrent fetching. However, if one of these endpoints fails, the entire Promise.all may reject. Consider adding fallback or individual error handling so that a single failing endpoint doesn’t break the entire data loading process.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/core/models.py (1)
2799-2807: Add validation for requirement.question access.The code accesses
requirement.questionwithout checking if it exists first. While the code handles the None case in the dict comprehension, it's better to validate the attribute access to prevent potential AttributeError.- answer=transform_question_to_answer(requirement.question) - if requirement.question - else {}, + answer=transform_question_to_answer(getattr(requirement, 'question', None)) + if hasattr(requirement, 'question') and requirement.question + else {},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/models.py(2 hunks)backend/core/views.py(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/messages/fr.json
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
3455-3455: RequirementAssessment may be undefined, or defined from star imports
(F405)
3485-3485: RequirementAssessment may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
frontend/messages/en.json (1)
902-902: LGTM! The new localization entry follows existing conventions.The added entry maintains consistency with:
- Key naming convention (camelCase with "SemiColon" suffix)
- Value format (capitalization and colon usage)
- JSON structure
backend/core/models.py (1)
2782-2853: Verify the impact of bulk operations on model signals and hooks.The use of
bulk_createandbulk_updatemight bypass model signals, save() methods, and validation. Ensure this doesn't affect any required business logic or data integrity checks.✅ Verification successful
Bulk operations are safe to use in this context
The verification confirms that RequirementAssessment model has no custom save methods, model-level validation, or signal handlers that could be bypassed. The only validation present is in the serializer for API input validation, which is not relevant for bulk operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for model signals, save methods, and validation in RequirementAssessment model # Look for signal handlers rg -A 5 "@receiver.*pre_save.*RequirementAssessment|@receiver.*post_save.*RequirementAssessment" # Look for save method overrides ast-grep --pattern 'class RequirementAssessment { $$$ def save($$$) { $$$ } $$$ }' # Look for validation methods ast-grep --pattern 'class RequirementAssessment { $$$ def clean($$$) { $$$ } $$$ }'Length of output: 293
Script:
#!/bin/bash # First, find the RequirementAssessment model definition echo "=== Looking for RequirementAssessment model definition ===" rg -l "class RequirementAssessment.*Model):" # Look for any signal connections related to RequirementAssessment echo -e "\n=== Looking for signal connections ===" rg -l "RequirementAssessment.*signals\.|@receiver.*RequirementAssessment" # Search for validation methods in the model echo -e "\n=== Looking for validation methods ===" rg "def clean|def validate|def save" $(rg -l "class RequirementAssessment.*Model):") # Look for any signal connections in apps.py files echo -e "\n=== Checking apps.py files for signals ===" fd "apps.py" -x rg "signals\.|connect" {}Length of output: 3641
Script:
#!/bin/bash # Find the RequirementAssessment model file echo "=== Looking for RequirementAssessment model ===" rg -l "class RequirementAssessment" # Once we find the file, look for any validation or signal-related code echo -e "\n=== Looking for validation and signals in the model file ===" rg "class RequirementAssessment" -A 50 # Look for any signal imports or connections echo -e "\n=== Looking for signal-related imports ===" rg "from django.db.models.signals|from django.dispatch"Length of output: 17135
backend/core/views.py (4)
3420-3426: LGTM! Clean parameter extraction.The code follows good practices by clearly extracting and handling optional parameters before the main logic.
3427-3430: LGTM! Proper transaction management.The code correctly uses atomic transactions to ensure data consistency during instance creation and requirement assessment creation.
3431-3491: LGTM! Excellent performance optimizations.The code implements several performance improvements:
- Uses
select_relatedfor efficient mapping set retrieval- Prefetches related data to avoid N+1 queries
- Implements bulk operations for updates and M2M relationships
- Efficiently handles requirement assessment results computation
🧰 Tools
🪛 Ruff (0.8.2)
3455-3455:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
3485-3485:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
3492-3501: LGTM! Efficient applied controls creation.The code efficiently handles applied controls creation by:
- Prefetching control suggestions to avoid N+1 queries
- Creating controls only when explicitly requested
| # Fetch all requirements in a single query | ||
| requirements = RequirementNode.objects.filter( | ||
| framework=self.framework | ||
| ).select_related() | ||
|
|
||
| # If there's a baseline, prefetch all related baseline assessments in one query | ||
| baseline_assessments = {} | ||
| if baseline and baseline.framework == self.framework: | ||
| baseline_assessments = { | ||
| ra.requirement_id: ra | ||
| for ra in RequirementAssessment.objects.filter( | ||
| compliance_assessment=baseline, requirement__in=requirements | ||
| ).prefetch_related("evidences", "applied_controls") | ||
| } | ||
|
|
||
| # Create all RequirementAssessment objects in bulk | ||
| requirement_assessments = [ | ||
| RequirementAssessment( | ||
| compliance_assessment=self, | ||
| requirement=requirement, | ||
| folder=Folder.objects.get(id=self.folder.id), | ||
| folder_id=self.folder.id, # Use foreign key directly | ||
| answer=transform_question_to_answer(requirement.question) | ||
| if requirement.question | ||
| else {}, | ||
| ) | ||
| if baseline and baseline.framework == self.framework: | ||
| baseline_requirement_assessment = RequirementAssessment.objects.get( | ||
| compliance_assessment=baseline, requirement=requirement | ||
| ) | ||
| requirement_assessment.result = baseline_requirement_assessment.result | ||
| requirement_assessment.status = baseline_requirement_assessment.status | ||
| requirement_assessment.score = baseline_requirement_assessment.score | ||
| requirement_assessment.is_scored = ( | ||
| baseline_requirement_assessment.is_scored | ||
| ) | ||
| requirement_assessment.observation = ( | ||
| baseline_requirement_assessment.observation | ||
| ) | ||
| requirement_assessment.evidences.set( | ||
| baseline_requirement_assessment.evidences.all() | ||
| for requirement in requirements | ||
| ] | ||
|
|
||
| # Bulk create all assessments | ||
| created_assessments = RequirementAssessment.objects.bulk_create( | ||
| requirement_assessments | ||
| ) | ||
|
|
||
| # If there's a baseline, update the created assessments with baseline data | ||
| if baseline_assessments: | ||
| updates = [] | ||
| m2m_operations = [] | ||
|
|
||
| for assessment in created_assessments: | ||
| baseline_assessment = baseline_assessments.get( | ||
| assessment.requirement_id | ||
| ) | ||
| requirement_assessment.applied_controls.set( | ||
| baseline_requirement_assessment.applied_controls.all() | ||
| if baseline_assessment: | ||
| # Update scalar fields | ||
| assessment.result = baseline_assessment.result | ||
| assessment.status = baseline_assessment.status | ||
| assessment.score = baseline_assessment.score | ||
| assessment.is_scored = baseline_assessment.is_scored | ||
| assessment.observation = baseline_assessment.observation | ||
| updates.append(assessment) | ||
|
|
||
| # Store M2M operations for later | ||
| m2m_operations.append( | ||
| ( | ||
| assessment, | ||
| baseline_assessment.evidences.all(), | ||
| baseline_assessment.applied_controls.all(), | ||
| ) | ||
| ) | ||
|
|
||
| # Bulk update scalar fields | ||
| if updates: | ||
| RequirementAssessment.objects.bulk_update( | ||
| updates, ["result", "status", "score", "is_scored", "observation"] | ||
| ) | ||
| requirement_assessment.save() | ||
| requirement_assessments.append(requirement_assessment) | ||
| return requirement_assessments | ||
|
|
||
| # Handle M2M relationships | ||
| for assessment, evidences, controls in m2m_operations: | ||
| assessment.evidences.set(evidences) | ||
| assessment.applied_controls.set(controls) | ||
|
|
||
| return created_assessments |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling, type hints, and documentation.
The method implementation needs several improvements for robustness and maintainability:
- Add type hints and return value annotation
- Add comprehensive docstring
- Add error handling for database operations
- Add validation for the baseline parameter
- Add transaction management
- def create_requirement_assessments(
- self, baseline: Self | None = None
- ) -> list["RequirementAssessment"]:
+ def create_requirement_assessments(
+ self, baseline: Self | None = None
+ ) -> list["RequirementAssessment"]:
+ """Create requirement assessments for all requirements in the framework.
+
+ Args:
+ baseline: Optional baseline compliance assessment to copy data from.
+ Must have the same framework as self.
+
+ Returns:
+ List of created RequirementAssessment objects.
+
+ Raises:
+ ValidationError: If baseline is provided but has a different framework.
+ ValueError: If self.folder is None.
+ """
+ if baseline and baseline.framework != self.framework:
+ raise ValidationError("Baseline must have the same framework")
+
+ if not self.folder:
+ raise ValueError("ComplianceAssessment must have a folder")
+
+ try:
+ with transaction.atomic():
# Fetch all requirements in a single query
requirements = RequirementNode.objects.filter(
framework=self.framework
).select_related()
# If there's a baseline, prefetch all related baseline assessments
baseline_assessments = {}
if baseline and baseline.framework == self.framework:
baseline_assessments = {
ra.requirement_id: ra
for ra in RequirementAssessment.objects.filter(
compliance_assessment=baseline,
requirement__in=requirements
).prefetch_related("evidences", "applied_controls")
}
# Create all RequirementAssessment objects in bulk
requirement_assessments = [
RequirementAssessment(
compliance_assessment=self,
requirement=requirement,
folder_id=self.folder.id,
answer=transform_question_to_answer(requirement.question)
if requirement.question
else {},
)
for requirement in requirements
]
# Bulk create all assessments
created_assessments = RequirementAssessment.objects.bulk_create(
requirement_assessments
)
# If there's a baseline, update the created assessments
if baseline_assessments:
updates = []
m2m_operations = []
for assessment in created_assessments:
baseline_assessment = baseline_assessments.get(
assessment.requirement_id
)
if baseline_assessment:
# Update scalar fields
assessment.result = baseline_assessment.result
assessment.status = baseline_assessment.status
assessment.score = baseline_assessment.score
assessment.is_scored = baseline_assessment.is_scored
assessment.observation = baseline_assessment.observation
updates.append(assessment)
# Store M2M operations for later
m2m_operations.append(
(
assessment,
baseline_assessment.evidences.all(),
baseline_assessment.applied_controls.all(),
)
)
# Bulk update scalar fields
if updates:
RequirementAssessment.objects.bulk_update(
updates,
["result", "status", "score", "is_scored", "observation"]
)
# Handle M2M relationships
for assessment, evidences, controls in m2m_operations:
assessment.evidences.set(evidences)
assessment.applied_controls.set(controls)
return created_assessments
+ except Exception as e:
+ logger.error(
+ "Failed to create requirement assessments",
+ error=str(e),
+ compliance_assessment_id=self.id,
+ baseline_id=baseline.id if baseline else None,
+ )
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Fetch all requirements in a single query | |
| requirements = RequirementNode.objects.filter( | |
| framework=self.framework | |
| ).select_related() | |
| # If there's a baseline, prefetch all related baseline assessments in one query | |
| baseline_assessments = {} | |
| if baseline and baseline.framework == self.framework: | |
| baseline_assessments = { | |
| ra.requirement_id: ra | |
| for ra in RequirementAssessment.objects.filter( | |
| compliance_assessment=baseline, requirement__in=requirements | |
| ).prefetch_related("evidences", "applied_controls") | |
| } | |
| # Create all RequirementAssessment objects in bulk | |
| requirement_assessments = [ | |
| RequirementAssessment( | |
| compliance_assessment=self, | |
| requirement=requirement, | |
| folder=Folder.objects.get(id=self.folder.id), | |
| folder_id=self.folder.id, # Use foreign key directly | |
| answer=transform_question_to_answer(requirement.question) | |
| if requirement.question | |
| else {}, | |
| ) | |
| if baseline and baseline.framework == self.framework: | |
| baseline_requirement_assessment = RequirementAssessment.objects.get( | |
| compliance_assessment=baseline, requirement=requirement | |
| ) | |
| requirement_assessment.result = baseline_requirement_assessment.result | |
| requirement_assessment.status = baseline_requirement_assessment.status | |
| requirement_assessment.score = baseline_requirement_assessment.score | |
| requirement_assessment.is_scored = ( | |
| baseline_requirement_assessment.is_scored | |
| ) | |
| requirement_assessment.observation = ( | |
| baseline_requirement_assessment.observation | |
| ) | |
| requirement_assessment.evidences.set( | |
| baseline_requirement_assessment.evidences.all() | |
| for requirement in requirements | |
| ] | |
| # Bulk create all assessments | |
| created_assessments = RequirementAssessment.objects.bulk_create( | |
| requirement_assessments | |
| ) | |
| # If there's a baseline, update the created assessments with baseline data | |
| if baseline_assessments: | |
| updates = [] | |
| m2m_operations = [] | |
| for assessment in created_assessments: | |
| baseline_assessment = baseline_assessments.get( | |
| assessment.requirement_id | |
| ) | |
| requirement_assessment.applied_controls.set( | |
| baseline_requirement_assessment.applied_controls.all() | |
| if baseline_assessment: | |
| # Update scalar fields | |
| assessment.result = baseline_assessment.result | |
| assessment.status = baseline_assessment.status | |
| assessment.score = baseline_assessment.score | |
| assessment.is_scored = baseline_assessment.is_scored | |
| assessment.observation = baseline_assessment.observation | |
| updates.append(assessment) | |
| # Store M2M operations for later | |
| m2m_operations.append( | |
| ( | |
| assessment, | |
| baseline_assessment.evidences.all(), | |
| baseline_assessment.applied_controls.all(), | |
| ) | |
| ) | |
| # Bulk update scalar fields | |
| if updates: | |
| RequirementAssessment.objects.bulk_update( | |
| updates, ["result", "status", "score", "is_scored", "observation"] | |
| ) | |
| requirement_assessment.save() | |
| requirement_assessments.append(requirement_assessment) | |
| return requirement_assessments | |
| # Handle M2M relationships | |
| for assessment, evidences, controls in m2m_operations: | |
| assessment.evidences.set(evidences) | |
| assessment.applied_controls.set(controls) | |
| return created_assessments | |
| def create_requirement_assessments( | |
| self, baseline: Self | None = None | |
| ) -> list["RequirementAssessment"]: | |
| """Create requirement assessments for all requirements in the framework. | |
| Args: | |
| baseline: Optional baseline compliance assessment to copy data from. | |
| Must have the same framework as self. | |
| Returns: | |
| List of created RequirementAssessment objects. | |
| Raises: | |
| ValidationError: If baseline is provided but has a different framework. | |
| ValueError: If self.folder is None. | |
| """ | |
| if baseline and baseline.framework != self.framework: | |
| raise ValidationError("Baseline must have the same framework") | |
| if not self.folder: | |
| raise ValueError("ComplianceAssessment must have a folder") | |
| try: | |
| with transaction.atomic(): | |
| # Fetch all requirements in a single query | |
| requirements = RequirementNode.objects.filter( | |
| framework=self.framework | |
| ).select_related() | |
| # If there's a baseline, prefetch all related baseline assessments | |
| baseline_assessments = {} | |
| if baseline and baseline.framework == self.framework: | |
| baseline_assessments = { | |
| ra.requirement_id: ra | |
| for ra in RequirementAssessment.objects.filter( | |
| compliance_assessment=baseline, | |
| requirement__in=requirements | |
| ).prefetch_related("evidences", "applied_controls") | |
| } | |
| # Create all RequirementAssessment objects in bulk | |
| requirement_assessments = [ | |
| RequirementAssessment( | |
| compliance_assessment=self, | |
| requirement=requirement, | |
| folder_id=self.folder.id, | |
| answer=transform_question_to_answer(requirement.question) | |
| if requirement.question | |
| else {}, | |
| ) | |
| for requirement in requirements | |
| ] | |
| # Bulk create all assessments | |
| created_assessments = RequirementAssessment.objects.bulk_create( | |
| requirement_assessments | |
| ) | |
| # If there's a baseline, update the created assessments | |
| if baseline_assessments: | |
| updates = [] | |
| m2m_operations = [] | |
| for assessment in created_assessments: | |
| baseline_assessment = baseline_assessments.get( | |
| assessment.requirement_id | |
| ) | |
| if baseline_assessment: | |
| # Update scalar fields | |
| assessment.result = baseline_assessment.result | |
| assessment.status = baseline_assessment.status | |
| assessment.score = baseline_assessment.score | |
| assessment.is_scored = baseline_assessment.is_scored | |
| assessment.observation = baseline_assessment.observation | |
| updates.append(assessment) | |
| # Store M2M operations for later | |
| m2m_operations.append( | |
| ( | |
| assessment, | |
| baseline_assessment.evidences.all(), | |
| baseline_assessment.applied_controls.all(), | |
| ) | |
| ) | |
| # Bulk update scalar fields | |
| if updates: | |
| RequirementAssessment.objects.bulk_update( | |
| updates, | |
| ["result", "status", "score", "is_scored", "observation"] | |
| ) | |
| # Handle M2M relationships | |
| for assessment, evidences, controls in m2m_operations: | |
| assessment.evidences.set(evidences) | |
| assessment.applied_controls.set(controls) | |
| return created_assessments | |
| except Exception as e: | |
| logger.error( | |
| "Failed to create requirement assessments", | |
| error=str(e), | |
| compliance_assessment_id=self.id, | |
| baseline_id=baseline.id if baseline else None, | |
| ) | |
| raise |
Mohamed-Hacene
force-pushed
the
fix/improve-perf
branch
from
c2f64c3 to
9112115
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/core/models.py (2)
2782-2795: Great optimization of database queries!The implementation efficiently handles database operations by:
- Using
select_related()to avoid N+1 queries- Prefetching baseline assessments in a single query
- Using dictionary for O(1) lookups
Consider these improvements:
- Add validation for baseline framework compatibility
- Wrap operations in a transaction for atomicity
+ @transaction.atomic def create_requirement_assessments( self, baseline: Self | None = None ) -> list["RequirementAssessment"]: + if baseline and baseline.framework != self.framework: + raise ValidationError("Baseline must have the same framework") + # Fetch all requirements in a single query requirements = RequirementNode.objects.filter( framework=self.framework ).select_related()
2815-2852: Excellent optimization of baseline data handling!The code efficiently handles baseline data by:
- Separating scalar and M2M updates
- Using bulk update for scalar fields
- Managing M2M relationships appropriately
Consider adding error handling and logging:
# If there's a baseline, update the created assessments with baseline data if baseline_assessments: + logger = get_logger(__name__) updates = [] m2m_operations = [] for assessment in created_assessments: baseline_assessment = baseline_assessments.get( assessment.requirement_id ) if baseline_assessment: # Update scalar fields assessment.result = baseline_assessment.result assessment.status = baseline_assessment.status assessment.score = baseline_assessment.score assessment.is_scored = baseline_assessment.is_scored assessment.observation = baseline_assessment.observation updates.append(assessment) # Store M2M operations for later m2m_operations.append( ( assessment, baseline_assessment.evidences.all(), baseline_assessment.applied_controls.all(), ) ) # Bulk update scalar fields if updates: RequirementAssessment.objects.bulk_update( updates, ["result", "status", "score", "is_scored", "observation"] ) # Handle M2M relationships for assessment, evidences, controls in m2m_operations: + try: assessment.evidences.set(evidences) assessment.applied_controls.set(controls) + except Exception as e: + logger.error( + "Failed to set M2M relationships", + error=str(e), + assessment_id=assessment.id, + )backend/core/views.py (2)
3440-3440: Remove empty comment line.This empty comment line can be removed to improve code cleanliness.
-
3497-3497: Remove empty line.This empty line can be removed to improve code cleanliness.
-
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/models.py(2 hunks)backend/core/views.py(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/messages/fr.json
- frontend/messages/en.json
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
backend/core/models.py (1)
2797-2813: Efficient bulk creation implementation!The code effectively minimizes database operations by:
- Using list comprehension for preparation
- Leveraging
bulk_createfor insertion- Using direct foreign key assignment
However, consider adding folder validation:
# Create all RequirementAssessment objects in bulk + if not self.folder: + raise ValueError("ComplianceAssessment must have a folder") + requirement_assessments = [ RequirementAssessment( compliance_assessment=self, requirement=requirement, folder_id=self.folder.id, answer=transform_question_to_answer(requirement.question) if requirement.question else {}, ) for requirement in requirements ]backend/core/views.py (3)
3427-3430: LGTM! Good use of transaction management.The transaction block ensures atomic operations during compliance assessment creation, preventing partial data in case of failures.
3431-3491: LGTM! Excellent performance optimizations.The code efficiently handles different framework cases with several performance improvements:
- Uses
select_related()for efficient mapping set retrieval- Performs bulk updates for observations
- Handles many-to-many relationships in bulk
3492-3501: LGTM! Good use of prefetching for applied controls creation.The code efficiently handles applied controls creation with performance optimization:
- Uses
prefetch_related()to efficiently load requirement assessments and their suggestions in a single query
Summary by CodeRabbit
Release Notes
Performance Improvements
Localization
Backend Enhancements