From 3bae636e2fa7f2a0ec569c5677c8f91d32dc09a8 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 27 Sep 2017 09:43:45 +0200 Subject: [PATCH] DATACMNS-1172 - Limit repository custom implementation scanning by default to repository interface packages. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Custom repository implementation scan defaults to the repository interface package and its subpackages and no longer scans all configured base packages. Scan for fragment implementations defaults to the fragment interface package. Using the interface package for scanning aligns the behavior with the documentation. This default can be changed with @Enable…Repositories annotations that support the limitImplementationBasePackages parameter to scan in repository base packages for custom implementations: @EnableJpaRepositories(limitImplementationBasePackages = false) @Configuration class AppConfiguration { // … } Declaring the implementation along with the interface in the same package is an established design pattern allowing to limit the scanning scope. A limited scope improves scanning performance as it can skip elements on the classpath, that do not provide that particular package. Previously, we scanned for implementations using the configured base packages that were also used to discover repository interfaces. These base packages can be broader for applications that spread repository interfaces across multiple packages. --- ...notationRepositoryConfigurationSource.java | 16 ++++++++++ ...ustomRepositoryImplementationDetector.java | 3 +- .../DefaultRepositoryConfiguration.java | 13 ++++++++ .../RepositoryBeanDefinitionBuilder.java | 3 +- .../config/RepositoryConfiguration.java | 9 ++++++ .../config/RepositoryConfigurationSource.java | 12 +++++++ .../RepositoryConfigurationSourceSupport.java | 9 ++++++ ...epositoryConfigurationSourceUnitTests.java | 16 ++++++++-- ...faultRepositoryConfigurationUnitTests.java | 32 ++++++++++++++++++- .../repository/config/EnableRepositories.java | 4 ++- ...anDefinitionRegistrarSupportUnitTests.java | 31 ++++++++++++++++-- .../config/basepackage/FragmentImpl.java | 23 +++++++++++++ .../config/basepackage/repo/Fragment.java | 21 ++++++++++++ .../basepackage/repo/PersonRepository.java | 24 ++++++++++++++ 14 files changed, 208 insertions(+), 8 deletions(-) create mode 100644 src/test/java/org/springframework/data/repository/config/basepackage/FragmentImpl.java create mode 100644 src/test/java/org/springframework/data/repository/config/basepackage/repo/Fragment.java create mode 100644 src/test/java/org/springframework/data/repository/config/basepackage/repo/PersonRepository.java diff --git a/src/main/java/org/springframework/data/repository/config/AnnotationRepositoryConfigurationSource.java b/src/main/java/org/springframework/data/repository/config/AnnotationRepositoryConfigurationSource.java index 1db850228..618fa1338 100644 --- a/src/main/java/org/springframework/data/repository/config/AnnotationRepositoryConfigurationSource.java +++ b/src/main/java/org/springframework/data/repository/config/AnnotationRepositoryConfigurationSource.java @@ -53,6 +53,7 @@ import org.springframework.util.StringUtils; * @author Thomas Darimont * @author Peter Rietzler * @author Jens Schauder + * @author Mark Paluch */ public class AnnotationRepositoryConfigurationSource extends RepositoryConfigurationSourceSupport { @@ -64,6 +65,7 @@ public class AnnotationRepositoryConfigurationSource extends RepositoryConfigura private static final String REPOSITORY_FACTORY_BEAN_CLASS = "repositoryFactoryBeanClass"; private static final String REPOSITORY_BASE_CLASS = "repositoryBaseClass"; private static final String CONSIDER_NESTED_REPOSITORIES = "considerNestedRepositories"; + private static final String LIMIT_IMPLEMENTATION_BASE_PACKAGES = "limitImplementationBasePackages"; private final AnnotationMetadata configMetadata; private final AnnotationMetadata enableAnnotationMetadata; @@ -320,6 +322,20 @@ public class AnnotationRepositoryConfigurationSource extends RepositoryConfigura return attributes.containsKey(CONSIDER_NESTED_REPOSITORIES) && attributes.getBoolean(CONSIDER_NESTED_REPOSITORIES); } + /* + * (non-Javadoc) + * @see org.springframework.data.repository.config.RepositoryConfigurationSourceSupport#shouldLimitRepositoryImplementationBasePackages() + */ + @Override + public boolean shouldLimitRepositoryImplementationBasePackages() { + + if (!attributes.containsKey(LIMIT_IMPLEMENTATION_BASE_PACKAGES)) { + return true; + } + + return attributes.getBoolean(LIMIT_IMPLEMENTATION_BASE_PACKAGES); + } + /* * (non-Javadoc) * @see org.springframework.data.repository.config.RepositoryConfigurationSource#getAttribute(java.lang.String) diff --git a/src/main/java/org/springframework/data/repository/config/CustomRepositoryImplementationDetector.java b/src/main/java/org/springframework/data/repository/config/CustomRepositoryImplementationDetector.java index 593c110b7..3552ab2ff 100644 --- a/src/main/java/org/springframework/data/repository/config/CustomRepositoryImplementationDetector.java +++ b/src/main/java/org/springframework/data/repository/config/CustomRepositoryImplementationDetector.java @@ -46,6 +46,7 @@ import org.springframework.util.Assert; * @author Christoph Strobl * @author Peter Rietzler * @author Jens Schauder + * @author Mark Paluch */ @RequiredArgsConstructor public class CustomRepositoryImplementationDetector { @@ -71,7 +72,7 @@ public class CustomRepositoryImplementationDetector { return detectCustomImplementation( // configuration.getImplementationClassName(), // configuration.getImplementationBeanName(), // - configuration.getBasePackages(), // + configuration.getImplementationBasePackages(configuration.getImplementationClassName()), // configuration.getExcludeFilters(), // bd -> configuration.getConfigurationSource().generateBeanName(bd)); } diff --git a/src/main/java/org/springframework/data/repository/config/DefaultRepositoryConfiguration.java b/src/main/java/org/springframework/data/repository/config/DefaultRepositoryConfiguration.java index 7bb577749..98180ddc4 100644 --- a/src/main/java/org/springframework/data/repository/config/DefaultRepositoryConfiguration.java +++ b/src/main/java/org/springframework/data/repository/config/DefaultRepositoryConfiguration.java @@ -34,6 +34,7 @@ import org.springframework.util.StringUtils; * * @author Oliver Gierke * @author Jens Schauder + * @author Mark Paluch */ @RequiredArgsConstructor public class DefaultRepositoryConfiguration @@ -71,6 +72,18 @@ public class DefaultRepositoryConfiguration getImplementationBasePackages(String interfaceClassName) { + + return configurationSource.shouldLimitRepositoryImplementationBasePackages() + ? Streamable.of(ClassUtils.getPackageName(interfaceClassName)) + : getBasePackages(); + } + /* * (non-Javadoc) * @see org.springframework.data.repository.config.RepositoryConfiguration#getRepositoryInterface() diff --git a/src/main/java/org/springframework/data/repository/config/RepositoryBeanDefinitionBuilder.java b/src/main/java/org/springframework/data/repository/config/RepositoryBeanDefinitionBuilder.java index f351b8707..7d5b81c81 100644 --- a/src/main/java/org/springframework/data/repository/config/RepositoryBeanDefinitionBuilder.java +++ b/src/main/java/org/springframework/data/repository/config/RepositoryBeanDefinitionBuilder.java @@ -202,7 +202,8 @@ class RepositoryBeanDefinitionBuilder { .concat(configuration.getConfigurationSource().getRepositoryImplementationPostfix().orElse("Impl")); Optional beanDefinition = implementationDetector.detectCustomImplementation(className, null, - configuration.getBasePackages(), exclusions, bd -> configuration.getConfigurationSource().generateBeanName(bd)); + configuration.getImplementationBasePackages(fragmentInterfaceName), exclusions, + bd -> configuration.getConfigurationSource().generateBeanName(bd)); return beanDefinition.map(bd -> new RepositoryFragmentConfiguration(fragmentInterfaceName, bd)); } diff --git a/src/main/java/org/springframework/data/repository/config/RepositoryConfiguration.java b/src/main/java/org/springframework/data/repository/config/RepositoryConfiguration.java index ccd418315..b1dc5368b 100644 --- a/src/main/java/org/springframework/data/repository/config/RepositoryConfiguration.java +++ b/src/main/java/org/springframework/data/repository/config/RepositoryConfiguration.java @@ -37,6 +37,15 @@ public interface RepositoryConfiguration getBasePackages(); + /** + * Returns the base packages to scan for repository implementations. + * + * @param interfaceClassName class name of the interface. + * @return + * @since 2.0 + */ + Streamable getImplementationBasePackages(String interfaceClassName); + /** * Returns the interface name of the repository. * diff --git a/src/main/java/org/springframework/data/repository/config/RepositoryConfigurationSource.java b/src/main/java/org/springframework/data/repository/config/RepositoryConfigurationSource.java index 3e4113e94..d48170f9c 100644 --- a/src/main/java/org/springframework/data/repository/config/RepositoryConfigurationSource.java +++ b/src/main/java/org/springframework/data/repository/config/RepositoryConfigurationSource.java @@ -31,6 +31,7 @@ import org.springframework.lang.Nullable; * @author Thomas Darimont * @author Peter Rietzler * @author Jens Schauder + * @author Mark Paluch */ public interface RepositoryConfigurationSource { @@ -62,6 +63,17 @@ public interface RepositoryConfigurationSource { */ Optional getRepositoryImplementationPostfix(); + /** + * Returns whether to limit repository implementation base packages for custom implementation scanning. If + * {@literal true}, then custom implementation scanning considers only the package of the repository/fragment + * interface and its subpackages for a scan. Otherwise, all {@link #getBasePackages()} are scanned for repository + * implementations + * + * @return {@literal true} if base packages are limited to the actual repository package. + * @since 2.0 + */ + boolean shouldLimitRepositoryImplementationBasePackages(); + /** * @return */ diff --git a/src/main/java/org/springframework/data/repository/config/RepositoryConfigurationSourceSupport.java b/src/main/java/org/springframework/data/repository/config/RepositoryConfigurationSourceSupport.java index 2014a7558..3851a9cbc 100644 --- a/src/main/java/org/springframework/data/repository/config/RepositoryConfigurationSourceSupport.java +++ b/src/main/java/org/springframework/data/repository/config/RepositoryConfigurationSourceSupport.java @@ -117,4 +117,13 @@ public abstract class RepositoryConfigurationSourceSupport implements Repository public boolean shouldConsiderNestedRepositories() { return false; } + + /* + * (non-Javadoc) + * @see org.springframework.data.repository.config.RepositoryConfigurationSource#isLimitRepositoryImplementationBasePackages() + */ + @Override + public boolean shouldLimitRepositoryImplementationBasePackages() { + return true; + } } diff --git a/src/test/java/org/springframework/data/repository/config/AnnotationRepositoryConfigurationSourceUnitTests.java b/src/test/java/org/springframework/data/repository/config/AnnotationRepositoryConfigurationSourceUnitTests.java index a15e184ed..fbb5bfaec 100755 --- a/src/test/java/org/springframework/data/repository/config/AnnotationRepositoryConfigurationSourceUnitTests.java +++ b/src/test/java/org/springframework/data/repository/config/AnnotationRepositoryConfigurationSourceUnitTests.java @@ -73,8 +73,9 @@ public class AnnotationRepositoryConfigurationSourceUnitTests { Streamable candidates = source.getCandidates(new DefaultResourceLoader()); - assertThat(candidates).hasSize(2).extracting("beanClassName").containsOnly(MyRepository.class.getName(), - ComposedRepository.class.getName()); + assertThat(candidates).extracting("beanClassName") + .contains(MyRepository.class.getName(), ComposedRepository.class.getName()) + .doesNotContain(MyOtherRepository.class.getName(), ExcludedRepository.class.getName()); } @Test // DATACMNS-47 @@ -102,6 +103,14 @@ public class AnnotationRepositoryConfigurationSourceUnitTests { assertThat(source.shouldConsiderNestedRepositories()).isTrue(); } + @Test // DATACMNS-1172 + public void returnsLimitImplementationBasePackages() { + + assertThat(getConfigSource(DefaultConfiguration.class).shouldLimitRepositoryImplementationBasePackages()).isTrue(); + assertThat(getConfigSource(DefaultConfigurationWithoutBasePackageLimit.class) + .shouldLimitRepositoryImplementationBasePackages()).isFalse(); + } + @Test // DATACMNS-456 public void findsStringAttributeByName() { @@ -155,6 +164,9 @@ public class AnnotationRepositoryConfigurationSourceUnitTests { @EnableRepositories(considerNestedRepositories = true) static class DefaultConfigurationWithNestedRepositories {} + @EnableRepositories(limitImplementationBasePackages = false) + static class DefaultConfigurationWithoutBasePackageLimit {} + @EnableRepositories(excludeFilters = { @Filter(Primary.class) }) static class ConfigurationWithExplicitFilter {} diff --git a/src/test/java/org/springframework/data/repository/config/DefaultRepositoryConfigurationUnitTests.java b/src/test/java/org/springframework/data/repository/config/DefaultRepositoryConfigurationUnitTests.java index 42253d1ea..e71f66c48 100755 --- a/src/test/java/org/springframework/data/repository/config/DefaultRepositoryConfigurationUnitTests.java +++ b/src/test/java/org/springframework/data/repository/config/DefaultRepositoryConfigurationUnitTests.java @@ -34,19 +34,20 @@ import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.config.ConstructorArgumentValues; import org.springframework.beans.factory.support.RootBeanDefinition; import org.springframework.data.repository.query.QueryLookupStrategy.Key; +import org.springframework.data.util.Streamable; /** * Unit tests for {@link DefaultRepositoryConfiguration}. * * @author Oliver Gierke * @author Jens Schauder + * @author Mark Paluch */ @RunWith(MockitoJUnitRunner.class) public class DefaultRepositoryConfigurationUnitTests { @Mock RepositoryConfigurationSource source; - BeanDefinition definition = new RootBeanDefinition("com.acme.MyRepository"); RepositoryConfigurationExtension extension = new SimplerRepositoryConfigurationExtension("factory", "module"); @Before @@ -83,6 +84,33 @@ public class DefaultRepositoryConfigurationUnitTests { assertThat(getConfiguration(source).getRepositoryFactoryBeanClassName()).isEqualTo("custom"); } + @Test // DATACMNS-1172 + public void limitsImplementationBasePackages() { + + when(source.shouldLimitRepositoryImplementationBasePackages()).thenReturn(true); + + assertThat(getConfiguration(source).getImplementationBasePackages("com.acme.MyRepository")) + .containsOnly("com.acme"); + } + + @Test // DATACMNS-1172 + public void limitsImplementationBasePackagesOfNestedClass() { + + when(source.shouldLimitRepositoryImplementationBasePackages()).thenReturn(true); + + assertThat(getConfiguration(source).getImplementationBasePackages(NestedInterface.class.getName())) + .containsOnly("org.springframework.data.repository.config"); + } + + @Test // DATACMNS-1172 + public void shouldNotLimitImplementationBasePackages() { + + when(source.getBasePackages()).thenReturn(Streamable.of("com", "org.coyote")); + + assertThat(getConfiguration(source).getImplementationBasePackages("com.acme.MyRepository")).contains("com", + "org.coyote"); + } + private DefaultRepositoryConfiguration getConfiguration( RepositoryConfigurationSource source) { RootBeanDefinition beanDefinition = createBeanDefinition(); @@ -105,4 +133,6 @@ public class DefaultRepositoryConfigurationUnitTests { return beanDefinition; } + + private interface NestedInterface {} } diff --git a/src/test/java/org/springframework/data/repository/config/EnableRepositories.java b/src/test/java/org/springframework/data/repository/config/EnableRepositories.java index 41798ad26..d6991138c 100644 --- a/src/test/java/org/springframework/data/repository/config/EnableRepositories.java +++ b/src/test/java/org/springframework/data/repository/config/EnableRepositories.java @@ -1,5 +1,5 @@ /* - * Copyright 2012 the original author or authors. + * Copyright 2012-2017 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. @@ -49,4 +49,6 @@ public @interface EnableRepositories { String repositoryImplementationPostfix() default "Impl"; boolean considerNestedRepositories() default false; + + boolean limitImplementationBasePackages() default true; } diff --git a/src/test/java/org/springframework/data/repository/config/RepositoryBeanDefinitionRegistrarSupportUnitTests.java b/src/test/java/org/springframework/data/repository/config/RepositoryBeanDefinitionRegistrarSupportUnitTests.java index 5add796e4..f8d37edcd 100755 --- a/src/test/java/org/springframework/data/repository/config/RepositoryBeanDefinitionRegistrarSupportUnitTests.java +++ b/src/test/java/org/springframework/data/repository/config/RepositoryBeanDefinitionRegistrarSupportUnitTests.java @@ -34,6 +34,7 @@ import org.springframework.core.env.StandardEnvironment; import org.springframework.core.io.DefaultResourceLoader; import org.springframework.core.type.AnnotationMetadata; import org.springframework.core.type.StandardAnnotationMetadata; +import org.springframework.data.repository.config.basepackage.FragmentImpl; import org.springframework.data.repository.core.support.DummyRepositoryFactoryBean; /** @@ -84,6 +85,28 @@ public class RepositoryBeanDefinitionRegistrarSupportUnitTests { assertNoBeanDefinitionRegisteredFor("excludedRepositoryImpl"); } + @Test // DATACMNS-1172 + public void shouldLimitImplementationBasePackages() { + + AnnotationMetadata metadata = new StandardAnnotationMetadata(LimitsImplementationBasePackages.class, true); + + registrar.registerBeanDefinitions(metadata, registry); + + assertBeanDefinitionRegisteredFor("personRepository"); + assertNoBeanDefinitionRegisteredFor("fragmentImpl"); + } + + @Test // DATACMNS-1172 + public void shouldNotLimitImplementationBasePackages() { + + AnnotationMetadata metadata = new StandardAnnotationMetadata(UnlimitedImplementationBasePackages.class, true); + + registrar.registerBeanDefinitions(metadata, registry); + + assertBeanDefinitionRegisteredFor("personRepository"); + assertBeanDefinitionRegisteredFor("fragmentImpl"); + } + @Test // DATACMNS-360 public void registeredProfileRepositoriesIfProfileActivated() { @@ -152,7 +175,11 @@ public class RepositoryBeanDefinitionRegistrarSupportUnitTests { @EnableRepositories( includeFilters = @Filter(type = FilterType.ASSIGNABLE_TYPE, value = RepositoryWithFragmentExclusion.class), basePackageClasses = RepositoryWithFragmentExclusion.class) - static class FragmentExclusionConfiguration { + static class FragmentExclusionConfiguration {} - } + @EnableRepositories(basePackageClasses = FragmentImpl.class) + static class LimitsImplementationBasePackages {} + + @EnableRepositories(basePackageClasses = FragmentImpl.class, limitImplementationBasePackages = false) + static class UnlimitedImplementationBasePackages {} } diff --git a/src/test/java/org/springframework/data/repository/config/basepackage/FragmentImpl.java b/src/test/java/org/springframework/data/repository/config/basepackage/FragmentImpl.java new file mode 100644 index 000000000..b1dfb3a4a --- /dev/null +++ b/src/test/java/org/springframework/data/repository/config/basepackage/FragmentImpl.java @@ -0,0 +1,23 @@ +/* + * Copyright 2017 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.repository.config.basepackage; + +import org.springframework.data.repository.config.basepackage.repo.Fragment; + +/** + * @author Mark Paluch + */ +public class FragmentImpl implements Fragment {} diff --git a/src/test/java/org/springframework/data/repository/config/basepackage/repo/Fragment.java b/src/test/java/org/springframework/data/repository/config/basepackage/repo/Fragment.java new file mode 100644 index 000000000..d4a35d861 --- /dev/null +++ b/src/test/java/org/springframework/data/repository/config/basepackage/repo/Fragment.java @@ -0,0 +1,21 @@ +/* + * Copyright 2017 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.repository.config.basepackage.repo; + +/** + * @author Mark Paluch + */ +public interface Fragment {} diff --git a/src/test/java/org/springframework/data/repository/config/basepackage/repo/PersonRepository.java b/src/test/java/org/springframework/data/repository/config/basepackage/repo/PersonRepository.java new file mode 100644 index 000000000..f94996761 --- /dev/null +++ b/src/test/java/org/springframework/data/repository/config/basepackage/repo/PersonRepository.java @@ -0,0 +1,24 @@ +/* + * Copyright 2017 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.repository.config.basepackage.repo; + +import org.springframework.data.mapping.Person; +import org.springframework.data.repository.Repository; + +/** + * @author Mark Paluch + */ +public interface PersonRepository extends Repository, Fragment {}