Yelp / swagger-spec-compatibility

Python library to check Swagger Spec backward compatibility
https://swagger-spec-compatibility.readthedocs.org
Apache License 2.0
20 stars 8 forks source link

Possibly missing check for nested required field. #10

Closed jhereth closed 4 years ago

jhereth commented 5 years ago

Changing a schema such that the object defined inside an (optional) list now requires a certain attribute did not trigger a compatibility warning. Although ok in this specific case this is unexpected behavior in general.

macisamuele commented 5 years ago

@qudade could you please define a small example (without internal details) that is able to trigger the issue?

macisamuele commented 4 years ago

To provide more context the highlighted case is similar to Old specs

swagger: '2.0'
info: {title: Test, version: '1.0'}
paths:
  /endpoint:
    post:
      parameters:
      - description: ''
        in: body
        name: body
        required: true
        schema:
          properties:
            param:
              type: string
              format: datetime
      responses: {default: {description: ''}}

New Specs

swagger: '2.0'
info: {title: Test, version: '1.0'}
paths:
  /endpoint:
    post:
      parameters:
      - description: ''
        in: body
        name: body
        required: true
        schema:
          properties:
            param:
              type: string
              format: datetime
            required: [param]
      responses: {default: {description: ''}}

In this case the tool is not providing information about the change that looks like a backward incompatible change as param is required in the new version of the specs.

My assumption here, and I'm eventually happy to discuss and re-evaluate it, is that the body schema does not really define a type, so the checker does not check for properties as technically it might also not be an object.

Note that adding type: object in #/paths//endpoint/post/parameters/0/schema/ would trigger the backward incompatible change

[REQ-E001] Added Required Property in Request contract: #/paths//endpoint/post/parameters/0/schema (documentation: https://swagger-spec-compatibility.readthedocs.io/en/latest/rules/REQ-E001.html)

My suggestion in this case would be to ensure that type: object is defined in the specs as multiple Swagger validators can make different calls (as this case is not fully described and spec-ed out on the JsonSchema level).

As for know I would close this as "do nothing" but in the future we might decide to re-evaluate this decision. @qudade thanks a lot for reporting ;)