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 resolving implicit connections (3.1.1) #3823

Closed
wants to merge 10 commits into from

Conversation

handrews
Copy link
Member

@handrews handrews commented May 19, 2024

NOTE: See PR #3856 for an alternative approach that may be simpler.


Fixes:

This clarifies how to handle resolving implicit (non-URI-based) connections in multi-document OpenAPI Descriptions.

While the behavior is implementation-defined overall, two approaches are RECOMMENDED for tool developers and OAD authors: one that is best suited for sharing components among OADs, and one that is compatible with pre-3.1 parsing approaches and is convenient for breaking a monolithic document up purely for author convenience.

Note that the term "complete OpenAPI document" has been defined in another change (PR #3820) pending approval on the 3.0.4 branch. Also, some relevant Discriminator Object guidance around mapping with relative URI references is being added in #3822, also pending for 3.0.4.

Tthis will get backported to 3.0.4, but without the full-document part; the backport will be easy once we've agreed on the 3.1 approach.

Reviewer Questions

  • operationId could resolve either from all Operation Objects in all documents in the OAD, or only from those reachable via the entry document's paths or webhooks fields
    • I settled on using every Operation Objects because it fits with whole-document parsing
    • For the case where only the entry document has an OpenAPI Object (and therefore Components Object), which is where the "via paths or webhooks" approach might make sense, the only thing this changes is if a Path Item Object in Components has an Operation Object with an operationId targetd by a Link Object, but that Path Item Obejct is not part of paths; it seems unlikely that the intent would be to exclude that Path Item Object and its Operation Objects as they are in the entry document
  • I originally had wording suggesting implementors add opt-in support for a proposed future solution to a major limitation around Security Requirement Objects
    • I decided it was too complicated to do that and just replaced it with an acknowledgement of the limitation and a note that we expect to address it
    • I can take out the note if that seems odd, but I do think it is important to plug this gap ASAP, and let users know that we are aware of it even if we do not say that we plan to address it
    • If you want to see the "from future"-esque idea, you can see it here

This clarifies how to handle resolving implicit (non-URI-based)
connections in multi-document OpenAPI Descriptions.  While the
behavior is implementation-defined overall, two approaches are
RECOMMENDED for tool developers and OAD authors: one that is
best suited for sharing components among OADs, and one that is
compatible with pre-3.1 parsing approaches and is convenient
for breaking a monolithic document up purely for author convenience.

Note that the term "complete OpenAPI document" has been defined
in another change pending approval on the 3.0.4 branch.
@handrews handrews added discriminator clarification requests to clarify, but not change, part of the spec re-use: globals/defaults Default or global components that can be overridden in some way re-use: ref/id resolution how $ref, operationId, or anything else is resolved security: config The mechanics of severs and structure of security-related objects labels May 19, 2024
@handrews handrews added this to the v3.1.1 milestone May 19, 2024
@handrews handrews requested a review from a team May 19, 2024 19:20
Some of what this said about the Discriminator Object is better
handled in that Object's section, which is being revised in
another PR.
Some servers/discriminator text that should have been removed
in the previous commit was accidentally left in.  Several
non-essential recommendations have been removed.
If we want to do this sort of thing it is probably best discussed
separately.
@handrews handrews marked this pull request as ready for review May 22, 2024 03:27
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
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.

Need more explanation

versions/3.1.1.md Show resolved Hide resolved
Also fix an incorrect section anchor.
@handrews
Copy link
Member Author

@ralfhandl I've updated the text in several places (it's a separate commit so you can look at the individual diff) - please let me know if that makes things more clear. I appreciate your attention to detail here, because if the text confuses anyone on the TSC it will definitely confuse other readers who have much less context!

handrews and others added 2 commits May 22, 2024 13:13
Co-authored-by: Ralf Handl <ralf.handl@sap.com>
ralfhandl
ralfhandl previously approved these changes May 23, 2024
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.

Good clarification!

Every time I try to reference this section in another PR I type
"implicit" instead of "indirect" so let's just go with that.

Let's also treat `operationId` uniformly, because it fits better
with how we parse documents, and the only thing this changes
is in the only-the-entry-document-has-an-OpenAPI-object case,
where it incorporates Path Item Objects in the entry document
that are otherwise un-referenced.  It seems likely that one would
only include Path Item Objects in the entry document's components
if one expected them to be used.
@handrews
Copy link
Member Author

@ralfhandl I made some other minor updates, again as a separate commit.

versions/3.1.1.md Outdated Show resolved Hide resolved
ralfhandl
ralfhandl previously approved these changes May 23, 2024
Co-authored-by: Ralf Handl <ralf.handl@sap.com>
ralfhandl
ralfhandl previously approved these changes May 24, 2024
These were left out but have the same problems as Security Requirements.
@handrews
Copy link
Member Author

I've converted this back to a draft because I realized I forgot tags, added them in the most recent commit, and then rethought the whole thing to have a single approach rather than two different ways to organize things. Here is the alternative PR:

@ralfhandl
Copy link
Contributor

I prefer the alternative proposal in #3856.

@ralfhandl ralfhandl self-requested a review May 28, 2024 09:57
@miqui
Copy link
Contributor

miqui commented May 29, 2024

@handrews , @ralfhandl The language in #3856 is less confusing AFAIK (As a spec author (and trying to take a pragmatic approach)).

lornajane added a commit that referenced this pull request Jun 13, 2024
Alternative: Clarify resolving implicit connections (3.1.1- alternative to #3823)
@handrews
Copy link
Member Author

Closing as we accepted #3856 instead 🎉

@handrews handrews closed this Jun 13, 2024
@handrews handrews deleted the implicit-311 branch June 13, 2024 17:12
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: globals/defaults Default or global components that can be overridden in some way re-use: ref/id resolution how $ref, operationId, or anything else is resolved security: config The mechanics of severs and structure of security-related objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants