powertools-lambda-java icon indicating copy to clipboard operation
powertools-lambda-java copied to clipboard

Bug: validation module should return 400 on error

Open scottgerring opened this issue 2 years ago • 6 comments

When you use the @Validation annotation on a RequestHandler and the provided message does not validate, the module will throw a ValidationException, resulting in a 500 back to the client.

Changing this behaviour is a breaking change.

What were you trying to accomplish? I would expect that validation errors should return a 400 and a description of why the validation failed. Alternatively if the annotation is applied to something other than a RequestHandler, throwing an exception for the user's code to catch seems reasonable.

Current Behavior

ValidationException / HTTP 500 returned to user.

Possible Solution

Steps to Reproduce (for bugs)

Check out the validation example as it exhibits this behaviour.

scottgerring avatar Jul 19 '23 12:07 scottgerring

Not that simple as we perform some validation on many different events, not only API GW...

400 is only applicable to API GW events and responses (APIGatewayProxyRequestEvent, APIGatewayV2HTTPEvent, APIGatewayProxyResponseEvent and APIGatewayV2HTTPResponse), otherwise an exception is still valid.

jeromevdl avatar Aug 21 '23 21:08 jeromevdl

Good point. Should we add some return-type specific behavior?

The alternative seems to be saying we aren't supporting the annotation directly on the handlers function itself, but only on "other methods"; then in the case of a HTTP API integration, the user's handler wraps this "other method", catches the exception, and maps back to a 400.

I feel that validation on a HTTP API is "common path" and should work without this level of glue. What do you reckon?

scottgerring avatar Aug 22 '23 04:08 scottgerring

Either we document how to not use the annotation and use the ValidationUtils in case of API and catch the validation exception to return 400. This is what python did (doc)

Either we implement this in the library : in case of one of the events above, we change the http code in the response to 400 and update the body with an error message... Probably a better experience for users but will be opinionated (message format: json? plain txt?).

jeromevdl avatar Aug 22 '23 06:08 jeromevdl

Let's tie this into the broader module-review issue for V2 - #1469. Oner person can knock both of these off at once.

scottgerring avatar Oct 10 '23 12:10 scottgerring

Happy to take that one

skal111 avatar Oct 13 '23 12:10 skal111

This will be released with v2

scottgerring avatar Dec 07 '23 09:12 scottgerring