Skip to content

Fix UniqueTogetherValidator with field sources#7086

Merged
lovelydinosaur merged 5 commits intoencode:masterfrom
rpkilby:unique-together-source
Dec 12, 2019
Merged

Fix UniqueTogetherValidator with field sources#7086
lovelydinosaur merged 5 commits intoencode:masterfrom
rpkilby:unique-together-source

Conversation

@rpkilby
Copy link
Contributor

@rpkilby rpkilby commented Dec 11, 2019 •
edited
Loading

This is an updated version of #7005 that I was helping @anveshagarwal put together. In short, there are two related bugs:

  • As pointed out by @anveshagarwal in Fix read_only + default unique_together validation. #5922 (comment), _read_only_defaults does not correctly account for the field source. The initial check does account for compatible sources, but when assigning the read only default, the field_name is used instead of source.
  • Generally, UniqueTogetherValidator doesn't handle field sources. Handling this is a little annoying, because the objective is to validate the serializer fields/data. However, the given attrs use the source/model field names, which are used in the actual validation check.

As to potential breaking changes, we may want to call out that validation against read-only fields that have defaults now obey their sources? ¯\_(ツ)_/¯

Fixes #7003

@rpkilby rpkilby marked this pull request as ready for review December 11, 2019 13:08
@rpkilby rpkilby mentioned this pull request Dec 11, 2019
Closed
@rpkilby rpkilby added this to the 3.11 Release milestone Dec 11, 2019
@carltongibson
Copy link
Collaborator

carltongibson commented Dec 11, 2019

Hey @rpkilby. Thanks for this. Tricky stuff. 🌶 Let me give it a look over. (But first glance it seems clean.)

carltongibson
carltongibson approved these changes Dec 11, 2019
View reviewed changes
Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

TBH, this looks great. I was worried it was going to be much dirtier than it is. That source just is the field name in the usual case is nice.

As to potential breaking changes, we may want to call out that validation against read-only fields that have defaults now obey their sources? ¯_(ツ)_/¯

Meh, this is clearly a bug. You can't be depending on the broken behaviour here. 😝

That the existing tests here pass is 👍 for me — We did exercise the difficult cases here (when we changed the old behaviour in 3.8(?)

@rpkilby
Copy link
Contributor Author

rpkilby commented Dec 11, 2019

I was worried it was going to be much dirtier than it is.

The initial implementation definitely was. At first, I didn't fully understand that the attrs was a map of {field source: value}, so there were a lot of messy code trying to work around handling field name vs source.

@rpkilby
Copy link
Contributor Author

rpkilby commented Dec 12, 2019

Expanded the writable field test to include validation for a missing value. This shouldn't extend to read-only fields, since their defaults are always provided.

@lovelydinosaur lovelydinosaur merged commit 236667b into encode:master Dec 12, 2019
@rpkilby rpkilby deleted the unique-together-source branch December 12, 2019 21:50
This was referenced Jan 13, 2020
Closed
Merged
@ihorm5
Copy link

ihorm5 commented Aug 14, 2020

I have encountered this issue when I was searching for a solution for exact same problem. It looks that this solution only works if you provide your own UniqueTogetherValidation in the serializer's meta validators field. While this is OK, without setting

        validators = [
            UniqueTogetherValidator(
                queryset=Model1.objects.all(),
                fields=['f1', 'f2']
            )
        ]

The code will raise KeyError. Is this the desired behavior?

@rpkilby
Copy link
Contributor Author

rpkilby commented Aug 17, 2020

Hi @fatalispm. I think you're running into #7143, which hasn't been released yet.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
@rpkilby
Fix UniqueTogetherValidator with field sources (encode#7086)
9fc8963 
* Add failing tests for unique_together+source

* Fix UniqueTogetherValidator source handling

* Fix read-only+default+source handling

* Update test to use functional serializer

* Test UniqueTogetherValidator error+source
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
@rpkilby @sigvef
Fix UniqueTogetherValidator with field sources (encode#7086)
2704e6d 
* Add failing tests for unique_together+source

* Fix UniqueTogetherValidator source handling

* Fix read-only+default+source handling

* Update test to use functional serializer

* Test UniqueTogetherValidator error+source
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carltongibson carltongibson carltongibson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.11 Release

Development

Successfully merging this pull request may close these issues.

UniqueTogetherValidator is incompatible with field source

4 participants

@rpkilby @carltongibson @ihorm5 @lovelydinosaur