search

redskap / swagger-brake

Swagger contract checker for breaking API changes
Apache License 2.0
58 stars 16 forks source link

Shouldn't removing a request body type be a breaking change? #14

Closed dalewking closed 5 years ago

dalewking commented 5 years ago

Say oldAPI contains a path with:

  requestBody:
    content:
      application/xml:
        schema:
          $ref: '#/components/schemas/schema1'
      application/json:
        schema:
          $ref: '#/components/schemas/schema2'

And the new API removes one:

  requestBody:
    content:
     application/json:
        schema:
          $ref: '#/components/schemas/schema2'

That should be a breaking change since the path no longer accepts xml

dalewking commented 5 years ago

Same thing for responses

galovics commented 5 years ago

@dalewking fixed. Could you please give it a try? Thx!

dalewking commented 5 years ago

Someone might complain about the fact that if old has "application/json" on a route and the new one says "*/*" that is a breaking change even though it is more permissive.

dalewking commented 5 years ago

I am running into this with a DELETE route. We have a delete route that accepts a body (containing ID of the user requesting it and a reason for the deletion). So the oldApi has contentType application/json.

It is a bit controversial whether DELETE allows a request body. Some tools think it is like GET and does not allow a body. The spec is somewhat ambiguous. Certain tools do not allow it. Unfortunately, spring-fox which is what is generating our new API from the spring controller annotations for comparison is one of them. Even if you specify a consumes type it strips it off and it ends up going to */*.

Not sure if I am asking for a change, just thinking out loud.

galovics commented 5 years ago

I mean it's a fair point, although I understand the concern with your specific case. Anyway, I've implemented the */* fix.