Stranger6667 / jsonschema-rs

JSON Schema validation library
https://docs.rs/jsonschema
MIT License
482 stars 90 forks source link

feat: Custom keyword validation #473

Closed samgqroberts closed 2 months ago

samgqroberts commented 2 months ago

This was originally put up by @bnjt here https://github.com/Stranger6667/jsonschema-rs/pull/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 commented 2 months ago

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

samgqroberts commented 2 months ago

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

Stranger6667 commented 2 months ago

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 commented 2 months ago

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:

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 commented 2 months ago

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 commented 2 months ago

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

Stranger6667 commented 2 months ago

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

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

Stranger6667 commented 2 months ago

Update:

There are not that many things left:

codecov[bot] commented 2 months ago

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 :warning:
jsonschema/src/keywords/custom.rs 87.50% 3 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Stranger6667 commented 2 months ago

I think it is ready to merge!

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

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 commented 1 month ago

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

Stranger6667 commented 1 month ago

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