GH-2347 - Track state of objects with assigned ids correctly.

This change makes the Cypher generator create a statement for persisting multiple instances with assigned ids that not only returns the assigned ID, but also the internally generated id. This id is needed for tracking the root objects state in the `NestedRelationshipProcessingStateMachine`.

Those ids must of course be retrieved. By doing so we currently loos the ability to debug log the number of nodes created.
While this removal of a log statement can be considered a breaking change, it is necessary.

To lighten that change: Storing multiple instances with internally generated did not debug log these informations at all.

This fixes #2347.

Author: michael-simons

src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java

@@ -19,6 +19,7 @@
 import static org.neo4j.cypherdsl.core.Cypher.asterisk;
 import static org.neo4j.cypherdsl.core.Cypher.parameter;
 
+import java.util.AbstractMap;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -42,9 +43,9 @@
 import org.neo4j.cypherdsl.core.Node;
 import org.neo4j.cypherdsl.core.Statement;
 import org.neo4j.cypherdsl.core.renderer.Renderer;
+import org.neo4j.driver.Value;
 import org.neo4j.driver.exceptions.NoSuchRecordException;
 import org.neo4j.driver.summary.ResultSummary;
-import org.neo4j.driver.summary.SummaryCounters;
 import org.neo4j.driver.types.Entity;
 import org.neo4j.driver.types.MapAccessor;
 import org.neo4j.driver.types.TypeSystem;
@@ -466,20 +467,22 @@ class Tuple3<T> {
 		Function<T, Map<String, Object>> binderFunction = neo4jMappingContext.getRequiredBinderFunctionFor(domainClass);
 		List<Map<String, Object>> entityList = entitiesToBeSaved.stream().map(h -> h.modifiedInstance).map(binderFunction)
 				.collect(Collectors.toList());
-		ResultSummary resultSummary = neo4jClient
+		Map<Value, Long> idToInternalIdMapping = neo4jClient
 				.query(() -> renderer.render(cypherGenerator.prepareSaveOfMultipleInstancesOf(entityMetaData)))
-				.bind(entityList).to(Constants.NAME_OF_ENTITY_LIST_PARAM).run();
-
-		SummaryCounters counters = resultSummary.counters();
-		log.debug(() -> String.format(
-				"Created %d and deleted %d nodes, created %d and deleted %d relationships and set %d properties.",
-				counters.nodesCreated(), counters.nodesDeleted(), counters.relationshipsCreated(),
-				counters.relationshipsDeleted(), counters.propertiesSet()));
+				.bind(entityList).to(Constants.NAME_OF_ENTITY_LIST_PARAM)
+				.fetchAs(Map.Entry.class)
+				.mappedBy((t, r) -> new AbstractMap.SimpleEntry<>(r.get(Constants.NAME_OF_ID), r.get(Constants.NAME_OF_INTERNAL_ID).asLong()))
+				.all()
+				.stream()
+				.collect(Collectors.toMap(m -> (Value) m.getKey(), m -> (Long) m.getValue()));
 
 		// Save related
 		return entitiesToBeSaved.stream().map(t -> {
 			PersistentPropertyAccessor<T> propertyAccessor = entityMetaData.getPropertyAccessor(t.modifiedInstance);
-			return processRelations(entityMetaData, t.originalInstance, propertyAccessor, t.wasNew, TemplateSupport.computeIncludePropertyPredicate(includedProperties, entityMetaData));
+			Neo4jPersistentProperty idProperty = entityMetaData.getRequiredIdProperty();
+			Object id = convertIdValues(idProperty, propertyAccessor.getProperty(idProperty));
+			Long internalId = idToInternalIdMapping.get(id);
+			return processRelations(entityMetaData, t.originalInstance, internalId, propertyAccessor, t.wasNew, TemplateSupport.computeIncludePropertyPredicate(includedProperties, entityMetaData));
 		}).collect(Collectors.toList());
 	}
 
