Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error codes to ValidationError #3137

Closed
wants to merge 1 commit into from

Conversation

qsorix
Copy link

@qsorix qsorix commented Jul 10, 2015

This change addresses use cases that require more information about reported
validation errors. Currently for each error that REST Framework reports users
get only that error's message string. The message can be translated so there's
no good way to recognize programmatically what sort of an error it is.

When building an API that is supposed to return error codes, I've found it very
limiting. For example, I was supposed to differentiate between missing fields
and invalid arguments. The only way to do it right now was to monkey-patch all
hard coded error messages and prefix them with an error code, then write a custom
exception handler that unpacked those error codes and acted accordingly.
Alternatively, I could write my own set of Field and Validator classes that
would throw different exception. In either case, it felt like this is something
that has to be addressed within the REST Framework itself.

This commit introduces proper error codes handling to the Validation Error, and
a customizable error builder that let's users pick how they want to represent
their errors.

ValidationError can hold a single error itself (text), a list of those, or a
dictionary mapping errors to fields. Error code is only meaningful for a single
error, and I've added assertions to check for proper usage.

To help with my development, I've added a setting that makes error code a
mandatory argument. Thanks to this, I was able to correct all uses of
ValidationError across the code.

Information about errors was originally available via ValidationError.detail,
and format of these data must not change, because users can have custom
exception handlers that rely on it. So to maintain backward compatibility, I've
added customizable error builder. By default, it discards the error code.

Users are supposed to change that error builder in settings if they want to use
error codes in their exception handler. If they do so, the structure of
ValidationError.detail does not change, but in the leafs they'll find results
from their error builder.

This change addresses use cases that require more information about reported
validation errors. Currently for each error that REST Framework reports users
get only that error's message string. The message can be translated so there's
no good way to recognize programmatically what sort of an error it is.

When building an API that is supposed to return error codes, I've found it very
limiting. For example, I was supposed to differentiate between missing fields
and invalid arguments. The only way to do it right now was to monkey-patch all
hard coded error messages and prefix them with an error code, then write a custom
exception handler that unpacked those error codes and acted accordingly.
Alternatively, I could write my own set of Field and Validator classes that
would throw different exception. In either case, it felt like this is something
that has to be addressed within the REST Framework itself.

This commit introduces proper error codes handling to the Validation Error, and
a customizable error builder that let's users pick how they want to represent
their errors.

ValidationError can hold a single error itself (text), a list of those, or a
dictionary mapping errors to fields. Error code is only meaningful for a single
error, and I've added assertions to check for proper usage.

To help with my development, I've added a setting that makes error code a
mandatory argument. Thanks to this, I was able to correct all uses of
ValidationError across the code.

Information about errors was originally available via ValidationError.detail,
and format of these data must not change, because users can have custom
exception handlers that rely on it. So to maintain backward compatibility, I've
added customizable error builder. By default, it discards the error code.

Users are supposed to change that error builder in settings if they want to use
error codes in their exception handler. If they do so, the structure of
ValidationError.detail does not change, but in the leafs they'll find results
from their error builder.
@tomchristie
Copy link
Member

The error codes work seems really good.

The 'build_error' stuff tho introduces more indirection than I'd be happy with - for many users that's going to hide what the actual behaviour is, so if there's any way of avoiding that while keeping the error codes bit that be more likely to get an okay.

I suppose ways to get around that would might be:

  • do any extra overriding of the resulting errors in plain method overrides.
  • have a default (eg string like objects, plus error code) and push any other behavioural changes to the exception handler.

@qsorix
Copy link
Author

qsorix commented Jul 10, 2015

Thanks for a quick feedback. I'm going to implement it differently, but please help me to decide on the right approach.

TL;DR: What do you think about introducing class Error (not an exception) with attributes message and error_code and use that instead of a string, as the basic building block of ValidationError?

The main motivation for introducing 'build_error' was adding error codes while staying backward compatible. The change wouldn't be dramatic, but it's still a change. Should I care so much?

I'm not sure what you mean by "extra overriding ... in plain method overrides" because I don't see classes where those overrides would be. The only sensible candidate is a View, but here the ValidationError already needs to be different. So probably backward compatibility was not what you was thinking about when mentioning that. If I can break the API then yeah, it makes sense, but...

... I like the approach of changing current basic error's type, and handling it differently in an exception handler. Candidates for a new type include:

  • (message, code) tuple (good for now, but any further extensions won't be easier)
  • ValidationError itself (I think django does it this way, but I'm not a fan of putting too much in this class)
  • dedicated Error class (not an exception, perhaps a named tuple?)
  • hackish thing that pretends to be a string but includes the error_code attribute

I think I prefer the 3rd option (dedicated class). What's your pick? Any other ideas?

The default exception_handler will ignore error codes, making the output the same as it is in the current version.

Impact is limited. People who have custom implementations of the handler will need to adjust them. Personally, I have no other exception handling code where I would catch Validation Errors, the exception_handler does the job well and I didn't need anything more. But people may have something, and that'll break too.

@tomchristie
Copy link
Member

I would restrict the change solely to this:

"Add optional .code descriptions to ValidationError"

No build_error functions, and no different handling of ValidationError by default - just the most minimal possible pull request to support the change described above.

It's unclear how we'd handle nested and lists of messages. Either we could do that by having the code attached as a property on the message, or .code could be a nested structure.

It's also not a given that we'd accept that pull request, but it would be viable, and it'd be much easier to assess.

@tomchristie
Copy link
Member

In it's current state I'm closing this as too-broad ranging. Any future iteration which solely targets introducing .code on ValidationError or introducing .code on ValidationError.message may be acceptable.

johnraz added a commit to johnraz/django-rest-framework that referenced this pull request Dec 8, 2015
This patch is meant to fix encode#3111, regarding comments made to encode#3137
and encode#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 all 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 the custom exception handler and can
be used to format the error response.

An custom exception handler example is available in the
`TestValidationErrorWithCode` test.
johnraz added a commit to johnraz/django-rest-framework that referenced this pull request Dec 8, 2015
This patch is meant to fix encode#3111, regarding comments made to encode#3137
and encode#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 all 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 the custom exception handler and can
be used to format the error response.

An custom exception handler example is available in the
`TestValidationErrorWithCode` test.
johnraz added a commit to johnraz/django-rest-framework that referenced this pull request Feb 23, 2016
This patch is meant to fix encode#3111, regarding comments made to encode#3137
and encode#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 all 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 the custom exception handler and can
be used to format the error response.

An custom exception handler example is available in the
`TestValidationErrorWithCode` test.
johnraz added a commit to johnraz/django-rest-framework that referenced this pull request Apr 7, 2016
This patch is meant to fix encode#3111, regarding comments made to encode#3137
and encode#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 all 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 the custom exception handler and can
be used to format the error response.

An custom exception handler example is available in the
`TestValidationErrorWithCode` test.
johnraz added a commit to johnraz/django-rest-framework that referenced this pull request Aug 28, 2016
This patch is meant to fix encode#3111, regarding comments made to encode#3137
and encode#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 all 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 the custom exception handler and can
be used to format the error response.

An custom exception handler example is available in the
`TestValidationErrorWithCode` test.

We keep `Serializer.errors`'s return type unchanged in
order to maintain backward compatibility.

The error codes will only be propagated to the `exception_handler`
or accessible through the `Serializer._errors` private attribute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants