Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alter read_only+default behaviour #5886

Merged
merged 2 commits into from Mar 20, 2018

Conversation

carltongibson
Copy link
Collaborator

@carltongibson carltongibson commented Mar 20, 2018

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 sourcedefault 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:

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
Copy link
Collaborator Author

carltongibson commented Mar 20, 2018

@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.

@carltongibson carltongibson added this to the 3.8 Release milestone Mar 20, 2018
@ticosax
Copy link
Contributor

ticosax commented Mar 20, 2018

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
Copy link
Collaborator Author

carltongibson commented Mar 20, 2018

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
Copy link
Contributor

ticosax commented Mar 20, 2018

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
Copy link
Member

tomchristie commented Mar 20, 2018

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.

In this context (without mentioning `save`) now slightly misleading.
carltongibson added a commit that referenced this issue Mar 20, 2018
@carltongibson carltongibson merged commit c2b24f8 into encode:master Mar 20, 2018
1 check passed
@carltongibson carltongibson deleted the 38/read_only+default branch Mar 20, 2018
carltongibson added a commit that referenced this issue Mar 26, 2018
carltongibson added a commit that referenced this issue Apr 3, 2018
@akx
Copy link
Contributor

akx commented Apr 5, 2018

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
Copy link
Collaborator Author

carltongibson commented Apr 5, 2018

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

class TestUniquenessTogetherValidation(TestCase):

It has a case for read-only fields:

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
Copy link
Contributor

akx commented Apr 5, 2018

@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
Copy link
Collaborator Author

carltongibson commented Apr 5, 2018

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

@akx
Copy link
Contributor

akx commented Apr 6, 2018

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

@carltongibson
Copy link
Collaborator Author

carltongibson commented Apr 6, 2018

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

@tiltec
Copy link
Contributor

tiltec commented Jun 5, 2018

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
Copy link
Collaborator Author

carltongibson commented Jun 5, 2018

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

@blueyed
Copy link
Contributor

blueyed commented Jun 19, 2018

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
Copy link
Collaborator Author

carltongibson commented Jun 19, 2018

@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
Copy link

mikelane commented Nov 2, 2018

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
Copy link
Collaborator

xordoquy commented Nov 2, 2018

This was nearly the headline of the 3.8 release notes.

@rpkilby
Copy link
Member

rpkilby commented Nov 2, 2018

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.

qeternity pushed a commit to qeternity/django-rest-framework that referenced this issue Nov 21, 2018
@johnmatthewtennant
Copy link

johnmatthewtennant commented Apr 1, 2019

@rpkilby
Copy link
Member

rpkilby commented Apr 1, 2019

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
Copy link

johnmatthewtennant commented Apr 1, 2019

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 rpkilby mentioned this pull request Apr 2, 2019
@rpkilby
Copy link
Member

rpkilby commented Apr 2, 2019

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

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this issue Nov 17, 2020
* Always exclude read_only fields from _writable_fields

* Remove `read_only` from `CreateOnlyDefault` example.
      In this context (without mentioning `save`) now slightly misleading.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants