-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
Overriding annotation/usage keywords at point of use #98
Comments
I miss the point completely - why do you need a special construct to add meta keywords? Why not just: {
"title": "foo",
"$ref": "bar"
} as title etc. are ignored anyway? Or if you want you can: {
"title": "foo",
"allOf": [ { "$ref": "bar" } ]
} $merge/$patch and banUnknownProperties idea solve additionalProperties issue. $merge/$patch also solve schema extension issue for recursive schemas (meta-schema is just an example, recursive schemas are very common). I agree with the objections against banUnknownProperties. The argument that $merge/$patch is powerful and allows to change the schema is an argument to use it, rather than to avoid. Leaving aside comparisons to other ideas, what problem that can't be solved already does this proposal solve? |
@epoberezkin did you read the linked email thread? It explains exactly how this proposal came about and how it relates to the other issues mentioned, and why it is preferable to $merge/$patch. I'm not going to rewrite it all here, although I can copy in key points if people think it will help. Issue #15 is the one for $merge/$patch, and it has numerous points related to this. @tadhgpearson agreed with the need for this use case in this comment. @seagreen and @awwright cover a lot of the concerns in comments after that.
This issue is not about recursive schemas, please keep the use cases separate. If we happen to adopt $merge/$patch for other reasons, this issue can be closed as moot, but unless that happens (and I've seen no indication that it will), this issue is only about the use case I identified here. In another issue you suggest that I try out $merge because you think it will grow on me. That's hilarious- as you can see in #15, my previous project made use of $merge well over 200 times in the published schemas alone (and more in internal schemas). This proposal is based on the experience that despite a great deal of |
@handrews I've read the thread. It does not answer the question why annotations need re-writing and why simply adding them to $ref as in my examples is not sufficient. I think the idea of re-writing anything follows from treating $ref as equivalent to inclusion. I think it is a big mistake to treat it in this way, because, as I wrote before:
Consider this simplified example: {
"id": "schema1",
"allOf": [ { "$ref": "schema2#/definitions/foo" } ]
} {
"id": "schema2",
"definitions": {
"foo": { "$ref": "#/definitions/bar" },
"bar": { "type": "string" }
}
} That is a very common use case. If you treat "$ref" as inclusion then This example is equivalent to the test case in JSON-Schema-Test-Suite - if $ref were equivalent to inclusion this test would have fail. There are use cases when allowing inclusion would be beneficial - it would support polymorphism (same subschemas doing different validation depending on where they are included). If we think it can be beneficial then we may consider an additional keyword that does inclusion (or maybe "$merge" without "with" would do just nicely). |
Because the only field allowed in a $ref object is $ref. It references, it doesn't merge. |
As I wrote above, you can: {
"title": "foo",
"allOf": [ { "$ref": "bar" } ]
} Why is it not sufficient? |
If you want to dig into the behavior of $ref, please do so in issue #66 rather than clogging up this issue with topics that are at best tangential to the proposal at hand. If there is a specific issue where you see this as a problem with $use, please explain it in terms of $use rather than complaining about general issues with $ref. Otherwise, let's sort out $ref in #66 and then come back to this. |
@epoberezkin : What part of "$ref cannot have other keys present because it doesn't merge, it just references" is unclear? I'm not trying to be snarky here, I really can't figure out how else to say it. If $ref behaved as you seem to want, it would more or less be $merge and we wouldn't be arguing over that proposal elsewhere (and pleas leave the $merge arguments in issue #15 ). And in any event, that's not how it behaves, and if you'd like to change its behavior please do that in #66 . |
@handrews I was just trying to understand the need for $use as it is defined here and the only explanation I can imagine is that you equate $ref to inclusion. Sure, let's continue in #66. Do you think I need to copy this comment there? On the substance of the proposal, it simply seems unnecessary. You already can add annotations to $refs by wrapping them into allOf, which is more concise than what is proposed. Can you explain why the existing solution is not adequate and why an additional vocabulary is needed to do the same you can do without it? |
I have a whole section about what the existing workarounds are, with "allOf". It discusses how "allOf"'s behavior with annotations is insufficient for documentation generation, and how "default" simply doesn't work in a meaningful way. "$ref" is irrelevant- you can't add other keys to it, and using $ref inside of "allOf" vs using a literal schema inside of "allOf" are equivalent for these purposes. What part of my discussion of the "allOf" alternative is unclear or unconvincing? |
I will review it again. Documentation generation is outside of spec. Whether the doc generator can understand $ref wrapped in allOf or not is the problem of the generator - extending the standard created for validation hardly seems an adequate solution to help doc generators.
Again, please review the cases in the comment above (#98 (comment)) - $ref and using literal schema are not equivalent. |
Um.. no. From the first paragraph of the JSON Schema Core specification:
While we confirmed in #89 that documenting how multiple schemas interact in an API is out-of-scope, documenting individual schemas is explicitly in-scope. Note that I don't mean fancy full I18N/L10N end-user documentation, I just mean reading the schema and presenting it at a developer level. I'm still not going to discuss your concerns about $ref and inclusion here as it distracts from the main proposal. If you comment on #66, I will address it there. |
The way it is written, JSON schema itself is intended as a documentation for JSON data. Deriving some other documentation format from the schema (I assume that's what you mean) is something else. |
Right. This is the declared purpose of title and description:
An automated system cannot disambiguate conflicting values for these keywords when trying to figure out what to present in such a user interface. That makes schemas using these keywords difficult to re-use, which is the problem being addressed here. Additionally, completely aside from documentation, the other metadata keyword, "default", is unusable in "allOf" without disambiguation. Please try to address these problems
|
Ok, I will spend more time to understand this issue, I can be missing something here :). Will come back with the questions. In the meanwhile, I am writing the comment re $refs in #66. |
@epoberezkin - while the allOf + $ref implementation makes theoretical sense to me, it may be difficult to implement simply because the behavior is not explicity defined in current versions of JSON schema, and causes today's tools to break. I think it would be useful to survey the ease of use of both syntaxes with other developers. I find the $use ( source + with ) syntax to be much clearer - but obviously, that's just my little opinion, and I think it would be good to see what some other users think ;) |
@epoberezkin now that #66 is resolved, any further thoughts? This is a high priority for me to resolve one way or the other for the next draft. The problem it addresses is a major pain point in a current project. @robbie-mac this addresses your concern about $merge/$patch violating the intent of closing a schema to extension that you raised in #15 . @seagreen I believe you were also concerned in #15 about $merge/$patch being overly powerful- this proposal essentially restricts $merge down to a form that cannot affect validation. @awwright any thoughts here? This was part of the email thread about different re-use use cases that you seemed to like when I first posted it. Although I'm not sure if that was in response to this particular proposal or one of the other use cases in the thread :-) And I might as well tag @Relequestual and @jdesrosiers while I'm at it to see if this resonates with anyone else who has been active here recently. As noted, I'd really like to get this either into the next draft or definitively rejected. |
I've re-read the thread. I understand the issue that some processing tools may face. I think that the problem of tooling of any kind should NOT be addressed in the standard that has validation as its primary purpose, as otherwise we will be facing feature creep from users of different tools (e.g. UI generation - why not add something to support them here too? How UI generation is worse than documentation generation? I am not suggesting it, I am using it as an illustration that documentation generation is equally narrow and out-of-scope concern). I think either of the following is a better option:
Also bear in mind that $use is more complex to define than $merge - it requires maintaining the list of keywords that are taken into account (maintaining = writing, extending, discussing, etc.), while $merge is quite simple - it can be defined once and forever. |
@handrews: Thanks for mentioning me here. This would definitely be an improvement over $merge/$patch from my perspective. That said, I'm actually still against it, but I'm not sure you should take my thoughts into account. To explain... My use case is super weird in that I need a way to write JSON schemas that do structural-only validation and absolutely nothing else. I won't even be using So as you can I'd prefer that the core, structural part of JSON Schema stayed extremely focused on just validation. I think there are reasons this is a good idea -- consider in how many situations things like That said, I appreciate that you have pragmatic use cases for both |
I'd missed this line. Obviously you need to be taken care of one way or another. Would a "JSON Document Schema" similar to the JSON UI Schema idea be too crazy of a thought? It could include things like "title" and "description", and also a powerful way to override them. |
I would not object to such a thing. Basically splitting the "meta-data" keywords out of validation (where they don't really belong anyway) and expanding that. I need to be able to easily access the |
Oh man, I didn't actually expect anyone to agree with this. This would be so cool!
I can't imagine this would be too difficult (obviously handling things like "title" is no trouble at all). I haven't been following all the open issues though, so maybe more complicated documentation-related keywords are being added. If that's the case though it would make having a JSON Schema Document Extension even more important. |
Another appealing thing about making the documentation stuff an extension is that we could push all of the I18N/L10N stuff there. I think. I kind of like the idea of: JSON Schema core (bare minimum necessary to use any vocabulary) Right now validation has just a bit of documentation stuff but there are arguments over what, exactly, it's there for. This probably calls for an issue of its own- @seagreen do you want to file one for this extension? I am supportive but have too many other things going on to champion another big proposal. |
Done! #136 |
I believe it's called |
Ok, my vote: 🎉 |
Spending some time reviewing #15 , is there any reason we couldn't JUST include $merge but not $pathch? $merge seems easier to understand with minimal effort. I don't know with other languages, but with perl, this concept is great and I utilise it often. I see general conversation is that people are worried it may be too complicated and result in people creating things they don't mean to, or are invalid. There's always going to be a gap between new users and power users, and right now, power users want a way to do this. While most seem to be mostly interested in just metadata (which $use would cover), I've still seen more than a handful of people ask for something along the lines of $merge, and not just in github issues. It's always possible to create nonsensical schemas. We can help by providing examples and common pitfalls on the website if we think it would help. @epoberezkin Can you speak to difficulty to implement? @handrews I'm downvoting this because I feel if added to the spec, we will get people asking why $use is limited to non validation key words. |
The null thing, I think. But if it's a choice between having just |
@erayd I thought the null issue was resolved? #15 (comment) (aka, it's not an issue) @epoberezkin Looks like you were on the same train of thought as me at one point: #15 (comment) |
@Relequestual If we do Looking at my comments in #364, my main reason to push for this was |
@nitmws , you indicated that you would advocate for this. Various people are pushing hard for |
@Relequestual I think there are still edge cases with |
@erayd definitely for |
Also, |
I think that some more granularity than the current $ref system is required to prevent massive copy-pasting of fields just to change descriptions. Whether this is directly overriding, $use, or whatever, something is necessary - 🎉 |
@tadhgpearson all of the discussions (elsewhere) about how These keywords have no impact on validation. So there is no implementation/interoperability concern there. Really, it is application-defined how these metadata/annotation keywords are used. This is even still true after moving So what are the interoperability requirements for these keywords? They are application-specific. We may want to guarantee behavior in an API Documentation context, but that would be better handled in the specification for that vocabulary. That could specify, for instance, that descriptions are concatenated with two newlines between each for display. Or that a description in a parent schema replaces all in child schemas for the purpose of displaying the parent schema. UI generation would have similar but perhaps not identical requirements. And many applications just won't care. Increasingly, I don't see a need to define combinatoric behavior for the metadata/annotation keywords within the validation or core specification. Application (or application domain)-specific vocabularies would be better places to nail down common behavior. So for those of us who do not want to allow merging/patching on arbitrary keywords, there's not actually anything that needs to be done. Except maybe opening an issue on the API Doc vocabulary at https://github.com/json-schema-org/json-schema-vocabularies/issues/new |
Re #98 (comment) : hm, I would have to dig into all the issues regarding |
@nitmws current discussions with @Relequestual and others are leaning towards punting all of this out of draft-07 (to get it wrapped up in the next month) and making it the primary focus of draft-08 (which would be published as soon as we have a resolution, possibly much sooner than 6 months after draft-07). When we've decided one way or another I'll update all the issues. Some will get closed as non-viable, while others will probably get consolidated into a few issues summarizing these epic discussions. No one wants to read 100 comments, many of which go in circles anyway :-P |
@handrews - I think that it's very important to specify this. As a user, it's a major advantage to be able to use JSON schema with many tools for many purposes. I can use it for a documentation generator on my website, a code generator and publish it in 3rd party directories which might have their own UIs. If the behavior of, for example This isn't an arbitrary problem. |
@tadhgpearson I think I wasn't clear. I think specifying makes sense, I'm just not convinced it should be specified in the validation spec. There is a larger discussion going on about whether the whole "Meta-data keywords" section belongs in the validation spec at all. That won't change in draft-07 but it is being kicked around, in part for exactly this reason: the community wants these things specified more, but such specification isn't really about data validation. By moving it to a separate (but still "official", whatever that means) specification, we can address the real concerns around these keywords without limiting validation behavior. All of your examples are about generating docs, code or UI. That's what the new specification(s) would focus on. None of your examples have anything to do with validation, unless I'm missing something. What we've come to realize is that validation and data definition are not the same thing, and a lot of the difficulty various tools struggle with has to do with finding a way to support concepts that are really only useful for validation. Or dealing with people complaining that they are not supported, even when the use cases for data definition are unclear. |
@tadhgpearson The same comment applies to other metadata keywords, although I think |
@erayd Correct, absolutely not related to |
@erayd yup. We toyed with the multiple keywords approach for For data definition, I think the specifications end up being a bit looser. We need to avoid things like accidentally using examples as runtime defaults ( |
@handrews |
@erayd it will probably go over to json-schema-org/json-schema-vocabularies. Which is currently set up for three vocabularies for (docs, UI, code) generation. I'm starting to think that there is actually a basic data definition spec to work out, which may work for some and form the basis for others (obviously annotating whether an enum should be a drop-down or a set of radio buttons is UI-specific). That would also let it evolve independent of the existing schedule. I expect it would be driven by a slightly different set of participants, just as we have people who look primarily at hyper-schema, and people who only care about validation. |
@erayd @tadhgpearson we actually already have an initial proposal from someone, which you can see here: there's some discussion already, and we'll pick it up more after draft-07 (but feel free to comment) |
@handrews I like the sound of that. |
At the end of the vote-a-rama, I said that I would consolidate these issues to focus the discussion for draft-08. I've filed #515 which outlines the two possible competing approaches. It's pretty abstract, but once we choose an approach, deciding which exact keyword and behavior we want should be less controversial. Therefore I'm closing this in favor of #515. |
Use Case and Motivation
This is one of several proposals to come out of the analysis of the "ban additional properties mode" vs
$merge
/$patch
debate and the various use cases that fed into that.One common use case identified was specifying information about a type's usage at the point of use rather than as part of the initial type definition. In some cases, it is reasonable to provide some generic usage information in the type definition which serves as default usage documentation, but needs to be overridden in some uses with more specific information.
Additionally,
default
cannot meaningfully appear in multiple branches of logical keywords such asallOf
, as there is no sensible way to choose which value is correct for the specific usage. Conflicting validations in anallOf
simply fail theallOf
, but conflictingdefault
values are just unusable, independent of validation.Prior experience
This particular use case has been a major pain point on both large-scale JSON Schema projects with which I have been involved.
One implemented
$merge
, which was very effective but (as several people have noted, and as I now agree) it is too powerful as it (and$patch
) can literally convert any schema into an arbitrarily different schema, or even into a non-schema JSON value. In this project,$merge
was used to override default values fordefault
,title
,description
, andreadOnly
, or disambiguate among several possible values for those keywords.The other project ended up redefining many types that should be sharing a definition. In some cases, nearly every schema has had to redefine a particular sub-schema just to change the title and description.
Preserving independent validation
In order to preserve the self-contained nature of validation, usage overriding only applies to the keywords in the "Metadata keywords" section of the validation specification (
title
,description
, anddefault
) plus usage-oriented hyper-schema keywords (readOnly
andpathStart
).Breaking self-contained validation has been the main objection to other proposals in this area, either through overly powerful keywords or changing validation modes in ways which are not reflected in the schema itself. This is the only proposal to date that solves this problem with no impact on validation whatsoever.
Existing workarounds
It is possible to use an
allOf
with a reference to the type schema and a schema that includes only the point-of-use annotations. This is tolerable when reading the schema, but is not good for documentation generation. Each schema within anallOf
should be self-contained, which separates the documentation from the thing it's documenting. Additionally, if default annotation fields have been filled out, a documentation tool that attempts to extract and combine annotation data from allallOf
branches will just produce conflicting documentation.Finally,
allOf
is nonsensical when combining multiple schemas usingdefault
,readOnly
, orpathStart
, as there is no reasonable way to choose which value to use.The proposal:
$use
The keyword
$use
would be an annotation/hyperschema keyword indicating that a type (via a$ref
) is being used with additional annotation and/or hypermedia for this specific use.$use
is an object with two required properties and one optional property; the format for the required properties is borrowed from the$merge
and$patch
proposals:source
MUST be a schema, and will nearly always be a$ref
as there is no need for$use
if you have the literal schema defined in line. Required.with
should be applied to the resolved source as anapplication/merge-patch+json
instance, which MUST NOT affect any JSON Schema-defined keyword that is not explicitly allowed by the proposal. Required.$use
to their extensions or not as they choose. Required.JSON Merge Patch considerations
Declaring the
with
object to follow JSON Merge Patch semantics allows implementations to use existing libraries, and frees us up from needing to debate yet another schema combination format.However, a notable limitation of JSON Merge Patch is that you cannot overwrite something to
null
, as specifying anull
value instead deletes the key entirely. A workaround for this that is consistent with existing JSON Schema behavior is to definenull
somewhere (perhaps right in the$use
object since it does not forbid additional properties) and then$ref
it. We would need to declare that$use
is processed before$ref
. Which is fine because$ref
often needs lazy evaluation due to circular references anyway.Elsewhere, there's been an assertion that
$ref
must always resolve to a schema. This would require breaking that assumption. Since the assumption was not present in Draft 04 (in which JSON Reference was an entirely separate specification), I think this isn't too unreasonable. A compromise could be that it can reference either a schema or null.If $ref-ing null doesn't gain acceptance, another option would be an additional keyword in the
$use
object listing JSON pointers to properties that should be set to null."$" in name
I chose
$use
rather thanuse
on the grounds that keywords that manipulate the schema itself should stand out (I will be filing a more comprehensive issue on the use of$
later as it comes up in yet another not-yet-filed proposal, plus there is an open issue to consider$id
in place ofid
).Example
This shows that the type can be concisely defined in a shared location, with a clear usage object applied to it.
Meta-schema considerations
This could be added to the meta-schema by moving most of the keyword definitions into the meta-schema's
definitions
section grouped by type of keyword, and definingwith
as a schema minus the forbidden keywords.The text was updated successfully, but these errors were encountered: