Skip to content

Fixing an issue where new Optional properties were incorrectly flagged as a breaking change #283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions docs/rules/1045.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}


Expand Down
10 changes: 10 additions & 0 deletions openapi-diff/src/modeler/AutoRest.Swagger/ComparisonContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
8 changes: 7 additions & 1 deletion openapi-diff/src/modeler/AutoRest.Swagger/Model/Schema.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down