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(optimizer): Avoid merging prefetches when using aliases #698

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

bellini666
Copy link
Member

@bellini666 bellini666 commented Jan 25, 2025

Merging querysets is usually fine, but when using an alias it might be that one queryset is filtering for something, and the other is filtering for something else, for the same field. That can lead to wrong results being returned.

From now on, if a field is specified more than once with an alias, the optimizer will skip it, possibly causing n+1 issues, but avoiding wrong results (which is worse).

Fix #695

Summary by Sourcery

Fix issue where using aliases with the same field could lead to incorrect results. Skip prefetching for fields specified multiple times with aliases to prevent this issue, which may cause n+1 problems.

Bug Fixes:

  • Fixed a bug where using aliases with the same field in a query could lead to incorrect results.

Tests:

  • Added a test case for queries with nested connections, filters, and aliases.

Merging querysets is usually fine, but when using an alias it might be
that one queryset is filtering for something, and the other is filtering
for something else, for the same field. That can lead to wrong
results being returned.

From now on, if a field is specified more than once with an alias,
the optimizer will skip it, possibly causing n+1 issues, but avoiding
wrong results (which is worse).

Fix #695
@bellini666 bellini666 self-assigned this Jan 25, 2025
Copy link
Contributor

sourcery-ai bot commented Jan 25, 2025

Reviewer's Guide by Sourcery

This pull request modifies the query optimizer to avoid merging prefetches when using aliases. This prevents incorrect results when filtering on the same field with different aliases, but may lead to n+1 issues in some cases.

Sequence diagram for query optimization with aliases

sequenceDiagram
    participant Client
    participant Optimizer
    participant QuerySet

    Client->>Optimizer: Execute query with aliases
    Note over Optimizer: Check field selection count
    alt Field has multiple selections (aliases)
        Optimizer-->>QuerySet: Skip optimization for field
        Note over QuerySet: May cause N+1 queries
    else Field has single selection
        Optimizer->>QuerySet: Apply optimization
        Note over QuerySet: Optimized query execution
    end
    QuerySet-->>Client: Return results
Loading

Flow diagram for field optimization decision

flowchart TD
    A[Start Field Processing] --> B{Field count > 1?}
    B -->|Yes| C[Skip Optimization]
    B -->|No| D[Apply Optimization]
    C --> E[Add parent relation fields]
    D --> E
    E --> F[Return Optimized Store]

    style C fill:#ffcccc
    style D fill:#ccffcc
Loading

File-Level Changes

Change Details Files
Avoid merging prefetches when using aliases
  • The optimizer will now skip fields that are specified more than once with an alias.
  • Added a test case to verify that the optimizer does not merge prefetches when using aliases.
  • Added a test case to verify that the optimizer does not cause n+1 issues when skipping optimization for a relation.
tests/test_optimizer.py
strawberry_django/optimizer.py
Removed redundant fields from test cases
  • Removed milestoneAgain field from test_query_forward_with_fragments test case.
  • Removed otherIssues field from test_query_prefetch_with_fragments test case.
tests/test_optimizer.py

Assessment against linked issues

Issue Objective Addressed Explanation
#695 Resolve the issue of same prefetch_related query with different filters returning identical results when optimizer is enabled
#695 Prevent incorrect query results when using field aliases with different filters
#695 Ensure query optimization does not compromise query accuracy

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

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.

Hey @bellini666 - I've reviewed your changes and they look great!

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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

tests/test_optimizer.py Show resolved Hide resolved
@bellini666 bellini666 requested a review from patrick91 January 25, 2025 13:38
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.12%. Comparing base (610e12b) to head (ba97980).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
- Coverage   89.05%   88.12%   -0.93%     
==========================================
  Files          42       42              
  Lines        3837     3850      +13     
==========================================
- Hits         3417     3393      -24     
- Misses        420      457      +37     

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

@bellini666 bellini666 merged commit d95acd6 into main Jan 26, 2025
30 checks passed
@bellini666 bellini666 deleted the fix_wrong_merge_of_prefetches branch January 26, 2025 11:25
@NT-Timm
Copy link
Contributor

NT-Timm commented Jan 29, 2025

Edit:
Nevermind. After thinking it over manually caching it would remove any problems with future library changes.

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.

Same prefetch_related query with different filter merged on optimizer enabled
3 participants