Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Alter read_only+default behaviour#5886

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@carltongibson
Copy link
Collaborator

@carltongibsoncarltongibson commentedMar 20, 2018
edited
Loading

Since ≈foreverread_only fields with adefault have set that default invalidated_data on create and update operations.

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

#5375 resolved theserialisation issues with dottedsourcedefault will be used if any step in the chain isNone — but revealed a latent issue fordeserialisation when usingread_only plus adefault. This comes up in a couple of ways:

Both of these cases come up because thedefault meansread_only fields end up in_writable_fields.

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

This PR adjusts the_writable_fields check toalways excluderead_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 theread_only fields with awritable default tovalidated_data, probably by overridingsave.

classMySerializer(serializers.ModelSerializer):user=serializers.PrimaryKeyRelatedField(read_only=True,default=CreateOnlyDefault(CurrentUserDefault()))    ...defsave(self,**kwargs):"""Include default for read_only `user` field"""kwargs["user"]=self.fields["user"].get_default()returnsuper().save(**kwargs)

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

Note thatin 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

blackary and stevanmilic reacted with thumbs up emoji
@carltongibson
Copy link
CollaboratorAuthor

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

@carltongibsoncarltongibson added this to the3.8 Release milestoneMar 20, 2018
@ticosax
Copy link
Contributor

ticosax commentedMar 20, 2018
edited
Loading

Fixes#5708, by addingdefault=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
CollaboratorAuthor

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 bevery welcome.

@ticosax
Copy link
Contributor

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.

carltongibson reacted with thumbs up emoji

@lovelydinosaur
Copy link
Contributor

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 pull requestMar 20, 2018
@carltongibsoncarltongibson merged commitc2b24f8 intoencode:masterMar 20, 2018
@carltongibsoncarltongibson deleted the 38/read_only+default branchMarch 20, 2018 20:09
carltongibson added a commit that referenced this pull requestMar 26, 2018
carltongibson added a commit that referenced this pull requestApr 3, 2018
@akx
Copy link
Contributor

akx commentedApr 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.

Whenowner is passed in viaserializer.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
CollaboratorAuthor

carltongibson commentedApr 5, 2018
edited
Loading

Hi@akx.

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

Theunique_together validation ismeant to be preserved, but it may not be covered.

Relevant test case is in

classTestUniquenessTogetherValidation(TestCase):

It has a case for read-only fields:

deftest_ignore_read_only_fields(self):
"""
When serializer fields are read only, then uniqueness
validators should not be added for that field.
"""
classReadOnlyFieldSerializer(serializers.ModelSerializer):
classMeta:
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)
""")
assertrepr(serializer)==expected

So we may need to make an adjustment.

@akx
Copy link
Contributor

akx commentedApr 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
CollaboratorAuthor

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

@akx
Copy link
Contributor

akx commentedApr 6, 2018

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

@carltongibson
Copy link
CollaboratorAuthor

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

@tiltec
Copy link
Contributor

Would it be worth adding a note tohttp://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.

Thedefault argument is still necessary for having the value present at validation time, but additionally the value needs to be passed viaserializer.save().

@carltongibson
Copy link
CollaboratorAuthor

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

@blueyed
Copy link
Contributor

I wonder if overridingcreate 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
CollaboratorAuthor

@blueyed: Yes — that's one way. Either, in the view with theperform_create hook, passing theuser tosave(), or in the serialiser overridingsave() orcreate() orupdate() 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

It was cool that I got to spend a day-and-a-half trying to figure out why I was getting apsycopg2.IntegrityError just because I upgraded fromDRF 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
Contributor

This was nearlythe headline of the 3.8 release notes.

@rpkilby
Copy link
Contributor

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 pull requestNov 21, 2018
@johnmatthewtennant
Copy link

@rpkilby
Copy link
Contributor

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.

blueyed reacted with thumbs up emoji

@johnmatthewtennant
Copy link

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 valuewill still be passed through to the validated_data."

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

adamwirth and iankpconcentricsky reacted with confused emojiaadu reacted with eyes emoji

@rpkilbyrpkilby mentioned this pull requestApr 2, 2019
@rpkilby
Copy link
Contributor

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 pull requestNov 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@lovelydinosaurlovelydinosaurAwaiting requested review from lovelydinosaur

@xordoquyxordoquyAwaiting requested review from xordoquy

@jpadillajpadillaAwaiting requested review from jpadilla

@kevin-brownkevin-brownAwaiting requested review from kevin-brown

@rpkilbyrpkilbyAwaiting requested review from rpkilby

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.8.0 Release

Development

Successfully merging this pull request may close these issues.

10 participants

@carltongibson@ticosax@lovelydinosaur@akx@tiltec@blueyed@mikelane@xordoquy@rpkilby@johnmatthewtennant

[8]ページ先頭

©2009-2025 Movatter.jp