Removing items from nested collections by UID

[HendrikThePendric]

Problem definition

The webapi currently has implemented JSON Patch according the RFC 6902 IETF specs. However, these specifications don't cover a convenient way to remove nested collection items by UID.

One of our goals was to do "canonical updates", i.e. being able to update an object (with nested collections) by issuing a single HTTP request. The inability to remove nested collection items by UID means we haven't reached our goal yet.

Possible solutions

We have several options to address this issue which are listed below as "suggested answers". Please upvote the solution that seems most appropriate to you and reply if you like to add your motivation. If you like one of the options, but would like to see some minor alterations, please add your suggestion as a reply. If you have a completely different option in mind, please suggest a new answer at the bottom.

A note about adding nested collection items

The text above doesn't cover adding items at all because that is covered by the IETF specs:

{ "op": "add",     "path": "/items/-", "value": { "id": "oXD88WWSQpR" } }

[HendrikThePendric]

Option A - forget about canonical updates and use the existing API

While I don't think this is actually a viable option, I feel I should still be mentioned here for completeness.

We do already have a dedicated API for managing nested collections (see docs). So we could just:

• Keep our JSON Patch implementation untouched, which would mean it is a 1:1 implementation of the IETF specs.
• Do (an) additional request(s) to add/remove collection items using the existing API.

This would be something we have to do everywhere we encounter nested collections. As such it would mean a lot of additional work in our apps, and also more network traffic.

[HendrikThePendric]

Option B - hijack the "replace by index" operation

Although the IETF specs don't include a way to remove collection items by UID, they do include a way to remove items by index:

{ "op": "remove", "path": "/nestedCollection/1" }

Since an index is (a string representation of) an integer, and a UID has a fixed length and a combination of alphanumeric characters, it is possible to differentiate between the two. As such we could implement things as follows:

// by index
{ "op": "remove", "path": "/nestedCollection/1" }
// by UID
{ "op": "remove", "path": "/nestedCollection/oXD88WWSQpR" }

We are not adding any new operators here, but extending what one of the IETF operators is capable of doing. We need to consider if that is a good or a bad thing.

[amcgee]

What about removing by other unique identifiers than UID? For example, remove by code? This could be something that's requested in the future...

[amcgee]

In practice this will probably be fine, but technically 11111111111 is a valid index and also a valid UID. So is 00000000001, technically. We could limit indexes to fewer than 11 digits, but that feels a bit arbitrary

[mortenoh]

11111111111 is not a valid UID actually, the regex is ^[a-zA-Z]{1}[a-zA-Z0-9]{10}$ which means it has to start with a letter, but a code could match both UID and a index of course.

[HendrikThePendric]

Option C - adopt the "value-based array operations" proposal

This proposal is already 6 year old and suggests this should be a valid JSON Patch operation:

{ "op": "remove",  "path": "/nestedCollection/-", "value": { "id": "oXD88WWSQpR"} }

I'm not sure this proposal will ever get adopted, and also find it quite generic. How would we deal with cases where value doesn't contain a UID? Would we traverse the entire DB table until we encounter an item that has the same property values? I worry about the performance impact of that and the risk of deleting the wrong item.

[amcgee]

I like the idea of doing something generic like this, though as you mention @HendrikThePendric we need to enforce uniqueness on the value property.

[ghost]

To me this could be potentially confusing, and misused. As you both mention, what if the value maps to multiple objects to remove? The generic nature of this syntax suggests that an operation like that is possible. So either we have to:

• Allow for something that potentially does way more than just remove by id. Which seems complex for us to implement well.
• Restrict it to just allowed operations. The syntax does not suggest that such a restriction exists so this seems confusing to the user.

[amcgee]

We could quite easily restrict the "key" selection to only keys which are defined as unique (this is present in the schema so it is easy for the backend to check) - that would be id and code, with possibly some additional unique properties depending on the endpoint

[amcgee]

We could be more explicit about this with the following:

{ "op": "remove", "path": "/nestedCollection/-", "key": "id", "value": "oXD88WWSQpR" }

Key might be optional and default to id

[amcgee]

Key could initially be restricted to id and code but could be extended to other guaranteed-unique properties in the future

[HendrikThePendric]

Option D - add a custom operator

We have discussed two variants of this:

D1 - a regular custom operator

{ "op": "remove-by-uid",  "path": "/nestedCollection/oXD88WWSQpR" }

D2- a name-spaced custom operator

{ "op": "dhis2-remove-by-uid",  "path": "/nestedCollection/oXD88WWSQpR" }

By using a custom operator we don't cause any surprises. Since this is not part of the standard, users will need to consult our documentation to become aware of its existence. But in all fairness, that would be an issue for all solutions since the IETF specs don't cover this feature anyway, and the only other way to delete items from nested collections would be to read the docs on our collection api.

[ghost]

Either of these seem good to me. The clear operation name combined with using a primitive for selection of something that should be unique seem like it would avoid the issues from the json-patch issue.

One thing I wonder is if we foresee needing other functionality in the future, and if so if we think that this name would combine well with that? remove-by-uid is specific enough that it should never really clash with anything right?

[amcgee]

Removing by code is something I could definitely foresee being requested, as could removal by unique attribute property for example - would we then need remove-by-id (we typically avoid referring to uid externally), remove-by-code, and remove-by-attribute as well? We support id and code for identifier selection in the import/export app, and attribute selection for data downloads from the various analytics apps, so it's not unreasonable that someone would want to use the identifiers from those downloads and then use them to request removal of specific collection members via the api

[amcgee]

I'd be open to a separate operation if we really want to avoid overloading the remove operation, but I don't think we should make it -by-id, I think we need it to at least also support code. So maybe something like "op": "remove-by-key" with a "key": "id" property could work...

[HendrikThePendric]

@amcgee just to confirm, would the operation you suggested look like this?

{ "op": "remove-by-key",  "path": "/nestedCollection/", "key": "uid", "value": "oXD88WWSQpR"  }

[amcgee]

Yeah, I think something like that would be clean and explicit. I can create another answer suggestion if needed

[HendrikThePendric]

Option E - add a custom remove-by-key operator

This solution was suggested by @amcgee. Its biggest advantage of this solution is that this is a fairly generic operator which would allow deletion by various unique keys. For example, we could allow deletion by id, code, or attribute.

{ "op": "remove-by-key",  "path": "/nestedCollection/", "key": "id", "value": "oXD88WWSQpR"  }

[HendrikThePendric]

I was initially quite positive about this suggestion because of its flexibility. But now I’m getting some second thoughts, because I wonder if we actually need that flexibility. We need some way to delete collection items based on a unique identifier. A UID is a unique identifier that every entity has. As such you could argue that a way to remove by UID is sufficient: if you need to remove nested collection items in a JSON Patch request, you just need to ensure you include the ID field when you GET the items.

[mortenoh]

I can't really foresee a situation where you would use code here. Having a simple d2-remove-uid should be enough for 99% of use cases. No UI client should use the codes, they are mainly used for integration purposes (where normally it would not be handled like this). An alternative would be d2-remove-id which would do: remove by uid, remove by code, .. (we are thinking of adding new identifier blocks in dhis2, so it would continue trying to match).

But to start with, I suggest simply d2-remove-uid (I'm generally in favor of adding new ops also, as I can see us adding more in the future, not only for removal)

[Mohammer5]

Option F - add a custom by-key matcher

I just thought about this now, but we'll probably need the same functionality to update in the future as well, won't we? I just copied some ideas from option D

{ "op": "remove", "match": "by-key",  "path": "/nestedCollection/", "key": "id", "value": "oXD88WWSQpR"  }
{ "op": "update", "match": "by-key",  "path": "/nestedCollection/", "key": "id", "value": "oXD88WWSQpR"  }

For embedded object ? mabye ?

{ "op": "read", "match": "by-key",  "path": "/nestedCollection/", "key": "id", "value": "oXD88WWSQpR"  }

For inserting new objects? maybe ?

{ "op": "create", "match": "before-key",  "path": "/nestedCollection/", "key": "id", "value": "oXD88WWSQpR"  }
{ "op": "create", "match": "after-key",  "path": "/nestedCollection/", "key": "id", "value": "oXD88WWSQpR"  }

The possible values for match could be: before-key (create only) | after-key (create only) | by-key (non-create only) | by-index (non-create only) (of which the latter is the default when not providing any value)

[Mohammer5]

Examples are:

• drag'n'drop (remove + create)
• updating values of an object by id
• creating new items before/after an existing item

[ghost]

Before key and after key are going to be difficult I think. The JSON patch issue that discusses adding this functionality got stuck on that very topic. I think we'll hit the same roadblocks as them in trying to implement that kind of functionality.

[HendrikThePendric]

Option G - custom operation with valid JSON Pointer string for path

This option is the result of a meeting held by Team Platform Frontend and from its perspective the most preferable syntax. It is inspired by option D1 above, but takes into account the fact that /nesteCollection/ID would make the path an invalid JSON Pointer.

{ "op": "remove-by-id", "path": "/nestedCollection”, "id": "oXD88WWSQpR" }

Some motivations for preferring this syntax:

• We should implement removal of nested collection items by ID as a custom operation, instead of adding custom behaviour to existing operations
• About the custom operation name:
  • The operation should be specific / have a very narrow scope. Since we are not 100% sure if use cases will present themselves for removing nested collection items by any other unique field (code was discussed specifically), we should go for a solution fully based on the ID field.
  • We do not believe the operator name needs to have an organisation namespace (i.e. dhis2-remove-by-id), because nothing about this new custom operation is specific to dhis2 or our business domain.
• We disregarded a few options regarding the path:
  • The /- suffix would be wrong because that explicitly means “the end of the array”
  • We considered nestedCollection/<ID>, but decided against that because that makes the path’s value an invalid JSON Pointer string, see JSON Pointer Specs and the quote below:

    If the currently referenced value is a JSON array, the reference token MUST contain either:

    • characters comprised of digits (see ABNF below; note that leading zeros are not allowed) that represent an unsigned base-10 integer value, making the new referenced value the array element with the zero-based index identified by the token, or
    • exactly the single character “-”, making the new referenced value the (nonexistent) member after the last array element.

Edit: changed UID to ID in the text above as recommended by @mortenoh

[mortenoh]

Can I suggest we rename to remove-by-id as we don't really want to expose uid externally (and the actual id is a database identifier, which we will never expose). This also aligns it to our web-api (where id === uid).

[HendrikThePendric]

Good point @mortenoh. I've updated the text above.