APIDevTools / swagger-cli

Swagger 2.0 and OpenAPI 3.0 command-line tool
https://apitools.dev/swagger-cli
MIT License
515 stars 69 forks source link

Parameter allows $ref to satisfy type #44

Open scop opened 4 years ago

scop commented 4 years ago

swagger-cli allows $ref in parameters, and does not throw error on missing type if the $ref contains one. The spec seems to leave some room for interpretation, but it does not seem to allow $ref in this context, and on the other hand requires type there.

FWIW https://github.com/zalando/intellij-swagger takes it the other (seemingly more correct IMHO) approach: it requires type and disallows $ref.

I lean towards thinking the approach of swagger-cli is not correct, because in other places in the spec where $ref is ok, it is also explicitly documented.

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#parameter-object

I think this example should raise two errors for the parameter:

swagger: '2.0'
info:
  version: 1.0.0
  title: A very simple API

paths:
  /foo:
    get:
      parameters:
        - in: query
          name: bar
          required: false
          $ref: "#/definitions/mytype"
      responses:
        "200":
          description: "OK"

definitions:
  mytype:
    type: string
    maxLength: 1
JamesMessinger commented 4 years ago

Swagger 2.0 and OpenAPI 3.0 don't allow $ref to be used on the same object as other properties. So this is invalid:

in: query
name: bar
required: false
$ref: "#/definitions/mytype"

If you choose to use $ref alongside other properties, then you opting-in to nonstandard behavior that swgger-cli provides due to popular demand. Since it's nonstandard behavior, it does not need to comply with the Swagger spec or any other tooling implementation. But you can opt-out by only using $ref as allowed by the spec.

It's also worth noting that due to the popularity of this nonstandard feature, it is being added to the OpenAPI 3.1 spec as an official standard.

scop commented 4 years ago

In validate mode, I would like to validate in the strictest possible sense according to the relevant spec -- the point of validation being ensuring validity and thus likely broadest possible interoperability. IMHO that's how it should work by default, and opt-in confirmed by passing a flag to swagger-cli validate signifying the desire to opt in to nonstandard behavior. Failing that, an option to explicitly opt out of it would be the next best thing.

JamesMessinger commented 4 years ago

Good point. I was thinking in terms of the dereference method, not the validate method. I agree that for validate (and maybe dereference too) it makes sense to default to spec-compliant mode.