PUT behaving like PATCH for nested collections [DATAREST-1012]

[spring-projects-issues]

Nick Ryan opened DATAREST-1012 and commented

With respect to the documentation at:

5.3. The item resource

it states for "PUT": Replaces the state of the target resource with the supplied request body.

When performing a PUT on a resource which contains a nested collection it invokes the method mergeForPut(...) in DomainObjectReader.java.

Based on the documentation I would expect:

PUT: the nested collection to be replaced rather than merged
PATCH: the nested collection to be merged

QUESTION 1: Is the documentation wrong, or the code wrong or am I misunderstanding the expected behaviour?

Secondary to this is the way this issue was encountered:

If we have a resource which contains a nested collection of polymorphic items:

e.g.

parent has a collection of child where each abstract child could be a concrete boy or girl

when POSTING to modify:

parent.children[0] = boy

to:

parent.children[0] = girl

the code ends up calling mergeForPut(...) and trying to modify a boy instance into a girl instance and I get a message like:

Target bean of type boy is not of type of the persistent entity (girl)!

QUESTION 2: What is the expected behaviour in this case or is this case not supported e.g. for a nested collection of abstract types it needs to be cleared before items of different concrete types are replaced?

---

Affects: 2.6 GA (Ingalls)

8 votes, 12 watchers

[spring-projects-issues]

Stefan Reichert commented

Hey guys,

since we face the same issue I'd really appreciate an answer to both questions mentioned above :).
The current implementation for DomainObjectReader.mergeForPut() assumes all members of an inline collection to be of the same class when updating via PATCH or PUT. Is this the intended behaviour (PUT / PATCH semantics for inline collections) or a optimisation aspect regarding reuse of exiting persistent objects?

Thanks in advance, best regards,
Stefan

[spring-projects-issues]

Stefan Reichert commented

forgot to say: I'd provide a patch in case the behaviour does not reflect the intended PUT / PATCH semantics for inline collections

[spring-projects-issues]

Stefan Reichert commented

please see pull request for DomainObjectReader fix for merging polymorphic persistent nested collections on PUT: #262

[spring-projects-issues]

Joseph Valerio commented

Is there any progress on this issue?

Note: relates to DATAREST-1015

[spring-projects-issues]

Oliver Drotbohm commented

In comparison to what has been proposed a solution in the pull request, wouldn't it be easier to identify the case where a collection item is going to be replaced, and if the types don't match, just completely replace the old one with the new one?

We also have to think about inheritance hierarchies and the fact that some of the backing entity's properties might be hidden from output (using @JsonIgnore). That situation is the reason we do merging on PUT in the first place, as we have to make sure that hidden resource state is maintained and not wiped

[spring-projects-issues]

Joseph Valerio commented

To your point about inheritance hierarchies, couldn't SDR provide an annotation to optionally skip the merging, to allow developers the control we need? In our case this complex field is mapped to a jsonb property (a Map) on the backend. So there is no inheritance, just merging causing issues.

[spring-projects-issues]

Stefan Reichert commented

Regarding identification of 'matches' when merging nested elements in a collection, what would be the matching rule? Since nested elements usually do not contain their id it is difficult to do it in a generic fashion, right? One could distinguish between List (which would work using the index) and Set (using the approach proposed in the pull request)

[spring-projects-issues]

Nick Ryan commented

Hello,

If understand correctly the comments on this ticket are relating to my original 2nd question: i.e. how to behave in the case of doing a merge of a collection of polymorphic items.

@Oliver Gierke : I agree with your statement on the proposed patch: "identify the case where a collection item is going to be replaced, and if the types don't match, just completely replace the old one with the new one".

However with respect to my original 1st question: I would expect:

• PUT: the nested collection to be replaced rather than merged
• PATCH: the nested collection to be merged

Why is the behaviour of merging being discussed with relation to PUT? I would have thought that merging would come into play only for PATCH and we should be discussing the behaviour of a method called mergeForPatch(...) rather than the existing mergeForPut(...).

I am still missing the basic reason of why a merge is being done in the case of PUT? If a merge was not done in the case of PUT, then these two items would be non-issues or possibly updated to just refer to the PATCH case only:

