Enhancement Request: Add support for 'upserting' values

[dcwangmit01]

The current code is able to change values for selector paths which already exist in the obj. It would be nice if there were support for 'upsert' which would update a field if the selector path already exists, OR add a field and selector path if the path does not exist.

This following works, because the selector path ["a", "b"] already exists in the 'obj' {a: {b: "before"}.

m.change(["a", "b"], "after")({a: {b: "before"})

The result is:

{"a": {"b": "after"}}

It would be nice if

m.upsert(["a", "b"], "after")({}) // Note the empty obj({})

Would produce the same result as above, even though the selector path ["a", "b"] does not exist in the obj {}.

[dcwangmit01]

I got it to work, but I'm certain I don't fully understand the code. I'm new to jsonnet and only hacking here.

colordiff -u5 modifiers.libsonnet upsert_modifiers.libsonnet
--- modifiers.libsonnet 2019-11-14 00:45:19.000000000 +0000
+++ upsert_modifiers.libsonnet  2019-11-14 00:44:52.000000000 +0000
@@ -35,11 +35,13 @@

 local override(field) = function(modifier) function(obj)
   assert std.isString(field);
   assert std.isFunction(modifier) : 'modifier is not a function: ' + modifier;
   assert std.isObject(obj);
-  obj { [field]: modifier(super[field]) }
+  //  obj { [field]: modifier(super[field]) }
+  local x = if field in obj then obj[field] else {};
+  obj { [field]+: modifier(x) }
 ;

 local elem(n) = function(modifier) function(arr)
   local changeIfNth(index, elem) = if index == n then modifier(elem) else elem;
   std.mapWithIndex(changeIfNth, arr);

If the above makes sense, I can submit a PR that adds an upsert function which calls a newnormalizeUpsert which calles an overrideUpsert function.

[dcwangmit01]

Here's what the PR might look like:

diff --git a/helm/modifiers.libsonnet b/helm/modifiers.libsonnet
index afbc32f..1201a91 100644
--- a/helm/modifiers.libsonnet
+++ b/helm/modifiers.libsonnet
@@ -40,6 +40,14 @@ local override(field) = function(modifier) function(obj)
   obj { [field]: modifier(super[field]) }
 ;

+local overrideUpsert(field) = function(modifier) function(obj)
+  assert std.isString(field);
+  assert std.isFunction(modifier) : 'modifier is not a function: ' + modifier;
+  assert std.isObject(obj);
+  local x = if field in obj then obj[field] else {};
+  obj { [field]+: modifier(x) }
+;
+
 local elem(n) = function(modifier) function(arr)
   local changeIfNth(index, elem) = if index == n then modifier(elem) else elem;
   std.mapWithIndex(changeIfNth, arr);
@@ -74,6 +82,16 @@ local normalize(val) =
     error ('Unexpected value: ' + val)
 ;

+local normalizeUpsert(val) =
+  if std.isString(val) then
+    overrideUpsert(val)
+  else if std.isNumber(val) then
+    elem(val)
+  else if std.isFunction(val) then
+    val
+  else
+    error ('Unexpected value: ' + val)
+;

 // TODO(sbarzowski) come up with better name
 local build(ops) =
@@ -84,6 +102,14 @@ local build(ops) =
   foldr1(apply, ops)
 ;

+local buildUpsert(ops) =
+  assert std.isArray(ops);
+  local o = std.map(normalizeUpsert, ops);
+  local ops = o;
+
+  foldr1(apply, ops)
+;
+
 local change(ops, val) =
   build(ops + [const(val)])
 ;
@@ -91,6 +117,14 @@ local change(ops, val) =
 local changeWith(ops, fun) =
   build(ops + [fun])
 ;
+
+local upsert(ops, val) =
+  buildUpsert(ops + [const(val)])
+;
+
+local upsertWith(ops, fun) =
+  buildUpsert(ops + [fun])
+;
 {
   override:: override,
   map:: map,
@@ -101,4 +135,6 @@ local changeWith(ops, fun) =
   build:: build,
   change:: change,
   changeWith:: changeWith,
+  upsert:: upsert,
+  upsertWith:: upsertWith,
 }

[sbarzowski]

Thank you for the proposal. This is a very good idea. We may want to think about the details, but it's pretty close.

This is very much related to #1. Actually this functionality is listed there explicitly as "implicit creating of nonexistent fields when modifying".

Some notes:

1. We may want to introduce a user-facing concept of "normalizer" (such as your - "normalizeUpsert" and existing "normalize"). Perhaps it's possible to (later) cover all the other operations listed in Support for checking existence of objects #1 this way....

2. In the code, I would add a normalizeFunc parameter to build instead of creating a new function buildUpsert. This breaks compatibility, but it's easy to fix and I put a clear warning that it's experimental for occasions like this (also, probably no one is using this function directly). This makes sense especially if more normalizers are coming.

3. I think there is a subtle bug in obj { [field]+: modifier(x) } - it should be just obj { [field]: modifier(x) } - with : instead of +:. Think about what would happen if someone ran m.upsert(["a"], null)({a: {b: 42}}).

4. I wouldn't use {} as sentinel value when accessed field doesn't exist here: local x = if field in obj then obj[field] else {};. Instead I would use null¹.

5. We may want to think about the names (both new and existing). In particular, I'm not a big fan of overrideUpsert (both override and upsert are verbs - and operations - so it is a bit weird). Perhaps upsertField?

If you want to go forward with this, please submit a PR and we can work more on this there.

¹ A really proper way to do it would be to add an explicit marker (e.g. return a tuple of a value and a boolean isFound), but that would require selectors further down the line to be aware of that, so I think null is a reasonable compromise in this case. And if someone really needs to distinguish null field vs nonexistent field they can write a custom selectors which do the necessary wrapping/unwrapping.

[dcwangmit01]

@sbarzowski Thanks for the reply. I need to reserve some time to fully understand the library and jsonnet in general. I think you've given me enough hints to make a PR. Hoping to do that soon.

[sbarzowski]

Awesome, please let me know if I can help :-). Please don't worry about applying (1) and (5) from my previous comment - I just wanted to let you know where I want to go with the lib and these things deserve separate PRs anyway. The diff you posted is just a few minor changes away from being ready.