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

Gregsdennis/locatable iri adjustments #1435

Merged
merged 9 commits into from
Sep 20, 2023

Conversation

gregsdennis
Copy link
Member

@gregsdennis gregsdennis marked this pull request as ready for review September 4, 2023 00:12
@gregsdennis gregsdennis requested a review from a team September 4, 2023 21:30
@gregsdennis gregsdennis added this to the stable-release milestone Sep 4, 2023
jsonschema-core.md Outdated Show resolved Hide resolved
Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

The two main changes here are addressing the same kind of thing, but in different sections far away from each other. I think it makes sense to address all of this in the same place. I think the "Loading Schemas" section is the best place. The "Schema References" section can link to the other section if we think that's necessary.

jsonschema-core.md Outdated Show resolved Hide resolved
Comment on lines 1274 to 1288
Implementations MAY provide functionality to automatically fetch schemas based
on location semantics expressed by the URI, however such functionality MUST be
disabled by default to prefer offline operation. When schemas are downloaded,
for example by a generic user-agent that does not know until runtime which
schemas to download, see {{hypermedia}}.
Copy link
Member

Choose a reason for hiding this comment

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

such functionality MUST be disabled by default to prefer offline operation

This would mean you can't write a hyper-schema client that functions at all in it's default configuration. We can't do that. It also contradicts line 1137 that says this is a "SHOULD". Saying it once on that line should be enough. I also don't think strengthening this "SHOULD" to a "MUST" accurately reflects the opinions expressed in the discussion.

Here's an alternative that I think avoids the useless-hyper-schema-client problem and reflects the opinions expressed in the discussion.

Suggested change
Implementations MAY provide functionality to automatically fetch schemas based
on location semantics expressed by the URI, however such functionality MUST be
disabled by default to prefer offline operation. When schemas are downloaded,
for example by a generic user-agent that does not know until runtime which
schemas to download, see {{hypermedia}}.
Some types of implementations, such as generic user-agents (see {{hypermedia}}), don't know what schemas they will need until runtime. Those implementations MAY automatically fetch schemas based on location semantics expressed by the URI.

We aren't explicitly giving permission to fetch schemas to any and all types of implementations, just ones where that's the only mode in which the implementation could possibly work. This doesn't apply to typical validator implementations that can know schemas ahead of time. The recommended behavior of those types of implementations is given on line 1137.

Copy link
Member

Choose a reason for hiding this comment

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

I think the proposed suggestion above is fine.

Copy link
Member Author

@gregsdennis gregsdennis Sep 13, 2023

Choose a reason for hiding this comment

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

He's right that it conflicts with 1137 (now 1147), though. I'm happy to make the MUST a SHOULD, but I think I prefer the wording that's there.

Copy link
Member

Choose a reason for hiding this comment

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

How does it conflict? This line just says that the config option can exist; it doesn't say that it's allowed to default to on.

I would prefer to say that the config option to download, if it exists at all, MUST default to off. Otherwise we have security considerations that must be documented more visibly.

Copy link
Member Author

Choose a reason for hiding this comment

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

From "Schema References" section

Implementations which can access the network SHOULD default to operating offline.

Then this text said

... such functionality MUST be disabled by default to prefer offline operation.

Copy link
Member

Choose a reason for hiding this comment

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

I think I prefer the wording that's there.

If you don't like the way I phrased it, that's fine, but it really needs to somehow acknowledge that the requirement is nonsense in some applications and that the requirement doesn't apply in those cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think your definition of what constitutes an "implementation" is more narrow than what our charter indicates.

https://github.com/json-schema-org/community/pull/325/files#diff-91d16dd057bd39f6431e90a1d78e9224eafafcedc611eba0878d77e6fda86601R10

JSON Schema aims to enable the confident and reliable use of the JSON data format. It does this primarily by providing specification documents which define a declarative language that allows annotation and validation of JSON documents.

The (proposed) charter clearly states that the primary purposes are "annotation and validation." Hypermedia and other concerns are explicitly listed as secondary.

We intend to enable and support other types of implementations that are not validators such as code generators, form generators, documentation generators, hyper-schema clients, and more that haven't been thought of yet.

But not at the cost of compromising the integrity of the primary concerns. Being able to reach out to the internets or access the file system is not a requirement of annotation or validation. The current text recognizes the need by permitting implementations to have an online mode, but I maintain that the default behavior must be offline.

Hyperschema, codegen, and all of those other uses are uses of JSON Schema. They are not targets. I will not weaken the spec to accommodate secondary and tertiary uses for the spec. Strictly speaking, none of these are JSON Schema implementations. They use JSON Schema. As such, JSON Schema only needs to be defined to serve its primary purposes, and users of JSON Schema (including secondary specifications) need to work within those bounds.

Copy link
Member

Choose a reason for hiding this comment

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

