Fixing an issue where new Optional properties were incorrectly flagge…

…s 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 Azure/azure-rest-api-specs#26680
and Azure/azure-rest-api-specs#22407 and
Azure/azure-rest-api-specs#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

@@ -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

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());
         }
 
 

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, 

openapi-diff/src/modeler/AutoRest.Swagger/Model/Schema.cs

@@ -290,7 +290,13 @@ private void CompareProperties(ComparisonContext<ServiceDefinition> 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);
                         }
                     }
                 }