GH-2118 - Don't overwrite unknown properties of known nodes and relationships.

This commit fixes the behaviour of overwrwiting unknown properties of
nodes and relationships: Instead of using the SET operation we must use
MUTATE, effectivly creating a `n += properties` instead of `n =
properties` assignment.

Furthermore, it requires a change by users: We offered the possibility
to use `@RelationshipProperties` without the need for a field storing
the internal id. The id however _is_ needed to check whether a
relationship already exist or not. While this is irrelevant for
relationships without properties, it is very much relevant for the ones
having properties.

We log a warning now about the fact. That will be a hard requirement for
6.1.

To make this work, the verification of the presence of an id property
needed to be reworked. It id property is now stored in a thread safe
lazy as well and assigned during the creation of a persistent entity.

Some minor improvements have been added as well:
* We don't pass the literal value for the target id when saving new
  relationships
* We avoid some parameters in the Cypher generator
* We optimized a condition in the converter so that it leaves evaluating
  retrieved values early

This closes #2118 and requires Cypher-DSL 2020.1.6.

Author: michael-simons

Diff for: pom.xml

@@ -76,7 +76,7 @@
 		<cdi>2.0</cdi>
 		<changelist>-SNAPSHOT</changelist>
 		<checkstyle.version>8.29</checkstyle.version>
-		<cypher-dsl.version>2020.1.4</cypher-dsl.version>
+		<cypher-dsl.version>2020.1.6</cypher-dsl.version>
 		<dist.id>spring-data-neo4j</dist.id>
 		<dist.key>SDNEO4J</dist.key>
 		<flatten-maven-plugin.version>1.2.5</flatten-maven-plugin.version>

Diff for: src/main/asciidoc/faq/faq.adoc

