Fixed issue with optional parameters when diffing

optional parameters being added are not breaking, this was reported in openapi-changes  https://github.com/pb33f/openapi-changes/issues/87

Signed-off-by: quobix <dave@quobix.com>

what-changed/model/operation.go

@@ -262,8 +262,15 @@ func CompareOperations(l, r any) *OperationChanges {
 				nil)
 		}
 		if lParamsUntyped.IsEmpty() && !rParamsUntyped.IsEmpty() {
+			rParams := rParamsUntyped.Value.([]low.ValueReference[*v2.Parameter])
+			breaking := false
+			for i := range rParams {
+				if rParams[i].Value.Required.Value {
+					breaking = true
+				}
+			}
 			CreateChange(&changes, PropertyAdded, v3.ParametersLabel,
-				nil, rParamsUntyped.ValueNode, true, nil,
+				nil, rParamsUntyped.ValueNode, breaking, nil,
 				rParamsUntyped.Value)
 		}
 
@@ -358,8 +365,15 @@ func CompareOperations(l, r any) *OperationChanges {
 				nil)
 		}
 		if lParamsUntyped.IsEmpty() && !rParamsUntyped.IsEmpty() {
+			rParams := rParamsUntyped.Value.([]low.ValueReference[*v3.Parameter])
+			breaking := false
+			for i := range rParams {
+				if rParams[i].Value.Required.Value {
+					breaking = true
+				}
+			}
 			CreateChange(&changes, PropertyAdded, v3.ParametersLabel,
-				nil, rParamsUntyped.ValueNode, true, nil,
+				nil, rParamsUntyped.ValueNode, breaking, nil,
 				rParamsUntyped.Value)
 		}
 

what-changed/model/operation_test.go

@@ -1588,3 +1588,90 @@ requestBody:
 	assert.Equal(t, 1, extChanges.TotalBreakingChanges())
 	assert.Equal(t, PropertyRemoved, extChanges.Changes[0].ChangeType)
 }
+
+func TestComparePathItem_V3_AddOptionalParam(t *testing.T) {
+
+	left := `operationId: listBurgerDressings`
+
+	right := `operationId: listBurgerDressings
+parameters:
+  - in: head
+    name: burgerId
+    required: false`
+
+	var lNode, rNode yaml.Node
+	_ = yaml.Unmarshal([]byte(left), &lNode)
+	_ = yaml.Unmarshal([]byte(right), &rNode)
+
+	// create low level objects
+	var lDoc v3.Operation
+	var rDoc v3.Operation
+	_ = low.BuildModel(lNode.Content[0], &lDoc)
+	_ = low.BuildModel(rNode.Content[0], &rDoc)
+	_ = lDoc.Build(context.Background(), nil, lNode.Content[0], nil)
+	_ = rDoc.Build(context.Background(), nil, rNode.Content[0], nil)
+
+	// compare.
+	extChanges := CompareOperations(&lDoc, &rDoc)
+	assert.Equal(t, 1, extChanges.TotalChanges())
+	assert.Len(t, extChanges.GetAllChanges(), 1)
+	assert.Equal(t, 0, extChanges.TotalBreakingChanges())
+}
+
+func TestComparePathItem_V2_AddOptionalParam(t *testing.T) {
+
+	left := `operationId: listBurgerDressings`
+
+	right := `operationId: listBurgerDressings
+parameters:
+  - in: head
+    name: burgerId
+    required: false`
+
+	var lNode, rNode yaml.Node
+	_ = yaml.Unmarshal([]byte(left), &lNode)
+	_ = yaml.Unmarshal([]byte(right), &rNode)
+
+	// create low level objects
+	var lDoc v2.Operation
+	var rDoc v2.Operation
+	_ = low.BuildModel(lNode.Content[0], &lDoc)
+	_ = low.BuildModel(rNode.Content[0], &rDoc)
+	_ = lDoc.Build(context.Background(), nil, lNode.Content[0], nil)
+	_ = rDoc.Build(context.Background(), nil, rNode.Content[0], nil)
+
+	// compare.
+	extChanges := CompareOperations(&lDoc, &rDoc)
+	assert.Equal(t, 1, extChanges.TotalChanges())
+	assert.Len(t, extChanges.GetAllChanges(), 1)
+	assert.Equal(t, 0, extChanges.TotalBreakingChanges())
+}
+
+func TestComparePathItem_V3_AddRequiredParam(t *testing.T) {
+
+	left := `operationId: listBurgerDressings`
+
+	right := `operationId: listBurgerDressings
+parameters:
+  - in: head
+    name: burgerId
+    required: true`
+
+	var lNode, rNode yaml.Node
+	_ = yaml.Unmarshal([]byte(left), &lNode)
+	_ = yaml.Unmarshal([]byte(right), &rNode)
+
+	// create low level objects
+	var lDoc v3.Operation
+	var rDoc v3.Operation
+	_ = low.BuildModel(lNode.Content[0], &lDoc)
+	_ = low.BuildModel(rNode.Content[0], &rDoc)
+	_ = lDoc.Build(context.Background(), nil, lNode.Content[0], nil)
+	_ = rDoc.Build(context.Background(), nil, rNode.Content[0], nil)
+
+	// compare.
+	extChanges := CompareOperations(&lDoc, &rDoc)
+	assert.Equal(t, 1, extChanges.TotalChanges())
+	assert.Len(t, extChanges.GetAllChanges(), 1)
+	assert.Equal(t, 1, extChanges.TotalBreakingChanges())
+}

what-changed/model/path_item.go

@@ -335,8 +335,16 @@ func compareSwaggerPathItem(lPath, rPath *v2.PathItem, changes *[]*Change, pc *P
 			nil)
 	}
 	if lPath.Parameters.IsEmpty() && !rPath.Parameters.IsEmpty() {
+		breaking := false
+		for i := range rPath.Parameters.Value {
+			param := rPath.Parameters.Value[i].Value
+			if param.Required.Value {
+				breaking = true
+				break
+			}
+		}
 		CreateChange(changes, PropertyAdded, v3.ParametersLabel,
-			nil, rPath.Parameters.ValueNode, true, nil,
+			nil, rPath.Parameters.ValueNode, breaking, nil,
 			rPath.Parameters.Value)
 	}
 
@@ -589,8 +597,16 @@ func compareOpenAPIPathItem(lPath, rPath *v3.PathItem, changes *[]*Change, pc *P
 			nil)
 	}
 	if lPath.Parameters.IsEmpty() && !rPath.Parameters.IsEmpty() {
+		breaking := false
+		for i := range rPath.Parameters.Value {
+			param := rPath.Parameters.Value[i].Value
+			if param.Required.Value {
+				breaking = true
+				break
+			}
+		}
 		CreateChange(changes, PropertyAdded, v3.ParametersLabel,
-			nil, rPath.Parameters.ValueNode, true, nil,
+			nil, rPath.Parameters.ValueNode, breaking, nil,
 			rPath.Parameters.Value)
 	}
 

what-changed/model/path_item_test.go

@@ -635,19 +635,71 @@ trace:
 	assert.Equal(t, 8, extChanges.TotalBreakingChanges())
 }
 
-func TestComparePathItem_V3_ChangeParam(t *testing.T) {
+func TestComparePathItem_V3_AddParam(t *testing.T) {
 
-	left := `get:
+	left := `summary: hello`
-  operationId: listBurgerDressings
-  parameters:
-    - in: query
-      name: burgerId`
 
-	right := `get:
+	right := `summary: hello
-  operationId: listBurgerDressings
+parameters:
-  parameters:
+  - in: head
-    - in: head
+    name: burgerId`
-      name: burgerId`
+
+	var lNode, rNode yaml.Node
+	_ = yaml.Unmarshal([]byte(left), &lNode)
+	_ = yaml.Unmarshal([]byte(right), &rNode)
+
+	// create low level objects
+	var lDoc v3.PathItem
+	var rDoc v3.PathItem
+	_ = low.BuildModel(lNode.Content[0], &lDoc)
+	_ = low.BuildModel(rNode.Content[0], &rDoc)
+	_ = lDoc.Build(context.Background(), nil, lNode.Content[0], nil)
+	_ = rDoc.Build(context.Background(), nil, rNode.Content[0], nil)
+
+	// compare.
+	extChanges := ComparePathItems(&lDoc, &rDoc)
+	assert.Equal(t, 1, extChanges.TotalChanges())
+	assert.Len(t, extChanges.GetAllChanges(), 1)
+	assert.Equal(t, 0, extChanges.TotalBreakingChanges())
+}
+
+func TestComparePathItem_V2_AddParamOptional(t *testing.T) {
+
+	left := `summary: hello`
+
+	right := `summary: hello
+parameters:
+  - in: head
+    name: burgerId`
+
+	var lNode, rNode yaml.Node
+	_ = yaml.Unmarshal([]byte(left), &lNode)
+	_ = yaml.Unmarshal([]byte(right), &rNode)
+
+	// create low level objects
+	var lDoc v2.PathItem
+	var rDoc v2.PathItem
+	_ = low.BuildModel(lNode.Content[0], &lDoc)
+	_ = low.BuildModel(rNode.Content[0], &rDoc)
+	_ = lDoc.Build(context.Background(), nil, lNode.Content[0], nil)
+	_ = rDoc.Build(context.Background(), nil, rNode.Content[0], nil)
+
+	// compare.
+	extChanges := ComparePathItems(&lDoc, &rDoc)
+	assert.Equal(t, 1, extChanges.TotalChanges())
+	assert.Len(t, extChanges.GetAllChanges(), 1)
+	assert.Equal(t, 0, extChanges.TotalBreakingChanges())
+}
+
+func TestComparePathItem_V3_AddParamRequired(t *testing.T) {
+
+	left := `summary: hello`
+
+	right := `summary: hello
+parameters:
+  - in: head
+    name: burgerId
+    required: true`
 
 	var lNode, rNode yaml.Node
 	_ = yaml.Unmarshal([]byte(left), &lNode)
@@ -667,3 +719,32 @@ func TestComparePathItem_V3_ChangeParam(t *testing.T) {
 	assert.Len(t, extChanges.GetAllChanges(), 1)
 	assert.Equal(t, 1, extChanges.TotalBreakingChanges())
 }
+
+func TestComparePathItem_V2_AddParamRequired(t *testing.T) {
+
+	left := `summary: hello`
+
+	right := `summary: hello
+parameters:
+  - in: head
+    name: burgerId
+    required: true`
+
+	var lNode, rNode yaml.Node
+	_ = yaml.Unmarshal([]byte(left), &lNode)
+	_ = yaml.Unmarshal([]byte(right), &rNode)
+
+	// create low level objects
+	var lDoc v2.PathItem
+	var rDoc v2.PathItem
+	_ = low.BuildModel(lNode.Content[0], &lDoc)
+	_ = low.BuildModel(rNode.Content[0], &rDoc)
+	_ = lDoc.Build(context.Background(), nil, lNode.Content[0], nil)
+	_ = rDoc.Build(context.Background(), nil, rNode.Content[0], nil)
+
+	// compare.
+	extChanges := ComparePathItems(&lDoc, &rDoc)
+	assert.Equal(t, 1, extChanges.TotalChanges())
+	assert.Len(t, extChanges.GetAllChanges(), 1)
+	assert.Equal(t, 1, extChanges.TotalBreakingChanges())
+}