Allow option for conflicts in validation to result in 409, not 400.

[foresmac]

I know this was brought up before, but I have to disagree with the rationale that otherwise valid input that fails because of a unique constraint should return a 400 Bad Request.

The RFC cited in the above thread notes that 400 should be reserved for malformed syntax; i.e. keys that aren't used or values that aren't the correct format. A 409 suggests a conflict, i.e. though an email address may be properly formatted, it conflicts with an existing entry in the database because that particular field is required to be unique. While agree with the idea that attempting to POST to create an entity with conflicting data is a "bad request", 409 is a more specific error code to return for a conflict to to uniqueness constraints.

Regardless of that issue, however, is the fact that 409 is a de facto standard response when otherwise valid input conflicts with uniqueness constraints. While we could simple override every HTTP method on every view class that would require us to return 409s instead of generic 400s for uniqueness checks, we would also have to override our serializers, resulting in more and more complex code when in all other respects we would be able to use the existing generic API views with little to no modifications. The other solution—maintaining our own fork of DRF—is even less palatable.

If some DRF users prefer the 400 only responses for data validation, perhaps a solution to to offer a way to set whether 400s or 409s are returned for conflicts.

[tomchristie]

If some DRF users prefer the 400 only responses for data validation, perhaps a solution to to offer a way to set whether 400s or 409s are returned for conflicts.

Yup, this. With 3.0 the handling of response style for validation errors will be down to the exception handler. I'm not clear on how you'd best deal with differentiating conflicts from other validation errors within the serializer API, but it'd certainly be easier that overriding the view code.

Does that sound sufficient to resolve this from your point of view?

[foresmac]

Our office has some dedicated hack days coming up; let me know if there's an area you'd like us to look at to help contribute to a solution.

[foresmac]

If it's built in to the serializer API, this is how I'd imagine doing it:

Either set a global config, so CONFLICT_ERROR_409 = True, or a on serializer-by-serializer basis with something like this: name = CharField(max_length=50, unique=True, conflict_409=True).

As far as I can tell, users that want a 409 returned want to do so on all DB conflicts (in Django, this means getting an IntegrityError when trying to save a model object that fails uniqueness). But this way allows individual overrides if needed.

Thoughts?

[foresmac]

(My preference is for a global config; that suits our use case.)

[xordoquy]

I'd rather have it handled by either the renderers and the exception handler.

[tomchristie]

Suggest further discussion of this once we've got some release notes for 3.0, as the validation changes there impact (and to my mind, resolve) this issue.

[foresmac]

@tomchristie I've got about 16 hours I can commit to helping with this issue (or possible something else DRF-related) next Thurs-Fri. Do let me know how I can help.

[foresmac]

Ok, thanks to @tomchristie I've tracked down where exactly this issue comes from:
https://github.com/django/django/blob/1101467ce0756272a54f4c7bc65c4c335a94111b/django/forms/models.py#L380

Effectively, by default Django sets _validate_unique to True for ModelForms by default. The only real way to fix this is to override Serializer.full_clean() to somehow override this behavior. The comments in the code suggest one possible method:

   # self._validate_unique will be set to True by BaseModelForm.clean().
   # It is False by default so overriding self.clean() and failing to call
   # super will stop validate_unique from being called.

[foresmac]

Ok, so the easy solution is to pass validate_unique=False when calling Django's <model_instance>.full_clean(). That keeps uniqueness checks from ending up in serializer.errors. The hard problem now is to:

1. Do uniqueness checks separately and store them in serializer.conflicts

2. Switch is_valid() to something like this:

   def is_valid(self):
       return not self.errors and not self.conflicts

3. update docs to note to check for serializer.conflicts and return something like Response(serializer.conflicts, status=status.HTTP_409_CONFLICT)

4. Make it somehow optional, so that conflicts can either be reported separately or in errors as they have been.

[foresmac]

Also noting that it should be possible to update the generic view classes to automatically do this; with the setting defaulting to the existing behavior, existing users will have to do nothing and everything will work as expected. Those that want a 409 for conflicts can set one setting and be done with it.

[tomchristie]

Those that want a 409 for conflicts can set one setting and be done with it.

This isn't quite what I'm expecting as the end result if this. It's enough for me that you're able to customize your serializers and/or exception handler in order to cater for this use case.

Personally I'm still if the opinion that choosing either 400 or 409 for all validation errors is perfectly adequate. Behaviourally that's enough info for the client. A validation error is just that - irrelevant if it was caused due to a DB constraint or a code constraint.

[foresmac]

If I understand correctly, DRF3 doesn't even call full_clean anymore, so, as you suggested earlier this may be a moot point.

This change also means that we no longer use the .full_clean() method on model instances, but instead perform all validation explicitly on the serializer.

We may still not be able I use ModelSerializer as I expect this would still perform unique constraint validation by default, or at least would still normally collect that and return it as a validation error. At the very least we should be able to write explicit Serializers and easily handle the 409 when create raises IntegrityError.

I'd still prefer a mechanism to handle this automatically with generic views, but I see that a) DRF is never going to do that, and b) my boss is never going to accept returning 400 for DB conflicts.

[tomchristie]

as I expect this would still perform unique constraint validation by default

It does, yes. The uniqueness validation in 3.0 is enforce by validators, rather than using full_clean.

See: https://github.com/tomchristie/django-rest-framework/blob/version-3.0/docs/topics/3.0-announcement.md#the-uniquevalidator-and-uniquetogethervalidator-classes

Firstly, braindump of various possible approaches...

• Subclass UniqueValidator and UniqueTogetherValidator into new validator classes which you'd use instead of the default ones. They could raise a custom UniquenessValidationError which you'd then handle in a custom error handler to return 409s. Alternatively they could set state on the serializer instance, with self.field.root.conflict = True if validation fails.
• Simply don't enforce the uniqueness on the serializer classes at all, and handle IntegrityError as a 409.
• Check for hardcoded message strings in the error handler to determine if you want to use 400 or 409.
• Subclass run_validators on the serializer, marking some "conflict" state onto the serializer instance if the failing validator is an instance of UniqueValidator or UniqueTogetherValidator.

How I would prob tackle this if I really wanted to be able to use standard ModelSerializers and the default generic views.

• Subclass ModelSerializer into a new ConflictAwareModelSerializer base class.
• Subclass run_validators on the class, marking some "conflict" state onto the serializer instance if the failing validator is an instance of UniqueValidator or UniqueTogetherValidator.
• Subclass is_valid, raising a ConflictError if raise_exception is set, self.conflict is True and there are errors.
• Handle ConflictError in a custom exception handler.

Little bit of work there, but end result would be a serializer that you could use exactly like the existing ModelSerializer and without adapting the class based views in any way.

[cancan101]

@tomchristie What ever came of your braindump in the most recent comment? It looks like 400s are raised right now with no easy way of changing that out. Further I think some of the issues you raise about DB errors are still present in #3876.

[tomchristie]

No further work here to discuss. Somewhat related to #3775.

[veeraipuvi]

how to i fix it

[auvipy]

how to i fix it

didn't #4550 fix it?