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

Did we break 3.2 compatibility by removing the Path Item $ref special case? #3734

Open
handrews opened this issue Apr 21, 2024 · 14 comments
Open
Assignees
Labels
re-use: ref/id resolution how $ref, operationId, or anything else is resolved re-use: traits/merges Selective or modified re-use review
Milestone

Comments

@handrews
Copy link
Member

It grieves me to even bring this up, as no one is a fan of the special-case behavior of $ref with siblings in the Path Item Object. But I feel that it's extremely important to restore trust by making all valid 3.1 OADs also be valid 3.2 OADs. The change in PR #2657 would invalidate certain 3.1 OADs.

We did deprecate the special case in 3.1.1 with PR #2656, but we don't really have a published deprecation policy that would allow us to break compatibility.

I hesitate to suggest any sort of creative workaround here, as any exception to compatibility will just remind everyone of the 3.1 experience.

@handrews handrews added the re-use: ref/id resolution how $ref, operationId, or anything else is resolved label Apr 21, 2024
@handrews handrews added this to the v3.2.0 milestone Apr 21, 2024
@handrews handrews self-assigned this Apr 24, 2024
@ralfhandl
Copy link
Contributor

I don't think that PR #2657 technically invalidates 3.1 OADs: the value of a path still is a JSON object that MAY contain $ref and any other fields next to it, so any 3.1 OAD should pass structure validation with 3.2 rules.

The semantic change is that

  • in 3.1

    In case a Path Item Object field appears both in the defined object and the referenced object, the behavior is undefined.

    Which means that the behavior is undefined even for summary and description 😮

  • in 3.2 an object containing $ref is a Reference Object which

    • explicitly allows summary and description and defines how to interpret them
    • requires to ignore additional properties, and thereby acknowledges that they may exist:

      any properties added SHALL be ignored.

If we worry about implementations that don't ignore these properties and have some specific behavior not defined by the spec, we could relax that sentence to

This object SHOULD NOT be extended with additional properties and any properties added SHOULD be ignored.

Implementations ignoring additional properties in Reference Objects are still compliant, and implementations with special behavior for additional properties in former Path Item Objects with $ref that now must be interpreted as Reference Objects also are.

As are future implementations that ignore both the SHOULD NOT and the SHOULD 🤷🏻‍♂️.

@handrews
Copy link
Member Author

@ralfhandl I definitely see your point here, and it's worth considering that approach. However, I'm concerned about limiting compatibility to whether or not past OADs are syntactically invalidated.

As a comparison, the "clarification" of nullable in 3.0.3 did not syntactically invalidate anything: You could still put nullable in an object without type present, but now we mandated that it would have no effect. Since I know you were impacted by that change, I'm guessing "we didn't syntactically invalidate your descriptions!" probably didn't make the experience any more enjoyable.

I'd probably be more open to relaxing a requirement in 3.3. I just feel like after 3.0.3/3.1.0 both exceeded the expected impact of patch/minor releases, we really ought to be very strict with 3.0.4/3.1.1/3.2.0. But if there's a consensus to relax this in 3.2, I'll go along with it.

@handrews
Copy link
Member Author

I'm still not sure how the "deprecate" PR in 3.1.1 impacts things - I tend to feel that you can't deprecate things unless you've published a deprecation policy explaining that it can happen and what it means.

I've tagged this for TSC review now that we're doing that (as of this morning's TDC call).

@handrews
Copy link
Member Author

@OAI/tsc review request: The things to decide here are:

  • Do we have (or can we reasonably create) any sort of published deprecation policy that makes previously-merged PR Deprecate Path Item Object ref field (#2635) #2656's deprecation notice meaningful and acceptable to implementors?
    • If yes, does it allow changing this functionality in 3.2? Keep in mind that as written in previously-merged PR Allow referencing Path Item only via Reference Object (#2635) #2657, the behavior would change in several ways (ignoring some fields, and changing the semantics of summary and description interactions) that would definitely break strict compatibility
    • If no, do we want to back it out entirely, or is there something we want to do with deprecation here?
  • If the TSC wants to keep the changes in PR Allow referencing Path Item only via Reference Object (#2635) #2657, how would we communicate that incompatibility in a way that does not remind folks of the difficulties caused by 3.1 incompatibilities with 3.0?

@karenetheridge
Copy link
Member

We can know for sure that we broke backcompat in 3.1->3.2 if we can construct a document that is valid in 3.1 but not valid in 3.2. Is that possible?

I believe it is possible -- we just need to have a $ref in a path-item object along with some other path-item properties (and not reuse those properties in the $ref'd object, or we trigger the undefined behaviour clause).

@handrews
Copy link
Member Author

@karenetheridge I'm not sure anything would outright invalidate a 3.1 document in this way- the "undefined behavior" technically isn't invalid, just undefined. And the Reference Object overrides summary and description and ignores everything else. So it wouldn't be invalid then either. But it would change behavior. Non-conflicting, non-summary/description fields would go from being used to being ignored.

@handrews
Copy link
Member Author

It turns out that we sort of have a past record of deprecation: In 3.0.2, we added the following to the Parameter Object's allowEmptyValue field (PR #1632, issue #1573 (comment)):

Use of this property is NOT RECOMMENDED, as it is likely to be removed in a later revision.

So far, it has not been removed (and there does not seem to have been a deprecation policy to go with this, and we don't actually use the word "deprecated" in any way). But I guess it's worth acknowledging the precedent? But also acknowledging that some of our 3.0.y releases were less compatible than they should have been.

@darrelmiller
Copy link
Member

darrelmiller commented Apr 28, 2024

I think it is worth clarifying the deprecation policy. My understanding is that our deprecation of OAS concepts would use the same approach as an API description's use of the term deprecated.

Declares this operation to be deprecated. Consumers SHOULD refrain from usage of the declared operation. Default value is false.

i.e. It's still supported, but we recommend avoiding its use because it will go away in a future version.

My worry about the change to have $ref being a reference object is I think one of the common use cases is to merge in other operations. e.g.

openapi: 3.1.0
info:
  title: My API
  version: 1.0.0
paths:
  /foo:
    get:
      responses:
        '200':
          description: OK
    $ref: '#/components/pathItems/deleteSomething'
components:
  pathItems:
    deleteSomething:
      delete:
        responses:
          '204':
            description: OK

Unless we use Ralf's SHOULD wording, then this is going to be invalid in 3.2.

@handrews
Copy link
Member Author

@darrelmiller

Perhaps we could soften the blow by allowing direct $ref of Operation Objects (and allowing them under #/components/operations)? A deprecation is far more likely to be successful if there is a viable alternative. $ref-ing Operations would require a bit more verbosity because theres no grouping of Operations, but it would be possible to more-or-less do this sort of thing.

Note that this would be separate from operationRef, which does not indicate the logidal-replacement of the reference, but is intended to point to an operation that is in-use (not a Component). Which... would be another weird headache... plus operationId is also intended to go to in-use Operations rather than Components.

Blah. Why do we have like 5 different linkage models with different syntax and semantics? It's really hard to accommodate this deprecation now that I think through it.

@ralfhandl
Copy link
Contributor

My worry about the change to have $ref being a reference object is I think one of the common use cases is to merge in other operations

Yes, this example works nicely in https://editor-next.swagger.io/

@mikekistler
Copy link
Contributor

Wasn't this decided back in the 3.1 days, when it was decided to drop SemVer? The 3.1 spec says:

Occasionally, non-backwards compatible changes may be made in minor versions of the OAS where impact is believed to be low relative to the benefit provided.

I read this to mean that there is no guarantee and should be no expectation that 3.1 specs will be 3.2 compatible.

I'm not saying that I agree with this situation -- just that I think it was already decided and documented.

@handrews
Copy link
Member Author

@mikekistler See #3529 – we emphatically decided that breaking compatibility, while necessary with 3.1, was a big problem for adoption and community confidence, and we don't want to do it again.

@handrews
Copy link
Member Author

handrews commented Sep 8, 2024

I think we really need to back out the change here. I can think of three options that will respect our restored use of SemVer:

  • Leave it the same as in 3.1
  • Add the HTTP method fields to the list of fields that the Reference Object can override; this would preserve the behavior of all previously valid 3.1 descriptions, and also allow more well-defined semantics for conflicts (I don't personally like those semantics better than what we have now, but it would at least eliminate one variation, and I don't think the HTTP methods collide with fields in any other Object)
  • Split the difference: allow a Reference Object (including summary/description override), and allow the other Path Item fields in a Reference Object, but only for Path Item references and treat them as they are treated now (this feels kind-of like the worst of both worlds, but I wanted to be thorough).

@ralfhandl
Copy link
Contributor

I prefer the first option, leave it the same as in 3.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
re-use: ref/id resolution how $ref, operationId, or anything else is resolved re-use: traits/merges Selective or modified re-use review
Projects
None yet
Development

No branches or pull requests

5 participants