Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Mapiarz opened this issue Mar 11, 2024 · 2 comments
Labels
bug Something isn't working question Further information is requested

Comments

@Mapiarz
Copy link
Contributor

Mapiarz commented Mar 11, 2024

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 170c27c. 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

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@Mapiarz Mapiarz added the bug Something isn't working label Mar 11, 2024
@bellini666
Copy link
Member

Hey @Mapiarz ,

Hrm, that change was done on #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
Copy link
Contributor

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?

@bellini666 bellini666 added the question Further information is requested label Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants