Separate Validation of the Client Submission from Resource Dependent checks

[claytondaley]

I was trying to figure out how to return a 409 error on uniqueness issues. When researching the possibility, I found several closed issues (#1848, #4509) and the logically related issue #3381. I'd like to make the case that handling validation and uniqueness (technically any database-dependent validation) should occur in two stages.

1. As I mention in my response to #4509, I don't think it even makes sense to compare invalid client submissions to the database to discover resource-dependent validation issues.
2. Avoiding database calls when catching validation issues in the client's submission should improve performance.
3. Separating validation of the client submission from resource-dependent checks (like uniqueness) makes the detection of 409 errors much easier. Because 409 is "conflict with the existing resource", the pattern actually mirrors the spec.
4. (EDIT 11/2017) I recently ran into issue Unique constraint prevents nested serializers from updating #2996. Separating the two phases of validation could reduce the complexity of this issue. Client-related validation issues (i.e. 400 errors) could still be enforced, but uniqueness (i.e. 409) errors ignored. It's not a perfect solution as non-PK uniqueness issues and concurrency issues should prevent a commit.

Reason (1) is actually the root cause of #3381. The implemented solution (#4217) was to make uniqueness checks robust to validation-derived issues.

Reason (3) is actually the most relevant to me. Right now, the ONLY way to distinguish a uniqueness issue in the handle exception stage is to introspect error messages. To make things worse django hardcodes these messages in ways that cannot be used by the detection code. To muddy the waters further, the error I get for uniqueness on a single id column is (according to the inline documentation) part of a "unique together" check. I'm not sure if I'm misunderstanding the inline documentation, but it would be annoying if a future "fix" for this issue invalidated my 409 detection code.

If two-pass validation is acceptable, I suggest returning a subclass of ValidationError (e.g. ResourceValidationError) so existing error handlers are unaffected. However, folks wanting to support 409 can check for the subclass and alter behavior accordingly. Eventually, this could be implemented in the core error handler with a settings flag.

[claytondaley]

FWIW I believe this is also the pattern used in Django in several places (e.g. models and forms)
and clearly documented in the docs, likely for the same reasons.

[tomchristie]

Two questions initially:

1. What (if any) behavioural difference would you expect 400 vs 409 to have on the client side?
2. Am I understanding correctly that you'd only make uniqueness checks if and only if the other validations have already passed?

As I see it the two-phase behaviour would be worse not better from the user perspective, as we'd hide uniqueness failures if we're in the presence of also having another unrelated field with a validation failure.

I'm not 100% a "no" on this, but I don't have any interest in tackling it myself. Addressing it as an alternative Serializer base class or something similar would probably be the best course. Would be very happy to be able to provide different styles of behaviour, without us having to take that work on in the core package.

[claytondaley]

What (if any) behavioural difference would you expect 400 vs 409 to have on the client side?

From my response (to the same question) in #4509:

Concurrency is the "easy" answer, but here's another that's close to something we're actually doing:

• Application components are distributed between server A and server B
• Server A creates a message for a component on server B and stores it locally
• We have a component that pushes the message from storage on A to storage on B
• Assume we use a UUID as the primary key (same id on both sides) and push the messages over using a REST call
• I send a message and a network issue causes the DB commit to succeed but the "ack" to fail.
• I retry the message. If I get a 409, I know the message exists so I can mark it as transferred.
• If I get a 400, I need to retry. For example, I may have upgraded one side of the connection but not the other. The 400 error code is stating that the problem is intrinsic to the request an not its relation to the resource in the database.

To your second question:

Am I understanding correctly that you'd only make uniqueness checks if and only if the other validations have already passed?

Yes. Not just uniqueness but any resource-dependent checks like concurrency. Until the sender provides a valid request, comparing it to the database is unpredictable. It creates issues like #3381 which is currently addressed through workarounds like #4217. Do you really feel that the ideal behavior for a uniqueness validator is to handle a bunch of unrelated error cases?

On the flip side, running both concurrently doesn't guarantee that the resulting list of errors is exhaustive. For example, a value that doesn't validate can never fail a uniqueness tests. For backwards compatibility, a two-stage validation system can still accumulate all of the errors before raising anything -- except that we'd skip resource-dependent checks involving any field that already failed request-level validation (the ideal fix for #3381).

without us having to take that work on in the core package

I appreciate your interest in keeping the core slim, but this is part of the REST spec. Unlike the last time I reported a spec issue, the change proposed here is fully compatible with DRF's architecture. In fact, all it requires is that DRF use a pattern of validation that is already the best practice in the Django universe. It also eliminates the need for the existing workaround in #4217.

FWIW the right thing to do is to run the resource-dependent checks in a transaction with the save method. That would address the race condition noted in #3876 and other conceivable race conditions --
like you'd see in a DRF-level optimistic concurrency validator.

[claytondaley]

I can assemble a PR that includes the acceptable changes, but want to make sure the effort is not going to go to waste.

[tomchristie]

If you provide a pull request then we can make a judgement call on that - that's not something that we can do up front.

but want to make sure the effort is not going to go to waste.

In which case the sensible thing to do would be to do the work as a third party package instead. If there's clear uptake, or if this issue is repeatedly re-raised then we could consider adopting it.

I've not hesitation in agreeing that 409 for uniqueness errors might be marginally nicer, but "it's in the REST spec" is over-playing it - there's nothing in the HTTP specification that precludes using 400 for those types of validation error, and it's also still unclear to me what status code you'd want to expect in the case where you have some field with value validation errors, and other fields with uniqueness validation errors.

If you've a proposal which is clearly a more simple or robust approach than #4217, or if you can demonstrate that it'd resolve some other behavioral issue, then go for it.

[claytondaley]

there's nothing in the HTTP specification that precludes using 400 for those types of validation error

Quoting from the spec:

The 409 (Conflict) status code indicates that the request could not be completed due to a conflict with the current state of the target resource ...

Uniqueness and concurrency are resource-dependent issues. The existence of an error code specifically documented as covering this case would be a strong "implicit" argument for me. However, the 400 error also says (again, emphasis added):

The 400 (Bad Request) status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing).

