Skip to content

Fragment identifiers with ':' #66

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

Closed
gkellogg opened this issue Sep 17, 2018 · 5 comments · Fixed by w3c/json-ld-api#42
Closed

Fragment identifiers with ':' #66

gkellogg opened this issue Sep 17, 2018 · 5 comments · Fixed by w3c/json-ld-api#42
Labels

Comments

@gkellogg
Copy link
Member

A bug was noted where the following was not expanded properly:

{
  "@context": {
    "@base": "https://ex.org/",
    "u": {"@id": "urn:u:", "@type": "@id"}
  },
  "u": ["#Test", "#Test:2"]
}

The issue is that the expand IRI algorithm splits "#Test:2" on the colon, where it is intended to be part of the fragment identifier, which is legal.

Either the algorithm needs to be made more complex by detecting this, or we should explicitly say that fragment identifiers containing ':' are not supported.

cc /@davidlehn

@azaroth42
Copy link
Contributor

Proposal: Accept the bug, errata 1.0 and fix in 1.1. We should allow : as a valid character, as it is valid according to the URI RFC (afaicr!)

R

@ajs6f
Copy link
Member

ajs6f commented Sep 27, 2018

👍 . I can think of a few examples of :- containing fragments in the wild.

@gkellogg
Copy link
Member Author

This will create a few corner cases that need to be addressed, for example, creating a term starting with a "#" could create issues:

{
  "@context": {
    "@base": "https://ex.org/",
    "u": {"@id": "urn:u:", "@type": "@id"},
    "#Test": "http://ex.com/"
  },
  "u": ["#Test", "#Test:2"]
}

renders

[
  {
    "urn:u:": [
      {
        "@id": "https://ex.org/#Test"
      },
      {
        "@id": "http://ex.com/2"
      }
    ]
  }
]

We should probably restrict terms from beginning with a "#", or at least provide a warning.

Additionally, the expand IRI function will need some provision to consider if it is not a compact IRI (given no defined term), then value may be considered to be a relative IRI to the document base.

There are certainly more corner cases, and enumerating specific tests would be useful to ensure coverage.

What about "a:b", where base includes "#"? Would that now expand to a relative IRI?

@iherman
Copy link
Member

iherman commented Sep 28, 2018

Just for the records...

@azaroth42 said

as it is valid according to the URI RFC (afaicr!)

I looked it up in rfc3986, which says:

fragment    = *( pchar / "/" / "?" )
...
pchar   = unreserved / pct-encoded / sub-delims / ":" / "@"

Ie, : is a valid character in a fragment ID, we should not disallow it.

@iherman
Copy link
Member

iherman commented Oct 1, 2018

This issue was discussed in a meeting.

  • RESOLVED: Accept the bug, submit errata 1.0 and fix in 1.1 by tightening the definition of a URI scheme
View the transcript 4. New Issues
4.1. Fragment identifiers with “:”
Ivan Herman: link: #66
Rob Sanderson: moving on to issues
… This was discovered as a bug over the past day
… We’ve been discussing it, and it’s valid to have : in fragment ids, some we need to fix this in the spec
Gregg Kellogg: I think that issue is that JSON-LD does not have a syntax for compact IRIs vs. URLs like RDFa so you can recognize prefixes vs. what is a URI scheme
… in this case, this is a bug
… since the pound sign is not a scheme, and it’s not a defined term
… if it were, there wouldn’t be a concern.
… the case we’re running across is that #test and #test:2 the first is a relative URI, since it’s interpreted as a ID
… so it’s a IRI or a document-relative IRI. The second case, it could be a compact IRI or an absolute IRI
… by tightening up the language, it must either match a term or the scheme production and a hash test would not match that production
… but there are other corner cases
… where there are other places where the hash sign may occur
… we need to come up with more breaking cases and tests
… what about a:b where the base includes a hash?
… a does match the production of the scheme, and could be an absolute IRI
Rob Sanderson: so long as it’s deterministic, but it’s these cases where it’s legal, but not permitted according to the algorithm where there’s the issue
… for fixing it, have we tried it?
Gregg Kellogg: I haven’t fixed it, but I think it’s straightforward if we tighten up what a URI scheme is
Rob Sanderson: We can fix this in 1.1, but we should also errata 1.0
Gregg Kellogg: I think we should not change the requirements, but highlight the issue
Ivan Herman: has it yet been submitted in the errata list?
Gregg Kellogg: no
Ivan Herman: We should document it
… editorially, when we have the list of changes, we should make a reference to the errata so we can change 1.0 because it’s a bug
Proposed resolution: Accept the bug, submit errata 1.0 and fix in 1.1 by tightening the definition of a URI scheme (Rob Sanderson)
Ivan Herman: this is explicit in our charter
Gregg Kellogg: +1
Adam Soroka: +1
Ivan Herman: +1
Rob Sanderson: +1
David Newbury: +1
Jeff Mixter: +1
David I. Lehn: +1
Simon Steyskal: +1
Resolution #3: Accept the bug, submit errata 1.0 and fix in 1.1 by tightening the definition of a URI scheme
Adam Soroka: Process: When we submit an errata, are we supposed to do something, or is it an action?
Ivan Herman: just describing it

gkellogg added a commit to w3c/json-ld-api that referenced this issue Oct 3, 2018
gkellogg added a commit to w3c/json-ld-api that referenced this issue Oct 4, 2018
kazarena added a commit to piprate/json-gold that referenced this issue Feb 24, 2019
gkellogg added a commit to ruby-rdf/json-ld that referenced this issue Apr 25, 2019
gkellogg added a commit to w3c/json-ld-api that referenced this issue Apr 25, 2019
gkellogg added a commit to w3c/json-ld-api that referenced this issue May 1, 2019
@azaroth42 azaroth42 added the satisfied Requirement Satisfied label Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@gkellogg @ajs6f @iherman @azaroth42 and others