OAI / OpenAPI-Specification

The OpenAPI Specification Repository
https://openapis.org
Apache License 2.0
28.59k stars 9.06k forks source link

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

Open handrews opened 2 months ago

handrews commented 2 months ago

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.

ralfhandl commented 2 months ago

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

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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

karenetheridge commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 https://github.com/OAI/OpenAPI-Specification/issues/1573#issuecomment-389029180):

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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.