fix: envelope is not taken into account with built-in types #960
Conversation
...on/src/main/java/software/amazon/lambda/powertools/validation/internal/ValidationAspect.java
Outdated
Show resolved
Hide resolved
| @@ -59,7 +64,9 @@ && placedOnRequestHandler(pjp)) { | |||
| JsonSchema inboundJsonSchema = getJsonSchema(validation.inboundSchema(), true); | |||
|
|
|||
| Object obj = pjp.getArgs()[0]; | |||
| if (obj instanceof APIGatewayProxyRequestEvent) { | |||
| if (validation.envelope() != null && !validation.envelope().isEmpty()) { | |||
There was a problem hiding this comment.
Could there be cases where customer functionality changes because of this reordering?
There was a problem hiding this comment.
Potentially, if they have an envelope specified (which is not used today). It will be used after update.
| public class SQSWithCustomEnvelopeHandler implements RequestHandler<SQSEvent, String> { | ||
|
|
||
| @Override | ||
| @Validation(inboundSchema = "classpath:/schema_v7.json", envelope = "records[*].powertools_json(body).powertools_json(Message)") |
There was a problem hiding this comment.
One last thing, notices how records in the envelope JMSPath starts with a lower-case r but that in the schema it starts with an upper-case R. Also, on your website documentation for builtin events, you're specifying the path with a capital R. When I was trying to build my custom envelope starting from the builtin events didn't really help. Also I am not sure but it's worth checking if JMSPaths are case-sensitive.
There was a problem hiding this comment.
You are actually right, the issue is that using the aws-lambda-java-events library, SQSEvent has a "records" (r) field while the true event has a "Records" (R).
The validation library uses the event library, which is actually wrong when talking about JMESPath which looks at the real JSON... The test should have envelope = "Records[*].powertools_json(body).powertools_json(Message)" with big R. I can change this to reflect the reality. We may end up with breaking changes. @msailes wdyt ?
There was a problem hiding this comment.
Hi Jerome,
I don't think that updating the test to have a big R would work, not sure maybe worth to check. I was more thinking that updating the documentation to have a small r would be the simpler option here.
In any case, I don't think this is related to this PR, so I think this PR does need to be updated.
There was a problem hiding this comment.
Actually I was thinking about correcting the code to match the truth (R) and make the test with R pass. Small r should not work as the real event has big R...
There was a problem hiding this comment.
I agree with you @jeromevdl
There was a problem hiding this comment.
see e54c4d9
Maybe not related to the PR, but we have to fix it anyway
ex: SQS Records with big R
There was a problem hiding this comment.
Thank you @jeromevdl, I can confirm this now works as expected. I tested it, creating events with the following code
SQSEvent.SQSMessage sqsMessage = new SQSEvent.SQSMessage();
sqsMessage.setBody(messageBody);
And it works with a big R
| Expression<JsonNode> expression = ValidationConfig.get().getJmesPath().compile(envelope); | ||
| subNode = expression.search(jsonNode); | ||
| if (subNode == null || subNode instanceof NullNode) { | ||
| throw new ValidationException("Not found"); |
There was a problem hiding this comment.
Can this error message be expanded to be clearer to a dev that hits it?
There was a problem hiding this comment.
Just changed the message. But the exception is catch and the main message is just below, with more details.
…rtools#960) * add precedence for the envelope over built-in types * cannot use same envelope for in and out * remove extra new line * handle event field names properly ex: SQS Records with big R * more explicit exception message
Issue #, if available: #959
Description of changes:
When an envelope is set, it takes precedence over the built-in types. If not set, built-in types validation apply. If the type is not handled, a warn is logged.
Checklist
Breaking change checklist
People using an envelope with a built-in type (useless) may now enter in the envelope validation rather than the built-in one, causing validation error.
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.