Skip to content

Commit 635ab9f

Browse files
committed
address coverage gaps and regressions introduced by fixes.
1 parent 09cb805 commit 635ab9f

13 files changed

Lines changed: 925 additions & 58 deletions

datamodel/low/extraction_functions.go

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1255,6 +1255,10 @@ func hashYamlNodeFast(n *yaml.Node) string {
12551255

12561256
// hashNodeTree walks the YAML tree and hashes it without marshaling
12571257
func hashNodeTree(h *maphash.Hash, n *yaml.Node, visited map[*yaml.Node]bool) {
1258+
hashNodeTreeWithNumericNormalization(h, n, visited, true)
1259+
}
1260+
1261+
func hashNodeTreeWithNumericNormalization(h *maphash.Hash, n *yaml.Node, visited map[*yaml.Node]bool, normalizeNumericScalars bool) {
12581262
if n == nil {
12591263
return
12601264
}
@@ -1268,7 +1272,7 @@ func hashNodeTree(h *maphash.Hash, n *yaml.Node, visited map[*yaml.Node]bool) {
12681272

12691273
// Hash node metadata. Numeric scalars are normalized so semantically equivalent
12701274
// values like `1e-08` and `1e-8` compare equal.
1271-
tag, value := comparableScalarTagAndValue(n)
1275+
tag, value := scalarTagAndValueForHash(n, normalizeNumericScalars)
12721276
h.Write([]byte{byte(n.Kind)})
12731277
h.Write([]byte(tag))
12741278
h.Write([]byte(value))
@@ -1290,7 +1294,7 @@ func hashNodeTree(h *maphash.Hash, n *yaml.Node, visited map[*yaml.Node]bool) {
12901294
case yaml.SequenceNode:
12911295
h.Write([]byte("["))
12921296
for _, child := range content {
1293-
hashNodeTree(h, child, visited)
1297+
hashNodeTreeWithNumericNormalization(h, child, visited, normalizeNumericScalars)
12941298
h.Write([]byte(","))
12951299
}
12961300
h.Write([]byte("]"))
@@ -1317,7 +1321,9 @@ func hashNodeTree(h *maphash.Hash, n *yaml.Node, visited map[*yaml.Node]bool) {
13171321
if i+1 < len(content) {
13181322
keyH := hasherPool.Get().(*maphash.Hash)
13191323
keyH.Reset()
1320-
hashNodeTree(keyH, content[i], visited)
1324+
keyVisited := getVisitedMap()
1325+
hashNodeTreeWithNumericNormalization(keyH, content[i], keyVisited, false)
1326+
putVisitedMap(keyVisited)
13211327
pairs = append(pairs, kvPair{
13221328
keyHash: keyH.Sum64(),
13231329
keyNode: content[i],
@@ -1334,36 +1340,43 @@ func hashNodeTree(h *maphash.Hash, n *yaml.Node, visited map[*yaml.Node]bool) {
13341340

13351341
// Hash in sorted order
13361342
for _, pair := range pairs {
1337-
hashNodeTree(h, pair.keyNode, visited)
1343+
hashNodeTreeWithNumericNormalization(h, pair.keyNode, visited, false)
13381344
h.Write([]byte(":"))
1339-
hashNodeTree(h, pair.valueNode, visited)
1345+
hashNodeTreeWithNumericNormalization(h, pair.valueNode, visited, true)
13401346
h.Write([]byte(","))
13411347
}
13421348
h.Write([]byte("}"))
13431349

13441350
case yaml.DocumentNode:
13451351
h.Write([]byte("DOC["))
13461352
for _, child := range content {
1347-
hashNodeTree(h, child, visited)
1353+
hashNodeTreeWithNumericNormalization(h, child, visited, normalizeNumericScalars)
13481354
}
13491355
h.Write([]byte("]"))
13501356

13511357
case yaml.AliasNode:
13521358
h.Write([]byte("ALIAS["))
13531359
if n.Alias != nil {
1354-
hashNodeTree(h, n.Alias, visited)
1360+
hashNodeTreeWithNumericNormalization(h, n.Alias, visited, normalizeNumericScalars)
13551361
}
13561362
h.Write([]byte("]"))
13571363
}
13581364
}
13591365

13601366
func comparableScalarTagAndValue(n *yaml.Node) (string, string) {
1367+
return scalarTagAndValueForHash(n, true)
1368+
}
1369+
1370+
func scalarTagAndValueForHash(n *yaml.Node, normalizeNumericScalars bool) (string, string) {
13611371
if n == nil {
13621372
return "", ""
13631373
}
13641374
if n.Kind != yaml.ScalarNode {
13651375
return n.Tag, n.Value
13661376
}
1377+
if !normalizeNumericScalars {
1378+
return n.Tag, n.Value
1379+
}
13671380
if n.Tag != "!!int" && n.Tag != "!!float" {
13681381
return n.Tag, n.Value
13691382
}

datamodel/low/extraction_functions_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2973,6 +2973,32 @@ func TestCompareYAMLNodes_ComplexNodesNumericEquivalence(t *testing.T) {
29732973
assert.True(t, CompareYAMLNodes(left, right))
29742974
}
29752975

2976+
func TestScalarTagAndValueForHash_DisablesNumericNormalization(t *testing.T) {
2977+
node := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!float", Value: "1.0"}
2978+
tag, value := scalarTagAndValueForHash(node, false)
2979+
assert.Equal(t, "!!float", tag)
2980+
assert.Equal(t, "1.0", value)
2981+
}
2982+
2983+
func TestCompareYAMLNodes_NumericMapKeysAreNotEquivalent(t *testing.T) {
2984+
left := &yaml.Node{
2985+
Kind: yaml.MappingNode,
2986+
Content: []*yaml.Node{
2987+
{Kind: yaml.ScalarNode, Tag: "!!int", Value: "1"},
2988+
{Kind: yaml.ScalarNode, Tag: "!!str", Value: "coffee"},
2989+
},
2990+
}
2991+
right := &yaml.Node{
2992+
Kind: yaml.MappingNode,
2993+
Content: []*yaml.Node{
2994+
{Kind: yaml.ScalarNode, Tag: "!!float", Value: "1.0"},
2995+
{Kind: yaml.ScalarNode, Tag: "!!str", Value: "coffee"},
2996+
},
2997+
}
2998+
2999+
assert.False(t, CompareYAMLNodes(left, right))
3000+
}
3001+
29763002
func TestGenerateHashString_SchemaProxyNoCache(t *testing.T) {
29773003
// Test that SchemaProxy types don't get cached (shouldCache = false)
29783004
// We can't easily test this without creating actual SchemaProxy objects

what-changed/model/comparison_functions.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@ const (
2525

2626
var changeMutex sync.Mutex
2727

28+
type changeCollection interface {
29+
GetAllChanges() []*Change
30+
}
31+
2832
// SetReferenceIfExists checks if a low-level value has a reference and sets it on the change object
2933
// if the change object implements the ChangeIsReferenced interface.
3034
func SetReferenceIfExists[T any](value *low.ValueReference[T], changeObj any) {
@@ -45,6 +49,56 @@ func PreserveParameterReference[T any](lRefs, rRefs map[string]*low.ValueReferen
4549
}
4650
}
4751

52+
func overrideChangeCollectionBreaking(changeObj any, breaking bool) {
53+
if changeObj == nil {
54+
return
55+
}
56+
collection, ok := changeObj.(changeCollection)
57+
if !ok {
58+
return
59+
}
60+
for _, change := range collection.GetAllChanges() {
61+
if change != nil {
62+
change.Breaking = breaking
63+
}
64+
}
65+
}
66+
67+
func compareExamplesWithParentBreaking(component, property string) func(l, r *base.Example) *ExampleChanges {
68+
return func(l, r *base.Example) *ExampleChanges {
69+
changes := CompareExamples(l, r)
70+
if changes == nil {
71+
return nil
72+
}
73+
74+
switch {
75+
case l == nil:
76+
overrideChangeCollectionBreaking(changes, BreakingAdded(component, property))
77+
case r == nil:
78+
overrideChangeCollectionBreaking(changes, BreakingRemoved(component, property))
79+
default:
80+
overrideChangeCollectionBreaking(changes, BreakingModified(component, property))
81+
}
82+
return changes
83+
}
84+
}
85+
86+
func CheckExampleMapForChangesWithRules(
87+
expLeft, expRight *orderedmap.Map[low.KeyReference[string], low.ValueReference[*base.Example]],
88+
changes *[]*Change, label string, component, property string,
89+
) map[string]*ExampleChanges {
90+
return CheckMapForChangesWithRules(expLeft, expRight, changes, label,
91+
compareExamplesWithParentBreaking(component, property), component, property)
92+
}
93+
94+
func CheckExampleMapForChangesWithNilSupportAndRules(
95+
expLeft, expRight *orderedmap.Map[low.KeyReference[string], low.ValueReference[*base.Example]],
96+
changes *[]*Change, label string, component, property string,
97+
) map[string]*ExampleChanges {
98+
return CheckMapForChangesWithNilSupportAndRules(expLeft, expRight, changes, label,
99+
compareExamplesWithParentBreaking(component, property), component, property)
100+
}
101+
48102
func checkLocation(ctx *ChangeContext, hs base.HasIndex) bool {
49103
if !reflect.ValueOf(hs).IsNil() {
50104
idx := hs.GetIndex()

what-changed/model/comparison_functions_test.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@
44
package model
55

66
import (
7+
"context"
78
"github.com/pb33f/libopenapi/index"
89
"testing"
910

1011
"github.com/pb33f/libopenapi/datamodel/low"
12+
"github.com/pb33f/libopenapi/datamodel/low/base"
1113
"github.com/pb33f/libopenapi/orderedmap"
1214
"github.com/pb33f/libopenapi/utils"
1315
"github.com/stretchr/testify/assert"
@@ -237,6 +239,78 @@ func TestCheckMapForAdditionRemoval(t *testing.T) {
237239
assert.Len(t, changes, 1)
238240
}
239241

242+
func buildTestExample(t *testing.T, raw string) *base.Example {
243+
t.Helper()
244+
245+
var node yaml.Node
246+
assert.NoError(t, yaml.Unmarshal([]byte(raw), &node))
247+
248+
var example base.Example
249+
assert.NoError(t, low.BuildModel(node.Content[0], &example))
250+
assert.NoError(t, example.Build(context.Background(), nil, node.Content[0], nil))
251+
return &example
252+
}
253+
254+
func TestOverrideChangeCollectionBreaking(t *testing.T) {
255+
overrideChangeCollectionBreaking(nil, true)
256+
overrideChangeCollectionBreaking("not-a-collection", true)
257+
258+
changes := &ExampleChanges{
259+
PropertyChanges: NewPropertyChanges([]*Change{
260+
{Breaking: true},
261+
{Breaking: false},
262+
}),
263+
}
264+
265+
overrideChangeCollectionBreaking(changes, false)
266+
assert.False(t, changes.Changes[0].Breaking)
267+
assert.False(t, changes.Changes[1].Breaking)
268+
269+
overrideChangeCollectionBreaking(changes, true)
270+
assert.True(t, changes.Changes[0].Breaking)
271+
assert.True(t, changes.Changes[1].Breaking)
272+
}
273+
274+
func TestCompareExamplesWithParentBreaking(t *testing.T) {
275+
ResetDefaultBreakingRules()
276+
ResetActiveBreakingRulesConfig()
277+
defer ResetDefaultBreakingRules()
278+
defer ResetActiveBreakingRulesConfig()
279+
280+
config := GenerateDefaultBreakingRules()
281+
config.Example.Value = rule(false, true, false)
282+
config.Parameter.Examples = rule(false, false, false)
283+
config.MediaType.Examples = rule(true, false, true)
284+
SetActiveBreakingRulesConfig(config)
285+
286+
modified := compareExamplesWithParentBreaking(CompParameter, PropExamples)(
287+
buildTestExample(t, "value: left"),
288+
buildTestExample(t, "value: right"),
289+
)
290+
assert.NotNil(t, modified)
291+
assert.False(t, modified.GetAllChanges()[0].Breaking)
292+
293+
added := compareExamplesWithParentBreaking(CompMediaType, PropExamples)(
294+
nil,
295+
buildTestExample(t, "value: right"),
296+
)
297+
assert.NotNil(t, added)
298+
assert.True(t, added.GetAllChanges()[0].Breaking)
299+
300+
removed := compareExamplesWithParentBreaking(CompMediaType, PropExamples)(
301+
buildTestExample(t, "value: left"),
302+
nil,
303+
)
304+
assert.NotNil(t, removed)
305+
assert.True(t, removed.GetAllChanges()[0].Breaking)
306+
307+
unchanged := compareExamplesWithParentBreaking(CompParameter, PropExamples)(
308+
buildTestExample(t, "value: same"),
309+
buildTestExample(t, "value: same"),
310+
)
311+
assert.Nil(t, unchanged)
312+
}
313+
240314
type test_hasIndex struct {
241315
index *index.SpecIndex
242316
}

what-changed/model/header.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,9 @@ func (h *HeaderChanges) TotalChanges() int {
7676
// TotalBreakingChanges returns the total number of breaking changes made between two Header instances.
7777
func (h *HeaderChanges) TotalBreakingChanges() int {
7878
c := h.PropertyChanges.TotalBreakingChanges()
79+
for k := range h.ExamplesChanges {
80+
c += h.ExamplesChanges[k].TotalBreakingChanges()
81+
}
7982
for k := range h.ContentChanges {
8083
c += h.ContentChanges[k].TotalBreakingChanges()
8184
}
@@ -282,8 +285,8 @@ func CompareHeaders(l, r any) *HeaderChanges {
282285
}
283286

284287
// examples
285-
hc.ExamplesChanges = CheckMapForChangesWithRules(lHeader.Examples.Value, rHeader.Examples.Value,
286-
&changes, v3.ExamplesLabel, CompareExamples, CompHeader, PropExamples)
288+
hc.ExamplesChanges = CheckExampleMapForChangesWithRules(lHeader.Examples.Value, rHeader.Examples.Value,
289+
&changes, v3.ExamplesLabel, CompHeader, PropExamples)
287290

288291
// content
289292
hc.ContentChanges = CheckMapForChanges(lHeader.Content.Value, rHeader.Content.Value,

what-changed/model/header_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,3 +346,39 @@ func TestCompareHeaders_v3_ExamplesRemoved(t *testing.T) {
346346
assert.Equal(t, 0, extChanges.TotalBreakingChanges())
347347
assert.Equal(t, ObjectRemoved, extChanges.Changes[0].ChangeType)
348348
}
349+
350+
func TestCompareHeaders_v3_ExamplesModified_WithCustomBreakingRule(t *testing.T) {
351+
ResetDefaultBreakingRules()
352+
ResetActiveBreakingRulesConfig()
353+
defer ResetDefaultBreakingRules()
354+
defer ResetActiveBreakingRulesConfig()
355+
356+
config := GenerateDefaultBreakingRules()
357+
config.Header.Examples = rule(false, true, false)
358+
SetActiveBreakingRulesConfig(config)
359+
360+
low.ClearHashCache()
361+
left := `examples:
362+
something:
363+
value: left header`
364+
right := `examples:
365+
something:
366+
value: right header`
367+
368+
var lNode, rNode yaml.Node
369+
_ = yaml.Unmarshal([]byte(left), &lNode)
370+
_ = yaml.Unmarshal([]byte(right), &rNode)
371+
372+
var lDoc v3.Header
373+
var rDoc v3.Header
374+
_ = low.BuildModel(lNode.Content[0], &lDoc)
375+
_ = low.BuildModel(rNode.Content[0], &rDoc)
376+
_ = lDoc.Build(context.Background(), nil, lNode.Content[0], nil)
377+
_ = rDoc.Build(context.Background(), nil, rNode.Content[0], nil)
378+
379+
extChanges := CompareHeadersV3(&lDoc, &rDoc)
380+
assert.Equal(t, 1, extChanges.TotalChanges())
381+
assert.Equal(t, 1, extChanges.TotalBreakingChanges())
382+
assert.Len(t, extChanges.ExamplesChanges, 1)
383+
assert.True(t, extChanges.ExamplesChanges["something"].GetAllChanges()[0].Breaking)
384+
}

what-changed/model/media_type.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,8 @@ func CompareMediaTypes(l, r *v3.MediaType) *MediaTypeChanges {
140140
}
141141

142142
// examples - use nil-aware version so added/removed examples appear in the map for tree rendering
143-
mc.ExampleChanges = CheckMapForChangesWithNilSupportAndRules(l.Examples.Value, r.Examples.Value,
144-
&changes, v3.ExamplesLabel, CompareExamples, CompMediaType, PropExamples)
143+
mc.ExampleChanges = CheckExampleMapForChangesWithNilSupportAndRules(l.Examples.Value, r.Examples.Value,
144+
&changes, v3.ExamplesLabel, CompMediaType, PropExamples)
145145

146146
// encoding
147147
mc.EncodingChanges = CheckMapForChanges(l.Encoding.Value, r.Encoding.Value,

0 commit comments

Comments
 (0)