fix: resolve slow DB query in org permission check (#8228) - #8230
fix: resolve slow DB query in org permission check (#8228)#8230srijantrpth wants to merge 18 commits into
Conversation
- Rewrote `user_has_organisation_permission` to evaluate user, group, and role permissions using sequential `EXISTS` checks. - This prevents Django from building a massive `LEFT OUTER JOIN` cross-product of the entire RBAC graph, eliminating the ~2.6s loading delay on the `/groups/` endpoint. - Added `test_user_has_organisation_permission_query_count` to prevent future regressions.
|
@srijantrpth is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
for more information, see https://pre-commit.ci
|
To follow up on the Sentry report, this fix drastically reduces the database load for the /groups/ endpoint. Before: After: Best case scenario: If a user has direct permissions, the DB stops looking immediately (only 2 fast queries). Worst case scenario: The DB executes a maximum of 4 isolated, indexed queries instead of one massive join. (This is now strictly enforced in the test suite via django_assert_max_num_queries(4)). |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 26aeea1b-9216-4d14-a694-c8e66ce01c8f
📒 Files selected for processing (2)
api/permissions/permission_service.pyapi/tests/unit/users/test_unit_users_models.py
There was a problem hiding this comment.
Pull request overview
This PR addresses the performance regression in organisation-level permission checks by replacing the single, join-heavy ORM query with a sequence of short-circuiting EXISTS checks, reducing the risk of RBAC join cross-products that were slowing /organisations/{id}/groups/.
Changes:
- Reworked
user_has_organisation_permissionto perform sequentialEXISTSchecks for direct user, group, and (optionally) role-based permissions. - Added a unit test to cap the number of queries executed by the organisation permission check to help prevent query-count regressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
api/permissions/permission_service.py |
Rewrites organisation permission checking logic to avoid a single large join and short-circuit via multiple EXISTS queries. |
api/tests/unit/users/test_unit_users_models.py |
Adds a query-count regression test for user_has_organisation_permission. |
Suppressed comments (2)
api/tests/unit/users/test_unit_users_models.py:282
- There is trailing whitespace in the argument list, and the permission key should use the
MANAGE_USER_GROUPSconstant (to avoid typos and keep it aligned with the rest of the codebase).
# (1 base check + up to 3 for user/group/role filters)
# If a join explosion is reintroduced, this will fail because the query
# complexity and count will change drastically.
with django_assert_max_num_queries(4):
has_permission = user_has_organisation_permission(
api/tests/unit/users/test_unit_users_models.py:8
- The imports include two separate
from users.models ...statements and the new test hard-codes the permission key string. This is likely to fail import-sorting checks and makes the test less robust than using the existingMANAGE_USER_GROUPSconstant.
This issue also appears on line 278 of the same file.
from common.projects.permissions import VIEW_PROJECT
from django.db.utils import IntegrityError
from organisations.models import Organisation, OrganisationRole
from organisations.permissions.models import UserOrganisationPermission
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @srijantrpth , thanks for the contribution. Could you take a look at the CI failures please? It looks like there are some new typing errors added here? You can ignore the 'API Documentation Artefacts' check - it looks like there's something we need to resolve there - and the Vercel Previews. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01df65b8-13dc-4a6d-8a87-e939cc873083
📒 Files selected for processing (2)
api/permissions/permission_service.pyapi/tests/unit/users/test_unit_users_models.py
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/permissions/permission_service.py (1)
220-226:⚠️ Potential issue | 🟠 MajorMake the model-class typing exception explicit to Mypy.
Line 224 passes the
Organisationmodel class, but the inline note states thatget_role_permission_filterexpects an instance. The note does not change the static type. If the helper annotation is still instance-only, Mypy will report an argument-type error.Update the helper annotation to accept model classes, or add a precise
# type: ignore[arg-type]with the runtime reason. Do not use an unqualified suppression.Based on learnings, API typing workarounds should use a precise, documented, removable suppression or an accurate
typing.cast(...).#!/bin/bash set -euo pipefail rg -n -A16 -B3 'def get_role_permission_filter\(' api/permissions/permission_service.py rg -n -C4 'get_role_permission_filter\(' api/permissions api/tests if command -v mypy >/dev/null 2>&1; then mypy --show-error-codes api/permissions/permission_service.py else echo "mypy is not installed" fiSource: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f8b0595-8df6-4a72-9fd7-46ff2d911a39
📒 Files selected for processing (2)
api/permissions/permission_service.pyapi/tests/unit/users/test_unit_users_models.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/tests/unit/users/test_unit_users_models.py (1)
278-282: 🚀 Performance & Scalability | 🟡 MinorAssert the query shape as well as the query count.
django_assert_max_num_queries(4)does not detect a regression to the RBACLEFT OUTER JOINcross-product. That regression could still execute as one query and remain below the limit. Replace this with exact per-configuration query assertions and add a SQL-shape or query-plan check that rejects the expensive join pattern. This is the same unresolved gap identified in the previous review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65c17559-e9e2-4cd5-83a3-1b5f77ee42e5
📒 Files selected for processing (1)
api/tests/unit/users/test_unit_users_models.py
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8230 +/- ##
==========================================
- Coverage 98.65% 98.57% -0.09%
==========================================
Files 1514 1543 +29
Lines 60110 61699 +1589
==========================================
+ Hits 59304 60819 +1515
- Misses 806 880 +74 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @matthewelwell , all fixed! I've resolved the mypy typing errors and the test linting issue. CI should be green now (ignoring the docs/Vercel checks as requested). Let me know if it looks good to go! |
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #8228
user_has_organisation_permissionto evaluate user, group, and role permissions using sequentialEXISTSchecks.LEFT OUTER JOINcross-product of the entire RBAC graph, eliminating the ~2.6s loading delay on the/groups/endpoint.How did you test this code?
organisationsanduserstest suites to guarantee functional parity (the rewritten ORM returns the exact same boolean logic as the old version).test_user_has_organisation_permission_query_countintests/unit/users/test_unit_users_models.pywrapped indjango_assert_max_num_queries(4)to prevent future query-count regressions.