Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add schema paths to compilation context; this change requires changin… #226

Conversation

DzikiChrzan
Copy link
Contributor

Add schema paths to compilation context; this change requires changing mutability of compilation context

Work in progress

#199

Copy link
Owner

@Stranger6667 Stranger6667 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is something I had in mind :) The invariant we should get in the end is - Looking up the resulting JSON Pointer in the input schema should lead to the sub-schema that failed validation.

For example:

{
    "properties": {
        "foo": {
            "type": "string"
        }
    }
}

In the schema above, the stack for the type validator should look like this - ["properties", "foo"]. Converting it to a JSON Pointer will give us /properties/foo and passing it to schema.pointer (assuming it is serde_json::Value) should return {"type": "string"}:

#[test]
fn test_schema_path() {
    let schema = json!({
        "properties": {
            "foo": {
                "type": "string"
            }
        }
    });
    let instance = json!({"foo": 42});
    let compiled = JSONSchema::compile(&schema).unwrap();
    let result = compiled.validate(&instance);
    let error = result.expect_err("Errors").next();
    let pointer = error.schema_path.to_string();
    assert_eq!(schema.pointer(&pointer), Some(&json!({"type": "string"})))
}

) -> CompilationResult {
context.schema_path.push(schema.to_string());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it should be context.schema_path.push("additionalItems".to_string());

let mut schemas = Vec::with_capacity(items.len());
for item in items {
context.schema_path.push(item.to_string());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we need indexes within items - e.g., apply enumerate to the items above and store indexes like strings. Similar applies to anyOf validator implementation below.

) -> Result<BigValidatorsMap, CompilationError> {
let mut properties = AHashMap::with_capacity(map.len());
for (key, subschema) in map {
context.schema_path.push(subschema.to_string());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be key instead of subschema

) -> Result<SmallValidatorsMap, CompilationError> {
let mut properties = Vec::with_capacity(map.len());
for (key, subschema) in map {
context.schema_path.push(subschema.to_string());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be key instead of subschema

@Stranger6667
Copy link
Owner

Hi @DzikiChrzan ! Let me know if I can help here :)

@Stranger6667 Stranger6667 force-pushed the report-schema-paths-in-validation-errors-199 branch from 6791aec to 61fa7ab Compare June 18, 2021 12:12
@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #226 (a6f1b8b) into master (865c70b) will decrease coverage by 0.31%.
The diff coverage is 85.53%.

❗ Current head a6f1b8b differs from pull request most recent head d279ceb. Consider uploading reports for the commit d279ceb to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
- Coverage   82.89%   82.57%   -0.32%     
==========================================
  Files          53       53              
  Lines        3660     3869     +209     
==========================================
+ Hits         3034     3195     +161     
- Misses        626      674      +48     
Impacted Files Coverage Δ
jsonschema/src/error.rs 62.70% <28.57%> (-1.19%) ⬇️
jsonschema/src/keywords/additional_items.rs 81.81% <58.33%> (-3.19%) ⬇️
jsonschema/src/keywords/exclusive_maximum.rs 76.27% <58.33%> (-3.73%) ⬇️
jsonschema/src/keywords/exclusive_minimum.rs 79.66% <58.33%> (-4.34%) ⬇️
jsonschema/src/keywords/additional_properties.rs 82.87% <60.97%> (-0.92%) ⬇️
jsonschema/src/compilation/mod.rs 93.82% <66.66%> (-1.18%) ⬇️
jsonschema/src/keywords/if_.rs 81.00% <66.66%> (-3.45%) ⬇️
jsonschema/src/keywords/multiple_of.rs 86.44% <80.00%> (-3.56%) ⬇️
jsonschema/src/keywords/enum_.rs 76.78% <83.33%> (-0.30%) ⬇️
jsonschema/src/keywords/maximum.rs 82.69% <83.33%> (-0.65%) ⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 865c70b...d279ceb. Read the comment docs.

@Stranger6667
Copy link
Owner

Stranger6667 commented Jun 18, 2021

I made some progress on this (I hope you don't mind), and it seems like there is not that much left to do. My current todo items are:

  • Write tests for each validator separately and fix cases that I missed (e.g., some combined validators might require some changes in validate)
  • Compare schema_path with what Python's jsonschema is doing for the test corpus
  • Extend Python's error messages with schema_path info
  • Improve variable / method naming
  • Introduce an enum for keywords to avoid some heap allocations during compilation & error reporting
  • Minor optimizations to avoid extra work during compilation

Changing ValidationError::schema calls with the proper error types & paths will be outside of this PR.

@Stranger6667 Stranger6667 force-pushed the report-schema-paths-in-validation-errors-199 branch from a6f1b8b to d279ceb Compare June 19, 2021 14:54
@Stranger6667 Stranger6667 marked this pull request as ready for review June 19, 2021 14:54
@Stranger6667 Stranger6667 merged commit 561f9cd into Stranger6667:master Jun 19, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants