Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vkhoroshko opened this issue Jan 2, 2024 · 6 comments
Assignees
Labels
status: feedback-provided Feedback has been provided

Comments

@vkhoroshko
Copy link

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 2, 2024
@odrotbohm
Copy link
Member

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.

@odrotbohm odrotbohm removed the status: waiting-for-triage An issue we've not yet triaged label Jan 12, 2024
@odrotbohm odrotbohm self-assigned this Jan 12, 2024
@odrotbohm odrotbohm added the status: waiting-for-feedback We need additional information before we can continue label Jan 12, 2024
@vkhoroshko
Copy link
Author

@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);
}

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 15, 2024
@odrotbohm
Copy link
Member

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

odrotbohm added a commit that referenced this issue Jan 15, 2024
odrotbohm added a commit that referenced this issue Jan 15, 2024
@odrotbohm
Copy link
Member

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

@kamangacode
Copy link

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided
Projects
None yet
Development

No branches or pull requests

4 participants