strawberry-graphql / strawberry-django

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

Built in create mutation has regressed and is not able to create model instances correctly #500

Open Mapiarz opened 3 months ago

Mapiarz commented 3 months ago

I have detected a regression where the built in create mutation is not able to properly process the input and create a django model with a foreign key relation. Consider the example:

class Booking(Model):
    boat_id: uuid.UUID
    boat = ForeignKey(Boat, on_delete=django_models.CASCADE)

@strawberry_django.input(floatist_models.Booking)
class CreateBookingInput:
    boat: uuid.UUID

This used to work in the past but has been broken with https://github.com/strawberry-graphql/strawberry-django/commit/170c27ca37ed72d095e6d131a8ce6d5d1b755074. Django now returns ValueError: Cannot assign "UUID('dafceb8a-2f28-4be8-b73a-b196a77126f4')": "Booking.boat" must be a "Boat" instance.

The input is not processed before calling the manager create method. The input is correctly processed when creating the dummy instance for form cleaning OR when using the built in update mutation. The update and create mutations have diverged significantly and cause unexpected behavior. And btw, the argument that the manager create method must be used can be extended to manager update method as well, so that's another inconsistency that has been introduced.

For the time being, I can work around the problem by annotating the input with strawberry.ID instead of uuid.UUID. But that's not good overall as I now have discrepancy between typing in my create vs update mutations, I lose the strong UUID typing in the schema and validation (so it will also accept an int or any string for that matter) and I have to deal with a string instead of a proper UUID object instance.

Upvote & Fund

Fund with Polar

bellini666 commented 3 months ago

Hey @Mapiarz ,

Hrm, that change was done on https://github.com/strawberry-graphql/strawberry-django/pull/394 to ensure proxy-model compatibility.

Taking a quick look at this I would say that it indeed should either require you to annotate it with strawberry.ID or change it to boat_id: uuid.UUID. But because you are saying that it used to work before, I'm wondering what we were doing differently. Because this was not a use case I had in mind before =P

Do you know what piece of code exactly made this work so we can try to find a way to make this work again, without breaking proxy-model compatibility?

cc @sdobbelaere

sdobbelaere commented 3 months ago

The main change is the usage of objects.create() rather than setting an instance and then saving it. It is correct that the dummy-instance still has that old behaviour, but the actual create() works differently than before.

@Mapiarz If you're in a shell, how would you create Booking instance?