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

Bump com.networknt:json-schema-validator to 1.4.0 #393

Merged
merged 2 commits into from
May 14, 2024

Conversation

YanWittmann
Copy link
Contributor

@YanWittmann YanWittmann commented Apr 12, 2024

<version>1.0.87</version>

It would be nice if the dependency to
com.networknt:json-schema-validator:1.0.73 as found in the pom.xml
could be upgraded to the latest 1.4.0, as one of our projects depends on
features in this latest version and therefore conflicts with the
dependency of this project.

This Pull Request implements the changes needed to use the
latest version of the json schema validator. More
specifically, it uses the new JSON Schema Mapper
format for creating the JsonSchemaFactory instance:

final MapSchemaMapper offlineSchemaMapper = new MapSchemaMapper(offlineMappings);
JsonSchemaFactory factory = JsonSchemaFactory.builder(JsonSchemaFactory.getInstance(SpecVersionDetector.detect(schemaNode)))
  .jsonMapper(mapper)
  .schemaMappers(s -> s.add(offlineSchemaMapper))
  .build();

As can be seen in master...YanWittmann:cyclonedx-core-java-bump-validator:master#diff-fc28290f55c35403fc95b45ee2714337621f54b48caba5e01f08d5760b54139a

Also see all the rejected PRs by dependabot to bump the version,
most likely rejected because they introduced that new schema
mapper which breaks the current usage.

 dependency version to 1.4.0.

Signed-off-by: ywittmann <yan.wittmann@metaeffekt.com>
@YanWittmann
Copy link
Contributor Author

Unrelated, but why do you require the description to be wrapped at 72 characters? Just asking to understand.

https://github.com/CycloneDX/.github/blob/0b572a6f8fc922c7ecc268dacb2c2ef33218838f/CONTRIBUTING.md

@mr-zepol
Copy link
Contributor

hey, @YanWittmann this is something we were checking, did you notice something about response time when testing? after testing we saw that after this change the unit test for the JSON validator schema went from 5 to 120 seconds, we are investigating what could be the reason

…mprove loading performance.

Signed-off-by: Yan Wittmann <mail@yanwittmann.de>
@YanWittmann
Copy link
Contributor Author

YanWittmann commented Apr 22, 2024

I'm unsure how I didn't notice this before, but you're right. It takes much longer than before to build the project.
I'm luckily pretty sure that I know what the issue is.

Please view the new implementation of the newJsonSchema method as internally called by the factory.getSchema method in the CycloneDxSchema.java class:

    protected JsonSchema newJsonSchema(final SchemaLocation schemaUri, final JsonNode schemaNode, final SchemaValidatorsConfig config) {
        final ValidationContext validationContext = createValidationContext(schemaNode, config);
        JsonSchema jsonSchema = doCreate(validationContext, getSchemaLocation(schemaUri),
                new JsonNodePath(validationContext.getConfig().getPathType()), schemaNode, null, false);
        if (config.isPreloadJsonSchema()) {
            try {
                /*
                 * Attempt to preload and resolve $refs for performance.
                 */
                jsonSchema.initializeValidators();
            } catch (Exception e) {
                /*
                 * Do nothing here to allow the schema to be cached even if the remote $ref
                 * cannot be resolved at this time. If the developer wants to ensure that all
                 * remote $refs are currently resolvable they need to call initializeValidators
                 * themselves.
                 */
            }
        }
        return jsonSchema;
    }

The if (config.isPreloadJsonSchema()) { ... } is new compared to the old version of the networknt library. Meaning, each time a new parser is created (each time a document is parsed) all of the related JSON Schemas are checked and loaded by default, since the default value of the preloadJsonSchema parameter is true. This setting did not exist before.

By now simply adding a

config.setPreloadJsonSchema(false);

to the class I edited, this issue is resolved; all tests still pass (as is to be expected) and the runtime is once again down to 2.5 seconds for the test cases on my machine for both versions.

I've pushed this change to my PR.

Thanks for taking the time to review this!

@mr-zepol
Copy link
Contributor

mr-zepol commented May 9, 2024

I'm unsure how I didn't notice this before, but you're right. It takes much longer than before to build the project. I'm luckily pretty sure that I know what the issue is.

Please view the new implementation of the newJsonSchema method as internally called by the factory.getSchema method in the CycloneDxSchema.java class:

    protected JsonSchema newJsonSchema(final SchemaLocation schemaUri, final JsonNode schemaNode, final SchemaValidatorsConfig config) {
        final ValidationContext validationContext = createValidationContext(schemaNode, config);
        JsonSchema jsonSchema = doCreate(validationContext, getSchemaLocation(schemaUri),
                new JsonNodePath(validationContext.getConfig().getPathType()), schemaNode, null, false);
        if (config.isPreloadJsonSchema()) {
            try {
                /*
                 * Attempt to preload and resolve $refs for performance.
                 */
                jsonSchema.initializeValidators();
            } catch (Exception e) {
                /*
                 * Do nothing here to allow the schema to be cached even if the remote $ref
                 * cannot be resolved at this time. If the developer wants to ensure that all
                 * remote $refs are currently resolvable they need to call initializeValidators
                 * themselves.
                 */
            }
        }
        return jsonSchema;
    }

The if (config.isPreloadJsonSchema()) { ... } is new compared to the old version of the networknt library. Meaning, each time a new parser is created (each time a document is parsed) all of the related JSON Schemas are checked and loaded by default, since the default value of the preloadJsonSchema parameter is true. This setting did not exist before.

By now simply adding a

config.setPreloadJsonSchema(false);

to the class I edited, this issue is resolved; all tests still pass (as is to be expected) and the runtime is once again down to 2.5 seconds for the test cases on my machine for both versions.

I've pushed this change to my PR.

Thanks for taking the time to review this!

Hey @YanWittmann thanks for looking into it, I just checked, and indeed the test time is almost the same.

@mr-zepol
Copy link
Contributor

mr-zepol commented May 9, 2024

+1 LGTM, thanks for contributing to the project

@stevespringett stevespringett merged commit 79ef783 into CycloneDX:master May 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants