Applied changes requested in review.

The major ones are:

* directly construct joins
* remove multiple places of duplication
* naming
* documentation

Original pull request #1957
See #574

Author: schauder

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateChangeExecutionContext.java

@@ -17,6 +17,7 @@
 
 import java.util.*;
 import java.util.function.BiConsumer;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 
 import org.springframework.dao.IncorrectUpdateSemanticsDataAccessException;
@@ -72,19 +73,16 @@ class JdbcAggregateChangeExecutionContext {
 
 	<T> void executeInsertRoot(DbAction.InsertRoot<T> insert) {
 
-		Object id = accessStrategy.insert(insert.getEntity(), insert.getEntityType(), Identifier.empty(),
-				insert.getIdValueSource());
+		Object id = accessStrategy.insert(insert.getEntity(), insert.getEntityType(), Identifier.empty(), insert.getIdValueSource());
 		add(new DbActionExecutionResult(insert, id));
 	}
 
 	<T> void executeBatchInsertRoot(DbAction.BatchInsertRoot<T> batchInsertRoot) {
 
 		List<DbAction.InsertRoot<T>> inserts = batchInsertRoot.getActions();
-		List<InsertSubject<T>> insertSubjects = inserts.stream()
-				.map(insert -> InsertSubject.describedBy(insert.getEntity(), Identifier.empty())).collect(Collectors.toList());
+		List<InsertSubject<T>> insertSubjects = inserts.stream().map(insert -> InsertSubject.describedBy(insert.getEntity(), Identifier.empty())).collect(Collectors.toList());
 
-		Object[] ids = accessStrategy.insert(insertSubjects, batchInsertRoot.getEntityType(),
-				batchInsertRoot.getBatchValue());
+		Object[] ids = accessStrategy.insert(insertSubjects, batchInsertRoot.getEntityType(), batchInsertRoot.getBatchValue());
 
 		for (int i = 0; i < inserts.size(); i++) {
 			add(new DbActionExecutionResult(inserts.get(i), ids.length > 0 ? ids[i] : null));
@@ -94,17 +92,14 @@ <T> void executeBatchInsertRoot(DbAction.BatchInsertRoot<T> batchInsertRoot) {
 	<T> void executeInsert(DbAction.Insert<T> insert) {
 
 		Identifier parentKeys = getParentKeys(insert, converter);
-		Object id = accessStrategy.insert(insert.getEntity(), insert.getEntityType(), parentKeys,
-				insert.getIdValueSource());
+		Object id = accessStrategy.insert(insert.getEntity(), insert.getEntityType(), parentKeys, insert.getIdValueSource());
 		add(new DbActionExecutionResult(insert, id));
 	}
 
 	<T> void executeBatchInsert(DbAction.BatchInsert<T> batchInsert) {
 
 		List<DbAction.Insert<T>> inserts = batchInsert.getActions();
-		List<InsertSubject<T>> insertSubjects = inserts.stream()
-				.map(insert -> InsertSubject.describedBy(insert.getEntity(), getParentKeys(insert, converter)))
-				.collect(Collectors.toList());
+		List<InsertSubject<T>> insertSubjects = inserts.stream().map(insert -> InsertSubject.describedBy(insert.getEntity(), getParentKeys(insert, converter))).collect(Collectors.toList());
 
 		Object[] ids = accessStrategy.insert(insertSubjects, batchInsert.getEntityType(), batchInsert.getBatchValue());
 
@@ -176,20 +171,34 @@ private Identifier getParentKeys(DbAction.WithDependingOn<?> action, JdbcConvert
 		Object id = getParentId(action);
 
 		JdbcIdentifierBuilder identifier = JdbcIdentifierBuilder //
-				.forBackReferences(converter, context.getAggregatePath(action.getPropertyPath()), id);
+				.forBackReferences(converter, context.getAggregatePath(action.getPropertyPath()),
+						getValueProvider(id, context.getAggregatePath(action.getPropertyPath()), converter));
 
-		for (Map.Entry<PersistentPropertyPath<RelationalPersistentProperty>, Object> qualifier : action.getQualifiers()
-				.entrySet()) {
+		for (Map.Entry<PersistentPropertyPath<RelationalPersistentProperty>, Object> qualifier : action.getQualifiers().entrySet()) {
 			identifier = identifier.withQualifier(context.getAggregatePath(qualifier.getKey()), qualifier.getValue());
 		}
 
 		return identifier.build();
 	}
 
+	static Function<AggregatePath, Object> getValueProvider(Object idValue, AggregatePath path, JdbcConverter converter) {
+
+		RelationalPersistentEntity<?> entity = converter.getMappingContext().getPersistentEntity(path.getIdDefiningParentPath().getRequiredIdProperty().getType());
+
+		Function<AggregatePath, Object> valueProvider = ap -> {
+			if (entity == null) {
+				return idValue;
+			} else {
+				PersistentPropertyPathAccessor<Object> propertyPathAccessor = entity.getPropertyPathAccessor(idValue);
+				return propertyPathAccessor.getProperty(ap.getRequiredPersistentPropertyPath());
+			}
+		};
+		return valueProvider;
+	}
+
 	private Object getParentId(DbAction.WithDependingOn<?> action) {
 
-		DbAction.WithEntity<?> idOwningAction = getIdOwningAction(action,
-				context.getAggregatePath(action.getPropertyPath()).getIdDefiningParentPath());
+		DbAction.WithEntity<?> idOwningAction = getIdOwningAction(action, context.getAggregatePath(action.getPropertyPath()).getIdDefiningParentPath());
 
 		return getPotentialGeneratedIdFrom(idOwningAction);
 	}
@@ -198,8 +207,7 @@ private DbAction.WithEntity<?> getIdOwningAction(DbAction.WithEntity<?> action,
 
 		if (!(action instanceof DbAction.WithDependingOn<?> withDependingOn)) {
 
-			Assert.state(idPath.isRoot(),
-					"When the id path is not empty the id providing action should be of type WithDependingOn");
+			Assert.state(idPath.isRoot(), "When the id path is not empty the id providing action should be of type WithDependingOn");
 
 			return action;
 		}
@@ -267,20 +275,16 @@ <T> List<T> populateIdsIfNecessary() {
 
 				if (newEntity != action.getEntity()) {
 
-					cascadingValues.stage(insert.getDependingOn(), insert.getPropertyPath(),
-							qualifierValue, newEntity);
+					cascadingValues.stage(insert.getDependingOn(), insert.getPropertyPath(), qualifierValue, newEntity);
 				} else if (insert.getPropertyPath().getLeafProperty().isCollectionLike()) {
 
-					cascadingValues.gather(insert.getDependingOn(), insert.getPropertyPath(),
-							qualifierValue, newEntity);
+					cascadingValues.gather(insert.getDependingOn(), insert.getPropertyPath(), qualifierValue, newEntity);
 				}
 			}
 		}
 
 		if (roots.isEmpty()) {
-			throw new IllegalStateException(
-					String.format("Cannot retrieve the resulting instance(s) unless a %s or %s action was successfully executed",
-							DbAction.InsertRoot.class.getName(), DbAction.UpdateRoot.class.getName()));
+			throw new IllegalStateException(String.format("Cannot retrieve the resulting instance(s) unless a %s or %s action was successfully executed", DbAction.InsertRoot.class.getName(), DbAction.UpdateRoot.class.getName()));
 		}
 
 		Collections.reverse(roots);
@@ -289,23 +293,19 @@ <T> List<T> populateIdsIfNecessary() {
 	}
 
 	@SuppressWarnings("unchecked")
-	private <S> Object setIdAndCascadingProperties(DbAction.WithEntity<S> action, @Nullable Object generatedId,
-			StagedValues cascadingValues) {
+	private <S> Object setIdAndCascadingProperties(DbAction.WithEntity<S> action, @Nullable Object generatedId, StagedValues cascadingValues) {
 
 		S originalEntity = action.getEntity();
 
-		RelationalPersistentEntity<S> persistentEntity = (RelationalPersistentEntity<S>) context
-				.getRequiredPersistentEntity(action.getEntityType());
-		PersistentPropertyPathAccessor<S> propertyAccessor = converter.getPropertyAccessor(persistentEntity,
-				originalEntity);
+		RelationalPersistentEntity<S> persistentEntity = (RelationalPersistentEntity<S>) context.getRequiredPersistentEntity(action.getEntityType());
+		PersistentPropertyPathAccessor<S> propertyAccessor = converter.getPropertyAccessor(persistentEntity, originalEntity);
 
 		if (IdValueSource.GENERATED.equals(action.getIdValueSource())) {
 			propertyAccessor.setProperty(persistentEntity.getRequiredIdProperty(), generatedId);
 		}
 
 		// set values of changed immutables referenced by this entity
-		cascadingValues.forEachPath(action, (persistentPropertyPath, o) -> propertyAccessor
-				.setProperty(getRelativePath(action, persistentPropertyPath), o));
+		cascadingValues.forEachPath(action, (persistentPropertyPath, o) -> propertyAccessor.setProperty(getRelativePath(action, persistentPropertyPath), o));
 
 		return propertyAccessor.getBean();
 	}
@@ -337,8 +337,7 @@ private <T> void updateWithoutVersion(DbAction.UpdateRoot<T> update) {
 
 		if (!accessStrategy.update(update.getEntity(), update.getEntityType())) {
 
-			throw new IncorrectUpdateSemanticsDataAccessException(
-					String.format(UPDATE_FAILED, update.getEntity(), getIdFrom(update)));
+			throw new IncorrectUpdateSemanticsDataAccessException(String.format(UPDATE_FAILED, update.getEntity(), getIdFrom(update)));
 		}
 	}
 
@@ -359,21 +358,20 @@ private <T> void updateWithVersion(DbAction.UpdateRoot<T> update) {
 	 */
 	private static class StagedValues {
 
-		static final List<MultiValueAggregator<?>> aggregators = Arrays.asList(SetAggregator.INSTANCE, MapAggregator.INSTANCE,
-				ListAggregator.INSTANCE, SingleElementAggregator.INSTANCE);
+		static final List<MultiValueAggregator<?>> aggregators = Arrays.asList(SetAggregator.INSTANCE, MapAggregator.INSTANCE, ListAggregator.INSTANCE, SingleElementAggregator.INSTANCE);
 
 		Map<DbAction, Map<PersistentPropertyPath, StagedValue>> values = new HashMap<>();
 
 		/**
 		 * Adds a value that needs to be set in an entity higher up in the tree of entities in the aggregate. If the
 		 * attribute to be set is multivalued this method expects only a single element.
 		 *
-		 * @param action The action responsible for persisting the entity that needs the added value set. Must not be
-		 *          {@literal null}.
-		 * @param path The path to the property in which to set the value. Must not be {@literal null}.
+		 * @param action    The action responsible for persisting the entity that needs the added value set. Must not be
+		 *                  {@literal null}.
+		 * @param path      The path to the property in which to set the value. Must not be {@literal null}.
 		 * @param qualifier If {@code path} is a qualified multivalued properties this parameter contains the qualifier. May
-		 *          be {@literal null}.
-		 * @param value The value to be set. Must not be {@literal null}.
+		 *                  be {@literal null}.
+		 * @param value     The value to be set. Must not be {@literal null}.
 		 */
 		void stage(DbAction<?> action, PersistentPropertyPath path, @Nullable Object qualifier, Object value) {
 
@@ -386,11 +384,9 @@ <T> StagedValue gather(DbAction<?> action, PersistentPropertyPath path, @Nullabl
 
 			MultiValueAggregator<T> aggregator = getAggregatorFor(path);
 
-			Map<PersistentPropertyPath, StagedValue> valuesForPath = this.values.computeIfAbsent(action,
-					dbAction -> new HashMap<>());
+			Map<PersistentPropertyPath, StagedValue> valuesForPath = this.values.computeIfAbsent(action, dbAction -> new HashMap<>());
 
-			StagedValue stagedValue = valuesForPath.computeIfAbsent(path,
-					persistentPropertyPath -> new StagedValue(aggregator.createEmptyInstance()));
+			StagedValue stagedValue = valuesForPath.computeIfAbsent(path, persistentPropertyPath -> new StagedValue(aggregator.createEmptyInstance()));
 			T currentValue = (T) stagedValue.value;
 
 			stagedValue.value = aggregator.add(currentValue, qualifier, value);
@@ -430,7 +426,8 @@ void forEachPath(DbAction<?> dbAction, BiConsumer<PersistentPropertyPath, Object
 	}
 
 	private static class StagedValue {
-		@Nullable Object value;
+		@Nullable
+		Object value;
 		boolean isStaged;
 
 		public StagedValue(@Nullable Object value) {

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/JdbcIdentifierBuilder.java

@@ -17,10 +17,7 @@
 
 import java.util.function.Function;
 
-import org.springframework.data.mapping.PersistentPropertyPathAccessor;
 import org.springframework.data.relational.core.mapping.AggregatePath;
-import org.springframework.data.relational.core.mapping.RelationalMappingContext;
-import org.springframework.data.relational.core.mapping.RelationalPersistentEntity;
 import org.springframework.data.relational.core.mapping.RelationalPersistentProperty;
 import org.springframework.util.Assert;
 
@@ -45,37 +42,46 @@ public static JdbcIdentifierBuilder empty() {
 	/**
 	 * Creates ParentKeys with backreference for the given path and value of the parents id.
 	 */
-	public static JdbcIdentifierBuilder forBackReferences(JdbcConverter converter, AggregatePath path, Object value) {
+	// gets called during insert. value contains the id from an insert of the parent
+	public static JdbcIdentifierBuilder forBackReferences(JdbcConverter converter, AggregatePath path, Function<AggregatePath, Object> valueProvider) {
 
-		RelationalPersistentProperty idProperty = path.getIdDefiningParentPath().getRequiredIdProperty();
-		AggregatePath.ColumnInfos infos = path.getTableInfo().reverseColumnInfos();
+		return new JdbcIdentifierBuilder(forBackReference(converter, path, Identifier.empty(), valueProvider));
+	}
 
-		// create property accessor
-		RelationalMappingContext mappingContext = converter.getMappingContext();
-		RelationalPersistentEntity<?> persistentEntity = mappingContext.getPersistentEntity(idProperty.getType());
+	/**
+	 * @param converter         used for determining the column types to be used for different properties. Must not be {@literal null}.
+	 * @param path              the path for which needs to back reference an id. Must not be {@literal null}.
+	 * @param defaultIdentifier Identifier to be used as a default when no backreference can be constructed. Must not be {@literal null}.
+	 * @param valueProvider     provides values for the {@link Identifier} based on an {@link AggregatePath}. Must not be {@literal null}.
+	 * @return Guaranteed not to be {@literal null}.
+	 */
+	public static Identifier forBackReference(JdbcConverter converter, AggregatePath path, Identifier defaultIdentifier, Function<AggregatePath, Object> valueProvider) {
 
-		Function<AggregatePath, Object> valueProvider;
-		if (persistentEntity == null) {
-			valueProvider = ap -> value;
-		} else {
-			PersistentPropertyPathAccessor<Object> propertyPathAccessor = persistentEntity.getPropertyPathAccessor(value);
-			valueProvider = ap -> propertyPathAccessor.getProperty(ap.getRequiredPersistentPropertyPath());
-		}
+		Identifier identifierToUse = defaultIdentifier;
 
-		Identifier identifierHolder = infos.reduce(Identifier.empty(), (ap, ci) -> {
+		AggregatePath idDefiningParentPath = path.getIdDefiningParentPath();
 
-			RelationalPersistentProperty property = ap.getRequiredLeafProperty();
-			return Identifier.of(ci.name(), valueProvider.apply(ap),
-					converter.getColumnType(property));
-		}, Identifier::withPart);
+		// note that the idDefiningParentPath might not itself have an id property, but have a combination of back
+		// references and possibly keys, that form an id
+		if (idDefiningParentPath.hasIdProperty()) {
 
-		return new JdbcIdentifierBuilder(identifierHolder);
+			AggregatePath.ColumnInfos infos = path.getTableInfo().backReferenceColumnInfos();
+			identifierToUse = infos.reduce(Identifier.empty(), (ap, ci) -> {
+
+				RelationalPersistentProperty property = ap.getRequiredLeafProperty();
+				return Identifier.of(ci.name(), valueProvider.apply(ap),
+						converter.getColumnType(property));
+			}, Identifier::withPart);
+		}
+
+		return identifierToUse;
 	}
 
+
 	/**
 	 * Adds a qualifier to the identifier to build. A qualifier is a map key or a list index.
 	 *
-	 * @param path path to the map that gets qualified by {@code value}. Must not be {@literal null}.
+	 * @param path  path to the map that gets qualified by {@code value}. Must not be {@literal null}.
 	 * @param value map key or list index qualifying the map identified by {@code path}. Must not be {@literal null}.
 	 * @return this builder. Guaranteed to be not {@literal null}.
 	 */