@@ -288,6 +288,9 @@ public final class Person {
 @RelationshipProperties
 public final class Actor {
 
+	@Id @GeneratedValue
+	private final Long id;
+
 	@TargetNode
 	private final Person person;
 

Diff for: src/main/asciidoc/object-mapping/mapping.adoc

@@ -160,6 +160,10 @@ A relationship property class and its usage may look like this:
 include::../../../../src/test/java/org/springframework/data/neo4j/documentation/domain/Roles.java[tags=mapping.relationship.properties]
 ----
 
+You should define a property for the generated, internal ID so that SDN can determine during save which relationships
+can be safely overwritten without losing properties.
+A generated id property is optional in SDN 6.0.4 and upwards and will be required in SDN 6.1 for `@RelationshipProperties`.
+
 .Defining relationship properties for an entity
 [source,java,indent=0]
 ----

Diff for: src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java

@@ -69,6 +69,7 @@
 /**
  * @author Michael J. Simons
  * @author Philipp Tölle
+ * @author Gerrit Meier
  * @soundtrack Motörhead - We Are Motörhead
  * @since 6.0
  */
@@ -237,7 +238,7 @@ private <T> T saveImpl(T instance, @Nullable String inDatabase) {
 		Optional<Long> optionalInternalId = neo4jClient
 				.query(() -> renderer.render(cypherGenerator.prepareSaveOf(entityMetaData, dynamicLabels)))
 				.in(inDatabase)
-				.bind((T) entityToBeSaved)
+				.bind(entityToBeSaved)
 				.with(neo4jMappingContext.getRequiredBinderFunctionFor((Class<T>) entityToBeSaved.getClass()))
 				.fetchAs(Long.class).one();
 
@@ -443,24 +444,32 @@ private void processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Ob
 				new NestedRelationshipProcessingStateMachine());
 	}
 
-	private void processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
+	private void processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Object parentObject,
 			boolean isParentObjectNew, @Nullable String inDatabase, NestedRelationshipProcessingStateMachine stateMachine) {
 
-		PersistentPropertyAccessor<?> propertyAccessor = neo4jPersistentEntity.getPropertyAccessor(parentObject);
-		Object fromId = propertyAccessor.getProperty(neo4jPersistentEntity.getRequiredIdProperty());
+		PersistentPropertyAccessor<?> propertyAccessor = sourceEntity.getPropertyAccessor(parentObject);
+		Object fromId = propertyAccessor.getProperty(sourceEntity.getRequiredIdProperty());
 
-		neo4jPersistentEntity.doWithAssociations((AssociationHandler<Neo4jPersistentProperty>) association -> {
+		sourceEntity.doWithAssociations((AssociationHandler<Neo4jPersistentProperty>) association -> {
 
 			// create context to bundle parameters
 			NestedRelationshipContext relationshipContext = NestedRelationshipContext.of(association, propertyAccessor,
-					neo4jPersistentEntity);
+					sourceEntity);
 
 			Collection<?> relatedValuesToStore = MappingSupport.unifyRelationshipValue(relationshipContext.getInverse(),
 					relationshipContext.getValue());
 
 			RelationshipDescription relationshipDescription = relationshipContext.getRelationship();
 			RelationshipDescription relationshipDescriptionObverse = relationshipDescription.getRelationshipObverse();
 
+			Neo4jPersistentProperty idProperty;
+			if (!relationshipDescription.hasInternalIdProperty()) {
+				idProperty = null;
+			} else {
+				Neo4jPersistentEntity<?> relationshipPropertiesEntity = (Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity();
+				idProperty = relationshipPropertiesEntity.getIdProperty();
+			}
+
 			// break recursive procession and deletion of previously created relationships
 			ProcessState processState = stateMachine.getStateOf(relationshipDescriptionObverse, relatedValuesToStore);
 			if (processState == ProcessState.PROCESSED_ALL_RELATIONSHIPS) {
@@ -470,12 +479,29 @@ private void processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEnti
 			// 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 (!isParentObjectNew) {
-				Statement relationshipRemoveQuery = cypherGenerator.prepareDeleteOf(neo4jPersistentEntity,
-						relationshipDescription);
+
+				List<Long> knownRelationshipsIds = new ArrayList<>();
+				if (idProperty != null) {
+					for (Object relatedValueToStore : relatedValuesToStore) {
+						if (relatedValueToStore == null) {
+							continue;
+						}
+
+						Long id = (Long) relationshipContext.getRelationshipPropertiesPropertyAccessor(relatedValueToStore).getProperty(idProperty);
+						if (id != null) {
+							knownRelationshipsIds.add(id);
+						}
+					}
+				}
+
+				Statement relationshipRemoveQuery = cypherGenerator.prepareDeleteOf(sourceEntity, relationshipDescription);
 
 				neo4jClient.query(renderer.render(relationshipRemoveQuery)).in(inDatabase)
-						.bind(convertIdValues(neo4jPersistentEntity.getIdProperty(), fromId))
-						.to(Constants.FROM_ID_PARAMETER_NAME).run();
+						.bind(convertIdValues(sourceEntity.getIdProperty(), fromId)) //
+							.to(Constants.FROM_ID_PARAMETER_NAME) //
+						.bind(knownRelationshipsIds) //
+							.to(Constants.NAME_OF_KNOWN_RELATIONSHIPS_PARAM) //
+						.run();
 			}
 
 			// nothing to do because there is nothing to map
@@ -489,33 +515,39 @@ private void processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEnti
 
 				// here a map entry is not always anymore a dynamic association
 				Object relatedNode = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
-				Neo4jPersistentEntity<?> targetNodeDescription = neo4jMappingContext
-						.getPersistentEntity(relatedNode.getClass());
+				Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedNode.getClass());
 
-				boolean isEntityNew = targetNodeDescription.isNew(relatedNode);
+				boolean isEntityNew = targetEntity.isNew(relatedNode);
 
 				relatedNode = eventSupport.maybeCallBeforeBind(relatedNode);
 
 				Long relatedInternalId = saveRelatedNode(relatedNode, relationshipContext.getAssociationTargetType(),
-						targetNodeDescription, inDatabase);
+						targetEntity, inDatabase);
 
 				CreateRelationshipStatementHolder statementHolder = neo4jMappingContext.createStatement(
-						neo4jPersistentEntity, relationshipContext, relatedInternalId, relatedValueToStore);
-
-				neo4jClient.query(renderer.render(statementHolder.getStatement())).in(inDatabase)
-						.bind(convertIdValues(targetNodeDescription.getRequiredIdProperty(), fromId))
-						.to(Constants.FROM_ID_PARAMETER_NAME).bindAll(statementHolder.getProperties())
-						.run();
+						sourceEntity, relationshipContext, relatedValueToStore);
+
+				Optional<Long> relationshipInternalId = neo4jClient.query(renderer.render(statementHolder.getStatement())).in(inDatabase)
+						.bind(convertIdValues(targetEntity.getRequiredIdProperty(), fromId)) //
+							.to(Constants.FROM_ID_PARAMETER_NAME)
+						.bind(relatedInternalId) //
+							.to(Constants.TO_ID_PARAMETER_NAME) //
+						.bindAll(statementHolder.getProperties())
+						.fetchAs(Long.class).one();
+
+				if (idProperty != null) {
+					relationshipContext
+							.getRelationshipPropertiesPropertyAccessor(relatedValueToStore)
+							.setProperty(idProperty, relationshipInternalId.get());
+				}
 
 				// if an internal id is used this must get set to link this entity in the next iteration
-				if (targetNodeDescription.isUsingInternalIds()) {
-					PersistentPropertyAccessor<?> targetPropertyAccessor = targetNodeDescription
-							.getPropertyAccessor(relatedNode);
-					targetPropertyAccessor
-							.setProperty(targetNodeDescription.getRequiredIdProperty(), relatedInternalId);
+				if (targetEntity.isUsingInternalIds()) {
+					PersistentPropertyAccessor<?> targetPropertyAccessor = targetEntity.getPropertyAccessor(relatedNode);
+					targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(), relatedInternalId);
 				}
 				if (processState != ProcessState.PROCESSED_ALL_VALUES) {
-					processNestedRelations(targetNodeDescription, relatedNode, isEntityNew, inDatabase, stateMachine);
+					processNestedRelations(targetEntity, relatedNode, isEntityNew, inDatabase, stateMachine);
 				}
 			}
 		});

