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.

Sign up for GitHub

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

Jump to bottom

GH-135: Follow URI scheme rules #136

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

GH-135: Follow URI scheme rules #136

wants to merge 3 commits into from
+16 −4

Conversation

afs
Copy link
Contributor

@afs afs commented Jan 17, 2025 •
edited by pr-preview bot
Loading

This closes #135.

This PR is close to the changes in #131 and #132 and will probably need adjusting after they are merged.

Preview | Diff

@afs afs requested review from gkellogg and pchampin January 17, 2025 10:40
pchampin
pchampin approved these changes Jan 17, 2025
View reviewed changes
spec/index.html Outdated
Comment on lines 589 to 590
<p>IRIs in the RDF abstract syntax MUST be <a data-cite="RFC3986#section-5">resolved</a>
per [[RFC3986]],
and MAY contain a fragment identifier.</p>
per [[RFC3986]], and MAY contain a fragment identifier. IRIs SHOULD
Copy link
Contributor

Choose a reason for hiding this comment

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

As suggested in my review,
I would suggest to rephrase this as follows
(sorry, I can not use the suggestion mode, because the first line is not part of the diff)

IRIs in the RDF abstract syntax MAY NOT be relative references, and MUST be resolved per [RFC3986]. They MAY contain a fragment identifier.

I like the addition about following the rules defined by the scheme.

Copy link
Contributor Author

@afs afs Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Added. MAY NOT is not in RFC 2119; "MAY" means optional. I think you meant a "MUST NOT" strength condition.

gkellogg
gkellogg approved these changes Jan 17, 2025
View reviewed changes
Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

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

I’m a bit concerned about saying that special rules without listing other important schemes, as this will lead to interoperability issues.

@afs afs requested review from pchampin and gkellogg January 17, 2025 17:52
@afs
Copy link
Contributor Author

afs commented Jan 17, 2025

I’m a bit concerned about saying that special rules without listing other important schemes, as this will lead to interoperability issues.

Which schemes did you have in mind? I picked HTTP (important) and DID (W3C) for the note.

URNs are complicated (there are rules per namespace).

file:///, even with RFC8089, are not global.

Some schemes don't exist! uuid:, oid: (properly, use the urn:uuid:, urn:oid: forms).

@gkellogg
Copy link
Member

I’m a bit concerned about saying that special rules without listing other important schemes, as this will lead to interoperability issues.

Which schemes did you have in mind? I picked HTTP (important) and DID (W3C) for the note.

those certainly make sense given the context, but we make a broad statement that could include obscure schemes, or those yet to be released. White-listing specific schemes (http, https, and did, roof example) would enhance interoperability.

@afs
Copy link
Contributor Author

afs commented Jan 17, 2025

White-listing specific schemes

Is the note text enough?
Or maybe move it up the list of points in the note?
Or change

IRIs SHOULD follow rules defined by the IRI scheme, such as those for HTTP/HTTPS [RFC7230].

URI Schemes: Examples of scheme-specific rules include the scheme rules for HTTP and the DID syntax.

I have changed it to "HTTP/HTTPS"

An advice paragraph somewhere to use specific schemes might be nice (but outside this PR).

Aside: ReSpec has used not used the more usual RFC reference -- https://datatracker.ietf.org/doc/html/rfc7230.

@pchampin
Copy link
Contributor

I think @gkellogg 's concern could be addressed in the note, by rephrasing it:

URI Schemes: Implementations may ignore URI schemes that they do not recognized, but are encouraged to enforce the scheme-specific rules of the most common ones, such as scheme rules for HTTP/HTTPS and the DID syntax.

@gkellogg
Copy link
Member

I think @pchampin’s suggested wording is okay. The SHOULD makes testing a bit challenging, though. Typically, we do not test SHOULD/MAY behavior.

@afs
Copy link
Contributor Author

afs commented Jan 18, 2025

Note updated.

If they don't recognize a scheme, it isn't an option to ignore - they must!

For the testing, my ideal is having rdf-tests as a record of community practice with an emphasis on common practice to warn about.

"SHOULD" is there because (1) this is new, not in RDF 1.1 (2) the "rules" aren't always cut-and-dried.

@afs
Copy link
Contributor Author

afs commented Jan 18, 2025

I will squash the PR to one commit before merge; for now, the PR is keeping each change as a commit.

@afs afs requested a review from pchampin January 18, 2025 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hartig hartig hartig approved these changes

@gkellogg gkellogg gkellogg approved these changes

@pchampin pchampin Awaiting requested review from pchampin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add text noting that IRIs schemes have scheme-specific rules
4 participants
@afs @gkellogg @pchampin @hartig