diff --git a/datamodel/spec_info.go b/datamodel/spec_info.go index 64efe0c2..1c958a0e 100644 --- a/datamodel/spec_info.go +++ b/datamodel/spec_info.go @@ -249,7 +249,6 @@ func ExtractSpecInfoWithDocumentCheck(spec []byte, bypass bool) (*SpecInfo, erro if err := parseJSON(spec, specInfo, &parsedSpec); err != nil { return nil, err } - parsed = true specInfo.Error = errors.New("spec type not supported by libopenapi, sorry") return specInfo, specInfo.Error } diff --git a/index/resolver.go b/index/resolver.go index a8df3ef4..944b63ff 100644 --- a/index/resolver.go +++ b/index/resolver.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "net/url" + "path" "path/filepath" "slices" "strings" @@ -545,7 +546,7 @@ func (resolver *Resolver) extractRelatives(ref *Reference, node, parent *yaml.No httpExp := strings.Split(ref.FullDefinition, "#/") u, _ := url.Parse(httpExp[0]) - abs, _ := filepath.Abs(utils.CheckPathOverlap(filepath.Dir(u.Path), exp[0], string(filepath.Separator))) + abs, _ := filepath.Abs(utils.CheckPathOverlap(path.Dir(u.Path), exp[0], string(filepath.Separator))) u.Path = utils.ReplaceWindowsDriveWithLinuxPath(abs) u.Fragment = "" fullDef = fmt.Sprintf("%s#/%s", u.String(), exp[1]) @@ -596,8 +597,8 @@ func (resolver *Resolver) extractRelatives(ref *Reference, node, parent *yaml.No // is the file def a http link? if strings.HasPrefix(fileDef[0], "http") { u, _ := url.Parse(fileDef[0]) - path, _ := filepath.Abs(utils.CheckPathOverlap(filepath.Dir(u.Path), exp[0], string(filepath.Separator))) - u.Path = utils.ReplaceWindowsDriveWithLinuxPath(path) + absPath, _ := filepath.Abs(utils.CheckPathOverlap(path.Dir(u.Path), exp[0], string(filepath.Separator))) + u.Path = utils.ReplaceWindowsDriveWithLinuxPath(absPath) fullDef = u.String() } else { @@ -830,7 +831,7 @@ func (resolver *Resolver) buildDefPath(ref *Reference, l string) string { if strings.HasPrefix(ref.FullDefinition, "http") { u, _ := url.Parse(ref.FullDefinition) - p, _ := filepath.Abs(utils.CheckPathOverlap(filepath.Dir(u.Path), exp[0], string(filepath.Separator))) + p, _ := filepath.Abs(utils.CheckPathOverlap(path.Dir(u.Path), exp[0], string(filepath.Separator))) u.Path = utils.ReplaceWindowsDriveWithLinuxPath(p) def = fmt.Sprintf("%s#/%s", u.String(), exp[1]) @@ -883,7 +884,7 @@ func (resolver *Resolver) buildDefPath(ref *Reference, l string) string { // split the url. u, _ := url.Parse(ref.FullDefinition) - abs, _ := filepath.Abs(utils.CheckPathOverlap(filepath.Dir(u.Path), l, string(filepath.Separator))) + abs, _ := filepath.Abs(utils.CheckPathOverlap(path.Dir(u.Path), l, string(filepath.Separator))) u.Path = utils.ReplaceWindowsDriveWithLinuxPath(abs) u.Fragment = "" def = u.String() diff --git a/index/search_index.go b/index/search_index.go index 3ecd979d..415c135f 100644 --- a/index/search_index.go +++ b/index/search_index.go @@ -246,6 +246,15 @@ func (index *SpecIndex) SearchIndexForReferenceByReferenceWithContext(ctx contex } if index.logger != nil { + // this is a last ditch effort. if this fails, all hope is lost. + if index.GetRolodex() != nil { + for _, i := range index.GetRolodex().GetIndexes() { + v := i.FindComponent(ctx, ref) + if v != nil { + return v, v.Index, ctx + } + } + } index.logger.Error("unable to locate reference anywhere in the rolodex", "reference", ref) } return nil, index, ctx diff --git a/index/search_index_test.go b/index/search_index_test.go index 6b2a512b..98fe507f 100644 --- a/index/search_index_test.go +++ b/index/search_index_test.go @@ -40,3 +40,55 @@ func TestSpecIndex_SearchIndexForReferenceWithContext(t *testing.T) { assert.Nil(t, idx.GetRootNode()) } + +// TestSearchIndexForReference_LastDitchRolodexFallback tests the last-ditch effort +// code path where a reference is found by iterating through rolodex indexes +// after all other lookup methods fail. +func TestSearchIndexForReference_LastDitchRolodexFallback(t *testing.T) { + // Primary index with NO components - searches will fail here + primarySpec := `openapi: 3.0.1 +info: + title: Primary + version: "1.0"` + + var primaryRoot yaml.Node + _ = yaml.Unmarshal([]byte(primarySpec), &primaryRoot) + + c := CreateOpenAPIIndexConfig() + primaryIdx := NewSpecIndexWithConfig(&primaryRoot, c) + + // Secondary index WITH the component we want to find + secondarySpec := `openapi: 3.0.1 +info: + title: Secondary + version: "1.0" +components: + schemas: + Pet: + type: object + properties: + name: + type: string` + + var secondaryRoot yaml.Node + _ = yaml.Unmarshal([]byte(secondarySpec), &secondaryRoot) + + secondaryIdx := NewSpecIndexWithConfig(&secondaryRoot, c) + + // Create rolodex and add secondary index + rolo := NewRolodex(c) + rolo.AddIndex(secondaryIdx) + + // Set rolodex on primary index + primaryIdx.SetRolodex(rolo) + + // Search for reference that: + // 1. Doesn't exist in primary index's allMappedRefs + // 2. Has roloLookup = "" (simple ref format) + // 3. Should be found via last-ditch rolodex iteration + ref, idx := primaryIdx.SearchIndexForReference("#/components/schemas/Pet") + + assert.NotNil(t, ref, "Reference should be found via rolodex fallback") + assert.NotNil(t, idx, "Index should be returned") + assert.Equal(t, "Pet", ref.Name) +} diff --git a/what-changed/model/change_types.go b/what-changed/model/change_types.go index b0e6e8f1..d2cbb98b 100644 --- a/what-changed/model/change_types.go +++ b/what-changed/model/change_types.go @@ -89,6 +89,14 @@ type Change struct { // New is the new value represented as a string. New string `json:"new,omitempty" yaml:"new,omitempty"` + // OriginalEncoded is the original value serialized to YAML (for complex types like extensions). + // Only populated for specific use cases (e.g., extension values that are objects/arrays). + OriginalEncoded string `json:"originalEncoded,omitempty" yaml:"originalEncoded,omitempty"` + + // NewEncoded is the new value serialized to YAML (for complex types like extensions). + // Only populated for specific use cases (e.g., extension values that are objects/arrays). + NewEncoded string `json:"newEncoded,omitempty" yaml:"newEncoded,omitempty"` + // Breaking determines if the change is a breaking one or not. Breaking bool `json:"breaking" yaml:"breaking"` @@ -138,6 +146,14 @@ func (c *Change) MarshalJSON() ([]byte, error) { data["new"] = c.New } + if c.OriginalEncoded != "" { + data["originalEncoded"] = c.OriginalEncoded + } + + if c.NewEncoded != "" { + data["newEncoded"] = c.NewEncoded + } + if c.Context != nil { data["context"] = c.Context } diff --git a/what-changed/model/change_types_test.go b/what-changed/model/change_types_test.go index 92949bd1..40d88135 100644 --- a/what-changed/model/change_types_test.go +++ b/what-changed/model/change_types_test.go @@ -89,6 +89,29 @@ func TestChange_MarshalJSON(t *testing.T) { rebuilt = rinseAndRepeat(&change) assert.Equal(t, "difficult", rebuilt["path"]) + // Test OriginalEncoded field + change = Change{ + OriginalEncoded: "key: value\n", + } + rebuilt = rinseAndRepeat(&change) + assert.Equal(t, "key: value\n", rebuilt["originalEncoded"]) + + // Test NewEncoded field + change = Change{ + NewEncoded: "items:\n - one\n - two\n", + } + rebuilt = rinseAndRepeat(&change) + assert.Equal(t, "items:\n - one\n - two\n", rebuilt["newEncoded"]) + + // Test both encoded fields together + change = Change{ + OriginalEncoded: "old: data", + NewEncoded: "new: data", + } + rebuilt = rinseAndRepeat(&change) + assert.Equal(t, "old: data", rebuilt["originalEncoded"]) + assert.Equal(t, "new: data", rebuilt["newEncoded"]) + prop := &PropertyChanges{Changes: []*Change{&change}} assert.Len(t, prop.GetPropertyChanges(), 1) } diff --git a/what-changed/model/comparison_functions.go b/what-changed/model/comparison_functions.go index fb859975..ed757cbc 100644 --- a/what-changed/model/comparison_functions.go +++ b/what-changed/model/comparison_functions.go @@ -35,6 +35,16 @@ func SetReferenceIfExists[T any](value *low.ValueReference[T], changeObj any) { } } +// PreserveParameterReference checks if a parameter is a reference and preserves it on the changes object. +// This eliminates duplicate reference preservation logic in operation.go and path_item.go. +func PreserveParameterReference[T any](lRefs, rRefs map[string]*low.ValueReference[T], name string, changes ChangeIsReferenced) { + if lRef := lRefs[name]; lRef != nil && lRef.IsReference() { + SetReferenceIfExists(lRef, changes) + } else if rRef := rRefs[name]; rRef != nil && rRef.IsReference() { + SetReferenceIfExists(rRef, changes) + } +} + func checkLocation(ctx *ChangeContext, hs base.HasIndex) bool { if !reflect.ValueOf(hs).IsNil() { idx := hs.GetIndex() @@ -87,6 +97,20 @@ func CreateChange(changes *[]*Change, changeType int, property string, leftValue if rightValueNode != nil && rightValueNode.Value != EMPTY_STR { c.New = rightValueNode.Value } + + // If node is nil but object is a string, use the object value as fallback + // This handles cases where the value is provided as the object parameter (e.g., security requirements) + if leftValueNode == nil && c.Original == "" { + if str, ok := originalObject.(string); ok { + c.Original = str + } + } + if rightValueNode == nil && c.New == "" { + if str, ok := newObject.(string); ok { + c.New = str + } + } + // original and new objects c.OriginalObject = originalObject c.NewObject = newObject @@ -98,6 +122,31 @@ func CreateChange(changes *[]*Change, changeType int, property string, leftValue return changes } +// CreateChangeWithEncoding is like CreateChange but also populates the encoded fields for complex values. +// use this ONLY for extensions or other cases where complex YAML structures need to be serialized. +// the encoded values are serialized to YAML format. +func CreateChangeWithEncoding(changes *[]*Change, changeType int, property string, leftValueNode, rightValueNode *yaml.Node, + breaking bool, originalObject, newObject any, +) *[]*Change { + CreateChange(changes, changeType, property, leftValueNode, rightValueNode, breaking, originalObject, newObject) + + c := (*changes)[len(*changes)-1] + + // serialize complex values to YAML for extension rendering (avoid inflating memory for scalar values) + if leftValueNode != nil && (utils.IsNodeArray(leftValueNode) || utils.IsNodeMap(leftValueNode)) { + if encoded, err := yaml.Marshal(leftValueNode); err == nil { + c.OriginalEncoded = string(encoded) + } + } + if rightValueNode != nil && (utils.IsNodeArray(rightValueNode) || utils.IsNodeMap(rightValueNode)) { + if encoded, err := yaml.Marshal(rightValueNode); err == nil { + c.NewEncoded = string(encoded) + } + } + + return changes +} + // CreateContext will return a pointer to a ChangeContext containing the original and new line and column numbers // of the left and right value nodes. func CreateContext(l, r *yaml.Node) *ChangeContext { @@ -135,29 +184,48 @@ func CountBreakingChanges(changes []*Change) int { return b } -// CheckForObjectAdditionOrRemoval will check for the addition or removal of an object from left and right maps. -// The label is the key to look for in the left and right maps. -// -// To determine this a breaking change for an addition then set breakingAdd to true (however I can't think of many -// scenarios that adding things should break anything). Removals are generally breaking, except for non contract -// properties like descriptions, summaries and other non-binding values, so a breakingRemove value can be tuned for -// these circumstances. -func CheckForObjectAdditionOrRemoval[T any](l, r map[string]*low.ValueReference[T], label string, changes *[]*Change, - breakingAdd, breakingRemove bool, +// checkForObjectAdditionOrRemovalInternal is the internal implementation that handles both encoding modes. +func checkForObjectAdditionOrRemovalInternal[T any](l, r map[string]*low.ValueReference[T], label string, changes *[]*Change, + breakingAdd, breakingRemove bool, withEncoding bool, ) { + createFn := CreateChange + if withEncoding { + createFn = CreateChangeWithEncoding + } var left, right T if CheckSpecificObjectRemoved(l, r, label) { left = l[label].GetValue() - CreateChange(changes, ObjectRemoved, label, l[label].GetValueNode(), nil, + createFn(changes, ObjectRemoved, label, l[label].GetValueNode(), nil, breakingRemove, left, nil) } if CheckSpecificObjectAdded(l, r, label) { right = r[label].GetValue() - CreateChange(changes, ObjectAdded, label, nil, r[label].GetValueNode(), + createFn(changes, ObjectAdded, label, nil, r[label].GetValueNode(), breakingAdd, nil, right) } } +// CheckForObjectAdditionOrRemoval will check for the addition or removal of an object from left and right maps. +// The label is the key to look for in the left and right maps. +// +// To determine this a breaking change for an addition then set breakingAdd to true (however I can't think of many +// scenarios that adding things should break anything). Removals are generally breaking, except for non contract +// properties like descriptions, summaries and other non-binding values, so a breakingRemove value can be tuned for +// these circumstances. +func CheckForObjectAdditionOrRemoval[T any](l, r map[string]*low.ValueReference[T], label string, changes *[]*Change, + breakingAdd, breakingRemove bool, +) { + checkForObjectAdditionOrRemovalInternal(l, r, label, changes, breakingAdd, breakingRemove, false) +} + +// CheckForObjectAdditionOrRemovalWithEncoding is like CheckForObjectAdditionOrRemoval but populates encoded fields. +// Use this for extensions where complex values need to be serialized to YAML. +func CheckForObjectAdditionOrRemovalWithEncoding[T any](l, r map[string]*low.ValueReference[T], label string, changes *[]*Change, + breakingAdd, breakingRemove bool, +) { + checkForObjectAdditionOrRemovalInternal(l, r, label, changes, breakingAdd, breakingRemove, true) +} + // CheckSpecificObjectRemoved returns true if a specific value is not in both maps. func CheckSpecificObjectRemoved[T any](l, r map[string]*T, label string) bool { return l[label] != nil && r[label] == nil @@ -181,6 +249,38 @@ func CheckProperties(properties []*PropertyCheck) { } } +// CheckPropertiesWithEncoding is like CheckProperties but uses CreateChangeWithEncoding for complex values. +// use this for extensions where YAML serialization is needed. +func CheckPropertiesWithEncoding(properties []*PropertyCheck) { + for _, n := range properties { + CheckPropertyAdditionOrRemovalWithEncoding(n.LeftNode, n.RightNode, n.Label, n.Changes, n.Breaking, n.Original, n.New) + CheckForModificationWithEncoding(n.LeftNode, n.RightNode, n.Label, n.Changes, n.Breaking, n.Original, n.New) + } +} + +// CheckPropertyAdditionOrRemovalWithEncoding checks for additions and removals with encoding. +func CheckPropertyAdditionOrRemovalWithEncoding[T any](l, r *yaml.Node, + label string, changes *[]*Change, breaking bool, orig, new T, +) { + checkForRemovalInternal(l, r, label, changes, breaking, orig, new, true) + checkForAdditionInternal(l, r, label, changes, breaking, orig, new, true) +} + +// CheckForRemovalWithEncoding checks for removals with YAML encoding. +func CheckForRemovalWithEncoding[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T) { + checkForRemovalInternal(l, r, label, changes, breaking, orig, new, true) +} + +// CheckForAdditionWithEncoding checks for additions with YAML encoding. +func CheckForAdditionWithEncoding[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T) { + checkForAdditionInternal(l, r, label, changes, breaking, orig, new, true) +} + +// CheckForModificationWithEncoding checks for modifications with YAML encoding. +func CheckForModificationWithEncoding[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T) { + checkForModificationInternal(l, r, label, changes, breaking, orig, new, true) +} + // CheckPropertyAdditionOrRemoval will run both CheckForRemoval (first) and CheckForAddition (second) func CheckPropertyAdditionOrRemoval[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T, @@ -189,6 +289,21 @@ func CheckPropertyAdditionOrRemoval[T any](l, r *yaml.Node, CheckForAddition[T](l, r, label, changes, breaking, orig, new) } +// checkForRemovalInternal is the internal implementation for removal checks with configurable encoding. +func checkForRemovalInternal[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T, withEncoding bool) { + createFn := CreateChange + if withEncoding { + createFn = CreateChangeWithEncoding + } + if l != nil && l.Value != EMPTY_STR && (r == nil || r.Value == EMPTY_STR && !utils.IsNodeArray(r) && !utils.IsNodeMap(r)) { + createFn(changes, PropertyRemoved, label, l, r, breaking, orig, new) + return + } + if l != nil && r == nil { + createFn(changes, PropertyRemoved, label, l, nil, breaking, orig, nil) + } +} + // CheckForRemoval will check left and right yaml.Node instances for changes. Anything that is found missing on the // right, but present on the left, is considered a removal. A new Change[T] will be created with the type // @@ -196,82 +311,94 @@ func CheckPropertyAdditionOrRemoval[T any](l, r *yaml.Node, // // The Change is then added to the slice of []Change[T] instances provided as a pointer. func CheckForRemoval[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T) { - if l != nil && l.Value != EMPTY_STR && (r == nil || r.Value == EMPTY_STR && !utils.IsNodeArray(r) && !utils.IsNodeMap(r)) { - CreateChange(changes, PropertyRemoved, label, l, r, breaking, orig, new) - return + checkForRemovalInternal(l, r, label, changes, breaking, orig, new, false) +} + +// checkForAdditionInternal is the internal implementation for addition checks with configurable encoding. +func checkForAdditionInternal[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T, withEncoding bool) { + createFn := CreateChange + if withEncoding { + createFn = CreateChangeWithEncoding } - if l != nil && r == nil { - CreateChange(changes, PropertyRemoved, label, l, nil, breaking, orig, nil) + // left doesn't exist if: nil OR (empty scalar AND not a map/array) OR (empty map/array) + leftDoesNotExist := l == nil || + (l.Value == EMPTY_STR && !utils.IsNodeMap(l) && !utils.IsNodeArray(l)) || + ((utils.IsNodeMap(l) || utils.IsNodeArray(l)) && len(l.Content) == 0) + // right exists if: not nil AND (has value OR is array OR is map) + rightExists := r != nil && (r.Value != EMPTY_STR || utils.IsNodeArray(r) || utils.IsNodeMap(r)) + + if leftDoesNotExist && rightExists { + createFn(changes, PropertyAdded, label, l, r, breaking, orig, new) } } // CheckForAddition will check left and right yaml.Node instances for changes. Anything that is found missing on the -// left, but present on the left, is considered an addition. A new Change[T] will be created with the type +// left, but present on the right, is considered an addition. A new Change[T] will be created with the type // // PropertyAdded // // The Change is then added to the slice of []Change[T] instances provided as a pointer. func CheckForAddition[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T) { - if (l == nil || l.Value == EMPTY_STR) && (r != nil && (r.Value != EMPTY_STR || utils.IsNodeArray(r)) || utils.IsNodeMap(r)) { - if r != nil { - if l != nil && (len(l.Content) < len(r.Content)) && len(l.Content) <= 0 { - CreateChange(changes, PropertyAdded, label, l, r, breaking, orig, new) - } - if l == nil { - CreateChange(changes, PropertyAdded, label, l, r, breaking, orig, new) - } - } - } + checkForAdditionInternal(l, r, label, changes, breaking, orig, new, false) } -// CheckForModification will check left and right yaml.Node instances for changes. Anything that is found in both -// sides, but vary in value is considered a modification. -// -// If there is a change in value the function adds a change type of Modified. -// -// The Change is then added to the slice of []Change[T] instances provided as a pointer. -func CheckForModification[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T) { +// checkForModificationInternal is the internal implementation for modification checks with configurable encoding. +func checkForModificationInternal[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T, withEncoding bool) { + createFn := CreateChange + if withEncoding { + createFn = CreateChangeWithEncoding + } if l != nil && l.Value != EMPTY_STR && r != nil && r.Value != EMPTY_STR && (r.Value != l.Value || r.Tag != l.Tag) { - CreateChange(changes, Modified, label, l, r, breaking, orig, new) + createFn(changes, Modified, label, l, r, breaking, orig, new) return } if l != nil && utils.IsNodeArray(l) && r != nil && !utils.IsNodeArray(r) { - CreateChange(changes, Modified, label, l, r, breaking, orig, new) + createFn(changes, Modified, label, l, r, breaking, orig, new) return } if l != nil && !utils.IsNodeArray(l) && r != nil && utils.IsNodeArray(r) { - CreateChange(changes, Modified, label, l, r, breaking, orig, new) + createFn(changes, Modified, label, l, r, breaking, orig, new) return } if l != nil && utils.IsNodeMap(l) && r != nil && !utils.IsNodeMap(r) { - CreateChange(changes, Modified, label, l, r, breaking, orig, new) + createFn(changes, Modified, label, l, r, breaking, orig, new) return } if l != nil && !utils.IsNodeMap(l) && r != nil && utils.IsNodeMap(r) { - CreateChange(changes, Modified, label, l, r, breaking, orig, new) + createFn(changes, Modified, label, l, r, breaking, orig, new) return } if l != nil && utils.IsNodeArray(l) && r != nil && utils.IsNodeArray(r) { if len(l.Content) != len(r.Content) { - CreateChange(changes, Modified, label, l, r, breaking, orig, new) + createFn(changes, Modified, label, l, r, breaking, orig, new) return } // Compare the YAML node trees directly without marshaling if !low.CompareYAMLNodes(l, r) { - CreateChange(changes, Modified, label, l, r, breaking, orig, new) + createFn(changes, Modified, label, l, r, breaking, orig, new) } return } if l != nil && utils.IsNodeMap(l) && r != nil && utils.IsNodeMap(r) { // Compare the YAML node trees directly without marshaling if !low.CompareYAMLNodes(l, r) { - CreateChange(changes, Modified, label, l, r, breaking, orig, new) + createFn(changes, Modified, label, l, r, breaking, orig, new) } return } } +// CheckForModification will check left and right yaml.Node instances for changes. Anything that is found in both +// sides, but vary in value is considered a modification. +// +// If there is a change in value the function adds a change type of Modified. +// +// The Change is then added to the slice of []Change[T] instances provided as a pointer. +func CheckForModification[T any](l, r *yaml.Node, label string, changes *[]*Change, breaking bool, orig, new T) { + checkForModificationInternal(l, r, label, changes, breaking, orig, new, false) +} + // CheckMapForChanges checks a left and right low level map for any additions, subtractions or modifications to // values. The compareFunc argument should reference the correct comparison function for the generic type. func CheckMapForChanges[T any, R any](expLeft, expRight *orderedmap.Map[low.KeyReference[string], low.ValueReference[T]], diff --git a/what-changed/model/comparison_functions_test.go b/what-changed/model/comparison_functions_test.go index 74593be6..74b4d8ab 100644 --- a/what-changed/model/comparison_functions_test.go +++ b/what-changed/model/comparison_functions_test.go @@ -427,3 +427,189 @@ func TestSetReferenceIfExists_Integration(t *testing.T) { assert.Equal(t, "#/components/parameters/TestParam", changes.GetChangeReference()) }) } + +// TestPreserveParameterReference tests the PreserveParameterReference helper function +func TestPreserveParameterReference(t *testing.T) { + t.Run("preserves left reference when left has ref", func(t *testing.T) { + lRef := low.ValueReference[string]{Value: "left"} + lRef.SetReference("#/components/parameters/LeftParam", nil) + rRef := low.ValueReference[string]{Value: "right"} + + lRefs := map[string]*low.ValueReference[string]{"param": &lRef} + rRefs := map[string]*low.ValueReference[string]{"param": &rRef} + + changes := &ParameterChanges{PropertyChanges: &PropertyChanges{}} + PreserveParameterReference(lRefs, rRefs, "param", changes) + + assert.Equal(t, "#/components/parameters/LeftParam", changes.GetChangeReference()) + }) + + t.Run("preserves right reference when only right has ref", func(t *testing.T) { + lRef := low.ValueReference[string]{Value: "left"} + rRef := low.ValueReference[string]{Value: "right"} + rRef.SetReference("#/components/parameters/RightParam", nil) + + lRefs := map[string]*low.ValueReference[string]{"param": &lRef} + rRefs := map[string]*low.ValueReference[string]{"param": &rRef} + + changes := &ParameterChanges{PropertyChanges: &PropertyChanges{}} + PreserveParameterReference(lRefs, rRefs, "param", changes) + + assert.Equal(t, "#/components/parameters/RightParam", changes.GetChangeReference()) + }) + + t.Run("handles missing parameter gracefully", func(t *testing.T) { + lRefs := map[string]*low.ValueReference[string]{} + rRefs := map[string]*low.ValueReference[string]{} + + changes := &ParameterChanges{PropertyChanges: &PropertyChanges{}} + PreserveParameterReference(lRefs, rRefs, "missing", changes) + + assert.Equal(t, "", changes.GetChangeReference()) + }) + + t.Run("prefers left reference over right", func(t *testing.T) { + lRef := low.ValueReference[string]{Value: "left"} + lRef.SetReference("#/components/parameters/LeftParam", nil) + rRef := low.ValueReference[string]{Value: "right"} + rRef.SetReference("#/components/parameters/RightParam", nil) + + lRefs := map[string]*low.ValueReference[string]{"param": &lRef} + rRefs := map[string]*low.ValueReference[string]{"param": &rRef} + + changes := &ParameterChanges{PropertyChanges: &PropertyChanges{}} + PreserveParameterReference(lRefs, rRefs, "param", changes) + + // Left takes priority + assert.Equal(t, "#/components/parameters/LeftParam", changes.GetChangeReference()) + }) +} + +// TestCheckForRemovalWithEncoding tests the removal check with encoding +func TestCheckForRemovalWithEncoding(t *testing.T) { + t.Run("detects removal with encoding", func(t *testing.T) { + var lNode yaml.Node + _ = yaml.Unmarshal([]byte(`key: value`), &lNode) + + changes := []*Change{} + CheckForRemovalWithEncoding(lNode.Content[0], nil, "test", &changes, false, "old", "new") + + assert.Len(t, changes, 1) + assert.Equal(t, PropertyRemoved, changes[0].ChangeType) + }) + + t.Run("no change when both nodes present", func(t *testing.T) { + var lNode, rNode yaml.Node + _ = yaml.Unmarshal([]byte(`value`), &lNode) + _ = yaml.Unmarshal([]byte(`value`), &rNode) + + changes := []*Change{} + CheckForRemovalWithEncoding(lNode.Content[0], rNode.Content[0], "test", &changes, false, "old", "new") + + assert.Empty(t, changes) + }) +} + +// TestCheckForAdditionWithEncoding tests the addition check with encoding +func TestCheckForAdditionWithEncoding(t *testing.T) { + t.Run("detects addition with encoding", func(t *testing.T) { + var rNode yaml.Node + _ = yaml.Unmarshal([]byte(`newvalue`), &rNode) + + changes := []*Change{} + CheckForAdditionWithEncoding[string](nil, rNode.Content[0], "test", &changes, false, "", "new") + + assert.Len(t, changes, 1) + assert.Equal(t, PropertyAdded, changes[0].ChangeType) + }) + + t.Run("detects array addition with encoding", func(t *testing.T) { + var rNode yaml.Node + _ = yaml.Unmarshal([]byte(`[item1, item2]`), &rNode) + + changes := []*Change{} + CheckForAdditionWithEncoding[string](nil, rNode.Content[0], "test", &changes, false, "", "new") + + assert.Len(t, changes, 1) + assert.Equal(t, PropertyAdded, changes[0].ChangeType) + assert.NotEmpty(t, changes[0].NewEncoded) + }) +} + +// TestCheckForModificationWithEncoding tests the modification check with encoding +func TestCheckForModificationWithEncoding(t *testing.T) { + tests := []struct { + name string + left any + right any + differ bool + }{ + {"scalar to different scalar", "value", "changed", true}, + {"array to non-array", []string{"a"}, "scalar", true}, + {"non-array to array", "scalar", []string{"a"}, true}, + {"map to non-map", map[string]string{"k": "v"}, "scalar", true}, + {"non-map to map", "scalar", map[string]string{"k": "v"}, true}, + {"same array", []string{"a"}, []string{"a"}, false}, + {"different array content", []string{"a"}, []string{"b"}, true}, + {"different array length", []string{"a"}, []string{"a", "b"}, true}, + {"same map", map[string]string{"k": "v"}, map[string]string{"k": "v"}, false}, + {"different map", map[string]string{"k": "v"}, map[string]string{"k": "x"}, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var lNode, rNode yaml.Node + encL, _ := yaml.Marshal(tt.left) + encR, _ := yaml.Marshal(tt.right) + _ = yaml.Unmarshal(encL, &lNode) + _ = yaml.Unmarshal(encR, &rNode) + + changes := []*Change{} + CheckForModificationWithEncoding(lNode.Content[0], rNode.Content[0], tt.name, &changes, false, "old", "new") + + if tt.differ { + assert.Len(t, changes, 1, "expected 1 change for %s", tt.name) + } else { + assert.Empty(t, changes, "expected no changes for %s", tt.name) + } + }) + } +} + +// TestCreateChangeWithEncoding_ComplexValues tests encoding behavior +func TestCreateChangeWithEncoding_ComplexValues(t *testing.T) { + t.Run("encodes map values", func(t *testing.T) { + var lNode yaml.Node + _ = yaml.Unmarshal([]byte(`{key: value, nested: {a: b}}`), &lNode) + + changes := []*Change{} + CreateChangeWithEncoding(&changes, Modified, "test", lNode.Content[0], nil, false, nil, nil) + + assert.Len(t, changes, 1) + assert.NotEmpty(t, changes[0].OriginalEncoded) + }) + + t.Run("encodes array values", func(t *testing.T) { + var rNode yaml.Node + _ = yaml.Unmarshal([]byte(`[item1, item2, item3]`), &rNode) + + changes := []*Change{} + CreateChangeWithEncoding(&changes, Modified, "test", nil, rNode.Content[0], false, nil, nil) + + assert.Len(t, changes, 1) + assert.NotEmpty(t, changes[0].NewEncoded) + }) + + t.Run("does not encode scalar values", func(t *testing.T) { + var lNode, rNode yaml.Node + _ = yaml.Unmarshal([]byte(`scalar`), &lNode) + _ = yaml.Unmarshal([]byte(`another`), &rNode) + + changes := []*Change{} + CreateChangeWithEncoding(&changes, Modified, "test", lNode.Content[0], rNode.Content[0], false, nil, nil) + + assert.Len(t, changes, 1) + assert.Empty(t, changes[0].OriginalEncoded) + assert.Empty(t, changes[0].NewEncoded) + }) +} diff --git a/what-changed/model/example.go b/what-changed/model/example.go index c0933a22..12ed9d96 100644 --- a/what-changed/model/example.go +++ b/what-changed/model/example.go @@ -121,63 +121,20 @@ func CompareExamples(l, r *base.Example) *ExampleChanges { } sort.Strings(lKeys) sort.Strings(rKeys) - //if (len(lKeys) > len(rKeys)) || (len(rKeys) > len(lKeys)) { - // CreateChange(&changes, Modified, v3.ValueLabel, - // l.Value.GetValueNode(), r.Value.GetValueNode(), false, l.Value.GetValue(), r.Value.GetValue()) - //} for k := range lKeys { if k < len(rKeys) && lKeys[k] != rKeys[k] { - - if utils.IsNodeMap(l.Value.ValueNode) || utils.IsNodeArray(l.Value.ValueNode) { - // render down object - rendered, _ := low.YAMLNodeToBytes(l.Value.ValueNode) - l.Value.ValueNode.Value = string(rendered) - } - - if utils.IsNodeMap(r.Value.ValueNode) || utils.IsNodeArray(r.Value.ValueNode) { - // render down object - rendered, _ := low.YAMLNodeToBytes(r.Value.ValueNode) - r.Value.ValueNode.Value = string(rendered) - } - - CreateChange(&changes, Modified, v3.ValueLabel, + CreateChangeWithEncoding(&changes, Modified, v3.ValueLabel, l.Value.GetValueNode(), r.Value.GetValueNode(), false, l.Value.GetValue(), r.Value.GetValue()) continue } if k >= len(rKeys) { - - if utils.IsNodeMap(l.Value.ValueNode) || utils.IsNodeArray(l.Value.ValueNode) { - // render down object - rendered, _ := low.YAMLNodeToBytes(l.Value.ValueNode) - l.Value.ValueNode.Value = string(rendered) - } - - if utils.IsNodeMap(r.Value.ValueNode) || utils.IsNodeArray(r.Value.ValueNode) { - // render down object - rendered, _ := low.YAMLNodeToBytes(r.Value.ValueNode) - r.Value.ValueNode.Value = string(rendered) - } - - CreateChange(&changes, PropertyRemoved, v3.ValueLabel, + CreateChangeWithEncoding(&changes, PropertyRemoved, v3.ValueLabel, l.Value.ValueNode, r.Value.ValueNode, false, l.Value.Value, r.Value.Value) } } for k := range rKeys { if k >= len(lKeys) { - - if utils.IsNodeMap(l.Value.ValueNode) || utils.IsNodeArray(l.Value.ValueNode) { - // render down object - rendered, _ := low.YAMLNodeToBytes(l.Value.ValueNode) - l.Value.ValueNode.Value = string(rendered) - } - - if utils.IsNodeMap(r.Value.ValueNode) || utils.IsNodeArray(r.Value.ValueNode) { - // render down object - rendered, _ := low.YAMLNodeToBytes(r.Value.ValueNode) - r.Value.ValueNode.Value = string(rendered) - } - - CreateChange(&changes, PropertyAdded, v3.ValueLabel, + CreateChangeWithEncoding(&changes, PropertyAdded, v3.ValueLabel, l.Value.ValueNode, r.Value.ValueNode, false, l.Value.Value, r.Value.Value) } } diff --git a/what-changed/model/extensions.go b/what-changed/model/extensions.go index 76b83662..0af73a6c 100644 --- a/what-changed/model/extensions.go +++ b/what-changed/model/extensions.go @@ -58,7 +58,7 @@ func CompareExtensions(l, r *orderedmap.Map[low.KeyReference[string], low.ValueR var changes []*Change for i := range seenLeft { - CheckForObjectAdditionOrRemoval[*yaml.Node](seenLeft, seenRight, i, &changes, false, true) + CheckForObjectAdditionOrRemovalWithEncoding[*yaml.Node](seenLeft, seenRight, i, &changes, false, true) if seenRight[i] != nil { var props []*PropertyCheck @@ -73,13 +73,13 @@ func CompareExtensions(l, r *orderedmap.Map[low.KeyReference[string], low.ValueR New: seenRight[i].Value, }) - // check properties - CheckProperties(props) + // check properties with encoding for extensions + CheckPropertiesWithEncoding(props) } } for i := range seenRight { if seenLeft[i] == nil { - CheckForObjectAdditionOrRemoval[*yaml.Node](seenLeft, seenRight, i, &changes, false, true) + CheckForObjectAdditionOrRemovalWithEncoding[*yaml.Node](seenLeft, seenRight, i, &changes, false, true) } } ex := new(ExtensionChanges) diff --git a/what-changed/model/header.go b/what-changed/model/header.go index c23e6468..c0f4e45d 100644 --- a/what-changed/model/header.go +++ b/what-changed/model/header.go @@ -109,8 +109,10 @@ func addOpenAPIHeaderProperties(left, right low.OpenAPIHeader, changes *[]*Chang left.GetExplode(), right.GetExplode(), changes, v3.ExplodeLabel, false) // example - addPropertyCheck(&props, left.GetExample().ValueNode, right.GetExample().ValueNode, - left.GetExample(), right.GetExample(), changes, v3.ExampleLabel, false) + CheckPropertyAdditionOrRemovalWithEncoding(left.GetExample().ValueNode, right.GetExample().ValueNode, + v3.ExampleLabel, changes, false, left.GetExample(), right.GetExample()) + CheckForModificationWithEncoding(left.GetExample().ValueNode, right.GetExample().ValueNode, + v3.ExampleLabel, changes, false, left.GetExample(), right.GetExample()) // deprecated addPropertyCheck(&props, left.GetDeprecated().ValueNode, right.GetDeprecated().ValueNode, diff --git a/what-changed/model/media_type.go b/what-changed/model/media_type.go index b9277fb8..a5261749 100644 --- a/what-changed/model/media_type.go +++ b/what-changed/model/media_type.go @@ -6,7 +6,6 @@ package model import ( "github.com/pb33f/libopenapi/datamodel/low" "github.com/pb33f/libopenapi/datamodel/low/v3" - "github.com/pb33f/libopenapi/utils" ) // MediaTypeChanges represent changes made between two OpenAPI MediaType instances. @@ -117,36 +116,10 @@ func CompareMediaTypes(l, r *v3.MediaType) *MediaTypeChanges { } // Example - if !l.Example.IsEmpty() && !r.Example.IsEmpty() { - if (utils.IsNodeMap(l.Example.ValueNode) && utils.IsNodeMap(r.Example.ValueNode)) || - (utils.IsNodeArray(l.Example.ValueNode) && utils.IsNodeArray(r.Example.ValueNode)) { - render, _ := low.YAMLNodeToBytes(l.Example.ValueNode) - render, _ = utils.ConvertYAMLtoJSON(render) - l.Example.ValueNode.Value = string(render) - render, _ = low.YAMLNodeToBytes(r.Example.ValueNode) - render, _ = utils.ConvertYAMLtoJSON(render) - r.Example.ValueNode.Value = string(render) - } - addPropertyCheck(&props, l.Example.ValueNode, r.Example.ValueNode, - l.Example.Value, r.Example.Value, &changes, v3.ExampleLabel, false) - - } else { - - if utils.IsNodeMap(l.Example.ValueNode) || utils.IsNodeArray(l.Example.ValueNode) { - render, _ := low.YAMLNodeToBytes(l.Example.ValueNode) - render, _ = utils.ConvertYAMLtoJSON(render) - l.Example.ValueNode.Value = string(render) - } - - if utils.IsNodeMap(r.Example.ValueNode) || utils.IsNodeArray(r.Example.ValueNode) { - render, _ := low.YAMLNodeToBytes(r.Example.ValueNode) - render, _ = utils.ConvertYAMLtoJSON(render) - r.Example.ValueNode.Value = string(render) - } - - addPropertyCheck(&props, l.Example.ValueNode, r.Example.ValueNode, - l.Example.Value, r.Example.Value, &changes, v3.ExampleLabel, false) - } + CheckPropertyAdditionOrRemovalWithEncoding(l.Example.ValueNode, r.Example.ValueNode, + v3.ExampleLabel, &changes, false, l.Example.Value, r.Example.Value) + CheckForModificationWithEncoding(l.Example.ValueNode, r.Example.ValueNode, + v3.ExampleLabel, &changes, false, l.Example.Value, r.Example.Value) CheckProperties(props) diff --git a/what-changed/model/operation.go b/what-changed/model/operation.go index 842e3fb1..bd21911e 100644 --- a/what-changed/model/operation.go +++ b/what-changed/model/operation.go @@ -224,14 +224,18 @@ func CompareOperations(l, r any) *OperationChanges { lv := make(map[string]*v2.Parameter, len(lParams)) rv := make(map[string]*v2.Parameter, len(rParams)) + lRefs := make(map[string]*low.ValueReference[*v2.Parameter], len(lParams)) + rRefs := make(map[string]*low.ValueReference[*v2.Parameter], len(rParams)) for i := range lParams { s := lParams[i].Value.Name.Value lv[s] = lParams[i].Value + lRefs[s] = &lParams[i] // Keep the reference wrapper } for i := range rParams { s := rParams[i].Value.Name.Value rv[s] = rParams[i].Value + rRefs[s] = &rParams[i] // Keep the reference wrapper } var paramChanges []*ParameterChanges @@ -240,13 +244,15 @@ func CompareOperations(l, r any) *OperationChanges { if !low.AreEqual(lv[n], rv[n]) { ch := CompareParameters(lv[n], rv[n]) if ch != nil { + // Preserve reference information if this parameter is a $ref + PreserveParameterReference(lRefs, rRefs, n, ch) paramChanges = append(paramChanges, ch) } } continue } CreateChange(&changes, ObjectRemoved, v3.ParametersLabel, - lv[n].Name.ValueNode, nil, true, lv[n].Name.Value, + lv[n].Name.ValueNode, nil, true, lv[n], nil) } @@ -254,7 +260,7 @@ func CompareOperations(l, r any) *OperationChanges { if _, ok := lv[n]; !ok { CreateChange(&changes, ObjectAdded, v3.ParametersLabel, nil, rv[n].Name.ValueNode, rv[n].Required.Value, nil, - rv[n].Name.Value) + rv[n]) } } oc.ParameterChanges = paramChanges @@ -327,14 +333,18 @@ func CompareOperations(l, r any) *OperationChanges { lv := make(map[string]*v3.Parameter, len(lParams)) rv := make(map[string]*v3.Parameter, len(rParams)) + lRefs := make(map[string]*low.ValueReference[*v3.Parameter], len(lParams)) + rRefs := make(map[string]*low.ValueReference[*v3.Parameter], len(rParams)) for i := range lParams { s := lParams[i].Value.Name.Value lv[s] = lParams[i].Value + lRefs[s] = &lParams[i] // Keep the reference wrapper } for i := range rParams { s := rParams[i].Value.Name.Value rv[s] = rParams[i].Value + rRefs[s] = &rParams[i] // Keep the reference wrapper } var paramChanges []*ParameterChanges @@ -343,13 +353,15 @@ func CompareOperations(l, r any) *OperationChanges { if !low.AreEqual(lv[n], rv[n]) { ch := CompareParameters(lv[n], rv[n]) if ch != nil { + // Preserve reference information if this parameter is a $ref + PreserveParameterReference(lRefs, rRefs, n, ch) paramChanges = append(paramChanges, ch) } } continue } CreateChange(&changes, ObjectRemoved, v3.ParametersLabel, - lv[n].Name.ValueNode, nil, true, lv[n].Name.Value, + lv[n].Name.ValueNode, nil, true, lv[n], nil) } @@ -357,7 +369,7 @@ func CompareOperations(l, r any) *OperationChanges { if _, ok := lv[n]; !ok { CreateChange(&changes, ObjectAdded, v3.ParametersLabel, nil, rv[n].Name.ValueNode, rv[n].Required.Value, nil, - rv[n].Name.Value) + rv[n]) } } oc.ParameterChanges = paramChanges @@ -468,7 +480,7 @@ func checkServers(lServers, rServers low.NodeReference[[]low.ValueReference[*v3. } lv[k].ValueNode.Value = lv[k].Value.URL.Value CreateChange(&changes, ObjectRemoved, v3.ServersLabel, - lv[k].ValueNode, nil, true, lv[k].Value.URL.Value, + lv[k].ValueNode, nil, true, lv[k].Value, nil) sc := new(ServerChanges) sc.PropertyChanges = NewPropertyChanges(changes) @@ -483,7 +495,7 @@ func checkServers(lServers, rServers low.NodeReference[[]low.ValueReference[*v3. rv[k].ValueNode.Value = rv[k].Value.URL.Value CreateChange(&changes, ObjectAdded, v3.ServersLabel, nil, rv[k].ValueNode, false, nil, - rv[k].Value.URL.Value) + rv[k].Value) sc := new(ServerChanges) sc.PropertyChanges = NewPropertyChanges(changes) diff --git a/what-changed/model/parameter.go b/what-changed/model/parameter.go index 99567134..e4672919 100644 --- a/what-changed/model/parameter.go +++ b/what-changed/model/parameter.go @@ -343,21 +343,8 @@ func CompareParameters(l, r any) *ParameterChanges { } func checkParameterExample(expLeft, expRight low.NodeReference[*yaml.Node], changes []*Change) { - if !expLeft.IsEmpty() && !expRight.IsEmpty() { - if low.GenerateHashString(expLeft.GetValue()) != low.GenerateHashString(expRight.GetValue()) { - CreateChange(&changes, Modified, v3.ExampleLabel, - expLeft.GetValueNode(), expRight.GetValueNode(), false, - expLeft.GetValue(), expRight.GetValue()) - } - } - if expLeft.Value == nil && expRight.Value != nil { - CreateChange(&changes, PropertyAdded, v3.ExampleLabel, - nil, expRight.GetValueNode(), false, - nil, expRight.GetValue()) - } - if expLeft.Value != nil && expRight.Value == nil { - CreateChange(&changes, PropertyRemoved, v3.ExampleLabel, - expLeft.GetValueNode(), nil, false, - expLeft.GetValue(), nil) - } + CheckPropertyAdditionOrRemovalWithEncoding(expLeft.ValueNode, expRight.ValueNode, + v3.ExampleLabel, &changes, false, expLeft.Value, expRight.Value) + CheckForModificationWithEncoding(expLeft.ValueNode, expRight.ValueNode, + v3.ExampleLabel, &changes, false, expLeft.Value, expRight.Value) } diff --git a/what-changed/model/parameter_test.go b/what-changed/model/parameter_test.go index 7a252c7a..61a763ab 100644 --- a/what-changed/model/parameter_test.go +++ b/what-changed/model/parameter_test.go @@ -10,6 +10,7 @@ import ( "github.com/pb33f/libopenapi/datamodel/low" "github.com/pb33f/libopenapi/datamodel/low/v2" "github.com/pb33f/libopenapi/datamodel/low/v3" + "github.com/pb33f/libopenapi/index" "github.com/stretchr/testify/assert" "go.yaml.in/yaml/v4" ) @@ -197,6 +198,105 @@ func TestCompareParameters_V3_ExampleChange(t *testing.T) { assert.Equal(t, 0, extChanges.TotalBreakingChanges()) } +func TestCompareParameters_V3_Reference(t *testing.T) { + // Test that parameter references are preserved when comparing parameters + // This is important for the Referenced Changes section in reports + left := ` +openapi: "3.1.0" +paths: + /test: + get: + parameters: + - $ref: "#/components/parameters/PageParam" +components: + parameters: + PageParam: + name: page + in: query + description: "Page number" + schema: + type: integer` + + right := ` +openapi: "3.1.0" +paths: + /test: + get: + parameters: + - $ref: "#/components/parameters/PageParam" +components: + parameters: + PageParam: + name: page + in: query + description: "Page number with more info" + schema: + type: integer` + + var lNode, rNode yaml.Node + _ = yaml.Unmarshal([]byte(left), &lNode) + _ = yaml.Unmarshal([]byte(right), &rNode) + + // Build the document context (needed for references) + ctx := context.Background() + + // Create spec index for references + idx := index.NewSpecIndex(&lNode) + + // Parse the parameters from the components section + var lParamNode, rParamNode yaml.Node + lCompParam := ` +name: page +in: query +description: "Page number" +schema: + type: integer` + rCompParam := ` +name: page +in: query +description: "Page number with more info" +schema: + type: integer` + + _ = yaml.Unmarshal([]byte(lCompParam), &lParamNode) + _ = yaml.Unmarshal([]byte(rCompParam), &rParamNode) + + var lDoc v3.Parameter + var rDoc v3.Parameter + _ = low.BuildModel(lParamNode.Content[0], &lDoc) + _ = low.BuildModel(rParamNode.Content[0], &rDoc) + _ = lDoc.Build(ctx, nil, lParamNode.Content[0], idx) + _ = rDoc.Build(ctx, nil, rParamNode.Content[0], idx) + + // Create ValueReferences with the reference path + lRef := low.ValueReference[*v3.Parameter]{ + Value: &lDoc, + ValueNode: lParamNode.Content[0], + } + lRef.SetReference("#/components/parameters/PageParam", nil) + + rRef := low.ValueReference[*v3.Parameter]{ + Value: &rDoc, + ValueNode: rParamNode.Content[0], + } + rRef.SetReference("#/components/parameters/PageParam", nil) + + // Compare the parameters + changes := CompareParameters(&lDoc, &rDoc) + assert.NotNil(t, changes) + assert.Equal(t, 1, changes.TotalChanges()) + + // Now apply the reference preservation logic (as done in operation.go) + if lRef.IsReference() { + SetReferenceIfExists(&lRef, changes) + } else if rRef.IsReference() { + SetReferenceIfExists(&rRef, changes) + } + + // Verify that the reference was preserved + assert.Equal(t, "#/components/parameters/PageParam", changes.GetChangeReference()) +} + func TestCompareParameters_V3_ExampleEqual(t *testing.T) { // Clear hash cache to ensure deterministic results in concurrent test environments low.ClearHashCache() diff --git a/what-changed/model/path_item.go b/what-changed/model/path_item.go index aae91f72..d41863d8 100644 --- a/what-changed/model/path_item.go +++ b/what-changed/model/path_item.go @@ -442,14 +442,18 @@ func extractV3ParametersIntoInterface(l, r []low.ValueReference[*v3.Parameter]) func checkParameters(lParams, rParams []low.ValueReference[low.SharedParameters], changes *[]*Change, pc *PathItemChanges) { lv := make(map[string]low.SharedParameters, len(lParams)) rv := make(map[string]low.SharedParameters, len(rParams)) + lRefs := make(map[string]*low.ValueReference[low.SharedParameters], len(lParams)) + rRefs := make(map[string]*low.ValueReference[low.SharedParameters], len(rParams)) for i := range lParams { s := lParams[i].Value.GetName().Value lv[s] = lParams[i].Value + lRefs[s] = &lParams[i] // Keep the reference wrapper } for i := range rParams { s := rParams[i].Value.GetName().Value rv[s] = rParams[i].Value + rRefs[s] = &rParams[i] // Keep the reference wrapper } var paramChanges []*ParameterChanges @@ -458,13 +462,15 @@ func checkParameters(lParams, rParams []low.ValueReference[low.SharedParameters] if !low.AreEqual(lv[n], rv[n]) { ch := CompareParameters(lv[n], rv[n]) if ch != nil { + // Preserve reference information if this parameter is a $ref + PreserveParameterReference(lRefs, rRefs, n, ch) paramChanges = append(paramChanges, ch) } } continue } CreateChange(changes, ObjectRemoved, v3.ParametersLabel, - lv[n].GetName().ValueNode, nil, true, lv[n].GetName().Value, + lv[n].GetName().ValueNode, nil, true, lv[n], nil) } @@ -472,7 +478,7 @@ func checkParameters(lParams, rParams []low.ValueReference[low.SharedParameters] if _, ok := lv[n]; !ok { CreateChange(changes, ObjectAdded, v3.ParametersLabel, nil, rv[n].GetName().ValueNode, rv[n].GetRequired().Value, nil, - rv[n].GetName().Value) + rv[n]) } } pc.ParameterChanges = paramChanges diff --git a/what-changed/model/schema.go b/what-changed/model/schema.go index 35306931..41d3858a 100644 --- a/what-changed/model/schema.go +++ b/what-changed/model/schema.go @@ -489,7 +489,7 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { checkExamples(lSchema, rSchema, &changes) // check schema core properties for changes. - checkSchemaPropertyChanges(lSchema, rSchema, &changes, sc) + checkSchemaPropertyChanges(lSchema, rSchema, l, r, &changes, sc) // now for the confusing part, there is also a schema's 'properties' property to parse. // inception, eat your heart out. @@ -728,6 +728,8 @@ func buildProperty(lProps, rProps []string, lEntities, rEntities map[string]*bas func checkSchemaPropertyChanges( lSchema *base.Schema, rSchema *base.Schema, + lProxy *base.SchemaProxy, + rProxy *base.SchemaProxy, changes *[]*Change, sc *SchemaChanges, ) { var props []*PropertyCheck @@ -1252,15 +1254,10 @@ func checkSchemaPropertyChanges( rnv = rSchema.Example.ValueNode } // Example - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.ExampleLabel, - Changes: changes, - Breaking: false, - Original: lSchema, - New: rSchema, - }) + CheckPropertyAdditionOrRemovalWithEncoding(lnv, rnv, + v3.ExampleLabel, changes, false, lSchema, rSchema) + CheckForModificationWithEncoding(lnv, rnv, + v3.ExampleLabel, changes, false, lSchema, rSchema) lnv = nil rnv = nil @@ -1557,6 +1554,28 @@ func checkSchemaPropertyChanges( // check core properties CheckProperties(props) + + // Post-process: Update context line numbers for Type changes to use schema KeyNode for better context + // This provides line where "schema:" is defined, not "type: value" + if changes != nil && len(*changes) > 0 { + for _, change := range *changes { + if change.Property == v3.TypeLabel && change.Context != nil { + if lProxy != nil && lProxy.GetKeyNode() != nil { + line := lProxy.GetKeyNode().Line + col := lProxy.GetKeyNode().Column + change.Context.OriginalLine = &line + change.Context.OriginalColumn = &col + } + if rProxy != nil && rProxy.GetKeyNode() != nil { + line := rProxy.GetKeyNode().Line + col := rProxy.GetKeyNode().Column + change.Context.NewLine = &line + change.Context.NewColumn = &col + } + break // found the type change, no need to continue + } + } + } } func checkExamples(lSchema *base.Schema, rSchema *base.Schema, changes *[]*Change) { @@ -1594,7 +1613,7 @@ func checkExamples(lSchema *base.Schema, rSchema *base.Schema, changes *[]*Chang if len(lExampKey) == len(rExampKey) { for i := range lExampKey { if lExampKey[i] != rExampKey[i] { - CreateChange(changes, Modified, v3.ExamplesLabel, + CreateChangeWithEncoding(changes, Modified, v3.ExamplesLabel, lExampN[lExampKey[i]], rExampN[rExampKey[i]], false, lExampVal[lExampKey[i]], rExampVal[rExampKey[i]]) } @@ -1604,12 +1623,12 @@ func checkExamples(lSchema *base.Schema, rSchema *base.Schema, changes *[]*Chang if len(lExampKey) > len(rExampKey) { for i := range lExampKey { if i < len(rExampKey) && lExampKey[i] != rExampKey[i] { - CreateChange(changes, Modified, v3.ExamplesLabel, + CreateChangeWithEncoding(changes, Modified, v3.ExamplesLabel, lExampN[lExampKey[i]], rExampN[rExampKey[i]], false, lExampVal[lExampKey[i]], rExampVal[rExampKey[i]]) } if i >= len(rExampKey) { - CreateChange(changes, ObjectRemoved, v3.ExamplesLabel, + CreateChangeWithEncoding(changes, ObjectRemoved, v3.ExamplesLabel, lExampN[lExampKey[i]], nil, false, lExampVal[lExampKey[i]], nil) } @@ -1620,12 +1639,12 @@ func checkExamples(lSchema *base.Schema, rSchema *base.Schema, changes *[]*Chang if len(lExampKey) < len(rExampKey) { for i := range rExampKey { if i < len(lExampKey) && lExampKey[i] != rExampKey[i] { - CreateChange(changes, Modified, v3.ExamplesLabel, + CreateChangeWithEncoding(changes, Modified, v3.ExamplesLabel, lExampN[lExampKey[i]], rExampN[rExampKey[i]], false, lExampVal[lExampKey[i]], rExampVal[rExampKey[i]]) } if i >= len(lExampKey) { - CreateChange(changes, ObjectAdded, v3.ExamplesLabel, + CreateChangeWithEncoding(changes, ObjectAdded, v3.ExamplesLabel, nil, rExampN[rExampKey[i]], false, nil, rExampVal[rExampKey[i]]) } diff --git a/what-changed/model/schema_test.go b/what-changed/model/schema_test.go index e5e2c406..34330cad 100644 --- a/what-changed/model/schema_test.go +++ b/what-changed/model/schema_test.go @@ -3513,7 +3513,7 @@ func TestCompareSchemas_fireNilCheck(t *testing.T) { // Clear hash cache to ensure deterministic results in concurrent test environments low.ClearHashCache() checkSchemaXML(nil, nil, nil, nil) - checkSchemaPropertyChanges(nil, nil, nil, nil) + checkSchemaPropertyChanges(nil, nil, nil, nil, nil, nil) checkExamples(nil, nil, nil) } @@ -4538,3 +4538,37 @@ components: } assert.True(t, foundDepReq) } + +// TestCompareSchemas_TypeChange_ContextLines tests that type changes have proper context +func TestCompareSchemas_TypeChange_ContextLines(t *testing.T) { + // Clear hash cache to ensure deterministic results + low.ClearHashCache() + left := `openapi: 3.0 +components: + schemas: + Something: + type: string` + + right := `openapi: 3.0 +components: + schemas: + Something: + type: integer` + + leftDoc, rightDoc := test_BuildDoc(left, right) + + lSchemaProxy := leftDoc.Components.Value.FindSchema("Something").Value + rSchemaProxy := rightDoc.Components.Value.FindSchema("Something").Value + + changes := CompareSchemas(lSchemaProxy, rSchemaProxy) + assert.NotNil(t, changes) + assert.Equal(t, 1, changes.TotalChanges()) + + // Verify the type change has context lines set + typeChange := changes.Changes[0] + assert.Equal(t, "type", typeChange.Property) + assert.NotNil(t, typeChange.Context) + // Context lines should be set from the schema KeyNode + assert.NotNil(t, typeChange.Context.OriginalLine) + assert.NotNil(t, typeChange.Context.NewLine) +} diff --git a/what-changed/model/security_requirement.go b/what-changed/model/security_requirement.go index 1e496b17..d531304c 100644 --- a/what-changed/model/security_requirement.go +++ b/what-changed/model/security_requirement.go @@ -6,7 +6,6 @@ package model import ( "github.com/pb33f/libopenapi/datamodel/low" "github.com/pb33f/libopenapi/datamodel/low/base" - v3 "github.com/pb33f/libopenapi/datamodel/low/v3" "github.com/pb33f/libopenapi/orderedmap" "go.yaml.in/yaml/v4" ) @@ -51,14 +50,32 @@ func CompareSecurityRequirement(l, r *base.SecurityRequirement) *SecurityRequire return sc } -func removedSecurityRequirement(vn *yaml.Node, name string, changes *[]*Change) { - CreateChange(changes, ObjectRemoved, v3.SecurityLabel, - vn, nil, true, name, nil) +func removedSecurityRequirement(vn *yaml.Node, schemeName, scopeName string, changes *[]*Change) { + property := schemeName + value := scopeName + var node *yaml.Node = vn + if scopeName == "" { + // entire scheme was removed, use scheme name as value + value = schemeName + // Don't use the node for entire scheme removal, as it may be an empty array [] + node = nil + } + CreateChange(changes, ObjectRemoved, property, + node, nil, true, value, nil) } -func addedSecurityRequirement(vn *yaml.Node, name string, changes *[]*Change) { - CreateChange(changes, ObjectAdded, v3.SecurityLabel, - nil, vn, false, nil, name) +func addedSecurityRequirement(vn *yaml.Node, schemeName, scopeName string, changes *[]*Change) { + property := schemeName + value := scopeName + var node *yaml.Node = vn + if scopeName == "" { + // entire scheme was added, use scheme name as value + value = schemeName + // Don't use the node for entire scheme addition, as it may be an empty array [] + node = nil + } + CreateChange(changes, ObjectAdded, property, + nil, node, false, nil, value) } // tricky to do this correctly, this is my solution. @@ -84,7 +101,7 @@ func checkSecurityRequirement(lSec, rSec *orderedmap.Map[low.KeyReference[string for z = range lKeys { if z < len(rKeys) { if _, ok := rValues[lKeys[z]]; !ok { - removedSecurityRequirement(lValues[lKeys[z]].ValueNode, lKeys[z], changes) + removedSecurityRequirement(lValues[lKeys[z]].ValueNode, lKeys[z], "", changes) continue } @@ -111,45 +128,47 @@ func checkSecurityRequirement(lSec, rSec *orderedmap.Map[low.KeyReference[string for t = range lRoleKeys { if t < len(rRoleKeys) { if _, ok := rRoleValues[lRoleKeys[t]]; !ok { - removedSecurityRequirement(lRoleValues[lRoleKeys[t]].ValueNode, lRoleKeys[t], changes) + removedSecurityRequirement(lRoleValues[lRoleKeys[t]].ValueNode, lKeys[z], lRoleKeys[t], changes) continue } } if t >= len(rRoleKeys) { if _, ok := rRoleValues[lRoleKeys[t]]; !ok { - removedSecurityRequirement(lRoleValues[lRoleKeys[t]].ValueNode, lRoleKeys[t], changes) + removedSecurityRequirement(lRoleValues[lRoleKeys[t]].ValueNode, lKeys[z], lRoleKeys[t], changes) } } } for t = range rRoleKeys { if t < len(lRoleKeys) { if _, ok := lRoleValues[rRoleKeys[t]]; !ok { - addedSecurityRequirement(rRoleValues[rRoleKeys[t]].ValueNode, rRoleKeys[t], changes) + addedSecurityRequirement(rRoleValues[rRoleKeys[t]].ValueNode, rKeys[z], rRoleKeys[t], changes) continue } } if t >= len(lRoleKeys) { - addedSecurityRequirement(rRoleValues[rRoleKeys[t]].ValueNode, rRoleKeys[t], changes) + if _, ok := lRoleValues[rRoleKeys[t]]; !ok { + addedSecurityRequirement(rRoleValues[rRoleKeys[t]].ValueNode, rKeys[z], rRoleKeys[t], changes) + } } } } if z >= len(rKeys) { if _, ok := rValues[lKeys[z]]; !ok { - removedSecurityRequirement(lValues[lKeys[z]].ValueNode, lKeys[z], changes) + removedSecurityRequirement(lValues[lKeys[z]].ValueNode, lKeys[z], "", changes) } } } for z = range rKeys { if z < len(lKeys) { if _, ok := lValues[rKeys[z]]; !ok { - addedSecurityRequirement(rValues[rKeys[z]].ValueNode, rKeys[z], changes) + addedSecurityRequirement(rValues[rKeys[z]].ValueNode, rKeys[z], "", changes) continue } } if z >= len(lKeys) { if _, ok := lValues[rKeys[z]]; !ok { - addedSecurityRequirement(rValues[rKeys[z]].ValueNode, rKeys[z], changes) + addedSecurityRequirement(rValues[rKeys[z]].ValueNode, rKeys[z], "", changes) } } } diff --git a/what-changed/model/security_requirement_test.go b/what-changed/model/security_requirement_test.go index bf08b1b5..ae47e87c 100644 --- a/what-changed/model/security_requirement_test.go +++ b/what-changed/model/security_requirement_test.go @@ -428,3 +428,206 @@ biscuit: assert.Equal(t, 0, extChanges.TotalBreakingChanges()) assert.Equal(t, ObjectAdded, extChanges.Changes[0].ChangeType) } + +// Test that scope additions show the scheme name as the property +func TestCompareSecurityRequirement_AddScope_ShowsSchemeName(t *testing.T) { + // Clear hash cache to ensure deterministic results in concurrent test environments + low.ClearHashCache() + left := `OAuth2: + - read` + + right := `OAuth2: + - read + - write` + + var lNode, rNode yaml.Node + _ = yaml.Unmarshal([]byte(left), &lNode) + _ = yaml.Unmarshal([]byte(right), &rNode) + + // create low level objects + var lDoc base.SecurityRequirement + var rDoc base.SecurityRequirement + _ = 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 := CompareSecurityRequirement(&lDoc, &rDoc) + assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Equal(t, 0, extChanges.TotalBreakingChanges()) + assert.Equal(t, ObjectAdded, extChanges.Changes[0].ChangeType) + assert.Equal(t, "OAuth2", extChanges.Changes[0].Property) // Verify scheme name in property + assert.Equal(t, "write", extChanges.Changes[0].New) // Verify scope name in new value +} + +// Test that scope removals show the scheme name as the property +func TestCompareSecurityRequirement_RemoveScope_ShowsSchemeName(t *testing.T) { + // Clear hash cache to ensure deterministic results in concurrent test environments + low.ClearHashCache() + left := `OAuth2: + - read + - write` + + right := `OAuth2: + - read` + + var lNode, rNode yaml.Node + _ = yaml.Unmarshal([]byte(left), &lNode) + _ = yaml.Unmarshal([]byte(right), &rNode) + + // create low level objects + var lDoc base.SecurityRequirement + var rDoc base.SecurityRequirement + _ = 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 := CompareSecurityRequirement(&lDoc, &rDoc) + assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Equal(t, 1, extChanges.TotalBreakingChanges()) + assert.Equal(t, ObjectRemoved, extChanges.Changes[0].ChangeType) + assert.Equal(t, "OAuth2", extChanges.Changes[0].Property) // Verify scheme name in property + assert.Equal(t, "write", extChanges.Changes[0].Original) // Verify scope name in original value +} + +// Test that multiple scope additions show the scheme name as the property +func TestCompareSecurityRequirement_AddMultipleScopes_ShowsSchemeName(t *testing.T) { + // Clear hash cache to ensure deterministic results in concurrent test environments + low.ClearHashCache() + left := `OAuth2: + - read` + + right := `OAuth2: + - read + - write + - admin` + + var lNode, rNode yaml.Node + _ = yaml.Unmarshal([]byte(left), &lNode) + _ = yaml.Unmarshal([]byte(right), &rNode) + + // create low level objects + var lDoc base.SecurityRequirement + var rDoc base.SecurityRequirement + _ = 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 := CompareSecurityRequirement(&lDoc, &rDoc) + assert.Equal(t, 2, extChanges.TotalChanges()) + assert.Equal(t, 0, extChanges.TotalBreakingChanges()) + + // Both changes should have OAuth2 as the property + for _, change := range extChanges.Changes { + assert.Equal(t, ObjectAdded, change.ChangeType) + assert.Equal(t, "OAuth2", change.Property) + } + // Verify the scope names + scopes := []string{extChanges.Changes[0].New, extChanges.Changes[1].New} + assert.Contains(t, scopes, "write") + assert.Contains(t, scopes, "admin") +} + +// Test that adding an entire scheme shows the scheme name as both property and value +func TestCompareSecurityRequirement_AddEntireScheme_ShowsSchemeName(t *testing.T) { + // Clear hash cache to ensure deterministic results in concurrent test environments + low.ClearHashCache() + left := `OAuth2: + - read` + + right := `OAuth2: + - read +ApiKey: []` + + var lNode, rNode yaml.Node + _ = yaml.Unmarshal([]byte(left), &lNode) + _ = yaml.Unmarshal([]byte(right), &rNode) + + // create low level objects + var lDoc base.SecurityRequirement + var rDoc base.SecurityRequirement + _ = 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 := CompareSecurityRequirement(&lDoc, &rDoc) + assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Equal(t, 0, extChanges.TotalBreakingChanges()) + assert.Equal(t, ObjectAdded, extChanges.Changes[0].ChangeType) + assert.Equal(t, "ApiKey", extChanges.Changes[0].Property) // Verify scheme name in property + assert.Equal(t, "ApiKey", extChanges.Changes[0].New) // Verify scheme name in new value (entire scheme added) +} + +// Test that removing an entire scheme shows the scheme name as both property and value +func TestCompareSecurityRequirement_RemoveEntireScheme_ShowsSchemeName(t *testing.T) { + // Clear hash cache to ensure deterministic results in concurrent test environments + low.ClearHashCache() + left := `OAuth2: + - read +ApiKey: []` + + right := `OAuth2: + - read` + + var lNode, rNode yaml.Node + _ = yaml.Unmarshal([]byte(left), &lNode) + _ = yaml.Unmarshal([]byte(right), &rNode) + + // create low level objects + var lDoc base.SecurityRequirement + var rDoc base.SecurityRequirement + _ = 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 := CompareSecurityRequirement(&lDoc, &rDoc) + assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Equal(t, 1, extChanges.TotalBreakingChanges()) + assert.Equal(t, ObjectRemoved, extChanges.Changes[0].ChangeType) + assert.Equal(t, "ApiKey", extChanges.Changes[0].Property) // Verify scheme name in property + assert.Equal(t, "ApiKey", extChanges.Changes[0].Original) // Verify scheme name in original value (entire scheme removed) +} + +// Test real-world OAuth2 example from the bug report +func TestCompareSecurityRequirement_OAuth2_RealWorld(t *testing.T) { + // Clear hash cache to ensure deterministic results in concurrent test environments + low.ClearHashCache() + left := `OAuth2: + - read + - execute` + + right := `OAuth2: + - read + - write + - execute` + + var lNode, rNode yaml.Node + _ = yaml.Unmarshal([]byte(left), &lNode) + _ = yaml.Unmarshal([]byte(right), &rNode) + + // create low level objects + var lDoc base.SecurityRequirement + var rDoc base.SecurityRequirement + _ = 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 := CompareSecurityRequirement(&lDoc, &rDoc) + assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Equal(t, 0, extChanges.TotalBreakingChanges()) + assert.Equal(t, ObjectAdded, extChanges.Changes[0].ChangeType) + // The key assertion: property should be "OAuth2", not "security" + assert.Equal(t, "OAuth2", extChanges.Changes[0].Property) + assert.Equal(t, "write", extChanges.Changes[0].New) +}