Shouldn't change in the type of any existing Request or Response property be a breaking change?
Hi,
Currently, if I change the type of any existing property in the source spec, e.g. In source spec:-
description:
type: string
description: property description, free text
In the destination spec:-
description:
type: integer
description: property description, free text
On doing the diff, it does not spit out as a breaking change…It shows as non-breaking change…Why so?
wont it impact the existing consumers who are passing the property as String and now it expects Integer?
Comments (10)
-
-
reporter You are correct Ben…My bad..
the breakingchanges: true attribute got missed in the long json that got spit out.Would be really good if there is a flag which spits exactly the change which is present and not the long JSON..
Resolving this one for now…Sorry for the confusion -
reporter - changed status to resolved
Not an issue
-
reporter - changed status to open
Opening this as there is one more confusion which led me to think this is an issue
-
reporter However Ben, I realised that the output JSON is pretty confusing..
Even if I change type of one property, the change is shown in Breaking as well as non-breaking changes..
Below are the examples:-petstore_old.yaml:-
openapi: "3.0.0" info: version: 1.0.0 title: Swagger Petstore license: name: MIT servers: - url: http://petstore.swagger.io/v1 paths: /pets: get: summary: List all pets operationId: listPets tags: - pets parameters: - name: limit in: query description: How many items to return at one time (max 100) required: false schema: type: integer format: int32 responses: '200': description: A paged array of pets headers: x-next: description: A link to the next page of responses schema: type: string content: application/json: schema: $ref: "#/components/schemas/Pets" default: description: unexpected error content: application/json: schema: $ref: "#/components/schemas/Error" post: summary: Create a pet operationId: createPets tags: - pets responses: '201': description: Null response default: description: unexpected error content: application/json: schema: $ref: "#/components/schemas/Error" /pets/{petId}: get: summary: Info for a specific pet operationId: showPetById tags: - pets parameters: - name: petId in: path required: true description: The id of the pet to retrieve schema: type: string responses: '200': description: Expected response to a valid request content: application/json: schema: $ref: "#/components/schemas/Pet" default: description: unexpected error content: application/json: schema: $ref: "#/components/schemas/Error" components: schemas: Pet: type: object required: - id - name properties: id: type: integer format: int64 name: type: string tag: type: string Pets: type: array items: $ref: "#/components/schemas/Pet" Error: type: object required: - code - message properties: code: type: integer format: int32 message: type: string
petstore_new.yaml
openapi: "3.0.0" info: version: 1.0.0 title: Swagger Petstore license: name: MIT servers: - url: http://petstore.swagger.io/v1 paths: /pets: get: summary: List all pets operationId: listPets tags: - pets parameters: - name: limit in: query description: How many items to return at one time (max 100) required: false schema: type: integer format: int32 responses: '200': description: A paged array of pets headers: x-next: description: A link to the next page of responses schema: type: string content: application/json: schema: $ref: "#/components/schemas/Pets" default: description: unexpected error content: application/json: schema: $ref: "#/components/schemas/Error" post: summary: Create a pet operationId: createPets tags: - pets responses: '201': description: Null response default: description: unexpected error content: application/json: schema: $ref: "#/components/schemas/Error" /pets/{petId}: get: summary: Info for a specific pet operationId: showPetById tags: - pets parameters: - name: petId in: path required: true description: The id of the pet to retrieve schema: type: string responses: '200': description: Expected response to a valid request content: application/json: schema: $ref: "#/components/schemas/Pet" default: description: unexpected error content: application/json: schema: $ref: "#/components/schemas/Error" components: schemas: Pet: type: object required: - id - name properties: id: type: string name: type: string tag: type: string Pets: type: array items: $ref: "#/components/schemas/Pet" Error: type: object required: - code - message properties: code: type: integer format: int32 message: type: string
Note:- the type of components → schemas → Pet → id is changed to String from integer.
Now when I run :- openapi-diff petstore_old.yaml petstore_new.yaml
It gives following output:-
Breaking changes found between the two specifications: { "breakingDifferences": [ { "type": "breaking", "action": "add", "code": "response.body.scope.add", "destinationSpecEntityDetails": [ { "location": "paths./pets.get.responses.200.content.application/json.schema", "value": { "type": "array", "items": { "type": "object", "required": [ "id", "name" ], "properties": { "id": { "type": "string" }, "name": { "type": "string" }, "tag": { "type": "string" } } } } } ], "entity": "response.body.scope", "source": "json-schema-diff", "sourceSpecEntityDetails": [ { "location": "paths./pets.get.responses.200.content.application/json.schema", "value": { "type": "array", "items": { "type": "object", "required": [ "id", "name" ], "properties": { "id": { "type": "integer", "format": "int64" }, "name": { "type": "string" }, "tag": { "type": "string" } } } } } ], "details": { "differenceSchema": { "type": "array", "items": { "type": "object", "properties": { "id": { "type": "string" }, "name": { "type": "string" }, "tag": { "type": "string" } }, "required": [ "id", "name" ] }, "minItems": 1 } } }, { "type": "breaking", "action": "add", "code": "response.body.scope.add", "destinationSpecEntityDetails": [ { "location": "paths./pets/{petId}.get.responses.200.content.application/json.schema", "value": { "type": "object", "required": [ "id", "name" ], "properties": { "id": { "type": "string" }, "name": { "type": "string" }, "tag": { "type": "string" } } } } ], "entity": "response.body.scope", "source": "json-schema-diff", "sourceSpecEntityDetails": [ { "location": "paths./pets/{petId}.get.responses.200.content.application/json.schema", "value": { "type": "object", "required": [ "id", "name" ], "properties": { "id": { "type": "integer", "format": "int64" }, "name": { "type": "string" }, "tag": { "type": "string" } } } } ], "details": { "differenceSchema": { "type": "object", "properties": { "id": { "type": "string" }, "name": { "type": "string" }, "tag": { "type": "string" } }, "required": [ "id", "name" ] } } } ], "breakingDifferencesFound": true, "nonBreakingDifferences": [ { "type": "non-breaking", "action": "remove", "code": "response.body.scope.remove", "destinationSpecEntityDetails": [ { "location": "paths./pets.get.responses.200.content.application/json.schema", "value": { "type": "array", "items": { "type": "object", "required": [ "id", "name" ], "properties": { "id": { "type": "string" }, "name": { "type": "string" }, "tag": { "type": "string" } } } } } ], "entity": "response.body.scope", "source": "json-schema-diff", "sourceSpecEntityDetails": [ { "location": "paths./pets.get.responses.200.content.application/json.schema", "value": { "type": "array", "items": { "type": "object", "required": [ "id", "name" ], "properties": { "id": { "type": "integer", "format": "int64" }, "name": { "type": "string" }, "tag": { "type": "string" } } } } } ], "details": { "differenceSchema": { "type": "array", "items": { "type": "object", "properties": { "id": { "type": "integer" }, "name": { "type": "string" }, "tag": { "type": "string" } }, "required": [ "id", "name" ] }, "minItems": 1 } } }, { "type": "non-breaking", "action": "remove", "code": "response.body.scope.remove", "destinationSpecEntityDetails": [ { "location": "paths./pets/{petId}.get.responses.200.content.application/json.schema", "value": { "type": "object", "required": [ "id", "name" ], "properties": { "id": { "type": "string" }, "name": { "type": "string" }, "tag": { "type": "string" } } } } ], "entity": "response.body.scope", "source": "json-schema-diff", "sourceSpecEntityDetails": [ { "location": "paths./pets/{petId}.get.responses.200.content.application/json.schema", "value": { "type": "object", "required": [ "id", "name" ], "properties": { "id": { "type": "integer", "format": "int64" }, "name": { "type": "string" }, "tag": { "type": "string" } } } } ], "details": { "differenceSchema": { "type": "object", "properties": { "id": { "type": "integer" }, "name": { "type": "string" }, "tag": { "type": "string" } }, "required": [ "id", "name" ] } } } ], "unclassifiedDifferences": [] }
The confusion is :-
Why does it populate the change in both breaking as well as non-breaking changes??
-
Because when you change the type of a property you are performing 2 seperate modifications to the schema:
- You are removing the possibility of the property being an integer, reducing the scope of the schema
- You are adding the possibility of the property being a string, adding to the scope of the schema
In the context of http responses, it is safe for a server to reduce the scope of a schema. This is because if a client supports all the responses permitted by the original schema, then it also supports all the responses of any subset of that schema. So modification 1 is a non-breaking change. However adding to the scope of the schema is not safe. If the new schema allows responses the old schema does not, we cannot assume the client will be able to handle them. So modification 2 is a breaking change.
While output of this tool is very accurate, it’s also difficult to consume and is not particularly human readable. We know it’s an issue, we’re open to suggestions on what might help.
-
@Ben Sayers , seems to me like there’s a lot of unnecessary information in the above result. Yes, the response body of both those endpoints changed, but the change could be described using a single add/remove at the location of
components/schemas/Pet
, right?Additionally, the described affected area seems overly-broad. It describes the entire response object as having changed when it’s really only a single property.
For example, perhaps the breaking change could be described like this instead?
{ "type": "breaking", "action": "add", "destinationSpecEntityDetails": [ { "location": "paths./pets.get.responses.200.content.application/json.schemas.Pet.properties.id", "value": { "type": "string" } } ], "sourceSpecEntityDetails": [ { "location": "paths./pets.get.responses.200.content.application/json.schemas.Pet.properties.id", "value": { "type": "integer", "format": "int64" } } ] // ... },
I’m sure there’s something I’m overlooking here, as I just tried this tool out for the first time today, but I too was astounded at the amount of output a single property type change generated.
-
The output contains the raw calculated result of what was added or removed. This raw result is very precise, but is also very verbose. I agree its difficult to consume and we should improve it. I like your idea but I’m not sure how we would implement it, how would we determine what parts of the calculated result are interesting?
You’ve actually made me wonder if we should simply display a git style text diff of what has changed instead of showing this calculation. Do you think that would be more useful?
-
That might be very familiar to your user base, yeah. On the other hand, your current format effectively ignores changes to the document that don't affect the JSON output structure, such as changing the order of parameters on the documentation, right? That seems useful given that most
HTTP implementations I’ve seen don’t care about the order of properties in a request. -
Yes you are right, outputting the text diff will potentially draw attention to changes that are not actually changing the values that are accepted, so it won’t be perfect. My thinking is outputting that combined with the tools conclusion about if any breaking changes were found might be more useful then what we are doing now.
- Log in to comment
I agree changing the type of a property in the request or response should be marked as a breaking change. However we have extensive test coverage in this tool that verifies that it does indeed behave this way. Can you please share the example files that demonstrate this issue along with the version number of node and this module you are using so we investigate what’s going on?