evenicoulddoit / django-rest-framework-serializer-extensions

Extensions to help DRY up Django Rest Framework serializers
Other
247 stars 23 forks source link

Saving object with expanded relations #22

Closed solartune closed 6 years ago

solartune commented 6 years ago

Hey, I've faced with a problem.
I have a model where I have FK field to Country model. I need to choose a country and get a response after saving the model with expanded country. But I got an error (that it should be a dict but got int) when I specified ?expand=country and tried to save it with its id like "country": 123. I tried to send data like this "country": {"id": 123} but it doesn't work either, because it tries to create a new Country object. When I'm not specifying ?expand=country and sending "country": 123 it works well, but in a response I see only its ID, because it's not expanded.

A part of my serializer:

expandable_fields = dict(
    country=dict(
        serializer=CountrySerializer,
        read_only=True,
        id_source=False
    )
)

It seems to me that with read_only=True it should work fine, but I actually don't see a difference between True and False in that case.

evenicoulddoit commented 6 years ago

Hi @solartune - have you read http://django-rest-framework-serializer-extensions.readthedocs.io/en/latest/usage-serializers/#writable-foreignkey-relationships ?

solartune commented 6 years ago

Hi @evenicoulddoit ! Frankly, I misunderstood a little bit this part when I read it for the first time. I've read it more carefully now, but I still have a problem. I don't see this key in response country_id_resolved.

{
    ...
    "country": 6697173,
    "country_id": 6697173
}

Changed the config to

expandable_fields = dict(
    country=dict(
        serializer=CountrySerializer,
        read_only=False
    )
)
evenicoulddoit commented 6 years ago

Hi @solartune - why are you posting both country and country_id. You should only post country_id I think

solartune commented 6 years ago

country exists in the response too if I send it without exclude=country I mean just to /object/id/. In my request there is only country_id (I tried with both though and there is no difference). But if I even send it with /object/id/?exclude=country I don't see the key in the response. Only country_id

evenicoulddoit commented 6 years ago

@solartune are you able to make a short-reproducible repo which demonstrates your issue? Are you using the latest version of extensions?

solartune commented 6 years ago

Yes, but I've found out one interesting thing. In my previous tests I tried to update the country field for an existing object. And I didn't see country_id_resolved field at all. Now I tried to create a new object and caught this error TypeError: 'country_id_resolved' is an invalid keyword argument for this function. So, now I see it (at least in error :D ). I'll create the repo soon, there is the same behaviour

solartune commented 6 years ago

Here you go :) https://github.com/solartune/drf_serializer_ext

solartune commented 6 years ago

Hey @evenicoulddoit, have you tried it?

evenicoulddoit commented 6 years ago

@solartune sorry - haven't got round to it yet. I'll try to look at it this weekend, thanks for the repo though!

solartune commented 6 years ago

Ok, thank you! :)

BerniWittmann commented 6 years ago

I've got the same issue and can confirm that @solartune 's repo is also having this issue

evenicoulddoit commented 6 years ago

Sorry sorry sorry - been very busy lately, I'll set a reminder to try to look at this this evening :+1:

evenicoulddoit commented 6 years ago

OK sigh hah - I've not even worked with Django for 6 months, so took me 10 minutes or so to get this going (also tried booting it with Py2, didn't realise your repo was only Py3 compatible).

If you re-read the docs you should be able to see what's wrong. In your example, you've defined country as both a field and as an expandable field. You never want to define something as both.

You need to remove the additional country field reference to get this to work. The point is, the expandable_fields and fields should not overlap, a field should be one or the other. You'll also need to manually handle the additional country_id_resolved field which you're left with. The reason that this field is left in place is because Django has already done the hard work of validating that the foreign key actually exists, and if you wanted to perform additional validation on the instance (e.g. should "London" be assignable to "Belgium"), you could do it more efficiently by grabbing the resolved instance.

So, in the case of this repo, we need to remove the duplicate field references, as well as .pop() the resolved ID from our validated data before creating, et voila!

Thanks, hope that helps. If you think the docs could be clearer, please feel free to submit a patch suggestion.


Diff

diff --git a/core/serializers.py b/core/serializers.py
index a4eac71..458ac5e 100644
--- a/core/serializers.py
+++ b/core/serializers.py
@@ -18,7 +18,7 @@ class CitySerializer(SerializerExtensionsMixin, serializers.ModelSerializer):

     class Meta:
         model = City
-        fields = ('id', 'name', 'country', 'url')
+        fields = ('id', 'name', 'url')

         read_only = ('id', 'url',)

@@ -28,3 +28,7 @@ class CitySerializer(SerializerExtensionsMixin, serializers.ModelSerializer):
                 read_only=False,
             )
         )
+
+    def create(self, validated_data):
+        validated_data.pop('country_id_resolved')
+        return super().create(validated_data)

Postman screenshot of POST

image


DRF API-Browser GET

image

solartune commented 6 years ago

Thank you very much @evenicoulddoit! I'll try it soon