yezyilomo / django-restql

Turn your API made with Django REST Framework(DRF) into a GraphQL like API.
https://yezyilomo.github.io/django-restql
MIT License
616 stars 43 forks source link

Updating a serializer won't work without the foreign key #188

Closed lesreaper closed 3 years ago

lesreaper commented 3 years ago

Awesome library, love what you're doing here.

My only challenge is updating parent model without adding the child key. So, if I just want to PATCH one field on the Room model below, like the name, I don't want to have to add the castle id as well to the main body call. Here's the code

serializers.py

class CastleSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    class Meta:
        model = Castle
        fields = '__all__'

class RoomSerializer(DynamicFieldsMixin, NestedModelSerializer):
    castle = NestedField(CastleSerializer, accept_pk=True)

    class Meta:
        model = Room
        fields = ('id', 'name', 'castle')

I would expect to be able to POST or PATCH an update method without having to add the nested castle id and have it just validate and work.

API call: PATCH /api/v1/rooms-update/

[
  {
    "id": 1,
    "name": "Throne Room Changed Text",
  }
]

should return

[
  {
    "id": 1,
    "name": "Throne Room Changed Text",
    "castle": {
        "id": 1,
        "name": "Daventry Castle"
    }
  }
]

It seems none of this is currently working, but when I add the castle primary key, it works just fine. I could change the

castle = NestedField(CastleSerializer, accept_pk=True)

to

castle = NestedField(CastleSerializer, required=False, allow_null=True)

but that would update the castle field to null, when instead I want it to keep it's previous value (since it's not part of the body text).

How do I go about this?

lesreaper commented 3 years ago

So, maybe I solved this with a hack.

In the PATCH view method for the Room, I wrote the following:

views_room.py

@api_view(['PATCH'])
def roomsUpdate(request):
  room_data = Room.objects.get(id=request.data.id)
  if not "castle" in request.data:
      try:
          castle = Castle.objects.get(pk=room_data.castle.id)
          request.data["castle"] = castle.id
      except Exception as e:
          print("[ERROR]: No castle to begin with")

  serializer = RoomSerializer(instance=room_data, data=request.data)

  if serializer.is_valid():
      serializer.save()

  return Response(serializer.data)

If there'a a better way, I'm definitely open to it!

yezyilomo commented 3 years ago

but that would update the castle field to null, when instead I want it to keep it's previous value (since it's not part of the body text).

How do I go about this?

I don't think it would update the castle field to null when you use castle = NestedField(CastleSerializer, required=False, allow_null=True). Check https://django-restql.yezyilomo.com/mutating_data/#using-dynamicfieldsmixin-and-nestedfield-together for more details about this behaviour.

resurrexi commented 3 years ago

You can use castle = NestedField(CastleSerializer, accept_pk=True, required=False, allow_null=True). That way, when you do need to pass a pk to this attribute on a POST/PATCH/PUT, it sets the castle attribute to the value, or preserves the old value if you don't pass a pk to the attribute on a PATCH.

dnievas04 commented 3 years ago

Hi @resurrexi I am facing the same problem. I have tried your solution but I keep getting a value error.

ValueError: Cannot assign "<class 'rest_framework.fields.empty'>": "Room.castle" must be an instance of "Castle".

In my case the related object cannot be null, so the required=False solution is not an option for our use case.

Any idea how to solve this problem? Is it a bug? Can it be considered a problem? or an improvement to implement? Thanks!

yezyilomo commented 3 years ago

Hi @resurrexi I am facing the same problem. I have tried your solution but I keep getting a value error.

ValueError: Cannot assign "<class 'rest_framework.fields.empty'>": "Room.castle" must be an instance of "Castle".

In my case the related object cannot be null, so the required=False solution is not an option for our use case.

Any idea how to solve this problem? Is it a bug? Can it be considered a problem? or an improvement to implement? Thanks!

Are you using PUT or PATCH? because there’s a difference.

dnievas04 commented 3 years ago

@yezyilomo, I am using PATCH. I think for a PUT this is the expected behavior, but when using PATCH one expects to send only the fields to update. What are your thoughts on this?

yezyilomo commented 3 years ago

@yezyilomo, I am using PATCH. I think for a PUT this is the expected behavior, but when using PATCH one expects to send only the fields to update. What are your thoughts on this?

I haven’t succeeded to reproduce it on my side, can I see your serializer and json data which you send through PATCH request. Specifying your django-restql version would be bery helpful.

gonzaloamadio commented 3 years ago

@yezyilomo there is a repository here where you can reproduce the bug. In the readme, there is a section with the tests we did to reproduce. Also at the beginning of it there are the package versions.

Basically you have to patch a model with nested fields. There, if we do not send all fields, it fails.

Repository

dnievas04 commented 3 years ago

@yezyilomo there is a repository here where you can reproduce the bug. In the readme, there is a section with the tests we did to reproduce. Also at the beginning of it there are the package versions.

Basically you have to patch a model with nested fields. There, if we do not send all fields, it fails.

Repository

Just to clarify, @gonzaloamadio is talking about the "Second Bug" reported in the Readme. Here is the link To reproduce the error you may have to change the identifiers we have used.

yezyilomo commented 3 years ago

@gonzaloamadio @dnievas04

@yezyilomo there is a repository here where you can reproduce the bug. In the readme, there is a section with the tests we did to reproduce. Also at the beginning of it there are the package versions.

Basically you have to patch a model with nested fields. There, if we do not send all fields, it fails.

Repository

You are actually using NestedField without inheriting NestedModelSerializer HERE, That's what causing that error.

It should be

class IngredientMixSerializer(DynamicFieldsMixin, NestedModelSerializer):

    ingredient = NestedField(IngredientSerializer, accept_pk_only=True)

    class Meta:
        model = IngredientMix
        fields = ['id', 'ingredient', 'product', 'name']

IngredientMix is a nested model because you have defined a nested field(ingredient) in it, so its serializer should inherit NestedModelSerializer.

dnievas04 commented 3 years ago

Oh! I'm sorry. You are absolutely right. It is now working as expected.

Thanks!

yezyilomo commented 3 years ago

Am closing this because it's not a bug, Just remember to inherit from NestedModelSerializer if your serializer is nested and you have used NestedField to define your nested fields.