Applied changes requested in review.

The major ones are:

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

See #574
Original pull request #1957

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;
@@ -176,7 +177,8 @@ 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()) {
@@ -186,6 +188,22 @@ private Identifier getParentKeys(DbAction.WithDependingOn<?> action, JdbcConvert
 		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,
@@ -267,12 +285,10 @@ <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);
 				}
 			}
 		}
@@ -359,8 +375,8 @@ 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<>();
 

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

@@ -106,7 +106,7 @@ public static Identifier from(Map<SqlIdentifier, Object> map) {
 	 * @param identifier the identifier to append.
 	 * @return the {@link Identifier} containing all existing keys and the key part for {@code name}, {@code value}, and a
 	 *         {@link Class target type}.
-	 * @since 3.5
+	 * @since 4.0
 	 */
 	public Identifier withPart(Identifier identifier) {
 
@@ -207,7 +207,6 @@ public Object get(SqlIdentifier columnName) {
 		return null;
 	}
 
-
 	/**
 	 * A single value of an Identifier consisting of the column name, the value and the target type which is to be used to
 	 * store the element in the database.

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,31 +42,42 @@ 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) {
+	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()) {
+
+			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 new JdbcIdentifierBuilder(identifierHolder);
+		return identifierToUse;
 	}
 
 	/**
@@ -85,8 +93,7 @@ public JdbcIdentifierBuilder withQualifier(AggregatePath path, Object value) {
 		Assert.notNull(value, "Value must not be null");
 
 		AggregatePath.TableInfo tableInfo = path.getTableInfo();
-		identifier = identifier.withPart(tableInfo.qualifierColumnInfo().name(), value,
-				tableInfo.qualifierColumnType());
+		identifier = identifier.withPart(tableInfo.qualifierColumnInfo().name(), value, tableInfo.qualifierColumnType());
 
 		return this;
 	}

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

@@ -361,7 +361,8 @@ public <T> T getPropertyValue(RelationalPersistentProperty property) {
 
 				if (property.isCollectionLike() || property.isMap()) {
 
-					Identifier identifier = constructIdentifier(aggregatePath);
+					Identifier identifier = JdbcIdentifierBuilder.forBackReference(MappingJdbcConverter.this, aggregatePath,
+							this.identifier, getWrappedValueProvider(delegate::getValue, aggregatePath));
 
 					Iterable<Object> allByPath = relationResolver.findAllByPath(identifier,
 							aggregatePath.getRequiredPersistentPropertyPath());
@@ -388,29 +389,6 @@ public <T> T getPropertyValue(RelationalPersistentProperty property) {
 			return (T) delegate.getValue(aggregatePath);
 		}
 
-		private Identifier constructIdentifier(AggregatePath aggregatePath) {
-
-			Identifier identifierToUse = this.identifier;
-			AggregatePath idDefiningParentPath = aggregatePath.getIdDefiningParentPath();
-
-			// 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()) {
-
-				RelationalPersistentProperty idProperty = idDefiningParentPath.getRequiredIdProperty();
-				AggregatePath idPath = idProperty.isEntity() ? idDefiningParentPath.append(idProperty) : idDefiningParentPath;
-				Identifier[] buildingIdentifier = new Identifier[] { Identifier.empty() };
-				aggregatePath.getTableInfo().reverseColumnInfos().forEach((ap, ci) -> {
-
-					Object value = delegate.getValue(idPath.append(ap));
-					buildingIdentifier[0] = buildingIdentifier[0].withPart(ci.name(), value,
-							ap.getRequiredLeafProperty().getActualType());
-				});
-				identifierToUse = buildingIdentifier[0];
-			}
-			return identifierToUse;
-		}
-
 		@Override
 		public boolean hasValue(RelationalPersistentProperty property) {
 
@@ -431,7 +409,7 @@ public boolean hasValue(RelationalPersistentProperty property) {
 					return delegate.hasValue(toUse);
 				}
 
-				return delegate.hasValue(aggregatePath.getTableInfo().reverseColumnInfos().any().alias());
+				return delegate.hasValue(aggregatePath.getTableInfo().backReferenceColumnInfos().any().alias());
 			}
 
 			return delegate.hasValue(aggregatePath);
@@ -457,7 +435,7 @@ public boolean hasNonEmptyValue(RelationalPersistentProperty property) {
 					return delegate.hasValue(toUse);
 				}
 
-				return delegate.hasValue(aggregatePath.getTableInfo().reverseColumnInfos().any().alias());
+				return delegate.hasValue(aggregatePath.getTableInfo().backReferenceColumnInfos().any().alias());
 			}
 
 			return delegate.hasNonEmptyValue(aggregatePath);
@@ -472,6 +450,22 @@ public RelationalPropertyValueProvider withContext(ConversionContext context) {
 		}
 	}
 
+	private static Function<AggregatePath, Object> getWrappedValueProvider(Function<AggregatePath, Object> valueProvider,
+			AggregatePath aggregatePath) {
+
+		AggregatePath idDefiningParentPath = aggregatePath.getIdDefiningParentPath();
+
+		if (!idDefiningParentPath.hasIdProperty()) {
+			return ap -> {
+				throw new IllegalStateException("This should never happen");
+			};
+		}
+
+		RelationalPersistentProperty idProperty = idDefiningParentPath.getRequiredIdProperty();
+		AggregatePath idPath = idProperty.isEntity() ? idDefiningParentPath.append(idProperty) : idDefiningParentPath;
+		return ap -> valueProvider.apply(idPath.append(ap));
+	}
+
 	/**
 	 * Marker object to indicate that the property value provider should resolve relations.
 	 *

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

@@ -66,10 +66,11 @@ Column getColumn(AggregatePath path) {
 	 * 
 	 * @param path must not be null.
 	 * @return a {@literal Column} that is part of the effective primary key for the given path.
+	 * @since 4.0
 	 */
 	Column getAnyReverseColumn(AggregatePath path) {
 
-		AggregatePath.ColumnInfo columnInfo = path.getTableInfo().reverseColumnInfos().any();
+		AggregatePath.ColumnInfo columnInfo = path.getTableInfo().backReferenceColumnInfos().any();
 		return getTable(path).column(columnInfo.name()).as(columnInfo.alias());
 	}
 }