Form submit validation and error handling: how to do it properly?

[simahawk]

Context
Imagine you a form that sends data to an endpoint which has its own validator.
The validator states that "country" field is required (or any other constrain).

Problem
If form data have an error you get this:

  File "/odoo/external-src/rest-framework/base_rest/components/service.py", line 142, in _secure_input
    raise UserError(_('BadRequest %s') % v.errors)
odoo.exceptions.UserError: ("BadRequest {'country': ['required field']}", '')

which, as far as I see, is not really nice to be handle as a response. The only way to handle it would be to parse the text of the response and look for a specific match with form fields.
This way seems very hard to handle form errors nicely.

Solutions?
@sbidoul @lmignon I wonder how you guys are handling this issue.
I think we should have a way to return a plain dict w/ detailed information on the errors w/ specific error codes. For instance:

    {
        'errors': {
            'country': 'missing',
            'phone': 'bad_lenght',
        },
        'errors_message': {
            'country': 'Country is required',
            'phone': 'Phone number must contain XX chars',
        }
    }

I'm doing something similar in cms_form and it helps very well on error handling.
BTW I'm thinking about an integration w/ cms_form to be able to define forms and expose their schema via rest api.

Possibly related to #2.

Thanks for your insights :)

[sbidoul]

One could also imagine to have a client library reading the OpenAPI schemas and doing pure client side form validation. I'd bet such JS libraries do exist.

Regarding server error responses I wonder if standards exist for that in the OpenAPI world.

[lmignon]

@simahawk It does not seem difficult to me to improve the content of the returned response in case of data validation error. I agree that we should return an easy to parse response (json) to ease the processing of this error on the client side. Nevertheless, as suggested by @sbidoul the use of a specialized library to validate the data on the client side could be a good idea.

[simahawk]

@sbidoul @lmignon yep, having a validation layer on client side would be nice and we should probably head to it for the long term. For the short term and for simple implementations, adding this small feature to base_rest is a big win w/ little effort I think. I can draft it if we agree on the specs ;)

What about:

1. adding a specific exception like ValidationError(werkzeug.exceptions.HTTPException) status = 400 (better status?)
2. handle it w/ wrapJsonException
3. content of the exception:

{
        'errors': {
            '$key/fieldname': '$specific_err_code',
        },
        'errors_message': {
            '$key/fieldname': '$specific_err_message',
        }
}

BTW how are you handling such case w/ shopinvader? I don't see any handling of such "bad requests" 😓

[lmignon]

@simahawk IMO, the changes to do are:

Create a new kind of exception into base_rest RequestValidationError(Exception)
Process this new kind of exception into the _handle_exception method defined into HttpRestRequest (https://github.com/OCA/rest-framework/blob/12.0/base_rest/http.py#L151)

Adapt the code into base.rest.service to raise this exception in case of validation error https://github.com/OCA/rest-framework/blob/12.0/base_rest/components/service.py#L141

from ..exceptions import RequestValidationError

        if v.validate(params):
            return v.document
        raise RequestValidationError(v.errors)

I'm not sure about the content of the exception. Indeed, the way errors are returned depends on the library used to validate the schema. I have the feeling that we will introduce a lot of unnecessary complexity if we try to change the content of the error returned by the library validating the schema.

I plan to modify this part of the code to also support a new way to specify Request and Response definition through serializable/deserializable models based on Marshmallow #41 That means that the format of the errors returned will not be the same if the validation is done by Cerberus or by Marshmallow.

Regarding shopinvader, I think that there is no specific code to handle this type of error and that at this stage, it is displayed as it is to the user (BadRequest is also used for UserError/ValidationError). I would rather opt for a client-side validation via a JS library to solve this problem. (For example sway )

[sbidoul]

In any case I think we'd need to do some research on common ways to return schema validation errors in the REST/OpenAPI world before inventing our own.

[simahawk]

That means that the format of the errors returned will not be the same if the validation is done by Cerberus or by Marshmallow.

but this is the point: you want to return - possibly - always the same schema for errors. You can always attach the specific validation error to the json.

In any case I think we'd need to do some research on common ways to return schema validation errors in the REST/OpenAPI world before inventing our own.

I agree. The way I propose is not far from what others do, is just less detailed. There's no official specification yet AFAIS.
A panoramic: https://nordicapis.com/best-practices-api-error-handling/ see "Good Error Examples".

Quote of the conclusions:
"""
Much of an error code structure is stylistic. How you reference links, what error code you generate, and how to display those codes is subject to change from company to company. However, there has been headway to standardize these approaches; the IETF recently published RFC 7807, which outlines how to use a JSON object as way to model problem details within HTTP response. The idea is that by providing more specific machine-readable messages with an error response, the API clients can react to errors more effectively.

In general, the goal with error responses is to create a source of information to not only inform the user of a problem, but of the solution to that problem as well. Simply stating a problem does nothing to fix it – and the same is true of API failures.

The balance then is one of usability and brevity. Being able to fully describe the issue at hand and present a usable solution needs to be balanced with ease of readability and parsability. When that perfect balance is struck, something truly powerful happens.
"""

The RFC mentioned is this https://tools.ietf.org/html/rfc7807
An example from there:

HTTP/1.1 400 Bad Request
   Content-Type: application/problem+json
   Content-Language: en

   {
   "type": "https://example.net/validation-error",
   "title": "Your request parameters didn't validate.",
   "invalid-params": [ {
                         "name": "age",
                         "reason": "must be a positive integer"
                       },
                       {
                         "name": "color",
                         "reason": "must be 'green', 'red' or 'blue'"}
                     ]
   }

Another example: https://medium.com/@suhas_chatekar/return-well-formed-error-responses-from-your-rest-apis-956b5275948

My conclusion: not having something similar is worse than not having anything IMO.
My proposal: let's agree on a simple and basic structure or pick an existing one (google?) and make it easy to override.
We can attach _original_validator_error or similar to include the tool-specific feedback.

What do you think?

[lmignon]

My proposal: let's agree on a simple and basic structure or pick an existing one (google?) and make it easy to override.

👍

[simahawk]

FTR I think I've found where/how errors are handled by locomotive module https://github.com/shopinvader/locomotive-shopinvader/blob/984262e9e09a38f06e8c35c4dd106d57c77966cf/lib/shop_invader/services/erp_service.rb#L120

[rousseldenis]

@simahawk @lmignon Is there a Roadmap for this ? Can we imagine doing that on OCA days ?

[simahawk]

@rousseldenis not that I know. The only change related to the problem is this #76 which allows to pass extra info the exception when you need it.
For the roadmap: we can discuss any time IMO ;)
If you have time, you could start drafting it here in another issue.
Regarding OCA days: personally I'm not sure I'll have time to work on anything this year :/

[rousseldenis]

@rousseldenis not that I know. The only change related to the problem is this #76 which allows to pass extra info the exception when you need it.
For the roadmap: we can discuss any time IMO ;)
If you have time, you could start drafting it here in another issue.
Regarding OCA days: personally I'm not sure I'll have time to work on anything this year :/

Ok, didn't see that PR.

Can you provide pointer to usage or README ?

[github-actions]

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.