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

Detect an attempt to compact an IRI which results in an absolute IRI #110

Merged
merged 6 commits into from
Jun 18, 2019

Conversation

gkellogg
Copy link
Member

@gkellogg gkellogg commented Jun 15, 2019

…where the scheme matches a term in the active context.

Fixes #104.


Preview | Diff

…where the scheme matches a term in the active context.

Fixes #104.
@gkellogg gkellogg requested review from dlongley and pchampin June 15, 2019 18:59
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Contributor

@pchampin pchampin left a comment

Choose a reason for hiding this comment

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

  • We don't need to raise an error when the absolute IRI has an 'authority' (i.e. the "suffix" starts with //) because the IRI expansion algorithm will never consider those as compact IRIs. I proposed changes to exclude these absolute IRIs from the error case.

  • The references to "IRI scheme" are pointing to RFC3986 "URIs" rather than RFC3987 "IRIs". I did the same when adding references to "IRI authority". I am assuming that this is deliberate, since RFC3987 does not describe in detail the structure of IRIs, it relies on RFC3986 for that, but I preferred to check.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@gkellogg
Copy link
Member Author

  • We don't need to raise an error when the absolute IRI has an 'authority' (i.e. the "suffix" starts with //) because the IRI expansion algorithm will never consider those as compact IRIs. I proposed changes to exclude these absolute IRIs from the error case.

I accepted this, but it may be that it's simpler to always just error when an absolute IRI has a scheme patching a prefixable term. The more specific test may imply that it's otherwise okay to do this, which is odd.

  • The references to "IRI scheme" are pointing to RFC3986 "URIs" rather than RFC3987 "IRIs". I did the same when adding references to "IRI authority". I am assuming that this is deliberate, since RFC3987 does not describe in detail the structure of IRIs, it relies on RFC3986 for that, but I preferred to check.

Yes, same reasoning. RFC3987 does not define these parts as it is based on RFC3986, which does, and thus the direct reference to the definition in RFC3986.

@gkellogg gkellogg dismissed pchampin’s stale review June 16, 2019 18:04

Committed the suggested changes.

@iherman
Copy link
Member

iherman commented Jun 17, 2019

This may be a change of far-reaching editorial consequences, so we may want to ignore it, but I thought it is worth raising: I was made to realize, lately, that the WhatWG URL spec encompasses URI-s, URN-s, IRI-s, etc. See, for example, my forensics, and also some additional references. The comment that made me realize my own, faulty misconception comes from this comment.

As I said, it may be too late by now, but maybe it is worth considering changing the terminology and thereby getting rid of the issue of which rfc to refer to...

@pchampin
Copy link
Contributor

@gkellogg In general I'm all in favour of simplicity. But in this case, the additional complexity serves a valid purpose, IMO: limiting the ambiguity btw absolute IRIs and compact IRIs.

@iherman I think this question deserves an issue of its own...

Copy link
Contributor

@pchampin pchampin left a comment

Choose a reason for hiding this comment

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

Sorry, I should have spotted that the first time...
And it adds some complexity to the test :-/

index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
gkellogg and others added 2 commits June 17, 2019 13:50
@gkellogg
Copy link
Member Author

This may be a change of far-reaching editorial consequences, so we may want to ignore it, but I thought it is worth raising: I was made to realize, lately, that the WhatWG URL spec encompasses URI-s, URN-s, IRI-s, etc. See, for example, my forensics, and also some additional references. The comment that made me realize my own, faulty misconception comes from this comment.

As I said, it may be too late by now, but maybe it is worth considering changing the terminology and thereby getting rid of the issue of which rfc to refer to...

We should definitely consider doing this; the references can be updated with some amount of work, but better to get it done right now. Agree that this should be a different issue.

@pchampin
Copy link
Contributor

This is looking good. Just a thought: shouldn't the "6.3.1.Overview" section also mention the change? For example, an additional paragraph saying:

In the case were this algorithm would return the input IRI as is, and that IRI can be mistaken for a compact IRI in the active context, this algorithm will raise an error, because it has no way to return an unambiguous representation of the original IRI.

@gkellogg
Copy link
Member Author

This is looking good. Just a thought: shouldn't the "6.3.1.Overview" section also mention the change? For example, an additional paragraph saying:

In the case were this algorithm would return the input IRI as is, and that IRI can be mistaken for a compact IRI in the active context, this algorithm will raise an error, because it has no way to return an unambiguous representation of the original IRI.

Yes, that's reasonable, I'll add that paragraph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compaction changes the meaning of absolute IRIs in some cases
4 participants