Introduce error code for validation errors.

[johnraz]

This patch is meant to fix #3111, regarding comments made to #3137
and #3169.

The ValidationError will now contain a code attribute.

Before this patch, ValidationError.detail only contained a
dict with values equal to a list of string error messages or
directly a list containing string error messages.

Now, the string error messages are replaced with ValidationError.

This means that, depending on the case, you will not only get a
string back but a full object containing both the error message and
the error code, respectively ValidationError.detail and
ValidationError.code.

It is important to note that the code attribute is not relevant
when the ValidationError represents a combination of errors and
hence is None in such cases.

The main benefit of this change is that the error message and
error code are now accessible in the custom exception handler and can
be used to format the error response.

A custom exception handler example is available in the
TestValidationErrorWithCode test.

Regarding the other discussion:

• This PR doesn't require any new settings
• The build_error method was dropped
• The build_error_from_django_validation_error has been kept because this is the only way to "convert" a regular DjangoValidationError to a djrf ValidationError.

@tomchristie @jpadilla @qsorix: if you don't mind giving this a look ;-)

[johnraz]

ping @xordoquy maybe :-)

[tomchristie]

Change the return type of serializer.errors in this way simply isn't an option.

[johnraz]

I could change the definition of errors to return the initial type.
Would that be enough ?

[tomchristie]

Maybe I'm not understanding something here?
We simply can't change the return type of errors - it'll break stacks of existing projects.

[johnraz]

Well what I mean is that I could keep the new ValidationError in _errors and change the definition of errors so that it will keep returning the same type as before and avoid backward incompatibilities.

[tomchristie]

Possible thad be okay, yes. Hard to say 100% for sure without seeing the change.

[johnraz]

K i will give it a shot and come up with a new version of this.

Thx for taking the time.

[tomchristie]

Ta. Sorry it's such a thorny one! 😄

[tomchristie]

Inline comment addresses the main issue here. The change footprint on this pull request still isn't minimal enough. If we start with simply introducing an optional code argument on ValidationError, and populating that in serializer fields, then we can consider what API we'd need to add or modify in order to expose that information from the serializer. (Eg Perhaps we'd have an error_codes attribute. Perhaps errors would remain as string like objects but have an additional .code attribute. Perhaps we wouldn't expose it by default but would require overriding on the serializer to do so) How we do so is unclear ATM, but there's an obvious pre-cursor which is just ValidationError with an optional code attribute.

[johnraz]

@tomchristie: I think I managed to keep backward compatibility in place this time ;-)

[johnraz]

Would be nice to get some feedback on this before the start of next week as I will have some spare time to work on it if required.

Sorry for being pushy again ^^

[johnraz]

@tomchristie , @xordoquy ?

[tomchristie]

If the PR was made without changing any existing tests then that'd better demonstrate that the new codes don't break or change any existing API. Difficult to review when so much pre-existing tested behavior also changes.

[johnraz]

Yes, I realize now that I focussed only on serializer.errors while other tests were impacted too...

I don't know where my mind was, really sorry about that.

I just ran the tests against the unchanged tests version and I ended up with 16 failures which are all the same as the following:

______________ TestEmailField.test_invalid_inputs ______________
tests/test_fields.py:429: in test_invalid_inputs
    assert exc_info.value.detail == expected_failure
E   assert [ValidationError()] == ['Enter a valid email address.']

Calling str(exc_info.value.detail[0]) will return 'Enter a valid email address.'.

We talked before of having a "string-like" object that could hold the error code, it is likely that this is where that string-like object should live.

I'll make another attempt that doesn't touch the tests at all this time and I'll get back to you ^^

[tomchristie]

Ta. We'll be closer to a properly reviewable state then.

[johnraz]

@tomchristie, so I gave it another try, it looks much simpler now with a lot less changes for the same end results.

Hopefully we can proceed with the actual review now ;-)

Side note: I plan to rebase this to one simple commit once this gets reviewed.

[tomchristie]

Not 100% convinced by the new ValidationErrorMessage and the change in the error argument for nested cases here. It ends up being a bit odd because in the nested case these's a different way of indicating the error code, than is used in the simple case.

Alternatives might include (1) constraining the code argument to only support simple cases, or (2) support code as a nested structure.

We might need to do some more thinking in this area. Unsure.

[johnraz]

Can you explain how the error argument for nested cases changes here ? I'm not following that part.

From what I get, everything stayed the same, it's just that the message became a little bit smarter than before and it is opt-in so you'd only have to change anything if you actually want to use error codes.

[johnraz]

See my hidden comment on the outdated diff above ;-)

[tomchristie]

"ValidationErrorMessage is now abstracted in ValidationError's constructor"

@johnraz Okay noted. Tho let's still see how error codes are presented in Django's Forms API before making any further judgements.

[johnraz]

@tomchristie : this stackoverflow answer is quite useful http://stackoverflow.com/a/24874373/925854:

In Django 1.7, you can now access the original error data from the form. You can call the as_data() method on an ErrorList or ErrorDict. For example: my_form.errors.as_data(). This basically gives you the original ValidationError object instead of the message itself. From this you can access the .code property, eg: my_form.errors["all"].as_data()[0].code.

You can also serialize form errors, great for APIs:

print(form.errors.as_json())
{"all": [
{"message": "Your account has not been activated.", "code": "inactive"}
]}

[johnraz]

And the django documentation on the subject:
https://docs.djangoproject.com/en/1.9/ref/forms/api/#django.forms.Form.errors.as_data

[johnraz]

So my current view on this is simple:

We can divert from django's way by using ValidationErrorMessage as a value of ValidationError.detail.

