strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
https://strawberry.rocks/docs/django
MIT License
415 stars 120 forks source link

Type arguments like filters, order, and pagnation do not work with strawberry.lazy() #257

Closed coderanger closed 1 year ago

coderanger commented 1 year ago

Describe the Bug

If a type has argument features enabled such as filter classes, order classes, or pagination, this schema metadata is not respected when using it as a field in another type via lazy() (to prevent import loops).

For example, I have a ForeignKey field between PetItems and Items which I want to be queryable in either direction so on one side I have:

@gql.django.type(models.PetItem, order=PetItemOrder)
class PetItem:
    id: int
    pet: Pet
    item: Item
    level: auto

and on the other:

@gql.django.type(models.Item, filters=ItemFilter, order=ItemOrder, pagination=True)
class Item:
    id: int
    name: auto
    image: auto
    pet_items: list[Annotated["PetItem", gql.lazy("pets.graphql")]]

In the top-level schema, the order works as expected but when querying items { petItems(order: ... the arguments aren't found. This is also reflected in the schema view. As a workaround I can do pet_items: list[Annotated["PetItem", gql.lazy("pets.graphql")]] = gql.django.field(order=PetItemOrder) which does work but requires rearranging my code in a very frustrating way since the whole reason for the lazy load in the first place is that my pets.graphql module has to import items.graphql. I've so far been able to move things like order and filter classes into their own modules which are imported in both and don't trigger a loop.

But overall this seems like something which should travel along with the type class since it is defined there. Is there an easier way to accomplish this?

Versions

Upvote & Fund

Fund with Polar

bellini666 commented 1 year ago

This should be fixed in the upcoming release!

bellini666 commented 1 year ago

This should be fixed in v0.10.0

coderanger commented 1 year ago

I've tried 0.10.3 and still seeing the same problem.

coderanger commented 1 year ago

Actually not only is the bug not fixed, it's much much worse because now when I try to use the previous workaround, it's somehow skipping the query optimizer Screenshot 2023-07-06 at 8 10 59 PM

Happy to provide more details or try alternate solutions :)

bellini666 commented 1 year ago

Hey @coderanger ,

Hrm, based on your queries, it seems that the optimizer did the prefetch correctly, but then did one more query for each result due to "something".

I suppose that that "something" is because of the ORDER BY. Are you passing an order to the petItems list?

coderanger commented 1 year ago

Yes, the test query was:

{
  items {
    name
    petItems(order: {level:ASC}) {
      pet {
        name
      }
    }
  }
}

without the order it runs in 2 queries an ~30ms :)

bellini666 commented 1 year ago

@coderanger that's the reason =P

The optimizer currently ordering, filtering or anything that would make the prefetched cache to be thrown away.

I want to try to support both soon though

coderanger commented 1 year ago

Okay, even putting that aside, the same issue as originally here still happens too with arguments not carrying through when using lazy() :)

bellini666 commented 1 year ago

Okay, even putting that aside, the same issue as originally here still happens too with arguments not carrying through when using lazy() :)

Oh, really? Your example from the first comment is still having issues?

I noticed that in your example you mentioned you were using strawberry == 0.175.0. What version are you currently using? If it is not the latest, could you test with its latest?

coderanger commented 1 year ago

Yep, tested with version = "0.193.1". I put up a branch with the version bumps at https://github.com/coderanger/farmrpg-etl2/tree/strawberry-django-0.10

erwinfeser commented 1 year ago

I upgraded to the latest versions:

strawberry-graphql==0.194.4
strawberry-graphql-django==0.12.0

And these filters stopped working in both cases now: using strawberry.lazy and writing the definition in the same module

bellini666 commented 1 year ago

@coderanger @erwinfeser I think I found the issue. It was not actually specific to lazy but instead non strawberry django fields being created defaulting filters/ordering/pagination to None instead of UNSET

i.e. if you were doing pet_items: list[Annotated["PetItem", gql.lazy("pets.graphql")]] = strawberry_django.field() it would probably work correctly.

Any way, just released a new version that should fix this for good. Please tell me if it indeed does

erwinfeser commented 1 year ago

@bellini666 Before your release I solved the problem by doing this:

cage_locations: List[
        Annotated[
            "CageLocation",
            strawberry.lazy("types.cage_location"),
        ]
    ] = strawberry.django.field(
        filters=CageLocationFilter,
        order=order.CageLocationOrder,
        pagination=True,
    )

and now I can solve the same problem by doing this:

cage_locations: List[
        Annotated[
            "CageLocation",
            strawberry.lazy("types.cage_location"),
        ]
    ] = strawberry.django.field()

but my CageLocationFilter filters are not available if I do this:

cage_locations: List[
        Annotated[
            "CageLocation",
            strawberry.lazy("types.cage_location"),
        ]
    ]
bellini666 commented 1 year ago

@erwinfeser is cage_locations inside a django type or a vanilla strawberry type (e.g. the Query itself)?

erwinfeser commented 1 year ago

@bellini666 CageLocation is defined like this:

@strawberry.django.type(
    physical_models.CageLocation,
    filters=CageLocationFilter,
    order=order.CageLocationOrder,
    pagination=True,
)
class CageLocation:
    id: int
    # A few other fields
bellini666 commented 1 year ago

@erwinfeser Oh, I mean the type where the field itself is defined. Like, is it:

@strawberry.type
class Query:
    cage_locations: ...

Or

@strawberry_django.type(SomeModel):
class SomeModelType:
    cage_locations: ...

Because if it is defined in a vanilla strawberry type, then indeed you need to set it to a strawberry_django field (i.e. = strawberry_django.field(...)). When defining it inside a strawberry django type, then you don't need to. (obs. I probably should document that better)

If it is the second case, then I need to take a look to see what might be going on there.