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

Uniqueness validators should not be run for excluded (read_only) fields #2848

Closed
ntucker opened this issue Apr 21, 2015 · 14 comments · Fixed by #4192
Closed

Uniqueness validators should not be run for excluded (read_only) fields #2848

ntucker opened this issue Apr 21, 2015 · 14 comments · Fixed by #4192
Labels
Milestone

Comments

@ntucker
Copy link

ntucker commented Apr 21, 2015

get_uniqueness_extra_kwargs and get_unique_together_validators both rely on Django's model unique_together specification. However, if read_only is set for both they should not be expected to be validated as normal. This reflects the behavior of django forms (exclude the field in form and it doesn't validated against this). This allows unique_together fields to be autogenerated in the save method of the model.

@tomchristie
Copy link
Member

tomchristie commented Apr 21, 2015

Presumably if either is read_only, then we shouldn't validate them? (Ie. same as excluding one?)

@ntucker
Copy link
Author

ntucker commented Apr 21, 2015

This is how django does it: https://github.com/django/django/blob/master/django/forms/models.py#L344 (build exclusion list)

Then here: https://github.com/django/django/blob/a10b4c010ab2cdaa6ba8bfaec3e3540299ea77be/django/db/models/base.py#L926
for name in check:
# If this is an excluded field, don't add this check.
if name in exclude:
break

So It looks like you are correct, a uniquetogether check is not done if ANY of the fields are missing

@ntucker
Copy link
Author

ntucker commented Apr 24, 2015

I can write a PR for this as long as the design is accepted. I just hate writing stuff that won't get approved.

@tomchristie
Copy link
Member

tomchristie commented Jun 25, 2015

I'm broadly in favor of this, but I don't think we can make any definite decisions without seeing an example fix and test case.

Labeling this as a valid bug to address tho.

@xordoquy xordoquy modified the milestone: 3.1.4 Release Jul 8, 2015
@xordoquy xordoquy modified the milestones: 3.1.4 Release, 3.1.5 Release Jul 16, 2015
@tomchristie tomchristie modified the milestones: 3.1.5 Release, 3.2.1 Release Jul 30, 2015
@tomchristie tomchristie modified the milestone: 3.2.1 Release Aug 7, 2015
@krisfields
Copy link

krisfields commented Sep 28, 2015

Uniqueness validators should also not be run if required=False. I have a serializer (Room) with required=False set on a nested user serializer, but it fails with a "This field is required" because user is part of a unique_together requirement on the Room model. Is there a workaround for now to exclude this validation on the serializer? For now, I've removed the unique_together requirement, but that's obviously less than ideal.

@xordoquy xordoquy added this to the 3.3.0 Release milestone Sep 28, 2015
@xordoquy
Copy link
Collaborator

xordoquy commented Sep 28, 2015

This use case is more complex. You want to run the uniqueness constraint when both have a value though not if only one is provided.

@philippeluickx
Copy link

philippeluickx commented Oct 13, 2015

+1.
Both for read_only=True or required=False
When both have a value: you want uniqueness. When only one or none have a value: no constraints.

Behaviour today is that they are both blocked as required when uniqueness is defined, even when the fields are read_only=True or required=False.
(I presume the same should happen if only 1 of the fields is required and the other not?)

@xordoquy
Copy link
Collaborator

xordoquy commented Oct 13, 2015

required=False will be "overridden" by the UniqueTogetherValidator.
I start feeling there are too many possible use cases. What if one field is not required, what if one is read only, what if we have both constraints and what if....
I'm not even sure what I should think about a UniqueTogetherValidator with one field having a read only constraint on top of it.
Let's rather use a custom UniqueTogetherValidator or use business logic level validation and make the documentation more clear about what the UniqueTogetherValidator does.
@tomchristie thoughts on that one ?

@philippeluickx
Copy link

philippeluickx commented Oct 14, 2015

It comes down to decide which constraint takes precedence?
If both are not required but have uniqueness, they might both be blank (thus technically violating the uniqueness if this happens more than once)

http://stackoverflow.com/questions/5772176/django-unique-together-and-blank-true

Checking on the behaviour of Django, it seems like the uniqueness takes precedence, as it is enforced on DB level.

What I would at least suggest is to handle the exception more gracefully. Now if you have a field that you put required=False but with uniqueness, you get the message "this field is required".

@xordoquy
Copy link
Collaborator

xordoquy commented Oct 14, 2015

It comes down to decide which constraint takes precedence?

It comes down to how to handle the most common use cases and easily allow developers to deal with edge cases. To me, it's very similar to the work done on writable nested serializers.

Raising warnings if conflicting constraints are found could be a nice way to make the developers aware that they might be hitting a wall.

@ntucker
Copy link
Author

ntucker commented Oct 14, 2015

I strongly recommend following Django's behavior. Partially for dev sanity, but mainly so unique_together fields can be autogenerated in the save method of the model.

If they are both read_only, then there is no point in validating because there is nothing to be sent from the wire. However, having validating fail, makes it so you cannot autogenerate values, which is very problematic.

@xordoquy
Copy link
Collaborator

xordoquy commented Oct 14, 2015

unique_together fields can be autogenerated in the save method of the model.

There's no way DRF can be aware of auto generated data at save time. This basically means the API will raise 500 errors because the unique_together constraint may be broken if the dev forgot to set the data.
This falls back to my previous comment.

@ntucker
Copy link
Author

ntucker commented Oct 14, 2015

There's no way DRF can be aware of auto generated data at save time.

Exactly. That's why DRF can't be imposing itself when you declare that you will be managing those fields yourself.

If you disagree, give me one scenario where read_only is set on a field, but DRF itself will save that field based on user input.

@xordoquy xordoquy modified the milestones: 3.3.2 Release, 3.3.3 Release Dec 14, 2015
@xordoquy xordoquy modified the milestones: 3.3.3 Release, 3.3.4 Release, 3.4.0 Release Feb 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants