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: Extract interface definition in optimizer to fix django-polymorphic #556

Merged
merged 6 commits into from
Jun 17, 2024

Conversation

ManiacMaxo
Copy link
Contributor

@ManiacMaxo ManiacMaxo commented Jun 13, 2024

Description

Extracting GraphQL definition from a strawberry definition based on the type (interface or object type)

Types of Changes

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

Issues Fixed or Closed by This PR

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).

Summary by Sourcery

This pull request addresses a bug in the optimizer by correctly extracting GraphQL definitions from Strawberry definitions, handling both interface and object types. It also includes a refactoring to improve code clarity and maintainability.

  • Bug Fixes:
    • Fixed the extraction of GraphQL definitions from Strawberry definitions by correctly handling both interface and object types.
  • Enhancements:
    • Refactored the optimizer code to use a new helper function _get_gql_definition for determining the GraphQL definition based on the type.

Copy link
Contributor

sourcery-ai bot commented Jun 13, 2024

Reviewer's Guide by Sourcery

This pull request addresses a bug by extracting the GraphQL definition from a strawberry definition based on whether it is an interface or an object type. The changes primarily involve refactoring the code to use a new helper function _get_gql_definition for this purpose.

File-Level Changes

Files Changes
strawberry_django/optimizer.py Refactored the code to use a new helper function _get_gql_definition for extracting GraphQL definitions, and reformatted the mark_optimized_by_prefetching function.

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.

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 @ManiacMaxo - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 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 to tell me if it was helpful.

strawberry_django/optimizer.py Outdated Show resolved Hide resolved
strawberry_django/optimizer.py Show resolved Hide resolved
strawberry_django/optimizer.py Show resolved Hide resolved
@patrick91
Copy link
Member

@ManiacMaxo this looks good, could you add a test? 😊

@ManiacMaxo
Copy link
Contributor Author

I'll try to add a unit test

@bellini666
Copy link
Member

Hey @ManiacMaxo ,

I saw that you mentioned on #550 (comment) that this might only be reproducible by django-polymorphic.

If that's the case, feel free to add django-polymorphic to the dev dependencies only, and write a test that depends on it, similar to how django-mptt was used for this #553

Let me know if you have any issues/doubts about that or anything else in here (e.g. the failing typing tests)

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.

Thanks! :)

@bellini666 bellini666 changed the title fix: Extracting interface definition in optimizer fix: Extract interface definition in optimizer to fix django-polymorphic Jun 17, 2024
@bellini666 bellini666 merged commit bfc598a into strawberry-graphql:main Jun 17, 2024
20 checks passed
@stygmate
Copy link
Contributor

stygmate commented Jun 24, 2024

@bellini666

Seems it don't work if the type class is a relay node.

I've added a test for it here: https://github.com/stygmate/strawberry-django/tree/test-optimizer-with-polymorphic-relay-node

you can test on the test-optimizer-with-polymorphic-relay-node branch of my fork by running: poetry run pytest tests/node_polymorphism

If you disable the optimizer in the schema of this test, the test pass.

@bellini666
Copy link
Member

Thanks for the test @stygmate ,

Will use it to investigate and try to fix the issue :)

@stygmate
Copy link
Contributor

stygmate commented Jun 25, 2024

@bellini666 I have made a patch. I modified two asserts of instance type.
The second make my test run without fail, i'm unsure of the modification for the first.

Here is the patch: https://github.com/stygmate/strawberry-django/tree/test-optimizer-with-polymorphic-relay-node

Do you want me to make a pull request ?

@bellini666
Copy link
Member

Hey @stygmate ,

Oh, I lost your comment, sorry. Yes please, open the PR :)

obs. PRs are always welcomed, no need to wait for my approval 😊

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.

Broken interfaces with optimiser extension
4 participants