DATAJDBC-330 - Move default bean lookup from JdbcRepositoryConfigExtension to JdbcRepositoryFactoryBean.

So far the lookup of `NamedParameterJdbcOperations` and `DataAccessStrategy` could happend before these beans were registered resulting in failure to consider those beans.

The logic is now split in two parts:
If a bean name is given, this is configured on the BeanDefinition level in the JdbcRepositoryConfigExtension.
If no name is give a bean is looked up by type in the JdbcRepositoryFactoryBean.

This makes the code even simpler and uses standard Spring features instead of reimplementing them.

Original pull request: #115.

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/config/JdbcRepositoryConfigExtension.java

@@ -15,27 +15,14 @@
  */
 package org.springframework.data.jdbc.repository.config;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
 import java.util.Locale;
-import java.util.Optional;
-import java.util.function.Supplier;
 
 import org.springframework.beans.factory.ListableBeanFactory;
-import org.springframework.beans.factory.NoSuchBeanDefinitionException;
-import org.springframework.beans.factory.NoUniqueBeanDefinitionException;
-import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
 import org.springframework.beans.factory.support.BeanDefinitionBuilder;
 import org.springframework.beans.factory.support.BeanDefinitionRegistry;
-import org.springframework.data.jdbc.core.DataAccessStrategy;
 import org.springframework.data.jdbc.repository.support.JdbcRepositoryFactoryBean;
 import org.springframework.data.repository.config.RepositoryConfigurationExtensionSupport;
 import org.springframework.data.repository.config.RepositoryConfigurationSource;
-import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations;
-import org.springframework.lang.Nullable;
-import org.springframework.util.Assert;
-import org.springframework.util.ObjectUtils;
 import org.springframework.util.StringUtils;
 
 /**
@@ -87,103 +74,20 @@ public class JdbcRepositoryConfigExtension extends RepositoryConfigurationExtens
 		}
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.repository.config.RepositoryConfigurationExtensionSupport#postProcess(org.springframework.beans.factory.support.BeanDefinitionBuilder, org.springframework.data.repository.config.RepositoryConfigurationSource)
 	 */
 	@Override
 	public void postProcess(BeanDefinitionBuilder builder, RepositoryConfigurationSource source) {
 
-		resolveReference(builder, source, "jdbcOperationsRef", "jdbcOperations", NamedParameterJdbcOperations.class, true);
-		resolveReference(builder, source, "dataAccessStrategyRef", "dataAccessStrategy", DataAccessStrategy.class, false);
-	}
+		source.getAttribute("jdbcOperationsRef") //
+				.filter(s -> !StringUtils.isEmpty(s)) //
+				.ifPresent(s -> builder.addPropertyReference("jdbcOperations", s));
 
-	private void resolveReference(BeanDefinitionBuilder builder, RepositoryConfigurationSource source,
-			String attributeName, String propertyName, Class<?> classRef, boolean required) {
-
-		Optional<String> beanNameRef = source.getAttribute(attributeName).filter(StringUtils::hasText);
-
-		String beanName = beanNameRef.orElseGet(() -> determineMatchingBeanName(propertyName, classRef, required));
-
-		if (beanName != null) {
-			builder.addPropertyReference(propertyName, beanName);
-		} else {
-			Assert.isTrue(!required,
-					"The beanName must not be null when requested as 'required'. Please report this as a bug.");
-		}
-
-	}
-
-	@Nullable
-	private String determineMatchingBeanName(String propertyName, Class<?> classRef, boolean required) {
-
-		if (this.beanFactory == null) {
-			return nullOrThrowException(required,
-					() -> new NoSuchBeanDefinitionException(classRef, "No BeanFactory available."));
-		}
-
-		List<String> beanNames = Arrays.asList(beanFactory.getBeanNamesForType(classRef));
-
-		if (beanNames.isEmpty()) {
-			return nullOrThrowException(required,
-					() -> new NoSuchBeanDefinitionException(classRef, String.format("No bean of type %s available", classRef)));
-		}
-
-		if (beanNames.size() == 1) {
-			return beanNames.get(0);
-		}
-
-		if (!(beanFactory instanceof ConfigurableListableBeanFactory)) {
-
-			return nullOrThrowException(required,
-					() -> new NoSuchBeanDefinitionException(String.format(
-							"BeanFactory does not implement ConfigurableListableBeanFactory when trying to find bean of type %s.",
-							classRef)));
-		}
-
-		List<String> primaryBeanNames = getPrimaryBeanDefinitions(beanNames, (ConfigurableListableBeanFactory) beanFactory);
-
-		if (primaryBeanNames.size() == 1) {
-			return primaryBeanNames.get(0);
-		}
-
-		if (primaryBeanNames.size() > 1) {
-			throw new NoUniqueBeanDefinitionException(classRef, primaryBeanNames.size(),
-					"more than one 'primary' bean found among candidates: " + primaryBeanNames);
-		}
-
-		for (String beanName : beanNames) {
-
-			if (propertyName.equals(beanName)
-					|| ObjectUtils.containsElement(beanFactory.getAliases(beanName), propertyName)) {
-				return beanName;
-			}
-		}
-
-		return nullOrThrowException(required,
-				() -> new NoSuchBeanDefinitionException(String.format("No bean of name %s found.", propertyName)));
-	}
-
-	private static List<String> getPrimaryBeanDefinitions(List<String> beanNames,
-			ConfigurableListableBeanFactory beanFactory) {
-
-		ArrayList<String> primaryBeanNames = new ArrayList<>();
-		for (String name : beanNames) {
-
-			if (beanFactory.getBeanDefinition(name).isPrimary()) {
-				primaryBeanNames.add(name);
-			}
-		}
-		return primaryBeanNames;
-	}
-
-	@Nullable
-	private static String nullOrThrowException(boolean required, Supplier<RuntimeException> exception) {
-
-		if (required) {
-			throw exception.get();
-		}
-		return null;
+		source.getAttribute("dataAccessStrategyRef") //
+				.filter(s -> !StringUtils.isEmpty(s)) //
+				.ifPresent(s -> builder.addPropertyReference("dataAccessStrategy", s));
 	}
 
 }

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

@@ -17,6 +17,7 @@ package org.springframework.data.jdbc.repository.support;
 
 import java.io.Serializable;
 
+import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.context.ApplicationEventPublisherAware;
@@ -47,6 +48,7 @@ public class JdbcRepositoryFactoryBean<T extends Repository<S, ID>, S, ID extend
 		extends TransactionalRepositoryFactoryBeanSupport<T, S, ID> implements ApplicationEventPublisherAware {
 
 	private ApplicationEventPublisher publisher;
+	private BeanFactory beanFactory;
 	private RelationalMappingContext mappingContext;
 	private RelationalConverter converter;
 	private DataAccessStrategy dataAccessStrategy;
@@ -113,7 +115,6 @@ public class JdbcRepositoryFactoryBean<T extends Repository<S, ID>, S, ID extend
 	/**
 	 * @param rowMapperMap can be {@literal null}. {@link #afterPropertiesSet()} defaults to {@link RowMapperMap#EMPTY} if
 	 *          {@literal null}.
-	 *
 	 * @deprecated use {@link #setQueryMappingConfiguration(QueryMappingConfiguration)} instead.
 	 */
 	@Deprecated
@@ -131,6 +132,14 @@ public class JdbcRepositoryFactoryBean<T extends Repository<S, ID>, S, ID extend
 		this.converter = converter;
 	}
 
+	@Override
+	public void setBeanFactory(BeanFactory beanFactory) {
+
+		super.setBeanFactory(beanFactory);
+
+		this.beanFactory = beanFactory;
+	}
+
 	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.repository.core.support.RepositoryFactoryBeanSupport#afterPropertiesSet()
@@ -141,12 +150,8 @@ public class JdbcRepositoryFactoryBean<T extends Repository<S, ID>, S, ID extend
 		Assert.state(this.mappingContext != null, "MappingContext is required and must not be null!");
 		Assert.state(this.converter != null, "RelationalConverter is required and must not be null!");
 
-		if (dataAccessStrategy == null) {
-
-			SqlGeneratorSource sqlGeneratorSource = new SqlGeneratorSource(mappingContext);
-			this.dataAccessStrategy = new DefaultDataAccessStrategy(sqlGeneratorSource, mappingContext, converter,
-					operations);
-		}
+		ensureJdbcOperationsIsInitialized();
+		ensureDataAccessStrategyIsInitialized();
 
 		if (queryMappingConfiguration == null) {
 			this.queryMappingConfiguration = QueryMappingConfiguration.EMPTY;
@@ -154,4 +159,27 @@ public class JdbcRepositoryFactoryBean<T extends Repository<S, ID>, S, ID extend
 
 		super.afterPropertiesSet();
 	}
