Skip to content

Commit 8dee56b

Browse files
meistermeiermeistermeier
committed
GH-2340 - Clear exception if mapping recursive immutable objects.
Also improving the documentation around this topic. Closes #2340
1 parent 18bdacf commit 8dee56b

File tree

4 files changed

+102
-41
lines changed

4 files changed

+102
-41
lines changed

‎src/main/asciidoc/object-mapping/sdc-object-mapping.adoc‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,16 @@ It's an established pattern to rather use static factory methods to expose these
222222
* _Use Lombok to avoid boilerplate code_ --
223223
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`.
224224

225+
[[mapping.fundamentals.recommendations.note-immutable]]
226+
=== A note on immutable mapping
227+
228+
Although we recommend to use immutable mapping and constructs wherever possible, there are some limitations when it comes to mapping.
229+
Given a bidirectional relationship where `A` has a constructor reference to `B` and `B` has a reference to `A`, or a more complex scenario.
230+
This hen/egg situation is not solvable for Spring Data Neo4j.
231+
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`.
232+
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.
233+
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.
234+
225235
[[mapping.fundamentals.kotlin]]
226236
== Kotlin support
227237

‎src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,15 @@ private <ET> ET map(MapAccessor queryResult, MapAccessor allValues, Neo4jPersist
246246
Long internalId = getInternalId(queryResult);
247247

248248
Supplier<Object> mappedObjectSupplier = () -> {
249+
if (knownObjects.isInCreation(internalId)) {
250+
throw new MappingException(
251+
String.format(
252+
"The node with id %s has a logical cyclic mapping dependency. " +
253+
"Its creation caused the creation of another node that has a reference to this.",
254+
internalId)
255+
);
256+
}
257+
knownObjects.setInCreation(internalId);
249258

250259
List<String> allLabels = getLabels(queryResult, nodeDescription);
251260
NodeDescriptionAndLabels nodeDescriptionAndLabels = nodeDescriptionStore
@@ -261,6 +270,7 @@ private <ET> ET map(MapAccessor queryResult, MapAccessor allValues, Neo4jPersist
261270
ET instance = instantiate(concreteNodeDescription, queryResult, allValues, relationships,
262271
nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity);
263272

273+
knownObjects.removeFromInCreation(internalId);
264274
PersistentPropertyAccessor<ET> propertyAccessor = concreteNodeDescription.getPropertyAccessor(instance);
265275

266276
if (concreteNodeDescription.requiresPropertyPopulation()) {
@@ -588,19 +598,45 @@ static class KnownObjects {
588598
private final Lock write = lock.writeLock();
589599

590600
private final Map<Long, Object> internalIdStore = new HashMap<>();
601+
private final Set<Long> idsInCreation = new HashSet<>();
591602

592603
private void storeObject(@Nullable Long internalId, Object object) {
593604
if (internalId == null) {
594605
return;
595606
}
596607
try {
597608
write.lock();
609+
idsInCreation.remove(internalId);
598610
internalIdStore.put(internalId, object);
599611
} finally {
600612
write.unlock();
601613
}
602614
}
603615

616+
private void setInCreation(@Nullable Long internalId) {
617+
if (internalId == null) {
618+
return;
619+
}
620+
try {
621+
write.lock();
622+
idsInCreation.add(internalId);
623+
} finally {
624+
write.unlock();
625+
}
626+
}
627+
628+
private boolean isInCreation(@Nullable Long internalId) {
629+
if (internalId == null) {
630+
return false;
631+
}
632+
try {
633+
read.lock();
634+
return idsInCreation.contains(internalId);
635+
} finally {
636+
read.unlock();
637+
}
638+
}
639+
604640
@Nullable
605641
private Object getObject(@Nullable Long internalId) {
606642
if (internalId == null) {
@@ -621,5 +657,17 @@ private Object getObject(@Nullable Long internalId) {
621657
}
622658
return null;
623659
}
660+
661+
private void removeFromInCreation(@Nullable Long internalId) {
662+
if (internalId == null) {
663+
return;
664+
}
665+
try {
666+
write.lock();
667+
idsInCreation.remove(internalId);
668+
} finally {
669+
write.unlock();
670+
}
671+
}
624672
}
625673
}

‎src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import static org.assertj.core.api.Assertions.assertThat;
1919
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
2020
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
21+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2122
import static org.assertj.core.api.Assertions.tuple;
2223

2324
import java.time.LocalDate;
@@ -74,6 +75,7 @@
7475
import org.springframework.data.geo.Distance;
7576
import org.springframework.data.geo.Metrics;
7677
import org.springframework.data.geo.Polygon;
78+
import org.springframework.data.mapping.MappingException;
7779
import org.springframework.data.neo4j.config.AbstractNeo4jConfig;
7880
import org.springframework.data.neo4j.core.DatabaseSelection;
7981
import org.springframework.data.neo4j.core.DatabaseSelectionProvider;
@@ -993,7 +995,7 @@ void findEntityWithRelationshipToTheSameNode(@Autowired RelationshipRepository r
993995
}
994996

995997
@Test
996-
void findEntityWithBidirectionalRelationship(@Autowired BidirectionalStartRepository repository) {
998+
void findEntityWithBidirectionalRelationshipInConstructorThrowsException(@Autowired BidirectionalStartRepository repository) {
997999

9981000
long startId;
9991001

@@ -1007,14 +1009,10 @@ void findEntityWithBidirectionalRelationship(@Autowired BidirectionalStartReposi
10071009
startId = startNode.id();
10081010
}
10091011

1010-
Optional<BidirectionalStart> entityOptional = repository.findById(startId);
1011-
assertThat(entityOptional).isPresent();
1012-
BidirectionalStart entity = entityOptional.get();
1013-
assertThat(entity.getEnds()).hasSize(1);
1014-
1015-
BidirectionalEnd end = entity.getEnds().iterator().next();
1016-
assertThat(end.getAnotherStart()).isNotNull();
1017-
assertThat(end.getAnotherStart().getName()).isEqualTo("Elmo");
1012+
assertThatThrownBy(() -> repository.findById(startId))
1013+
.hasRootCauseMessage("The node with id " + startId + " has a logical cyclic mapping dependency. " +
1014+
"Its creation caused the creation of another node that has a reference to this.")
1015+
.hasRootCauseInstanceOf(MappingException.class);
10181016

10191017
}
10201018

‎src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveRepositoryIT.java‎

Lines changed: 37 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -15,35 +15,6 @@
1515
*/
1616
package org.springframework.data.neo4j.integration.reactive;
1717

18-
import static org.assertj.core.api.Assertions.assertThat;
19-
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
20-
import static org.assertj.core.api.Assertions.tuple;
21-
22-
import org.springframework.data.neo4j.integration.shared.common.BidirectionalAssignedId;
23-
import org.springframework.data.neo4j.integration.shared.common.BidirectionalExternallyGeneratedId;
24-
import org.springframework.data.neo4j.integration.shared.common.DtoPersonProjection;
25-
import org.springframework.data.neo4j.integration.shared.common.EntitiesWithDynamicLabels;
26-
import org.springframework.data.neo4j.integration.shared.common.SimplePerson;
27-
import org.springframework.data.neo4j.integration.shared.common.ThingWithFixedGeneratedId;
28-
import reactor.core.publisher.Flux;
29-
import reactor.core.publisher.Mono;
30-
import reactor.test.StepVerifier;
31-
32-
import java.time.LocalDate;
33-
import java.util.ArrayList;
34-
import java.util.Arrays;
35-
import java.util.Collection;
36-
import java.util.Collections;
37-
import java.util.HashMap;
38-
import java.util.HashSet;
39-
import java.util.List;
40-
import java.util.Map;
41-
import java.util.Optional;
42-
import java.util.Set;
43-
import java.util.UUID;
44-
import java.util.stream.Collectors;
45-
import java.util.stream.IntStream;
46-
4718
import org.junit.jupiter.api.BeforeEach;
4819
import org.junit.jupiter.api.Nested;
4920
import org.junit.jupiter.api.Tag;
@@ -69,6 +40,7 @@
6940
import org.springframework.data.domain.ExampleMatcher;
7041
import org.springframework.data.domain.PageRequest;
7142
import org.springframework.data.domain.Sort;
43+
import org.springframework.data.mapping.MappingException;
7244
import org.springframework.data.neo4j.config.AbstractReactiveNeo4jConfig;
7345
import org.springframework.data.neo4j.core.DatabaseSelection;
7446
import org.springframework.data.neo4j.core.ReactiveDatabaseSelectionProvider;
@@ -79,10 +51,14 @@
7951
import org.springframework.data.neo4j.integration.shared.common.AltLikedByPersonRelationship;
8052
import org.springframework.data.neo4j.integration.shared.common.AltPerson;
8153
import org.springframework.data.neo4j.integration.shared.common.AnotherThingWithAssignedId;
54+
import org.springframework.data.neo4j.integration.shared.common.BidirectionalAssignedId;
8255
import org.springframework.data.neo4j.integration.shared.common.BidirectionalEnd;
56+
import org.springframework.data.neo4j.integration.shared.common.BidirectionalExternallyGeneratedId;
8357
import org.springframework.data.neo4j.integration.shared.common.BidirectionalStart;
8458
import org.springframework.data.neo4j.integration.shared.common.Club;
8559
import org.springframework.data.neo4j.integration.shared.common.DeepRelationships;
60+
import org.springframework.data.neo4j.integration.shared.common.DtoPersonProjection;
61+
import org.springframework.data.neo4j.integration.shared.common.EntitiesWithDynamicLabels;
8662
import org.springframework.data.neo4j.integration.shared.common.EntityWithConvertedId;
8763
import org.springframework.data.neo4j.integration.shared.common.Hobby;
8864
import org.springframework.data.neo4j.integration.shared.common.ImmutablePerson;
@@ -94,7 +70,9 @@
9470
import org.springframework.data.neo4j.integration.shared.common.PersonWithRelationshipWithProperties;
9571
import org.springframework.data.neo4j.integration.shared.common.Pet;
9672
import org.springframework.data.neo4j.integration.shared.common.SimilarThing;
73+
import org.springframework.data.neo4j.integration.shared.common.SimplePerson;
9774
import org.springframework.data.neo4j.integration.shared.common.ThingWithAssignedId;
75+
import org.springframework.data.neo4j.integration.shared.common.ThingWithFixedGeneratedId;
9876
import org.springframework.data.neo4j.integration.shared.common.ThingWithGeneratedId;
9977
import org.springframework.data.neo4j.integration.shared.common.WorksInClubRelationship;
10078
import org.springframework.data.neo4j.repository.ReactiveNeo4jRepository;
@@ -110,6 +88,28 @@
11088
import org.springframework.transaction.ReactiveTransactionManager;
11189
import org.springframework.transaction.annotation.EnableTransactionManagement;
11290
import org.springframework.transaction.reactive.TransactionalOperator;
91+
import reactor.core.publisher.Flux;
92+
import reactor.core.publisher.Mono;
93+
import reactor.test.StepVerifier;
94+
95+
import java.time.LocalDate;
96+
import java.util.ArrayList;
97+
import java.util.Arrays;
98+
import java.util.Collection;
99+
import java.util.Collections;
100+
import java.util.HashMap;
101+
import java.util.HashSet;
102+
import java.util.List;
103+
import java.util.Map;
104+
import java.util.Optional;
105+
import java.util.Set;
106+
import java.util.UUID;
107+
import java.util.stream.Collectors;
108+
import java.util.stream.IntStream;
109+
110+
import static org.assertj.core.api.Assertions.assertThat;
111+
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
112+
import static org.assertj.core.api.Assertions.tuple;
113113

114114
/**
115115
* @author Gerrit Meier
@@ -869,9 +869,14 @@ void loadEntityWithBidirectionalRelationship(@Autowired BidirectionalStartReposi
869869
startId = startNode.id();
870870
}
871871

872-
StepVerifier.create(repository.findById(startId)).assertNext(entity -> {
873-
assertThat(entity.getEnds()).hasSize(1);
874-
}).verifyComplete();
872+
StepVerifier.create(repository.findById(startId))
873+
.verifyErrorMatches(error -> {
874+
Throwable cause = error.getCause();
875+
return cause instanceof MappingException && cause.getMessage().equals(
876+
"The node with id " + startId + " has a logical cyclic mapping dependency. " +
877+
"Its creation caused the creation of another node that has a reference to this.");
878+
});
879+
875880
}
876881

877882
@Test

0 commit comments

Comments
 (0)