-
-
Notifications
You must be signed in to change notification settings - Fork 125
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
refactor: Remove guardian ObjectPermissionChecker monkey patch #631
Conversation
This is an old monkey patch that was introduced back when this code used to live in `strawberry-django-plus`, with the intention of avoiding too many queries to the database when fetching permissions. This ended up causing an issue, as checking for a permission would cache the result, and later when trying to modify the permissions and checking those again would return the cached result instead. This seems like to not be required anymore as well, based on the fact that we have a lot of unit tests that test the permissioning stuff with guardian, while also ensuring that the number of db requests are known, and none presented any issues.
Reviewer's Guide by SourceryThis pull request removes a legacy monkey patch for the ObjectPermissionChecker in the guardian integration. The patch was originally implemented to reduce database queries when fetching permissions, but it caused issues with caching and is no longer necessary due to extensive unit testing of the permissioning system. File-Level Changes
Sequence DiagramNo sequence diagram generated. Tips
|
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.
Hey @bellini666 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Could you provide some benchmark results or performance data to confirm that removing this optimization doesn't significantly increase the number of database queries in typical use cases?
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
==========================================
- Coverage 88.79% 88.75% -0.05%
==========================================
Files 41 41
Lines 3605 3592 -13
==========================================
- Hits 3201 3188 -13
Misses 404 404
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This is an old monkey patch that was introduced back when this code used to live in
strawberry-django-plus
, with the intention of avoiding too many queries to the database when fetching permissions.This ended up causing an issue, as checking for a permission would cache the result, and later when trying to modify the permissions and checking those again would return the cached result instead.
This seems like to not be required anymore as well, based on the fact that we have a lot of unit tests that test the permissioning stuff with guardian, while also ensuring that the number of db requests are known, and none presented any issues.
Summary by Sourcery
Refactor the code to remove the custom monkey patch for ObjectPermissionChecker, which was causing caching issues with permission checks. This change relies on existing unit tests to ensure that permission handling remains efficient and correct without the patch.
Enhancements: