Skip to content
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

Fix permissioned pagination #480

Merged
merged 4 commits into from
Feb 17, 2024

Conversation

vecchp
Copy link
Contributor

@vecchp vecchp commented Feb 16, 2024

Fixes an error when trying to apply permission checks using the HasRetvalPerm extension on a paginated list.

TypeError: Cannot filter a query once a slice has been taken.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Fix #471

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Type: Bug fix

PR Summary: This pull request addresses a TypeError encountered when applying permission checks on a paginated queryset using the HasRetvalPerm extension. The core of the fix involves modifying the order in which filter_with_perms is applied in relation to super().get_queryset, aiming to resolve the issue without breaking existing functionality. Additionally, the PR extends test coverage to include scenarios that validate the correct behavior of permissioned pagination.

Decision: Comment

📝 Type: 'Bug fix' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.

General suggestions:

  • Ensure comprehensive testing around the modified order of filter_with_perms and super().get_queryset to catch any potential edge cases that could arise from this change.
  • Consider adding more detailed comments in the code changes to explain the reasoning behind the order modification for future maintainers.
  • Review the new test cases added for paginated queries with permissions to ensure they cover a wide range of scenarios, including edge cases not immediately apparent.

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? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@vecchp vecchp closed this Feb 16, 2024
@vecchp vecchp reopened this Feb 16, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (94830a1) 87.90% compared to head (ccb8d77) 87.90%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #480   +/-   ##
=======================================
  Coverage   87.90%   87.90%           
=======================================
  Files          37       37           
  Lines        3167     3167           
=======================================
  Hits         2784     2784           
  Misses        383      383           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bellini666 bellini666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! :)

@bellini666 bellini666 merged commit 083d1a3 into strawberry-graphql:main Feb 17, 2024
54 checks passed
@vecchp vecchp deleted the fix-perms-with-pagination branch February 17, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination + HasRetValPerm fails
3 participants