-
-
Notifications
You must be signed in to change notification settings - Fork 348
Define compound schema documents (#936) #977
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
Conversation
Weird, I interpreted that issue as being about something completely different - that is, what happens when there is a $schema
keyword lower down in the document (that is different than the root level $schema
). Bundling seems somewhat orthogonal to that concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Relequestual (and team), great catch on the annotations and $ref
issue.
I think simplifying this down to strictly bundling, and then doing a separate PR to talk about dereferencing, would make this a lot easier.
jsonschema-core.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is easier if you just say that the original reference MUST be left unchanged (but could later be changed through dereferencing, which is defined separately).
jsonschema-core.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be an alternate for compound schema documents, or just an alternate for schema documents (schemas against meta-schemas) in general? You should always be able to tell whether a given schema is compound, so if you know your instance is a schema that is enough.
We don't want to encourage two different meta-schema functions, because if someone validating a non-bundled schema with the "regular" meta-schema validation function, but then that schema got changed to a bundled schema, it would now require a code change to be handled correctly. We never want to require a code change to keep validation working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consensus in the associated issue was that a compound document would be transport only, in that you can't validate it by applying a meta schema to it as an instance.
Check the issue for the reasoning and justification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Relequestual I think @handrews was just concerned that this could be misunderstood to mean that you need a separate process to validate a schema than you do to validate a compound schema. The process for validating schema documents is the process for validating compound schema documents.
I think the suggestion would be to do something like ...
It is RECOMMENDED that an alternate validation process be provided in order to validate
(削除) Compound (削除ここまで)Schema Documents
The "Compound" part is implied because all schemas are potentially compound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Relequestual yes, what @jdesrosiers said.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we are on the same page now. I don't disagree.
But, I'm now not sure about a few things. I'm saying the following having not reviewed the issue again (because this is a quick comment in a few mins spare time).
I thought in the issue we said it wouldn't be possible to identify a compound schema document vs a normal schema document, although that may have been in context of being an instance.
I'm going to HAVE to review the issue discussion again. It's fine =]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought in the issue we said it wouldn't be possible to identify a compound schema document vs a normal schema document, although that may have been in context of being an instance.
Yes, that was in the context of being an instance. It's not possible to identify an instance as a schema (compound or otherwise).
Why do the referenced resources have to have absolute URI identifiers in order to be bundled? In my (admittedly simple) implementation of bundling in a json-schema-using application, which uses $ref
s of the form "$ref": "common_defs.json#/definitions/Foo"
, I just move the definition subschema over into the main document and only rewrite the definition name if it conflicts with another name -- so there are no $id
s in use anywhere.
@karenetheridge What if common_defs.json#/definitions/Foo
has a local reference to another definition in common_defs.json
? The reference would now point to definitions
in the wrong document. Although simple transculsion works in many cases, the only way it always work is if the schema being referenced has an $id
.
@jdesrosiers All the definitions being moved are also scanned for embedded $ref
s and those schemas moved across as well, and so on recursively until all definitions are moved (with $ref
s pointing to them rewritten as needed). It's not very hard at all as long as there are no definition names that collide, and in that case they just get a _1
or somesuch appended to them.
Just because there may be edge cases some of the time that require an $id
(or would make the bundling process easier) is no reason to require an $id
in all cases.
Sorry, what I wrote doesn't make sense. Let me try again. You're right, you can bundle schemas without $id
s, but it means you need to recursively rewrite all the references.
Ideally, whether a schema is bundled or not, you should get the same output. When you rewrite references, your keywordLocation
and absoluteKeywordLocation
values will be different. A bundle should just be a convenient delivery mechanism, it shouldn't create a new schema. I, for one, want to be able to maintain traceability to where the referenced schema originated.
Also, when you need to embed a schema of a different draft, you need to include $schema
, which means you need to have an $id
. There's no way around that one.
I boradly agree with @jdesrosiers regarding tracability. It should be EASY for people to work out which of the schemas they have bundled, back to the original files. The FHIR bundled schema is comprised of a LOT of schemas. A LOT.
Also, when you need to embed a schema of a different draft, you need to include $schema, which means you need to have an $id. There's no way around that one.
I'm not sure I follow. $schema
is required, but $id
isn't specifically required in 2019-09
or indeed in current main branch iirc. How did you arrive at this conclusion?
@Relequestual I didn't mean anything controversial with that statement. I just mean that without $id
you can't embed a schema that is a different version as the referring schema if they are not using the same draft. Quick example ...
{ "$schema": "http://json-schema.org/draft-04/schema#", "type": "object", "properties": { "foo": { "$ref": "./schema/foo#/definitions/aaa" } } }
{ "$id": "http://example.com/schema/foo", "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { "aaa": { "const": "example" } } }
Using the method @karenetheridge described for bundling, you would get this ...
{ "$schema": "http://json-schema.org/draft-04/schema#", "type": "object", "properties": { "foo": { "$ref": "#/definitions/aaa" } }, "definitions": { "aaa": { "const": "example" } } }
This schema will not work properly because const
is not a draft-04 keyword in this context. You would have to include $schema
to not lose what draft the embedded schema is using.
{ "$schema": "http://json-schema.org/draft-04/schema#", "type": "object", "properties": { "foo": { "$ref": "#/definitions/aaa" } }, "definitions": { "aaa": { "$schema": "http://json-schem.org/draft-07/schema#", "const": "example" } } }
But, you can only use $schema
at the root of the schema, so you need an $id
to make #/definitions/aaa
a root schema.
{ "$schema": "http://json-schema.org/draft-04/schema#", "type": "object", "properties": { "foo": { "$ref": "#/definitions/aaa" } }, "definitions": { "aaa": { "$id": "???", "$schema": "http://json-schem.org/draft-07/schema#", "const": "example" } } }
But, the referenced schema has no $id
for us to put there. Therefore, when embedding schemas with differing drafts, the referenced schema must be addressable with an $id
.
Thanks for clarifying @jdesrosiers, crystal clear now.
I'm going to make some changes based on this, other feedback, and discussion from the associated issue!
I've made a few changes based on suggestions.
I still feel like something needs to be said about how $ids of embedded resources can be referenced AS IS.
Ah, it's failing because I need to add an anchor first, then xref target that anchor. I'll fix tomorrow.
34bb747
to
396ec66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good progress.
I think in general we want to be clear about the use case (bundling) vs the mechanism (compound documents) vs the notion of "dereferencing" which is actually something that happens during evaluation, at least based on how we use it elsewhere.
The question of "how do I detect and handle a compound schema document" is somewhat separate from "why would I make a compound schema document." The "why" part is important to motivate this, but once you have a compound document it doesn't matter why it was created.
jsonschema-core.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically you can just write a compound schema document from the start, so I think a more accurate statement would be something like "A Bundled Schema is a Compound Schema Document that is created..." (idk if I'm capitalizing the right things there but that can be sorted out before publication).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm making a change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is supposed to explain how to bundle multiple schemas to form a compound schema document. Therefore I think this is OK, but I'll re-phrase to make it clearer.
jsonschema-core.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically the document's root is only a SHOULD for $id
, although the embedded resources are indeed a MUST. We probably just want to talk about the embedded ones here as the document root is covered under $id
.
jsonschema-core.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk that this is a MAY thing.... is there really anything to enforce here beyond the obvious need for unique keys under $defs
? Is this more of a best practices thing that belongs elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
Define Compound Schema Document and associated concerns
Fix typo Co-authored-by: Karen Etheridge <ether@cpan.org>
Fix typo Co-authored-by: Karen Etheridge <ether@cpan.org>
...d document. The phrase has a specific meaning
...scluding external Schema Resources
..., to said section
...nt, as it is possible to identify embedded schema resources if you know it's a schema as opposed to just an instance.
...enes need to change, and the of an embedded shema resource is still used as the referene string for by-reference applicator keywords.
...pound Schema Document
$id requirements for document root are already covered, and shouldn't be overriden.
Previously 'dereferenced' was used incorrectly. Now "bundling" is clearly defined, and MUST and MUST NOT are split for clairty.
I've forced pushed but something has done gone wrong or I've done something wrong with the naming of the branches and I'm not seeing my changes =/
396ec66
to
4eacfe3
Compare
Accidentally deleted the wrong branch in github UI...
I think this is looking good- Any of my remaining comments are more personal style sort of things so however y'all want to resolve them (or leave as is) should be fine.
Co-authored-by: Karen Etheridge <ether@cpan.org>
...id the need to alter URIs used for referencing
...a Compound Schema Document
@karenetheridge After your further review, I think this is good to merge.
Define Compound Schema Document and associated concerns
Resolves #936