spring-projects / spring-projects/spring-data-rest

Merge patch drops linkable associations of existing array elements since 4.3

Open
#2,596 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

status: waiting-for-triage
Dominant language
Java
Stars
958
Forks
568
PR merge metrics
No merged PRs in 30d

Description

Since 4.3, a merge-patch (PATCH with application/json) no longer updates linkable associations of entities that are nested in an array when the array element already exists. Non-association properties of the same element are updated, and elements appended beyond the current size (or written into an empty collection) do get their associations resolved. So the very same payload behaves differently depending on the index of the element.

Versions
  • Observed with Spring Data REST 5.0.5 (Boot 4.0.6), 5.1.1 and current main (5.2.0-SNAPSHOT).
  • Worked up to 4.2.x. The behavior changed with #2358 (4.3 M1), which switched the handling of existing array elements from doMerge(…) to readPut(…).
Reproducer

Unit test in the style of DomainObjectReaderUnitTests (fails on main, the other 42 tests in the class pass):

@Test
void patchReplacesReferenceOfEntityNestedInArray() throws Exception {

	Associations associations = mock(Associations.class);
	when(associations.isLinkableAssociation(any(PersistentProperty.class)))
			.thenAnswer(invocation -> invocation.<PersistentProperty<?>> getArgument(0).isAssociation());
	when(associations.isLinkableAssociation(any(Association.class)))
			.thenAnswer(invocation -> invocation.<Association<?>> getArgument(0).getInverse().isAssociation());

	DomainObjectReader reader = new DomainObjectReader(entities, associations);

	Tag first = new Tag();
	Tag second = new Tag();

	Track track = new Track();
	track.label = "old label";
	track.tag = first;

	Playlist playlist = new Playlist();
	playlist.tracks.add(track);

	SimpleModule module = new SimpleModule().addDeserializer(Tag.class,
			new SelectValueByIdSerializer<Tag>(Map.of(first.id, first, second.id, second)));
	ObjectMapper mapper = JsonMapper.builder().addModule(module).build();

	ObjectNode node = (ObjectNode) mapper.readTree(
			String.format("{ \"tracks\" : [ { \"label\" : \"new label\", \"tag\" : \"%s\" } ] }", second.id));

	Playlist result = reader.doMerge(node, playlist, mapper);

	assertThat(result.tracks).hasSize(1);
	assertThat(result.tracks.get(0).label).isEqualTo("new label"); // passes
	assertThat(result.tracks.get(0).tag).isSameAs(second);         // fails: still `first`
}

@JsonAutoDetect(fieldVisibility = Visibility.ANY)
static class Playlist {
	@Id UUID id = UUID.randomUUID();
	List<Track> tracks = new ArrayList<Track>();
}

@JsonAutoDetect(fieldVisibility = Visibility.ANY)
static class Track {
	String label;
	@Reference Tag tag;
}

(Tag and SelectValueByIdSerializer are the existing fixtures from the DATAREST-1030 test. Playlist and Track need to be registered with the KeyValueMappingContext in setUp().)

The same happens end-to-end with JPA: an @ElementCollection of an @Embeddable that holds a @ManyToOne to an exported entity. PATCH /orders/{id} with {"items":[{"quantity":2,"product":"/products/{id}"}]} changes quantity but leaves product untouched, while PATCH {"items":[]} followed by the same request applies both.

Analysis

DomainObjectReader#handleArrayNode applies an ObjectNode to an existing element via readPut(…):

if (ObjectNode.class.isInstance(jsonNode)) {
	nestedObjectFound = true;
	readPut((ObjectNode) jsonNode, next, mapper);
}

readPut deserializes the node into a fresh instance and calls mergeForPut(source, target, mapper), which copies properties with MergingPropertyHandler but wraps it in LinkedAssociationSkippingAssociationHandler, i.e. linkable associations are deliberately not copied. That rule makes sense for a top-level PUT, where associations are managed through association resources. For an element nested in an array it does not:

  • there is no association resource for a nested element, so the request body is the only place the association can be expressed;
  • RFC 7386, which #2358 refers to, treats the array as a value that is replaced as a whole, so the association is part of the replaced value;
  • appended elements (rawValues.apply(current)) and elements written into an empty collection are deserialized by Jackson and do resolve the association, and top-level association collections are resolved as well (DATAREST-1030). Only the "existing element at the same index" path drops it.

Afterwards the array is removed from the root node (i.remove()), so the outer readerForUpdating(target) never sees it and cannot recover the association.

Proposal

Treat an existing element the same way as an appended one: deserialize the node into a new element (as rawValues already does) and replace the existing element in the collection, instead of merging with PUT semantics. This keeps the RFC 7386 replacement semantics introduced in #2358 (handlesEntityNestedInAnArrayLikePutForPatchRequest still passes, unspecified properties become null) while restoring the association handling.

I'm happy to submit a PR with the test above and the change to handleArrayNode.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in DomainObjectReader#handleArrayNode and readPut, then review the DomainObjectReaderUnitTests reproducer for patching an existing array element. Compare that path with appended-element handling and add the regression test using Playlist, Track, Tag, and SelectValueByIdSerializer. Done means the existing element's linkable association is updated while the existing merge-patch behavior tests continue to pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
api, backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.