Repository navigation
API V2: Inconsistent 'add' behavior for empty arrays #224
Description
Activity
Should we always support an array value in
addoperations for list-valued properties? By which I mean also in the case that there is already a list with existing values? It makes a certain sense to allow appending multiple values to an existing list at once, or inserting multiple values at once at some position within an existing list. Then the case of initializing a property that doesn't yet have an array (in JSON terms isundefined) could be appending a list of multiple values where there isn't yet any list, using the-index that is always valid, even for empty arrays and (by our extension here) absent arrays.So, perhaps we should support the entire matrix of:
- append to a property that already has an array value (empty or not)
- a single value
- an array of one or more values (I suppose zero values could be supported as a no-op)
- insert in a property that already has an array value
- a single value
- an array of one/zero or more values per above
- initialize a property that is
undefinedwith- a single value (that EMF knows to put into an array)
- an array of zero or more values, the empty array case being useful for explicit initialization
I think this may all be fairly clear for flat array properties. But I suppose a model might have some property that is an array of arrays. Then anything added to such a property must itself be an array and we get ambiguity when faced with empty arrays where the property currently is
undefined. Is an empty array to be appended as the first member of the array-of-arrays? Or does it initialize the property to an array of zero arrays?Perhaps this suggests that initialization of an
undefinedproperty should be done using areplacecommand without an index, because that is unambiguously setting the value of the property in toto?- append to a property that already has an array value (empty or not)
Perhaps this suggests that initialization of an undefined property should be done using a replace command without an index, because that is unambiguously setting the value of the property in toto?
In that case, the issue is that we have little control about how Json Patch diffs are generated; so we need to stay as close as possible to the Json Patch semantics. This means that supporting only "replace" is not really an option.
AFAIK, in Json Patch, "add" is always an append, i.e. it never removes anything ("replace" is "remove" + "add"). Adding an array of values at a specific index would create a multi-dimensional array:
{ "array": [1, 2] }{ "op": "add", "path": "/array/-", "value": [3, 4] }should result in:
{ "array": [1, 2, [3, 4]] }(Which may or may not be valid in EMF, depending on the Ecore model).
The general issue with the JsonPatchHelper is that it was implemented in a simplified way; we wanted to support all "necessary" model operations, but not all "possible" operations. However, since we now more heavily rely on generic Json Patch Diff libraries, which may produce more different operations (and don't know anything about the underlying EMF Model), we need to fill the gaps.
So we need:
- To handle "add" without an index
- To handle "collection values" (we only support primitive values at the moment, and convert them to a Singleton list for the EMF Command)
I don't think it is necessary to handle all possible combinations. Especially, index + list would (should) work exclusively for EMF Multi-dimensional collections, just like it does with native Json Patch.
Initializing an EMF Collection with an empty array would be a no-op (Except maybe in some custom cases, where the list isn't initialized automatically by EMF?). The reason EMF empty lists appear as undefined in Json is because the Codec doesn't serialize default values (But IMO it should serialize everything, because the client may not know anything about the EMF default values to fill the gaps by itself).
Empty arrays are handled differently in EMF and Json. For example, these 2 models are equivalent in EMF (because lists are never "null"; they default to an empty collection), but different in Json:
{ "array": [] }{ }In the first case, we can add an array value with this patch:
{ "op": "add", "path": "/array/-", "value": "newValue" }In the second case, we need to add the entire array to the root node:
{ "op": "add", "path": "/array", "value": ["newValue"] }The JsonPatchHelper doesn't support the latter case (index-less "add" operation with a full array value). We should support direct feature edition (
/arrayinstead of/array/-) for arrays , and expect an array value in that case ([value1, value2]instead ofvalue1).