Django-rest-framework: PATCH does not return 400

[latusaki]

Checklist

• [x] I have verified that that issue exists against the master branch of Django REST framework.
• [x] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• [x] This is not a usage question. (Those should be directed to the discussion group instead.)
• [x] This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• [x] I have reduced the issue to the simplest possible case.
• [x] I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Make a PATCH request against an Update endpoint, with payload keys that do no exist on serializer.

Expected behavior

Making a PATCH request, with keys that do not exist in the serializer, should return 400, based on https://www.django-rest-framework.org/api-guide/generic-views/#updatemodelmixin
"If the request data provided for updating the object was invalid, a 400 Bad Request response will be returned, with the error details as the body of the response."

Is this a case of wrong documentation? Just saw this issue: https://github.com/encode/django-rest-framework/issues/4086

Actual behavior

Response is 200, no changes applied, but no error either

Additional Info

Found this while writing unittest for a very basic RetrieveUpdateDestroy endpoint on my application. The endpoint is backed by a ModelSerializer. When read-only values are passed through PATCH, no error is returned (although no edits happen either). If non-existent keys are passed, again no error. If correct keys are passed, partial update works.

Here is a unittest that I think demonstrates the issue:

https://github.com/latusaki/django-rest-framework/commit/3e1427402fbc5a5304406904110c0343462771fb

[tomchristie]

Serializers lieniently ignore unknown fields. (Irrespective of PUT, POST, PATCH)

Having strict erroring on unknown fields would be better in some cases, and worse in others. Unlikely that we'll change the behavior at this point since it'd liekly result in a major breaking change for lots of users.

[latusaki]

@tomchristie what about PATCH requests on read-only fields? Would this not be in scope?

[tomchristie]

Probably not. The most consistent thing for us to do is to be lenient where possible on what data we accept/reject.

That's not to say that it wouldn't also be a perfectly reasonable behavior, but there's no wrong or right answer here - and staying with a simple, consistent lenient approach to validation is where we'd probably opt for in this case. Might be a different story if someone actually wanted to put in the time and demonstrate a pull request, show that the additional complexity was worthwhile, but we're not otherwise likely to consider it worth our time.

[latusaki]

Ok, in my (simple) use case overriding is_valid to check {initial_data} - {fields - read_only_fields} will produce the desired outcome. The documentation lead me to believe DRF would be raising an error by default. Thanks for clarifying.

[gitaarik]

@latusaki what exactly did you do in your code that overrides the is_valid() method? I'm having the same issue. It seems to me obvious that you get an error when you're trying to write to a read-only field.

[gitaarik]

I eventually made a mixin class myself which fixes my problem:

from rest_framework.fields import empty
from rest_framework.exceptions import ValidationError


class ErrorOnWriteToReadOnlyFieldMixin():

    def run_validation(self, data=empty):

        for fieldname, field in self.fields.items():
            if field.read_only and fieldname in data.keys():
                raise ValidationError(
                    code='write_on_read_only_field',
                    detail={
                        fieldname: (
                            f"You're trying to write to the field "
                            "'{fieldname}' which is a read-only field."
                        )
                    }
                )

        return super().run_validation(data)

[latusaki]

I had a similar approach. Its probably not generic enough, but works fine for my usecase.

    def is_valid(self, raise_exception=False):
        is_valid = super().is_valid(raise_exception)
        editable_fields = set(self.Meta.fields) - set(self.Meta.read_only_fields)
        extra_keys = set(self.initial_data) - editable_fields
        if extra_keys:
            raise ValidationError(f'Non-editable keys provided: {extra_keys}')
        return is_valid