Closed justin-tay closed 4 months ago
Thanks for submitting this PR. It looks good to me but I have 3 questions.
JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(VersionFlag.V202012, builder ->
builder.schemaMappers(schemaMappers -> schemaMappers.mapPrefix("https://www.example.org/", "classpath:schema/"))
);
Thanks for submitting this PR. It looks good to me but I have 3 questions.
- Is there any breaking change compared to previous version: can we reuse the same schema files? is the validation the same (appart from the output message)? ...?
There were breaking API changes, but given that it doesn't seem like this library provides much options to customize the validator I'm not sure it applies here, like if users had custom keyword or format implementations etc that would require code changes.
This fixes a lot of issues with respect to compliance with the specifications with respect to the validation so if users relied on incorrect behavior that will break. For instance previously certain keywords for 2019-09 and in particular 2020-12 didn't work eg $dynamicRef
, $dynamicAnchor
.
- Do we need to update the documentation to reflect this change? If yes, can you do it part of this PR.
I'm not really sure which aspect of the documentation would require updating. The message is likely the most visible change.
Did you test on Lambda? Because I remember we had some problem with the "online" schemas on Lambda (see http error 403 when accessing https://json-schema.org/draft/2019-09/meta/core networknt/json-schema-validator#867).
- We may need to do something like this
JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(VersionFlag.V202012, builder -> builder.schemaMappers(schemaMappers -> schemaMappers.mapPrefix("https://www.example.org/", "classpath:schema/")) );
There's actually no need to do that for the standard schema dialects because the MetaSchemaMapper
will map the standard dialects to the classpath but in this case this library already maps them to the classpath which I opted to retain since I'm not sure if the intent was to prevent users from pulling other resources from https etc and only allow classpath resources.
@justin-tay, Can you try to run the two end-to-end tests in powertools-e2e-tests ? Then if it works we should be good. Thank you.
Will there be any costs involved running the two end-to-end tests in powertools-e2e-tests?
The ValidationApiGWE2ET
is based on API GW / Lambda, and you have free tier with these services so it should not cost anything. Do not test the ALB one if you don't want to expense anything.
I only ran ValidationApiGWE2ET
but I expect that ValidationALBE2ET
should also pass with the updated messages.
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 51.41 s -- in software.amazon.lambda.powertools.ValidationApiGWE2ET
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
Issue #, if available: #1663
Description of changes:
Upgrades com.networknt:json-schema-validator to 1.4.3.
Note that the validation error messages have changed since 1.0.87.
Checklist
* [x] [Meet tenets criteria](https://docs.powertools.aws.dev/lambda-java/#tenets) * [x] Update tests * [ ] Update docs * [x] PR title follows [conventional commit semantics](https://www.conventionalcommits.org/en/v1.0.0/) ## Breaking change checklistRFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.