DATAJDBC-123 - Polishing.

Incorporating review comments.

Code formatting.
Improved comments.
Better method name.
Converted initialiser to before method.

src/main/java/org/springframework/data/jdbc/core/DataAccessStrategy.java

@@ -33,7 +33,8 @@ public interface DataAccessStrategy {
 
 	void delete(Object id, Class<?> domainType);
 
-	/** Deletes all entities reachable via {@literal propertyPath} from the instance identified by {@literal rootId}.
+	/**
+	 * Deletes all entities reachable via {@literal propertyPath} from the instance identified by {@literal rootId}.
 	 *
 	 * @param rootId Id of the root object on which the {@literal propertyPath} is based.
 	 * @param propertyPath Leading from the root object to the entities to be deleted.
@@ -42,7 +43,8 @@ public interface DataAccessStrategy {
 
 	<T> void deleteAll(Class<T> domainType);
 
-	/** Deletes all entities reachable via {@literal propertyPath} from any instance.
+	/**
+	 * Deletes all entities reachable via {@literal propertyPath} from any instance.
 	 *
 	 * @param propertyPath Leading from the root object to the entities to be deleted.
 	 */

src/main/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategy.java

@@ -44,7 +44,7 @@ import org.springframework.jdbc.support.KeyHolder;
 import org.springframework.util.Assert;
 
 /**
- * Generates and executes actual SQL statements.
+ * The default {@link DataAccessStrategy} is to generate SQL statements based on meta data from the entity.
  *
  * @author Jens Schauder
  */
@@ -70,7 +70,7 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 	}
 
 	/**
-	 * creates a {@link DefaultDataAccessStrategy} which references it self for resolution of recursive data accesses.
+	 * Creates a {@link DefaultDataAccessStrategy} which references it self for resolution of recursive data accesses.
 	 *
 	 * Only suitable if this is the only access strategy in use.
 	 */
@@ -141,18 +141,15 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 		HashMap<String, Object> parameters = new HashMap<>();
 		parameters.put("rootId", rootId);
 		operations.update(format, parameters);
-
 	}
 
 	@Override
 	public <T> void deleteAll(Class<T> domainType) {
-
 		operations.getJdbcOperations().update(sql(domainType).createDeleteAllSql(null));
 	}
 
 	@Override
 	public <T> void deleteAll(PropertyPath propertyPath) {
-
 		operations.getJdbcOperations().update(sql(propertyPath.getOwningType().getType()).createDeleteAllSql(propertyPath));
 	}
 
@@ -244,10 +241,10 @@ public class DefaultDataAccessStrategy implements DataAccessStrategy {
 
 		ID idValue = entityInformation.getId(instance);
 
-		return isIdPropertySimpleTypeAndValueZero(idValue, persistentEntity) ? null : idValue;
+		return isIdPropertyNullOrScalarZero(idValue, persistentEntity) ? null : idValue;
 	}
 
-	private <S, ID> boolean isIdPropertySimpleTypeAndValueZero(ID idValue, JdbcPersistentEntity<S> persistentEntity) {
+	private <S, ID> boolean isIdPropertyNullOrScalarZero(ID idValue, JdbcPersistentEntity<S> persistentEntity) {
 
 		JdbcPersistentProperty idProperty = persistentEntity.getIdProperty();
 		return idValue == null //

src/main/java/org/springframework/data/jdbc/repository/support/JdbcRepositoryFactoryBean.java

@@ -82,6 +82,24 @@ public class JdbcRepositoryFactoryBean<T extends Repository<S, ID>, S, ID extend
 
 		final JdbcMappingContext context = new JdbcMappingContext(findOrCreateNamingStrategy());
 
+		return new JdbcRepositoryFactory(applicationEventPublisher, context, createDataAccessStrategy(context));
+	}
+
+	/**
+	 * <p>
+	 * Create the {@link DataAccessStrategy}, by combining all applicable strategies into one.
+	 * </p>
+	 * <p>
+	 * The challenge is that the {@link DefaultDataAccessStrategy} when used for reading needs a
+	 * {@link DataAccessStrategy} for loading referenced entities (see.
+	 * {@link DefaultDataAccessStrategy#getEntityRowMapper(Class)}. But it should use all configured
+	 * {@link DataAccessStrategy}s for this. This creates a cyclic dependency. In order to build this the
+	 * {@link DefaultDataAccessStrategy} gets passed in a {@link DelegatingDataAccessStrategy} which at the end gets set
+	 * to the full {@link CascadingDataAccessStrategy}.
+	 * </p>
+	 */
+	private CascadingDataAccessStrategy createDataAccessStrategy(JdbcMappingContext context) {
+
 		DelegatingDataAccessStrategy delegatingDataAccessStrategy = new DelegatingDataAccessStrategy();
 
 		List<DataAccessStrategy> accessStrategies = Stream.of( //
@@ -95,7 +113,7 @@ public class JdbcRepositoryFactoryBean<T extends Repository<S, ID>, S, ID extend
 		CascadingDataAccessStrategy strategy = new CascadingDataAccessStrategy(accessStrategies);
 		delegatingDataAccessStrategy.setDelegate(strategy);
 
-		return new JdbcRepositoryFactory(applicationEventPublisher, context, strategy);
+		return strategy;
 	}
 
 	private Optional<DataAccessStrategy> createMyBatisDataAccessStrategy() {

src/test/java/org/springframework/data/jdbc/core/MyBatisDataAccessStrategyUnitTests.java

@@ -23,6 +23,7 @@ import java.util.Collections;
 
 import org.apache.ibatis.session.SqlSession;
 import org.apache.ibatis.session.SqlSessionFactory;
+import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mockito;
@@ -44,7 +45,8 @@ public class MyBatisDataAccessStrategyUnitTests {
 
 	MyBatisDataAccessStrategy accessStrategy = new MyBatisDataAccessStrategy(sessionFactory);
 
-	{
+	@Before
+	public void before() {
 
 		doReturn(session).when(sessionFactory).openSession();
 		doReturn(false).when(session).selectOne(any(), any());

src/test/java/org/springframework/data/jdbc/mybatis/MyBatisHsqlIntegrationTests.java

@@ -50,7 +50,6 @@ public class MyBatisHsqlIntegrationTests {
 
 	@org.springframework.context.annotation.Configuration
 	@Import(TestConfiguration.class)
-
 	@EnableJdbcRepositories(considerNestedRepositories = true)
 	static class Config {