The easy test for a client error is whether adequate client-side validation would have discovered it. It's self-evident that uniqueness tests (except within a multi-object submission) don't meet this criteria. It follows that uniqueness constraints are not client issues so 400 is not the spec response for these issues. This is also the consensus of the community.

it's also still unclear to me what status code you'd want to expect in the case where you have some field with value validation errors, and other fields with uniqueness validation errors

Re-read (1) in my original post. Comparing invalid client data (400) to the resource (409) is logically dubious and creates unpredictable errors. Until a client resolves all client-side errors (400), the ideal flow wouldn't even run the other (409) checks. Even in a backwards-compatible mode (i.e. running both checks simultaneously), the client must resolve their errors (400) first so this response code always takes precedence. It's even logically possible that the resource-dependent issue will fix itself (e.g. a duplicate is deleted) while the client fixes client-side (400) issues. The opposite is never true.

If you provide a pull request then we can make a judgement call on that

I certainly can, but I wanted to make sure the change and proposed approach wasn't DOA. #4231 was the last significant issue I ran into. It took a lot of back-and-forth to reach consensus that there was a problem and then the discussion of implementation options made it clear that a compact PR was going to be rejected because it would violate architecture-level assumptions.

[xordoquy]

My point of view here is that 400 is OK for uniqueness constraints.
I have used 409 for concurrent access to the same resource tracked by either a timestamp or a UUID to prevent overriding other's changes. I just had a look at the RFC and this is the use case they are using.

You may want to return 409 for uniqueness issues in some specific use cases such as acquiring a lock or something similar but that should better be performed by specific code rather than the generic DRF one that may have some issues here and there (such as concurrency issues, one of them being in our list of issues).

[tomchristie]

Until a client resolves all client-side errors (400), the ideal flow wouldn't even run the other (409) checks.

For most users that'd be a degradation in expected behavior, rather than an improvement. (You're going to have a knock-on effect all the way through to end-users editing form fields in an application, correcting an error in one field, resubmitting the form, and unexpected having an error in a different field, which previously appeared to be okay.)

By all means go ahead and look into altering the current behavior, but I'd strongly suggest that resolution in an optional third party package would be the best first-pass.

