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

Fix read_only + default unique_together validation. #5922

Merged

Conversation

carltongibson
Copy link
Collaborator

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.

@carltongibson carltongibson added this to the 3.8.2 Release milestone Apr 6, 2018
@carltongibson
Copy link
Collaborator Author

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

@akx
Copy link
Contributor

akx commented Apr 6, 2018

@carltongibson Yup, seems to work. 🏆

Copy link
Member

@tomchristie tomchristie left a comment

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.

@carltongibson carltongibson merged commit 42eb5a4 into encode:master Apr 6, 2018
@carltongibson carltongibson deleted the 382/read_only-default-validation branch April 6, 2018 13:20
@carltongibson
Copy link
Collaborator Author

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

default = field.get_default()
except SkipField:
continue
defaults[field.field_name] = default

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

Copy link
Member

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

Copy link
Member

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

Copy link
Member

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

5 participants