diff --git a/fieldpath/pathelementmap.go b/fieldpath/pathelementmap.go index 2611a228..41fc2474 100644 --- a/fieldpath/pathelementmap.go +++ b/fieldpath/pathelementmap.go @@ -28,20 +28,15 @@ import ( // for PathElementSet and SetNodeMap, so we could probably share the // code. type PathElementValueMap struct { - members sortedPathElementValues + valueMap PathElementMap } func MakePathElementValueMap(size int) PathElementValueMap { return PathElementValueMap{ - members: make(sortedPathElementValues, 0, size), + valueMap: MakePathElementMap(size), } } -type pathElementValue struct { - PathElement PathElement - Value value.Value -} - type sortedPathElementValues []pathElementValue // Implement the sort interface; this would permit bulk creation, which would @@ -55,6 +50,38 @@ func (spev sortedPathElementValues) Swap(i, j int) { spev[i], spev[j] = spev[j], // Insert adds the pathelement and associated value in the map. // If insert is called twice with the same PathElement, the value is replaced. func (s *PathElementValueMap) Insert(pe PathElement, v value.Value) { + s.valueMap.Insert(pe, v) +} + +// Get retrieves the value associated with the given PathElement from the map. +// (nil, false) is returned if there is no such PathElement. +func (s *PathElementValueMap) Get(pe PathElement) (value.Value, bool) { + v, ok := s.valueMap.Get(pe) + if !ok { + return nil, false + } + return v.(value.Value), true +} + +// PathElementValueMap is a map from PathElement to interface{}. +type PathElementMap struct { + members sortedPathElementValues +} + +type pathElementValue struct { + PathElement PathElement + Value interface{} +} + +func MakePathElementMap(size int) PathElementMap { + return PathElementMap{ + members: make(sortedPathElementValues, 0, size), + } +} + +// Insert adds the pathelement and associated value in the map. +// If insert is called twice with the same PathElement, the value is replaced. +func (s *PathElementMap) Insert(pe PathElement, v interface{}) { loc := sort.Search(len(s.members), func(i int) bool { return !s.members[i].PathElement.Less(pe) }) @@ -73,7 +100,7 @@ func (s *PathElementValueMap) Insert(pe PathElement, v value.Value) { // Get retrieves the value associated with the given PathElement from the map. // (nil, false) is returned if there is no such PathElement. -func (s *PathElementValueMap) Get(pe PathElement) (value.Value, bool) { +func (s *PathElementMap) Get(pe PathElement) (interface{}, bool) { loc := sort.Search(len(s.members), func(i int) bool { return !s.members[i].PathElement.Less(pe) }) diff --git a/typed/compare.go b/typed/compare.go index 6fb398ae..ed483cbb 100644 --- a/typed/compare.go +++ b/typed/compare.go @@ -213,7 +213,16 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err lLen = lhs.Length() } - lValues := fieldpath.MakePathElementValueMap(lLen) + maxLength := rLen + if lLen > maxLength { + maxLength = lLen + } + // Contains all the unique PEs between lhs and rhs, exactly once. + // Order doesn't matter since we're just tracking ownership in a set. + allPEs := make([]fieldpath.PathElement, 0, maxLength) + + // Gather all the elements from lhs, indexed by PE, in a list for duplicates. + lValues := fieldpath.MakePathElementMap(lLen) for i := 0; i < lLen; i++ { child := lhs.At(i) pe, err := listItemToPathElement(w.allocator, w.schema, t, child) @@ -224,14 +233,18 @@ 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.([]value.Value) + lValues.Insert(pe, append(list, child)) + } else { + lValues.Insert(pe, []value.Value{child}) + allPEs = append(allPEs, pe) } - lValues.Insert(pe, child) } - rValues := fieldpath.MakePathElementValueMap(rLen) + // Gather all the elements from rhs, indexed by PE, in a list for duplicates. + rValues := fieldpath.MakePathElementMap(rLen) for i := 0; i < rLen; i++ { rValue := rhs.At(i) pe, err := listItemToPathElement(w.allocator, w.schema, t, rValue) @@ -242,31 +255,75 @@ 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.([]value.Value) + rValues.Insert(pe, append(list, rValue)) + } else { + rValues.Insert(pe, []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.([]value.Value) } - if _, found := rValues.Get(pe); found { - continue + rList := []value.Value(nil) + if l, ok := rValues.Get(pe); ok { + rList = l.([]value.Value) + } + + switch { + case len(lList) == 0 && len(rList) == 0: + // We shouldn't be here anyway. + return + // Normal use-case: + // We have no duplicates for this PE, compare items one-to-one. + 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)...) + // Duplicates before & after use-case: + // Compare the duplicates lists as if they were atomic, mark modified if they changed. + 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)) + } + // Duplicates before & not anymore use-case: + // Rcursively add new non-duplicate items, Remove duplicate marker, + 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)) + // New duplicates use-case: + // Recursively remove old non-duplicate items, add duplicate marker. + 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 diff --git a/typed/symdiff_test.go b/typed/symdiff_test.go index 57751a73..0ef15a2c 100644 --- a/typed/symdiff_test.go +++ b/typed/symdiff_test.go @@ -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":[]}`, @@ -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", @@ -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")), }}, }} @@ -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) }