further specify the format of iri-references throughout #1085

karenetheridge · 2021-03-30T18:25:35Z

$id cannot have a fragment at all, so separate it from the common definiiion
$ref, $dynamicRef, $recursiveRef must be a iri-reference to a schema
location: when the fragment is non-empty, it must refer to an $anchor or a
json-pointer
absoluteKeywordLocation in result outputs must use canonical
IRIs/iri-references: either there is no fragment, or it is non-empty and
encodes a json pointer

Also fixed the output schema which erroneously still identifies absoluteSchemaLocation as a uri, not an iri.

jdesrosiers

I'm not sure this change is necessary, but I'm ok with it. I assume we aren't talking about a change to the already released meta-schema, but rather that this would be part of the next draft?

jdesrosiers · 2021-03-31T18:38:54Z

schema.json

@@ -51,7 +51,7 @@
        },
        "$recursiveRef": {
            "$comment": "\"$recursiveRef\" has been replaced by \"$dynamicRef\".",
-            "$ref": "meta/core#/$defs/uriReferenceString",
+            "$ref": "meta/core#/$defs/uriReferenceToSchemaString",


The only defined value for $recursiveRef is #, so this could be "const": "#". On the other hand, if there is no $recursiveAnchor, then it's supposed to behave like a $ref which allows any URI, so ¯\_(ツ)_/¯.

The only defined value for $recursiveRef is #

Not really. The 201909 spec leaves the behaviour for other uri-references somewhat open -- it doesn't actually restrict (i.e. with a MUST) to the # value. I've gotten other $recursiveRef values to work just fine in my implementation, with no extra code.

Good point. It's not forbidden, it's just undefined.

karenetheridge · 2021-03-31T18:53:28Z

I'm not sure this change is necessary, but I'm ok with it. I assume we aren't talking about a change to the already released meta-schema, but rather that this would be part of the next draft?

There's no reason why this can't go into the current draft's metaschema, because it is not adding an additional restriction that wasn't already there, but rather making the metaschema more conformant to existing language.

That is -- it lets implementations catch problems earlier by applying a schema against the metaschema, rather than having to catch problems with custom code during evaluation.

jdesrosiers · 2021-03-31T19:04:10Z

There's no reason why this can't go into the current draft's metaschema

We generally don't consider meta-schemas living documents. Once they are released, we don't change them any more than we would change the spec. We have made exceptions for fixing bugs in meta-schemas, but this isn't fixing anything, it's just making it more strict. I don't think we have a rule written down anywhere about how to deal with this kind of thing (we probably should), but historical precedent seems to indicate that this should go in the next draft.

karenetheridge · 2021-03-31T19:43:57Z

We generally don't consider meta-schemas living documents.

We have in the past, however, fixed inconsistencies between the written spec and the metaschema, where the written spec prevails (because it very definitely is versioned and cannot be revised before the next release).

jdesrosiers · 2021-03-31T23:28:36Z

We have in the past, however, fixed inconsistencies between the written spec and the metaschema, where the written spec prevails

Right, which is essentially bug fixes. This is not a bug fix. I don't feel strongly enough about this to try to block it, but I do feel that it's important for more voices to express opinions on whether we should be making this kind of change for meta-schemas before this gets approved. Even better would be if we decide on guidelines for when we want to allow updating meta-schemas and document that decision.

handrews · 2021-05-12T16:10:00Z

@karenetheridge @jdesrosiers it would probably be a good idea to start a discussion in the community repo on the meta-schema process. It can complement my spec release process discussion (json-schema-org/community#7) but please start a new discussion! The meta-schema process should be separate. I do not have an opinion on it (or rather, we tried my opinion, but no one liked it- which I say with humor, it's just time to try someone else's ideas).

In the meantime let's put this on hold while we sort out the process concerns.

jdesrosiers · 2022-07-08T15:33:32Z

The draft-next branch has been merged and is now closed. The merge target for this PR has been changed to main. Here are the recommended steps to get your branch reabsed properly.

Make sure your remote for the json-schema-org/json-schema-spec repo is up-to-date. (Example: git fetch upstream).
Rebase your commits onto main. (Example: git rebase --onto upstream/main abcd123~1 (replace abcd123 with the commit hash of the first commit in your PR)).
Force push the rebased branch to your fork. (Example: git push --force origin my-branch).

handrews · 2022-08-14T16:47:38Z

@karenetheridge I think this would need to be reworked now that JSON Schema uses IRIs rather than URIs, and I'm not quite sure how the regexes would work in that case. Along with the branch changes, perhaps this is best closed and a new PR submitted?

karenetheridge · 2022-09-05T19:14:49Z

I have rebased against the updates to main.

- $id cannot have a fragment at all, so separate it from the common definiiion - $ref, $dynamicRef, $recursiveRef must be a iri-reference to a schema location: when the fragment is non-empty, it must refer to an $anchor or a json-pointer - absoluteKeywordLocation in result outputs must use canonical IRIs/iri-references: either there is no fragment, or it is non-empty and encodes a json pointer Also fixed the output schema which erroneously still identifies absoluteSchemaLocation as a uri, not an iri.

gregsdennis · 2022-09-06T06:07:31Z

Good reminder that I need to update the output schema to the new structure. 👍

This should go through first, then I'll do that update.

notEthan · 2022-09-06T07:53:28Z

meta/core.json

-            "format": "iri-reference"
+            "format": "iri-reference",
+            "$comment": "any fragment must be empty, or match anchor or json-pointer syntax",
+            "pattern": "^[^#]*(#([A-Za-z_][-A-Za-z0-9.:_]*|/([^~]|~[01])*))?$"


Suggested change

"pattern": "^[^#]*(#([A-Za-z_][-A-Za-z0-9.:_]*|/([^~]|~[01])*))?$"

"pattern": "^[^#]*(#([A-Za-z_][-A-Za-z0-9.:_]*|(/([^~/]|~[01])*)*))?$"

The second half of the fragment match, for JSON Pointer, won't match the empty pointer ($ref: "#", $ref: "foo#"). The suggested change follows the JSON Pointer ABNF.

This is still incomplete though. Fragments may contain %xx encoded characters. In a pointer their use may be necessary, but in either a pointer or anchor they are expected to be treated like the characters they represent, even when those characters don't actually need to be %-encoded.

Suggested change

"pattern": "^[^#]*(#([A-Za-z_][-A-Za-z0-9.:_]*|/([^~]|~[01])*))?$"

"pattern": "^[^#]*(#(([A-Za-z_]|%[A-Fa-f0-9]{2})([-A-Za-z0-9.:_]|%[A-Fa-f0-9]{2})*|((/|%2[fF])([^~/%]|~[01]|%[A-Fa-f0-9]{2})*)*))?$"

Not too pretty, but I think it allows anything that's valid. It won't reject some invalid anchors or pointers if they're %-encoded - it would be possible to express the same character ranges as allowed combinations of %-encoded digits, but I'm not masochistic enough to do it.

I'm a little uncertain describing a uri fragment with a regex isn't more trouble than it's worth.

notEthan · 2022-09-06T08:04:18Z

output/schema.json

-          "format": "uri"
+          "format": "iri",
+          "$comment": "any fragment must be non-empty, and use json-pointer syntax",
+          "pattern": "^[^#]*(#(/([^~]|~[01])*)*)?$"


The comment says must be non-empty, but this does allow an empty fragment.

Suggested change

"pattern": "^[^#]*(#(/([^~]|~[01])*)*)?$"

"pattern": "^[^#]*(#(/([^~/]|~[01])*)+)?$"

The change to + requires a non-empty pointer; disallowing the / from the reference-token character range is better consistent with its spec's ABNF (and iriReferenceToSchemaString above), though it doesn't ultimately change what the regex matches.

And of course %-encoding needs fixing here too.

actually I think we need to allow empty fragments, so it's the comment that is wrong here.

@karenetheridge Agreed, you need to be able to represent the root with a JSON Pointer fragment, which would be an empty fragment.

gregsdennis · 2022-09-17T21:05:58Z

@karenetheridge please address @notEthan's regex concerns, but I think this is good.

jdesrosiers reviewed Mar 31, 2021

View reviewed changes

handrews added the Status: On Hold label May 12, 2021

handrews changed the base branch from master to draft-next May 19, 2021 19:07

handrews added this to the draft-next milestone May 19, 2021

jdesrosiers changed the base branch from draft-next to main July 8, 2022 15:33

karenetheridge force-pushed the ether/schema-uri-reference branch from 22c5cd1 to 1e16053 Compare September 5, 2022 19:13

karenetheridge changed the title ~~further specify the format of uri-references throughout~~ further specify the format of iri-references throughout Sep 5, 2022

karenetheridge force-pushed the ether/schema-uri-reference branch from 1e16053 to b17f02f Compare September 5, 2022 22:39

karenetheridge force-pushed the ether/schema-uri-reference branch from b17f02f to a7eebf9 Compare September 5, 2022 22:50

notEthan reviewed Sep 6, 2022

View reviewed changes

gregsdennis mentioned this pull request Sep 8, 2022

Update output schema to describe new structure #1285

Merged

gregsdennis mentioned this pull request Sep 21, 2022

add pr-dependencies-check workflow #1308

Merged

karenetheridge marked this pull request as draft October 30, 2022 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

further specify the format of iri-references throughout #1085

further specify the format of iri-references throughout #1085

karenetheridge commented Mar 30, 2021 •

edited

jdesrosiers left a comment

jdesrosiers Mar 31, 2021

karenetheridge Mar 31, 2021

jdesrosiers Mar 31, 2021

unavailable10 Mar 28, 2024

karenetheridge commented Mar 31, 2021 •

edited

jdesrosiers commented Mar 31, 2021

karenetheridge commented Mar 31, 2021

jdesrosiers commented Mar 31, 2021

handrews commented May 12, 2021

jdesrosiers commented Jul 8, 2022

handrews commented Aug 14, 2022

karenetheridge commented Sep 5, 2022

gregsdennis commented Sep 6, 2022

notEthan Sep 6, 2022

notEthan Sep 6, 2022

karenetheridge Oct 3, 2022

handrews Oct 3, 2022

gregsdennis commented Sep 17, 2022

	"pattern": "^[^#](#([A-Za-z_][-A-Za-z0-9.:_]\|/([^~]\|~[01])*))?$"
	"pattern": "^[^#](#([A-Za-z_][-A-Za-z0-9.:_]\|(/([^~/]\|~[01]))))?$"

	"pattern": "^[^#](#([A-Za-z_][-A-Za-z0-9.:_]\|/([^~]\|~[01])*))?$"
	"pattern": "^[^#](#(([A-Za-z_]\|%[A-Fa-f0-9]{2})([-A-Za-z0-9.:_]\|%[A-Fa-f0-9]{2})\|((/\|%2[fF])([^~/%]\|~[01]\|%[A-Fa-f0-9]{2}))))?$"

	"pattern": "^[^#](#(/([^~]\|~[01]))*)?$"
	"pattern": "^[^#](#(/([^~/]\|~[01]))+)?$"

further specify the format of iri-references throughout #1085

Are you sure you want to change the base?

further specify the format of iri-references throughout #1085

Conversation

karenetheridge commented Mar 30, 2021 • edited

jdesrosiers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenetheridge commented Mar 31, 2021 • edited

jdesrosiers commented Mar 31, 2021

karenetheridge commented Mar 31, 2021

jdesrosiers commented Mar 31, 2021

handrews commented May 12, 2021

jdesrosiers commented Jul 8, 2022

handrews commented Aug 14, 2022

karenetheridge commented Sep 5, 2022

gregsdennis commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsdennis commented Sep 17, 2022

karenetheridge commented Mar 30, 2021 •

edited

karenetheridge commented Mar 31, 2021 •

edited