OpenAPITools / openapi-diff

Utility for comparing two OpenAPI specifications.
Apache License 2.0
796 stars 154 forks source link

Introduce check concept from "Azure/openapi-diff" project #26

Open jmini opened 6 years ago

jmini commented 6 years ago

The project Azure/openapi-diff produce following output when two versions of a spec are compared: a list of items (error, warning, info) is computed.

Example:

$ oad compare old/added_path.json new/added_path.json
{
  "id": "1001",
  "code": "NoVersionChange",
  "message": "The versions have not changed.",
  "jsonref": "#",
  "json-path": "#",
  "type": "Info"
}
{
  "id": "1039",
  "code": "AddedOperation",
  "message": "The new version is adding an operation that was not found in the old version.",
  "jsonref": "#/paths/~1api~1Operations/post",
  "json-path": "#/paths/api/Operations/post",
  "type": "Info"
}
{
  "id": "1038",
  "code": "AddedPath",
  "message": "The new version is adding a path that was not found in the old version.",
  "jsonref": "#/paths/~1api~1Paths",
  "json-path": "#/paths/api/Paths",
  "type": "Info"
}

The list of rules that are currently checked can be found in their git repository: docs I had a look at their test suite in this folder: Swagger

To run their the oad tool against it, I had to perform some changes: https://github.com/Azure/openapi-diff/pull/108 Then I have converted the old Swagger 2.0 format to OpenAPI 3.0.1 format.

Of course, I will share my work. We can then continue the discussion and see how this could be integrated.

quen2404 commented 6 years ago

Hi @jmini, i'm looking the docs folder in Azure/openapi-diff. Currently their rules are for swagger 2.0, so i tried to upgrade them to OpenAPI 3.0.1 format. But I don't agree with some points of their rules. For example, remove a component like parameter or schema in components, it's not always a breaking change. It is a breaking change only if a parameter in an operation is removed. We should consider that changing the specification architecture is not a breaking change if the api description is equivalent. But maybe we should add new report level about that, like {type: "warning", "id": "XXX", "SchemaNameChanged", message: "Schema reference has been renamed in the new version."}. Because it is not an api change, but it could broke code of clients/servers generated from specification.

Otherwise, using a json report related to defined code is a good idea. Thank you for sharing, I will be happy to discuss with you about that subject. I'm sure we will integrate a strong enhancement.

jmini commented 6 years ago

Thank you for your answer.

Of course I am not saying that the Azure team approach is right. Maybe some rules do not make sense. For the moment, the Azure team is not planing to use the OpenAPI 3.0.1 format yet. See: https://github.com/Azure/autorest/issues/2680 (if I understood everything correctly autorest is a dependency of the Azure/openapi-diff tool)

When I have modified their test suite in order to remove the errors and in order to run the oad tool, I was not sure about the collectionFormat attribute. I have removed it, but this might break some test cases.

I have run oad for each of the modified test cases. See results here: oad-results

I have converted the tests cases to the OpenAPI 3.0.1 format here: openapi301-json

quen2404 commented 6 years ago

Ok, thank you. I will look that this noon.

jmini commented 6 years ago

When I have modified their test suite in order to remove the errors and in order to run the oad tool, I was not sure about the collectionFormat attribute. I have removed it, but this might break some test cases.

There is a clear definition, how collectionFormat should be migrated from Swagger v2 spec to OpenAPI v3 spec. The new attributes are style and explode. See a conversion table in https://github.com/swagger-api/swagger-parser/issues/690.

Note: swagger-parser should do the conversion when https://github.com/swagger-api/swagger-parser/pull/696 is merged.