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 in UniqueTogetherValidator to allow it to handle querysets #2575

Closed
wants to merge 3 commits into from

Conversation

miki725
Copy link
Contributor

@miki725 miki725 commented Feb 19, 2015

this is especially important when using a serializer with many=True and the model having unique multiple constrains

example failure can be found at miki725/django-rest-framework-bulk#30

TODO

  • failure test
  • possibly more test cases

this is especially important when using a serializer with many=True and the model having unique multiple constrains
@tomchristie
Copy link
Member

Could we get this broken down into a simple example case?
Difficult to assess this as it currently stands.

@nikolaz111
Copy link

When a model has a unique_together constraint, the UniqueTogetherValidator assumes the object has a pk defined. If the serializer is handling multiple objects (many=True flag), the instance that is used by the UniqueTogetherValidator becomes a QuerySet and QuerySets don't have any pks.

@tomchristie
Copy link
Member

If the serializer is handling multiple objects (many=True flag)

Serializer level validators should absolutely apply per-instance, not ever at the queryset level.
This pull request won't quite be the right approach.
Possible to demonstrate the simplest possible case of this? Still need to verify to myself that the issue really is stated, and there's not something else going on here.

@miki725
Copy link
Contributor Author

miki725 commented Feb 20, 2015

Im not sure if this issue can be easily demonstrated in DRF by itself since ListSerializer does not define update() which is where the issue happens. I created a test case in DRF-bulk to test for this since there I do overwrite update() in ListSerializer. Here are some resources to look at.

  • the test - source. You can see travis failure here
  • ListSerializer.update() implementation - source. Notice that I use use a child serializer's update() implementation which in this case is implementation from ModelSerializer.
  • View.perform_bulk_update() - source. this simply calls serializer.save() so nothing special is going on.

The issue might be in DRF-bulk, specifically in ListSerializer.update() but I do not do anything special there so some pointer on fixing that would be appreciated. If the issue however is in DRF3 and this PR approach is not appropriate, what would be your recommendation?

EDIT: just realized that ListSerializer.update() is never called and that error happens during serializer.is_valid(). Guess I still need to wake up.... Tonight Ill send you a gist or something trying to reproduce an error in vanilla DRF3.

@tomchristie
Copy link
Member

since ListSerializer does not define update() which is where the issue happens.

Shouldn't need to if this is occuring during validation... it'll be triggered by "is_valid()" - you won't need to call ".save()"

@miki725
Copy link
Contributor Author

miki725 commented Feb 20, 2015

Here is example reproducing the error using vanilla DRF. you can disregard my morning comment dealing with DRF-bulk.

https://gist.github.com/miki725/1513401315f5b0fec670

In the example I am using a sample Django app from DRF-bulk tests but all information is enclosed so you should be able to reproduce it as well.

@tomchristie
Copy link
Member

So again, this pull request won't be quite the right approach.
Instead of handling querysets in the validation, we should ensure that the validator is applied per item, not at the list serialiser level.
Probably best to close this and open an issue to discuss the issue first rather than diving in with code changes.

@tomchristie
Copy link
Member

Closing based on previous comment. Very happy to discuss this in more fully as an issue if desired.

@tomchristie tomchristie closed this Mar 3, 2015
@miki725
Copy link
Contributor Author

miki725 commented Mar 3, 2015

Thanks for feedback. When Ill have a chance, Ill try to implement your approach.

@sassanh
Copy link
Contributor

sassanh commented Feb 27, 2017

@tomchristie seems like the validator still isn't applied per item. Should I create an issue for this or I'm wrong?

@xordoquy
Copy link
Collaborator

@sassanh as per comments here you may open a new issue to discuss that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants