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

Move "readOnly" from Hyper-Schema to Validation #378

Merged
merged 9 commits into from
Sep 10, 2017

Conversation

dlax
Copy link
Member

@dlax dlax commented Aug 28, 2017

This addresses part of #363, which states that some keywords historically in the Hyper-Schema specification would be better in the Validation document. The main argument for moving keywords such as "readOnly" is that the Hyper-Schema document could then only focus on describing the hypermedia linking model.

The second commit fixes the default value of readOnly (wrong type).

Copy link
Contributor

@handrews handrews left a comment

Choose a reason for hiding this comment

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

Thanks! I found some very minor nits but otherwise this looks good.

As we discussed elsewhere, this will need to stay open for at least 14 days for feedback, particularly since we moved quickly from issue to PR.

If it has a value of boolean true, this keyword indicates that the value of the
instance is managed exclusively by the server or the owning authority, and
attempts by a user agent to modify the value of this property are expected to be
ignored or rejected by a server.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change "by a server" to "by that owning authority" or something similar. We want to emphasize the generality of the concept beyond client/server hypermedia.

</t>
<t>
The value of this keyword MUST be a boolean.
The default value is false.
Copy link
Contributor

Choose a reason for hiding this comment

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

After much debate, we settled on this sort of language for default behavior:

Omitting this keyword has the same behavior as a value of false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed both remarks. Also move the type description in its own paragraph similarly with other keywords.

@adamvoss
Copy link
Contributor

For what it is worth, I find this unexpected.

On its own, a JSON document has no inherit concept of "read-only" so there would be nothing for a document validator to do with this information. There would also be limits to how much a generic JSON document editor could utilize this (at what point is the value read-only?).

This assumes either a concept of outside ownership (which IMO should be more generic than the 'sever' language used here) or of a "lifetime" of a document; which I do not see elsewhere in the spec.

From the Draft 06 introduction:

JSON Schema can be used to require that a given JSON document (an instance) satisfies a certain number of criteria. These criteria are asserted by using keywords described in this specification. In addition, a set of keywords is also defined to assist in interactive user interface instance generation.

I'm clear on the first two sentences. However, I don't know if the third sentence should be taken to mean intelligent, schema-aware JSON editors, or UIs that completely hide that JSON document is being edited (ex. web forms). If it is the former, this seems out of scope. If it is the latter, I still find it odd all of that is in scope for a "document validation" specification, but this would definitely be in-scope too.

@handrews
Copy link
Contributor

handrews commented Aug 30, 2017

@adamvoss nothing in that section has anything to do with validation at all. We may move this entire section into its own spec at some point, but "title", "description", "default", and "examples" are all about documentation and/or presenting interfaces to the user. The interfaces here are end-user interfaces of any sort. The level of detail is basic (there is a separate proposal for UI details). However, "should this be presented as an editable control or a fixed value" is very basic.

It would be incorrect for a keyword in this section to affect validation in any way. Many people think "default" interacts with validation but it does not. Some libraries and applications will write defaults in during validation, but that is not part of the specification.

So "readOnly" is definitely in scope for this section, although it's debatable as to whether this section should stay in the validation spec. The applicability of "readOnly" outside of Hyper-Schema was discussed in detail in the related issue. If we split the whole section out, that would be in the following draft, as setting up a new home for it is non-trivial.

I agree with generalizing the server language, and in fact requested that earlier (@dlax fixed some but not all of that language).

@dlax
Copy link
Member Author

dlax commented Aug 30, 2017

I agree with generalizing the server language, and in fact requested that earlier (@dlax fixed some but not all of that language).

@handrews If you're are referring to the "description" of readOnly in meta-schema, indeed I missed it. So I just removed the description altogether in 10358ad. Let me know if there's anything else.

Copy link
Contributor

@handrews handrews left a comment

Choose a reason for hiding this comment

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

I think you got all of the "server-supplied" ones, but reading through again I notice some similarly out-of-place terminology.

Also, this all does focus on the scenario of writing changes back to somewhere. Perhaps it would be good to also mention that this can be used for UI display purposes? Since there is wording around that for title and description.

rejected by that owning authority.
</t>
<t>
For example, this property would be used to mark a server-generated serial
Copy link
Contributor

Choose a reason for hiding this comment

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

Not the same as the other uses of server, but perhaps it would read better here if we said "database-generated" or "algorithmically generated" or something?

<t>
If it has a value of boolean true, this keyword indicates that the value of the
instance is managed exclusively by the owning authority, and attempts by a user
agent to modify the value of this property are expected to be ignored or
Copy link
Contributor

Choose a reason for hiding this comment

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

"user agent" is a hypermedia thing. Probably replace with "application", which is more generic?

@handrews
Copy link
Contributor

@adamvoss another reason for this is that I see it get used in JSON Schemas that are documentation-oriented but that do not make use of hyper-schema. It just seems to have a different usage profile than links; more like that of default.

@Relequestual
Copy link
Member

Relequestual commented Aug 30, 2017

We may move this entire section into its own spec at some point, but "title", "description", "default", and "examples" are all about documentation and/or presenting interfaces to the user. - @handrews

Not only that, also for annototaion purposes; someone inspecting the JSON Schema document.
Anyway, I don't think further justification is required.

I don't think there's a problem with having informational key words, which have a defined format, but allow the application to define how they are used, if at all.

Is there any reason it couldn't be moved to core as opposed to validation?

@Relequestual Relequestual self-requested a review August 30, 2017 09:50
@handrews
Copy link
Contributor

@Relequestual

Is there any reason it couldn't be moved to core as opposed to validation?

If you mean "readOnly" on its own, I don't think it's any more of a core concept than any of the other annotation keywords.

If you mean the annotation keywords as a whole, I'd say file an issue for us to discuss that. I think the answer is "because JSON Schema as a media type doesn't need them to function", but we can debate the point. Or file the issue to just be about finding a better home for the annotation keywords, and we can discuss whether that's core, somewhere else, or whether validation is really the best home for them.

@Relequestual
Copy link
Member

I'm not convinced that it belongs in validation any more than it does in hyperschema =/

@handrews
Copy link
Contributor

handrews commented Sep 1, 2017

@Relequestual let's put it this way: Assuming that "title", "description", "default", and "examples" belong in validation (because they've been there the whole time), why would "readOnly" not go with them?

We can definitely debate where all of these keywords should be, but let's do that separately. One step at a time. "readOnly" documents usage and provides a hint as to how to present things in a UI. That's what the other four do as well. "readOnly" definitely does not need to be in a hypermedia context to make sense, so it fits better in validation with the other four than in hyper-schema.

It definitely does not go in core. Nothing that directly describes data goes in core. "$schema", "$id", "$ref" and "$comment" are all about describing how the media type works (connecting to a meta schema / declaring the vocabulary, identification of and references to specific subschemas, and reserving a field across all vocabularies to guarantee that no vocabulary will process it).

I do not see any possible rationale for "readOnly" in core.

For draft-07, our only options are to put it in core, validation, or hyper-schema. We're not putting together a whole additional vocabulary in a month and a half (unless you're volunteering?)

So if you want to block this, you need to make an argument that "readOnly" really needs to be in one of the other two specs, and I'm not hearing that argument right now.

@handrews handrews added this to the draft-07 (wright-*-02) milestone Sep 3, 2017
@jdesrosiers
Copy link
Member

I agree that readOnly is a little awkward in the validation spec, but no more so than the other metadata keywords. I'm in favor of this change.

@adamvoss
Copy link
Contributor

adamvoss commented Sep 3, 2017

but no more so than the other metadata keywords.

I disagree that all these metadata are the same.

"description" and "examples" are both very useful when reading schemas and writing documents to understand how they should be used and are used by JSON editors in the capacity. "default" is a little odd because it introduces external semantics but also could be useful in writing documents and could be used to provide special behavior in JSON editors (though I do not know of any that do).

"title" seems out of place relative to the others; perhaps an argument could be made for a shorter description than "description" being valid documentation, but the name title clearly suggests UI usage.

While "readOnly" is metadata the data it provides is only relevant in the context of a external systems and has no utility at the "low-level" of directly editing documents.

@handrews
Copy link
Contributor

handrews commented Sep 3, 2017

