Alter read_only+default behaviour

[carltongibson]

Since ≈forever read_only fields with a default have set that default in validated_data on create and update operations.

This has led to issues as we have tried to Fix default value handling for dotted sources #5375: uses such as source=a.b.c, default='x' where e.g. b may be null were returning None, rather than the default x.

#5375 resolved the serialisation issues with dotted source — default will be used if any step in the chain is None — but revealed a latent issue for deserialisation when using read_only plus a default. This comes up in a couple of ways:

• The check for nested writes errors on a read_only field with dotted source and default. _writable_fields in Serializer retrieves read only fields #4666. (Exact reproduce pending on this one.)
• Skipping that, validated_data ends up including a nested dict, which cannot be assigned in e.g. places where a model object is expected. not required read_only Fields with allow_null #5708 (comment)

Both of these cases come up because the default means read_only fields end up in _writable_fields.

Whilst we've been doing it ≈forever, it's still a Que? here. How on earth is a read_only field writable?

This PR adjusts the _writable_fields check to always exclude read_only fields.

---

This is a backwards incompatible change.

The previous use-case was for e.g. setting the current user as the owner of a object on create.

This change will require an extra workaround — to explicitly add the read_only fields with a writable default to validated_data, probably by overriding save.

class MySerializer(serializers.ModelSerializer):
    user = serializers.PrimaryKeyRelatedField(read_only=True, default=CreateOnlyDefault(CurrentUserDefault()))
    ...
    
    def save(self, **kwargs):
        """Include default for read_only `user` field"""
        kwargs["user"] = self.fields["user"].get_default()
        return super().save(**kwargs)