• DATAREST-1001
• DATAREST-1015

My suggestion would be to make the following changes:

• modify the proposed merge patch for supporting polymorphic items in nested resource collections as per Oliver Gierke's comment
• make a change so that merging is only performed for PATCH requests

I can probably work on the first item and submit something, the second one I have no idea where to look but could investigate.

But I'd prefer to do this only after having my thoughts about expected behaviour confirmed.

[spring-projects-issues]

Nick Ryan commented

Hello, not sure if "Provide Feedback for Developer" is the right thing to do..

Just wanted to point out that the ticket was updated with my comments, but didn't see that the status changed from Waiting For Feedback. Hoping you can answer my queries and I might be able to assist with a patch

[spring-projects-issues]

Stefan Reichert commented

This issue came up again and I re-submitted a pull request.

The PR does not change the current behaviour for homogeneous collections but sticks to the "merge on PUT" strategy. It enhances the functionality to support heterogenous (polymorphic) inline collections as well. I hope the comments and test are sufficient.

Please let me know whether this is feasible or if you prefer to simply recreate all inline objects on PUT. I personally don't favour that "replace on PUT" approach for the following reasons:

• breaks with current behaviour of persisting homogeneous inline collections
• might fail on complex inline objects with references (sort of refers to your argumentation for PATCH, but I did not test that though)
• replace on PUT would result in creating new objects with new PKs on each update which is sort of counter intuitive

Anyway, if you prefer the "replace on PUT" approach, please let me know. I'll then take care of the PR. This one really bothers us since it has negative impact on our domain design

[spring-projects-issues]

Stefan Reichert commented

Hey Oliver Drotbohm, I am aware of this issue's "maturity" :). Nevertheless any comment (positive or negative) is highly appreciated. Thanks in advance and best from Hamburg

[spring-projects-issues]

Oliver Drotbohm commented

I might be misunderstanding the suggested change but it feels rather arbitrary to me to group the objects by type and then effectively merge them based on that. The merging solely happens to make sure we keep values of fields around that are not exposed in the JSON. I'd still argue that the payload data used in the request expresses that the first item is supposed to be what's in the payload, not expecting additional data transferred, that some object at a different index in the list that has the same type by accident.

Imagine your Pear type had an additional property foo and that would've been set to bar initially (at index 2 ), would you really expect that that value makes it into the new first element of the resulting list?

[spring-projects-issues]

Stefan Reichert commented

Hey Oliver Drotbohm, thanks for your comment. I totally agree that the "matching by type" approach has exactly that weakness.

To answer your question: No I wouldn't. Nevertheless the current implementation - from my perspective - already does exactly what you described as rather arbitrary: It creates iterators from both the incoming payload's collection and the persistent object's collection. Since JPA uses Sets as default collection type, there might not be an index for the iterator created - so the actual order of the objects to be mapped isn't necessarily being kept anyway. In other word's: If you pass 2 Pears as containment of a basket of Pears, it already is not guaranteed which Pear in the persistent object's collection us updated with which Pear of the incoming payload's collection.

The patch sticks to the current version's approach. Did I understand the current handling right? What would be a suitable alternative for you (I'll provide another patch :))?

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[yejianfengblue]

1. Why not if hidden fields with @JsonIgnore and the state (such as primary key, audit info) is expected to be kept, just go with patch for merge; leave put for replacement. Sorry I don't see an answer from the conversation.

Also would like to hear your opinion about
2. a choice of either merge or replace is provided by annotation or configuration
3. if the types don't match, just completely replace the old one with the new one, said by Oliver

[odrotbohm]

This one has come up again today in the context of #2130. That one targets the change of entity type handling for PUT requests.

As the original reporter mentioned this ticket here and pointed to #368, I took another look at the PR. I've come to the conclusion that the assumption expressed in the test case are invalid, as it expects an instance of a concrete type to be automagically reused to establish an instance of the same type at a different index. That might work if there's a single instance per concrete type in the original collection but not a deterministic approach.

I still wanted to ping this ticket here, as the fix for #2130 might just fix the original reporter's problem. I'm inclined to decline #368 as is.