Skip to content
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

Add "$recurse" for extending recursive schemas #589

Closed
wants to merge 7 commits into from

Conversation

handrews
Copy link
Contributor

@handrews handrews commented May 24, 2018

Addresses #558.

The 3rd and 2nd to last two commits rework $ref and $recurse as adjacent subsections in the "Schema Referencing" section, as most of their behavior is identical (particularly once #585 is complete).

The third-to-last commit just moves things around with no content changes other than obvious section titles, while the second-to-last commit updates the text, including minor updates to $ref's subsection.

handrews added 2 commits May 23, 2018 23:18
This allows deleting most of the hyper-schema meta-schema.
<t>
The presence of this keyword with a boolean true value indicates that,
during processing, it MUST be treated as a reference to the schema document
where processing was initiated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it MUST be treated as a reference to the schema document where processing was initiated.

vs

Note that even if processing began at a subschema within a document, the "$recurse" target MUST be the root of the document.

The first seems like its saying if we ref into a document at some lower level, it should recurse to THAT level, but the 2nd says it must be the root? Confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the key here is the wording "schema document" which is defined in section 4.3 in both draft-07 and the current HEAD commit. A schema document is a document, not a JSON object. So saying that it MUST be treated as a reference to the schema document where processing was initiated means exactly what the clarification says.

But I'm open to suggestion on how to word it better, preferably without entirely re-stating 4.3. Maybe just an <xref /> to it? I suspect the complete list of people who understand the distinction without having to look it consists of is @awwright and me.

}

{
"$schema": "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

er... yeah, thanks!

@handrews handrews force-pushed the recurse branch 2 times, most recently from 8e5412b to f302ed1 Compare May 25, 2018 05:23
handrews added 3 commits May 24, 2018 22:24
This commit *only* includes formatting changes:

* New overall section title
* New section title for the now sub-section on "$ref"
* "$ref" subsection indented and wrapped to 100 characters
* "$recurse" section moved to right after "$ref" sub-section
* "$recurse subsection indented and wrapped to 100 characters
* Other reference subsections wrapped to 100 characters as needed

The next commit will update the actual text.
Add an intro paragraph about reference keywords and their
nature as indirect in-place applicators.

"$ref" is no longer really targeted at recursive schemas,
so tweak its wording slightly.

Do a few further improvements to "$recurse" wording, particularly
around root schemas and schema documents.

Correct an apparent typo about a schema "against a schema", this
is almost certainly supposed to be "against an instance", as
the rest of the paragraph talks in terms of instance validation.
So remove the advice about that part.
@awwright
Copy link
Member

awwright commented Jun 3, 2018

How would I refer to extended schemas from schemas of my own?

For example, suppose I have a media type that tries to represent HTML forms. It's an array of objects, each describing a form field. One of the properties is a schema that can describe a schema, and I want to

{
  type: "array",
  items: {
    type: "object",
    properties: {
      "name": { type: "string" },
      "schema": { $ref: "http://json-schema.org/draft-nnn/hyper-schema" }
    }
  }
}

where http://json-schema.org/draft-nnn/hyper-schema is some future schema for Hyper-schema that uses this feature.

What happens when a JSON Schema validator hits $recurse: true?

@handrews
Copy link
Contributor Author

handrews commented Jun 4, 2018

@awwright that is an excellent question, and definitely a problem for this proposal as it stands.

Embedding the meta-schema is a different use case than "extending" a schema (I hate to use the word "extending" as it makes people think of OOP, but JSON Schema is a constraint system which is very different- not sure what else to call it, though). But there is no way in this proposal to distinguish the two, so your example would break. And in a non-obvious way.

The simplest fix would be to add something alongside the $ref that controls whether recursion takes the containing schema into account. This draws from your other idea in #558 of changing how a specific $ref is resolved. I'm worried about the added complexity, but $recurse as proposed here can't work so we need some sort of fix.

The thing I want to preserve about $recurse is that it's target is dynamic, while $ref's target is static. I'm open to any proposal that keeps that distinction.

@handrews
Copy link
Contributor Author

handrews commented Jun 5, 2018

I'm going to move this discussion back to the issue and make some revised proposals there.

@handrews
Copy link
Contributor Author

Closing per #558 (comment) due to the problems noted by @awwright. I hope to revisit this, but as noted in the issue, it will be a while and we can get by without it if we must.

@handrews handrews closed this Jun 10, 2018
@handrews handrews deleted the recurse branch August 23, 2019 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants