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

Updated use of URL to URI #76

Merged
merged 3 commits into from
Oct 15, 2024
Merged

Updated use of URL to URI #76

merged 3 commits into from
Oct 15, 2024

Conversation

darrelmiller
Copy link
Member

This PR attempts to clean up some of the language that was copied from OAS that we have since realized was ambiguous. It also changes the specification to use URI references instead of URLs for the field extends.

The reason for this change is that we want to give the option for tooling to choose whether to treat the URI reference as an identifier or a locator. With the previous wording a tooling author may interpret the specification as saying that the tooling is required to dereference the extends URI reference to acquire the target document. That wouldn't be good, for security reasons and it could lead to users to assume that they can pass an overlay with the extends keyword to tooling that expect to receive an OpenAPI document, because it appears that the Overlay is self contained. (This is similar to how Kubernetes Kustomize overlays can be used https://kubectl.docs.kubernetes.io/guides/config_management/introduction/)

This aligns Overlay's use of URI references with the OAS's use of URI references.

handrews
handrews previously approved these changes Oct 12, 2024
Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

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

One minor nit but otherwise this looks great!

versions/1.0.0.md Outdated Show resolved Hide resolved
Co-authored-by: Henry Andrews <[email protected]>
@ralfhandl ralfhandl requested review from handrews and a team October 14, 2024 08:43
@ralfhandl
Copy link
Contributor

Committed @handrews' suggestion

Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

The changes here seem fine, though I'm not really sure how they address this concern:

it could lead to users to assume that they can pass an overlay with the extends keyword to tooling that expect to receive an OpenAPI document, because it appears that the Overlay is self contained.

Also I wonder if we need to define the base URI for relative references as we do in OAS.


Unless specified otherwise, all properties that are URLs MAY be relative references as defined by [RFC3986](https://tools.ietf.org/html/rfc3986#section-4.2).
Unless specified otherwise, relative references are resolved using the URL of the referring document.
Unless specified otherwise, all fields that are URI references MAY be relative references as defined by [RFC3986](https://tools.ietf.org/html/rfc3986#section-4.2).
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to say what the Base URI is here? I think we do in the OAS.

Copy link
Member

@handrews handrews Oct 14, 2024

Choose a reason for hiding this comment

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

@mikekistler no, because RFC3986 takes care of it, and the Overlay Specification does not define an internal base URI-setting mechanism under RFC3986 §5.1.1. Most of the weird problems regarding base URIs in the OpenAPI Specification are because someone tried to re-write things already covered by RFC3986 and messed it up (e.g. forgetting about §5.1.2, or creating ambiguity around the term "document'), so let's not do that here. Just referencing RFC3986 is sufficient.

@ralfhandl ralfhandl merged commit fef7638 into main Oct 15, 2024
2 checks passed
@ralfhandl ralfhandl deleted the dm/URLtoURI branch October 15, 2024 06:18
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.

4 participants