cdevents / spec

A common specification for Continuous Delivery events
Apache License 2.0
129 stars 22 forks source link

Link's ref path needs an update for all the event schemas #211

Closed rjalander closed 4 months ago

rjalander commented 4 months ago

The ref path needs to be updated with the correct path to refer links, From

"$ref": "/schema/links/embeddedlinksarray.json"

To

"$ref": "./links/embeddedlinksarray.json"

afrittoli commented 4 months ago

Thanks, I also realised that while working on the golang SDK. I'm just waiting to see if there is any more issue, and then I'll make a PR.

afrittoli commented 4 months ago

I believe a reference "$ref": "links/embeddedlinksarray.json" would work too.

afrittoli commented 4 months ago

According to the jsonschema docs, schema references are relative to the schema baseURI.

In our case, the schema id looks like https://cdevents.dev/0.4.0/schema/artifact-deleted-event, the baseURI would be https://cdevents.dev/ and the $ref for the embedded schema should be /0.4.0/schema/links/embeddedlinksarray.json.

I tried with check-jsonschema and the format /0.4.0/schema/links/embeddedlinksarray.json works with it - the tool fetches the schema from the internet:

➜ check-jsonschema examples/artifact_deleted.json --schemafile schemas/artifactdeleted.json
ok -- validation done

If I used the --base-uri option instead, it works as long as $ref does not start with a / and the combination baseUri + $ref points to a valid location on my local disk. For instance, with $ref == schemas/links/embeddedlinksarray.json, this works:

➜ check-jsonschema examples/artifact_deleted.json --schemafile schemas/artifactdeleted.json --base-uri /git/github.com/cdevents/spec/
ok -- validation done

In CI we need to use local files (not fetched from the internet) because the schemas from PRs have not been published yet. In the SDK the schemas can be embedded in the code, so they don't have to be fetched every time. The code that generates the actual SDK code should rely on the local versions from disk too.

In the jsonschema docs it even says that the URIs do not necessarily need to be network addressable, they're only identifiers, and that typically the schemas are embedded in the tool that does the validation.

I lean slightly towards having the internet based resolution working (i.e. using a $ref like /0.4.0/schema/links/embeddedlinksarray.json) and have tools like CI and code generation do some magic (like rewrite the refs) to be able to resolve all the references from local disk.

@xibz @e-backmark-ericsson @rjalander WDYT?

afrittoli commented 4 months ago

Looking at how references are used for the meta schemas:

So one level is stripped from the root schema ID and then the relative path applied

If we followed the same approach for CDEvents:

I think the relative references option would be better because, to use the absolute one, we would include the spec version in the reference, so every new release would require updating all references, even if it wasn't changed.

rjalander commented 4 months ago

As per the jsonschema docs, the $ref is a URI-reference that is resolved against the schema's Base URI. So we can not use ref pointing from a classpath or local system path, it always resolved against root schema ID URL, In this case the schema fails to load without internet,

[main] ERROR com.networknt.schema.JsonSchemaFactory - Failed to load json schema from https://cdevents.dev/0.4.0/schema/links/embeddedlinksarray.json
java.net.UnknownHostException: cdevents.dev
afrittoli commented 4 months ago

@rjalander what I found is that you need to load the schemas from the local disk into your validator, so that it will use its local database of schemas instead of going out to the internet.