Fix _perm_cache processing
#498
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vecchp - I've reviewed your changes and they look great!
General suggestions:
- Ensure that the changes to
_perm_cacheprocessing do not inadvertently affect the expected behavior of permission checks. - Consider the impact of the changes on performance, especially in scenarios with a large number of permissions.
- Verify the compatibility of the direct iteration and comparison logic with the structure and content of
perm_cache.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| f = any if any_perm else all | ||
| user_perms: Set[str] = {p.codename for p in perm_cache} |
There was a problem hiding this comment.
issue (performance): The removal of the explicit set creation and type hint for user_perms simplifies the code. However, ensure that perm_cache directly supports efficient membership testing as a set does. If perm_cache is a list or another data structure with O(n) lookup time, this change could negatively impact performance, especially with a large number of permissions.
| user_perms: Set[str] = {p.codename for p in perm_cache} | |
| if isinstance(perm_cache, set) or isinstance(perm_cache, frozenset): | |
| if f(p in perm_cache for p in perms_list): | |
| pass # Your existing logic here | |
| else: | |
| perm_cache_set = set(perm_cache) # Convert to set for efficient lookup | |
| if f(p in perm_cache_set for p in perms_list): | |
| pass # Your existing logic here |
| f = any if any_perm else all | ||
| user_perms: Set[str] = {p.codename for p in perm_cache} |
There was a problem hiding this comment.
issue (bug_risk): This change assumes that perm_cache directly contains permission codenames or objects that can be compared to perms_list. It's crucial to verify that the structure and content of perm_cache are compatible with this direct iteration and comparison, to avoid introducing bugs related to permission checks.
| user_perms: Set[str] = {p.codename for p in perm_cache} | |
| if f(perm_cache.get(p, False) for p in perms_list): |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #498 +/- ##
==========================================
- Coverage 87.98% 87.97% -0.02%
==========================================
Files 37 37
Lines 3180 3184 +4
==========================================
+ Hits 2798 2801 +3
- Misses 382 383 +1 ☔ View full report in Codecov by Sentry. |
Description
It appears this line in
utils/query.pyis not checking cached permissions properly.In my codebase the
_perm_cachecomes up as a set of strings which then causes an exceptionLooking at the django codebase I think this line confirms that the
perm_cacheis indeed a set of strings. This seems to be the same behavior going back to django 0.90.Also a previous PR #428, seemed to have run into the same issue but for some reason it was closed.
Types of Changes
Issues Fixed or Closed by This PR
Checklist