Move logic for creating SqlParamterSource for insert and update into its own class.

+ To focus DefaultDataAccessStrategy tests on the influence of insert parameters on how it collaborates with JdbcOperations

Author: ctailor2

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

@@ -76,6 +76,7 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 	private final JdbcConverter converter;
 	private final NamedParameterJdbcOperations operations;
 	private final BatchJdbcOperations batchOperations;
+	private final SqlParametersFactory sqlParametersFactory;
 
 	/**
 	 * Creates a {@link DefaultDataAccessStrategy}
@@ -87,24 +88,26 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 	 * @since 1.1
 	 */
 	public DefaultDataAccessStrategy(SqlGeneratorSource sqlGeneratorSource, RelationalMappingContext context,
-			JdbcConverter converter, NamedParameterJdbcOperations operations) {
-		this(sqlGeneratorSource, context, converter, operations, new BatchJdbcOperations(operations.getJdbcOperations()));
+			JdbcConverter converter, NamedParameterJdbcOperations operations, SqlParametersFactory sqlParametersFactory) {
+		this(sqlGeneratorSource, context, converter, operations, new BatchJdbcOperations(operations.getJdbcOperations()), sqlParametersFactory);
 	}
 
 	DefaultDataAccessStrategy(SqlGeneratorSource sqlGeneratorSource, RelationalMappingContext context,
 			JdbcConverter converter, NamedParameterJdbcOperations operations,
-			BatchJdbcOperations batchOperations) {
+			BatchJdbcOperations batchOperations, SqlParametersFactory sqlParametersFactory) {
 
 		Assert.notNull(sqlGeneratorSource, "SqlGeneratorSource must not be null");
 		Assert.notNull(context, "RelationalMappingContext must not be null");
 		Assert.notNull(converter, "JdbcConverter must not be null");
 		Assert.notNull(operations, "NamedParameterJdbcOperations must not be null");
+		Assert.notNull(sqlParametersFactory, "SqlParametersFactory must not be null");
 
 		this.sqlGeneratorSource = sqlGeneratorSource;
 		this.context = context;
 		this.converter = converter;
 		this.operations = operations;
 		this.batchOperations = batchOperations;
+		this.sqlParametersFactory = sqlParametersFactory;
 	}
 
 	/*
@@ -114,24 +117,12 @@ public DefaultDataAccessStrategy(SqlGeneratorSource sqlGeneratorSource, Relation
 	@Override
 	public <T> Object insert(T instance, Class<T> domainType, Identifier identifier, boolean includeId) {
 
-		SqlGenerator sqlGenerator = sql(domainType);
-		RelationalPersistentEntity<T> persistentEntity = getRequiredPersistentEntity(domainType);
-
-		SqlIdentifierParameterSource parameterSource = getParameterSource(instance, persistentEntity, "",
-				PersistentProperty::isIdProperty, getIdentifierProcessing());
-
-		identifier.forEach((name, value, type) -> addConvertedPropertyValue(parameterSource, name, value, type));
-
-		Object idValue = getIdValueOrNull(instance, persistentEntity);
-		if (idValue != null) {
+		SqlIdentifierParameterSource parameterSource = sqlParametersFactory.getInsert(instance, domainType, identifier);
 
-			RelationalPersistentProperty idProperty = persistentEntity.getRequiredIdProperty();
-			addConvertedPropertyValue(parameterSource, idProperty, idValue, idProperty.getColumnName());
-		}
-
-		String insertSql = sqlGenerator.getInsert(parameterSource.getIdentifiers());
+		String insertSql = sql(domainType).getInsert(parameterSource.getIdentifiers());
 
 		if (!includeId) {
+			RelationalPersistentEntity<T> persistentEntity = getRequiredPersistentEntity(domainType);
 			return executeInsertAndReturnGeneratedId(persistentEntity, parameterSource, insertSql);
 		} else {
 
@@ -145,9 +136,8 @@ public <T> Object[] insert(List<RecordDescriptor<T>> recordDescriptors, Class<T>
 //							   TODO: A better name for this may be the inverse (generatedIdExpected)
 							   boolean includeId) {
 
-		RelationalPersistentEntity<T> persistentEntity = getRequiredPersistentEntity(domainType);
 		SqlIdentifierParameterSource[] sqlParameterSources = recordDescriptors.stream()
-				.map(recordDescriptor -> getParameterSource(recordDescriptor, persistentEntity))
+				.map(recordDescriptor -> sqlParametersFactory.getInsert(recordDescriptor.getInstance(), domainType, recordDescriptor.getIdentifier()))
 				.toArray(SqlIdentifierParameterSource[]::new);
 
 		String insertSql = sql(domainType).getInsert(sqlParameterSources[0].getIdentifiers());
@@ -160,6 +150,7 @@ public <T> Object[] insert(List<RecordDescriptor<T>> recordDescriptors, Class<T>
 
 		IdGeneration idGeneration = sqlGeneratorSource.getDialect().getIdGeneration();
 
+		RelationalPersistentEntity<T> persistentEntity = getRequiredPersistentEntity(domainType);
 		if (idGeneration.driverRequiresKeyColumnNames()) {
 
 			String[] keyColumnNames = getKeyColumnNames(persistentEntity.getType());
@@ -171,6 +162,7 @@ public <T> Object[] insert(List<RecordDescriptor<T>> recordDescriptors, Class<T>
 		} else {
 			batchOperations.insert(insertSql, sqlParameterSources, holder);
 		}
+//		TODO: Is this needed?
 		if (!persistentEntity.hasIdProperty()) {
 			return new Object[sqlParameterSources.length];
 		}
@@ -185,29 +177,13 @@ public <T> Object[] insert(List<RecordDescriptor<T>> recordDescriptors, Class<T>
 			} else {
 				id = keys.entrySet().stream().findFirst() //
 						.map(Map.Entry::getValue) //
-//								TODO: Missing a test for this
-						.orElse(null);
+						.orElseThrow(() -> new IllegalStateException("KeyHolder contains an empty key list."));
 			}
 			ids[i] = id;
 		}
 		return ids;
 	}
 
-	private <T> SqlIdentifierParameterSource getParameterSource(RecordDescriptor<T> recordDescriptor, RelationalPersistentEntity<T> persistentEntity) {
-		SqlIdentifierParameterSource parameterSource = getParameterSource(recordDescriptor.getInstance(), persistentEntity, "",
-				PersistentProperty::isIdProperty, getIdentifierProcessing());
-
-		recordDescriptor.getIdentifier().forEach((name, value, type) -> addConvertedPropertyValue(parameterSource, name, value, type));
-
-		Object idValue = getIdValueOrNull(recordDescriptor.getInstance(), persistentEntity);
-		if (idValue != null) {
-
-			RelationalPersistentProperty idProperty = persistentEntity.getRequiredIdProperty();
-			addConvertedPropertyValue(parameterSource, idProperty, idValue, idProperty.getColumnName());
-		}
-		return parameterSource;
-	}
-
 	@Nullable
 	private <T> Object executeInsertAndReturnGeneratedId(RelationalPersistentEntity<T> persistentEntity, SqlIdentifierParameterSource parameterSource, String insertSql) {
 
@@ -236,10 +212,7 @@ private <T> Object executeInsertAndReturnGeneratedId(RelationalPersistentEntity<
 	 */
 	@Override
 	public <S> boolean update(S instance, Class<S> domainType) {
-
-		RelationalPersistentEntity<S> persistentEntity = getRequiredPersistentEntity(domainType);
-		return operations.update(sql(domainType).getUpdate(),
-				getParameterSource(instance, persistentEntity, "", Predicates.includeAll(), getIdentifierProcessing())) != 0;
+		return operations.update(sql(domainType).getUpdate(), sqlParametersFactory.getUpdate(instance, domainType)) != 0;
 	}
 
 	/*
@@ -252,8 +225,7 @@ public <S> boolean updateWithVersion(S instance, Class<S> domainType, Number pre
 		RelationalPersistentEntity<S> persistentEntity = getRequiredPersistentEntity(domainType);
 
 		// Adjust update statement to set the new version and use the old version in where clause.
-		SqlIdentifierParameterSource parameterSource = getParameterSource(instance, persistentEntity, "",
-				Predicates.includeAll(), getIdentifierProcessing());
+		SqlIdentifierParameterSource parameterSource = sqlParametersFactory.getUpdate(instance, domainType);
 		parameterSource.addValue(VERSION_SQL_PARAMETER, previousVersion);
 
 		int affectedRows = operations.update(sql(domainType).getUpdateWithVersion(), parameterSource);
@@ -504,67 +476,6 @@ public <T> Iterable<T> findAll(Class<T> domainType, Pageable pageable) {
 		return operations.query(sql(domainType).getFindAll(pageable), (RowMapper<T>) getEntityRowMapper(domainType));
 	}
 
-	private <S, T> SqlIdentifierParameterSource getParameterSource(@Nullable S instance,
-			RelationalPersistentEntity<S> persistentEntity, String prefix,
-			Predicate<RelationalPersistentProperty> skipProperty, IdentifierProcessing identifierProcessing) {
-
-		SqlIdentifierParameterSource parameters = new SqlIdentifierParameterSource(identifierProcessing);
-
-		PersistentPropertyAccessor<S> propertyAccessor = instance != null ? persistentEntity.getPropertyAccessor(instance)
-				: NoValuePropertyAccessor.instance();
-
-		persistentEntity.doWithAll(property -> {
-
-			if (skipProperty.test(property) || !property.isWritable()) {
-				return;
-			}
-			if (property.isEntity() && !property.isEmbedded()) {
-				return;
-			}
-
-			if (property.isEmbedded()) {
-
-				Object value = propertyAccessor.getProperty(property);
-				RelationalPersistentEntity<?> embeddedEntity = context.getPersistentEntity(property.getType());
-				SqlIdentifierParameterSource additionalParameters = getParameterSource((T) value,
-						(RelationalPersistentEntity<T>) embeddedEntity, prefix + property.getEmbeddedPrefix(), skipProperty,
-						identifierProcessing);
-				parameters.addAll(additionalParameters);
-			} else {
-
-				Object value = propertyAccessor.getProperty(property);
-				SqlIdentifier paramName = property.getColumnName().transform(prefix::concat);
-
-				addConvertedPropertyValue(parameters, property, value, paramName);
-			}
-		});
-
-		return parameters;
-	}
-
-	/**
-	 * Returns the id value if its not a primitive zero. Returns {@literal null} if the id value is null or a primitive
-	 * zero.
-	 */
-	@Nullable
-	@SuppressWarnings("unchecked")
-	private <S, ID> ID getIdValueOrNull(S instance, RelationalPersistentEntity<S> persistentEntity) {
-
-		ID idValue = (ID) persistentEntity.getIdentifierAccessor(instance).getIdentifier();
-
-		return isIdPropertyNullOrScalarZero(idValue, persistentEntity) ? null : idValue;
-	}
-
-	private static <S, ID> boolean isIdPropertyNullOrScalarZero(@Nullable ID idValue,
-			RelationalPersistentEntity<S> persistentEntity) {
-
-		RelationalPersistentProperty idProperty = persistentEntity.getIdProperty();
-		return idValue == null //
-				|| idProperty == null //
-				|| (idProperty.getType() == int.class && idValue.equals(0)) //
-				|| (idProperty.getType() == long.class && idValue.equals(0L));
-	}
-
 	@Nullable
 	private <S> Object getIdFromHolder(KeyHolder holder, RelationalPersistentEntity<S> persistentEntity) {
 
@@ -601,6 +512,7 @@ private RowMapper<?> getMapEntityRowMapper(PersistentPropertyPathExtension path,
 		return new MapEntityRowMapper<>(path, converter, identifier, keyColumn, getIdentifierProcessing());
 	}
 
+//	TODO: Move to SqlParametersFactory
 	private <T> SqlIdentifierParameterSource createIdParameterSource(Object id, Class<T> domainType) {
 
 		SqlIdentifierParameterSource parameterSource = new SqlIdentifierParameterSource(getIdentifierProcessing());
@@ -689,47 +601,4 @@ private <T> String[] getKeyColumnNames(Class<T> domainType) {
 		return new String[] { idColumn.getReference(getIdentifierProcessing()) };
 	}
 
-	/**
-	 * Utility to create {@link Predicate}s.
-	 */
-	static class Predicates {
-
-		/**
-		 * Include all {@link Predicate} returning {@literal false} to never skip a property.
-		 *
-		 * @return the include all {@link Predicate}.
-		 */
-		static Predicate<RelationalPersistentProperty> includeAll() {
-			return it -> false;
-		}
-	}
-
-	/**
-	 * A {@link PersistentPropertyAccessor} implementation always returning null
-	 *
-	 * @param <T>
-	 */
-	static class NoValuePropertyAccessor<T> implements PersistentPropertyAccessor<T> {
-
-		private static final NoValuePropertyAccessor INSTANCE = new NoValuePropertyAccessor();
-
-		static <T> NoValuePropertyAccessor<T> instance() {
-			return INSTANCE;
-		}
-
-		@Override
-		public void setProperty(PersistentProperty<?> property, @Nullable Object value) {
-			throw new UnsupportedOperationException("Cannot set value on 'null' target object.");
-		}
-
-		@Override
-		public Object getProperty(PersistentProperty<?> property) {
-			return null;
-		}
-
-		@Override
-		public T getBean() {
-			return null;
-		}
-	}
 }