Validate content of folder items

[mtojek]

I'm opening this as draft for visibility of the progress and changes.

Fixes: #38

This PR adds support for validating file content (JSON or YAML) against JSON-schema.

TODO:

• extract folder item to a separate file
• call item validate's method
• load JSON-schema from YAML file
• load folder item content based on the content-type (MIME)
• validate the file content and report problems
• code cleanup
• I went through integrations and adjusted few schema files to make them passing (mostly missing field types)
• refs to external item schemas are not supported at the moment
• support inline specs for folder items

Notes:

• YAML fields encoded like:

conditions:
  kibana.version: "^7.10.0"

... are not extended to:

conditions:
  kibana:
    version: "^7.10.0"

If there is a quick and nice fix, I can apply it. At the moment I enforced the upper version.

[elasticmachine]

💚 Build Succeeded

Expand to view the summary

Build stats

• Build Cause: [Pull request #41 updated]

• Start Time: 2020-08-31T22:06:51.521+0000

• Duration: 3 min 21 sec

[ycombinator]

Aren't we duplicating this spec? That's why I had the $ref originally.

[mtojek]

Yes, but the provided loader doesn't support the statik filesystem. I can try to implement a custom resource loader.

Also, the library enforces a canonical path here with schema, so it would be: file://./dataset/manifest.spec.yml#streams/items/properties/vars

[ycombinator]

I see. I think the duplication is fine for now but we should probably make comments in both places pointing to the other as a duplicate that needs to be kept in sync whenever any one of the places is changed. Longer term, let's look into implementing a custom resource loader.

[mtojek]

I will try to focus on this as followup. Got interested with this :)

[ycombinator]

What is the schema is defined inline and not referenced via $ref?

[mtojek]

not sure if I follow you...
this might be bad wording, should be: schema reference not provided. Does it sound better?

[ycombinator]

Sorry, I was referring to needing something like this: https://github.com/elastic/package-spec/blob/master/code/go/internal/validator/folder.go#L125-L141. Maybe this is not the right place for it but then it should be added where this method is called from.

[mtojek]

Ah, now I see... I totally missed the case where schema is defined inline, didn't know about this. I will improve the implementation.

[mtojek]

@ycombinator Hm.. I dived into this topic and got a bit confused. Do we really need it? What would a use case for this? This $ref refers to a place in folder schema in which we refer to item's schema. It's not part of the JSON-schema, so I believe we can enforce to keep it in a separate file (to separate concerns and prevent from creating a huge all-in-one files).

tl;dr $ref in folder schema is always external.

WDYT?

[ycombinator]

So in the folder schema, there can be two types of items: folder (for describing the structure of a sub-folder within the folder) and file (for describing the structure of a file within the folder).

For type: folder, we already have a couple of places in the spec where use $ref (example) and a couple other places where we inline the definition instead of using $ref (example).

For type: file, I checked, and at the moment we only have places where we use $ref (example). We do not yet have any places where we are inlining the spec (aka schema) for the file itself. However, I can see this being useful for simple file specs (example).

So I think there is a use case for it and we should support it.

Also, if we allowing inline specs for sub-folder items but not for file items, we are introducing an inconsistency. We could, of course, resolve this inconsistency by going the other way: that is, we can say that we only allow $refs for item specs, regardless of whether those are folder items or file items; that we do not accept inline specs for either item type. I think this is slightly inconvenient when the item has a small spec but I'd be okay with this approach too. If you prefer this, then let's make a separate PR to first remove support for inline specs for type: folder items and adjust the spec files accordingly.

[ycombinator]

Not for this PR but we could probably convert the schema YAML files to JSON as part of the make update step so we don't have to do this yaml.Unmarshal + json.Marshal dance every time at run time.

[mtojek]

Yes, this could be an improvement in the future (convertion is always error prone), although I admin that YAML here is much more human readable and easier to interact.

[ycombinator]

Indeed, which is why we'd keep the source spec files in YAML but only convert them to JSON when make update is run.

[mtojek]

Ok, I fixed the issue with schema referenced inside item specs. The solution is that all "exported" elements must be specified under definitions. Then we can refer to them (out of the box, without JSON pointer magic) via:

$ref: "./dataset/manifest.spec.yml#/definitions/vars"

(for input vars)

[ycombinator]

The solution is that all "exported" elements must be specified under definitions

Hmm, according to https://json-schema.org/understanding-json-schema/structuring.html it's not required to put the "exported" elements under definitions:

Since we are going to reuse this schema, it is customary (but not required) to put it in the parent schema under a key called definitions:

Maybe this is something required by the library we are using for validating JSON schema. Anyway, good find and it's actually quite nice to have a well-defined location for "exported" vars so 👍 all around!

[ycombinator]

LGTM. Thanks for implementing this feature — it's going to fill a major gap in our validation.