Skip to content

Commit

Permalink
Fix NRE in CompareRequired (#345)
Browse files Browse the repository at this point in the history
  • Loading branch information
Konrad Jamrozik authored Jul 23, 2024
1 parent 1bc9117 commit 8850294
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 5 deletions.
10 changes: 6 additions & 4 deletions openapi-diff/src/modeler/AutoRest.Swagger/Model/Schema.cs
Original file line number Diff line number Diff line change
Expand Up @@ -213,14 +213,16 @@ private void CompareRequired(ComparisonContext<ServiceDefinition> context, Schem
var newRequiredNonReadOnlyPropNames = new List<string>();
foreach (string requiredPropName in Required)
{
Properties.TryGetValue(requiredPropName, out Schema propSchema);
priorSchema.Properties.TryGetValue(requiredPropName, out Schema priorPropSchema);
bool propWasRequired = priorSchema.Required?.Contains(requiredPropName) == true;
Schema propSchema = null;

This comment has been minimized.

Copy link
@konrad-jamrozik

konrad-jamrozik Jul 24, 2024

@mikekistler what should happen if a property is marked as required but the spec has not defined this property (line 218), or the entire Properties block is missing (line 216)?

This comment has been minimized.

Copy link
@mikekistler

mikekistler Jul 24, 2024

Member

Technically it is allowed to list a property as required and not define it in the properties. In this case, treat the property as not readOnly for the purposes of this logic.

This comment has been minimized.

Copy link
@konrad-jamrozik

konrad-jamrozik Jul 25, 2024

OK so we should be good, as I believe this is what already happens, as this will evaluate to false:

bool propIsReadOnly = propSchema?.ReadOnly == true && priorPropSchema?.ReadOnly == true;

Schema priorPropSchema = null;
Properties?.TryGetValue(requiredPropName, out propSchema);
priorSchema?.Properties?.TryGetValue(requiredPropName, out priorPropSchema);
bool propWasRequired = priorSchema?.Required?.Contains(requiredPropName) == true;
// Note that property is considered read-only only if it is consistently read-only both in the old and new models.
bool propIsReadOnly = propSchema?.ReadOnly == true && priorPropSchema?.ReadOnly == true;
if (!propWasRequired && !propIsReadOnly)
{
// Property is newly required and it is not read-only, hence it is a breaking change.
// Property is newly required, and it is not read-only, hence it is a breaking change.
newRequiredNonReadOnlyPropNames.Add(requiredPropName);
}
else
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@azure/oad",
"version": "0.10.12",
"version": "0.10.13",
"author": {
"name": "Microsoft Corporation",
"email": "azsdkteam@microsoft.com",
Expand Down

0 comments on commit 8850294

Please sign in to comment.