Skip to content

Commit

Permalink
typed: Update compare algorithm to handle duplicates
Browse files Browse the repository at this point in the history
  • Loading branch information
@apelisse
apelisse committed Oct 9, 2023
1 parent 66711bf commit 7d04114
Show file tree
Hide file tree
Showing 2 changed files with 155 additions and 26 deletions.
95 changes: 71 additions & 24 deletions typed/compare.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,14 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
lLen = lhs.Length()
}

maxLength := rLen
if lLen > maxLength {
maxLength = lLen
}
// Contains all the unique PEs between lhs and rhs, exactly once.
allPEs := make([]fieldpath.PathElement, 0, maxLength)

// So for each value that we find, we save them in a list, we do that for both sides.
lValues := fieldpath.MakePathElementValueMap(lLen)
for i := 0; i < lLen; i++ {
child := lhs.At(i)
Expand All @@ -224,11 +232,14 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
// this element.
continue
}
// Ignore repeated occurences of `pe`.
if _, found := lValues.Get(pe); found {
continue

if v, found := lValues.Get(pe); found {
list := v.Unstructured().([]value.Value)
lValues.Insert(pe, value.NewValueInterface(append(list, child)))
} else {
lValues.Insert(pe, value.NewValueInterface([]value.Value{child}))
allPEs = append(allPEs, pe)
}
lValues.Insert(pe, child)
}

rValues := fieldpath.MakePathElementValueMap(rLen)
Expand All @@ -242,31 +253,67 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
// this element.
continue
}
// Ignore repeated occurences of `pe`.
if _, found := rValues.Get(pe); found {
continue
if v, found := rValues.Get(pe); found {
list := v.Unstructured().([]value.Value)
rValues.Insert(pe, value.NewValueInterface(append(list, rValue)))
} else {
rValues.Insert(pe, value.NewValueInterface([]value.Value{rValue}))
if _, found := lValues.Get(pe); !found {
allPEs = append(allPEs, pe)
}
}
rValues.Insert(pe, rValue)
// We can compare with nil if lValue is not present.
lValue, _ := lValues.Get(pe)
errs = append(errs, w.compareListItem(t, pe, lValue, rValue)...)
}

// Add items from left that are not in right.
for i := 0; i < lLen; i++ {
lValue := lhs.At(i)
pe, err := listItemToPathElement(w.allocator, w.schema, t, lValue)
if err != nil {
errs = append(errs, errorf("element %v: %v", i, err.Error())...)
// If we can't construct the path element, we can't
// even report errors deeper in the schema, so bail on
// this element.
continue
for _, pe := range allPEs {
lList := []value.Value(nil)
if l, ok := lValues.Get(pe); ok {
lList = l.Unstructured().([]value.Value)
}
if _, found := rValues.Get(pe); found {
continue
rList := []value.Value(nil)
if l, ok := rValues.Get(pe); ok {
rList = l.Unstructured().([]value.Value)
}

switch {
case len(lList) == 0 && len(rList) == 0:
// We shouldn't be here anyway.
return
case len(lList) <= 1 && len(rList) <= 1:
lValue := value.Value(nil)
if len(lList) != 0 {
lValue = lList[0]
}
rValue := value.Value(nil)
if len(rList) != 0 {
rValue = rList[0]
}
errs = append(errs, w.compareListItem(t, pe, lValue, rValue)...)
case len(lList) >= 2 && len(rList) >= 2:
listEqual := func(lList, rList []value.Value) bool {
if len(lList) != len(rList) {
return false
}
for i := range lList {
if !value.Equals(lList[i], rList[i]) {
return false
}
}
return true
}
if !listEqual(lList, rList) {
w.comparison.Modified.Insert(append(w.path, pe))
}
case len(lList) >= 2:
if len(rList) != 0 {
errs = append(errs, w.compareListItem(t, pe, nil, rList[0])...)
}
w.comparison.Removed.Insert(append(w.path, pe))
case len(rList) >= 2:
if len(lList) != 0 {
errs = append(errs, w.compareListItem(t, pe, lList[0], nil)...)
}
w.comparison.Added.Insert(append(w.path, pe))
}
errs = append(errs, w.compareListItem(t, pe, lValue, nil)...)
}

return
Expand Down
86 changes: 84 additions & 2 deletions typed/symdiff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,24 @@ var symdiffCases = []symdiffTestCase{{
removed: _NS(),
modified: _NS(),
added: _NS(_P("setStr", _V("c"))),
}, {
lhs: `{"setStr":["a"]}`,
rhs: `{"setStr":["a","b","b"]}`,
removed: _NS(),
modified: _NS(),
added: _NS(_P("setStr", _V("b"))),
}, {
lhs: `{"setStr":["a","b"]}`,
rhs: `{"setStr":["a","b","b"]}`,
removed: _NS(_P("setStr", _V("b"))),
modified: _NS(),
added: _NS(_P("setStr", _V("b"))),
}, {
lhs: `{"setStr":["b","b"]}`,
rhs: `{"setStr":["a","b","b"]}`,
removed: _NS(),
modified: _NS(),
added: _NS(_P("setStr", _V("a"))),
}, {
lhs: `{"setStr":["a","b","c"]}`,
rhs: `{"setStr":[]}`,
Expand All @@ -618,6 +636,28 @@ var symdiffCases = []symdiffTestCase{{
removed: _NS(_P("setNumeric", _V(3.14159))),
modified: _NS(),
added: _NS(_P("setNumeric", _V(3))),
}, {
lhs: `{"setStr":["a","b","b","c","a"]}`,
rhs: `{"setStr":[]}`,
removed: _NS(
_P("setStr", _V("a")),
_P("setStr", _V("b")),
_P("setStr", _V("c")),
),
modified: _NS(),
added: _NS(),
}, {
lhs: `{"setBool":[true,true]}`,
rhs: `{"setBool":[false]}`,
removed: _NS(_P("setBool", _V(true))),
modified: _NS(),
added: _NS(_P("setBool", _V(false))),
}, {
lhs: `{"setNumeric":[1,2,2,3.14159,1]}`,
rhs: `{"setNumeric":[1,2,3]}`,
removed: _NS(_P("setNumeric", _V(1)), _P("setNumeric", _V(2)), _P("setNumeric", _V(3.14159))),
modified: _NS(),
added: _NS(_P("setNumeric", _V(1)), _P("setNumeric", _V(2)), _P("setNumeric", _V(3))),
}},
}, {
name: "associative list",
Expand Down Expand Up @@ -743,6 +783,48 @@ var symdiffCases = []symdiffTestCase{{
removed: _NS(),
modified: _NS(_P("atomicList")),
added: _NS(),
}, {
lhs: `{"list":[{"key":"a","id":1,"nv":2},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"a","id":2}]}`,
removed: _NS(
_P("list", _KBF("key", "b", "id", 1)),
_P("list", _KBF("key", "b", "id", 1), "key"),
_P("list", _KBF("key", "b", "id", 1), "id"),
_P("list", _KBF("key", "a", "id", 1)),
),
modified: _NS(),
added: _NS(
_P("list", _KBF("key", "a", "id", 1)),
_P("list", _KBF("key", "a", "id", 1), "key"),
_P("list", _KBF("key", "a", "id", 1), "id"),
_P("list", _KBF("key", "a", "id", 2)),
_P("list", _KBF("key", "a", "id", 2), "key"),
_P("list", _KBF("key", "a", "id", 2), "id"),
),
}, {
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true,"nv":1}]}`,
removed: _NS(),
modified: _NS(_P("list", _KBF("key", "a", "id", 1))),
added: _NS(),
}, {
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
removed: _NS(),
modified: _NS(),
added: _NS(),
}, {
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1,"bv":true},{"key":"a","id":1,"bv":true}]}`,
removed: _NS(),
modified: _NS(),
added: _NS(_P("list", _KBF("key", "b", "id", 1), "bv")),
}, {
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}],"atomicList":["unrelated"]}`,
removed: _NS(),
modified: _NS(),
added: _NS(_P("atomicList")),
}},
}}

Expand All @@ -757,11 +839,11 @@ func (tt symdiffTestCase) test(t *testing.T) {
t.Parallel()
pt := parser.Type(tt.rootTypeName)

tvLHS, err := pt.FromYAML(quint.lhs)
tvLHS, err := pt.FromYAML(quint.lhs, typed.AllowDuplicates)
if err != nil {
t.Fatalf("failed to parse lhs: %v", err)
}
tvRHS, err := pt.FromYAML(quint.rhs)
tvRHS, err := pt.FromYAML(quint.rhs, typed.AllowDuplicates)
if err != nil {
t.Fatalf("failed to parse rhs: %v", err)
}
Expand Down

0 comments on commit 7d04114

Please sign in to comment.