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

fix: update format for servers.url and links.operationRef in v3.1 schema #3235

Closed

Conversation

MikeRalphson
Copy link
Member

uri-reference is not the right format for these properties, as they can contain unescaped { and } characters. As @jdesrosiers points out, they are not uri-template either, as they do not support RFC6570 URI templates in full.

uri-reference is not the right format for these properties, as they can
contain unescaped { and } characters. As @jdesrosiers points out, they
are not uri-template either, as they do not support RFC6570 URI
templates in full.
@handrews
Copy link
Member

handrews commented Apr 7, 2023

can't operationRef have embedded runtime expressions in addition to the regular templating?

@MikeRalphson
Copy link
Member Author

@handrews yes, but does that mean it requires a new format value?

@handrews
Copy link
Member

handrews commented Apr 7, 2023

@MikeRalphson it depends on what you want tooling to be able to do with the format. If all you're going for is "the old format was wrong and we should have a not-wrong format", then pretty much anything you want is fine. If you want schema tools to be able to recognize exactly what sort of templating is happening and validate that (either within JSON Schema if the format-assertion vocabulary is in use, or as a post-schema-validation step if the format-annotation vocabulary is in use, which it is the default), then your format value(s) need to be specific enough to convey that information.

It's hard for me to judge whether this format value is what we'd want in either or both situations without knowing the expectations for implementation. This, btw, is part of the problem of format, as we saw with decimal — it seems to encourage underspecification and over-expectation. In other words, format consistently over-promises to schema authors while allowing/encouraging implementation to under-deliver.

But really, if all you want this to do is indicate "uri-reference with balanced { and } allowed" then sure this is fine. I did not notice yesterday (COVID-brain) but now I see that in PR #3234 you are referencing the Paths Object for "usage" (rather than syntax?) and not the Operation Reference. Does the operation reference allow syntax that is not allowed in the Paths Object? I'm definitely a little unclear on how runtime expressions fit into the templating for the operation reference - I need to study that when I'm feeling better.

@handrews
Copy link
Member

See also #3256 (Formally define templating behavior)

davishmcclurg added a commit to davishmcclurg/OpenAPI-Specification that referenced this pull request Nov 20, 2023
`Server.url` and `Link.operationRef` both allow variable substitution
with {brackets}, which means they're not always valid URI references.

For example, the [current specification][0] shows
`https://{username}.gigantic-server.com:{port}/{basePath}` as a Server
Object `url`, but it's not a valid URI reference because the host
includes curly brackets.

[`operationRef`][1] similarly includes
`https://na2.gigantic-server.com/#/paths/~12.0~1repositories~1{username}/get`
as an example that isn't valid using the `uri-reference` format.

I looked into the other uses of `uri-reference` and they seemed ok.

Related:
- OAI#2586
- OAI#3235
- OAI#3256
- davishmcclurg/json_schemer#158

[0]: https://spec.openapis.org/oas/v3.1.0#server-object-example
[1]: https://spec.openapis.org/oas/v3.1.0#operationref-examples
@handrews
Copy link
Member

While I agree that we need to do something like this, I'm going to close this particular PR so that:

We can open a new PR for this once the number and name of formats is resolved.

@handrews handrews closed this Jan 26, 2024
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.

None yet

3 participants