Polishing.

Replace code duplications with doWithBatch(…) method. Return most concrete type in DefaultDataAccessStrategy and MyBatisDataAccessStrategy.

See #1623
Original pull request: #1897

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

@@ -16,6 +16,7 @@
 package org.springframework.data.jdbc.core;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -23,6 +24,7 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.StreamSupport;
@@ -56,6 +58,7 @@ import org.springframework.data.util.Streamable;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
+import org.springframework.util.ObjectUtils;
 
 /**
  * {@link JdbcAggregateOperations} implementation, storing aggregates in and obtaining them from a JDBC data store.
@@ -173,19 +176,8 @@ public class JdbcAggregateTemplate implements JdbcAggregateOperations {
 
 	@Override
 	public <T> List<T> saveAll(Iterable<T> instances) {
-
-		Assert.notNull(instances, "Aggregate instances must not be null");
-
-		if (!instances.iterator().hasNext()) {
-			return Collections.emptyList();
-		}
-
-		List<EntityAndChangeCreator<T>> entityAndChangeCreators = new ArrayList<>();
-		for (T instance : instances) {
-			verifyIdProperty(instance);
-			entityAndChangeCreators.add(new EntityAndChangeCreator<>(instance, changeCreatorSelectorForSave(instance)));
-		}
-		return performSaveAll(entityAndChangeCreators);
+		return doWithBatch(instances, entity -> changeCreatorSelectorForSave(entity).apply(entity), this::verifyIdProperty,
+				this::performSaveAll);
 	}
 
 	/**
@@ -206,21 +198,7 @@ public class JdbcAggregateTemplate implements JdbcAggregateOperations {
 
 	@Override
 	public <T> List<T> insertAll(Iterable<T> instances) {
-
-		Assert.notNull(instances, "Aggregate instances must not be null");
-
-		if (!instances.iterator().hasNext()) {
-			return Collections.emptyList();
-		}
-
-		List<EntityAndChangeCreator<T>> entityAndChangeCreators = new ArrayList<>();
-		for (T instance : instances) {
-
-			Function<T, RootAggregateChange<T>> changeCreator = entity -> createInsertChange(prepareVersionForInsert(entity));
-			EntityAndChangeCreator<T> entityChange = new EntityAndChangeCreator<>(instance, changeCreator);
-			entityAndChangeCreators.add(entityChange);
-		}
-		return performSaveAll(entityAndChangeCreators);
+		return doWithBatch(instances, entity -> createInsertChange(prepareVersionForInsert(entity)), this::performSaveAll);
 	}
 
 	/**
@@ -241,21 +219,35 @@ public class JdbcAggregateTemplate implements JdbcAggregateOperations {
 
 	@Override
 	public <T> List<T> updateAll(Iterable<T> instances) {
+		return doWithBatch(instances, entity -> createUpdateChange(prepareVersionForUpdate(entity)), this::performSaveAll);
+	}
+
+	private <T> List<T> doWithBatch(Iterable<T> iterable, Function<T, RootAggregateChange<T>> changeCreator,
+			Function<List<EntityAndChangeCreator<T>>, List<T>> performFunction) {
+		return doWithBatch(iterable, changeCreator, entity -> {}, performFunction);
+	}
 
-		Assert.notNull(instances, "Aggregate instances must not be null");
+	private <T> List<T> doWithBatch(Iterable<T> iterable, Function<T, RootAggregateChange<T>> changeCreator,
+			Consumer<T> beforeEntityChange, Function<List<EntityAndChangeCreator<T>>, List<T>> performFunction) {
 
-		if (!instances.iterator().hasNext()) {
+		Assert.notNull(iterable, "Aggregate instances must not be null");
+
+		if (ObjectUtils.isEmpty(iterable)) {
 			return Collections.emptyList();
 		}
 
-		List<EntityAndChangeCreator<T>> entityAndChangeCreators = new ArrayList<>();
-		for (T instance : instances) {
+		List<EntityAndChangeCreator<T>> entityAndChangeCreators = new ArrayList<>(
+				iterable instanceof Collection<?> c ? c.size() : 16);
+
+		for (T instance : iterable) {
+
+			beforeEntityChange.accept(instance);
 
-			Function<T, RootAggregateChange<T>> changeCreator = entity -> createUpdateChange(prepareVersionForUpdate(entity));
 			EntityAndChangeCreator<T> entityChange = new EntityAndChangeCreator<>(instance, changeCreator);
 			entityAndChangeCreators.add(entityChange);
 		}
-		return performSaveAll(entityAndChangeCreators);
+
+		return performFunction.apply(entityAndChangeCreators);
 	}
 
 	@Override

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

@@ -272,12 +272,12 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Class<T> domainType) {
+	public <T> List<T> findAll(Class<T> domainType) {
 		return operations.query(sql(domainType).getFindAll(), getEntityRowMapper(domainType));
 	}
 
 	@Override
-	public <T> Iterable<T> findAllById(Iterable<?> ids, Class<T> domainType) {
+	public <T> List<T> findAllById(Iterable<?> ids, Class<T> domainType) {
 
 		if (!ids.iterator().hasNext()) {
 			return Collections.emptyList();
@@ -290,7 +290,7 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 
 	@Override
 	@SuppressWarnings("unchecked")
-	public Iterable<Object> findAllByPath(Identifier identifier,
+	public List<Object> findAllByPath(Identifier identifier,
 			PersistentPropertyPath<? extends RelationalPersistentProperty> propertyPath) {
 
 		Assert.notNull(identifier, "identifier must not be null");
@@ -338,12 +338,12 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Class<T> domainType, Sort sort) {
+	public <T> List<T> findAll(Class<T> domainType, Sort sort) {
 		return operations.query(sql(domainType).getFindAll(sort), getEntityRowMapper(domainType));
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Class<T> domainType, Pageable pageable) {
+	public <T> List<T> findAll(Class<T> domainType, Pageable pageable) {
 		return operations.query(sql(domainType).getFindAll(pageable), getEntityRowMapper(domainType));
 	}
 
@@ -361,7 +361,7 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Query query, Class<T> domainType) {
+	public <T> List<T> findAll(Query query, Class<T> domainType) {
 
 		MapSqlParameterSource parameterSource = new MapSqlParameterSource();
 		String sqlQuery = sql(domainType).selectByQuery(query, parameterSource);
@@ -370,7 +370,7 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Query query, Class<T> domainType, Pageable pageable) {
+	public <T> List<T> findAll(Query query, Class<T> domainType, Pageable pageable) {
 
 		MapSqlParameterSource parameterSource = new MapSqlParameterSource();
 		String sqlQuery = sql(domainType).selectByQuery(query, parameterSource, pageable);

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/mybatis/MyBatisDataAccessStrategy.java

@@ -256,7 +256,7 @@ public class MyBatisDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Class<T> domainType) {
+	public <T> List<T> findAll(Class<T> domainType) {
 
 		String statement = namespace(domainType) + ".findAll";
 		MyBatisContext parameter = new MyBatisContext(null, null, domainType, Collections.emptyMap());
@@ -264,13 +264,13 @@ public class MyBatisDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	@Override
-	public <T> Iterable<T> findAllById(Iterable<?> ids, Class<T> domainType) {
+	public <T> List<T> findAllById(Iterable<?> ids, Class<T> domainType) {
 		return sqlSession().selectList(namespace(domainType) + ".findAllById",
 				new MyBatisContext(ids, null, domainType, Collections.emptyMap()));
 	}
 
 	@Override
-	public Iterable<Object> findAllByPath(Identifier identifier,
+	public List<Object> findAllByPath(Identifier identifier,
 			PersistentPropertyPath<? extends RelationalPersistentProperty> path) {
 
 		String statementName = namespace(getOwnerTyp(path)) + ".findAllByPath-" + path.toDotPath();
@@ -288,7 +288,7 @@ public class MyBatisDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Class<T> domainType, Sort sort) {
+	public <T> List<T> findAll(Class<T> domainType, Sort sort) {
 
 		Map<String, Object> additionalContext = new HashMap<>();
 		additionalContext.put("sort", sort);
@@ -297,7 +297,7 @@ public class MyBatisDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Class<T> domainType, Pageable pageable) {
+	public <T> List<T> findAll(Class<T> domainType, Pageable pageable) {
 
 		Map<String, Object> additionalContext = new HashMap<>();
 		additionalContext.put("pageable", pageable);
@@ -311,12 +311,12 @@ public class MyBatisDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Query query, Class<T> probeType) {
+	public <T> List<T> findAll(Query query, Class<T> probeType) {
 		throw new UnsupportedOperationException("Not implemented");
 	}
 
 	@Override
-	public <T> Iterable<T> findAll(Query query, Class<T> probeType, Pageable pageable) {
+	public <T> List<T> findAll(Query query, Class<T> probeType, Pageable pageable) {
 		throw new UnsupportedOperationException("Not implemented");
 	}