0.21.0 Adding required property marked non-breaking

[Sherman Thompson]

As title reads. In 0.21.0, if you add a new required property it is marked as non-breaking. Should be breaking.

Additionally, if you make a required property of an object definition optional it is marked as a breaking change. Should be non-breaking.

Source:

{
  "swagger": "2.0",
  "info": {
    "version": "1.0.0",
    "title": "Swagger Petstore",
    "description": "A sample API that uses a petstore as an example to demonstrate features in the swagger-2.0 specification",
    "termsOfService": "http://swagger.io/terms/",
    "contact": {
      "name": "Swagger API Team"
    },
    "license": {
      "name": "MIT"
    }
  },
  "host": "petstore.swagger.io",
  "basePath": "/api",
  "schemes": [
    "http"
  ],
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/pets": {
      "get": {
        "description": "Returns all pets from the system that the user has access to",
        "produces": [
          "application/json"
        ],
        "responses": {
          "200": {
            "description": "A list of pets.",
            "schema": {
              "$ref": "#/definitions/Pet"
            }
          }
        }
      }
    }
  },
  "definitions": {
    "Pet": {
      "type": "object",
      "required": [
        "id",
        "name"
      ],
      "properties": {
        "id": {
          "type": "integer",
          "format": "int64"
        },
        "name": {
          "type": "string"
        },
        "tag": {
          "type": "string"
        },
        "animal": {
          "type": "string"
        },
        "foodType": {
          "type": "string"
        }
      }
    }
  }
}

Destination:

{
  "swagger": "2.0",
  "info": {
    "version": "1.0.0",
    "title": "Swagger Petstore",
    "description": "A sample API that uses a petstore as an example to demonstrate features in the swagger-2.0 specification",
    "termsOfService": "http://swagger.io/terms/",
    "contact": {
      "name": "Swagger API Team"
    },
    "license": {
      "name": "MIT"
    }
  },
  "host": "petstore.swagger.io",
  "basePath": "/api",
  "schemes": [
    "http"
  ],
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/pets": {
      "get": {
        "description": "Returns all pets from the system that the user has access to",
        "produces": [
          "application/json"
        ],
        "responses": {
          "200": {
            "description": "A list of pets.",
            "schema": {
              "$ref": "#/definitions/Pet"
            }
          }
        }
      }
    }
  },
  "definitions": {
    "Pet": {
      "type": "object",
      "required": [
        "id",
        "name",
        "tag"
      ],
      "properties": {
        "id": {
          "type": "integer",
          "format": "int64"
        },
        "name": {
          "type": "string"
        },
        "tag": {
          "type": "string"
        },
        "animal": {
          "type": "string"
        },
        "foodType": {
          "type": "string"
        }
      }
    }
  }
}

• Change is that "tag" is removed from the required list.

[Sherman Thompson]

... it must have been my mistake on which files I was editing. It's working as intended, sorry!

[Sherman Thompson]

Nope I had it right, confusion was that it shows as non-breaking when it should be breaking. It's backwards

[Ben Sayers]

@boogiiman thanks for raising this. We have intentionally designed the tool to consider additions of required constraints to response bodies to be non-breaking changes. I'll explain the thinking behind this and if you still believe this behaviour isn't right you can reply with the specific use case to help us see what we have missed.

The intention of this tool is to tell api providers if the changes they are making to their api's are backward compatible for their consumers.

When you do not specify a property within the required keyword that property is optional. So the source schema will permit tag property to either be not defined or defined as a string. If you think about this from the perspective of consumers of this api, they must be able to support when tag is defined and when it is not.

The change introduced by the destination schema is to add tag to the required list, meaning the schema no longer permits tag to be undefined. All other constraints remain the same. From the perspective of the consumers of the api, no code changes are required to support this api change. There is now dead code in the consumers that handles the case where tag is not defined, which is now no longer possible. However leaving this code in place will not break the integration.

If you were to add a required constraint to a request body instead of a response body the tool would consider this a breaking change. In this example consumers would be sending requests without a tag property defined and this change to the schema would cause those requests to be considered invalid. Consumers would need to be changed to always send the tag property prior to rolling out that api change.

I hope that makes sense, let us know what you think.

[Sherman Thompson]

I had to think about it for a while, but it does make more sense because it is a response body, not request.

I am still unsure of the reverse: if tag was previously a required property and is made optional, why is it marked as a breaking change? It is still listed in the properties, so it's not like it was removed from the response (obviously breaking change). The data type did not change either.

I guess my confusion is more related to the purpose of a required list for object definitions that are only ever used for response bodies. It feels redundant, but that may be more of a design question for swagger than this tool.

Thanks for the explanation!

edit: leaving open to see your response. I do consider this resolved though.

[Sherman Thompson]

Ben,

Sorry for the late(r) reply. I replied on the issue with my thoughts. If everything looks in order you can resolve the issue, or I can do it myself after reading your reply.

I appreciate the thoughtful interactions.

On a side note, if folks looking at the project wanted to suggest features for the tool, is there a place to do so? For example, I would like an option to cut down on the potentially huge json output. But I don't think the issues tab would be the right place for such an idea.

Thanks, Sherman

[Ben Sayers]

Hey @Sherman Thompson , sorry it’s taken me so long to get back to you, I lost track of this ticket.

In general, request schemas are allowed to be relaxed and are not allowed to be constrained. In other words, new request schemas could accept things that were previously not accepted, but must accept everything that was previously accepted. This is because clients could continue to send anything that the old schema permitted.

Response schemas are the opposite, they are not allowed to be relaxed but are allowed to be constrained. In other words, every value accepted by the new schema must be accepted by the old schema, but not every value accepted by the old schema needs to be accepted by the new schema. This is because clients are known to support everything allowed by the old schema.

So if tag is a required property on a request and is subsequently removed from the required list this is relaxing the schema - all requests with a tag property will continue to be accepted in the new schema, so this is not a breaking change.

We don’t really have any other public place for you to raise feature requests so I’m happy for you to make issues here.