Skip to content

Commit 8f742cc

Browse files
apelisseapelisse
committed
typed: Update compare algorithm to handle duplicates
1 parent 66711bf commit 8f742cc

File tree

2 files changed

+165
-26
lines changed

2 files changed

+165
-26
lines changed

Diff for: ‎typed/compare.go

+81-24
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,15 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
213213
lLen = lhs.Length()
214214
}
215215

216+
maxLength := rLen
217+
if lLen > maxLength {
218+
maxLength = lLen
219+
}
220+
// Contains all the unique PEs between lhs and rhs, exactly once.
221+
// Order doesn't matter since we're just tracking ownership in a set.
222+
allPEs := make([]fieldpath.PathElement, 0, maxLength)
223+
224+
// Gather all the elements from lhs, indexed by PE, in a list for duplicates.
216225
lValues := fieldpath.MakePathElementValueMap(lLen)
217226
for i := 0; i < lLen; i++ {
218227
child := lhs.At(i)
@@ -224,13 +233,17 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
224233
// this element.
225234
continue
226235
}
227-
// Ignore repeated occurences of `pe`.
228-
if _, found := lValues.Get(pe); found {
229-
continue
236+
237+
if v, found := lValues.Get(pe); found {
238+
list := v.Unstructured().([]value.Value)
239+
lValues.Insert(pe, value.NewValueInterface(append(list, child)))
240+
} else {
241+
lValues.Insert(pe, value.NewValueInterface([]value.Value{child}))
242+
allPEs = append(allPEs, pe)
230243
}
231-
lValues.Insert(pe, child)
232244
}
233245

246+
// Gather all the elements from rhs, indexed by PE, in a list for duplicates.
234247
rValues := fieldpath.MakePathElementValueMap(rLen)
235248
for i := 0; i < rLen; i++ {
236249
rValue := rhs.At(i)
@@ -242,31 +255,75 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
242255
// this element.
243256
continue
244257
}
245-
// Ignore repeated occurences of `pe`.
246-
if _, found := rValues.Get(pe); found {
247-
continue
258+
if v, found := rValues.Get(pe); found {
259+
list := v.Unstructured().([]value.Value)
260+
rValues.Insert(pe, value.NewValueInterface(append(list, rValue)))
261+
} else {
262+
rValues.Insert(pe, value.NewValueInterface([]value.Value{rValue}))
263+
if _, found := lValues.Get(pe); !found {
264+
allPEs = append(allPEs, pe)
265+
}
248266
}
249-
rValues.Insert(pe, rValue)
250-
// We can compare with nil if lValue is not present.
251-
lValue, _ := lValues.Get(pe)
252-
errs = append(errs, w.compareListItem(t, pe, lValue, rValue)...)
253267
}
254268

255-
// Add items from left that are not in right.
256-
for i := 0; i < lLen; i++ {
257-
lValue := lhs.At(i)
258-
pe, err := listItemToPathElement(w.allocator, w.schema, t, lValue)
259-
if err != nil {
260-
errs = append(errs, errorf("element %v: %v", i, err.Error())...)
261-
// If we can't construct the path element, we can't
262-
// even report errors deeper in the schema, so bail on
263-
// this element.
264-
continue
269+
for _, pe := range allPEs {
270+
lList := []value.Value(nil)
271+
if l, ok := lValues.Get(pe); ok {
272+
lList = l.Unstructured().([]value.Value)
265273
}
266-
if _, found := rValues.Get(pe); found {
267-
continue
274+
rList := []value.Value(nil)
275+
if l, ok := rValues.Get(pe); ok {
276+
rList = l.Unstructured().([]value.Value)
277+
}
278+
279+
switch {
280+
case len(lList) == 0 && len(rList) == 0:
281+
// We shouldn't be here anyway.
282+
return
283+
// Normal use-case:
284+
// We have no duplicates for this PE, compare items one-to-one.
285+
case len(lList) <= 1 && len(rList) <= 1:
286+
lValue := value.Value(nil)
287+
if len(lList) != 0 {
288+
lValue = lList[0]
289+
}
290+
rValue := value.Value(nil)
291+
if len(rList) != 0 {
292+
rValue = rList[0]
293+
}
294+
errs = append(errs, w.compareListItem(t, pe, lValue, rValue)...)
295+
// Duplicates before & after use-case:
296+
// Compare the duplicates lists as if they were atomic, mark modified if they changed.
297+
case len(lList) >= 2 && len(rList) >= 2:
298+
listEqual := func(lList, rList []value.Value) bool {
299+
if len(lList) != len(rList) {
300+
return false
301+
}
302+
for i := range lList {
303+
if !value.Equals(lList[i], rList[i]) {
304+
return false
305+
}
306+
}
307+
return true
308+
}
309+
if !listEqual(lList, rList) {
310+
w.comparison.Modified.Insert(append(w.path, pe))
311+
}
312+
// Duplicates before & not anymore use-case:
313+
// Rcursively add new non-duplicate items, Remove duplicate marker,
314+
case len(lList) >= 2:
315+
if len(rList) != 0 {
316+
errs = append(errs, w.compareListItem(t, pe, nil, rList[0])...)
317+
}
318+
w.comparison.Removed.Insert(append(w.path, pe))
319+
// New duplicates use-case:
320+
// Recursively remove old non-duplicate items, add duplicate marker.
321+
case len(rList) >= 2:
322+
if len(lList) != 0 {
323+
errs = append(errs, w.compareListItem(t, pe, lList[0], nil)...)
324+
}
325+
w.comparison.Added.Insert(append(w.path, pe))
268326
}
269-
errs = append(errs, w.compareListItem(t, pe, lValue, nil)...)
270327
}
271328

272329
return

Diff for: ‎typed/symdiff_test.go

+84-2
Original file line numberDiff line numberDiff line change
@@ -596,6 +596,24 @@ var symdiffCases = []symdiffTestCase{{
596596
removed: _NS(),
597597
modified: _NS(),
598598
added: _NS(_P("setStr", _V("c"))),
599+
}, {
600+
lhs: `{"setStr":["a"]}`,
601+
rhs: `{"setStr":["a","b","b"]}`,
602+
removed: _NS(),
603+
modified: _NS(),
604+
added: _NS(_P("setStr", _V("b"))),
605+
}, {
606+
lhs: `{"setStr":["a","b"]}`,
607+
rhs: `{"setStr":["a","b","b"]}`,
608+
removed: _NS(_P("setStr", _V("b"))),
609+
modified: _NS(),
610+
added: _NS(_P("setStr", _V("b"))),
611+
}, {
612+
lhs: `{"setStr":["b","b"]}`,
613+
rhs: `{"setStr":["a","b","b"]}`,
614+
removed: _NS(),
615+
modified: _NS(),
616+
added: _NS(_P("setStr", _V("a"))),
599617
}, {
600618
lhs: `{"setStr":["a","b","c"]}`,
601619
rhs: `{"setStr":[]}`,
@@ -618,6 +636,28 @@ var symdiffCases = []symdiffTestCase{{
618636
removed: _NS(_P("setNumeric", _V(3.14159))),
619637
modified: _NS(),
620638
added: _NS(_P("setNumeric", _V(3))),
639+
}, {
640+
lhs: `{"setStr":["a","b","b","c","a"]}`,
641+
rhs: `{"setStr":[]}`,
642+
removed: _NS(
643+
_P("setStr", _V("a")),
644+
_P("setStr", _V("b")),
645+
_P("setStr", _V("c")),
646+
),
647+
modified: _NS(),
648+
added: _NS(),
649+
}, {
650+
lhs: `{"setBool":[true,true]}`,
651+
rhs: `{"setBool":[false]}`,
652+
removed: _NS(_P("setBool", _V(true))),
653+
modified: _NS(),
654+
added: _NS(_P("setBool", _V(false))),
655+
}, {
656+
lhs: `{"setNumeric":[1,2,2,3.14159,1]}`,
657+
rhs: `{"setNumeric":[1,2,3]}`,
658+
removed: _NS(_P("setNumeric", _V(1)), _P("setNumeric", _V(2)), _P("setNumeric", _V(3.14159))),
659+
modified: _NS(),
660+
added: _NS(_P("setNumeric", _V(1)), _P("setNumeric", _V(2)), _P("setNumeric", _V(3))),
621661
}},
622662
}, {
623663
name: "associative list",
@@ -743,6 +783,48 @@ var symdiffCases = []symdiffTestCase{{
743783
removed: _NS(),
744784
modified: _NS(_P("atomicList")),
745785
added: _NS(),
786+
}, {
787+
lhs: `{"list":[{"key":"a","id":1,"nv":2},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
788+
rhs: `{"list":[{"key":"a","id":1},{"key":"a","id":2}]}`,
789+
removed: _NS(
790+
_P("list", _KBF("key", "b", "id", 1)),
791+
_P("list", _KBF("key", "b", "id", 1), "key"),
792+
_P("list", _KBF("key", "b", "id", 1), "id"),
793+
_P("list", _KBF("key", "a", "id", 1)),
794+
),
795+
modified: _NS(),
796+
added: _NS(
797+
_P("list", _KBF("key", "a", "id", 1)),
798+
_P("list", _KBF("key", "a", "id", 1), "key"),
799+
_P("list", _KBF("key", "a", "id", 1), "id"),
800+
_P("list", _KBF("key", "a", "id", 2)),
801+
_P("list", _KBF("key", "a", "id", 2), "key"),
802+
_P("list", _KBF("key", "a", "id", 2), "id"),
803+
),
804+
}, {
805+
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
806+
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true,"nv":1}]}`,
807+
removed: _NS(),
808+
modified: _NS(_P("list", _KBF("key", "a", "id", 1))),
809+
added: _NS(),
810+
}, {
811+
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
812+
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
813+
removed: _NS(),
814+
modified: _NS(),
815+
added: _NS(),
816+
}, {
817+
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
818+
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1,"bv":true},{"key":"a","id":1,"bv":true}]}`,
819+
removed: _NS(),
820+
modified: _NS(),
821+
added: _NS(_P("list", _KBF("key", "b", "id", 1), "bv")),
822+
}, {
823+
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
824+
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}],"atomicList":["unrelated"]}`,
825+
removed: _NS(),
826+
modified: _NS(),
827+
added: _NS(_P("atomicList")),
746828
}},
747829
}}
748830

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

760-
tvLHS, err := pt.FromYAML(quint.lhs)
842+
tvLHS, err := pt.FromYAML(quint.lhs, typed.AllowDuplicates)
761843
if err != nil {
762844
t.Fatalf("failed to parse lhs: %v", err)
763845
}
764-
tvRHS, err := pt.FromYAML(quint.rhs)
846+
tvRHS, err := pt.FromYAML(quint.rhs, typed.AllowDuplicates)
765847
if err != nil {
766848
t.Fatalf("failed to parse rhs: %v", err)
767849
}

0 commit comments

Comments
 (0)