We should all agree what constitutes an "implementation". Let's discuss that topic at the next OCWM.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for the purposes of this, we're on the same page on the definition of an implementation. I think MUST is more appropriate, but I'm okay with this being a SHOULD. Technically the hyper-schema bits can annotate an instance offline, but to do so outside of the context of a client (which definitely needs online access) doesn't really seem useful.

Copy link
Member

Choose a reason for hiding this comment

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

Technically the hyper-schema bits can annotate an instance offline

Fetching a schema can't be completely decoupled from evaluation of the schema in the case of hyper-schema. The schema may include references to other schemas that also need to be fetched. You don't know about those additional schemas until you are evaluating the schema, so it's necessary for the fetch functionality to be integrated with the evaluation process.

jsonschema-core.md Outdated Show resolved Hide resolved
@gregsdennis gregsdennis force-pushed the gregsdennis/locatable-iri-adjustments branch from ae31067 to 82041cd Compare September 13, 2023 20:46
@gregsdennis
Copy link
Member Author

Also, digging this up from the abyss of forgetfulness: #1231

@jdesrosiers
Copy link
Member

restore requirement to MUST

This is not what we discussed and I don't think there's strong support for strengthening this requirement. The discussion was about whether or not to remove the requirement. We didn't discuss strengthening it. My understanding is that Juan and I are in favor of this being entirely an implementation decision based on the unique needs of their use-case. Julian is a proponent of implementers having the freedom to make the choice, but thinks it should be discouraged. Greg and Karen and for strengthening the requirement. I'm not clear where others fall, but so far there's definitely not a clear consensus for strengthening this requirement.

@gregsdennis
Copy link
Member Author

@Relequestual I think we need a tie breaker from our leader.

Do you prefer MUST or SHOULD default to offline operation?

@gregsdennis
Copy link
Member Author

I should add that the current spec (2020-12) does have a note in the context of dereferencing:

In the case of an evolving API described by Hyper-Schema, it is expected that new schemas will be added to the system dynamically, so placing an absolute requirement of pre-loading schema documents is not feasible.

I think this supports Jason's case for a SHOULD.

@gregsdennis
Copy link
Member Author

@jdesrosiers what are your thoughts on replacing that note with

Implementations which have the ability to access externally defined resources MUST do so per the requirements outlined in {{loading}}.

It seems to fit in with this PR.

(then of course, we still need to resolve the SHOULD/MUST question for the loading section)

@Relequestual
Copy link
Member

@Relequestual I think we need a tie breaker from our leader.

Do you prefer MUST or SHOULD default to offline operation?

It depends how we resolve #1440 (Created so we could track as an OCWM agenda item).

I have some thoughts about that, but I'll write them over in that Issue.

@jdesrosiers
Copy link
Member

what are your thoughts on replacing that note with

If {{loading}} says something equivalent to that note, then that would be great. Currently that's not the case. If you like the words used in that note better than the ones I used in my suggestion, then by all means use those words instead, but I do think it needs to be said.

@gregsdennis
Copy link
Member Author

{{loading}} is the section containing second change in this PR.

@jdesrosiers
Copy link
Member

{{loading}} is the section containing second change in this PR.

I know. Like I said, that section doesn't say anything equivalent to the note. The only way I'd be in favor of removing that note and pointing to that section is if we say something equivalent to the note in that section.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

@gregsdennis, can you address my suggestion to not have this defined in two different sections? Do you think there is good reason for this separation?

The two main changes here are addressing the same kind of thing, but in different sections far away from each other. I think it makes sense to address all of this in the same place. I think the "Loading Schemas" section is the best place. The "Schema References" section can link to the other section if we think that's necessary.

Regarding the redudancy, I'm happy leaving it as it's pertinent to both cases, and I don't feel that it's necessary to reference an entire section when all we need from it is a single sentence. I think the duplication is fine in this case.

I don't see that these are two distinct cases. I think we're just saying the same thing in two different places. It doesn't need to be said twice.

Comment on lines 1146 to 1147
if it is a network-addressable URL. Implementations which can access the network
MUST default to operating offline.
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be a "SHOULD" to not contradict the "SHOULD" on 1285.

@gregsdennis
Copy link
Member Author

Do you think there is good reason for this separation?

The separation isn't the intent. It's isolating the specific requirement.

We can really only reference an entire section, but what I specifically need is just "functionality SHOULD be disabled by default to prefer offline operation." I don't think a lnik to another section reads well. I'm happy to get a proposal.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

I don't think a lnik to another section reads well. I'm happy to get a proposal.

I'll look into it another time. It's good enough for now and I don't want to hold this up any longer.

@gregsdennis gregsdennis merged commit 49e58a5 into main Sep 20, 2023
3 checks passed
@gregsdennis gregsdennis deleted the gregsdennis/locatable-iri-adjustments branch September 20, 2023 20:40
@gregsdennis gregsdennis self-assigned this Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants