patch command will not add values to null key

[rspurgeon]

Take this input config (input.yaml)

# This will be used as the base file for
# all default _plugin_configs.  The actual
# defaults will be patched in incrementally
_plugin_configs:

And this patch file (patch.yaml):

_format_version: "1.0"
patches:
  - selectors:
    - $._plugin_configs
    values:
      default-jwt:
        name: jwt
        config:
          enabled: true

Ran as:

deck file patch -s input.yaml patch.yaml

Results in:

_plugin_configs: null

If I change input.yaml to:

# This will be used as the base file for
# all default _plugin_configs.  The actual
# defaults will be patched in incrementally
_plugin_configs:
  dummy:

The result will be:

_plugin_configs:
  default-jwt:
    config:
      enabled: true
    name: jwt
  dummy: null

The patch command should be able to create a new key in an existing null valued key.

[rspurgeon]

Another option for the user that currently works is specifying an empty object with brackets

# This will be used as the base file for
# all default _plugin_configs.  The actual
# defaults will be patched in incrementally
_plugin_configs: {}

[Tieske]

Current behaviour is correct imo. JSON makes an explicit distinction between a NULL value and no value (absent). Since patch tries to add values to an object, and NULL is not an object, it refuses to add it. Which is correct.

[rspurgeon]

Disagree based on user experience. From the perspective of a "patch" operation the user doesn't care if it's null or empty, and they are providing values to apply to the target key (which exists). There is no technical reason to prevent this, and it makes more sense from the user's perspective. That JSON nuance isn't relevant to the user.

[Tieske]

they are providing values to apply to the target key (which exists).

So the desired behaviour is that if a key exists, but is not an object, we drop the value, replace it with an empty object (or array if that's what we're patching), and then apply the patch?

[rspurgeon]

if a key exists, but is not an object, we drop the value

I assume it's possible to determine if an existing key's value is either an object, null, or an array? If so, and if a key is null, and the user is expressing the patching of object values to it, why not allow them to do that? If they are attempting to patch an object to an existing Array, then an error seems appropriate. Same with an array to an existing object. The upshot is, it seems very likely to me that a key will be defined with objects as a placeholder for values applied later in a pipeline. The placeholder expresses something in the "code", which is "values to be applied later".

[Tieske]

I assume it's possible to determine if an existing key's value is either an object, null, or an array?

correct

If so, and if a key is null, and the user is expressing the patching of object values to it, why not allow them to do that?

That would effectively mean replacing a null value with an empty-object {} before patching (in the internal implementation, not by the user).

Referring to my previous comment; only for null values? or for any non-matching type?

If null only then I find it inconsistent; a null value is a valid json value, equal to strings and numbers for example. So why nulls? It's also problematic since null values have an explicit meaning in Kong configs, they are the ones that allow you to remove default values.
Additionally it would probably be a convenience to a json starter, but a surprise to a json expert.

If overwriting any non-matching type, then it would at least be consistent and less surprising, but it would also make it easier to overwrite other entries as well if one doesn't specifify the selector well enough.

If they are attempting to patch an object to an existing Array, then an error seems appropriate. Same with an array to an existing object.

Current behaviour is defined as: patching object-fields to something that is not an object, simply will be ignored, no errors. Same for adding array entries to something that is not an array.

The upshot is, it seems very likely to me that a key will be defined with objects as a placeholder for values applied later in a pipeline. The placeholder expresses something in the "code", which is "values to be applied later".

Shouldn't those place holders then have the target type in the first place? eg. an object {} or and array [] ?

Either way (nulls or all non-matching types) is going to be a breaking change.

Maybe we can add a flag to specify that non-matching types should be replaced?

@mheap wdyt?

[mheap]

Is there a way to detect the difference between explicit null and undefined?

[mheap]

I'm still interested in the answer, but I just realised that this wouldn't work due to the selector not matching anything. Continuing to think

[rspurgeon]

I think null is special, it represents the absence of a type and value.

If null only then I find it inconsistent; a null value is a valid json value, equal to strings and numbers for example. So why nulls?

I agree that "changing the type" may be unexpected, but null here is different IMO. It's an absence of a type and value. The user may want to use that to express either a placeholder (empty key) or the erasure of an existing value (a value). The empty key can give a federated design a place to select off of, add structure, comments, etc...

I would expect this to work like so:

If the value of a key is null, define it with the provided value. If the provided value is null, set the target key's value to null. If a key has a value and provided type does not match (except null), then the patch does not make sense and I would expect an error (not silence as is today).

It's also problematic since null values have an explicit meaning in Kong configs, they are the ones that allow you to remove default values.

The user, by nature of running the "last" patch command, is expressing what they want the key's value to be. If they want it to be null then the last patch command should set it to null.

[mheap]

I would expect an error (not silence as is today).

Strong agree - we should default to fail fast where we can

I think null is special, it represents the absence of a type and value.

In general, yes. In JSON patch, it has a semantic meaning (remove the existing value). Given the user needs to add an empty _plugin_configs: key I don't think it's unreasonable to ask them to create it as an empty object rather than null.

If I were building this feature, I'd expect the following:

1. null is not the same as undefined and is a data type of it's own
2. If you try to patch an incompatible data type (including null) you get a very loud error

[Tieske]

Is there a way to detect the difference between explicit null and undefined?

undefined is not a JSON concept afaik, but a JS one. But as you said, you cannot select something that isn't there.

---

Given the user needs to add an empty _plugin_configs: key I don't think it's unreasonable to ask them to create it as an empty object rather than null.

Agree. It's best practice to convey the type info this way (as a general programming practice, eg. if a function returns an array, but there is nothing to return, return an empty one instead of null, to save the caller an additional type-check).

---

The user, by nature of running the "last" patch command, is expressing what they want the key's value to be. If they want it to be null then the last patch command should set it to null.

Requesting the user to define the placeholder as the proper type (eg. {} or []) is easier on them, over telling them to do another patch with a null.

---

If you try to patch an incompatible data type (including null) you get a very loud error

I do not think that is helpful in this case; The patch operations explicitly defined the patch to be on an array or an object. So the user clearly expresses their intent to patch only those types. So it is safe to ignore other values. Consider the following json:

{
  "filter_by": {
    "tags": true,
    "name": false
  },
  "data": {
    "key1": {
      "tags": { "hello": "world" } 
    },
    "key2":{
      "tags": { "hello": "world" } 
    },
    "tags": { "hello": "world" } 
  },
  "tags": { "hello": "world" } 
}

Currently a recursive patch for all "tags" objects, setting field; "hello": "universe", will work as intended. Since it will skip $.filter_by.tags since it is a boolean.

If we throw a hard error on this then for a user to work around this would be quite cumbersome I think. Especially because of the dynamic data we're dealing with. Any custom plugin can introduce a field that has a name collision with an existing Kong field.

[mheap]

That's a good example. Users can work around it by specifying more specific selectors. On balance, I think raising an error is the better experience for 90% of users.

[RobSerafini]

Trying to catch up on this issue. My summary and comments:

• Today a user who tries to patch an array or object to a json key of a different type fails silently. I strongly agree this needs to be improved
• A change to this behavior may be a breaking change but I think it could also be considered a bug fix or non breaking change since the output may or may not be unexpected/incorrect today depending on if someone is relying on this behavior
• I agree that auto replacing a null with a different datatype is problematic
• That leaves behavior options of a hard error with no output as a result of the command or perhaps a warning that a patch element was not applied because of a type mismatch

To decide between these options I think about whether there would be cases where a partial patch application success would be desirable or not. On the one hand the warning approach is the least 'breaking' from the current behavior: pipelines still complete and the output files are still the same, but it does potentially leave someone with an output that may be incomplete in someway. On the other hand a hard error would force people to really inspect the output and fix the problem in their pipeline. I tend to come down on changing this to a hard error.

Does this summary reflect everyone else's understanding?

[rspurgeon]

Requesting the user to define the placeholder as the proper type (eg. {} or []) is easier on them, over telling them to do another patch with a null.

In a pipeline you do not know what "comes before", so my point is if something should be null, you should set it to null as you can't assume from earlier in a federated pipeline.

[rspurgeon]

I don't believe there is value in continuing this debate under this current ticket. Despite my view that we could initialize a null object in these cases, the user can accomplish what is needed by explicitly initializing a key as an empty object as the placeholder. Additionally, a user can overwrite or change any type of object as they see fit by selecting the parent.

However, there is a pattern that has emerged from the discussion that the tool will fail silently (ignore given configurations and patches), and I consider that a bug and should be addressed independently. Captured here: #108