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?

[tomchristie]

Going to close this along same lines as https://code.djangoproject.com/ticket/9581

If anyone is actually hitting this in production it'd be useful to know, but either way around I don't see it as likely that we'll do anything other than allowing the 5xx response by default - if you need something else, write some view code, and catch the exceptional case.

[johnraz]

Seems acceptable to me.

[BarrensZeppelin]

I'm only reviving this since you wrote that it'd be useful to know if someone was hitting this in production.
I have experienced this today. I was notified of two such IntegrityErrors by external users with the django mail_admin-system. After investigating I was able to reproduce the error by rapidly sending two identical POST requests to an endpoint using a serializer with a UniqueValidator.
However, I think this arises as a symptom of a problem with the Backend/DB connection.

[tomchristie]

However, I think this arises as a symptom of a problem with the Backend/DB connection.

Okay. If you get any further digging into that do shout out here. Useful for broader context.

[fildred13]

I've got this exact issue occurring on my company's production system, and can more or less reliably replicate by sending rapid, identical PATCH requests to one of our endpoints. A UniqueTogetherConstraint on a model is violated despite the UniqueTogetherValidator being in place.

I'm about to patch our serializer to just catch and handle the raised Integrity errors, transforming them to drf Validation errors instead.

Not the end of the world, but it does seem counterintuitive that UniqueTogetherValidator has a race condition which isn't mentioned in the docs. And to be consistent I need to parse the IntegrityError message and transform it to look just like the normal UniqueTogetherValidator message, which is non-trivial. Naively, I expected UniqueTogetherValidator to handle this.

[rpkilby]

Hi @fildred13. Ideally, #6172 will be merged in 3.11, which should fix the race condition.

[tomchristie]

@fildred13 Also worth confirming if you're using ATOMIC_REQUESTS = True? Seems like #6172 can still present in that case, but there's a whole set of cases where you can get integrity errors if you're not using transactions around requests.

[jnns]

We encountered this problem in production as well and we're not doing anything fancy in the save method that would increase the window between validation and saving to the database.

I was able to create a minimal verifiable example that you can clone here. The gist of it:

from django.db import models

class Entry(models.Model):
    slug = models.SlugField(unique=True)

http :8000/entries/ slug=hello & http :8000/entries/ slug=hello
HTTP/1.1 201 Created
{
    "slug": "hellow"
}

HTTP/1.1 500 Internal Server Error
IntegrityError at /entries/
UNIQUE constraint failed: my_app_entry.slug

This also happens with ATOMIC_REQUEST = True.

Would you consider this a case that should be handled by DRF?

[orientalperil]

@rpkilby Can you explain how #6172 relates to IntegrityError raised by unique constraint violation because of a race condition? From what I can see #6172 is about a different kind of race condition where serializers were referring to the wrong context because some code was not thread safe.