From 893087f469a95ca3ba8a69a9cd2abcfc6cde01db Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 26 Sep 2017 16:27:59 +0200 Subject: [PATCH 1/2] DATACMNS-1172 - Slow boot caused by CustomRepositoryImplementationDetector.detectCustomImplementation(). Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 0cd3b24018..45c9f26589 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 2.0.0.BUILD-SNAPSHOT + 2.0.0.DATACMNS-1172-SNAPSHOT Spring Data Core From 6bf35da3e4b08a4476c728fa9542a6c114249ade Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 27 Sep 2017 09:43:45 +0200 Subject: [PATCH 2/2] 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 1db8502280..618fa13382 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 @@ * @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 boolean shouldConsiderNestedRepositories() { 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 593c110b76..3552ab2fff 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 @@ * @author Christoph Strobl * @author Peter Rietzler * @author Jens Schauder + * @author Mark Paluch */ @RequiredArgsConstructor public class CustomRepositoryImplementationDetector { @@ -71,7 +72,7 @@ public Optional detectCustomImplementation(RepositoryCon 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 7bb5777497..98180ddc43 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 @@ * * @author Oliver Gierke * @author Jens Schauder + * @author Mark Paluch */ @RequiredArgsConstructor public class DefaultRepositoryConfiguration @@ -71,6 +72,18 @@ public Streamable getBasePackages() { return configurationSource.getBasePackages(); } + /* + * (non-Javadoc) + * @see org.springframework.data.repository.config.RepositoryConfiguration#getBasePackages(String) + */ + @Override + public Streamable 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 f351b87074..7d5b81c81a 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 @@ private Optional detectRepositoryFragmentConfig .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 ccd4183155..b1dc5368b2 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 3e4113e948..d48170f9cb 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 @@ * @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 2014a75589..3851a9cbc1 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 @@ protected Iterable getIncludeFilters() { 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 a15e184ed4..fbb5bfaec6 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 void evaluatesExcludeFiltersCorrectly() { 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 void returnsConsiderNestedRepositories() { 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 @@ static class DefaultConfigurationWithBasePackage {} @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 42253d1ea2..e71f66c483 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.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 void prefersSourcesRepositoryFactoryBeanClass() { 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 @@ private static RootBeanDefinition createBeanDefinition() { 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 41798ad26b..d6991138c6 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 @@ 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 5add796e46..f8d37edcd4 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.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 void registersBeanDefinitionWithoutFragmentImplementations() { 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 @@ protected String getModulePrefix() { @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 0000000000..b1dfb3a4aa --- /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 0000000000..d4a35d8612 --- /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 0000000000..f94996761b --- /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 {}