Partial update skip required field validation

[simone6021]

Checklist

• I have verified that that issue exists against the master branch of Django REST framework.
• I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• This is not a usage question. (Those should be directed to the discussion group instead.)
• 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.)
• I have reduced the issue to the simplest possible case.
• I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Hello, first thanks for this great piece code.
I am referring to the issue ##1725 because it still seems like a bug to me even in version 3.x.

Steps to reproduce

If a partial update is performed and a field marked as required is passed with a an empty value (even null if allow_null is set to true) the validation passes.

Expected behavior

In my humble opinion, required fields should be checked and validated as such even in partial update, if they are passed in the payload.

Actual behavior

The required fields are saved with the passed value even if empty and even null if allow_null is set to true.

[xordoquy]

At this point, a test case would help us triage the issue. It might be a side effect of sending forms instead of JSON format.

[simone6021]

Hello xordoquy, thanks for the answer.

I have now understood the drf behaviour and in my opinion the documentation about this should be at least improved.
As of now you can control if a field is required with the combination of 'required' and 'allow_null' parameters (adds 'allow_blank' for CharField derived fields), the concept that was not clear to me is that required is not considered at all in the partial update so a field can be saved with an 'empty' value: null if allow_null is true (and/or emtpy string when allow_blank is available and true).
However, in full update but more important in creation, a field 'required' is always, well, required.
The source of my confusion is how i (but i think many of us) think about 'required': to me a field required needs a value AND do not admit empty values. In drf this can be achieved with 'allow_null' and 'allow_blank' parameters and not with the 'required'.

My proposal is to emphasize that 'required' is only relative to the presence of a field in the payload and only for create and full update operations, but not considered in the partial update.
If you agree, i can also try to make a pull request for the documentation.

[xordoquy]

Part of your proposal is already in the documentation:

Normally an error will be raised if a field is not supplied during deserialization.

I agree we could add a -short- notice about the partial update.

[simone6021]

You're right but to me that sentence could be at least more explicit :-)
Furthermore, the 'normally' can be safely replaced with 'In create and full update operations' and added a note to clear out that in partial update the parameter is not considered.

I will create a pull request for this, in the next few days.

[tomchristie]

Furthermore, the 'normally' can be safely replaced with 'In create and full update operations' and added a note the explain why in partial update that parameter is not considered.

Okay, although let's phrase it the other ways around. With "partial update" mentioned afterwards, as the exception to the norm.

[sehmaschine]

Maybe I'm missing something, but this is still not clear to me ...

The docs about partial updates:
By default, serializers must be passed values for all required fields or they will raise validation errors. You can use the partial argument in order to allow partial updates.

This sentence explicitely refers to "all required fields" (not mentioning null/blank values).
Besides, I think required is different from null/blank – if I need a field to be required it has to be part of my request. The null/blank issue is the next step (e.g. the field is part of data, but it is blank). Not sure how these different concepts are mixed up when it comes to partial updates.

[Skorpyon]

I still periodically failing with this issue. Is it possible add additional always_required attr for a fields, so this fields will be required for partial update validation?

[CelestialGuru]

I'm using serializers.PrimaryKeyRelatedField(queryset=..., required=True, allow_null=False, allow_empty=False), yet the serializer with partial=True still ignores my required field. The only way around this issue, to force the field to be required even in partial updates, is with this:

class CustomPrimaryKeyField(serializers.PrimaryKeyRelatedField):
    def validate_empty_values(self, data):
        try:
            return super().validate_empty_values(data)
        except fields.SkipField:
            if self.required:
                self.fail("required")
            raise

I believe the issue stems from fields.py:Field.validate_empty_values():

class Field:
    def validate_empty_values(self, data):
        ...
        if data is empty:
            if getattr(self.root, 'partial', False):
                raise SkipField()
            if self.required:
                self.fail('required')
            return (True, self.get_default())
        ...

If the order of checking self.root.partial and self.required were swapped, I think this would work:

class Field:
    def validate_empty_values(self, data):
        ...
        if data is empty:
            if self.required:
                self.fail('required')  # Field is required. Don't care about partial=True
            if getattr(self.root, 'partial', False):
                raise SkipField()
            return (True, self.get_default())
        ...

If the field is required, and data was not passed, just err. Then, if it's not required, may we check if partial=True.

[auvipy]

I'm using serializers.PrimaryKeyRelatedField(queryset=..., required=True, allow_null=False, allow_empty=False), yet the serializer with partial=True still ignores my required field. The only way around this issue, to force the field to be required even in partial updates, is with this:

class CustomPrimaryKeyField(serializers.PrimaryKeyRelatedField):
    def validate_empty_values(self, data):
        try:
            return super().validate_empty_values(data)
        except fields.SkipField:
            if self.required:
                self.fail("required")
            raise

I believe the issue stems from fields.py:Field.validate_empty_values():

class Field:
    def validate_empty_values(self, data):
        ...
        if data is empty:
            if getattr(self.root, 'partial', False):
                raise SkipField()
            if self.required:
                self.fail('required')
            return (True, self.get_default())
        ...

If the order of checking self.root.partial and self.required were swapped, I think this would work:

class Field:
    def validate_empty_values(self, data):
        ...
        if data is empty:
            if self.required:
                self.fail('required')  # Field is required. Don't care about partial=True
            if getattr(self.root, 'partial', False):
                raise SkipField()
            return (True, self.get_default())
        ...

If the field is required, and data was not passed, just err. Then, if it's not required, may we check if partial=True.

can you come with a draft PR to improve this?

[SteliosHD]

It is still relevant today. It would be helpful to add a note to the docs stating the actual behavior when the field is required.