spring-projects / spring-data-rest

Simplifies building hypermedia-driven REST web services on top of Spring Data repositories
https://spring.io/projects/spring-data-rest
Apache License 2.0
913 stars 561 forks source link

The merge+patch implementation does not support merging arrays nested within a map #2350

Closed vkhoroshko closed 8 months ago

vkhoroshko commented 9 months ago

Assume an entity mapped like that (using hibernate-types-60 library for JsonType):

    @Type(JsonType.class)
    @Column(name = "additional_info")
    private Map<String, Object> additionalInfo;

When a first PATCH request is executed for the entity with the following request body:

PATCH /testEntities/1
{  
    "additionalInfo": {
        "testArray": ["val1"]       
    }   
}

Then, correct JSON is stored in a database.

However, on all subsequent PATCH requests it's simply impossible to update this nested array property. E.g. the following call:

PATCH /testEntities/1
{  
    "additionalInfo": {
        "testArray": ["val1", "val2", "val3"]       
    }   
}

does not make any effect.

The expected behavior is to have an ability to overwrite nested array fields. Currently the only possible way to accomplish that is to make 2 calls - 1st to set the array back to null (this works as DomainObjectReader does not even try to merge and simply replaces the value. And 2nd to set the array to the desired value.

Thanks in advance.

odrotbohm commented 8 months ago

Can you provide a reproducer for that? I've just tried a unit tests on DomainObjectMerger like this:

@Test
void replacesNestedArrays() throws Exception {

    MapWrapper wrapper = new MapWrapper();
    wrapper.map.put("array", new String[] { "original" });

    ObjectMapper mapper = new ObjectMapper();

    JsonNode node = mapper.readTree("{ \"map\" : { \"array\" : [ \"first\", \"update\" ] } }");
    MapWrapper result = reader.doMerge((ObjectNode) node, wrapper, mapper);

    node = mapper.readTree("{ \"map\" : { \"array\" : [ \"second\", \"update\" ] } }");
    result = reader.doMerge((ObjectNode) node, wrapper, mapper);

    System.out.println(result.map.get("array"));
}

static class MapWrapper {
    public Map<String, Object> map = new HashMap<>();
}

And it shows the second update being applied properly.

Also, merge patch requires arrays to be replaced entirely according to the RFC:

Also, it is not possible to patch part of a target that is not an object, such as to replace just some of the values in an array.

vkhoroshko commented 8 months ago
@Test
void replacesNestedArrays() throws Exception {

  MapWrapper wrapper = new MapWrapper();
  wrapper.map.put("array", new String[] { "original" });

  ObjectMapper mapper = new ObjectMapper();

  JsonNode node = mapper.readTree("{ \"map\" : { \"array\" : [ \"first\", \"update\" ] } }");
  MapWrapper result = reader.doMerge((ObjectNode) node, wrapper, mapper);

  node = mapper.readTree("{ \"map\" : { \"array\" : [ \"second\", \"update\" ] } }");
  result = reader.doMerge((ObjectNode) node, wrapper, mapper);

  System.out.println(result.map.get("array"));
}

static class MapWrapper {
  public Map<String, Object> map = new HashMap<>();
}

Assuming you're running DomainObjectReaderUnitTests just add the persistent entity to reproduce it:

mappingContext.getPersistentEntity(MapWrapper.class);

It's very important because it jumps into loop while in your case it simply stops here:

if (!candidate.isPresent()) {
    return mapper.readerForUpdating(target).readValue(root);
}
odrotbohm commented 8 months ago

I can reproduce the problem now. Thanks for the additional hint! 🙇

odrotbohm commented 8 months ago

This is fixed on main and backported to 4.2.x and 4.1.x. Feel free to give the snapshots a try.

kamangacode commented 8 months ago

Hello @odrotbohm the fix doesn't work on my use case. I tried it with version 4.2.3-SNAPSHOT but still have the issue. You can reproduce it using my github example: https://github.com/kamangacode/spring-data-rest-ano Just follow the instruction in the readme

odrotbohm commented 8 months ago

I'm going to keep this one closed, as it's specifically about array handling in maps. That said, I've filed #2357.