From a8a2a0155f0d29f1f60961c62cdc3b572f18b861 Mon Sep 17 00:00:00 2001 From: Michal Domagala Date: Tue, 3 Nov 2020 10:31:23 +0100 Subject: [PATCH] Fix for @EnableFeignClients(clients) scans classes in nested packages (#422) * Test to verify sub level client error Error is described in https://github.com/spring-cloud/spring-cloud-openfeign/issues/331 Application fails when one Feign client is `org.TopClient` and second is in subpackage: `org.sub.SubClient` The error reason is that `SubClient` is registered twice * Fix for @EnableFeignClients(clients) scans classes in nested packages twice https://github.com/spring-cloud/spring-cloud-openfeign/issues/331 (cherry picked from commit fe62b0db9002052a72e5bbb9fdc5458993c16951) # Conflicts: # spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsRegistrar.java * delete unused code (cherry picked from commit 4df865d1d64fc0a9b5e37ea4c6466a0dbe485141) * add file header * Apply review Co-authored-by: michal --- .../openfeign/FeignClientsRegistrar.java | 105 +++++------------- .../openfeign/FeignClientsRegistrarTests.java | 22 ++++ .../feignclientsregistrar/TopLevelClient.java | 27 +++++ .../sub/SubLevelClient.java | 27 +++++ 4 files changed, 102 insertions(+), 79 deletions(-) create mode 100644 spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/feignclientsregistrar/TopLevelClient.java create mode 100644 spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/feignclientsregistrar/sub/SubLevelClient.java diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsRegistrar.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsRegistrar.java index 4dfb48d3..dbb04fef 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsRegistrar.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsRegistrar.java @@ -16,19 +16,18 @@ package org.springframework.cloud.openfeign; -import java.io.IOException; import java.net.MalformedURLException; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; -import java.util.Arrays; import java.util.HashSet; -import java.util.List; +import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; import org.springframework.beans.factory.FactoryBean; import org.springframework.beans.factory.annotation.AnnotatedBeanDefinition; +import org.springframework.beans.factory.annotation.AnnotatedGenericBeanDefinition; import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.config.BeanDefinitionHolder; import org.springframework.beans.factory.support.AbstractBeanDefinition; @@ -43,12 +42,7 @@ import org.springframework.core.annotation.AnnotationAttributes; import org.springframework.core.env.Environment; import org.springframework.core.io.ResourceLoader; import org.springframework.core.type.AnnotationMetadata; -import org.springframework.core.type.ClassMetadata; -import org.springframework.core.type.classreading.MetadataReader; -import org.springframework.core.type.classreading.MetadataReaderFactory; -import org.springframework.core.type.filter.AbstractClassTestingTypeFilter; import org.springframework.core.type.filter.AnnotationTypeFilter; -import org.springframework.core.type.filter.TypeFilter; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.StringUtils; @@ -58,6 +52,7 @@ import org.springframework.util.StringUtils; * @author Jakub Narloch * @author Venil Noronha * @author Gang Li + * @author Michal Domagala */ class FeignClientsRegistrar implements ImportBeanDefinitionRegistrar, ResourceLoaderAware, EnvironmentAware { @@ -165,11 +160,8 @@ class FeignClientsRegistrar public void registerFeignClients(AnnotationMetadata metadata, BeanDefinitionRegistry registry) { - ClassPathScanningCandidateComponentProvider scanner = getScanner(); - scanner.setResourceLoader(this.resourceLoader); - - Set basePackages; + LinkedHashSet candidateComponents = new LinkedHashSet<>(); Map attrs = metadata .getAnnotationAttributes(EnableFeignClients.class.getName()); AnnotationTypeFilter annotationTypeFilter = new AnnotationTypeFilter( @@ -177,48 +169,37 @@ class FeignClientsRegistrar final Class[] clients = attrs == null ? null : (Class[]) attrs.get("clients"); if (clients == null || clients.length == 0) { - scanner.addIncludeFilter(annotationTypeFilter); - basePackages = getBasePackages(metadata); + ClassPathScanningCandidateComponentProvider scanner = getScanner(); + scanner.setResourceLoader(this.resourceLoader); + scanner.addIncludeFilter(new AnnotationTypeFilter(FeignClient.class)); + Set basePackages = getBasePackages(metadata); + for (String basePackage : basePackages) { + candidateComponents.addAll(scanner.findCandidateComponents(basePackage)); + } } else { - final Set clientClasses = new HashSet<>(); - basePackages = new HashSet<>(); for (Class clazz : clients) { - basePackages.add(ClassUtils.getPackageName(clazz)); - clientClasses.add(clazz.getCanonicalName()); + candidateComponents.add(new AnnotatedGenericBeanDefinition(clazz)); } - AbstractClassTestingTypeFilter filter = new AbstractClassTestingTypeFilter() { - @Override - protected boolean match(ClassMetadata metadata) { - String cleaned = metadata.getClassName().replaceAll("\\$", "."); - return clientClasses.contains(cleaned); - } - }; - scanner.addIncludeFilter( - new AllTypeFilter(Arrays.asList(filter, annotationTypeFilter))); } - for (String basePackage : basePackages) { - Set candidateComponents = scanner - .findCandidateComponents(basePackage); - for (BeanDefinition candidateComponent : candidateComponents) { - if (candidateComponent instanceof AnnotatedBeanDefinition) { - // verify annotated class is an interface - AnnotatedBeanDefinition beanDefinition = (AnnotatedBeanDefinition) candidateComponent; - AnnotationMetadata annotationMetadata = beanDefinition.getMetadata(); - Assert.isTrue(annotationMetadata.isInterface(), - "@FeignClient can only be specified on an interface"); + for (BeanDefinition candidateComponent : candidateComponents) { + if (candidateComponent instanceof AnnotatedBeanDefinition) { + // verify annotated class is an interface + AnnotatedBeanDefinition beanDefinition = (AnnotatedBeanDefinition) candidateComponent; + AnnotationMetadata annotationMetadata = beanDefinition.getMetadata(); + Assert.isTrue(annotationMetadata.isInterface(), + "@FeignClient can only be specified on an interface"); - Map attributes = annotationMetadata - .getAnnotationAttributes( - FeignClient.class.getCanonicalName()); + Map attributes = annotationMetadata + .getAnnotationAttributes( + FeignClient.class.getCanonicalName()); - String name = getClientName(attributes); - registerClientConfiguration(registry, name, - attributes.get("configuration")); + String name = getClientName(attributes); + registerClientConfiguration(registry, name, + attributes.get("configuration")); - registerFeignClient(registry, annotationMetadata, attributes); - } + registerFeignClient(registry, annotationMetadata, attributes); } } } @@ -398,38 +379,4 @@ class FeignClientsRegistrar this.environment = environment; } - /** - * Helper class to create a {@link TypeFilter} that matches if all the delegates - * match. - * - * @author Oliver Gierke - */ - private static class AllTypeFilter implements TypeFilter { - - private final List delegates; - - /** - * Creates a new {@link AllTypeFilter} to match if all the given delegates match. - * @param delegates must not be {@literal null}. - */ - AllTypeFilter(List delegates) { - Assert.notNull(delegates, "This argument is required, it must not be null"); - this.delegates = delegates; - } - - @Override - public boolean match(MetadataReader metadataReader, - MetadataReaderFactory metadataReaderFactory) throws IOException { - - for (TypeFilter filter : this.delegates) { - if (!filter.match(metadataReader, metadataReaderFactory)) { - return false; - } - } - - return true; - } - - } - } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientsRegistrarTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientsRegistrarTests.java index 26d643a1..b9c924dc 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientsRegistrarTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientsRegistrarTests.java @@ -20,7 +20,9 @@ import java.util.Collections; import org.junit.Test; +import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.cloud.openfeign.test.TestAutoConfiguration; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Configuration; import org.springframework.mock.env.MockEnvironment; @@ -28,10 +30,12 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; /** * @author Spencer Gibb * @author Gang Li + * @author Michal Domagala */ public class FeignClientsRegistrarTests { @@ -89,6 +93,17 @@ public class FeignClientsRegistrarTests { new AnnotationConfigApplicationContext(FallbackFactoryTestConfig.class); } + @Test + public void shouldPassSubLevelFeignClient() { + AnnotationConfigApplicationContext config = new AnnotationConfigApplicationContext(); + ((DefaultListableBeanFactory) config.getBeanFactory()).setAllowBeanDefinitionOverriding(false); + config.register(TopLevelSubLevelTestConfig.class); + assertThatCode(() -> config.refresh()) + .as("Case https://github.com/spring-cloud/spring-cloud-openfeign/issues/331 should be solved") + .doesNotThrowAnyException(); + + } + @FeignClient(name = "fallbackTestClient", url = "http://localhost:8080/", fallback = FallbackClient.class) protected interface FallbackClient { @@ -122,4 +137,11 @@ public class FeignClientsRegistrarTests { } + @EnableFeignClients(clients = { + org.springframework.cloud.openfeign.feignclientsregistrar.TopLevelClient.class, + org.springframework.cloud.openfeign.feignclientsregistrar.sub.SubLevelClient.class}) + @EnableAutoConfiguration(exclude = TestAutoConfiguration.class) + protected static class TopLevelSubLevelTestConfig { + } + } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/feignclientsregistrar/TopLevelClient.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/feignclientsregistrar/TopLevelClient.java new file mode 100644 index 00000000..2128bce4 --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/feignclientsregistrar/TopLevelClient.java @@ -0,0 +1,27 @@ +/* + * Copyright 2013-2020 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 + * + * https://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.cloud.openfeign.feignclientsregistrar; + +import org.springframework.cloud.openfeign.FeignClient; + +/** + * @author Michal Domagala + */ + +@FeignClient("top-level") +public interface TopLevelClient { +} diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/feignclientsregistrar/sub/SubLevelClient.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/feignclientsregistrar/sub/SubLevelClient.java new file mode 100644 index 00000000..84d01aea --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/feignclientsregistrar/sub/SubLevelClient.java @@ -0,0 +1,27 @@ +/* + * Copyright 2013-2020 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 + * + * https://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.cloud.openfeign.feignclientsregistrar.sub; + +import org.springframework.cloud.openfeign.FeignClient; + +/** + * @author Michal Domagala + */ + +@FeignClient("sub-level") +public interface SubLevelClient { +}