+
+	private void ensureJdbcOperationsIsInitialized() {
+
+		if (operations != null) {
+			return;
+		}
+
+		operations = beanFactory.getBean(NamedParameterJdbcOperations.class);
+	}
+
+	private void ensureDataAccessStrategyIsInitialized() {
+
+		if (dataAccessStrategy != null) {
+			return;
+		}
+
+		dataAccessStrategy = beanFactory.getBeanProvider(DataAccessStrategy.class).getIfAvailable(() -> {
+
+			SqlGeneratorSource sqlGeneratorSource = new SqlGeneratorSource(mappingContext);
+			return new DefaultDataAccessStrategy(sqlGeneratorSource, mappingContext, converter, operations);
+		});
+	}
+
 }

spring-data-jdbc/src/test/java/org/springframework/data/jdbc/mybatis/MyBatisCustomizingNamespaceHsqlIntegrationTests.java

@@ -32,6 +32,7 @@ import org.mybatis.spring.SqlSessionTemplate;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Import;
+import org.springframework.context.annotation.Primary;
 import org.springframework.core.io.support.PathMatchingResourcePatternResolver;
 import org.springframework.data.jdbc.repository.config.EnableJdbcRepositories;
 import org.springframework.data.jdbc.testing.TestConfiguration;
@@ -105,6 +106,7 @@ public class MyBatisCustomizingNamespaceHsqlIntegrationTests {
 		}
 
 		@Bean
+		@Primary
 		MyBatisDataAccessStrategy dataAccessStrategy(SqlSession sqlSession) {
 
 			MyBatisDataAccessStrategy strategy = new MyBatisDataAccessStrategy(sqlSession);

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

@@ -30,6 +30,7 @@ import org.mybatis.spring.SqlSessionTemplate;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Import;
+import org.springframework.context.annotation.Primary;
 import org.springframework.data.jdbc.core.DataAccessStrategy;
 import org.springframework.data.jdbc.repository.config.EnableJdbcRepositories;
 import org.springframework.data.jdbc.testing.TestConfiguration;
@@ -88,8 +89,10 @@ public class MyBatisHsqlIntegrationTests {
 		}
 
 		@Bean
+		@Primary
 		DataAccessStrategy dataAccessStrategy(RelationalMappingContext context, RelationalConverter converter,
 				SqlSession sqlSession, EmbeddedDatabase db) {
+
 			return MyBatisDataAccessStrategy.createCombinedAccessStrategy(context, converter,
 					new NamedParameterJdbcTemplate(db), sqlSession);
 		}

spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/config/EnableJdbcRepositoriesIntegrationTests.java

@@ -79,6 +79,7 @@ public class EnableJdbcRepositoriesIntegrationTests {
 
 	@BeforeClass
 	public static void setup() {
+
 		MAPPER_MAP.setAccessible(true);
 		OPERATIONS.setAccessible(true);
 		DATA_ACCESS_STRATEGY.setAccessible(true);
@@ -105,8 +106,9 @@ public class EnableJdbcRepositoriesIntegrationTests {
 
  	@Test // DATAJDBC-293
 	public void jdbcOperationsRef() {
+
 		NamedParameterJdbcOperations operations = (NamedParameterJdbcOperations) ReflectionUtils.getField(OPERATIONS, factoryBean);
-		assertThat(operations).isNotSameAs(defaultDataAccessStrategy).isSameAs(qualifierJdbcOperations);
+		assertThat(operations).isNotSameAs(defaultOperations).isSameAs(qualifierJdbcOperations);
 
 		DataAccessStrategy dataAccessStrategy = (DataAccessStrategy) ReflectionUtils.getField(DATA_ACCESS_STRATEGY, factoryBean);
 		assertThat(dataAccessStrategy).isNotSameAs(defaultDataAccessStrategy).isSameAs(qualifierDataAccessStrategy);

spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/config/JdbcRepositoryConfigExtensionUnitTests.java

@@ -1,146 +0,0 @@
-/*
- * Copyright 2018-2019 the original author or authors.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.springframework.data.jdbc.repository.config;
-
-import static org.assertj.core.api.Assertions.*;
-import static org.mockito.Mockito.*;
-
-import org.junit.Test;
-import org.springframework.beans.factory.NoSuchBeanDefinitionException;
-import org.springframework.beans.factory.config.RuntimeBeanReference;
-import org.springframework.beans.factory.support.BeanDefinitionBuilder;
-import org.springframework.beans.factory.support.DefaultListableBeanFactory;
-import org.springframework.context.support.GenericApplicationContext;
-import org.springframework.data.repository.config.RepositoryConfigurationSource;
-import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations;
-
-/**
- * @author Jens Schauder
- */
-public class JdbcRepositoryConfigExtensionUnitTests {
-
-	BeanDefinitionBuilder definitionBuilder = BeanDefinitionBuilder.genericBeanDefinition();
-	RepositoryConfigurationSource configSource = mock(RepositoryConfigurationSource.class);
-	DefaultListableBeanFactory listableBeanFactory = new DefaultListableBeanFactory();
-
-	JdbcRepositoryConfigExtension extension = new JdbcRepositoryConfigExtension();
-
-	@Test // DATAJDBC-293
-	public void exceptionIsThrownOnPostProcessIfNoBeanFactoryIsAvailable() {
-
-		assertThatThrownBy( //
-				() -> extension.postProcess(definitionBuilder, configSource)) //
-						.isInstanceOf(NoSuchBeanDefinitionException.class) //
-						.hasMessageContaining("No BeanFactory");
-
-	}
-
-	@Test // DATAJDBC-293
-	public void exceptionIsThrownOnPostProcessIfNoJdbcOperationsBeanIsAvailable() {
-
-		extension.registerBeansForRoot(listableBeanFactory, null);
-
-		assertThatThrownBy( //
-				() -> extension.postProcess(definitionBuilder, configSource)) //
-						.isInstanceOf(NoSuchBeanDefinitionException.class) //
-						.hasMessageContaining("NamedParameterJdbcOperations"); //
-
-	}
-
-	@Test // DATAJDBC-293
-	public void exceptionIsThrownOnPostProcessIfMultipleJdbcOperationsBeansAreAvailableAndNoConfigurableBeanFactoryAvailable() {
-
-		GenericApplicationContext applicationContext = new GenericApplicationContext();
-
-		applicationContext.registerBean( //
-				"one", //
-				NamedParameterJdbcOperations.class, //
-				() -> mock(NamedParameterJdbcOperations.class));
-		applicationContext.registerBean( //
-				"two", //
-				NamedParameterJdbcOperations.class, //
-				() -> mock(NamedParameterJdbcOperations.class));
-
-		applicationContext.refresh();
-
-		extension.registerBeansForRoot(applicationContext, null);
-
-		assertThatThrownBy( //
-				() -> extension.postProcess(definitionBuilder, configSource)) //
-						.isInstanceOf(NoSuchBeanDefinitionException.class) //
-						.hasMessageContaining("NamedParameterJdbcOperations"); //
-
-	}
-
-	@Test // DATAJDBC-293
-	public void exceptionIsThrownOnPostProcessIfMultiplePrimaryNoJdbcOperationsBeansAreAvailable() {
-
-		registerJdbcOperations("one", true);
-		registerJdbcOperations("two", true);
-
-		extension.registerBeansForRoot(listableBeanFactory, null);
-
-		assertThatThrownBy( //
-				() -> extension.postProcess(definitionBuilder, configSource)) //
-						.isInstanceOf(NoSuchBeanDefinitionException.class) //
-						.hasMessageContaining("NamedParameterJdbcOperations"); //
-
-	}
-
-	@Test // DATAJDBC-293
-	public void uniquePrimaryBeanIsUsedOfNamedParameterJdbcOperations() {
-
-		registerJdbcOperations("one", false);
-		registerJdbcOperations("two", true);
-
-		extension.registerBeansForRoot(listableBeanFactory, null);
-
-		extension.postProcess(definitionBuilder, configSource);
-
-		Object jdbcOperations = definitionBuilder.getBeanDefinition().getPropertyValues().get("jdbcOperations");
-
-		assertThat(jdbcOperations) //
-				.isInstanceOf(RuntimeBeanReference.class) //
-				.extracting(rbr -> ((RuntimeBeanReference) rbr).getBeanName()).contains("two");
-	}
-
-	@Test // DATAJDBC-293
-	public void matchesByNameAsLastResort() {
-
-		registerJdbcOperations("jdbcOperations", false);
-		registerJdbcOperations("two", false);
-
-		extension.registerBeansForRoot(listableBeanFactory, null);
-
-		extension.postProcess(definitionBuilder, configSource);
-
-		Object jdbcOperations = definitionBuilder.getBeanDefinition().getPropertyValues().get("jdbcOperations");
-
-		assertThat(jdbcOperations) //
-				.isInstanceOf(RuntimeBeanReference.class) //
-				.extracting(rbr -> ((RuntimeBeanReference) rbr).getBeanName()).contains("jdbcOperations");
-
-	}
-
-	private void registerJdbcOperations(String name, boolean primary) {
-
-		listableBeanFactory.registerBeanDefinition(name, BeanDefinitionBuilder.genericBeanDefinition( //
-				NamedParameterJdbcOperations.class, //
-				() -> mock(NamedParameterJdbcOperations.class)) //
-				.applyCustomizers(bd -> bd.setPrimary(primary)) //
-				.getBeanDefinition());
-	}
-}

spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/support/JdbcRepositoryFactoryBeanUnitTests.java

@@ -21,9 +21,14 @@ import static org.mockito.Mockito.*;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Answers;
 import org.mockito.Mock;
+import org.mockito.invocation.InvocationOnMock;
 import org.mockito.junit.MockitoJUnitRunner;
+import org.mockito.stubbing.Answer;
 import org.springframework.beans.factory.BeanFactory;
+import org.springframework.beans.factory.ListableBeanFactory;
+import org.springframework.beans.factory.ObjectProvider;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.data.annotation.Id;
 import org.springframework.data.jdbc.core.DataAccessStrategy;
@@ -33,8 +38,11 @@ import org.springframework.data.jdbc.repository.QueryMappingConfiguration;
 import org.springframework.data.relational.core.conversion.BasicRelationalConverter;
 import org.springframework.data.relational.core.mapping.RelationalMappingContext;
 import org.springframework.data.repository.CrudRepository;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations;
 import org.springframework.test.util.ReflectionTestUtils;
 
+import java.util.function.Supplier;
+
 /**
  * Tests the dependency injection for {@link JdbcRepositoryFactoryBean}.
  *
@@ -44,7 +52,6 @@ import org.springframework.test.util.ReflectionTestUtils;
  * @author Oliver Gierke
  * @author Mark Paluch
  * @author Evgeni Dimitrov
- *
  */
 @RunWith(MockitoJUnitRunner.class)
 public class JdbcRepositoryFactoryBeanUnitTests {
@@ -53,6 +60,7 @@ public class JdbcRepositoryFactoryBeanUnitTests {
 
 	@Mock DataAccessStrategy dataAccessStrategy;
 	@Mock ApplicationEventPublisher publisher;
+	@Mock ListableBeanFactory beanFactory;
 
 	RelationalMappingContext mappingContext;
 
@@ -63,6 +71,14 @@ public class JdbcRepositoryFactoryBeanUnitTests {
 
 		// Setup standard configuration
 		factoryBean = new JdbcRepositoryFactoryBean<>(DummyEntityRepository.class);
+
+
+		when(beanFactory.getBean(NamedParameterJdbcOperations.class)).thenReturn(mock(NamedParameterJdbcOperations.class));
+
+		ObjectProvider<DataAccessStrategy> provider = mock(ObjectProvider.class);
+		when(beanFactory.getBeanProvider(DataAccessStrategy.class)).thenReturn(provider);
+		when(provider.getIfAvailable(any()))
+				.then((Answer) invocation -> ((Supplier)invocation.getArgument(0)).get());
 	}
 
 	@Test
@@ -72,6 +88,7 @@ public class JdbcRepositoryFactoryBeanUnitTests {
 		factoryBean.setMappingContext(mappingContext);
 		factoryBean.setConverter(new BasicRelationalConverter(mappingContext));
 		factoryBean.setApplicationEventPublisher(publisher);
+		factoryBean.setBeanFactory(beanFactory);
 		factoryBean.afterPropertiesSet();
 
 		assertThat(factoryBean.getObject()).isNotNull();
@@ -88,6 +105,7 @@ public class JdbcRepositoryFactoryBeanUnitTests {
 
 		factoryBean.setMappingContext(null);
 		factoryBean.setApplicationEventPublisher(publisher);
+		factoryBean.setBeanFactory(beanFactory);
 		factoryBean.afterPropertiesSet();
 	}
 
@@ -97,12 +115,14 @@ public class JdbcRepositoryFactoryBeanUnitTests {
 		factoryBean.setMappingContext(mappingContext);
 		factoryBean.setConverter(new BasicRelationalConverter(mappingContext));
 		factoryBean.setApplicationEventPublisher(publisher);
+		factoryBean.setBeanFactory(beanFactory);
 		factoryBean.afterPropertiesSet();
 
 		assertThat(factoryBean.getObject()).isNotNull();
 		assertThat(ReflectionTestUtils.getField(factoryBean, "dataAccessStrategy"))
 				.isInstanceOf(DefaultDataAccessStrategy.class);
-		assertThat(ReflectionTestUtils.getField(factoryBean, "queryMappingConfiguration")).isEqualTo(QueryMappingConfiguration.EMPTY);
+		assertThat(ReflectionTestUtils.getField(factoryBean, "queryMappingConfiguration"))
+				.isEqualTo(QueryMappingConfiguration.EMPTY);
 	}
 
 	private static class DummyEntity {