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

Give more context about validation path on ValidationException #1843

Closed
Traderjoe95 opened this issue Jan 14, 2021 · 11 comments
Closed

Give more context about validation path on ValidationException #1843

Traderjoe95 opened this issue Jan 14, 2021 · 11 comments

Comments

@Traderjoe95
Copy link

Traderjoe95 commented Jan 14, 2021

Describe the feature

When validating properties of an object or items of an array (in a request body), add more context on where the validation failed. That information should preferably returned in form of a JsonPointer.

At the moment, the property path taken in validation can't be derived from the ValidationException as it never carries that information, not event in the scope property which is always set to the root schema.

Use cases

For the consumer of a REST API dealing with error responses, it is necessary to know where exactly the error happened. Therefore, the pointer to the specific element failing validation should be returned in the error response.

Contribution

I am not yet custom with the internal structure of Vert.x validation, so I think I wouldn't be the best person to implement this. However, I might find some time to deep dive into it, but I can't guarantee it. So if anyone else from the community is able to implement this, feel free to take over.

@Traderjoe95 Traderjoe95 added the enhancement New feature or request label Jan 14, 2021
@mwm-twx
Copy link

mwm-twx commented Jan 14, 2021

The current vertx-web-openapi implementation is lacking compared to the prior vertx-web-api-contract. There is no indication of what field in the body caused the validation error, and no way to obtain it from the underlying ValidationException.

api-contract produced this error: "$.field1: must be at least 1 characters long"
openapi produces this error: "[Bad Request] Validation error for body application/json: provided string should have size >= 1"

@slinkydeveloper slinkydeveloper added component/openapi and removed enhancement New feature or request labels Jan 14, 2021
@slinkydeveloper
Copy link
Member

slinkydeveloper commented Jan 14, 2021

The nesting of error system in the new validation library is:

  1. BadRequestException is the parent exception for anything wrong that happens in ValidationHandler: https://github.com/vert-x3/vertx-web/blob/master/vertx-web-validation/src/main/java/io/vertx/ext/web/validation/BadRequestException.java
  2. Depending on where the error happened, the cause is ParameterProcessorException or BodyProcessorException.
  3. For both, if the error is a parsing error, the cause is MalformedValueException or, if the error is a validation exception from the json schema validator, the cause is ValidationException

In this ValidationException you actually get far more information than from the old ValidationException from web-api-contract: which schema failed, json pointer of the context, input, etc...

Do you have any particular suggestions about that? Maybe it's just a lack of docs? Or should we have better helpers?

api-contract produced this error: "$.field1: must be at least 1 characters long"
openapi produces this error: "[Bad Request] Validation error for body application/json: provided string should have size >= 1"

I honestly never noticed it, but I believe this is more a problem of the correct toString() implementations. That should be trivial to fix 😄

@Traderjoe95
Copy link
Author

Traderjoe95 commented Jan 14, 2021

In this ValidationException you actually get far more information than from the old ValidationException from web-api-contract: which schema failed, json pointer of the context, input, etc...

As far as I have observed (correct me if I'm wrong), the JSON pointer in the ValidationException does only point to the root schema, but not to the concrete element failing the validation. Perhaps that is because of the SchemaImpl class catching any ValidationException instances and setting the scope to its own scope.

So maybe this is a bug rather than a feature request?

@mwm-twx
Copy link

mwm-twx commented Jan 14, 2021

I'm admittedly not familiar with the code, but I tried to pull information from the ValidationException using the scope() and schema() and wasn't able to get the necessary information. I think scope().traceQuery(...) would provide similar information to the $.field1 in the old API, but I think traceQuery needs the complete JsonObject from the body, and the input() on the exception is the segment which failed validation (empty-string in this case).

@Traderjoe95
Copy link
Author

Traderjoe95 commented Jan 14, 2021

I think scope().traceQuery(...) would provide similar information to the $.field1 in the old API, but I think traceQuery needs the complete JsonObject from the body, and the input() on the exception is the segment which failed validation (empty-string in this case).

I am adding the custom response body in an error handler on the router, so I am having access to the RoutingContext which also gives me access to the JSON body. So it's no problem to acquire the whole body JSON. However, the problem that the JSON pointer returned by .scope() only points to the root object (i.e. the pointer is empty) and therefore, a call to any of the ...query() methods of it will only ever yield the body object and nothing else.

What is the scope() supposed to point to? Could you please clarify, @slinkydeveloper? Should it point to the field that caused the validation error or the schema field that failed on the input?

Do you have any particular suggestions about that? Maybe it's just a lack of docs? Or should we have better helpers?

What I really need is more than the local information provided by Validation exception. It only provides a local input value and a JsonPointer pointing to the root of a schema. But I would like to know the actual context of the input value. If that is what scope() is supposed to be, than I think there's a bug.

@vietj vietj added this to the 4.1.0 milestone Jan 18, 2021
@slinkydeveloper
Copy link
Member

the JSON pointer in the ValidationException does only point to the root schema, but not to the concrete element failing the validation
What is the scope() supposed to point to? Could you please clarify, @slinkydeveloper? Should it point to the field that caused the validation error or the schema field that failed on the input?

ValidationException#scope points to the scope of the schema and then through keyword() you can figure out which keyword failed the validation. ValidationException doesn't carry the information of where, in the input, the validation failed, but it returns the input part that failed the validation with input(). Is that what you're looking for?

@mwm-twx
Copy link

mwm-twx commented Jan 19, 2021

In the case of failing a minimum-length constraint, for example, input is "" and keyword is minLength; there's nothing that indicates what was empty.

@Traderjoe95
Copy link
Author

The schema scope also doesn't help in that case. If I am working with a simple schema, I could try to derive the location of the validation failure by matching the keyword in the validation schema. But if we suppose that the schema is more complex (like in my case) the keyword, input and schema scope alone may not be sufficient to distinguish the concrete location of the error. (like when, for example, two different fields in the request body have the same value, then input would be ambiguous)

@slinkydeveloper
Copy link
Member

Ok I get your concerns, so I guess what we miss on the vertx-json-schema side is the ability to "trace" the execution of the validator, so when it fails, it's able to return a json pointer expliciting where it failed. I've opened an issue for that eclipse-vertx/vertx-json-schema#32

@slinkydeveloper
Copy link
Member

@mwm-twx @Traderjoe95 I've drafted an implementation in json schema, do you want to try out? eclipse-vertx/vertx-json-schema#37

@slinkydeveloper
Copy link
Member

slinkydeveloper commented Mar 22, 2021

The PR in vertx-json-schema was merged, so i consider this done. Reopen if you need it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants