Skip to content

Commit e9c9600

Browse files
author
Konrad Jamrozik
authored
Do not report AddedRequiredProperty if the property is readOnly (#336)
1 parent 92878df commit e9c9600

File tree

8 files changed

+164
-26
lines changed

8 files changed

+164
-26
lines changed

‎openapi-diff/src/modeler/AutoRest.Swagger.Tests/AutoRest.Swagger.Tests.csproj

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,12 @@
88
<LangVersion>7.1</LangVersion>
99
</PropertyGroup>
1010

11-
<ItemGroup>
12-
<None Remove="Resource\Swagger\new\enum_as_string.json" />
13-
<None Remove="Resource\Swagger\old\enum_as_string.json" />
14-
</ItemGroup>
11+
1512

1613
<ItemGroup>
1714
<Content Include="Resource\Swagger\new\added_path.json" />
18-
<Content Include="Resource\Swagger\new\added_required_property.json">
19-
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
20-
</Content>
15+
<Content Include="Resource\Swagger\new\added_readonly_required_property.json" />
16+
<Content Include="Resource\Swagger\new\added_required_property.json" />
2117
<Content Include="Resource\Swagger\new\changed_operation_id.json" />
2218
<Content Include="Resource\Swagger\new\enum_as_string.json" />
2319
<Content Include="Resource\Swagger\new\enum_values_changed.json" />
@@ -52,9 +48,8 @@
5248
<Content Include="Resource\Swagger\new\version_check_02.json" />
5349
<Content Include="Resource\Swagger\new\version_check_03.json" />
5450
<Content Include="Resource\Swagger\new\version_check_04.json" />
55-
<Content Include="Resource\Swagger\old\added_required_property.json">
56-
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
57-
</Content>
51+
<Content Include="Resource\Swagger\old\added_readonly_required_property.json" />
52+
<Content Include="Resource\Swagger\old\added_required_property.json" />
5853
<Content Include="Resource\Swagger\old\recursive_model.json" />
5954
<Content Include="Resource\Swagger\old\changed_operation_id.json" />
6055
<Content Include="Resource\Swagger\old\added_path.json" />

‎openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/new/added_readonly_required_property.json

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
{
2+
"swagger": 2.0,
3+
"info": {
4+
"title": "added_required_property",
5+
"version": "1.0"
6+
},
7+
"host": "localhost:8000",
8+
"schemes": [ "http", "https" ],
9+
"consumes": [ "text/plain", "text/json" ],
10+
"produces": [ "text/plain" ],
11+
"paths": {
12+
"/api/Parameters": {
13+
"put": {
14+
"tag": [ "Parameters" ],
15+
"operationId": "Parameters_Put",
16+
"produces": [
17+
"text/plain"
18+
],
19+
"parameters": [
20+
{
21+
"name": "database",
22+
"in": "body",
23+
"required": true,
24+
"type": "object",
25+
"schema": { "$ref": "#/definitions/Database" }
26+
}
27+
]
28+
}
29+
}
30+
},
31+
"definitions": {
32+
"Database": {
33+
"properties": {
34+
"a": {
35+
"type": "string",
36+
"description": "Enum.",
37+
"enum": [ "a1", "a2", "a3" ]
38+
39+
},
40+
"b": {
41+
"type": "string",
42+
"description": "Enum.",
43+
"enum": [ "b1" ],
44+
"readOnly": true
45+
}
46+
},
47+
"required": [ "b" ]
48+
}
49+
}
50+
}

‎openapi-diff/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/old/added_readonly_required_property.json

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
{
2+
"swagger": 2.0,
3+
"info": {
4+
"title": "added_required_property",
5+
"version": "1.0"
6+
},
7+
"host": "localhost:8000",
8+
"schemes": [ "http", "https" ],
9+
"consumes": [ "text/plain", "text/json" ],
10+
"produces": [ "text/plain" ],
11+
"paths": {
12+
"/api/Parameters": {
13+
"put": {
14+
"tag": [ "Parameters" ],
15+
"operationId": "Parameters_Put",
16+
"produces": [
17+
"text/plain"
18+
],
19+
"parameters": [
20+
{
21+
"name": "database",
22+
"in": "body",
23+
"required": true,
24+
"type": "object",
25+
"schema": { "$ref": "#/definitions/Database" }
26+
}
27+
]
28+
}
29+
}
30+
},
31+
"definitions": {
32+
"Database": {
33+
"properties": {
34+
"a": {
35+
"type": "string",
36+
"description": "Enum.",
37+
"enum": [ "a1", "a2", "a3" ]
38+
39+
},
40+
"b": {
41+
"type": "string",
42+
"description": "Enum.",
43+
"enum": [ "b1" ],
44+
"readOnly": true
45+
}
46+
}
47+
}
48+
}
49+
}

‎openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -342,20 +342,35 @@ public void OptionalParameterRemoved()
342342
}
343343