ValidationErrorMessage will then contain both the message in itself and the error code in ValidationErrorMessage.code.

This solution doesn't have any backward incompatibility.

OR

We can stick to django's way by nesting ValidationError in ValidationError.detail.

ValidationError will then contain both the message in ValidationError.detail and the code in ValidationError.code

This solution will actually break backward compatibility for anyone who used to manipulate ValidationError.detail as a list of string.

This is where this would happen:

______________ TestEmailField.test_invalid_inputs ______________
tests/test_fields.py:429: in test_invalid_inputs
assert exc_info.value.detail == expected_failure
E assert [ValidationError()] == ['Enter a valid email address.']

[tomchristie]

How I would like to see this proceed:

• We drop ValidationErrorMessage - the code should simply be a new optional argument on ValidationError.
• Internally we store the ValidationError in ._errors, rather than just the message.
• Calling .errors parses _errors and returns an equivalent structure with just the messages.
• Calling .error_data parses _errors and returns an equivalent structure with just the codes.

[johnraz]

We drop ValidationErrorMessage - the code should simply be a new optional argument on ValidationError.
Internally we store the ValidationError in ._errors, rather than just the message.
Calling .errors parses _errors and returns an equivalent structure with just the messages.

So this is the state of what I had in johnraz@1834760 (and to be more specific johnraz@1834760#diff-80f43677c94659fbd60c8687cce68eafR253)

So, you are okay with breaking the 16 tests that I pointed out earlier in the discussion ?

Calling .error_data parses _errors and returns an equivalent structure with just the codes.

The problem with this approach is that you can have multiple errors for a single key in the error dict:

errors = { 'a_field': ['error_1', 'error_2']}
errors_data = {'a_field': ['required', 'unique']}

It will be a pain to mach the error with the error code

Shouldn't we instead do something like:

errors = { 'a_field': ['error_1', 'error_2']}
errors_data = {'a_field': [
     {'message': 'error_1', code:'required'}, {'message': 'error_2', code:'unique'}
]}

Or maybe this is what you meant all along :) ?

[johnraz]

There is obviously something wrong with all those isinstance checks and if you have any idea how to get rid of them or at least improve this code, I'm listening ;-)

[tomchristie]

As usually, I'd probably rather see just the ValidationError instantiation being addressed before we move to the next step. As it stands the pull requests are too large a footprint to thoroughly review.

Smallest coherent step possible at each point, please :)

[johnraz]

I can't help It, it seems :) Will simplify on tuesday \o/

[tomchristie]

😄

[johnraz]

There you go, I hope it is a bit simpler to read, nothing is done yet to deal with serializer.errors, only the ValidationError instantiation has been modified.

And yet the code is still very messy...

[tomchristie]

We don't really want this stuff in. At least at this point.

[johnraz]

What do you mean ? Above or below ?

[tomchristie]

The population of .full_details. Should just be storing the message and code at this point.
(Let me know if I'm being braindead tho - I only get a brief amount of time on each review)

[johnraz]

I'm not sure to follow.

It seems to me that this is what I'm doing right now:

The population of .full_details. Should just be storing the message and code at this point.

It does store message and code for single items, a list when a list is provided and a dict when a dict is provided
Basically it does store what is passed to the __init__ method.

We then need to transform whatever is given as the detail parameter to the former self.detail format which is what this code does (quite painfully).

[johnraz]

The more we progress on this, the more I'm convinced that the string-like object (aka ValidationErrorMessage) solution was way better and required way less changes...

If you have a minute to review those 2 commits in parallel now that you have a better understanding of what we try to achieve.

It covers all the topics we discussed above (instantiation, exposing error message and code).

It enables all the features (ability to handle error codes exposure in the exception_handler and maintain backward compat')

All tests pass with a simpler code change and a nice API (ValidationError(message, code)).

The only drawback is that it requires an explanation of how to get back the code in the exception_handler which is, IMHO, trivial.

johnraz@42f4c55
johnraz@7971343

Sorry to insist but I think having a second thought here is healthy and I want to make sure we don't chose the wrong path and that we stay away from adding too much complexity.

[johnraz]

I created #3775 to help you compare ;-)

[johnraz]

@tomchristie: I'd also like to add to my argument in favor of the string-like object solution that it has the same impact on the api than the tuple solution has, in the end I cannot see any drawbacks by going with the string like object at all...

[johnraz]

Considering that the ValidationError's argument signature is the same in both solutions, I think the use of either ValidationErrorMessage in one case or ErrorDetails in the other is just an implementation detail of django-rest-framework itself, it shouldn't and wouldn't impact users of the lib that much.

Also, if we wan't to make things more obvious, nothing stops us to implement full_details with the string like object solution either.

[johnraz]

Another argument:

We don't need to reconstruct the ValidationError.detail with the string-like object solution where we do have to do a lot of tinkering to reconstruct it with this pr (see the ValidationError.__init__) method.

(I think I'm done with arguments for today :-p)

[johnraz]

@tomchristie don't be afraid we will get to it :-)

[johnraz]

Bump ^^ :)

[tomchristie]

Apologies - squeezed for time ATM.

[johnraz]

Accepted 👍

[johnraz]

@tomchristie : any chance we could go back to this? I really think the proposal I made in #3775 is worth looking at and merging.

@xordoquy maybe you could give it a shot too ?

I just don't want to see all the efforts we put into this buried by time and loss of memory :)

cheers ;-)

[tomchristie]

We could try to, yup - want to address the failing travis build first?

[johnraz]

It fails on purpose - this is not yet complete.
#3775 does pass everything though and I want to push it forward :-)

[johnraz]

Closing this in favor of #3775 to reduce noise.