Skip to content

Clarify various things about canonical URIs #1104

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
wants to merge 0 commits into from

Conversation

handrews
Copy link
Contributor

Fixes issue #937, clarifying a number of other things along the way.
While it touches a fair number of lines, I'm fairly sure that it
doesn't anything about conformance.

[NOTE: This does not address the question of whether "canonical" is what we want - that is too large of a change for a patch release which is what this PR is targeted for. That topic should get its own issue if someone wants to propose something about it. It also does not deal with #1059, although it clarifies some things based on discussions there.]

After spending more time reading various writings on the concept
of the "canonical" URI for a resource, and reviewing our language,
I came to the following conclusions:

  • canonical URIs only make sense at the whole-resource scope
  • A URI with a fragment is neither canonical nor non-canonical
  • It makes more sense to talk about fragments w.r.t. canonical URIs
  • Our language was sufficiently confusing that going this way seems fine.

As part of this, I fixed an outright incorrect statement that
identifier keywords set canonical URIs. Since there is only
one canonical URI and a single schema object could contain
three ($id, $anchor, $dynamicAnchor) or more identifier keywords,
this statement is clearly a bug. These keywords assign URIs,
but only $id assigns a canonical one.

I revamped a lot of wording in descriptions and examples to
hopefully be more precise. I separated the discussion of
the empty fragment in $id from the main paragraph of its
functionality, and clarified that this is talking about a
media-type-specific semantic equivalence, and is not asserting
that RFC 3986 normalization applies to fragments (this has
been a point of confusion).

If someone wants me to split this up and sees a way to do it, I would be happy to do that.

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 have a couple suggestions, but I think this cleans up a lot of the concerns I had about how the term canonical is used in the spec.

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.

Approved. I think this is a big improvement.

@Relequestual Relequestual self-requested a review August 10, 2021 14:23
Copy link
Member

@Relequestual Relequestual left a comment

Choose a reason for hiding this comment

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

One question but otherwise looks good!

@Relequestual
Copy link
Member

Given the current absense of @handrews, I'll look to create a new superseding PR using these commits, but with one additional change as per the above not yet resolved comment thread.

@Relequestual
Copy link
Member

I've re-evaluated my position and thinking on this, and I believe it's now more aligned with what Henry put in his first comment on this PR. I THINK the associated issue could be resolved with a much smaller set of changes, so will proceed to try that.

@jdesrosiers
Copy link
Member

Remember this issue is just intended to be minimal clean up to fix glaring problems. It doesn't have to solve everything.

@Relequestual
Copy link
Member

Remember this issue is just intended to be minimal clean up to fix glaring problems. It doesn't have to solve everything.

Indeed. I'm going to propose a super minimal change (far smaller than this) based on my comment from yesterday.

@jdesrosiers
Copy link
Member

Why was this closed? Even if the goal is to remove the "canonical" language in the next release, I'd still like to have these fixes for the patch release if possible. This fixes the vast majority of the problems with the way "canonical" is used. If we didn't already have this, I'd say don't bother if the goal is to remove "canonical" anyway. But, the work is already done and it's an improvement over what we have, so why not use it?

@Relequestual Relequestual reopened this Feb 24, 2022
@Relequestual
Copy link
Member

OK. Reopened.

I closed it because it seems like there are outstanding questions which we aren't going to get answered, and I feel my alternate PR does enough for now to fix the immediate and biggest problem.

I'll re-evaluate this PR with new eyes.
How would you like to proceed? Make a new PR, cerrypicking those commits, and make additional commits to form the PR?

@jdesrosiers
Copy link
Member

it seems like there are outstanding questions which we aren't going to get answered

What questions are you referring to? We don't have to solve everything for the patch release. If anything is controversial, we don't need to change it now.

How would you like to proceed?

Merge it as it is. As it is now, it addresses problems and doesn't introduce any new problems. Any additional things we feel needs to be addressed for the patch release can be a separate PR.

Comment on lines 435 to 440
<cref>
Note that documents that embed schemas in another format will not
have a root schema resource in this sense. Exactly how such usages
fit with the JSON Schema document and resource concepts will be
clarified in a future draft.
</cref>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the purpose is of this paragraph. What does "embed schemas in another format" mean?

Copy link
Member

Choose a reason for hiding this comment

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

This is referring to something like a JSON Schema in an OpenAPI document.

@Relequestual
Copy link
Member

Relequestual commented Mar 14, 2022

Hey all. I screwed up, and force-pushed to try undo a merge commit (which happened when trying to resolve a conflict using the GitHub UI... mistake!).
I now do not have the permissions to force-push and re-introduce the commits.
As such, I'll do so on a new branch and create a new PR, referencing this one.
I'll then resolve the conflicts and merge, as there were three approvals on this PR.

I've filed a ticket with GH, but it is non-blocking so I guess they will get back to me "sometime".
In fairness, I've seen reasonable response times when I've filed bugs with their ticketing system!

@karenetheridge
Copy link
Member

I now do not have the permissions to force-push and re-introduce the commits.

You have admin privileges on the repository, so you can temporarily give yourself permission to force-push master and then remove the privileges again. (don't tell anyone but it's been known to happen before.) ;)

@handrews handrews deleted the canon branch September 22, 2022 03:04
@gregsdennis gregsdennis added clarification Items that need to be clarified in the specification and removed Type: Maintenance labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Items that need to be clarified in the specification core Priority: Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants