Consider options for dealing with non-List collections

[habuma]

At the moment, Patch can only deal with changes to Objects and Lists of Objects. For many use cases, that is sufficient. But what if the objects being compare are java.util.Sets or have properties of type Set? What about Map or properties of type Map?

JSON Patch doesn't address these cases very well and thus Spring Sync's Patch (which is initially inspired from JSON Patch) doesn't deal with them very well, either.

Set is particularly tricky. What would the path look like if an entry in a set is to be removed? What would the path look like if the property on a Set entry were to change? Indeed, the path interpreter (which assumes a JSON Patch-style path) would need to be changed to support additional path syntax or even pluggable to support optional path syntax.

[xak2000]

At the moment, Patch can only deal with changes to Objects and Lists of Objects. For many use cases, that is sufficient.

How about JPA? Even if I would model OneToMany relationships with List<SubEntity> then JPA provider still doesn't guaranties to return them always in the same order. So if we do PATCH to /items/1 [{"op":"replace","path":"/subentities/3/description","value":"new description"}] then how can we ensure that we changing description of the same SubEntity which returned from GET /items/1 as

{
  "id": 1,
  "name": "item1",
  "subentities": [
    { "id": 1, "description": "subentity1" },
    { "id": 2, "description": "subentity2" },
    { "id": 3, "description": "subentity3" }
  ]
}

Maybe item.getSubentites() will return them before patch as 1,3,2 or something.

Also how about filtering? If we do GET /items?active=true then we receive only subset of items. Now we edit some of them and want to persist changes. What patch we should send to /items? We don't know their positions in full list. We address them by ID (or _links.self in HATEOAS). So patch must somehow include IDs of the items, not just their positions in list.

Not to mention the fact that Set is more natural collection type for JPA as rows in DB doesn't haw an index. Their index is their PK.

I think that working with data from DB is primary goal for spring-sync, right? Then we must somehow extend the patch mechanism to understand PKs.

[xak2000]

Also note even if we were somehow able to guarantee the order of items in a list, we still can't guarantee that this list doesn't changed in between GET and PATCH.

For example:

• user 1 GET /items
• user 2 GET /items
• user 1 removes item2, adds item3 and sends a patch to the server.
• server gets items from persistent store as [item1,item2] and successfully saves the changes as [item1,item3].
• user 2 modify item2.description and sends a patch to the server.
• server gets items from persistent store as [item1,item3] and changes item3.description.

Now we have the item3 with new description in or DB but user 2 wanted to change the item2's!

user 2 can use { "op": "test", "path": "/2/id", "value": "2" } in his patch but it is still a workaround. He must test if the item that he wants to change is still that item. This will work, but only for this narrow use case.

[pablogomezp]

With hibernate usually it is wiser to work with Sets, in a Set the order is not important by definition so in that case, it doesnt matter in which order hibernate returns the entities.

However, guys you clearly have a bug in the source code. Lines 119-121 in Diff.java
if (Collection.class.isAssignableFrom(fieldType)) {
diffList(operations, path + "/" + field.getName(), (List) origValue, (List) modValue);
}

you are casting a collection to a list. As you might know, every List is a collection but not the other way around!
So I'd say that this is not an enhancement, it is a bug

[pablogomezp]

I'd propose to extend that function like:
if (List.class.isAssignableFrom(fieldType)) {
diffList(operations, path + "/" + field.getName(), (List) origValue, (List) modValue);
}else if(Set.class.isAssignableFrom(fieldType)){
diffSet(operations, path + "/" + field.getName(), (Set) origValue, (Set) modValue);
}else if (fieldType.isArray()) {
diffList(operations, path + "/" + field.getName(), Arrays.asList((Object[]) origValue), Arrays.asList((Object[]) modValue));
}else{
throw new Exception("Collection not supported "+fieldType);
}

where diffSet(operations, path + "/" + field.getName(), (Set) origValue, (Set) modValue);
should relly on hashcode of the objects for instance, as in a set the order is not important.. so If a user uses a Set should care of providing proper hashcode/equals e.g based on primary key in case of jpa/hibernate