344344
/// <summary>
345-
/// Verifies that if you add a required property in the model, it's found.
345+
/// Verifies that if you add a required property in the model, it is reported.
346346
/// </summary>
347347
[Fact]
348348
public void AddedRequiredProperty()
349349
{
350-
var messages = CompareSwagger("added_required_property.json").ToArray();
351-
var missing = messages.Where(m => m.Severity == Category.Error && m.Id == ComparisonMessages.AddedRequiredProperty.Id);
350+
ComparisonMessage[] messages = CompareSwagger("added_required_property.json").ToArray();
351+
List<ComparisonMessage> missing = messages.Where(
352+
m => m.Severity == Category.Error && m.Id == ComparisonMessages.AddedRequiredProperty.Id).ToList();
352353
Assert.Equal(2, missing.Count());
353-
var error = missing.First();
354+
ComparisonMessage error = missing.First();
354355
Assert.Equal("new/added_required_property.json#/paths/~1api~1Parameters/put/parameters/0/schema", error.NewJsonRef);
355356
Assert.NotNull(error.NewJson());
356357
Assert.NotNull(error.OldJson());
357358
}
358359

360+
/// <summary>
361+
/// Verifies that if you add a required property in the model on a property that is 'readOnly',
362+
/// it is not reported.
363+
/// Details: https://github.com/Azure/azure-sdk-tools/issues/7184
364+
/// </summary>
365+
[Fact]
366+
public void AddedReadOnlyRequiredProperty()
367+
{
368+
ComparisonMessage[] messages = CompareSwagger("added_readonly_required_property.json").ToArray();
369+
List<ComparisonMessage> addedReqPropMessages = messages.Where(
370+
m => m.Severity == Category.Error && m.Id == ComparisonMessages.AddedRequiredProperty.Id).ToList();
371+
Assert.Equal(0, addedReqPropMessages.Count);
372+
}
373+
359374
/// <summary>
360375
/// Verifies that if you remove a required request header, it's found.
361376
/// </summary>

‎openapi-diff/src/modeler/AutoRest.Swagger/ComparisonMessages.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ public static class ComparisonMessages
1414
{
1515
Id = 1034,
1616
Code = nameof(ComparisonMessages.AddedRequiredProperty),
17-
Message = "The new version has new required property '{0}' that was not found in the old version.",
17+
Message = "The new version lists new non-read-only properties as required: '{0}'. These properties were not listed as required in the old version.",
1818
Type = MessageType.Addition
1919
};
2020

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

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -184,27 +184,54 @@ Schema previous
184184
}
185185

186186
/// <summary>
187-
/// Comapares list of required properties of this model
187+
/// Compares list of required properties of this (i.e. "new") model to the previous (i.e. "old") model.
188+
///
189+
/// 1. If the property **was** required and **is** required: do not report a breaking change.
190+
/// This is regardless of property being actually defined or not (previously or now).
191+
/// 2. If the property **was not** required and **is** required then:
192+
/// - If the property **was defined and read-only** and **is defined and read-only** then do not report breaking change.
193+
/// - In all other cases, report a breaking change.
194+
/// - E.g. if the property **was not required and not defined** and now **is required and defined and read-only**
195+
/// this still counts as a breaking change.
196+
///
197+
/// Related:
198+
/// https://stackoverflow.com/questions/40113049/how-to-specify-if-a-field-is-optional-or-required-in-openapi-swagger
199+
/// https://github.com/Azure/azure-sdk-tools/issues/7184
200+
/// https://github.com/Azure/openapi-diff/pull/336#discussion_r1638996922
188201
/// </summary>
189-
/// <param name="context">Comaprision Context</param>
202+
/// <param name="context">Comparison Context</param>
190203
/// <param name="priorSchema">Schema of the old model</param>
191204
private void CompareRequired(ComparisonContext<ServiceDefinition> context, Schema priorSchema)
192205
{
206+
// If Required list is null then no properties are required in the new model,
207+
// so there are no breaking changes.
193208
if (Required == null)
194209
{
195210
return;
196211
}
197212

198-
if (Required != null && priorSchema.Required == null)
213+
var newRequiredNonReadOnlyPropNames = new List<string>();
214+
foreach (string requiredPropName in Required)
199215
{
200-
context.LogBreakingChange(ComparisonMessages.AddedRequiredProperty, String.Join(", ", Required));
201-
return;
216+
Properties.TryGetValue(requiredPropName, out Schema propSchema);
217+
priorSchema.Properties.TryGetValue(requiredPropName, out Schema priorPropSchema);
218+
bool propWasRequired = priorSchema.Required?.Contains(requiredPropName) == true;
219+
// Note that property is considered read-only only if it is consistently read-only both in the old and new models.
220+
bool propIsReadOnly = propSchema?.ReadOnly == true && priorPropSchema?.ReadOnly == true;
221+
if (!propWasRequired && !propIsReadOnly)
222+
{
223+
// Property is newly required and it is not read-only, hence it is a breaking change.
224+
newRequiredNonReadOnlyPropNames.Add(requiredPropName);
225+
}
226+
else
227+
{
228+
// Property was required and is required, or it is read-only, hence it is not a breaking change.
229+
}
202230
}
203231

204-
List<string> addedRequiredProperties = Required.Except(priorSchema.Required).ToList();
205-
if (addedRequiredProperties.Count > 0)
232+
if (newRequiredNonReadOnlyPropNames.Any())
206233
{
207-
context.LogBreakingChange(ComparisonMessages.AddedRequiredProperty, String.Join(", ", addedRequiredProperties));
234+
context.LogBreakingChange(ComparisonMessages.AddedRequiredProperty, string.Join(", ", newRequiredNonReadOnlyPropNames));
208235
}
209236
}
210237

‎package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@azure/oad",
3-
"version": "0.10.9",
3+
"version": "0.10.10",
44
"author": {
55
"name": "Microsoft Corporation",
66
"email": "[email protected]",

‎src/test/expandsAllOfFullCoversTest.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ test("expands allOf full covers", async () => {
105105
{
106106
id: "1034",
107107
code: "AddedRequiredProperty",
108-
message: "The new version has new required property 'a' that was not found in the old version.",
108+
message:
109+
"The new version lists new non-read-only properties as required: 'a'. These properties were not listed as required in the old version.",
109110
old: {
110111
ref: `${oldFilePath}#/paths/~1api~1Parameters/put/parameters/0/schema`,
111112
path: "paths./api/Parameters.put.parameters[0].schema",
@@ -141,7 +142,8 @@ test("expands allOf full covers", async () => {
141142
{
142143
id: "1034",
143144
code: "AddedRequiredProperty",
144-
message: "The new version has new required property 'a' that was not found in the old version.",
145+
message:
146+
"The new version lists new non-read-only properties as required: 'a'. These properties were not listed as required in the old version.",
145147
old: {
146148
ref: `${oldFilePath}#/definitions/Database`,
147149
path: "definitions.Database",

0 commit comments

Comments
 (0)