From 74764c08d4dd9cdad5373d35520fcba43438a1f7 Mon Sep 17 00:00:00 2001 From: Gerrit Meier Date: Wed, 28 Jul 2021 17:37:46 +0200 Subject: [PATCH] GH-2340 - Clear exception if mapping recursive immutable objects. Also improving the documentation around this topic. Closes #2340 --- .../object-mapping/sdc-object-mapping.adoc | 10 ++++ .../mapping/DefaultNeo4jEntityConverter.java | 48 +++++++++++++++++++ .../integration/imperative/RepositoryIT.java | 16 +++---- .../reactive/ReactiveRepositoryIT.java | 12 +++-- 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/main/asciidoc/object-mapping/sdc-object-mapping.adoc b/src/main/asciidoc/object-mapping/sdc-object-mapping.adoc index 88f36890c..f6a453c4c 100644 --- a/src/main/asciidoc/object-mapping/sdc-object-mapping.adoc +++ b/src/main/asciidoc/object-mapping/sdc-object-mapping.adoc @@ -222,6 +222,16 @@ It's an established pattern to rather use static factory methods to expose these * _Use Lombok to avoid boilerplate code_ -- As persistence operations usually require a constructor taking all arguments, their declaration becomes a tedious repetition of boilerplate parameter to field assignments that can best be avoided by using Lombok's `@AllArgsConstructor`. +[[mapping.fundamentals.recommendations.note-immutable]] +=== A note on immutable mapping + +Although we recommend to use immutable mapping and constructs wherever possible, there are some limitations when it comes to mapping. +Given a bidirectional relationship where `A` has a constructor reference to `B` and `B` has a reference to `A`, or a more complex scenario. +This hen/egg situation is not solvable for Spring Data Neo4j. +During the instantiation of `A` it eagerly needs to have a fully instantiated `B`, which on the other hand requires an instance (to be precise, the _same_ instance) of `A`. +SDN allows such models in general, but will throw a `MappingException` at runtime if the data that gets returned from the database contains such constellation as described above. +In such cases or scenarios, where you cannot foresee what the data that gets returned looks like, you are better suited with a mutable field for the relationships. + [[mapping.fundamentals.kotlin]] == Kotlin support diff --git a/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java b/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java index 72eb76731..4e6920570 100644 --- a/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java +++ b/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java @@ -254,6 +254,15 @@ final class DefaultNeo4jEntityConverter implements Neo4jEntityConverter { Long internalId = getInternalId(queryResult); Supplier mappedObjectSupplier = () -> { + if (knownObjects.isInCreation(internalId)) { + throw new MappingException( + String.format( + "The node with id %s has a logical cyclic mapping dependency. " + + "Its creation caused the creation of another node that has a reference to this.", + internalId) + ); + } + knownObjects.setInCreation(internalId); List allLabels = getLabels(queryResult, nodeDescription); NodeDescriptionAndLabels nodeDescriptionAndLabels = nodeDescriptionStore @@ -264,6 +273,7 @@ final class DefaultNeo4jEntityConverter implements Neo4jEntityConverter { ET instance = instantiate(concreteNodeDescription, queryResult, nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity, relationshipsFromResult, nodesFromResult); + knownObjects.removeFromInCreation(internalId); PersistentPropertyAccessor propertyAccessor = concreteNodeDescription.getPropertyAccessor(instance); if (concreteNodeDescription.requiresPropertyPopulation()) { @@ -634,6 +644,7 @@ final class DefaultNeo4jEntityConverter implements Neo4jEntityConverter { private final Lock write = lock.writeLock(); private final Map internalIdStore = new HashMap<>(); + private final Set idsInCreation = new HashSet<>(); private void storeObject(@Nullable Long internalId, Object object) { if (internalId == null) { @@ -641,12 +652,37 @@ final class DefaultNeo4jEntityConverter implements Neo4jEntityConverter { } try { write.lock(); + idsInCreation.remove(internalId); internalIdStore.put(internalId, object); } finally { write.unlock(); } } + private void setInCreation(@Nullable Long internalId) { + if (internalId == null) { + return; + } + try { + write.lock(); + idsInCreation.add(internalId); + } finally { + write.unlock(); + } + } + + private boolean isInCreation(@Nullable Long internalId) { + if (internalId == null) { + return false; + } + try { + read.lock(); + return idsInCreation.contains(internalId); + } finally { + read.unlock(); + } + } + @Nullable private Object getObject(@Nullable Long internalId) { if (internalId == null) { @@ -667,5 +703,17 @@ final class DefaultNeo4jEntityConverter implements Neo4jEntityConverter { } return null; } + + private void removeFromInCreation(@Nullable Long internalId) { + if (internalId == null) { + return; + } + try { + write.lock(); + idsInCreation.remove(internalId); + } finally { + write.unlock(); + } + } } } diff --git a/src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java b/src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java index cd6ae5d97..cef305797 100644 --- a/src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java +++ b/src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java @@ -18,6 +18,7 @@ package org.springframework.data.neo4j.integration.imperative; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.tuple; import java.time.Instant; @@ -78,6 +79,7 @@ import org.springframework.data.geo.Circle; import org.springframework.data.geo.Distance; import org.springframework.data.geo.Metrics; import org.springframework.data.geo.Polygon; +import org.springframework.data.mapping.MappingException; import org.springframework.data.neo4j.config.AbstractNeo4jConfig; import org.springframework.data.neo4j.core.DatabaseSelection; import org.springframework.data.neo4j.core.DatabaseSelectionProvider; @@ -977,21 +979,17 @@ class RepositoryIT { } @Test - void findEntityWithBidirectionalRelationship(@Autowired BidirectionalStartRepository repository) { + void findEntityWithBidirectionalRelationshipInConstructorThrowsException(@Autowired BidirectionalStartRepository repository) { long startId = doWithSession(session -> session .run("CREATE (n:BidirectionalStart{name:'Ernie'})-[:CONNECTED]->(e:BidirectionalEnd{name:'Bert'}), " + "(e)<-[:ANOTHER_CONNECTION]-(anotherStart:BidirectionalStart{name:'Elmo'})" + "RETURN n") .single().get("n").asNode().id()); - Optional entityOptional = repository.findById(startId); - assertThat(entityOptional).isPresent(); - BidirectionalStart entity = entityOptional.get(); - assertThat(entity.getEnds()).hasSize(1); - - BidirectionalEnd end = entity.getEnds().iterator().next(); - assertThat(end.getAnotherStart()).isNotNull(); - assertThat(end.getAnotherStart().getName()).isEqualTo("Elmo"); + assertThatThrownBy(() -> repository.findById(startId)) + .hasRootCauseMessage("The node with id " + startId + " has a logical cyclic mapping dependency. " + + "Its creation caused the creation of another node that has a reference to this.") + .hasRootCauseInstanceOf(MappingException.class); } diff --git a/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveRepositoryIT.java b/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveRepositoryIT.java index de7be387d..f1f01d75f 100644 --- a/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveRepositoryIT.java +++ b/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveRepositoryIT.java @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.tuple; +import org.springframework.data.mapping.MappingException; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; @@ -829,9 +830,14 @@ class ReactiveRepositoryIT { return startNode.id(); }); - StepVerifier.create(repository.findById(startId)).assertNext(entity -> { - assertThat(entity.getEnds()).hasSize(1); - }).verifyComplete(); + StepVerifier.create(repository.findById(startId)) + .verifyErrorMatches(error -> { + Throwable cause = error.getCause(); + return cause instanceof MappingException && cause.getMessage().equals( + "The node with id " + startId + " has a logical cyclic mapping dependency. " + + "Its creation caused the creation of another node that has a reference to this."); + }); + } @Test