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

feat(v2): Validation failures return 400s #1489

Merged
merged 21 commits into from
Dec 1, 2023

Conversation

skal111
Copy link
Contributor

@skal111 skal111 commented Oct 24, 2023

api gateway events validation will be catched and returned as a 400 error

#1298

Description of changes:

ValidationException for events of type APIGateway are catched and transformed into a valid response (status 400).

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@skal111 skal111 marked this pull request as draft October 24, 2023 11:03
@scottgerring scottgerring changed the title #1298 first implem - api gateway events validation will be catched an… feat(v2): Validation failures return 400s Oct 24, 2023
@skal111 skal111 marked this pull request as ready for review October 26, 2023 12:45
Copy link
Contributor

@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comments.

@jeromevdl
Copy link
Contributor

jeromevdl commented Oct 30, 2023

Additional point that we completely forgot: it would be great to make the @Validation annotation compatible with the batch module (SQS, Kinesis), i.e. if one message is wrong do not fail the whole batch but add the message to the partial failure list.

my idea:

  • browse the list of messages and validate each one (here),
  • log a warn for each invalid message
  • remove the ones that are not good (no need to pass them to the rest of the batch processing),
  • add them to the list of failure (example)

@scottgerring what do you think?

@scottgerring
Copy link
Contributor

Additional point that we completely forgot: it would be great to make the @Validation annotation compatible with the batch module (SQS, Kinesis), i.e. if one message is wrong do not fail the whole batch but add the message to the partial failure list.

This is a great idea - I think PT should aspire to "magically do what you'd expect" when you combine modules. We'd need to pay special attention to the coupling between the modules - explicitly maven coupling between batch<->validation is IMHO a non-starter. Perhaps we could pull validation exceptions into their own package, or something 🤷 - i've not thought about this in depth, but I think how this coupling would work will be decisive here.

@jeromevdl pragmatically we could also treat this integration in a subsequent PR. WDYT?

@jeromevdl
Copy link
Contributor

jeromevdl commented Oct 30, 2023

I agree we could do that in another PR: #1496

Regarding the coupling, there should not be any dependency between the 2 modules. And I think there won't be any.

  1. We can keep the validation code as today (almost) but rather than throwing an exception, do just like @skal111 did for API GW, ie. creating an SQSBatchResponse / StreamsEventResponse and put the messages in error in the partial error.

  2. Then the handler is called with all the remaining messages (or directly return if no message is valid), and here the batch module will work.

  3. In the end the validation module completes the SQSBatchResponse / StreamsEventResponse returned by the handler (batch module) with the validation failure from 1.

Finger in the nose ?!

Copy link
Contributor

@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last couple of things + rebase v2 and we should be good

@skal111 skal111 force-pushed the feat/1298-validation-400 branch from 80ceb05 to a414353 Compare November 29, 2023 16:09
@scottgerring
Copy link
Contributor

@jeromevdl let's merge this when the build succeeds

@scottgerring
Copy link
Contributor

@skal111 it looks like you are using Java11+ collection helpers - if you can port that back so the java 8 build succeeds we can merge this!

Comment on lines 253 to 264
APIGatewayV2HTTPEvent event = new APIGatewayV2HTTPEvent();
event.setBody("{" +
" \"id\": 1," +
" \"name\": \"Lampshade\"" +
"}");
event.setHeaders(Map.of("header1", "value1"));

APIGatewayV2HTTPResponse response = handler.handleRequest(event, context);
assertThat(response.getBody()).isNotBlank();
assertThat(response.getStatusCode()).isEqualTo(400);
assertThat(response.getHeaders()).isEmpty();
assertThat(response.getMultiValueHeaders()).isEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
APIGatewayV2HTTPEvent event = new APIGatewayV2HTTPEvent();
event.setBody("{" +
" \"id\": 1," +
" \"name\": \"Lampshade\"" +
"}");
event.setHeaders(Map.of("header1", "value1"));
APIGatewayV2HTTPResponse response = handler.handleRequest(event, context);
assertThat(response.getBody()).isNotBlank();
assertThat(response.getStatusCode()).isEqualTo(400);
assertThat(response.getHeaders()).isEmpty();
assertThat(response.getMultiValueHeaders()).isEmpty();
APIGatewayV2HTTPEvent event = new APIGatewayV2HTTPEvent();
event.setBody("{" +
" \"id\": 1," +
" \"name\": \"Lampshade\"" +
"}");
event.setHeaders(Map.of("header1", "value1"));
APIGatewayV2HTTPResponse response = handler.handleRequest(event, context);
assertThat(response.getBody()).isNotBlank();
assertThat(response.getStatusCode()).isEqualTo(400);
assertThat(response.getHeaders()).isEmpty();
assertThat(response.getMultiValueHeaders()).isEmpty();

Copy link
Contributor

@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @skal111

Copy link

sonarqubecloud bot commented Dec 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jeromevdl
Copy link
Contributor

I've tested the e2e locally, it's ok!

@jeromevdl jeromevdl merged commit 412118d into aws-powertools:v2 Dec 1, 2023
10 of 16 checks passed
@skal111 skal111 deleted the feat/1298-validation-400 branch December 5, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants