From 95d1bf434a3d5f1b2fb759fc3501e73efeadb295 Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Wed, 15 Nov 2023 10:11:44 +0100 Subject: [PATCH] Fixing an issue where new Optional properties were incorrectly flagges as a breaking change Breaking changes happen when the Request payload between one version of the API and another version of the API are incompatible - meaning that the minimum viable payload from the old API version does not work with the new API version. In the event of a new Optional field being added to the Request (or Response) payload the minimum viable request payload remains valid, therefore provided the field is correctly flagged as Required/Optional, we can lean on that to determine whether a breaking change is actually a breaking change. This fixes an issue seen in https://github.com/Azure/azure-rest-api-specs/pull/26680 and https://github.com/Azure/azure-rest-api-specs/pull/22407 and https://github.com/Azure/azure-rest-api-specs/pull/25080 where the API Definition doesn't correctly document all of the possible fields within the Request/Response payloads. Since this is going a conditional check, this commit changes this from an Error to a Warning - as whilst there are situations where this can be a breaking change; this requires understanding the change. --- docs/rules/1045.md | 6 ++++-- .../SwaggerModelerCompareTests.cs | 2 +- .../src/modeler/AutoRest.Swagger/ComparisonContext.cs | 10 ++++++++++ .../src/modeler/AutoRest.Swagger/Model/Schema.cs | 8 +++++++- 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/docs/rules/1045.md b/docs/rules/1045.md index deeb07f2..eddacc7d 100644 --- a/docs/rules/1045.md +++ b/docs/rules/1045.md @@ -1,10 +1,12 @@ ### 1045 - AddedOptionalProperty +> **Note:** Previous versions of this tool incorrectly flagged this as a breaking change - whilst care _does_ need to be taken when reviewing these - new **Optional** fields MUST be optional in request payloads for this to not be a breaking change. + **Description**: Checks whether a property is added to the model from the previous specification. The model includes all the models that referenced by any request or response. -**Cause**: This is considered a breaking change. +**Cause**: A new optional field is added to the payload. This field is **Optional** therefore does not constitute a breaking change, in the event this field is **Required** in the new API version it should instead be marked as such. -**Example**: Property `c` is being added into model `Parameters` . +**Example**: Property `c` is being added into model `Parameters`. Old specification ```json5 diff --git a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs index 5ac2f97e..3279407e 100644 --- a/openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs +++ b/openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs @@ -899,7 +899,7 @@ public void ChangedXmsLongRunningOperation() public void AddedOptionalProperty() { var messages = CompareSwagger("added_optional_property.json").ToArray(); - Assert.Equal(1, messages.Where(m => m.Id == ComparisonMessages.AddedOptionalProperty.Id).Count()); + Assert.Equal(1, messages.Where(m => m.Id == ComparisonMessages.AddedOptionalProperty.Id && m.Severity == Category.Warning).Count()); } diff --git a/openapi-diff/src/modeler/AutoRest.Swagger/ComparisonContext.cs b/openapi-diff/src/modeler/AutoRest.Swagger/ComparisonContext.cs index de033bc7..af24f395 100644 --- a/openapi-diff/src/modeler/AutoRest.Swagger/ComparisonContext.cs +++ b/openapi-diff/src/modeler/AutoRest.Swagger/ComparisonContext.cs @@ -71,6 +71,16 @@ public void LogInfo(MessageTemplate template, params object[] formatArguments) formatArguments )); + public void LogWarning(MessageTemplate template, params object[] formatArguments) + => _messages.Add(new ComparisonMessage( + template, + Path, + _PreviousRootDoc, + _CurrentRootDoc, + Category.Warning, + formatArguments + )); + public void LogError(MessageTemplate template, params object[] formatArguments) => _messages.Add(new ComparisonMessage( template, diff --git a/openapi-diff/src/modeler/AutoRest.Swagger/Model/Schema.cs b/openapi-diff/src/modeler/AutoRest.Swagger/Model/Schema.cs index 2aad27fb..fcff9c6a 100644 --- a/openapi-diff/src/modeler/AutoRest.Swagger/Model/Schema.cs +++ b/openapi-diff/src/modeler/AutoRest.Swagger/Model/Schema.cs @@ -290,7 +290,13 @@ private void CompareProperties(ComparisonContext context, Sch } else if(IsReferenced && property.Value != null) { - context.LogBreakingChange(ComparisonMessages.AddedOptionalProperty, property.Key); + // This is an Optional Property, therefore the minimal payload (e.g. just required fields) + // from the previous API version remains valid in the new API version - therefore this + // IS NOT a breaking change by definition. + // + // Whilst the additional field CAN be specified, it's OPTIONAL by design meaning this can + // be omitted - and is therefore NOT a breaking change. + context.LogWarning(ComparisonMessages.AddedOptionalProperty, property.Key); } } }