@adamvoss while you make some good points, none of them convince me that readOnly should stay in Hyper-Schema. A JSON editor would make use of readOnly by preventing users from editing the field. JSON is never really used in isolation. You do stuff with it. Edit it, display it, send it off for processing, store it in a database... all of these things involve using JSON within an external system. Most of them have nothing to do with hypermedia, and are therefore not covered by the Hyper-Schema spec.

For the purpose of this PR and draft-07, the only options are Hyper-Schema or the annotations/meta-data section of Validation. They absolutely do not belong in Core, as application/schema+json and even the vaguely proposed application/schema-instance+json do not require them to function as media types, which is the scope of the Core spec.

If you would like to make a proposal for new homes for all of these, please file your proposal as an issue and we will consider it for draft-08.

@adamvoss
Copy link
Contributor

adamvoss commented Sep 3, 2017

If there is a realistic chance that it does not belong in validation and will end up in a different home, it would be better to keep it in Hyper-Schema. From a consumer's perspective removing a member is a breaking change. Better to only make one breaking change that moves it to a proper home, than two one to remove it from Hyper-Schema and one to remove it from Validation.

@handrews
Copy link
Contributor

handrews commented Sep 3, 2017

@adamvoss if there were any implementation obligations associated with "readOnly", then I would be more receptive to your argument. However, there are not. It changes nothing for Hyper-Schema as Hyper-Schema normatively references Validation already. From its perspective, with a few explicitly noted exceptions, anything defined in Validation is also defined in Hyper-Schema.

Like the other keywords in this section, "readOnly" carries no validation requirements whatsoever. Implementations are free to simply ignore them as long as they are allowed to pass through to the application layer. This is identical to the required behavior for unrecognized keywords, so there is absolutely no difference to validators between using "readOnly" in a validation schema when it is defined in Hyper-Schema (treat it as unrecognized) vs in the metadata section of Validation (it is recognized, but requires no handling). Even for the UI presentation use case, those usage notes are only MAY, which is the weakest level.

Finally, I haven't slightest idea what the chances are of these keywords moving elsewhere. If we had anything resembling an acceptable proposal, we'd be talking about that. It is low on my personal list of priorities for draft-08 as well.

All this does is organize things in a way that everyone else finds more sensible. I think it's pretty clear so far that you hold the minority opinion here (even @Relequestual formally approved the PR), so barring a compelling new argument, this will move forward under the "rough consensus" model. The PR will be up for a minimum of 7 more days should a new argument arise.

@handrews
Copy link
Contributor

handrews commented Sep 3, 2017

And while moving "readOnly" carries no new implementation requirements, it does bring a keyword that is very commonly used without Hyper-Schema into a spec that is more broadly accepted. So it has no cost for implementors, and benefits for schema authors.

@Relequestual
Copy link
Member

I agree it should not be in hyperschem. I'm not sure it belongs in validation long term, but that's fine for this draft and this PR.

@handrews
Copy link
Contributor

handrews commented Sep 8, 2017

@dlax can you rebase or merge locally to resolve conflicts? Or enable maintainers to edit the change and I can do it in the UI here.

We move the description and schema definition of "readOnly" verbatim
from the Hyper-Schema specification (and meta-schema) to the validation
one, in the "Metadata keywords" section (along with "default", in
particular).

This addresses part of json-schema-org#363, which states that some keywords
historically in the Hyper-Schema specification would be better in the
Validation document. The main argument for moving keywords such as
"readOnly" is that the Hyper-Schema document could then only focus on
describing the hypermedia linking model.
Change from the "false" string to the `false` Boolean value.
The readOnly is property is actually the only one having a description;
it got inherited when moved from hyper-schema meta-schema. So drop it to
be consistent with the rest of the schema.

Additionnally, the current description contains ambiguous "server
language" which we try to avoid.
The term "user agent" is appropriate in the scope of hypermedia,
not that much elsewhere; "application" is more generic.
@handrews handrews merged commit 4f9bcb0 into json-schema-org:master Sep 10, 2017
@dlax dlax deleted the readOnly-in-validation branch September 11, 2017 07:14
@dlax dlax mentioned this pull request Mar 23, 2018
@gregsdennis gregsdennis added clarification Items that need to be clarified in the specification and removed Type: Maintenance labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Items that need to be clarified in the specification Priority: Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants