feat: Custom keyword validation

[samgqroberts]

This was originally put up by @bnjt here #394. That repository was deleted which deleted the PR, so this PR re-creates that PR.

Additionally, this PR adds an improvement to the original PR whereby existing keywords can also have their behavior be overridden by custom logic.

Resolves #379
Resolves #354

[Stranger6667]

Great work! The main point I'd like to discuss is whether there should be a way to store extra, user-defined state inside custom validators. What do you think?

[Stranger6667]

I am thinking about whether it would give more flexibility if there was a trait instead? This way it would be possible to have some inner state inside

[samgqroberts]

That sounds like a good idea, but a few clarifying questions: do you mean creating a trait like CustomValidator with the validate and is_valid functions as methods on it, and then the user would define a struct that implements CustomValidator and provide it to CompilationOptions (in like a Box I think)? If so perhaps it should be combined somehow with the Validator trait?

Also while instinctually this feels like the right move, I'm struggling a bit to come up with a concrete use case example, like for a test case. There's this #394 (comment), but I wonder if that doesn't need state internal to the validator, the validate / is_valid functions could potentially just reference the lookup table. Something like that's probably worth a test case anyway also.

[Stranger6667]

do you mean creating a trait like CustomValidator with the validate and is_valid functions as methods on it, and then the user would define a struct that implements CustomValidator and provide it to CompilationOptions (in like a Box I think)? If so perhaps it should be combined somehow with the Validator trait?

Yep, or, maybe exposing the Validator trait?

Also while instinctually this feels like the right move, I'm struggling a bit to come up with a concrete use case example, like for a test case

An example that I have in mind is a counter. Even though it might sound trivial, it unblocks interesting use cases like collecting usage statistics from different keywords (i.e. if the user "wraps" the existing keyword implementation with such a counter), which in turn is useful for building coverage maps for test cases sent to API endpoints (documented with Open API), which in turn is useful for coverage-guided fuzzing of such endpoints.

[Stranger6667]

I got to set rustfmt.toml to make sure everything is consistently formatted, but generally the imports are grouped in this crate.

[Stranger6667]

Btw, I merged #474 so, rustfmt now checks for such cases

[Stranger6667]

I see that the CustomIsValidFn function uses regular references, do we need Arc here? I.e. how would the signature look like otherwise as you mention extra lifetimes

[samgqroberts]

@Stranger6667 unfortunately I think I've hit the end of my expertise with trait objects, Arc, Mutex, Box, etc. Everything else compiles, however I can't figure out how to maintain the type of validator here so that I can access the count member later on in assertions.

error[E0308]: mismatched types
   --> src/compilation/mod.rs:623:62
    |
623 |         let options = options.with_custom_keyword("countme", validator.clone());
    |                               -------------------            ^^^^^^^^^^^^^^^^^ expected `Arc<Mutex<Box<...>>>`, found `Arc<Mutex<Box<CountingValidator>>>`
    |                               |
    |                               arguments to this method are incorrect
    |
    = note: expected struct `Arc<std::sync::Mutex<Box<(dyn for<'instance, 'schema> CustomKeywordValidator<'instance, 'schema> + 'static)>>>`
               found struct `Arc<std::sync::Mutex<Box<CountingValidator>>>`
    = help: `CountingValidator` implements `CustomKeywordValidator` so you could box the found value and coerce it to the trait object `Box<dyn CustomKeywordValidator>`, you will have to change the expected type as well

Also, I'm not thrilled with how the with_custom_keyword interface requires an Arc of a Mutex of a Box of the thing... for non inner state use cases that's unnecessary (see the other custom keyword tests above), but I'm not sure how to support both. Any ideas for either problem? (including suggestions like I've modeled this completely wrong lol)

[Stranger6667]

Looking at it right now. I'll push a few commits to your branch which will this work and then we can continue discussing the interface :)

[Stranger6667]

Here are a few thoughts:

Given the inner state, I think that custom_keywords should store initialization functions (boxed this time, unlike formats), so such validators won't be shared across multiple locations in the schema. This will remove the need for Arc<Mutex<T>>, so no unconditional locking when any custom validator is used - stateless validators will be more performant. Alternatively, we can store something that implements Validator + Default, which is likely a better option.

[samgqroberts]

Hey, so, I think again my inexperience is showing. My attempts with Validator + Default fail because that cannot be a trait object, and if we try to store a fn in CompilationOptions we can't capture variables when in the closure and that wouldn't be able to clone the count in the inner state test, and if we try to store a Fn then that's either not thread safe or not cloneable. Any ideas?

[Stranger6667]

Oh, I haven't thought about the trait object thing. Let me play with the code a bit

P.S. Offtopic - I've put together some ideas about this library's 1.0 API and would appreciate any feedback (see #475)

[Stranger6667]

Done in a240bd5! However, there are a few issues:

• Initialization functions require &'static so, it is problematic to pass a value there (like a counter) and then read from it without leaking.
• The API does not spark joy :( However, given that there could be multiple occurrences of the same keyword, every one of them should have its separate state, so it is probably the only way to provide this kind of capability. I.e. - each validator is unique, so we need to give a way to initialize each of them. With stateless validators, things could be simpler.

Not 100% sure if it is worth the trouble and if the use case I mentioned above could be implemented differently

[samgqroberts]

It's interesting that Rust can't preserve that this is a CountingValidator while passing it to a function that expects a dyn CustomKeywordValidator. That's what I was getting hung up on. The inner state being behind another Arc<Mutex<T>> seems like a good alternative.

[Stranger6667]

Technically, Rust preserves this information, but behind an indirection. Dynamic dispatch "knows" at runtime what kind of object is there, so it can do a vtable lookup and find the relevant implementation of methods from CustomKeywordValidator. It is also possible to downcast dyn CustomKeywordValidator to CountingValidator via downcast_ref, though it is not that convenient to use.

Without this kind of indirection, it would not be clear how much space to allocate on the stack for that type + as we store it in a hashmap, we can only use boxed trait objects.

[Stranger6667]

Let me know if there is anything I can do to help with moving this forward :)

[samgqroberts]

@Stranger6667 sounds good! day job has taken over this week but I'll dig back into this PR this weekend

[Stranger6667]

I am thinking about moving the internal state of CompiledCustomKeywordValidator to the inner validator, so if they are not needed, the user may choose to keep the overall structure smaller, i.e. pushing this decision to the user rather than always storing it (as in compile_custom_keyword_validator)

So, instead of closure without arguments, it could be an equivalent of compile-like functions which will return Box<CustomValidator> which in turn will be stored in the wrapper.

However, at this point, the CustomKeywordValidator trait looks very akin to Validator, so I am thinking if it should be used instead, this way there will be one less indirection + we'll reuse the existing code. Let me know what you think!

[Stranger6667]

I have an idea how what the API could look like:

#[derive(Debug)]
struct AsciiKeyword {
    size: usize
}

impl jsonschema::CustomKeyword for AsciiKeyword {
    fn is_valid(&self, instance: &Value) -> bool {
        todo!()
    }
}

fn ascii_keyword_factory(schema: &Value) -> Arc<dyn jsonschema::CustomKeyword> {
    Arc::new(AsciiKeyword { size: 42 })
}

let validator = JSONSchema::options()
    .with_keyword(
        "ascii",
        |schema| -> Arc<dyn jsonschema::CustomKeyword> {
            Arc::new(AsciiKeyword { size: 42 })
        }
    )
    .with_keyword("also-ascii", ascii_keyword_factory)
    .compile(&schema)
    .unwrap();

I've implemented it for the new jsonschema version - at least it compiles and the code above works. Here is the implementation:

• Traits - CustomKeyword that provides the relevant methods (is_valid, etc), CustomKeywordFactory is what is actually stored inside options, and is implemented for functions that accept 1 argument of the Value type. Those should be extended to match all the needed arguments from the current implementation
• Function to add new keywords

With some adjustments (mainly for other arguments like path, etc) it should work

P.S. I haven't solved how to use such trait objects just yet, but I think it should be possible

[Stranger6667]

I'll push some changes to make things work with the API I described above, though there is still enough room to adjust compilation / is_valid / validate signatures so that your use case is still possible

[samgqroberts]

@Stranger6667 Thanks for running with this. I'll be ready to take another look after your updates.

[Stranger6667]

I'll add some more changes soon! Here are a few observations:

• Some logic should be moved to the "build" phase, so as soon as we know that the schema is invalid it should be reported. Hence the keyword factory should return Result
• Should custom validators return a single error? It may simplify the logic a bit

Otherwise, I think that the path forward is more or less clear

[Stranger6667]

Update:

There are not that many things left:

• Tune privacy of some types (InstancePath)
• Update docs (missing docs + doctests)
• Tests for schema errors inside custom keywords
• Custom validation errors (there should be a way to provide a custom error message)
• Expose Validate instead of Keyword (they are more or less the same)
• Try to make keyword factories work with closures without adding a lifetime to CompilationOptions
• Extract changes related to paths.rs to a separate PR

[codecov]

Codecov Report

Attention: Patch coverage is 94.66192% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 90.02%. Comparing base (092b573) to head (b284f75).

Files	Patch %	Lines
jsonschema/src/compilation/mod.rs	94.59%	12 Missing ⚠️
jsonschema/src/keywords/custom.rs	87.50%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
+ Coverage   89.70%   90.02%   +0.32%     
==========================================
  Files          57       58       +1     
  Lines        9643     9923     +280     
==========================================
+ Hits         8650     8933     +283     
+ Misses        993      990       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Stranger6667]

I think it is ready to merge!

From what I see it should be possible to implement all the cases mentioned in different issues:

• Original cases in this PR by @samgqroberts (they could be seen in tests)
• Cases originally added by @bnjt in feat: Custom keyword validation #394 (their reworked versions are in this PR)
• File path validation described by @tamasfe in Custom Validators #379
• Sub-schema-based validation from tests in feat: custom keywords #354 by @antouhou (custom min-max logic could be re-implemented in a custom validator quite easily)

Please, let me know if there is anything missing to cover your use cases. I'd be glad to work on them here, or in a follow-up PR.

Thank you all for opening those issues and providing a lot of details and context, I appreciate it a lot! Sorry for the delay, but I hope to get this feature released this week.

cc @eirnym @platoscave as you participated in #394, feel free to leave your comments here :)

[samgqroberts]

👏 Thank you @Stranger6667 – the final draft looks great for my use case

[Stranger6667]

Thank you @samgqroberts ! Let me know if you need anything else from my side :) I’d be happy to chat