Diff for: src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java

@@ -40,7 +40,6 @@
 import org.neo4j.cypherdsl.core.Statement;
 import org.neo4j.cypherdsl.core.renderer.Renderer;
 import org.neo4j.driver.exceptions.NoSuchRecordException;
-import org.neo4j.driver.summary.ResultSummary;
 import org.neo4j.driver.summary.SummaryCounters;
 import org.springframework.beans.BeansException;
 import org.springframework.beans.factory.BeanFactory;
@@ -232,7 +231,7 @@ private <T> Mono<T> saveImpl(T instance, @Nullable String inDatabase) {
 					Statement saveStatement = cypherGenerator.prepareSaveOf(entityMetaData, dynamicLabels);
 
 					Mono<Long> idMono = this.neo4jClient.query(() -> renderer.render(saveStatement)).in(inDatabase)
-							.bind((T) entity).with(neo4jMappingContext.getRequiredBinderFunctionFor((Class<T>) entity.getClass()))
+							.bind(entity).with(neo4jMappingContext.getRequiredBinderFunctionFor((Class<T>) entity.getClass()))
 							.fetchAs(Long.class).one().switchIfEmpty(Mono.defer(() -> {
 								if (entityMetaData.hasVersionProperty()) {
 									return Mono.error(() -> new OptimisticLockingFailureException(OPTIMISTIC_LOCKING_ERROR_MESSAGE));
@@ -440,26 +439,34 @@ private Mono<Void> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEnti
 				new NestedRelationshipProcessingStateMachine());
 	}
 
-	private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
+	private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Object parentObject,
 		  boolean isParentObjectNew, @Nullable String inDatabase, NestedRelationshipProcessingStateMachine stateMachine) {
 
 		return Mono.defer(() -> {
-			PersistentPropertyAccessor<?> propertyAccessor = neo4jPersistentEntity.getPropertyAccessor(parentObject);
-			Object fromId = propertyAccessor.getProperty(neo4jPersistentEntity.getRequiredIdProperty());
+			PersistentPropertyAccessor<?> propertyAccessor = sourceEntity.getPropertyAccessor(parentObject);
+			Object fromId = propertyAccessor.getProperty(sourceEntity.getRequiredIdProperty());
 			List<Mono<Void>> relationshipCreationMonos = new ArrayList<>();
 
-			neo4jPersistentEntity.doWithAssociations((AssociationHandler<Neo4jPersistentProperty>) association -> {
+			sourceEntity.doWithAssociations((AssociationHandler<Neo4jPersistentProperty>) association -> {
 
 				// create context to bundle parameters
 				NestedRelationshipContext relationshipContext = NestedRelationshipContext.of(association, propertyAccessor,
-						neo4jPersistentEntity);
+						sourceEntity);
 
 				Collection<?> relatedValuesToStore = MappingSupport.unifyRelationshipValue(relationshipContext.getInverse(),
 						relationshipContext.getValue());
 
 				RelationshipDescription relationshipDescription = relationshipContext.getRelationship();
 				RelationshipDescription relationshipDescriptionObverse = relationshipDescription.getRelationshipObverse();
 
+				Neo4jPersistentProperty idProperty;
+				if (!relationshipDescription.hasInternalIdProperty()) {
+					idProperty = null;
+				} else {
+					Neo4jPersistentEntity<?> relationshipPropertiesEntity = (Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity();
+					idProperty = relationshipPropertiesEntity.getIdProperty();
+				}
+
 				// break recursive procession and deletion of previously created relationships
 				ProcessState processState = stateMachine.getStateOf(relationshipDescriptionObverse, relatedValuesToStore);
 				if (processState == ProcessState.PROCESSED_ALL_RELATIONSHIPS) {
@@ -469,13 +476,32 @@ private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> neo4jPersiste
 				// 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 (!isParentObjectNew) {
-					Statement relationshipRemoveQuery = cypherGenerator.prepareDeleteOf(neo4jPersistentEntity,
-							relationshipDescription);
+
+					List<Long> knownRelationshipsIds = new ArrayList<>();
+					if (idProperty != null) {
+						for (Object relatedValueToStore : relatedValuesToStore) {
+							if (relatedValueToStore == null) {
+								continue;
+							}
+
+							Long id = (Long) relationshipContext
+									.getRelationshipPropertiesPropertyAccessor(relatedValueToStore)
+									.getProperty(idProperty);
+							if (id != null) {
+								knownRelationshipsIds.add(id);
+							}
+						}
+					}
+
+					Statement relationshipRemoveQuery = cypherGenerator.prepareDeleteOf(sourceEntity, relationshipDescription);
 
 					relationshipCreationMonos.add(
 							neo4jClient.query(renderer.render(relationshipRemoveQuery)).in(inDatabase)
-									.bind(convertIdValues(neo4jPersistentEntity.getIdProperty(), fromId))
-									.to(Constants.FROM_ID_PARAMETER_NAME).run().checkpoint("delete relationships").then());
+									.bind(convertIdValues(sourceEntity.getIdProperty(), fromId)) //
+										.to(Constants.FROM_ID_PARAMETER_NAME) //
+									.bind(knownRelationshipsIds) //
+										.to(Constants.NAME_OF_KNOWN_RELATIONSHIPS_PARAM) //
+									.run().checkpoint("delete relationships").then());
 				}
 
 				// nothing to do because there is nothing to map
@@ -487,37 +513,47 @@ private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> neo4jPersiste
 
 				for (Object relatedValueToStore : relatedValuesToStore) {
 
-					Object valueToBeSavedPreEvt = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
+					Object relatedNodePreEvt = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
 
-					Mono<Void> createRelationship = eventSupport.maybeCallBeforeBind(valueToBeSavedPreEvt)
-							.flatMap(valueToBeSaved -> {
-								Neo4jPersistentEntity<?> targetNodeDescription = neo4jMappingContext
-										.getPersistentEntity(valueToBeSavedPreEvt.getClass());
-								return Mono.just(targetNodeDescription.isNew(valueToBeSaved)).flatMap(isNew ->
-										saveRelatedNode(valueToBeSaved, relationshipContext.getAssociationTargetType(),
-										targetNodeDescription, inDatabase).flatMap(relatedInternalId -> {
+					Mono<Void> createRelationship = eventSupport.maybeCallBeforeBind(relatedNodePreEvt)
+							.flatMap(relatedNode -> {
+								Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext
+										.getPersistentEntity(relatedNodePreEvt.getClass());
+								return Mono.just(targetEntity.isNew(relatedNode)).flatMap(isNew ->
+										saveRelatedNode(relatedNode, relationshipContext.getAssociationTargetType(),
+										targetEntity, inDatabase).flatMap(relatedInternalId -> {
 
 											// if an internal id is used this must get set to link this entity in the next iteration
-											if (targetNodeDescription.isUsingInternalIds()) {
-												PersistentPropertyAccessor<?> targetPropertyAccessor = targetNodeDescription
-														.getPropertyAccessor(valueToBeSaved);
-												targetPropertyAccessor.setProperty(targetNodeDescription.getRequiredIdProperty(),
+											if (targetEntity.isUsingInternalIds()) {
+												PersistentPropertyAccessor<?> targetPropertyAccessor = targetEntity
+														.getPropertyAccessor(relatedNode);
+												targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(),
 														relatedInternalId);
 											}
 
 											CreateRelationshipStatementHolder statementHolder = neo4jMappingContext.createStatement(
-													neo4jPersistentEntity, relationshipContext, relatedInternalId, relatedValueToStore);
+													sourceEntity, relationshipContext, relatedValueToStore);
 
 											// in case of no properties the bind will just return an empty map
-											Mono<ResultSummary> relationshipCreationMonoNested = neo4jClient
+											Mono<Long> relationshipCreationMonoNested = neo4jClient
 													.query(renderer.render(statementHolder.getStatement())).in(inDatabase)
-													.bind(convertIdValues(targetNodeDescription.getRequiredIdProperty(), fromId))
-													.to(Constants.FROM_ID_PARAMETER_NAME)
-													.bindAll(statementHolder.getProperties()).run();
+													.bind(convertIdValues(targetEntity.getRequiredIdProperty(), fromId)) //
+														.to(Constants.FROM_ID_PARAMETER_NAME) //
+													.bind(relatedInternalId) //
+														.to(Constants.TO_ID_PARAMETER_NAME) //
+													.bindAll(statementHolder.getProperties())
+													.fetchAs(Long.class).one()
+													.doOnNext(relationshipInternalId -> {
+														if (idProperty != null) {
+															relationshipContext
+																	.getRelationshipPropertiesPropertyAccessor(relatedValueToStore)
+																	.setProperty(idProperty, relationshipInternalId);
+														}
+													});
 
 											if (processState != ProcessState.PROCESSED_ALL_VALUES) {
 												return relationshipCreationMonoNested.checkpoint().then(
-														processNestedRelations(targetNodeDescription, valueToBeSaved,
+														processNestedRelations(targetEntity, relatedNode,
 																isNew, inDatabase, stateMachine));
 											} else {
 												return relationshipCreationMonoNested.checkpoint().then();

Diff for: src/main/java/org/springframework/data/neo4j/core/mapping/Constants.java

@@ -52,6 +52,7 @@ public final class Constants {
 	 */
 	public static final String NAME_OF_STATIC_LABELS_PARAM = "__staticLabels__";
 	public static final String NAME_OF_ENTITY_LIST_PARAM = "__entities__";
+	public static final String NAME_OF_KNOWN_RELATIONSHIPS_PARAM = "__knownRelationShipIds__";
 	public static final String NAME_OF_PATHS = "__paths__";
 	public static final String NAME_OF_ALL_PROPERTIES = "__allProperties__";
 
@@ -60,6 +61,7 @@ public final class Constants {
 	public static final String NAME_OF_SYNTHESIZED_RELATIONS = "__sr__";
 
 	public static final String FROM_ID_PARAMETER_NAME = "fromId";
+	public static final String TO_ID_PARAMETER_NAME = "toId";
 
 	private Constants() {
 	}