Possible IntegrityError even with UniqueValidator

[cancan101]

Even if a Serializer field is using UniqueValidator (likewise for UniqueTogetherValidator) there is still a race where after the check is performed, a conflicting row may be added to the DB. Right now that results in a 500 error with something like the following:

Exception Type: IntegrityError at /api/xxx/
Exception Value: UNIQUE constraint failed:  ...

I suggest in these cases DFF return a 409 with a generic error message. At the very least there should be some opt in behavior to get this.

I do think this logic should be handled by the framework rather than by the application.

[johnraz]

I do agree with the 500 not being what we would expect, but isn't a 400 with the correct validation error more appropriate?
Also, I guess there is a strongly valid reason why this is done the way it is and not handled by the framework (maybe because some database backend don't have unicity or something?)

[cancan101]

I prefer 409 here, though certainly a 400 is still "correct":

409 Conflict
Indicates that the request could not be processed because of conflict in the request,...

I am not sure what the concern about other backends would be? if they didn't raise an error even in the case of a conflict, then there is nothing to catch.

[johnraz]

I was just fishing about the backends it may be a totally different reason.

To me a race condition raising a unique constraint exception or a UniqueValidator check raising a ValidationError should blindly return the same error because in the end they are the same, and so far exception raised by UniqueValidator are 400 so I'd keep them as 400. Also you could end up with other fields failing which would defeat the 409.

Again I'm not sure what are the constraints are here as the UniqueValidator always looked odd to me and I always wondered why the db backend exception was not used instead.

[cancan101]

Ah ok. If 400s are raised now then sure. For some reason I thought 409s were being raised, but you are right. (and now I recall this discussion]).

Again I'm not sure what are the constraints are here as the UniqueValidator always looked odd to me and I always wondered why the db backend exception was not used instead.

See: https://groups.google.com/forum/#!topic/django-rest-framework/d8LV38uFW_I/discussion

[johnraz]

Ha sweet! That's what I was looking for so yes, this seems to be a tricky use case...
All in all I guess a 409 is better than a 500 in this case as it gives more context on what the error really was.
But as @xordoquy explains in the above discussion, this is something that should be handled by a custom serializer, so well I guess this is a nice to have.

[xordoquy]

I'm reluctant to bring in complexity in favor of a theoretical - yet possible - use case.
Nobody raised the issue based on a real case.

[cancan101]

In addition to handling the possible race, having the framework handle the errors coming back from the DB also make it easier for the user to remove the validators in the serializers / fields to improve performance but removing the extra query.

Right now the user then needs to handle the exceptions being raised by the DB rather than having the framework deal with it.

[tomchristie]

I'd be interested to see this replicated locally if possible, as that'd make it easier to more thoroughly verify a fix - any idea how easy/difficult it is to trigger, and if we have any/many sites that'd come across this in production?

[xordoquy]

@tomchristie that should be fairly easy to reproduce.

• have two serializer instances to create the same user
• perform is_valid on both
• call save on both

First should be saved, second would raise a DB error.
This being said, I think the exception is linked to the DB - this needs to be checked.

[tomchristie]

I was more meaning replicate the issue given an actual view & incoming requests. Mocking out a non-real-world example may be handy tho'.

[xordoquy]

It is nearly impossible to reproduce this kind of issue without relying on some luck.
As I mentioned earlier, it's really unlikely to happen as it hasn't been reported yet from a concrete use case.

[tomchristie]

Any similar issue raised against ModelForm in Django's tickets, perhaps?

[johnraz]

I guess setting up a view with a sleep in it and issuing two concurrent api calls with the same value for the unique field should make it no?

edit: the sleep should occur after the serializer validation.

[tomchristie]

That'd be interesting to see for a start, yes. Tho still useful to know at what point you need to get to in load until this starts to become viable. And Eg. what status have the Django Core team attached to the same issue with ModelForm? Closed as wontfix, or something else?