Prefer rhs ordering while merging list. (#203)

* add list merge ordering test.

* implement new rhs-preferred ordering.

- merge and add each child of rhs
- append each remaining child of lhs

* update ordering in tests

to reflect new ordering algorithm.

* simplify merging algorithm.

* simplify again.

* additional tests for ordered merging.

Author: jiahuif

typed/merge.go

@@ -172,55 +172,51 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
 	}
 	out := make([]interface{}, 0, int(math.Max(float64(rLen), float64(lLen))))
 
-	// TODO: ordering is totally wrong.
-	// TODO: might as well make the map order work the same way.
+	lhsOrder := make([]fieldpath.PathElement, 0, lLen)
 
-	// This is a cheap hack to at least make the output order stable.
-	rhsOrder := make([]fieldpath.PathElement, 0, rLen)
-
-	// First, collect all RHS children.
-	observedRHS := fieldpath.MakePathElementValueMap(rLen)
-	if rhs != nil {
-		for i := 0; i < rhs.Length(); i++ {
-			child := rhs.At(i)
+	// First, collect all LHS children.
+	observedLHS := fieldpath.MakePathElementValueMap(lLen)
+	if lhs != nil {
+		for i := 0; i < lhs.Length(); i++ {
+			child := lhs.At(i)
 			pe, err := listItemToPathElement(w.allocator, w.schema, t, i, child)
 			if err != nil {
-				errs = append(errs, errorf("rhs: element %v: %v", i, err.Error())...)
+				errs = append(errs, errorf("lhs: 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
 			}
-			if _, ok := observedRHS.Get(pe); ok {
-				errs = append(errs, errorf("rhs: duplicate entries for key %v", pe.String())...)
+			if _, ok := observedLHS.Get(pe); ok {
+				errs = append(errs, errorf("lhs: duplicate entries for key %v", pe.String())...)
 			}
-			observedRHS.Insert(pe, child)
-			rhsOrder = append(rhsOrder, pe)
+			observedLHS.Insert(pe, child)
+			lhsOrder = append(lhsOrder, pe)
 		}
 	}
 
-	// Then merge with LHS children.
-	observedLHS := fieldpath.MakePathElementSet(lLen)
-	if lhs != nil {
-		for i := 0; i < lhs.Length(); i++ {
-			child := lhs.At(i)
+	// Then merge with RHS children.
+	observedRHS := fieldpath.MakePathElementSet(rLen)
+	if rhs != nil {
+		for i := 0; i < rhs.Length(); i++ {
+			child := rhs.At(i)
 			pe, err := listItemToPathElement(w.allocator, w.schema, t, i, child)
 			if err != nil {
-				errs = append(errs, errorf("lhs: element %v: %v", i, err.Error())...)
+				errs = append(errs, errorf("rhs: 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
 			}
-			if observedLHS.Has(pe) {
-				errs = append(errs, errorf("lhs: duplicate entries for key %v", pe.String())...)
+			if observedRHS.Has(pe) {
+				errs = append(errs, errorf("rhs: duplicate entries for key %v", pe.String())...)
 				continue
 			}
-			observedLHS.Insert(pe)
+			observedRHS.Insert(pe)
 			w2 := w.prepareDescent(pe, t.ElementType)
-			w2.lhs = value.Value(child)
-			if rchild, ok := observedRHS.Get(pe); ok {
-				w2.rhs = rchild
+			w2.rhs = child
+			if lchild, ok := observedLHS.Get(pe); ok {
+				w2.lhs = lchild
 			}
 			errs = append(errs, w2.merge(pe.String)...)
 			if w2.out != nil {
@@ -230,13 +226,13 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
 		}
 	}
 
-	for _, pe := range rhsOrder {
-		if observedLHS.Has(pe) {
+	for _, pe := range lhsOrder {
+		if observedRHS.Has(pe) {
 			continue
 		}
-		value, _ := observedRHS.Get(pe)
+		value, _ := observedLHS.Get(pe)
 		w2 := w.prepareDescent(pe, t.ElementType)
-		w2.rhs = value
+		w2.lhs = value
 		errs = append(errs, w2.merge(pe.String)...)
 		if w2.out != nil {
 			out = append(out, *w2.out)

typed/merge_test.go

@@ -283,12 +283,11 @@ var mergeCases = []mergeTestCase{{
 	}, {
 		`{"setBool":[true]}`,
 		`{"setBool":[false]}`,
-		`{"setBool":[true,false]}`,
+		`{"setBool":[false,true]}`,
 	}, {
 		`{"setNumeric":[1,2,3.14159]}`,
 		`{"setNumeric":[1,2,3]}`,
-		// KNOWN BUG: this order is wrong
-		`{"setNumeric":[1,2,3.14159,3]}`,
+		`{"setNumeric":[1,2,3,3.14159]}`,
 	}},
 }, {
 	name:         "associative list",
@@ -346,11 +345,23 @@ var mergeCases = []mergeTestCase{{
 	}, {
 		`{"list":[{"key":"a","id":1,"value":{"a":"a"}}]}`,
 		`{"list":[{"key":"a","id":2,"value":{"a":"a"}}]}`,
-		`{"list":[{"key":"a","id":1,"value":{"a":"a"}},{"key":"a","id":2,"value":{"a":"a"}}]}`,
+		`{"list":[{"key":"a","id":2,"value":{"a":"a"}},{"key":"a","id":1,"value":{"a":"a"}}]}`,
 	}, {
 		`{"list":[{"key":"a","id":1},{"key":"b","id":1}]}`,
 		`{"list":[{"key":"a","id":1},{"key":"a","id":2}]}`,
-		`{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":2}]}`,
+		`{"list":[{"key":"a","id":1},{"key":"a","id":2},{"key":"b","id":1}]}`,
+	}, {
+		`{"list":[{"key":"b","id":2}]}`,
+		`{"list":[{"key":"a","id":1},{"key":"b","id":2},{"key":"c","id":3}]}`,
+		`{"list":[{"key":"a","id":1},{"key":"b","id":2},{"key":"c","id":3}]}`,
+	}, {
+		`{"list":[{"key":"a","id":1},{"key":"b","id":2},{"key":"c","id":3}]}`,
+		`{"list":[{"key":"c","id":3},{"key":"b","id":2}]}`,
+		`{"list":[{"key":"c","id":3},{"key":"b","id":2},{"key":"a","id":1}]}`,
+	}, {
+		`{"list":[{"key":"a","id":1},{"key":"b","id":2},{"key":"c","id":3}]}`,
+		`{"list":[{"key":"c","id":3},{"key":"a","id":1}]}`,
+		`{"list":[{"key":"c","id":3},{"key":"a","id":1},{"key":"b","id":2}]}`,
 	}, {
 		`{"atomicList":["a","a","a"]}`,
 		`{"atomicList":null}`,