perf: query set optimization #1491
Conversation
…ated inside requirement assessment
- optimize get_accessible_object_ids by first calculating the folders that match, and only then retrieving (optimally) the objects from these folders. This should be way more scalable. - use scope_folder_id parameter to scope the retrieved objects
WalkthroughThe pull request enhances both backend and frontend components to support folder-based scoping. In the backend, the Changes
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Nitpick comments (4)
backend/iam/models.py (3)
692-692: Be mindful of performance when collecting large folder trees.
Callingfolder.get_sub_folders()and instantly converting it to a set is straightforward, but for heavily nested structures, repeated conversions may become expensive. You might consider caching or a more efficient traversal if scale becomes a concern.
694-698: Optimize role assignment lookups.
Repeatedly callingra.role.permissions.all()in this list comprehension can result in multiple database queries. Consider using prefetching (e.g.,prefetch_related('role__permissions')) to minimize query overhead.
700-711: Avoid repeated subtree expansions in recursive folder lookups.
Within the loop, callingfolder.get_sub_folders()for each folder inra_perimetermay cause repeated traversals if the same folders appear in multiple role assignments. You could unify the list of all required subfolders once per assignment to improve performance.backend/core/views.py (1)
135-161: Add validation for scope_folder_id and user's access to the folder.While the implementation of folder-based scoping is good, consider adding:
- Validation that the user has access to the specified scope folder
- Error handling for invalid scope_folder_id values (non-UUID format)
def get_queryset(self): """the scope_folder_id query_param allows scoping the objects to retrieve""" if not self.model: return None object_ids_view = None if self.request.method == "GET": if q := re.match( r"/api/[\w-]+/([\w-]+/)?([0-9a-fA-F]{8}(-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12}(,[0-9a-fA-F]{8}(-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12})+)", self.request.path, ): """"get_queryset is called by Django even for an individual object via get_object https://stackoverflow.com/questions/74048193/why-does-a-retrieve-request-end-up-calling-get-queryset""" id = UUID(q.group(1)) if RoleAssignment.is_object_readable(self.request.user, self.model, id): object_ids_view = [id] if not object_ids_view: scope_folder_id = self.request.query_params.get("scope_folder_id") + if scope_folder_id: + try: + scope_folder_id = UUID(scope_folder_id) + except ValueError: + raise ValidationError({"scope_folder_id": "Invalid UUID format"}) scope_folder = ( get_object_or_404(Folder, id=scope_folder_id) if scope_folder_id else Folder.get_root_folder() ) + if scope_folder_id and not RoleAssignment.is_access_allowed( + user=self.request.user, + perm=Permission.objects.get(codename=f"view_{self.model._meta.model_name}"), + folder=scope_folder + ): + raise PermissionDenied("User does not have access to the specified folder") object_ids_view = RoleAssignment.get_accessible_object_ids( scope_folder, self.request.user, self.model )[0] queryset = self.model.objects.filter(id__in=object_ids_view) return queryset🧰 Tools
🪛 Ruff (0.8.2)
140-144: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/views.py(3 hunks)backend/iam/models.py(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte(4 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: ruff (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
backend/iam/models.py (3)
683-687: Consider handling missing permissions gracefully.
If a corresponding permission record doesn't exist in the database,Permission.objects.get()will raise aPermission.DoesNotExisterror. Handling or logging this case might prevent unexpected runtime errors.
688-689: Initialization of result sets looks good.
No issues with creating separate sets for each permission level.
741-745: Confirm the publishing logic for folder-scoped objects.
This publishing logic only executes if the object has bothis_publishedandfolderattributes. Ensure that objects requiring limited visibility are properly handled, and consider unit tests for corner cases where objects may or may not have these attributes.frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte (5)
175-177: Parameter passing for scope_folder_id looks valid.
Ensure that this parameter is handled and verified on the backend side to prevent potential injection or empty-value issues.
185-187: Repeated logic for scope_folder_id parameter.
The addition closely mirrors the pattern on lines 175–177. Maintaining consistency is good; just verify that the folder ID is present and correct in every usage scenario.
197-199: Consistent approach with scope_folder_id for vulnerabilities.
Same note about verifying the backend acceptance of this parameter.
219-221: Extended parameter usage for existing applied controls.
Again, ensure the backend properly interprets'scope_folder_id'in this context.
295-297: Ensuring param for extra applied controls as well.
No functional concerns; just confirm that any new logic fully supports scoping in every relevant endpoint.frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (2)
413-415: Ensure backend alignment with scope_folder_id for applied controls.
Passing the folder ID here is consistent with the pattern in other files. Verify that the backend is ready to handle this parameter for partial scoping if needed.
449-451: Include folder ID scoping for evidence retrieval.
As above, confirm that the backend receives and utilizes this parameter correctly for evidence queries.backend/core/views.py (1)
50-50: LGTM!The addition of
get_object_or_404is appropriate for handling folder retrieval in the updatedget_querysetmethod.
backend/iam/models.py
Outdated
| if hasattr(object_type, "folder"): | ||
| objects_ids = object_type.objects.filter(folder=f).values_list( | ||
| "id", flat=True | ||
| ) | ||
| elif hasattr(object_type, "risk_assessment"): | ||
| objects_ids = object_type.objects.filter( | ||
| risk_assessment__folder=f | ||
| ).values_list("id", flat=True) | ||
| elif hasattr(object_type, "entity"): | ||
| objects_ids = object_type.objects.filter(entity__folder=f).values_list( | ||
| "id", flat=True | ||
| ) | ||
| for p in [p for p in permissions if p in ra_permissions]: | ||
| if p == permissions[0]: | ||
| folders_with_local_view.add(my_folder) | ||
| for object in [ | ||
| x for x in all_objects if folder_for_object[x] in target_folders | ||
| ]: | ||
| # builtins objects cannot be edited or deleted | ||
| if not ( | ||
| hasattr(object, "builtin") | ||
| and object.builtin | ||
| and p != permissions[0] | ||
| ): | ||
| permissions_per_object_id[object.id].add(p) | ||
|
|
||
| if hasattr(object_type, "is_published"): | ||
| elif hasattr(object_type, "provider_entity"): | ||
| objects_ids = object_type.objects.filter( | ||
| provider_entity__folder=f | ||
| ).values_list("id", flat=True) | ||
| elif hasattr(object_type, "parent_folder"): | ||
| objects_ids = object_type.objects.filter(parent_folder=f).values_list( | ||
| "id", flat=True | ||
| ) | ||
| else: | ||
| raise NotImplementedError("type not supported") | ||
| if permission_view in result_folders[f]: | ||
| result_view.update(objects_ids) | ||
| if permission_change in result_folders[f]: | ||
| result_change.update(objects_ids) | ||
| if permission_delete in result_folders[f]: | ||
| result_delete.update(objects_ids) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the conditional blocks for folder-based object filtering.
These cascading hasattr checks can become unwieldy and error-prone, especially as new object types with different folder relationships emerge. Consider centralizing the mapping in a dictionary or leveraging a shared helper function to reduce repetition and improve maintainability.
special case of folder for get_accessible_object_ids
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/iam/models.py (1)
712-731: Refactor object type handling to reduce complexity.The cascading attribute checks make the code harder to maintain. Consider using a mapping or registry pattern.
- if hasattr(object_type, "folder"): - objects_ids = object_type.objects.filter(folder=f).values_list( - "id", flat=True - ) - elif hasattr(object_type, "risk_assessment"): - objects_ids = object_type.objects.filter( - risk_assessment__folder=f - ).values_list("id", flat=True) - elif hasattr(object_type, "entity"): - objects_ids = object_type.objects.filter(entity__folder=f).values_list( - "id", flat=True - ) - elif hasattr(object_type, "provider_entity"): - objects_ids = object_type.objects.filter( - provider_entity__folder=f - ).values_list("id", flat=True) - elif hasattr(object_type, "parent_folder"): - objects_ids = [f.id] - else: - raise NotImplementedError("type not supported") + FOLDER_PATHS = { + "folder": lambda obj, f: obj.objects.filter(folder=f), + "risk_assessment": lambda obj, f: obj.objects.filter(risk_assessment__folder=f), + "entity": lambda obj, f: obj.objects.filter(entity__folder=f), + "provider_entity": lambda obj, f: obj.objects.filter(provider_entity__folder=f), + "parent_folder": lambda obj, f: [f.id] + } + + for attr, query_func in FOLDER_PATHS.items(): + if hasattr(object_type, attr): + objects_ids = query_func(object_type, f).values_list("id", flat=True) if attr != "parent_folder" else query_func(object_type, f) + break + else: + raise NotImplementedError(f"Object type {object_type.__name__} does not support any known folder relationship")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/models.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: migrations-check (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
backend/iam/models.py (3)
683-689: LGTM! Efficient permission handling using sets.The change from list-based to set-based permission handling improves performance for membership testing operations.
692-711: LGTM! Efficient folder permission calculation.The new implementation optimizes performance by:
- Pre-calculating the folder perimeter
- Using defaultdict to group folders by permissions
- Reducing database queries through set operations
739-755: LGTM! Clear handling of published objects.The implementation correctly manages published objects visibility while maintaining good readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/iam/models.py (2)
704-706: Consider optimizing recursive folder updates.The current implementation might be expensive for deep folder hierarchies. Consider using a generator-based approach to reduce memory usage.
- ra_perimeter.update( - *[folder.get_sub_folders() for folder in ra_perimeter] - ) + for folder in ra_perimeter.copy(): + ra_perimeter.update(folder.get_sub_folders())
730-731: Enhance error handling for unsupported types.The current error message could be more informative. Consider including the actual type that's not supported.
- raise NotImplementedError("type not supported") + raise NotImplementedError( + f"Object type '{object_type.__name__}' is not supported. " + "The type must have one of these attributes: folder, risk_assessment, " + "entity, provider_entity, or parent_folder" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/models.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (3)
backend/iam/models.py (3)
683-689: LGTM! Improved permission handling initialization.The refactored code improves clarity and efficiency by:
- Using individual permission variables for better readability
- Using sets for efficient permission lookups and result storage
692-711: LGTM! Efficient folder permission calculation.The refactored code significantly improves performance by:
- Pre-calculating the perimeter once
- Using defaultdict to efficiently track permissions per folder
- Processing role assignments in a more streamlined manner
739-755: LGTM! Improved published objects handling.The refactored code efficiently handles published objects by:
- Checking for required attributes before processing
- Using list comprehension for better performance
- Properly scoping the published objects retrieval
Summary by CodeRabbit
New Features
Refactor