@@ -632,22 +635,13 @@ private <T> ExecutableQuery<T> createExecutableQuery(Class<T> domainType, @Nulla
 	 * @param isParentObjectNew      A flag if the parent was new
 	 * @param includeProperty        A predicate telling to include a relationship property or not
 	 */
-	private <T> void processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance, Long internalId,
+	private <T> T processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance, Long internalId,
 								   PersistentPropertyAccessor<?> parentPropertyAccessor,
 								   boolean isParentObjectNew, PropertyFilter includeProperty) {
 
-		PropertyFilter.RelaxedPropertyPath startingPropertyPath = PropertyFilter.RelaxedPropertyPath.withRootType(neo4jPersistentEntity.getUnderlyingClass());
-		processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew,
-				new NestedRelationshipProcessingStateMachine(originalInstance, internalId), includeProperty, startingPropertyPath);
-	}
-
-	private <T> T processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance,
-			PersistentPropertyAccessor<?> parentPropertyAccessor,
-			boolean isParentObjectNew, PropertyFilter includeProperty) {
-
 		PropertyFilter.RelaxedPropertyPath startingPropertyPath = PropertyFilter.RelaxedPropertyPath.withRootType(neo4jPersistentEntity.getUnderlyingClass());
 		return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew,
-				new NestedRelationshipProcessingStateMachine(originalInstance), includeProperty, startingPropertyPath);
+				new NestedRelationshipProcessingStateMachine(originalInstance, internalId), includeProperty, startingPropertyPath);
 	}
 
 	private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, PersistentPropertyAccessor<?> propertyAccessor,

src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java

@@ -19,6 +19,7 @@
 import static org.neo4j.cypherdsl.core.Cypher.asterisk;
 import static org.neo4j.cypherdsl.core.Cypher.parameter;
 
+import org.neo4j.driver.Value;
 import org.springframework.data.mapping.PropertyPath;
 import org.springframework.data.neo4j.core.mapping.EntityFromDtoInstantiatingConverter;
 import org.springframework.data.neo4j.core.mapping.PropertyFilter;
@@ -49,7 +50,6 @@
 import org.neo4j.cypherdsl.core.Node;
 import org.neo4j.cypherdsl.core.Statement;
 import org.neo4j.cypherdsl.core.renderer.Renderer;
-import org.neo4j.driver.summary.SummaryCounters;
 import org.neo4j.driver.types.Entity;
 import org.neo4j.driver.types.MapAccessor;
 import org.neo4j.driver.types.TypeSystem;
@@ -499,18 +499,23 @@ private <T> Flux<T> saveAllImpl(Iterable<T> instances, @Nullable List<PropertyPa
 							.map(binderFunction).collect(Collectors.toList());
 					return neo4jClient
 							.query(() -> renderer.render(cypherGenerator.prepareSaveOfMultipleInstancesOf(entityMetaData)))
-							.bind(boundedEntityList).to(Constants.NAME_OF_ENTITY_LIST_PARAM).run();
-				}).doOnNext(resultSummary -> {
-					SummaryCounters counters = resultSummary.counters();
-					log.debug(() -> String.format(
-							"Created %d and deleted %d nodes, created %d and deleted %d relationships and set %d properties.",
-							counters.nodesCreated(), counters.nodesDeleted(), counters.relationshipsCreated(),
-							counters.relationshipsDeleted(), counters.propertiesSet()));
-				}).thenMany(Flux.fromIterable(entitiesToBeSaved)
-						.flatMap(t -> processRelations(entityMetaData, t.getT1(),
-								entityMetaData.getPropertyAccessor(t.getT3()), t.getT2(),
-								TemplateSupport.computeIncludePropertyPredicate(includedProperties, entityMetaData)))
-				));
+							.bind(boundedEntityList).to(Constants.NAME_OF_ENTITY_LIST_PARAM)
+							.fetchAs(Tuple2.class)
+							.mappedBy((t, r) -> Tuples.of(r.get(Constants.NAME_OF_ID), r.get(Constants.NAME_OF_INTERNAL_ID).asLong()))
+							.all()
+							.collectMap(m -> (Value) m.getT1(), m -> (Long) m.getT2());
+				}).flatMapMany(idToInternalIdMapping -> Flux.fromIterable(entitiesToBeSaved)
+						.flatMap(t -> {
+							PersistentPropertyAccessor<T> propertyAccessor = entityMetaData.getPropertyAccessor(t.getT3());
+							Neo4jPersistentProperty idProperty = entityMetaData.getRequiredIdProperty();
+							Object id = convertIdValues(idProperty, propertyAccessor.getProperty(idProperty));
+							Long internalId = idToInternalIdMapping.get(id);
+							return processRelations(entityMetaData, t.getT1(), internalId,
+									propertyAccessor, t.getT2(),
+								TemplateSupport.computeIncludePropertyPredicate(includedProperties,
+										entityMetaData));
+						}))
+				);
 	}
 
 	@Override
@@ -741,15 +746,6 @@ private <T> Mono<T> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEnt
 				new NestedRelationshipProcessingStateMachine(originalInstance, internalId), includeProperty, startingPropertyPath);
 	}
 
-	private <T> Mono<T> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance,
-			PersistentPropertyAccessor<?> parentPropertyAccessor,
-			boolean isParentObjectNew, PropertyFilter includeProperty) {
-
-		PropertyFilter.RelaxedPropertyPath startingPropertyPath = PropertyFilter.RelaxedPropertyPath.withRootType(neo4jPersistentEntity.getUnderlyingClass());
-		return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew,
-				new NestedRelationshipProcessingStateMachine(originalInstance), includeProperty, startingPropertyPath);
-	}
-
 	private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, PersistentPropertyAccessor<?> parentPropertyAccessor,
 											   boolean isParentObjectNew, NestedRelationshipProcessingStateMachine stateMachine, PropertyFilter includeProperty, PropertyFilter.RelaxedPropertyPath previousPath) {
 

src/main/java/org/springframework/data/neo4j/core/mapping/CypherGenerator.java

@@ -366,7 +366,8 @@ public Statement prepareSaveOfMultipleInstancesOf(NodeDescription<?> nodeDescrip
 		return Cypher.unwind(parameter(Constants.NAME_OF_ENTITY_LIST_PARAM)).as(row)
 				.merge(rootNode.withProperties(nameOfIdProperty, Cypher.property(row, Constants.NAME_OF_ID)))
 				.mutate(rootNode, Cypher.property(row, Constants.NAME_OF_PROPERTIES_PARAM))
-				.returning(Functions.collect(rootNode.property(nameOfIdProperty)).as(Constants.NAME_OF_IDS)).build();
+				.returning(rootNode.internalId().as(Constants.NAME_OF_INTERNAL_ID), rootNode.property(nameOfIdProperty).as(Constants.NAME_OF_ID))
+				.build();
 	}
 
 	@NonNull

src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java

@@ -27,6 +27,7 @@
 import org.apiguardian.api.API;
 import org.springframework.lang.NonNull;
 import org.springframework.lang.Nullable;
+import org.springframework.util.Assert;
 
 /**
  * This stores all processed nested relations and objects during save of objects so that the recursive descent can be
@@ -72,12 +73,12 @@ public enum ProcessState {
 	private final Map<Object, Long> processedObjectsIds = new HashMap<>();
 
 	public NestedRelationshipProcessingStateMachine(Object initialObject, Long internalId) {
-		this(initialObject);
-		processedObjectsIds.put(initialObject, internalId);
-	}
 
-	public NestedRelationshipProcessingStateMachine(Object initialObject) {
+		Assert.notNull(initialObject, "Initial object must not be null.");
+		Assert.notNull(internalId, "The initial objects internal ID must not be null.");
+
 		processedObjects.add(initialObject);
+		processedObjectsIds.put(initialObject, internalId);
 	}
 
 	/**

src/test/java/org/springframework/data/neo4j/integration/issues/gh2347/Application.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright 2011-2021 the original author or authors.
+ *
+ * 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.springframework.data.neo4j.integration.issues.gh2347;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.springframework.data.neo4j.core.schema.Id;
+import org.springframework.data.neo4j.core.schema.Node;
+import org.springframework.data.neo4j.core.schema.Relationship;
+
+/**
+ * @author Michael J. Simons
+ */
+@Node
+public class Application {
+
+	@Id
+	private String id;
+
+	@Relationship(direction = Relationship.Direction.OUTGOING, type = "CONTAINS_WORKFLOW")
+	private List<Workflow> workflows = new ArrayList<>();
+
+	public Application(String id) {
+		this.id = id;
+	}
+
+	public String getId() {
+		return id;
+	}
+
+	public List<Workflow> getWorkflows() {
+		return workflows;
+	}
+}

src/test/java/org/springframework/data/neo4j/integration/issues/gh2347/GH2347IT.java

@@ -0,0 +1,95 @@
+/*
+ * Copyright 2011-2021 the original author or authors.
+ *
+ * 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.springframework.data.neo4j.integration.issues.gh2347;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.jupiter.api.Test;
+import org.neo4j.driver.Driver;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.data.neo4j.config.AbstractNeo4jConfig;
+import org.springframework.data.neo4j.core.DatabaseSelectionProvider;
+import org.springframework.data.neo4j.core.transaction.Neo4jBookmarkManager;
+import org.springframework.data.neo4j.core.transaction.Neo4jTransactionManager;
+import org.springframework.data.neo4j.repository.Neo4jRepository;
+import org.springframework.data.neo4j.repository.config.EnableNeo4jRepositories;
+import org.springframework.data.neo4j.test.BookmarkCapture;
+import org.springframework.data.neo4j.test.Neo4jIntegrationTest;
+import org.springframework.transaction.PlatformTransactionManager;
+import org.springframework.transaction.annotation.EnableTransactionManagement;
+
+/**
+ * @author Michael J. Simons
+ */
+@Neo4jIntegrationTest
+class GH2347IT extends TestBase {
+
+	@Test
+	void entitiesWithAssignedIdsSavedInBatchMustBeIdentifiableWithTheirInternalIds(
+			@Autowired ApplicationRepository applicationRepository,
+			@Autowired Driver driver,
+			@Autowired BookmarkCapture bookmarkCapture
+	) {
+		List<Application> savedApplications = applicationRepository.saveAll(Collections.singletonList(createData()));
+
+		assertThat(savedApplications).hasSize(1);
+		assertDatabase(driver, bookmarkCapture);
+	}
+
+	@Test
+	void entitiesWithAssignedIdsMustBeIdentifiableWithTheirInternalIds(
+			@Autowired ApplicationRepository applicationRepository,
+			@Autowired Driver driver,
+			@Autowired BookmarkCapture bookmarkCapture
+	) {
+		applicationRepository.save(createData());
+		assertDatabase(driver, bookmarkCapture);
+	}
+
+	interface ApplicationRepository extends Neo4jRepository<Application, String> {
+	}
+
+	@Configuration
+	@EnableTransactionManagement
+	@EnableNeo4jRepositories(considerNestedRepositories = true)
+	static class Config extends AbstractNeo4jConfig {
+
+		@Bean
+		public BookmarkCapture bookmarkCapture() {
+			return new BookmarkCapture();
+		}
+
+		@Override
+		public PlatformTransactionManager transactionManager(Driver driver,
+				DatabaseSelectionProvider databaseNameProvider) {
+
+			BookmarkCapture bookmarkCapture = bookmarkCapture();
+			return new Neo4jTransactionManager(driver, databaseNameProvider,
+					Neo4jBookmarkManager.create(bookmarkCapture));
+		}
+
+		@Bean
+		public Driver driver() {
+
+			return neo4jConnectionSupport.getDriver();
+		}
+	}
+}