strawberry-graphql / strawberry-django

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

Partial inputs not working for inherited fields #320

Closed jaydensmith closed 10 months ago

jaydensmith commented 1 year ago

If specifying an input that inherits fields from a parent class as a partial input, non-optional fields in the parent class are required in the partial class.

Upvote & Fund

Fund with Polar

bellini666 commented 1 year ago

Hey @jaydensmith ,

That should not be the case. Can you provide a minimal reproduction example so I can reproduce it here to fix that?

jaydensmith commented 1 year ago

@bellini666 Previously, this worked:

@strawberry_django.input(models.Fruit)
class FruitInput:
    id: auto
    name: auto
    color: auto
    some_other_field: str

@strawberry_django.input(models.Fruit, partial=True)
class FruitPartialInput(FruitInput):
    pass

It seems not specifying any field as auto causes this issue. I can't always do this as some mutations have extra fields + custom mutations.

bellini666 commented 1 year ago

@jaydensmith oh, I see what you mean now

That's actually a behaviour change that I forgot to document. partial doesn't force non auto types to be optional anymore, so for those you should be typing them as str | None (in that case)

Having said that, I'm wondering about the inheriting scenario you are mentioning, because although it does make sense to respect the actual typing and not forcing it to optional when defined in the type itself, it does make sense to change the typing for the inheritance.

whardeman commented 11 months ago

@bellini666 I just discovered this same issue after upgrading and finding no mention in the docs. Glad I found this explanation!

That said, have you given more thought to this? If the current implementation is the desired behavior, I'd be happy to submit a PR to the documentation for this change.

bellini666 commented 11 months ago

That said, have you given more thought to this? If the current implementation is the desired behavior, I'd be happy to submit a PR to the documentation for this change.

Hi @whardeman ,

Yes, it is the desired behavior! explicit type hints should always be preserved, while auto should be converted to their proper types, considering the ORM introspection and if it is inside a type, input or partial.

And any contributions are always very welcomed! Specially to the docs, as it is something requiring some love =P

bellini666 commented 10 months ago

Closing this as the behaviour was explained and documented