Fixing unevaluated properties with larger test base

[prashanthjos]

@stevehu I have made some changes to unevaluated properties. As suggested by you I have added all the tests from https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/tests/draft2019-09/unevaluatedProperties.json to
json-schema-validator/src/test/resources/schema/unevaluatedTests/unevaluated-tests.json. I dint want to read the http url from the tests, so I just copied them.

Also unevaluatedProperties implementation just accepts a boolean value and not a schema today. I have three tests failing from the above tests and remaining all are passing. I will be working on fixing those tests soon.

[prashanthjos]

@stevehu also I see there is no CI running on the PR's. Would this result in compilation issues escaping to master branch?

[stevehu]

@prashanthjos I think we can add CI to all the PRs and it makes sense to do the build before we merge it. At the moment, I am building it and doing the directory comparison between the PR branch and the master. BTW, I have merged one PR and the AllOfValidator has conflicts now. I am wondering if you could take a look. Thanks.

[stevehu]

@prashanthjos Thanks a lot for the fix. Since we have a memory leak issue in the master. I am wondering if we should wait until that issue is resolved before merging this issue. Please let me know what do you think?

[prashanthjos]

@stevehu looks like master build is failing because of test failures after the last merge. Can you please take a look.

[stevehu]

I have rolled back the other PR. Thanks.

[prashanthjos]

@stevehu looks like users are not realising the correct usage of CollectorContext and resetting it correctly. So I am going to clear everything from CollectorContext for the existing walk and validate methods. I will add new validate and walk methods that will take an additional boolean parameter resetCollectorContext or we can add an additional attribute in SchemaValidatorConfig about resetting the CollectorContext. Please let me know your thoughts. I will update the same PR.

[stevehu]

@prashanthjos Most users just want to validate the JSON with the schema and they might not understand the implementation details. It is our responsibility to make the library easy to use for basic functionalities and additional features for advanced users with customized configurations. Even if we put an additional parameter, some users will have trouble understanding how to use it and they might just copy the code from others without understanding the usage of the indicator. In my opinion, it is best to add a config property and the default is to reset the collector context. For users who want to maintain context across validators/walkers, they can set this to false. What do you think?

[AndreasALoew]

I think both proposals as described above by @prashanthjos and @stevehu are viable.

Which one is preferable IMO also depends on the scenario for keeping and reusing CollectorContexts throughout a series of multiple validations (which I admit I still haven't really understood). If it is possible that both approaches can be dynamically mixed and matched, then the additional parameter to validate() and walk() seems more appropriate, while in contrast in case it is a strict either-or whether you want to reuse CollectorContexts throughout more than one message processing or not, a config property whould be sufficient and even easier from a user perspective, as it might be a little more streamlined with the existing config approach...

So can you describe the use case for reusing a CollectorContext across multipe requests? This should help im making the best decision about which way to implement it.

[prashanthjos]

@stevehu @AndreasALoew can you please have a look at this PR let me know your comments. I fixed the CollectorContext reset issue along with unevaluated properties test fix.

[AndreasALoew]

@prashanthjos From everything that I can tell (without having run the reproducer myself to definitely confirm) it looks to me that your changes do properly reset thread local storage at the end of validate() and walk() and therefore, successfully prevent the memory leak.

Sorry, as I am pretty new to the internals of the validator, I cannot really comment on the details of the additional refactorings/code cleanups that you have done - they also look ok at my first glance...

[SiemelNaran]

In this PR in JsonSchema.java they call CollectorContext.getInstance().reset(). But is it possible for JsonSchema.java to call itself recursively, as after all the code is:

        for (JsonValidator v : getValidators().values()) {
            errors.addAll(v.validate(jsonNode, rootNode, at));
        }

And is it possible for v to itself be a JsonSchema? I don't know if it is possible, but if it is then it may mean that resetCollectorContext should be true only for the outermost call, and the inner calls to JsonSchema.validate should not reset the context as the outer layer may need it.

But not sure, just an open question.

[SiemelNaran]

There should be tests for the auto resetting. For example in JsonWalkApplyDefaultsTest and other tests they have code like this

    @AfterEach
    void cleanup() {
        CollectorContext.getInstance().reset();
    }

I had to add the above others mvn test was failing. But I just checked and you can now remove these lines, and mvn test still passes. So lines like these should be removed from all tests wherever possible, as the tests are an example of how to use the library.