strawberry-graphql / strawberry-django

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

self referencing Parent relation has pagination if type has it #252

Closed hvdklauw closed 5 months ago

hvdklauw commented 1 year ago

I have a category type:


@strawberry.django.type(models.Category, pagination=True)
class Category:
    id: auto
    name: auto
    slug: auto
    parent: "Category"
    children: List["Category"]

The graphql then adds pagination to the parent relation, which makes no sense, the children it does make sense for.

Describe the Bug

If the type is a single instance it should not have pagination by default.

System Information

Additional Context

I can fix it like this:


@strawberry.django.type(models.Category, pagination=True)
class Category:
    id: auto
    name: auto
    slug: auto
    parent: "Category" = strawberry.django.field(pagination=False)
    children: List["Category"]

but that shouldn't be needed imho.

Should also note that filters and order are not passed to the parent relation!

Upvote & Fund

Fund with Polar

bellini666 commented 1 year ago

Hey @hvdklauw ,

What version are you currently using? This commit should have fixed that issue, which is included in 0.9.4+

hvdklauw commented 1 year ago

I am on 0.9.4 and yes, filters and order are not shown, pagination still is.

I think this needs the same logic to exclude it.

bellini666 commented 1 year ago

@hvdklauw oh, I see. You are probably right.

Do you want to try to open a PR for this? Otherwise I'll try to fix this in the weekend

hvdklauw commented 1 year ago

I'll give it a go

hvdklauw commented 1 year ago

Ok, was writing tests and was wondering if get_pagination should return None for fields like that.


def test_pagination_on_related_field_definition():
    from strawberry_django.fields.field import StrawberryDjangoField

    group_field = StrawberryDjangoField(type_annotation=StrawberryAnnotation(List[Group]))
    assert group_field.get_pagination() is not None
    assert User._type_definition.get_field("group").get_pagination() is None

Or should it return Unset? Then the arguments function would also not return the arguments automatically.

hvdklauw commented 1 year ago

https://github.com/hvdklauw/strawberry-graphql-django/commit/125789984e2a23c643730c962aa774fd3126532c is what I made of it.

By returning UNSET in get_pagination it automatically won't add the arguments.

The thing is, now I think we also need to do this in filters and ordering to keep it consistent. Also couldn't find tests for those two in relation to not adding filters/order to those kind of fields.

hvdklauw commented 1 year ago

@bellini666 What do you think? should get_pagination (and thus get_filters and get_order) return UNSET when self.is_list is False or should just the arguments function not add arguments in those cases?

bellini666 commented 1 year ago

@hvdklauw I think your fix is correct. The only change I would request in a review would be to simply return None instead of UNSET. If you look at the code you will notice that it returns None at the end when no pagination is defined

Other than that, feel free to open the PR with that change :)