umutbozkurt / django-rest-framework-mongoengine

Mongoengine support for Django Rest Framework
MIT License
617 stars 166 forks source link

DynamicDocumentSerializer should ignore values passed for read only fields instead of raising an exception #218

Open Benzene opened 7 years ago

Benzene commented 7 years ago

Hello,

when I pass to a DynamicDocumentSerializer some data that applies to a read-only field (could be "id", but I think it applies to others too), it raises an AssertionError.

DocumentSerializer (and I think DRF) would silently ignore this value, but DynamicDocumentSerializer throws an error on that. There is a comment in the code (https://github.com/umutbozkurt/django-rest-framework-mongoengine/blob/master/rest_framework_mongoengine/serializers.py#L629) explaining it, but I find this behaviour a bit annoying in practice. It basically means I have to strip the id from the object manually before passing it to the serializer. Is there some way around it? Or can the code be changed so that values for read_only fields are just ignored?

Some example code that triggers the issue:

class TestDocument(DynamicDocument):
    pass

class TestDocumentSerializer(DynamicDocumentSerializer):
    class Meta:
        model = TestDocument
        fields = '__all__'

s = TestDocumentSerializer(data={})
s.is_valid()
testDoc = s.save()

s_data = TestDocumentSerializer(testDoc).data

s2 = TestDocumentSerializer(testDoc, data=s_data)
s2.is_valid()
BurkovBA commented 7 years ago

@Benzene Hi, Matt, yes, this is done on purpose.

So, if you want to make your read-only field, such as id... ahem... non-read-only, you can just override the auto-generated model field with an explicitly declared field on your DynamicDocumentSerializer like this:

class TestDocumentSerializer(DynamicDocumentSerializer):
    id = rest_framework_mongoengine.fields.ObjectIdField()  # or try DynamicField if this fails

    class Meta:
        model = TestDocument
        fields = '__all__'

Good luck!

Benzene commented 7 years ago

Hi Boris,

Sorry, I was not clear enough. I don't want to make the id writeable, I just want the serializer to ignore the 'id' that the client sends back, instead of raising a 500 error, because if the client is most likely sending it by accident. I shouldn't have focused on the 'id', as the issue is about read-only fields in general. 'id' just happens to be the most common one.

In most of my usage (and especially with DynamicDocumentSerializer, where I don't always know what fields are present on my object), what my javascript code does is a GET to retrieve the object, it mutates the response, and then sends the entire object back directly as a PUT parameter. But currently, if I do things like that, the PUT fails with a 500 error because I passed it back the read-only fields coming from the GET.

The django-rest-framework docs (http://www.django-rest-framework.org/api-guide/fields/#read_only) says: "Any 'read_only' fields that are incorrectly included in the serializer input will be ignored." So I think DRF-mongoengine should match that behavior. DocumentSerializer already acts like that, only DynamicDocumentSerializer doesn't.

The workaround I'm using is to override _get_dynamic_data in DynamicDocumentSerializer to skip read-only fields

class DynamicDocumentSerializer2(mongo_serializers.DynamicDocumentSerializer):
    def _get_dynamic_data(self, validated_data):
        result = {}

        for key in self.initial_data:
            if key not in validated_data:
                try:
                    field = self.fields[key]

                    if not (isinstance(field, drf_fields.SkipField) or field.read_only):
                        msg = (
                            'Field %s is missing from validated data,'
                            'but is not a SkipField!'
                        ) % key
                        raise AssertionError(msg)
                except KeyError:  # ok, this is dynamic data
                    result[key] = self.initial_data[key]
        return result

But I don't know if it has any consequences on the rest of the code.

And well, I found my workaround, so you can close this issue if you think the current behaviour works better !