not required read_only Fields with allow_null

[ticosax]

Description

Since #5639 we are unable to render fields that were rendered previously with allow_null=True.
Backward compatibility is not preserved, because required=True and read_only=True are conflicting, so in our case, some fields just disappeared and it seems there is no way to get them back.

I'm not sure about the fix, but the regression test is probably right.

[rpkilby]

btw, the required=False is unnecessary here, as it's implied by read_only=True.

[ticosax]

I can take it out if you prefer

[rpkilby]

In hindsight, I think the behavior in #5639 is not correct. Fields with required=False and no explicit default are omitted from the serialized output, however, allow_null does imply a default serialization value, so it should be present in the output.

[rpkilby]

To reiterate, I'd argue that it's not correct for allow_null to imply a default for required fields, but not for non-required fields. Having this distinction makes the serialization model a little more difficult to reason about, and having an additional check for read-only fields would only make this more complicated.

Barring some reason for the distinction, I'm inclined to partially revert #5639.

[RomuloOliveira]

@ticosax What was the behaviour before the 3.7.2 version? I've opened the #5639 because the #5518 brought backwards incompatible changes that broke code in an application of mine.

I'm not sure if the compatibility was broken at #5639 or it's a confusion with implicit values, but if it was broken at it, I think it is ok to reversed it.

I've provided more details of the why here.

[ticosax]

@RomuloOliveira After #5639 we are unable include fields with nullable=True, because they are also read-only.

[rpkilby]

@ticosax Are these fields automatically generated by a ModelSerializer, or are you manually declaring these fields? If declared, I think you should be able to leave off allow_null on the read_only field. That said, It would help if you could provide an example serializer and initial data/instance.

[ticosax]

Are these fields automatically generated by a ModelSerializer, or are you manually declaring these fields?

declared.

If declared, I think you should be able to leave off allow_null on the read_only field.

eventually. it doesn't change anything though. SkipField() is still raised.

It would help if you could provide an example serializer and initial data/instance.

Sorry, I don't see how I could do better than the regression test this PR is adding. IMO there is everything you ask.

[carltongibson]

@rpkilby We've had a sequence of little changes here. Each time it's not been exactly clear what the supported use-cases and behaviours are. I think we need to review the history, write it up in black and white (for us and the docs) and make at most one more change to resolve this.

Do you have capacity to go over the history here and draw up an overview? If so that would be awesome. If not I'm happy to take that on.

[rpkilby]

@carltongibson Yep, I'd be happy to take that on.

[blueyed]

Some findings in this regard:

1. _writable_fields includes fields which have a non-empty default (this is different from None):

   django-rest-framework/rest_framework/serializers.py

   Lines 367 to 372 in 522d453

   	@cached_property
   	def _writable_fields(self):
   	return [
   	field for field in self.fields.values()
   	if (not field.read_only) or (field.default is not empty)
   	]

   
   This causes a problem with a field like the following, where allow_null and default are added for recent changes in DRF to keep the field showing up:

class Serializer(DefaultBaseHyperlinkedModelSerializer):
    phone_numbers = serializers.JSONField(
        source='profile.phone_numbers',
        read_only=True,
        # Because of https://github.com/encode/django-rest-framework/pull/5518
        allow_null=True,
        # XXX: causes field to show up in _writable_fields.
        default=None,
    )

e37619f added tests for this, but changed something else.

It basically causes serializer.data['phone_numbers'] = None to be turned into
('profile', {'phone_numbers': None}) with serializer.validated_data.

The error then is:

  File "…/Vcs/django-rest-framework/rest_framework/serializers.py", line 210, in save
    self.instance = self.update(self.instance, validated_data)
  File "…/app/serializers.py", line 195, in update
    instance = super().update(instance, validated_data)
  File "…/Vcs/django-rest-framework/rest_framework/serializers.py", line 958, in update
    setattr(instance, attr, value)
  File "…/Vcs/django/django/db/models/fields/related_descriptors.py", line 216, in __set__
    self.field.remote_field.model._meta.object_name,
ValueError: Cannot assign "{'phone_numbers': None}": "User.profile" must be a "UserProfile" instance.

Not having read_only fields in _writable_fields seems like a sensible thing to change?!

2. I've found a fix for our use case by changing fields.get_attribute to return None if a FK lookup fails (similar to when ObjectDoesNotExist would be raised for m2m). See fields.get_attribute: return None for FK lookups #5727.

[rpkilby]

Thanks for the writeup @blueyed.

[blueyed]

I think this can be skipped/closed in favor of #5727 - at least that's what is working for us now.

[carltongibson]

Thanks again @blueyed.

I'm leaving this to @rpkilby at the moment, as he's looking into the wider issue as per the discussion above. We'll resolve shortly.

[carltongibson]

Closing this in favour of #5888. Thanks for the input @ticosax!