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 UniqueTogetherValidator with field sources #7086

Merged
merged 5 commits into from Dec 12, 2019

Conversation

@rpkilby
Copy link
Member

@rpkilby rpkilby commented Dec 11, 2019

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 #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 requested a review from carltongibson Dec 11, 2019
@rpkilby rpkilby marked this pull request as ready for review Dec 11, 2019
@rpkilby rpkilby added this to the 3.11 Release milestone Dec 11, 2019
@carltongibson
Copy link
Collaborator

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

Copy link
Collaborator

@carltongibson carltongibson left a comment

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

@rpkilby 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
Member Author

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

@tomchristie tomchristie merged commit 236667b into encode:master Dec 12, 2019
1 check passed
@rpkilby rpkilby deleted the unique-together-source branch Dec 12, 2019
@fatalispm
Copy link

@fatalispm fatalispm 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
Member Author

@rpkilby 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 issue Nov 17, 2020
* 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants