Fix read_only + default unique_together validation. by carltongibson · Pull Request #5922 · encode/django-rest-framework (original) (raw)

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

Already on GitHub?Sign in to your account

Conversation9 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

In order to validate a unique_together relation on a read_only field, you need to specify a default on the field to be used for validation purposes.

e.g.:

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

Where owner is part of a unique_together relation on the model.

#5886 unwittingly broke this behaviour:

• The UniqueTogetherValidator was not added to the serialiser.
• Nor was the default value used in validation.

This PR:

• Adds a test case covering the expected behaviour. (That passes on 3.7.7 but fails on 3.8.0)
• Ensures the UniqueTogetherValidator is correctly generated
• Uses the default value for read_only fields (where specified) only for validation, in line with the changes for Alter read_only+default behaviour #5886.

@akx Any chance you could install this branch and see if your problem is resolved? Thanks.

@xordoquy @rpkilby: If you have capacity your eye over this would be helpful. Thanks.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, looks pretty reasonable to me.

v3.8.2 is now available on PyPI. Thanks all.

vdel mentioned this pull request

Oct 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are using a field with field_name different from the field in model that is used as source then this is failing and it should be

defaults[field.source] = default

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somehow missed your comment on this, but you're correct. The code above specifically checks for a compatible field source, but uses the serializer field_name instead of the source model field.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had reported this and tried to work on it, but couldnt continue
and along with this issue other issue is there defined in the Issue below
#7005
#7003

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anveshagarwal. I'm currently following up on that PR. That's what prompted my comment. 😄

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity's sake, my reasoning above was slightly wrong. The source checks are an indicator that we should be operating on field.source instead of field.field_name, however the way to determine this is by looking at run_validation. Before it calls run_validators (then _read_only_defaults), it calls to_internal_value. At this point, the data has been converted to its source structure. So, it makes sense that the read-only defaults would also map to its source structure, instead of the raw field names.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request

Nov 17, 2020

• Add test for read_only + default unique_together validation.
• Fix read_only + default validation