Smart Validator

[mbasaglia]

Currently using https://ajv.js.org/ for schema validation. Depending on how things work we might want to switch to completely custom validation to ensure the errors are sensible

[mbasaglia]

@b-wils

[mbasaglia]

Ideally I'd like to tag all unknown properties as warnings, maybe with an option to turn them off.

[b-wils]

Very cool, excited to look at this. Some related thoughts I had on validation-

1. We could consider adding discriminators to the schema (https://ajv.js.org/json-schema.html#discriminator) to reduce oneOf error noise. The two big downsides are (a) they aren't part of the json schema spec, though they do have support in some tooling (b) they only work on string types, so we couldn't use them for layers.

2. I likely have some use cases where we want unknown properties as errors. If we just want a list of unknown properties, we could probably create a strict version of the schema and then diff it with the non-strict version to get that list.

[mbasaglia]

Ajv also allows you to perform custom validation, that could also be a possibility

[mbasaglia]

OK so I managed to use AJV builtin functionality to perform validation based on ty. The discriminator stuff was too unflexible so I wrote a custom keyword that does it. Their documentation on the matter was rather lacking so it was quite tricky to figure out but now it works:

[b-wils]

What do you think of converting the schema to use if/then/else instead of oneOf for animated properties? That should improve those error messages and would not need any custom logic so would work with any validator.

One thing I'm not sure of is if that would impact the doc generation tools.

[mbasaglia]

I think it would be harder to maintain, also from what I've heard if/else is not always well supported by tooling, there were some issues in the lottie-docs schema that used if/else more often

[b-wils]

I think "if" would be easier to maintain as we currently repeat the animated definition in each oneOf.

That is interesting with the tooling support though, do you recall roughly long ago that was? If/then/else was added in draft7 (2018) so maybe it just hadn't had widespread adoption yet?

[mbasaglia]

It was like a year or two ago

[mbasaglia]

as we currently repeat the animated definition in each oneOf

Ah my bad, I thought you meant for layers

[mbasaglia]

I think most of the validation is implemented, it would be great if people could test the validator and gave feedback

[mbasaglia]

Also, probably we want to extract the validator as a JS module and publish it for others to use as a library

[b-wils]

I surfaced #65 while testing this out locally. Any ideas why this surfaced in your validation tooling but not other validators? Looks like it is using ajv so I'm surprised at the discrepancy. I also wonder if this could be one of the previous concerns with using "if"

[b-wils]

Looks like this is flagging a and k as unknown properties as well.

[b-wils]

In the interest of getting this in quickly so we can have it for the review period, what do people think about checking this in and we can migrate it's own repo/npm later? Not sure if it makes sense to do the warnings in a follow up if there are still some rough edges that may take time to work out.

[fmalita]

Here's another failing sample: https://www.jsonschemavalidator.net/s/Klsfo2z4

Looks like the shape validation fails, but afaict it does match Rectangle - so not sure what's going on...

[mbasaglia]

Looks like this is flagging a and k as unknown properties as well.

Fixed

[mbasaglia]

Here's another failing sample: https://www.jsonschemavalidator.net/s/Klsfo2z4

Looks like the shape validation fails, but afaict it does match Rectangle - so not sure what's going on...

Validates ok on the validator

[mbasaglia]

Added some custom validation for keyframes, for reference this is a json showing various cases:

{
    "fr": 60,
    "ip": 0,
    "op": 60,
    "w": 512,
    "h": 512,
    "layers": [
        {
            "ty": 4,
            "st": 0,
            "ip": 0,
            "op": 60,
            "ks": {},
            "shapes": [
                {
                    "ty": "el",
                    "s": {
                        "a": 0,
                        "k": [
                            200,
                            200
                        ]
                    },
                    "p": {
                        "a": 1,
                        "k": [
                            {
                                "_comment": "valid, has i/o",
                                "t": 0,
                                "s": [200, 200],
                                "i": {"x": 1, "y": 1},
                                "o": {"x": 0, "y": 0}
                            },
                            {
                                "_comment": "invalid t <= previous t",
                                "t": 0,
                                "s": [200, 200],
                                "i": {"x": 1, "y": 1},
                                "o": {"x": 0, "y": 0}
                            },
                            {
                                "_comment": "valid, has h = 1",
                                "t": 20,
                                "s": [200, 200],
                                "h": 1
                            },
                            {
                                "_comment": "invalid, has h = 0 but no i/o",
                                "t": 30,
                                "s": [200, 200],
                                "h": 0
                            },
                            {
                                "_comment": "invalid, needs i/o",
                                "t": 40,
                                "s": [200, 200]
                            },
                            {
                                "_comment": "valid, last frame",
                                "t": 50,
                                "s": [200, 200]
                            }
                        ]
                    }
                },
                {
                    "ty": "fl",
                    "c": {
                        "a": 0,
                        "k": [
                            0,
                            0,
                            0
                        ]
                    },
                    "o": {
                        "a": 0,
                        "k": 100
                    }
                }
            ]
        }
    ]
}

[mbasaglia]

what do people think about checking this in and we can migrate it's own repo/npm later?

Yeah that was my idea, keep it here until we are ready to publish 1.0 so it's easier to develop

[b-wils]

A couple more minor issues/feedback. Some of these are probably fine to wait for a separate PR

• The keyframe checking seems to add some duplicate errors- https://www.jsonschemavalidator.net/s/Klsfo2z4
• Some cases it thinks the position is split and fails that validation? https://www.jsonschemavalidator.net/s/abvBBILj
• Can we add a message like "no errors" when there aren't any?
• I'm wondering if we should default the warnings to off. There's a decent amount we aren't covering yet and may just be extra noise.
• When uploading, how hard would it be to include the filename or have a text box with the JSON?
• Can we add unit tests like we do in https://github.com/lottie/lottie-spec/blob/main/tests/validate_animations.test.js ? I'm not sure if those still make sense to keep or we could just replace them with the smart validator. Maybe helpful to keep to help narrow what are schema bugs and what are smart validator bugs.

[mbasaglia]

I think most of those points have been fixed. I think it would make sense to use the validator in unit tests as it can provide more granular output than simple pass/fail (it can tell which objects are failing)

[b-wils]

I think most of those points have been fixed. I think it would make sense to use the validator in unit tests as it can provide more granular output than simple pass/fail (it can tell which objects are failing)

Yeah, this is looking great!

[heathmill]

Great to see! Let's get this in so we can start reviews, and can continue to iterate as needed.