json-schema-org / JSON-Schema-Test-Suite

A language agnostic test suite for the JSON Schema specifications
MIT License
625 stars 209 forks source link

draft2020-12 test cases are not compliant with the specification #750

Closed mathematikoi closed 5 months ago

mathematikoi commented 5 months ago

I'm writing a fully-compliant json schema implementation, when running the draft2020-12 test cases it did throw for this reason:

The "$schema" keyword SHOULD be used in the document root schema object, and MAY be used in the root schema objects of embedded schema resources. It MUST NOT appear in non-resource root schema objects. If absent from the document root schema, the resulting behavior is implementation-defined.

tldr; the schemas have the $schema keyword while they are not resource schemas!

would be happy to update the test cases, once this is confirmed to be a non-compliance issue!

gregsdennis commented 5 months ago

The document root for the tests is not a schema, so that passage does not apply to it. This part applies:

MAY be used in the root schema objects of embedded schema resources

The $schema keywords are in the schema resource roots.


the schemas have the $keyword while they are not resource schemas

They are schema resources because they declare $id. This is the defining feature of a schema resource.

gregsdennis commented 5 months ago

You may also want to review https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/586 and related issues.

mathematikoi commented 5 months ago

fixed typo in original comment

tldr; the schemas have the $schema keyword while they are not resource schemas!

thanks for the pr reference.

They are schema resources because they declare $id. This is the defining feature of a schema resource.

they don't declare $id actually, e.g.: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/9fc880bfb6d8ccd093bc82431f17d13681ffae8e/tests/draft2020-12/additionalProperties.json#L6-L11

gregsdennis commented 5 months ago

Point taken. They are intended as part of the test suite to be schema resources. Take that at face value.

I do know that at some point @karenetheridge wanted all of the $ids in the tests to be unique, but maybe that didn't mean they all needed $id.

mathematikoi commented 5 months ago

i see, for now this inspired me to adding turning off specific rules of the spec to our implementation, i will need to think more about the rationale for this constraint!

Screenshot 2024-06-20 at 01 28 41
gregsdennis commented 5 months ago

It's not in violation of the spec, though, because the document is not a schema. The test suite defines the content of the schema property inside the test as a schema. Therefore, the schema is embedded in another document and $schema is permitted at the schema root.

You shouldn't need to change anything in your implementation to run through the test suite.

It sounds to me like you need to allow for schemas embedded in other non-schema documents.

mathematikoi commented 5 months ago

the implementation supports that, im disabling enforcing this part of the spec (in bold)

The "$schema" keyword SHOULD be used in the document root schema object, and MAY be used in the root schema objects of embedded schema resources. It MUST NOT appear in non-resource root schema objects. If absent from the document root schema, the resulting behavior is implementation-defined.

the implementation panics when it sees $schema in a non-resource schema!

gregsdennis commented 5 months ago

I'm saying that those are resource root schemas because the test file format says that the data contained at that location in the file is a schema and Core Section 4.3.5 says:

The root schema is the schema that comprises the entire JSON document in question. The root schema is always a schema resource, where the URI is determined as described in section 9.1.1.

It's a resource because it's the root of the schema. It's just that in this case, the schema is embedded in another document (which means it's not the root of the document, and that's okay).

mathematikoi commented 5 months ago

totally clears it for me! tysm, i appreciate the specification links :D