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

Validation based on Serde #40

Closed
wants to merge 4 commits into from
Closed

Conversation

tamasfe
Copy link
Contributor

@tamasfe tamasfe commented Jun 4, 2020

I'm not sure whether this should be done in a separate crate or not as it is quite a large addition, and isn't about generation, but I thought I'd start here anyway.

Also this has got to be my longest PR description thus far, sorry!

The goal of this PR is to implement validation of values with unknown structures against Schemas.

For example:

let schema_value = json! {
    {
        "$schema": "http://json-schema.org/draft-07/schema#",
        "title": "SomeStruct",
        "type": "object",
        "required": [
          "some_inner",
          "some_int"
        ],
        "additionalProperties": false,
        "properties": {
          "some_inner": {
            "type": "object",
            "required": [
              "inner_values",
              "inner_value"
            ],
            "properties": {
              "inner_values": {
                "type": "array",
                "maxItems": 2,
                "items": {
                    "type": "string"
                }
              },
              "inner_value": {
                "type": "integer",
                "enum": [1, 3]
              }
            }
          },
          "some_int": {
            "type": "integer",
            "format": "int32"
          }
        }
      }
};

let value = json! {
    {
        "some_inner": {
          "inner_value": 2,
          "inner_values": ["value", 2]
        },
        "unexpected_property": 2
    }
};

let schema = serde_json::from_value::<RootSchema>(schema_value).unwrap();
let valid = schema.validate(&Spanned::new(&value, KeySpanner::new()));

if let Err(errors) = valid {
    for error in errors {
        println!(
            "({span}) {err}",
            span = error.span.map(|s| s.join(".")).unwrap_or_default(),
            err = error.value
        )
    }
}
// (some_inner.inner_value) invalid enum value, expected to be one of 1, 3
// (some_inner.inner_values.1) invalid type, expected "String"
// (unexpected_property) value is not allowed here
// () the required property "some_int" is missing

It doesn't touch any derive features.

There are some unresolved questions and limitations that I haven't been able to solve yet.

My initial criteria were:

  • Values of probably unknown structure (similar to serde_json::Value)
  • Unknown external JSON-serialized schema
  • Validation errors should be easily associated with the values that caused them without any traversals or searches after validation

Notable traits

Validate

Modelled after Serde's Serialize it allows validation of a value with a Validator. It also has an associated Span type which the Validator can use in its errors.

Validator

Modelled after Serde's Serializer with a bit less complexity, it provides interface for validation of values of different types.

Span

Any kind of information about a value, usually probably a span of bytes or characters, but can be anything.

Spanner

It's a new word in this sense, but it is rather logical to me, it is used to provide spans for values.

Notable types

Spanned

A wrapper over Serde Serialize values, it implements Validate and also implements Serializer (in an inner type that is not exposed) that just proxies calls to the Validator while collecting errors and using a given Spanner to provide spans.

Features

validation

Everything in this PR is under a new feature called validation, and it is not enabled by default.

Added Dependencies

  • regex: for pattern validations
  • smallvec: premature optimization because why not.

Unresolved questions and missing things

  • The format field is ignored for now.
  • Serde's "variant" (externally tagged) types are not yet handled and will panic
  • The schema itself is not validated, and an invalid schema might cause panic (like an invalid regex)
  • Validation will not work with $refs in the schema, and they have to be resolved before validation, I haven't found anything that does this, does it exist, or is it planned?
  • Tests, a lot of tests should be added after the above points are resolved.
  • Map keys have a ToString bound, this issue is actually the only one that would tie this implementation to this crate, otherwise it's not a problem.

There are also a few "TODO" comments that should be resolved before this is merged (if ever).

@tamasfe tamasfe mentioned this pull request Jun 4, 2020
@tamasfe
Copy link
Contributor Author

tamasfe commented Jun 5, 2020

After thinking about this a bit, it would probably be better to create a separate crate that "consumes" schemas named something like "schemars-validate" or something more generic. It would do validation, $ref resolution and whatnot.

What do you think?

@mocsy
Copy link

mocsy commented Jun 10, 2020

Is this about validating the schema generated by schemars?

@tamasfe
Copy link
Contributor Author

tamasfe commented Jun 10, 2020

No, this is for validating the actual values with a schema. However schemas could validate each other as they implement Serialize.

I've ended up writing a more general validation library that will provide integration with Schemars and include most of this PR. So this is obsolete now.

@tamasfe tamasfe closed this Jun 10, 2020
@tamasfe tamasfe deleted the validation branch June 10, 2020 11:59
@mocsy
Copy link

mocsy commented Jun 10, 2020

I think we would need support in schemars for JSON Schema Validation spec in order to be able to describe the validation requirements.

And then actual validator libraries like yours which leverage/augment that support.

We need the constraints be present in the schema output in the end.

@tamasfe
Copy link
Contributor Author

tamasfe commented Jun 10, 2020

Definitely, this PR had nothing to do with that part unfortunately. There was some discussion in #12, but I haven't seen any progress on it yet.

@GREsau
Copy link
Owner

GREsau commented Jun 11, 2020

Sorry for my lack of response - but at a glance I'd agree this makes most sense to me as a separate crate

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

Successfully merging this pull request may close these issues.

3 participants