[claytondaley]

Anything's "OK" by client and server consent. I thought I was adopting a REST-compliant framework, not a by-consent API framework. I certainly would like it to at least allow a REST-compliant endpoint.

should better be performed by specific code rather than the generic DRF one

OK. So how should this be done in the current architecture?

• The error handler (where I do make other changes) gets a non-specific exception so the only way to distinguish among errors is exception message introspection.
• Subclassing the validation error and unique validator doesn't help because the framework collects errors and re-raises the ambiguous exception.
• Changing that behavior requires an edit to Field.run_validators. That's definitely not a compact part of my serializer or view.

Am I missing something obvious?

[claytondaley]

For most users that'd be a degradation in expected behavior, rather than an improvement.

I've repeatedly covered this case:

For backwards compatibility, a two-stage validation system can still accumulate all of the errors before raising anything

Even in a backwards-compatible mode (i.e. running both checks simultaneously), the client must resolve their errors (400) first so this response code always takes precedence

[carltongibson]

What you’re missing is that you’ve been told repeatedly that you’re welcome to implement this behaviour yourself but that it’s not something we’d accept in the core framework.

I can see that that is not the answer you’re looking for but I need to ask you to accept it nonetheless.

I’m afraid our time to maintain the framework is limited. We can’t continue this discussion here now. Thank you for your understanding.

[claytondaley]

@carltongibson. I'm more than happy to contribute to a 3rd party plugin when that model makes sense (e.g. significant contributions to drf-hal-json). I even understand when small changes in core can make it easier to push behaviors into 3rd party plugins. I've even reported issues that are still open against this repo because the best (or only) place to address the limitation is in core. Give a contributor some credit.

My most recent question was how a 3rd party plugin would implement this behavior subject to the current architecture. I realize I'm not an expert at DRF's architecture, but haven't seen a natural place/way to do so. If someone can give me a satisfactory answer, I'll do it.

More importantly, I'm not demanding that you fix it for me. I'm offering to submit a PR. I'm trying to get a rough architecture/approach pre-approved. I'm no paragon of OSS, but that's normally a welcomed approach that's considerate of everyone's time. We use the package and would be motivated to help fix issues if it didn't feel I was pulling teeth each time I had to communicate with the team. If your concern is ongoing maintenance, you could even make the improvements conditional on contribution to development or a support package. Instead... well you wrote the post.

[xordoquy]

@claytondaley as for the how you can implement this, drop the unique constraint validators, override the view function and catch integrity errors.
Been there done that with some extra (extra fast batch creation by inserting first and looking for validation if it didn't insert).

[claytondaley]

@xordoquy Thanks. That's certainly more pythonic than introspecting error codes (the obvious option left). +1 for eliminating the race condition on uniqueness. +1 for supporting 409 codes on other resource consistency issues (like foreign keys). It sounds like I may be able to wrap this up in Mixins (+decorators for overloaded functions).

To make the code reusable, can I iterate through each field's validators on a serializer instance? or do they get moved somewhere problematic that I need to remove them before instance creation? Are there any other edge cases you've run into that are worth anticipating?

[tomchristie]

I'm trying to get a rough architecture/approach pre-approved.

Totally fair enough - for which the answer is no. I don't agree that the proposed behavior would be an improvement.

For backwards compatibility, a two-stage validation system can still accumulate all of the errors before raising anything

We still have to take a judgement on what the default behavior should be. Having a default that masks uniqueness errors in the presence of other unrelated errors on other fields would be a degradation for most users.

REST-compliant

There's no such thing as "REST-compliant" in the context of HTTP status codes. There is the HTTP spec, tho. 400 and 409 are behaviorally identical, the only difference is semantic, and 400 is broad enough to be a reasonable choice for both types of validation error.

We provide further semantic information if users require it, in the form of per-field validation codes and messages, which have the benefit of being able to differentiate in the case where there are both uniqueness and value errors.

I think it would be great to have an option of the behavior that you require in the way you've specified it, but I'm not convinced we'd want it to be the default, and I think that "as a third party" is both the most practical way to move this forward, and the best option for our users.