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

Clarify interoperable parsing expectations (3.0.4 port of #3732) #3772

Merged
merged 2 commits into from May 22, 2024

Conversation

handrews
Copy link
Contributor

@handrews handrews commented May 1, 2024

[EDIT: After much discussion, based on today's TDC call this PR is now a direct port of #3732, as the intended model had shifted from 2.0's "copy the JSON/YAML and then apply OAS Semantics" to "the reference target has OAS Semantics, but the implications of this were not entirely worked out."]

This is sort-of a port of #3732, but modified because the scenario involved was mostly assumed to be valid based on the more direct text of OAS 2.0. So instead of making the scenario implementation-defined, I opted to acknowledge that it's the common assumption (there is probably better wording for this, please feel free to suggest - I fear that my current wording is too skeptical or judgemental), but also that the text does not cleary require it in 3.0. And then recommend against doing it because it is not required to be supported in 3.1. See the commit message below for more details of how this is different from the 3.1.1 PR.


As discovered through the OASComply project, certain referencing scenarios are ambiguous, with different authorities holding contradictory interpretations regarding whether and how they are to be supported.

This particular scenario, where the same JSON/YAML object is parsed in different semantic contexts, is well-defined if the requirements of 2.0 are assumed to apply despite no longer appearing in the text. However, the scenario is not well-defined in 3.1, so we should advise against it.

While the 3.1 version of this change made the scenario explicitly implementation-defined, it seems better to acknowledge the assumption here while still noting that it is not clearly required.

This also removes the confusing reference to JSON Schema for "$ref" as the Reference Object explicitly states that it is governed by JSON Reference and not JSON Schema. It also enumerates more of the referencing keywords like we do in 3.1.

@handrews handrews added clarification requests to clarify, but not change, part of the spec re-use: ref/id resolution how $ref, operationId, or anything else is resolved labels May 1, 2024
@handrews handrews added this to the v3.0.4 milestone May 1, 2024
@handrews handrews requested a review from a team May 1, 2024 19:44
@handrews handrews changed the title Clarify interoperable parsing expectations (3.0.4) Clarify interoperable parsing expectations (3.0.4 partial port of #3732) May 1, 2024
ralfhandl
ralfhandl previously approved these changes May 2, 2024
ralfhandl
ralfhandl previously approved these changes May 3, 2024
miqui
miqui previously approved these changes May 3, 2024
@handrews
Copy link
Contributor Author

handrews commented May 3, 2024

I've asked a few people who were around for 2.0 and 3.0 to weigh in on the wording here, so even though this has two approvals, let's not merge it just yet please (but thank you to the approvers so far!)

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.

Looks good. 👍

I had to read this three times before I finally understood it though.

@@ -133,10 +133,22 @@ In order to preserve the ability to round-trip between YAML and JSON formats, YA

### <a name="documentStructure"></a>OpenAPI Description Structure

An OpenAPI Description MAY be made up of a single document or be divided into multiple, connected parts at the discretion of the author. In the latter case, `$ref` fields MUST be used in the specification to reference those parts as follows from the [JSON Schema](https://json-schema.org) definitions. In a multi-document description, the document containing the [OpenAPI Object](#oasObject) is known as the **entry OpenAPI document.**
An OpenAPI Description (OAD) MAY be made up of a single document or be divided into multiple, connected parts at the discretion of the author. In the latter case, [Reference Object](#referenceObject) and [Path Item Object](#pathItemObject) `$ref` keywords, as well as the [Link Object](#linkObject) `operationRef` keyword, are used. In a multi-document description, the document containing the [OpenAPI Object](#oasObject) is known as the **entry OpenAPI document.**

Choose a reason for hiding this comment

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

be divided into multiple, connected parts (called a "multi-document description")
(to unambiguously define a term that is used later)

versions/3.0.4.md Outdated Show resolved Hide resolved
When parsing an OAD, JSON or YAML objects are parsed into specific Objects (such as [Operation Objects](#operationObject), [Response Objects](#responseObject), [Reference Objects](#referenceObject), etc.) based on the parsing context. Depending on how references are arranged, a given JSON or YAML object can be parsed in multiple different contexts:

* As a full OpenAPI Description document (an [OpenAPI Object](#oasObject) taking up an entire document)
* As the Object type implied by its parent Object within the document

Choose a reason for hiding this comment

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

does item 2 allow for a $ref to refer to a JSON schema document (or a schema within a JSON schema, such as to schema a in a schema bundle containing a schema named a and perhaps other schemas that a references and perhaps other unrelated schemas)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSON Schema bundles aren't supported in 3.0, because id/$id is not supported (nor are any other keywords that would require scanning a whole document - see #3758 for how that is handled in 3.1).

These bullet points are not intended to allow anything as much as they are to describe what implementations already do. Otherwise I'd have probably thrown in a MAY.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DavidBiesack let me know if I did not answer your question fully - I'm not sure if you were just talking about formal bundling or something else.

versions/3.0.4.md Show resolved Hide resolved
@handrews handrews marked this pull request as draft May 14, 2024 14:23
@handrews
Copy link
Contributor Author

handrews commented May 14, 2024

I have converted this to a draft just to make sure it does not get merged as (despite having several TSC approvals) there is still discussion going on.

* As the Object type implied by its parent Object within the document
* As a reference target, with the Object type matching the reference source's context

In version 2.0 of this specification, this section specified that the OAD is a "single file", which (together with the treatment of `$ref` as a URL resolved separately each time it is encountered), resulted in the common interpretation that if the same object is parsed multiple times in contexts that require it to be parsed as _different_ Object types, then as long as the reference target is valid for each Object type, then there is no error. An example would be parsing an empty JSON object as a reference target twice: Once as a Path Item Object and once as a Schema Object. Since both Object types allow empty objects, this is syntactically valid for both Objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space

Suggested change
In version 2.0 of this specification, this section specified that the OAD is a "single file", which (together with the treatment of `$ref` as a URL resolved separately each time it is encountered), resulted in the common interpretation that if the same object is parsed multiple times in contexts that require it to be parsed as _different_ Object types, then as long as the reference target is valid for each Object type, then there is no error. An example would be parsing an empty JSON object as a reference target twice: Once as a Path Item Object and once as a Schema Object. Since both Object types allow empty objects, this is syntactically valid for both Objects.
In version 2.0 of this specification, this section specified that the OAD is a "single file", which (together with the treatment of `$ref` as a URL resolved separately each time it is encountered), resulted in the common interpretation that if the same object is parsed multiple times in contexts that require it to be parsed as _different_ Object types, then as long as the reference target is valid for each Object type, then there is no error. An example would be parsing an empty JSON object as a reference target twice: Once as a Path Item Object and once as a Schema Object. Since both Object types allow empty objects, this is syntactically valid for both Objects.

versions/3.0.4.md Outdated Show resolved Hide resolved
@handrews
Copy link
Contributor Author

@RomanHotsiy I'm not sure I know of specific misinterpretations, more like assumptions from those coming from JSON Schema and adding OpenAPI support. They (we) tend to assume referencing works like it's intended to in 3.1, because there's nothing obvious that says otherwise in 3.0. @ralfhandl 's reading of the JSON Reference spec is valid, but that'a level of textual analysis most people don't do.

I'm probably being overly paranoid from some past debates. If no one else shows up with more context in tomorrow morning's call, I'll probably incorporate the feedback, take a shot at better wording, and move ahead.

As discovered through the OASComply project, certain referencing
scenarios are ambiguous, with different authorities holding
contradictory interpretations regarding whether and how they are
to be supported.  As a result, it is impossible to define
compliance, as all of the interpretations can be argued to be
"correct" in some sense.

This change excludes some particularly challenging scenarios from
compliance testing by making their behavior explicitly
implementation-defined.  This has several benefits:

* No current implementation is rendered non-compliant
* No currently usable OAD is rendered invalid
* New implementers need not put effort into handling these scenarios
* User expectations are set to _not_ expect consistent behavior
* Linters can write a rule to match these expectations
* Everyone is guided towards straightforwad best practices

Includes substantially better wording from ralfhandl from
review comments for the 3.1.1 version of this change.

Co-authored-by: Ralf Handl <ralf.handl@sap.com>
@handrews
Copy link
Contributor Author

Based on today's TDC call, the intention regarding $ref and documents had shifted by 3.0:

  • In 2.0, first you copied the target JSON/YAML and then you applied the OAS semantics
  • In 3.0, the target is assumed to have OAS semantics, however the details of how that works were never entirely worked out

Because of this, the 3.1.1 wording was deemed appropriate, with one change (the 3.0 Schema Object does not have its own $ref; Reference Objects are used in its place). This is now a straight port of 3.1.1 plus a commit to remove the Schema Object $ref clause.

@handrews handrews marked this pull request as ready for review May 16, 2024 19:01
Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

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

The new text is slightly too restrictive, can't do that in a patch version.

@handrews
Copy link
Contributor Author

@ralfhandl

The new text is slightly too restrictive, can't do that in a patch version.

Which text do you mean? If it's the text I did not change, then it's already in and that is not a reason to block this PR (although it is a good reason to open a new PR to address the separate problem).

@handrews
Copy link
Contributor Author

@ralfhandl I have filed PR #3820 (replacing 3819 which had errors and a misnamed branch) for the "entry document" concern. Let's please get this PR merged and I will resolve conflicts in that one and we can finish that discussion separately.

@handrews handrews changed the title Clarify interoperable parsing expectations (3.0.4 partial port of #3732) Clarify interoperable parsing expectations (3.0.4 port of #3732) May 20, 2024
Copy link
Contributor

@lornajane lornajane left a comment

Choose a reason for hiding this comment

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

This is a direct port of what we already agreed for 3.1, I'm happy with it but @ralfhandl it would be good to have you circle back to your "request changes" review and see if we're ready to move forward or not.

@miqui
Copy link
Contributor

miqui commented May 21, 2024

I am ok with the language of this PR. Any other concern should be handled in a new PR, etc..etc. @handrews has already done that with #3820

@handrews handrews merged commit c069212 into OAI:v3.0.4-dev May 22, 2024
1 check passed
@handrews handrews deleted the multi-parse-304 branch May 22, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification requests to clarify, but not change, part of the spec re-use: ref/id resolution how $ref, operationId, or anything else is resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants