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

Bug: validation module should return 400 on error #1298

Open
scottgerring opened this issue Jul 19, 2023 · 6 comments
Open

Bug: validation module should return 400 on error #1298

scottgerring opened this issue Jul 19, 2023 · 6 comments
Assignees
Labels
bug Something isn't working status/staged-major-release This change will go with the next major release v2 Version 2
Milestone

Comments

@scottgerring
Copy link
Contributor

scottgerring commented Jul 19, 2023

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.

@jeromevdl
Copy link
Contributor

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.

@scottgerring
Copy link
Contributor Author

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?

@jeromevdl
Copy link
Contributor

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?).

@scottgerring
Copy link
Contributor Author

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

@skal111
Copy link
Contributor

skal111 commented Oct 13, 2023

Happy to take that one

@scottgerring scottgerring added the status/staged-major-release This change will go with the next major release label Dec 7, 2023
@scottgerring
Copy link
Contributor Author

This will be released with v2

@scottgerring scottgerring added this to the v2 milestone Dec 21, 2023
@jeromevdl jeromevdl moved this from Backlog to Coming soon in Powertools for AWS Lambda (Java) Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working status/staged-major-release This change will go with the next major release v2 Version 2
Projects
Status: Coming soon
Development

No branches or pull requests

3 participants