Possibly we could add some convenience to this? (I'm a bit loathe to add API here.)

Note that in the tutorial we demonstrate this kind of this at the view level:

def perform_create(self, serializer):
    serializer.save(owner=self.request.user)

---

My overall take here is that the previous use-case saves a line or two, and is a convenience, but it inhibits correctness with the dotted source issue, so I think we need to swallow the pill on this one and make the change.

---

TODO:

• Release notes for BC change and workaround.
• Docs changes

[carltongibson]

@encode/django-rest-framework-core-team Can I ask for input on this please? Breaking BC for a long-standing behaviour isn't top of my list of things to do.

Thanks.

[ticosax]

Fixes #5708, by adding default=None to the Field definition.
@carltongibson For what it worth, BC has been broken since 3.7.4 for us already. So I would be relieved if we break it again so we can use DRF upstream.

[carltongibson]

Hi @ticosax. #5708 is on my radar here. There's a little bit of unpicking to do to get to it directly though. (There are a few related points.)

Following this I'm expecting now that we will revert #5639.

I'm then hoping that we've pinned down the whole nest of issues here.

To wit: any test cases you think are missing, anywhere in this ballpark would be very welcome.

[ticosax]

I think I made a mistake in my previous comment. there is no relation between the fixed behavior in our test suite and this PR. sorry. I will try to isolate better the changes.

[tomchristie]

Yup looks good to me, and I think it's more intuitive behaviour this way around.
Good sign that there's only that one test case that needs tweaking.

[akx]

With this change, what is the correct way to do unique-together validation where one of the fields is a read-only field?

In my case, (owner, name) are unique-together, and owner was previously set implicitly via

    owner = serializers.PrimaryKeyRelatedField(
        read_only=True,
        default=serializers.CreateOnlyDefault(serializers.CurrentUserDefault()),
    )

but that field is now ignored.

When owner is passed in via serializer.save(...), validation has already run and there are no (owner=None, name='...') duplicates even if there might be one when the owner is actually set, and so the view crashes with a database integrity error (500 Infernal Server Terror).

[carltongibson]

Hi @akx.

Can you put that into a minimal test-case please?

The unique_together validation is meant to be preserved, but it may not be covered.

Relevant test case is in

django-rest-framework/tests/test_validators.py

Line 166 in bc35345

class TestUniquenessTogetherValidation(TestCase):

It has a case for read-only fields:

django-rest-framework/tests/test_validators.py

Lines 260 to 278 in bc35345

	def test_ignore_read_only_fields(self):
	"""
	When serializer fields are read only, then uniqueness
	validators should not be added for that field.
	"""
	class ReadOnlyFieldSerializer(serializers.ModelSerializer):
	class Meta:
	model = UniquenessTogetherModel
	fields = ('id', 'race_name', 'position')
	read_only_fields = ('race_name',)
	serializer = ReadOnlyFieldSerializer()
	expected = dedent("""
	ReadOnlyFieldSerializer():
	id = IntegerField(label='ID', read_only=True)
	race_name = CharField(read_only=True)
	position = IntegerField(required=True)
	""")
	assert repr(serializer) == expected

So we may need to make an adjustment.

[akx]

@carltongibson Sure thing. Here you are: https://github.com/akx/drf_unique_together_testcase

I get error 400, as expected, on DRF 3.7.x, and an IntegrityError on DRF 3.8.x.

[carltongibson]

@akx Awesome thanks. I'll have a look.

[akx]

@carltongibson Should I create an issue for this, erm, issue, for better tracking?

[carltongibson]

@akx I'm hoping #5922 resolves this. Please give it a run. 🙂

[tiltec]

Would it be worth adding a note to http://www.django-rest-framework.org/api-guide/validators/#advanced-field-defaults where it says

Using a standard field with read_only=True, but that also includes a default=… argument. This field will be used in the serializer output representation, but cannot be set directly by the user.

The default argument is still necessary for having the value present at validation time, but additionally the value needs to be passed via serializer.save().

[carltongibson]

Yeah, maybe. It is mentioned in the unique_together docs, but calling it out further may have value.

[blueyed]

I wonder if overriding create on the Serializer is a good way to work around / fix this: https://github.com/lock8/django-rest-framework-jwt-refresh-token/pull/35/files#diff-84229b1d4af72d928beddfa32e80cc8eR23

[carltongibson]

@blueyed: Yes — that's one way. Either, in the view with the perform_create hook, passing the user to save(), or in the serialiser overriding save() or create() or update() as appropriate.

The key is that (because of the whole history here) we need the user to pass the value on save somewhere.

The question is how best to add that to the docs. (I don't think it'll need much.) You're the ones reading them: PRs welcome 🙂.

[mikelane]

It was cool that I got to spend a day-and-a-half trying to figure out why I was getting a psycopg2.IntegrityError just because I upgraded from DRF 3.7. Would maybe have been nice to have had an exception raised that said something like "Hi. We noticed that you've got a read-only field with a default value. We thought you might like to know that we changed how we handle such things. Here's a helpful link to our issues page on github. Have a nice day!"

:-\

[xordoquy]

This was nearly the headline of the 3.8 release notes.

[rpkilby]

Hi @mikelane. Contributions and suggestions are always welcome. If you have a minimal example where the behavior is suboptimal and what a helpful exception might have been, it would be appreciated. There's always a chance to improve the error handling and assertion guards.

[johnmatthewtennant]

The docs are misleading on this issue:
https://www.django-rest-framework.org/api-guide/validators/#using-with-a-read-only-date-field

[rpkilby]

Hi @johnmatthewtennant. Can you be more specific on what's misleading? Also, if you have the time, opening a PR with a documentation update would be helpful. If not, just open a separate issue so we don't lose track of the discrepancy.

[johnmatthewtennant]

The docs recommend

"If you want the date field to be visible, but not editable by the user, then set read_only=True and additionally set a default=... argument."

It then says:

"The field will not be writable to the user, but the default value will still be passed through to the validated_data."

This is not true. It will not be passed through to the validated_data.

[rpkilby]

Thanks - I opened a new issue pointing to your comment so this isn't forgotten.