strawberry-graphql / strawberry-django

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

Default primary key search filter no longer being generated for single queries #329

Closed fireteam99 closed 11 months ago

fireteam99 commented 11 months ago

Describe the Bug

As explained in the documentation here, automatically generated single queries should have a built in filter for the primary key. This doesn't seem to happen on the latest version of strawberry/strawberry-django.

#schema.py

import strawberry

from .types import User

@strawberry.type
class Query:

    user: User = strawberry.django.field()

schema = strawberry.Schema(query=Query)

Would expect to work:

query User($id: ID!) {
  user(pk: $id) {
    id
  }
}

But get this error instead:

"Unknown argument 'pk' on field 'Query.user'."

GraphIQL also reports the filter to be missing.

Before: image

After: image

System Information

Additional Context

This bug was not present in previous versions. Specifically, it was working as expected in: strawberry-graphql 0.133.5, strawberry-graphql-django 0.7

Upvote & Fund

Fund with Polar

bellini666 commented 11 months ago

Hi @fireteam99 ,

Hrm, that's strange. It should be working just like before, we actually have a unit test which tests that exact behaviour.

Do you have a minimum reproduction example to understand what you might be doing different?

fireteam99 commented 11 months ago

Hi @bellini666, yes that is strange indeed. I will try to replicate in a MRE and get back to you. Thank you!

NT-Timm commented 11 months ago

I've encountered this issue as well. It's really random.

In my prototype branch it did not appear. But once I copied my code to my main branch the singular type filters disapeared. Both projects have the same version of django/strawberry_graphl/strawberry_graphl_django as well. I tried changing it to older version of strawberry_graphl_django but none made the singular type filters to appear again in my main branch.

Could it be a hidden caching problem?

bellini666 commented 11 months ago

I've encountered this issue as well. It's really random.

In my prototype branch it did not appear. But once I copied my code to my main branch the singular type filters disapeared. Both projects have the same version of django/strawberry_graphl/strawberry_graphl_django as well. I tried changing it to older version of strawberry_graphl_django but none made the singular type filters to appear again in my main branch.

Could it be a hidden caching problem?

TBH I'm not sure. If you can help with a MRE, it will be very helpful. I'm still not able to replicate the problem here T_T

NT-Timm commented 11 months ago

Ok. I did some testing. And I think I found a reproducible way to trigger this.

It happens when you merge nested queries

@strawberry.type
class ActualQuery:
    test: TestType = strawberry_django.field()
schema = strawberry.Schema(
    query=ActualQuery,
    extensions=[
        DjangoOptimizerExtension,
    ],
)

This works.

But this:

@strawberry.type
class ActualQuery:
    test: TestType = strawberry_django.field()
@strawberry.type
class AnotherActualQuery:
    bah: TestType = strawberry_django.field()
@strawberry.type
class CombinedQuery(ActualQuery, AnotherActualQuery):
    pass
schema = strawberry.Schema(
    query=CombinedQuery,
    extensions=[
        DjangoOptimizerExtension,
    ],
)

This strips the primary key filter. Replacing CombinedQuery with strawberry.merge_types does not work as well. Combining the queries without the strawberry.type decorator also doesn't work.

Could it be that the primary key filter is added to the Query class having the django_field variable and not the Query class actually being used? (I've run into stuff like that before while using strawberry in a autogenerating dynamic way)

bellini666 commented 11 months ago

Hey @NT-Timm ,

Indeed doing multiple inheritance for queries has its problems and doesn't work properly. For that you should actually use strawberry.merge_types. But if even that is triggering the issue, the problem must be deeper than that.

Thanks for the MRE, I'll try to reproduce and resolve during the weekend.

bellini666 commented 11 months ago

@NT-Timm ,

Was taking a look at your example and just found the issue. We expect that the Query type is called Query, and only inject the pk argument in it inside those, as you can see here

The reason for that can be seen in this commit: https://github.com/strawberry-graphql/strawberry-graphql-django/commit/bec0fb5d54657780ea018ac1b25666a2912f61b6

Considering that, a fix for your issue is to either define the type as Query or decorate it with @strawberry.type(name="Query")

I'm not sure if there's a better fix for that ATM, because there's no way for us to differentiate a type that is the root Query currently, so I relied on the fact that it is usually called Query.


Having said that, @fireteam99 's example is using Query, so I still need a MRE for that because this explanation doesn't explain his issue.

NT-Timm commented 11 months ago

@bellini666 that fixed my issue like a charm.

Also regarding @fireteam99's issue. I did notice they are using the strawberry.django version instead of the strawberry_django version. Could it be that the linked version is one where it's not fixed? How does strawberry populate the strawberry.django version anyway?

fireteam99 commented 11 months ago

@bellini666 it looks like you have to name all query types as Query or decorate with @strawberry.type(name="Query") otherwise the pk filter will not be automatically generated. This applies to the queries passed into merge_types as well as the "root" query. See example:

@strawberry.django.type(model=models.Person, fields="__all__")
class Person:
    pass

@strawberry.type(name="Query") # Decorated name must be "Query" if class name is not already "Query"
class PersonQuery:
    person: Person = strawberry.django.field()
    people: list[Person] = strawberry.django.field()

# This must also be named "Query" or have a decorated name of "Query"
Query = merge_types(
    "Query",
    (
        PersonQuery,
        SomeOtherQuery
    ),
)
schema = strawberry.Schema(query=Query)
NT-Timm commented 11 months ago

Yeah that's what I did. Just nuked all with Query and Mutation where applicable.

bellini666 commented 11 months ago

@bellini666 it looks like you have to name all query types as Query or decorate with @strawberry.type(name="Query") otherwise the pk filter will not be automatically generated. This applies to the queries passed into merge_types as well as the "root" query. See example:

Exactly!

I think that it doesn't happen too often because people usually define those as Query. For example, I have a Query and a Mutation inside each app's schema.py file, and I import those in a main schema.py like from api.user.schema import Query as UserQuery and pass them to the merge_types

I think it is worth adding a note about this in the documentation. If any of you want to do that, feel free to open a PR. Otherwise I'll take a look at that soon (maybe during the weekend)

fireteam99 commented 11 months ago

Sounds good, I can take a take a stab at updating the documentation

bellini666 commented 11 months ago

Fixed by https://github.com/strawberry-graphql/strawberry-graphql-django/pull/341