From 8dee56b2428885652974806d445953c22aedd0d0 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 | 69 ++++++++++--------- 4 files changed, 102 insertions(+), 41 deletions(-) diff --git a/src/main/asciidoc/object-mapping/sdc-object-mapping.adoc b/src/main/asciidoc/object-mapping/sdc-object-mapping.adoc index 8a4a5d211..152d2778b 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 b2d8bd602..c928eba26 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 @@ -246,6 +246,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 @@ -261,6 +270,7 @@ final class DefaultNeo4jEntityConverter implements Neo4jEntityConverter { ET instance = instantiate(concreteNodeDescription, queryResult, allValues, relationships, nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity); + knownObjects.removeFromInCreation(internalId); PersistentPropertyAccessor propertyAccessor = concreteNodeDescription.getPropertyAccessor(instance); if (concreteNodeDescription.requiresPropertyPopulation()) { @@ -588,6 +598,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) { @@ -595,12 +606,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) { @@ -621,5 +657,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 70a0f07c5..a27e9e719 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.LocalDate; @@ -74,6 +75,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; @@ -993,7 +995,7 @@ class RepositoryIT { } @Test - void findEntityWithBidirectionalRelationship(@Autowired BidirectionalStartRepository repository) { + void findEntityWithBidirectionalRelationshipInConstructorThrowsException(@Autowired BidirectionalStartRepository repository) { long startId; @@ -1007,14 +1009,10 @@ class RepositoryIT { startId = startNode.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 9e244058b..e4666d251 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 @@ -15,35 +15,6 @@ */ package org.springframework.data.neo4j.integration.reactive; -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.neo4j.integration.shared.common.BidirectionalAssignedId; -import org.springframework.data.neo4j.integration.shared.common.BidirectionalExternallyGeneratedId; -import org.springframework.data.neo4j.integration.shared.common.DtoPersonProjection; -import org.springframework.data.neo4j.integration.shared.common.EntitiesWithDynamicLabels; -import org.springframework.data.neo4j.integration.shared.common.SimplePerson; -import org.springframework.data.neo4j.integration.shared.common.ThingWithFixedGeneratedId; -import reactor.core.publisher.Flux; -import reactor.core.publisher.Mono; -import reactor.test.StepVerifier; - -import java.time.LocalDate; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.Set; -import java.util.UUID; -import java.util.stream.Collectors; -import java.util.stream.IntStream; - import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Tag; @@ -69,6 +40,7 @@ import org.springframework.data.domain.Example; import org.springframework.data.domain.ExampleMatcher; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Sort; +import org.springframework.data.mapping.MappingException; import org.springframework.data.neo4j.config.AbstractReactiveNeo4jConfig; import org.springframework.data.neo4j.core.DatabaseSelection; import org.springframework.data.neo4j.core.ReactiveDatabaseSelectionProvider; @@ -79,10 +51,14 @@ import org.springframework.data.neo4j.integration.shared.common.AltHobby; import org.springframework.data.neo4j.integration.shared.common.AltLikedByPersonRelationship; import org.springframework.data.neo4j.integration.shared.common.AltPerson; import org.springframework.data.neo4j.integration.shared.common.AnotherThingWithAssignedId; +import org.springframework.data.neo4j.integration.shared.common.BidirectionalAssignedId; import org.springframework.data.neo4j.integration.shared.common.BidirectionalEnd; +import org.springframework.data.neo4j.integration.shared.common.BidirectionalExternallyGeneratedId; import org.springframework.data.neo4j.integration.shared.common.BidirectionalStart; import org.springframework.data.neo4j.integration.shared.common.Club; import org.springframework.data.neo4j.integration.shared.common.DeepRelationships; +import org.springframework.data.neo4j.integration.shared.common.DtoPersonProjection; +import org.springframework.data.neo4j.integration.shared.common.EntitiesWithDynamicLabels; import org.springframework.data.neo4j.integration.shared.common.EntityWithConvertedId; import org.springframework.data.neo4j.integration.shared.common.Hobby; import org.springframework.data.neo4j.integration.shared.common.ImmutablePerson; @@ -94,7 +70,9 @@ import org.springframework.data.neo4j.integration.shared.common.PersonWithRelati import org.springframework.data.neo4j.integration.shared.common.PersonWithRelationshipWithProperties; import org.springframework.data.neo4j.integration.shared.common.Pet; import org.springframework.data.neo4j.integration.shared.common.SimilarThing; +import org.springframework.data.neo4j.integration.shared.common.SimplePerson; import org.springframework.data.neo4j.integration.shared.common.ThingWithAssignedId; +import org.springframework.data.neo4j.integration.shared.common.ThingWithFixedGeneratedId; import org.springframework.data.neo4j.integration.shared.common.ThingWithGeneratedId; import org.springframework.data.neo4j.integration.shared.common.WorksInClubRelationship; import org.springframework.data.neo4j.repository.ReactiveNeo4jRepository; @@ -110,6 +88,28 @@ import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; import org.springframework.transaction.ReactiveTransactionManager; import org.springframework.transaction.annotation.EnableTransactionManagement; import org.springframework.transaction.reactive.TransactionalOperator; +import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; + +import java.time.LocalDate; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.UUID; +import java.util.stream.Collectors; +import java.util.stream.IntStream; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.tuple; /** * @author Gerrit Meier @@ -869,9 +869,14 @@ class ReactiveRepositoryIT { startId = 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