Inconsistent update semantics when fields are omitted

[frankpape]

When performing a full update (i.e., partial=False) on a ModelSerializer and omitting fields, my expectation is that omitted, optional fields will not be set in the deserialized instance.

The documentation says:

By default, serializers must be passed values for all required fields or they will throw validation errors.

But what is the desired behavior for non-required fields?

The existing behavior is as follows:

1. For required fields (null=False, blank=False), is_valid() returns False.
2. For fields with a default specified, the value is set to the default.
3. For fields with null=True or blank=True, the value is left unchanged.

One reasonable (IMO) option for consistent behavior might be:

1. Continue to throw validation error for required fields.
2. Continue to set the value to default for fields with a default specified.
3. For fields with null=True, set the value to None.
4. For fields with blank=True, set the value to an empty string.

I suggest this behavior because it matches the creation semantics when fields are omitted.

Another possible option is to leave all unspecified values unchanged, but it seems like if this were desired, the caller would specify partial=True (this would make, for example, HTTP PUT requests behave like PATCH). It also feels like this conflicts with the requirement that a value is specified for required fields.

I will create a pull request with a failing test that asserts my expected behavior.

[frankpape]

Another option (that IMO also seems reasonable) would be to throw a validation error for any omitted fields, instead of just required fields, although this has the potential to break existing applications that depend on the existing behavior.

[frankpape]

The more I think about this, the less sure I am of what the desired behavior should be.

On creation, we get the distinction between None (for null=True) and '' (for blank=True) because django's CharField gives it to us. When the WritableField is doing the updating, we don't have easy access to that information (we only know if the field is required). And regardless, we'd have to do something sensible for non-model serializers. And for non-character fields.

My test is really asking for a default value for fields with no default specified, and that seems inappropriate given the variety of field types. I think I'm ending up with a preference for validation errors on any omitted fields, unless partial is True (optional: or unless there is a default specified).

Let me know what you prefer.

[frankpape]

Added an alternative option: behavior to raise a validation error on missing fields with no defaults in the above pull request.

[tomchristie]

So I think we need to better separate the two different parts of this discussion...

1: What options should we use when instantiate fields that are automatically created by a ModelSerializer. When should we set required=True, when should we set a default value?
2: Is any of the current behaviour for omitting fields handled incorrectly or non-intuitively?

We should test against (2) using plain old serializers (not model serializers) as that's more explicit, and doesn't conflate the two separate issues.

I've not reviewed this ticket sufficiently to come up with any decisions, but I think it'd be clearer if we could look at resolving those issues separately rather than bundling them together.

Hope I've explained that adequately?

[frankpape]

Yes, that makes sense. I agree that adding ModelSerializer into the mix adds an unnecessary complication (and this specific issue ought to focus on part 2). Will follow up with a test that uses a vanilla serializer when I have a chance.

[tomchristie]

Thanks @frankpape, sounds good. Do we think it makes sense to close the outstanding PRs in the meantime?

[frankpape]

Sure, will do.

[tomchristie]

We've made the decision to close of all currently open serializer tickets, pending the 3.0 release.

The 3.0 release will involve a major redesign and improvement of the current serializer implementation and should invalidate the large majority of these outstanding tickets.

We will be reviewing the closed tickets prior to the 3.0 launch and try to ensure that we have fully covered any long-standing issues, and adequately dealt with the outstanding problems.

If your issue does not appear to be addressed by the upcoming 3.0 release please do comment on your ticket with the current status, and we can reopen any valid tickets as required. In particular, if your ticket represents a long-standing or fundamental problem please do make sure to follow the mailing list and review any 3.0 pre-release candidates where possible.

Note that you can review the closed tickets with the following searches:

Serializer tickets: https://github.com/tomchristie/django-rest-framework/issues?q=label%3ASerializers
Writable nested serializer tickets: https://github.com/tomchristie/django-rest-framework/issues?q=label%3A%22Writable+nested+serializers%22

Many thanks for your understanding and here's looking forward to a new and improved version, and a cleaner more actionable issue list.

All the best,

Tom