From 1747e829b9846def19d8127429d9bb663d1c680d Mon Sep 17 00:00:00 2001 From: Guillaume Pansier Date: Thu, 8 Jun 2023 18:38:50 +0200 Subject: [PATCH 1/3] Add rule RemovedRequiredProperty Resolves #26 --- documentation/rules/2001.md | 64 +++++++++++++++ .../OpenApiSpecificationsCompareTests.cs | 15 ++++ .../new/removed_required_property.json | 75 ++++++++++++++++++ .../Resource/old/different_discriminator.json | 3 - .../old/removed_required_property.json | 77 +++++++++++++++++++ .../Comparators/SchemaComparator.cs | 19 ++++- .../ComparisonRules.cs | 11 +++ 7 files changed, 260 insertions(+), 4 deletions(-) create mode 100644 documentation/rules/2001.md create mode 100644 src/Criteo.OpenApi.Comparator.UTest/Resource/new/removed_required_property.json create mode 100644 src/Criteo.OpenApi.Comparator.UTest/Resource/old/removed_required_property.json diff --git a/documentation/rules/2001.md b/documentation/rules/2001.md new file mode 100644 index 0000000..c25e901 --- /dev/null +++ b/documentation/rules/2001.md @@ -0,0 +1,64 @@ +### 1033 - RemovedProperty + +**Description**: Checks whether an existing required property is removed from the previous specification. + +**Cause**: This is considered a breaking change when the property is used in responses. + +**Example**: Property `name` of schema `Person` is removed in the new version. + +Old specification +```json5 +{ + "Person": { + "type": "object", + "properties": { + "id": { + "type": "integer", + "format": "int32", + "xml": { + "attribute": true + } + }, + "name": { + "type": "string", + "xml": { + "namespace": "http://example.com/schema/sample", + "prefix": "sample" + } + } + }, + "required": [ + "id", + "name" + ] + } +} +``` + +New specification +```json5 +{ + "Person": { + "type": "object", + "properties": { + "id": { + "type": "integer", + "format": "int32", + "xml": { + "attribute": true + } + }, + "name": { + "type": "string", + "xml": { + "namespace": "http://example.com/schema/sample", + "prefix": "sample" + } + } + }, + "required": [ + "id" + ] + } +} +``` diff --git a/src/Criteo.OpenApi.Comparator.UTest/OpenApiSpecificationsCompareTests.cs b/src/Criteo.OpenApi.Comparator.UTest/OpenApiSpecificationsCompareTests.cs index 307d81f..bda882d 100644 --- a/src/Criteo.OpenApi.Comparator.UTest/OpenApiSpecificationsCompareTests.cs +++ b/src/Criteo.OpenApi.Comparator.UTest/OpenApiSpecificationsCompareTests.cs @@ -810,6 +810,21 @@ public void Compare_OAS_Should_Return_Nullable_Property_Changed_When_Nullable_Pr }); } + [Test] + public void CompareOAS_ShouldReturn_RemovedRequiredPropertyDifferences() + { + var differences = CompareSpecifications("removed_required_property.json"); + + var expectedDifference = new ExpectedDifference + { + Rule = ComparisonRules.RemovedRequiredProperty, + Severity = Severity.Warning, + NewJsonRef = "#/paths/~1pets/put/responses/200/content/text~1plain/schema" + }; + differences.AssertContains(expectedDifference, 1); + differences.AssertContainsOnly(expectedDifference); + } + /// /// Helper method -- load two Open Api Specification documents and invoke the comparison logic. /// diff --git a/src/Criteo.OpenApi.Comparator.UTest/Resource/new/removed_required_property.json b/src/Criteo.OpenApi.Comparator.UTest/Resource/new/removed_required_property.json new file mode 100644 index 0000000..28164bb --- /dev/null +++ b/src/Criteo.OpenApi.Comparator.UTest/Resource/new/removed_required_property.json @@ -0,0 +1,75 @@ +{ + "openapi": "3.0.0", + "info": { + "title": "Removed_required_property", + "version": "2.0" + }, + "paths": { + "/pets": { + "put": { + "description": "Update a pet", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PetWrite" + } + } + } + }, + "responses": { + "200": { + "description": "Request successful", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/PetRead" + } + } + } + } + } + } + } + }, + "components": { + "schemas": { + "PetRead": { + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "petType": { + "type": "string", + "enum": [ + "cat", + "dog" + ] + } + }, + "required": [ + "petType" + ] + }, + "PetWrite": { + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "petType": { + "type": "string", + "enum": [ + "cat", + "dog" + ] + } + }, + "required": [ + "petType" + ] + } + } + } +} \ No newline at end of file diff --git a/src/Criteo.OpenApi.Comparator.UTest/Resource/old/different_discriminator.json b/src/Criteo.OpenApi.Comparator.UTest/Resource/old/different_discriminator.json index 943a522..3f1d717 100644 --- a/src/Criteo.OpenApi.Comparator.UTest/Resource/old/different_discriminator.json +++ b/src/Criteo.OpenApi.Comparator.UTest/Resource/old/different_discriminator.json @@ -70,9 +70,6 @@ "schemas": { "Pet": { "type": "object", - "required": [ - "name" - ], "discriminator": { "propertyName": "petType" }, diff --git a/src/Criteo.OpenApi.Comparator.UTest/Resource/old/removed_required_property.json b/src/Criteo.OpenApi.Comparator.UTest/Resource/old/removed_required_property.json new file mode 100644 index 0000000..2e80fb3 --- /dev/null +++ b/src/Criteo.OpenApi.Comparator.UTest/Resource/old/removed_required_property.json @@ -0,0 +1,77 @@ +{ + "openapi": "3.0.0", + "info": { + "title": "Removed_required_property", + "version": "1.0" + }, + "paths": { + "/pets": { + "put": { + "description": "Update a pet", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PetWrite" + } + } + } + }, + "responses": { + "200": { + "description": "Request successful", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/PetRead" + } + } + } + } + } + } + } + }, + "components": { + "schemas": { + "PetRead": { + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "petType": { + "type": "string", + "enum": [ + "cat", + "dog" + ] + } + }, + "required": [ + "name", + "petType" + ] + }, + "PetWrite": { + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "petType": { + "type": "string", + "enum": [ + "cat", + "dog" + ] + } + }, + "required": [ + "name", + "petType" + ] + } + } + } +} \ No newline at end of file diff --git a/src/Criteo.OpenApi.Comparator/Comparators/SchemaComparator.cs b/src/Criteo.OpenApi.Comparator/Comparators/SchemaComparator.cs index 0442ebb..3a7a7bd 100644 --- a/src/Criteo.OpenApi.Comparator/Comparators/SchemaComparator.cs +++ b/src/Criteo.OpenApi.Comparator/Comparators/SchemaComparator.cs @@ -547,7 +547,7 @@ private static void CompareRequired(ComparisonContext context, ISet oldRequired, ISet newRequired) { - if (newRequired == null) + if (newRequired == null && oldRequired == null) return; if (oldRequired == null) @@ -556,12 +556,29 @@ private static void CompareRequired(ComparisonContext context, return; } + if (newRequired == null) + { + context.LogBreakingChange(ComparisonRules.RemovedRequiredProperty, string.Join(", ", oldRequired)); + return; + } + List addedRequiredProperties = newRequired.Except(oldRequired).ToList(); if (addedRequiredProperties.Any()) { context.LogBreakingChange(ComparisonRules.AddedRequiredProperty, string.Join(", ", addedRequiredProperties)); } + + if (context.Direction == DataDirection.Request) + { + return; + } + List removedRequiredProperties = oldRequired.Except(newRequired).ToList(); + if (removedRequiredProperties.Any()) + { + context.LogBreakingChange(ComparisonRules.RemovedRequiredProperty, + string.Join(", ", addedRequiredProperties)); + } } } } diff --git a/src/Criteo.OpenApi.Comparator/ComparisonRules.cs b/src/Criteo.OpenApi.Comparator/ComparisonRules.cs index 4d4c4cb..398378c 100644 --- a/src/Criteo.OpenApi.Comparator/ComparisonRules.cs +++ b/src/Criteo.OpenApi.Comparator/ComparisonRules.cs @@ -609,5 +609,16 @@ public static class ComparisonRules Message = "The nullable property has changed from '{0}' to '{1}'.", Type = MessageType.Update }; + + /// + /// OpenApi Specification version 3 specific + /// + public static ComparisonRule RemovedRequiredProperty = new ComparisonRule() + { + Id = 2001, + Code = nameof(RemovedRequiredProperty), + Message = "The required property '{0}' was removed in the new version.", + Type = MessageType.Removal + }; } } From f021523f8482c928db1cede42717d7b4d51b0bc6 Mon Sep 17 00:00:00 2001 From: guillaume Date: Mon, 12 Jun 2023 09:18:01 +0200 Subject: [PATCH 2/3] Update 2001.md Fix typo in rule 2001 documentation --- documentation/rules/2001.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/rules/2001.md b/documentation/rules/2001.md index c25e901..1f7dfbe 100644 --- a/documentation/rules/2001.md +++ b/documentation/rules/2001.md @@ -1,4 +1,4 @@ -### 1033 - RemovedProperty +### 2001 - RemovedRequiredProperty **Description**: Checks whether an existing required property is removed from the previous specification. From de44d016d6d0acda7b78de94f8a861821c39294e Mon Sep 17 00:00:00 2001 From: guillaume Date: Mon, 12 Jun 2023 09:40:01 +0200 Subject: [PATCH 3/3] Fix RemoveRequiredAttribute (schema) edge case When all required properties are removed, and none added, the comparator reports an error for both Request / Response directions (should be response only) --- src/Criteo.OpenApi.Comparator/Comparators/SchemaComparator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Criteo.OpenApi.Comparator/Comparators/SchemaComparator.cs b/src/Criteo.OpenApi.Comparator/Comparators/SchemaComparator.cs index 3a7a7bd..51ab4a0 100644 --- a/src/Criteo.OpenApi.Comparator/Comparators/SchemaComparator.cs +++ b/src/Criteo.OpenApi.Comparator/Comparators/SchemaComparator.cs @@ -547,7 +547,7 @@ private static void CompareRequired(ComparisonContext context, ISet oldRequired, ISet newRequired) { - if (newRequired == null && oldRequired == null) + if (newRequired == null && (oldRequired == null || context.Direction == DataDirection.Request)) return; if (oldRequired == null)