Fix a bug in hidden field handling in std.mergePatch (#250)

This PR fixes a bug related to handling of hidden fields in
`std.mergePatch`:

In both google/jsonnet and google/go-jsonnet, hidden fields are
ineligible to participate in field merges:
- `std.mergePatch({a: 1}, {a :: 2}`
  - should return `{a: 1}` (ignoring the hidden patch field)
  - sjsonnet returns `{a: 2}`
- `std.mergePatch({a: {a: 1}}, {a:: {b: 1}}`
  - should return `{a: {a: 1}}`
  - sjsonnet returns `{a: {a: 1, b: 1}}`
- `std.mergePatch({a:: {a: 1}}, {a: {b: 1}}` (symmetrical to previous
case, but swaps hidden between target and patch)
  - should return `{a: {b: 1}}`
  - sjsonnet returns `{a: {a: 1, b: 1}}`

The problem is that the current code iterates over the union of visible
fields, but never re-checks the specific visibility in target and patch
during per-field merges.

This PR fixes that bug and significantly expands unit test coverage for
`std.mergePatch`.

sjsonnet/src/sjsonnet/Std.scala

@@ -916,8 +916,8 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map.
         case (l: Val.Obj, r: Val.Obj) =>
           val kvs = for {
             k <- (l.visibleKeyNames ++ r.visibleKeyNames).distinct
-            lValue = Option(l.valueRaw(k, l, pos)(ev))
-            rValue = Option(r.valueRaw(k, r, pos)(ev))
+            lValue = if (l.containsVisibleKey(k)) Option(l.valueRaw(k, l, pos)(ev)) else None
+            rValue = if (r.containsVisibleKey(k)) Option(r.valueRaw(k, r, pos)(ev)) else None
             if !rValue.exists(_.isInstanceOf[Val.Null])
           } yield (lValue, rValue) match{
             case (Some(lChild), None) => k -> createMember{lChild}

sjsonnet/test/src/sjsonnet/StdMergePatchTests.scala

@@ -4,26 +4,92 @@ import utest._
 import TestUtils.eval
 object StdMergePatchTests extends TestSuite {
 
+  // These test cases' behavior matches v0.20.0 of google/jsonnet and google/go-jsonnet.
+
   def tests = Tests {
-    test {
+
+    test("top-level merging of non-objects") {
+      // Both target and patch are non-objects, so patch wins:
       eval("std.mergePatch([{a: 1}], [{b: 2}])") ==> ujson.Arr(ujson.Obj("b" -> 2))
+      // Target is an object, patch is an array, so patch wins:
+      eval("std.mergePatch({a: 1}, [{b: 2}])") ==> ujson.Arr(ujson.Obj("b" -> 2))
+      // Target is an array, patch is an object, so patch wins:
+      eval("std.mergePatch([{a: 1}], {b: 2})") ==> ujson.Obj("b" -> 2)
     }
-    test {
+
+    test("top-level nulls") {
+      // Target is null, so patch wins:
       eval("std.mergePatch(null, {a: 1})") ==> ujson.Obj("a" -> 1)
+      // A null patch always produces a null result:
+      eval("std.mergePatch({a: 1}, null)") ==> ujson.Null
+    }
+
+    test("basic merges") {
+      // Basic non-conflicting merge of top-level fields:
+      eval("std.mergePatch({a: 1}, {b: 2})") ==> ujson.Obj("a" -> 1, "b" -> 2)
+      // Basic conflicting merge of top-level fields (patch wins)
+      eval("std.mergePatch({a: 1}, {a: 2})") ==> ujson.Obj("a" -> 2)
+      // Basic recursive non-conflicting merging:
+      eval("std.mergePatch({a: {b: 1}}, {a: {c: 2}})") ==> ujson.Obj("a" -> ujson.Obj("b" -> 1, "c" -> 2))
+      // Basic recursive conflicting merging (patch wins):
+      eval("std.mergePatch({a: {b: 1, c: 1}}, {a: {b: 2}})") ==> ujson.Obj("a" -> ujson.Obj("b" -> 2, "c" -> 1))
+    }
+
+    test("target field order preservation") {
+      eval("std.mergePatch({b: 1, a: 1}, {a: 2, b: 2})", preserveOrder = true).toString ==> """{"b":2,"a":2}"""
     }
-    test {
+
+    test("null fields") {
+      // Nested nulls in patch can remove nested fields from target
+      eval("std.mergePatch({a: {b: 1, c: 1}}, {a: {b: null}})") ==> ujson.Obj("a" -> ujson.Obj("c" -> 1))
+      // Nested nulls in the target are preserved:
       eval("std.mergePatch({a: null}, {b: 2})") ==> ujson.Obj("a" -> ujson.Null, "b" -> 2)
     }
-    test {
-      eval("{a:: 1} + std.mergePatch({}, {a: 2})") ==> ujson.Obj("a" -> 2)
+
+    // Regarding hidden field behavior in other implementations, see also:
+    // https://github.com/google/jsonnet/issues/219
+    // https://github.com/google/jsonnet/issues/1041
+
+    test("hidden target fields") {
+      // Hidden target fields are dropped in the output, even if nothing merges with them:
+      eval("std.objectFieldsAll({hidden:: 1, visible: 1})") ==> ujson.Arr("hidden", "visible")
+      eval("std.objectFieldsAll(std.mergePatch({hidden:: 1, visible: 1}, {}))") ==> ujson.Arr("visible")
+      eval("std.objectFieldsAll(std.mergePatch({hidden:: 1, visible: 1}, {added: 1}))") ==> ujson.Arr("added", "visible")
+      // But hidden nested target fields are preserved as hidden if nothing merges with them:
+      eval("std.objectFields(std.mergePatch({a: {h:: 1, v: 1}}, {}).a)") ==> ujson.Arr("v")
+      eval("std.objectFieldsAll(std.mergePatch({a: {h:: 1, v: 1}}, {}).a)") ==> ujson.Arr("h", "v")
+      eval("std.mergePatch({a: {h:: 1, v: 1}}, {}).a.h") ==> ujson.Num(1)
+      // Those hidden nested fields are still dropped if something merges with their enclosing object:
+      eval("std.objectFieldsAll(std.mergePatch({a: {h:: 1, v: 1}}, {a: {}}).a)") ==> ujson.Arr("v")
+      // Hidden target fields are ineligible to merge with visible patch fields;
+      // it should be as if the hidden target field doesn't exist:
+      eval("std.mergePatch({ a:: { a: 1 } , visible: 1 }, { a: { b: 2 }})") ==> ujson.Obj("visible" -> 1, "a" -> ujson.Obj("b" -> 2))
     }
-    test {
-      eval("{a: 1} + std.mergePatch({}, {a+: 2})") ==> ujson.Obj("a" -> 2)
+
+    test("hidden patch fields") {
+      // Hidden patch fields are dropped in the output, even if nothing merges with them:
+      eval("std.objectFieldsAll(std.mergePatch({visible: 1}, {hidden:: 2}))") ==> ujson.Arr("visible")
+      // Hidden patch fields are ineligible to merge with visible target fields;
+      // it should be as if the hidden patch field doesn't exist:
+      eval("std.mergePatch({ a: 1 }, { a:: 2})") ==> ujson.Obj("a" -> 1)
+      // Nesting:
+      eval("std.mergePatch({ a: { b: 1 } }, { a:: { c: 1 }})") ==> ujson.Obj("a" -> ujson.Obj("b" -> 1))
+      // Make sure the nested hidden patch field is indeed absent, not just hidden:
+      eval("std.objectFieldsAll(std.mergePatch({ a: { b: 1 } }, { a:: { c: 1 }}).a)") ==> ujson.Arr("b")
     }
 
-    test {
-      eval("""std.mergePatch({"a": {b: "B"}}, {a: {c: "C"}})""") ==>
-        ujson.Obj("a" -> ujson.Obj("b" -> "B", "c" -> "C"))
+    test("plus is ignored during merge") {
+      // Ordinarily, the :+ operator would cause `+` to performed on fields during object inheritance:
+      eval("{a: 1} + {a+: 2}") ==> ujson.Obj("a" -> 3)
+      // But mergePatch intentionally does not consider `+:` fields to be special:
+      eval("std.mergePatch({a: 1}, {a+: 2})") ==> ujson.Obj("a" -> 2)
+      // We also need to check that the resulting output fields aren't treated as `+:` fields for
+      // the purposes of subsequent object inheritance:
+      eval("{a: 1} + std.mergePatch({}, {a+: 2})") ==> ujson.Obj("a" -> 2)
+      // The `+:` behavior is lost even if it is from the target:
+      eval("{a: 1} + std.mergePatch({a+: 2}, {})") ==> ujson.Obj("a" -> 2)
+      // It's also lost in nested fields:
+      eval("{a: {b: 1}} + std.mergePatch({a: {b +: 2}}, {})") ==> ujson.Obj("a" -> ujson.Obj("b" -> 2))
     }
   }
 }