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: Fix specialized connection aliases missing filters/ordering #547

Merged
merged 3 commits into from
Jun 7, 2024

Conversation

bellini666
Copy link
Member

@bellini666 bellini666 commented Jun 7, 2024

When retrieving the strawberry django type, we need to make sure to resolve any LazyType and also to consider specialized type vars for aliases to get the NodeType from them.

This also bumps the minimum strawberry version to 0.234.2 as the fix in that release is required for this change.

Without this change, Author.books would not contain filters and order arguments

Fix #535

Thanks @Eraldo for reporting and creating an MRE that helped to identify the issue

Summary by Sourcery

This pull request fixes a bug where specialized connection aliases were missing filters and ordering arguments by resolving LazyType and considering specialized type vars for aliases. New tests were added to ensure the correctness of these changes.

  • Bug Fixes:
    • Fixed an issue where specialized connection aliases were missing filters and ordering arguments by resolving LazyType and considering specialized type vars for aliases.
  • Tests:
    • Added new tests to verify the correct resolution of LazyType and specialized type vars for connection aliases, ensuring filters and ordering arguments are included.

@bellini666 bellini666 self-assigned this Jun 7, 2024
Copy link
Contributor

sourcery-ai bot commented Jun 7, 2024

Reviewer's Guide by Sourcery

This pull request addresses an issue where specialized connection aliases were missing filters and ordering arguments. The changes ensure that when retrieving the strawberry Django type, any LazyType is resolved, and specialized type variables for aliases are considered to get the NodeType. This fix ensures that fields like Author.books will now include filters and order arguments.

File-Level Changes

Files Changes
tests/relay/lazy/b.py
tests/relay/lazy/a.py
tests/relay/lazy/models.py
Added new test files and models to verify the fix for specialized connection aliases including filters and ordering arguments.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

@bellini666 bellini666 requested a review from patrick91 June 7, 2024 14:31
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: 4 issues found
  • 🟢 Security: all looks good
  • 🟡 Testing: 5 issues found
  • 🟢 Complexity: 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 to tell me if it was helpful.

strawberry_django/fields/base.py Show resolved Hide resolved
strawberry_django/fields/base.py Show resolved Hide resolved
strawberry_django/fields/base.py Show resolved Hide resolved
strawberry_django/fields/base.py Show resolved Hide resolved
tests/relay/lazy/test_lazy_annotations.py Show resolved Hide resolved
tests/relay/lazy/test_lazy_annotations.py Outdated Show resolved Hide resolved
tests/relay/lazy/test_lazy_annotations.py Show resolved Hide resolved
tests/relay/lazy/a.py Show resolved Hide resolved
tests/relay/lazy/b.py Show resolved Hide resolved
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

@bellini666 bellini666 merged commit 6299390 into main Jun 7, 2024
52 checks passed
@bellini666 bellini666 deleted the fix_specialized_aliases branch June 7, 2024 14:42
@Eraldo
Copy link
Contributor

Eraldo commented Jun 7, 2024

@bellini666 Awesome! This really helps me with my migration. 🙏 👏
Works perfectly! 👍

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.

Lazy loaded connections don't show filters or ordering
3 participants