strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
MIT License
391 stars 115 forks source link

fix: Extract interface definition in optimizer to fix django-polymorphic #556

Closed ManiacMaxo closed 1 week ago

ManiacMaxo commented 2 weeks ago

Description

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

Types of Changes

Issues Fixed or Closed by This PR

Checklist

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.

sourcery-ai[bot] commented 2 weeks ago

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](https://sourcery.ai/dashboard): - Enable or disable the Sourcery-generated pull request summary or reviewer's guide; - Change the review language; - You can always [contact us](mailto:support@sourcery.ai) if you have any questions or feedback.
patrick91 commented 2 weeks ago

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

ManiacMaxo commented 2 weeks ago

I'll try to add a unit test

bellini666 commented 2 weeks ago

Hey @ManiacMaxo ,

I saw that you mentioned on https://github.com/strawberry-graphql/strawberry-django/issues/550#issuecomment-2164762964 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 https://github.com/strawberry-graphql/strawberry-django/pull/553

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

stygmate commented 5 days ago

@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 commented 4 days ago

Thanks for the test @stygmate ,

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

stygmate commented 4 days ago

@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 commented 1 hour ago

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 😊