fix: fix issues when using pagination together with HasRetvalPerm

[bellini666]

Fix #471

[sourcery-ai]

PR Type: Bug fix

PR Summary: The pull request addresses an issue with filtering QuerySets after pagination has been applied, which is not natively supported by Django. It introduces a workaround by temporarily clearing the pagination limits, applying the filter, and then restoring the original pagination limits. Additionally, the PR includes changes to tests to ensure compatibility with the new pagination feature and to handle permission checks more robustly.

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.

📝 Complexity: the changes are too large or complex for Sourcery to approve.

• Unsupported files: the diff contains files that Sourcery does not currently support during reviews.

General suggestions:

• Consider adding more detailed comments explaining the rationale behind the workaround for future maintainability. While the current comments describe what is being done, explaining why it's necessary and the specific problem it solves could be beneficial.
• Review the added tests to ensure they comprehensively cover the new functionality and any potential edge cases that could arise from the changes to the QuerySet filtering logic.
• Given the direct manipulation of QuerySet's internal state, it might be worth exploring if there are alternative approaches that achieve the same goal without needing to adjust low_mark and high_mark directly, to minimize potential side effects.

strawberry_django/utils/query.py: Consider simplifying by cloning the original `QuerySet` and reapplying slicing, avoiding direct manipulation of internal state for better maintainability.

While the intention behind the changes is clear and addresses a specific issue with Django's QuerySet slicing, the approach introduces a higher level of complexity and direct manipulation of QuerySet internals. This can make the code harder to maintain and understand, especially for developers unfamiliar with the intricacies of Django's QuerySet behavior. Additionally, directly altering QuerySet internal state (like low_mark and high_mark) could lead to brittle code that might break with future Django updates.

A less complex approach could involve cloning the original QuerySet to preserve its state, applying the necessary filters, and then reapplying the original slicing if it was present. This method avoids directly manipulating internal states and keeps the code more maintainable. Here's a suggested revision:

def filter_for_user(
    qs: QuerySet,
    user: UserType,
    perms: TypeOrIterable[str],
    *,
    any_perm: bool = True,
    with_groups: bool = True,
    with_superuser: bool = False,
):
    # Clone the original queryset to avoid altering its state
    cloned_qs = qs.all()

    # Apply the filter
    filtered_qs = cloned_qs.filter(
        filter_for_user_q(
            cloned_qs,
            user,
            perms,
            any_perm=any_perm,
            with_groups=with_groups,
            with_superuser=with_superuser,
        ),
    )

    # If the original queryset had slicing applied, reapply it to the filtered queryset
    if qs.query.low_mark or qs.query.high_mark:
        filtered_qs = filtered_qs[qs.query.low_mark:qs.query.high_mark]

    return filtered_qs

This approach maintains the original functionality while reducing complexity and improving maintainability.

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

• X
• Mastodon
• LinkedIn
• Facebook

---

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

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (521397c) 87.90% compared to head (1ccf906) 87.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
+ Coverage   87.90%   87.92%   +0.02%     
==========================================
  Files          37       37              
  Lines        3167     3173       +6     
==========================================
+ Hits         2784     2790       +6     
  Misses        383      383              

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

[bellini666]

Just noticed there's a PR fixing the same issue: #480

Closing this as I liked the approach there even more