diff --git a/docs/README.md b/docs/README.md index 070a41c9..32bd1327 100644 --- a/docs/README.md +++ b/docs/README.md @@ -73,3 +73,7 @@ [1039 - AddedOperation](rules/1039.md) [1040 - AddedReadOnlyPropertyInResponse](rules/1040.md) + +[1041 - RemovedDeprecatedOperation](rules/1041.md) + +[1042 - RemovedDeprecatedPath](rules/1042.md) diff --git a/docs/rules/1041.md b/docs/rules/1041.md new file mode 100644 index 00000000..2e098a89 --- /dev/null +++ b/docs/rules/1041.md @@ -0,0 +1,71 @@ +### 1041 - RemovedDeprecatedOperation + +**Description**: Checks whether an existing operation with `deprecated: true` from a path is removed from the previous specification. + +**Cause**: This is considered a change but not necessarily breaking. Removing deprecated operations can be a part of planned maintenance. + +**Example**: Deprecated operation `get` from Path `/subscriptions/{subscriptionId}/providers/Microsoft.Contoso/resource1/subResource1` is being removed. + +Old specification +```json5 +{ + "swagger": "2.0", + "info": { + "title": "swagger", + "description": "The Azure Management API.", + "version": "2016-12-01", + ... + ... + "paths": { + "/subscriptions/{subscriptionId}/providers/Microsoft.Contoso/resource1": { + "get": { + ... + }, + "put": { + ... + } + }, + "/subscriptions/{subscriptionId}/providers/Microsoft.Contoso/resource1/subResource1": { + "get": { + ... + "deprecated": true + }, + "delete": { + ... + } + } + ... + ... +``` + +New specification +```json5 +{ + "swagger": "2.0", + "info": { + "title": "swagger", + "description": "The Azure Management API.", + "version": "2016-12-01", + ... + ... + "paths": { + "/subscriptions/{subscriptionId}/providers/Microsoft.Contoso/resource1": { + "get": { + ... + }, + "put": { + ... + } + }, + "/subscriptions/{subscriptionId}/providers/Microsoft.Contoso/resource1/subResource1": { + "post": { + ... + }, + "delete": { + ... + } + } + ... + ... +``` + diff --git a/docs/rules/1042.md b/docs/rules/1042.md new file mode 100644 index 00000000..dcd5e6c1 --- /dev/null +++ b/docs/rules/1042.md @@ -0,0 +1,63 @@ +### 1042 - RemovedDeprecatedPath + +**Description**: Checks whether a deprecated path is removed from the previous specification. A path is considered deprecated when all its operations are deprecated + +**Cause**: This is considered change but not necessarily breaking. Removing deprecated paths can be a part of planned maintenance. + +**Example**: Path `/subscriptions/{subscriptionId}/providers/Microsoft.Contoso/resource1/subResource1` is being removed. + +Old specification +```json5 +{ + "swagger": "2.0", + "info": { + "title": "swagger", + "description": "The Azure Management API.", + "version": "2016-12-01", + ... + ... + "paths": { + "/subscriptions/{subscriptionId}/providers/Microsoft.Contoso/resource1": { + "get": { + ... + }, + "put": { + ... + } + }, + "/subscriptions/{subscriptionId}/providers/Microsoft.Contoso/resource1/subResource1": { + "get": { + ... + "deprecated": true + }, + "delete": { + ... + "deprecated": true + } + } + ... + ... +``` + +New specification +```json5 +{ + "swagger": "2.0", + "info": { + "title": "swagger", + "description": "The Azure Management API.", + "version": "2016-12-01", + ... + ... + "paths": { + "/subscriptions/{subscriptionId}/providers/Microsoft.Contoso/resource1": { + "get": { + ... + }, + "put": { + ... + } + } + ... + ... +``` diff --git a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/AutoRest.Swagger.Tests.csproj b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/AutoRest.Swagger.Tests.csproj index 2210b087..6628adfd 100644 --- a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/AutoRest.Swagger.Tests.csproj +++ b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/AutoRest.Swagger.Tests.csproj @@ -24,6 +24,8 @@ + + @@ -52,6 +54,8 @@ + + diff --git a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/new/removed_deprecated_operation.json b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/new/removed_deprecated_operation.json new file mode 100644 index 00000000..ad5905ac --- /dev/null +++ b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/new/removed_deprecated_operation.json @@ -0,0 +1,24 @@ +{ + "swagger": 2.0, + "info": { + "title": "removed_deprecated_operation", + "version": "1.0" + }, + "host": "localhost:8000", + "schemes": [ "http", "https" ], + "paths": { + "/api/Paths": { + "get": { + "tag": [ "Paths" ], + "operationId": "Paths_Get", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": {} + } + }, + "/api/Operations": { + } + } +} diff --git a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/new/removed_deprecated_path.json b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/new/removed_deprecated_path.json new file mode 100644 index 00000000..b9f4f4e4 --- /dev/null +++ b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/new/removed_deprecated_path.json @@ -0,0 +1,42 @@ +{ + "swagger": 2.0, + "info": { + "title": "removed_deprecated_path", + "version": "1.0" + }, + "host": "localhost:8000", + "schemes": [ "http", "https" ], + "paths": { + "/api/Paths": { + "get": { + "tag": [ "Paths" ], + "operationId": "Paths_Get", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": {} + } + }, + "/api/Operations": { + "get": { + "tag": [ "Operations" ], + "operationId": "Operations_Get", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": {} + }, + "post": { + "tag": [ "Operations" ], + "operationId": "Operations_Post", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": {} + } + } + } +} diff --git a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/old/removed_deprecated_operation.json b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/old/removed_deprecated_operation.json new file mode 100644 index 00000000..53cdfc70 --- /dev/null +++ b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/old/removed_deprecated_operation.json @@ -0,0 +1,34 @@ +{ + "swagger": 2.0, + "info": { + "title": "removed_deprecated_operation", + "version": "1.0" + }, + "host": "localhost:8000", + "schemes": [ "http", "https" ], + "paths": { + "/api/Paths": { + "get": { + "tag": [ "Paths" ], + "operationId": "Paths_Get", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": {} + } + }, + "/api/Operations": { + "get": { + "tag": [ "Operations" ], + "operationId": "Operations_Get", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": {}, + "deprecated": true + } + } + } +} diff --git a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/old/removed_deprecated_path.json b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/old/removed_deprecated_path.json new file mode 100644 index 00000000..ce82ad01 --- /dev/null +++ b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/old/removed_deprecated_path.json @@ -0,0 +1,117 @@ +{ + "swagger": 2.0, + "info": { + "title": "removed_deprecated_path", + "version": "1.0" + }, + "host": "localhost:8000", + "schemes": [ "http", "https" ], + "paths": { + "/api/Paths": { + "get": { + "tag": [ "Paths" ], + "operationId": "Paths_Get", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": {} + } + }, + "/api/Operations": { + "get": { + "tag": [ "Operations" ], + "operationId": "Operations_Get", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": {} + }, + "post": { + "tag": [ "Operations" ], + "operationId": "Operations_Post", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": {} + } + }, + "/api/Parameters/{a}": { + "get": { + "tag": [ "Parameters" ], + "operationId": "Parameters_Get", + "produces": [ + "text/plain" + ], + "parameters": [ + { + "name": "a", + "in": "path", + "required": true, + "type": "string" + }, + { + "name": "b", + "in": "query", + "required": true, + "type": "string" + }, + { + "name": "d", + "in": "query", + "required": true, + "type": "string" + }, + { + "name": "e", + "in": "query", + "required": false, + "type": "string" + }, + { + "name": "f", + "in": "query", + "required": true, + "type": "string", + "enum": [ "theonlyvalue" ] + } + ], + "deprecated": true + } + }, + "/api/Responses": { + "get": { + "tag": [ "Responses" ], + "operationId": "Responses_Get", + "produces": [ + "text/plain" + ], + "parameters": [], + "responses": { + "200": { + "schema": { + "type": "integer" + } + }, + "201": { + "schema": { + "type": "integer" + } + }, + "400": { + "schema": { + "type": "object", + "properties": { + "message": { "type": "string" }, + "id": { "type": "string" } + } + } + } + }, + "deprecated": true + } + } + } +} diff --git a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs index 29c0b648..5bb0de4e 100644 --- a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs +++ b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs @@ -200,6 +200,20 @@ public void PathRemoved() Assert.NotEmpty(missing.Where(m => m.Severity == Category.Error && m.OldJsonRef == "old/removed_path.json#/paths/~1api~1Responses")); } + /// + /// Verifies that if you remove (or rename) a path, it's caught. + /// But not the same warning as removing a non deprecated path + /// + [Fact] + public void DeprecatedPathRemoved() + { + var messages = CompareSwagger("removed_deprecated_path.json").ToArray(); + var missing = messages.Where(m => m.Id == ComparisonMessages.RemovedDeprecatedpath.Id); + Assert.Equal(2, missing.Count()); + Assert.NotEmpty(missing.Where(m => m.Severity == Category.Info && m.OldJsonRef == "old/removed_deprecated_path.json#/paths/~1api~1Parameters~1{a}")); + Assert.NotEmpty(missing.Where(m => m.Severity == Category.Info && m.OldJsonRef == "old/removed_deprecated_path.json#/paths/~1api~1Responses")); + } + /// /// Verifies that if you remove an operation, it's caught. /// @@ -212,6 +226,19 @@ public void OperationRemoved() Assert.NotEmpty(missing.Where(m => m.Severity == Category.Error && m.NewJsonRef == "new/removed_operation.json#/paths/~1api~1Operations")); } + /// + /// Verifies that if you remove a deprecated operation, it's caught. + /// But not with the same warning as removing an non-deprecated operation + /// + [Fact] + public void DeprecatedOperationRemoved() + { + var messages = CompareSwagger("removed_deprecated_operation.json").ToArray(); + var missing = messages.Where(m => m.Id == ComparisonMessages.RemovedDeprecatedOperation.Id); + Assert.Single(missing); + Assert.NotEmpty(missing.Where(m => m.Severity == Category.Info && m.NewJsonRef == "new/removed_deprecated_operation.json#/paths/~1api~1Operations")); + } + /// /// Verifies that if you change the operations id for a path, it's caught. /// diff --git a/openapi-diff/src/modeler/AutoRest.Swagger/ComparisonMessages.cs b/openapi-diff/src/modeler/AutoRest.Swagger/ComparisonMessages.cs index 075e43d7..84e40185 100644 --- a/openapi-diff/src/modeler/AutoRest.Swagger/ComparisonMessages.cs +++ b/openapi-diff/src/modeler/AutoRest.Swagger/ComparisonMessages.cs @@ -50,6 +50,14 @@ public static class ComparisonMessages Type = MessageType.Removal }; + public static MessageTemplate RemovedDeprecatedpath = new MessageTemplate + { + Id = 1042, + Code = nameof(ComparisonMessages.RemovedDeprecatedpath), + Message = "The new version is missing a path that was deprecated in the old version. Can upstream services function without path '{0}'?", + Type = MessageType.Removal + }; + public static MessageTemplate AddedPath = new MessageTemplate { Id = 1038, @@ -66,6 +74,14 @@ public static class ComparisonMessages Type = MessageType.Removal }; + public static MessageTemplate RemovedDeprecatedOperation = new MessageTemplate + { + Id = 1041, + Code = nameof(ComparisonMessages.RemovedDeprecatedOperation), + Message = "The new version is missing an operation that was deprecated in the old version. Can upstream services function without operationId '{0}'?", + Type = MessageType.Removal + }; + public static MessageTemplate ModifiedOperationId = new MessageTemplate { Id = 1008, diff --git a/openapi-diff/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs b/openapi-diff/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs index da750e0b..bdd0dce2 100644 --- a/openapi-diff/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs +++ b/openapi-diff/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs @@ -1,4 +1,5 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. + +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. See License.txt in the project root for license information. using System; @@ -196,8 +197,16 @@ ServiceDefinition previousDefinition if (!newPaths.TryGetValue(p, out var operations)) { - // Entrie path was removeed - context.LogBreakingChange(ComparisonMessages.RemovedPath, path); + if(previousDefinition.Paths[path].Count != 0 && previousDefinition.Paths[path].All(operation => operation.Value.Deprecated)) + { + // Entry path removed contained only deprecated operations, implies path was deprecated + context.LogInfo(ComparisonMessages.RemovedDeprecatedpath, path); + } + else + { + // Entry path was removeed + context.LogBreakingChange(ComparisonMessages.RemovedPath, path); + } } else { @@ -211,6 +220,12 @@ ServiceDefinition previousDefinition { if (!operations.TryGetValue(previousOperation.Key, out var newOperation)) { + if (previousOperation.Value.Deprecated) + { + // Deprecated operation was removed from the path + context.LogInfo(ComparisonMessages.RemovedDeprecatedOperation, previousOperation.Value.OperationId); + } + // Operation was removed from the path context.LogBreakingChange(ComparisonMessages.RemovedOperation, previousOperation.Value.OperationId); } diff --git a/package-lock.json b/package-lock.json index 792ca709..c20ca630 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "@azure/oad", - "version": "0.6.0", + "version": "0.6.3", "lockfileVersion": 1, "requires": true, "dependencies": {