Fix saving of bidirectional relationships.

If a bidirectional relationship on the same type gets modelled as OUTGOING
the mapping will run endless following the relationship until a stack overflow occurs.

This commits also improves on the state handling and the wording of nested relationships being processed.

Co-authored-by: Michael Simons <michael.simons@neo4j.com>
This commit is contained in:
Gerrit Meier
2020-06-18 11:47:23 +02:00
committed by GitHub
parent e39ccf07cf
commit 555c98b2c2
7 changed files with 227 additions and 62 deletions

View File

@@ -26,11 +26,9 @@ import static org.neo4j.springframework.data.core.schema.Constants.*;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import org.apache.commons.logging.LogFactory;
@@ -236,11 +234,11 @@ public final class Neo4jTemplate implements Neo4jOperations, BeanFactoryAware {
PersistentPropertyAccessor<T> propertyAccessor = entityMetaData.getPropertyAccessor(entityToBeSaved);
if (!entityMetaData.isUsingInternalIds()) {
processAssociations(entityMetaData, entityToBeSaved, inDatabase);
processRelations(entityMetaData, entityToBeSaved, inDatabase);
return entityToBeSaved;
} else {
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), optionalInternalId.get());
processAssociations(entityMetaData, entityToBeSaved, inDatabase);
processRelations(entityMetaData, entityToBeSaved, inDatabase);
return propertyAccessor.getBean();
}
@@ -314,9 +312,7 @@ public final class Neo4jTemplate implements Neo4jOperations, BeanFactoryAware {
.run();
// Save related
entitiesToBeSaved.forEach(entityToBeSaved -> {
processAssociations(entityMetaData, entityToBeSaved, databaseName);
});
entitiesToBeSaved.forEach(entityToBeSaved -> processRelations(entityMetaData, entityToBeSaved, databaseName));
SummaryCounters counters = resultSummary.counters();
log.debug(() -> String
@@ -403,13 +399,13 @@ public final class Neo4jTemplate implements Neo4jOperations, BeanFactoryAware {
return toExecutableQuery(preparedQuery);
}
private void processAssociations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
@Nullable String inDatabase) {
processNestedAssociations(neo4jPersistentEntity, parentObject, inDatabase, new HashSet<>());
private void processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject, @Nullable String inDatabase) {
processNestedRelations(neo4jPersistentEntity, parentObject, inDatabase, new NestedRelationshipProcessState());
}
private void processNestedAssociations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
@Nullable String inDatabase, Set<RelationshipDescription> processedRelationshipDescriptions) {
private void processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
@Nullable String inDatabase, NestedRelationshipProcessState processState) {
PersistentPropertyAccessor<?> propertyAccessor = neo4jPersistentEntity.getPropertyAccessor(parentObject);
@@ -419,9 +415,14 @@ public final class Neo4jTemplate implements Neo4jOperations, BeanFactoryAware {
NestedRelationshipContext relationshipContext = NestedRelationshipContext
.of(handler, propertyAccessor, neo4jPersistentEntity);
Collection<?> relatedValuesToStore = Relationships
.unifyRelationshipValue(relationshipContext.getInverse(), relationshipContext.getValue());
RelationshipDescription relationshipDescription = relationshipContext.getRelationship();
RelationshipDescription relationshipDescriptionObverse = relationshipDescription.getRelationshipObverse();
// break recursive procession and deletion of previously created relationships
RelationshipDescription relationshipObverse = relationshipContext.getRelationship().getRelationshipObverse();
if (hasProcessed(processedRelationshipDescriptions, relationshipObverse)) {
if (processState.hasProcessedEither(relationshipDescriptionObverse, relatedValuesToStore)) {
return;
}
@@ -433,7 +434,7 @@ public final class Neo4jTemplate implements Neo4jOperations, BeanFactoryAware {
// this avoids the usage of cache but might have significant impact on overall performance
if (!neo4jPersistentEntity.isNew(parentObject)) {
Statement relationshipRemoveQuery = cypherGenerator.createRelationshipRemoveQuery(neo4jPersistentEntity,
relationshipContext.getRelationship(), relationshipsToRemoveDescription);
relationshipDescription, relationshipsToRemoveDescription);
neo4jClient.query(renderer.render(relationshipRemoveQuery))
.in(inDatabase)
@@ -445,13 +446,12 @@ public final class Neo4jTemplate implements Neo4jOperations, BeanFactoryAware {
return;
}
processedRelationshipDescriptions.add(relationshipContext.getRelationship());
processState.markAsProcessed(relationshipDescription, relatedValuesToStore);
for (Object relatedValue : Relationships
.unifyRelationshipValue(relationshipContext.getInverse(), relationshipContext.getValue())) {
for (Object relatedValueToStore : relatedValuesToStore) {
// here map entry is not always anymore a dynamic association
Object valueToBeSaved = relationshipContext.identifyAndExtractRelationshipValue(relatedValue);
Object valueToBeSaved = relationshipContext.identifyAndExtractRelationshipValue(relatedValueToStore);
Neo4jPersistentEntity<?> targetNodeDescription = neo4jMappingContext.getPersistentEntity(valueToBeSaved.getClass());
@@ -461,7 +461,7 @@ public final class Neo4jTemplate implements Neo4jOperations, BeanFactoryAware {
targetNodeDescription, inDatabase);
RelationshipStatementHolder statementHolder = RelationshipStatementHolder.createStatement(
neo4jMappingContext, neo4jPersistentEntity, relationshipContext, relatedInternalId, relatedValue);
neo4jMappingContext, neo4jPersistentEntity, relationshipContext, relatedInternalId, relatedValueToStore);
neo4jClient.query(renderer.render(statementHolder.getRelationshipCreationQuery()))
.in(inDatabase)
@@ -476,20 +476,11 @@ public final class Neo4jTemplate implements Neo4jOperations, BeanFactoryAware {
targetPropertyAccessor
.setProperty(targetNodeDescription.getRequiredIdProperty(), relatedInternalId);
}
processNestedAssociations(targetNodeDescription, valueToBeSaved, inDatabase, processedRelationshipDescriptions);
processNestedRelations(targetNodeDescription, valueToBeSaved, inDatabase, processState);
}
});
}
private boolean hasProcessed(Set<RelationshipDescription> processedRelationshipDescriptions,
RelationshipDescription relationshipDescription) {
if (relationshipDescription != null) {
return processedRelationshipDescriptions.contains(relationshipDescription);
}
return false;
}
private <Y> Long saveRelatedNode(Object entity, Class<Y> entityType, NodeDescription targetNodeDescription, @Nullable String inDatabase) {
DynamicLabels dynamicLabels = determineDynamicLabels(entity, (Neo4jPersistentEntity) targetNodeDescription, inDatabase);

View File

@@ -25,6 +25,7 @@ import org.neo4j.springframework.data.core.mapping.Neo4jPersistentProperty;
import org.neo4j.springframework.data.core.schema.RelationshipDescription;
import org.springframework.data.mapping.Association;
import org.springframework.data.mapping.PersistentPropertyAccessor;
import org.springframework.lang.Nullable;
/**
* Working on nested relationships happens in a certain algorithmic context.
@@ -33,6 +34,7 @@ import org.springframework.data.mapping.PersistentPropertyAccessor;
* the algorithm.
*
* @author Philipp Tölle
* @author Gerrit Meier
* @since 1.0
*/
final class NestedRelationshipContext {
@@ -43,7 +45,7 @@ final class NestedRelationshipContext {
private final boolean inverseValueIsEmpty;
private NestedRelationshipContext(Neo4jPersistentProperty inverse, Object value,
private NestedRelationshipContext(Neo4jPersistentProperty inverse, @Nullable Object value,
RelationshipDescription relationship, Class<?> associationTargetType, boolean inverseValueIsEmpty) {
this.inverse = inverse;
this.value = value;
@@ -56,6 +58,7 @@ final class NestedRelationshipContext {
return inverse;
}
@Nullable
Object getValue() {
return value;
}

View File

@@ -0,0 +1,102 @@
/*
* Copyright (c) 2019-2020 "Neo4j,"
* Neo4j Sweden AB [https://neo4j.com]
*
* This file is part of Neo4j.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.neo4j.springframework.data.core;
import java.util.Collection;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.neo4j.springframework.data.core.schema.RelationshipDescription;
import org.springframework.lang.Nullable;
/**
* This stores all processed nested relations and objects during save of objects so that the recursive descent can be
* stopped accordingly.
*
* @author Michael J. Simons
* @soundtrack Helge Schneider - Heart Attack No. 1
*/
final class NestedRelationshipProcessState {
private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
private final Lock read = lock.readLock();
private final Lock write = lock.writeLock();
/**
* The set of already processed relationships.
*/
private final Set<RelationshipDescription> processedRelationshipDescriptions = new HashSet<>();
/**
* The set of already processed related objects.
*/
private final Set<Object> processedObjects = new HashSet<>();
/**
* @param relationshipDescription Check whether this relationship description has been processed
* @param valuesToStore Check whether all the values in the collection have been processed
* @return True, if either the relationship has been already process or all of the values to store.
*/
boolean hasProcessedEither(RelationshipDescription relationshipDescription, @Nullable Collection<?> valuesToStore) {
try {
read.lock();
return hasProcessed(relationshipDescription) || hasProcessedAllOf(valuesToStore);
} finally {
read.unlock();
}
}
/**
* Marks the passed objects as processed
*
* @param relationshipDescription To be marked as processed
* @param valuesToStore If not {@literal null}, all non-null values will be marked as processed
*/
void markAsProcessed(RelationshipDescription relationshipDescription, @Nullable Collection<?> valuesToStore) {
try {
write.lock();
this.processedRelationshipDescriptions.add(relationshipDescription);
if (valuesToStore != null) {
valuesToStore.stream().filter(v -> v != null).forEach(processedObjects::add);
}
} finally {
write.unlock();
}
}
private boolean hasProcessedAllOf(@Nullable Collection<?> valuesToStore) {
// there can be null elements in the unified collection of values to store.
if (valuesToStore == null) {
return false;
}
return processedObjects.containsAll(valuesToStore);
}
private boolean hasProcessed(RelationshipDescription relationshipDescription) {
if (relationshipDescription != null) {
return processedRelationshipDescriptions.contains(relationshipDescription);
}
return false;
}
}

View File

@@ -23,7 +23,6 @@ import static java.util.stream.Collectors.*;
import static org.neo4j.springframework.data.core.DatabaseSelection.*;
import static org.neo4j.opencypherdsl.Cypher.*;
import static org.neo4j.springframework.data.core.schema.Constants.*;
import static org.neo4j.springframework.data.core.support.Relationships.*;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
@@ -33,10 +32,8 @@ import reactor.util.function.Tuples;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;
import org.apache.commons.logging.LogFactory;
@@ -54,6 +51,7 @@ import org.neo4j.springframework.data.core.mapping.Neo4jPersistentProperty;
import org.neo4j.springframework.data.core.schema.CypherGenerator;
import org.neo4j.springframework.data.core.schema.NodeDescription;
import org.neo4j.springframework.data.core.schema.RelationshipDescription;
import org.neo4j.springframework.data.core.support.Relationships;
import org.neo4j.springframework.data.repository.event.ReactiveBeforeBindCallback;
import org.springframework.beans.BeansException;
import org.springframework.beans.factory.BeanFactory;
@@ -249,15 +247,14 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Bea
if (!entityMetaData.isUsingInternalIds()) {
return idMono.then(processAssociations(entityMetaData, entity, inDatabase))
.thenReturn(entity);
return idMono.then(processRelations(entityMetaData, entity, inDatabase)).thenReturn(entity);
} else {
return idMono.map(internalId -> {
PersistentPropertyAccessor<T> propertyAccessor = entityMetaData.getPropertyAccessor(entity);
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), internalId);
return propertyAccessor.getBean();
}).flatMap(savedEntity -> processAssociations(entityMetaData, savedEntity, inDatabase)
}).flatMap(savedEntity -> processRelations(entityMetaData, savedEntity, inDatabase)
.thenReturn(savedEntity));
}
});
@@ -408,14 +405,13 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Bea
return this.toExecutableQuery(preparedQuery);
}
private Mono<Void> processAssociations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
@Nullable String inDatabase) {
private Mono<Void> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject, @Nullable String inDatabase) {
return processNestedAssociations(neo4jPersistentEntity, parentObject, inDatabase, new HashSet<>());
return processNestedRelations(neo4jPersistentEntity, parentObject, inDatabase, new NestedRelationshipProcessState());
}
private Mono<Void> processNestedAssociations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
@Nullable String inDatabase, Set<RelationshipDescription> processedRelationshipDescriptions) {
private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
@Nullable String inDatabase, NestedRelationshipProcessState processState) {
return Mono.defer(() -> {
PersistentPropertyAccessor<?> propertyAccessor = neo4jPersistentEntity.getPropertyAccessor(parentObject);
@@ -428,9 +424,14 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Bea
NestedRelationshipContext relationshipContext = NestedRelationshipContext
.of(handler, propertyAccessor, neo4jPersistentEntity);
Collection<?> relatedValuesToStore = Relationships
.unifyRelationshipValue(relationshipContext.getInverse(), relationshipContext.getValue());
RelationshipDescription relationshipDescription = relationshipContext.getRelationship();
RelationshipDescription relationshipDescriptionObverse = relationshipDescription.getRelationshipObverse();
// break recursive procession and deletion of previously created relationships
RelationshipDescription relationshipObverse = relationshipContext.getRelationship().getRelationshipObverse();
if (hasProcessed(processedRelationshipDescriptions, relationshipObverse)) {
if (processState.hasProcessedEither(relationshipDescriptionObverse, relatedValuesToStore)) {
return;
}
@@ -440,9 +441,7 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Bea
// remove all relationships before creating all new if the entity is not new
// this avoids the usage of cache but might have significant impact on overall performance
if (!neo4jPersistentEntity.isNew(parentObject)) {
Statement relationshipRemoveQuery = cypherGenerator
.createRelationshipRemoveQuery(neo4jPersistentEntity, relationshipContext.getRelationship(),
targetNodeDescription);
Statement relationshipRemoveQuery = cypherGenerator.createRelationshipRemoveQuery(neo4jPersistentEntity, relationshipDescription, targetNodeDescription);
relationshipCreationMonos.add(
neo4jClient.query(renderer.render(relationshipRemoveQuery))
.in(inDatabase)
@@ -455,12 +454,11 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Bea
return;
}
processedRelationshipDescriptions.add(relationshipContext.getRelationship());
processState.markAsProcessed(relationshipDescription, relatedValuesToStore);
for (Object relatedValue : unifyRelationshipValue(relationshipContext.getInverse(),
relationshipContext.getValue())) {
for (Object relatedValueToStore : relatedValuesToStore) {
Object valueToBeSavedPreEvt = relationshipContext.identifyAndExtractRelationshipValue(relatedValue);
Object valueToBeSavedPreEvt = relationshipContext.identifyAndExtractRelationshipValue(relatedValueToStore);
Mono<Object> valueToBeSavedMono = eventSupport.maybeCallBeforeBind(valueToBeSavedPreEvt);
relationshipCreationMonos.add(
@@ -480,7 +478,7 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Bea
}
RelationshipStatementHolder statementHolder = RelationshipStatementHolder.createStatement(
neo4jMappingContext, neo4jPersistentEntity, relationshipContext, relatedInternalId, relatedValue);
neo4jMappingContext, neo4jPersistentEntity, relationshipContext, relatedInternalId, relatedValueToStore);
// in case of no properties the bind will just return an empty map
Mono<ResultSummary> relationshipCreationMonoNested = neo4jClient
@@ -491,7 +489,7 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Bea
.run();
return relationshipCreationMonoNested.checkpoint()
.then(processNestedAssociations(targetNodeDescription, valueToBeSaved, inDatabase, processedRelationshipDescriptions));
.then(processNestedRelations(targetNodeDescription, valueToBeSaved, inDatabase, processState));
}).checkpoint()));
}
});
@@ -500,15 +498,6 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Bea
});
}
private boolean hasProcessed(Set<RelationshipDescription> processedRelationshipDescriptions,
RelationshipDescription relationshipDescription) {
if (relationshipDescription != null) {
return processedRelationshipDescriptions.contains(relationshipDescription);
}
return false;
}
private <Y> Mono<Long> saveRelatedNode(Object relatedNode, Class<Y> entityType, NodeDescription targetNodeDescription,
@Nullable String inDatabase) {

View File

@@ -26,6 +26,7 @@ import java.util.Collections;
import java.util.Map;
import org.neo4j.springframework.data.core.mapping.Neo4jPersistentProperty;
import org.springframework.lang.Nullable;
/**
* @author Michael J. Simons
@@ -42,6 +43,7 @@ public final class Relationships {
* @return A unified collection (Either a collection of Map.Entry for dynamic and relationships with properties
* or a list of related values)
*/
@Nullable
public static Collection<?> unifyRelationshipValue(Neo4jPersistentProperty property, Object rawValue) {
Collection<?> unifiedValue;
if (property.isDynamicAssociation()) {

View File

@@ -754,6 +754,21 @@ class RepositoryIT {
}
@Test
void findEntityWithSelfReferencesInBothDirections(@Autowired PetRepository repository) {
long petId;
try (Session session = createSession()) {
petId = session.run("CREATE (luna:Pet{name:'Luna'})-[:Has]->(daphne:Pet{name:'Daphne'})"
+ "-[:Has]->(luna2:Pet{name:'Luna'})"
+ "RETURN id(luna) as id").single().get("id").asLong();
}
Pet loadedPet = repository.findById(petId).get();
assertThat(loadedPet.getFriends().get(0).getName()).isEqualTo("Daphne");
assertThat(loadedPet.getFriends().get(0).getFriends().get(0).getName()).isEqualTo("Luna");
}
@Test
void findEntityWithBidirectionalRelationshipFromIncomingSide(@Autowired BidirectionalEndRepository repository) {
@@ -1531,6 +1546,27 @@ class RepositoryIT {
}
}
@Test
void saveEntityWithSelfReferencesInBothDirections(@Autowired PetRepository repository) {
Pet luna = new Pet("Luna");
Pet daphne = new Pet("Daphne");
luna.setFriends(singletonList(daphne));
daphne.setFriends(singletonList(luna));
repository.save(luna);
try (Session session = createSession()) {
Record record = session.run("MATCH (luna:Pet{name:'Luna'})-[:Has]->(daphne:Pet{name:'Daphne'})"
+ "-[:Has]->(luna2:Pet{name:'Luna'})"
+ "RETURN luna, daphne, luna2").single();
assertThat(record.get("luna").asNode().get("name").asString()).isEqualTo("Luna");
assertThat(record.get("daphne").asNode().get("name").asString()).isEqualTo("Daphne");
assertThat(record.get("luna2").asNode().get("name").asString()).isEqualTo("Luna");
}
}
@Test
void saveEntityGraphWithSelfInverseRelationshipDefined(@Autowired SimilarThingRepository repository) {
SimilarThing originalThing = new SimilarThing().withName("Original");

View File

@@ -852,6 +852,24 @@ class ReactiveRepositoryIT {
.verifyComplete();
}
@Test
void findEntityWithSelfReferencesInBothDirections(@Autowired ReactivePetRepository repository) {
long petId;
try (Session session = createSession()) {
petId = session.run("CREATE (luna:Pet{name:'Luna'})-[:Has]->(daphne:Pet{name:'Daphne'})"
+ "-[:Has]->(luna2:Pet{name:'Luna'})"
+ "RETURN id(luna) as id").single().get("id").asLong();
}
StepVerifier.create(repository.findById(petId))
.assertNext(loadedPet -> {
assertThat(loadedPet.getFriends().get(0).getName()).isEqualTo("Daphne");
assertThat(loadedPet.getFriends().get(0).getFriends().get(0).getName()).isEqualTo("Luna");
})
.verifyComplete();
}
}
@Nested
@@ -1824,6 +1842,30 @@ class ReactiveRepositoryIT {
.expectNextCount(4)
.verifyComplete();
}
@Test
void saveEntityWithSelfReferencesInBothDirections(@Autowired ReactivePetRepository repository) {
Pet luna = new Pet("Luna");
Pet daphne = new Pet("Daphne");
luna.setFriends(singletonList(daphne));
daphne.setFriends(singletonList(luna));
StepVerifier.create(repository.save(luna))
.expectNextCount(1)
.verifyComplete();
try (Session session = createSession()) {
Record record = session.run("MATCH (luna:Pet{name:'Luna'})-[:Has]->(daphne:Pet{name:'Daphne'})"
+ "-[:Has]->(luna2:Pet{name:'Luna'})"
+ "RETURN luna, daphne, luna2").single();
assertThat(record.get("luna").asNode().get("name").asString()).isEqualTo("Luna");
assertThat(record.get("daphne").asNode().get("name").asString()).isEqualTo("Daphne");
assertThat(record.get("luna2").asNode().get("name").asString()).isEqualTo("Luna");
}
}
}
@Nested