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

Use meta-schemas to validate input schemas #198

Closed
Stranger6667 opened this issue Apr 29, 2021 · 5 comments · Fixed by #225
Closed

Use meta-schemas to validate input schemas #198

Stranger6667 opened this issue Apr 29, 2021 · 5 comments · Fixed by #225

Comments

@Stranger6667
Copy link
Owner

Stranger6667 commented Apr 29, 2021

If the input schema is invalid during the schema compilation process, then CompilationError is emitted. It has no useful info inside, which leads to a bad UX in such cases.

It will be much better to use meta-schemas and existing ValidationError in such circumstances:

  • Meta-schemas are known to be valid;
  • Could be globally reused via lazy_static;
  • Validation error will have much more verbose error messages;
  • instance_path will be available in those errors;
  • Adding schema_path to validation errors in the future will benefit the compilation process automatically.

Also, if meta-schemas will be sufficient, then CompilationError might be deleted. Even if not, then constructing ValidationError instead of them will provide much more useful output.

But it is a backward-incompatible change, which still seems worth it.

Roughly required steps are:

  • Create different validators within lazy_static! in compilation::options. Probably it will require a slight refactoring there as meta-schemas are already included in the resulting binary - it will be nice to reuse those serde_json::Value values. It should be fine to use expect there as those validators are known to be valid;
  • Make the input schema validation configurable - these schemas comply with themselves, which is a nice example of recursion - i.e. their validation depends on themselves :) So there should be a way to either force using some concrete meta-schema validation or disable it completely;
  • Choose one of those validators within compilation::options::CompilationOptions::compile depending on the detected draft and the configuration logic from the previous step;
  • Perform schema validation with this schema;
  • Change the return type of the compile function to ValidationError;
  • At this point, if schema validation is performed, it should not be possible to encounter some CompilationError down in compilation logic - but I am not 100% sure about it. Maybe we can create some "InvalidSchema" variant to ValidationError for such cases when the meta schema didn't catch the error. I assume that it is either pretty rare or even not possible, so having one descriptive extra variant in ValidationError seems like a good option instead of having unreachable! or expect that are panicking.
@Stranger6667
Copy link
Owner Author

@aedeny is willing to work on this issue :)

@aedeny
Copy link
Contributor

aedeny commented May 9, 2021

Hi @Stranger6667, can you please remind me how should the new validators be created from within the lazy_static! section in compilation::options? I remember you showed me this part.

@Stranger6667
Copy link
Owner Author

Stranger6667 commented May 9, 2021

Hi @aedeny !

Thanks for asking!

First, we got to reuse schemas as serde_json::Value:

lazy_static::lazy_static! {
    static ref DRAFT_4: serde_json::Value = serde_json::from_str(include_str!("../../meta_schemas/draft4.json")).expect("Valid schema!");
    ...  // Similar with other schemas
    static ref META_SCHEMAS: AHashMap<String, Value> = {
        let mut store = AHashMap::with_capacity(3);
        store.insert(
            "http://json-schema.org/draft-04/schema".to_string(),
            DRAFT_4
        );
        ...
        store
    };
}

Then validators could be compiled like this:

lazy_static::lazy_static! {
    static ref META_SCHEMA_VALIDATORS: AHashMap<draft::Draft, JSONSchema<'static>> = {
        let mut store = AHashMap::with_capacity(3);
        store.insert(
            draft::Draft::Draft4,
            JSONSchema::compile(DRAFT_4).expect("Valid metaschema")
        );
        ...
        store
    };
}

I didn't check whether the code above works, but it should be like this or similar (probably with some .clone()), so META_SCHEMA_VALIDATORS then could be used in compilation.

@aedeny
Copy link
Contributor

aedeny commented May 10, 2021

@Stranger6667 - Thanks for the help and explanation! I've applied your suggestions 😃

As I understand we should also change the return type of each validator's (e.g. MaxLengthValidator) compile method from CompilationResult to a new type with ValidationError, such as the following:

pub(crate) type ValidationResult<'a> = Result<BoxedValidator, error::ValidationError<'a>>;

And for the validators:

impl MaxItemsValidator {
    #[inline]
    pub(crate) fn compile(schema: &Value) -> ValidationResult {
        if let Some(limit) = schema.as_u64() {
            Ok(Box::new(MaxItemsValidator { limit }))
        } else {
            Err(ValidationError...) // not sure what to do here
        }
    }
}

Since we currently don't have context to populate the ValidationError here, what should be done here?

@Stranger6667
Copy link
Owner Author

Stranger6667 commented May 11, 2021

You're very welcome! :)

As I understand we should also change the return type of each validator's (e.g. MaxLengthValidator) compile method from CompilationResult to a new type with ValidationError, such as the following:

Exactly! :)

Since we currently don't have context to populate the ValidationError here, what should be done here?

There is a special kind of ValidationError - ValidationErrorKind::Schema that is created with ValidationError::schema(). We can use it and extend a bit - so instead of not accepting any argument, it can take schema for its instance attribute:

pub(crate) fn schema(instance: &'a Value) -> ValidationError<'a> {
    ValidationError {
        instance_path: JSONPointer::default(),
        instance: Cow::Borrowed(instance),
        kind: ValidationErrorKind::Schema,
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants