diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java index 6acac2c0d..b58304efa 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java @@ -95,9 +95,11 @@ else if ("toString".equals(method.getName())) { return toString(); } - String circuitName = circuitBreakerNameResolver.resolveCircuitBreakerName(feignClientName, target, method); - CircuitBreaker circuitBreaker = circuitBreakerGroupEnabled ? factory.create(circuitName, feignClientName) - : factory.create(circuitName); + String resolvedFeignClientName = feignClientName != null ? feignClientName : target.name(); + String circuitName = circuitBreakerNameResolver.resolveCircuitBreakerName(resolvedFeignClientName, target, + method); + CircuitBreaker circuitBreaker = circuitBreakerGroupEnabled + ? factory.create(circuitName, resolvedFeignClientName) : factory.create(circuitName); Supplier supplier = asSupplier(method, args); if (this.nullableFallbackFactory != null) { Function fallbackFunction = throwable -> { diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java index f85e59b30..6f8e2557c 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java @@ -36,6 +36,7 @@ import org.springframework.beans.factory.ObjectProvider; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; @@ -45,6 +46,8 @@ import org.springframework.boot.http.converter.autoconfigure.ClientHttpMessageConvertersCustomizer; import org.springframework.cloud.client.circuitbreaker.CircuitBreaker; import org.springframework.cloud.client.circuitbreaker.CircuitBreakerFactory; +import org.springframework.cloud.openfeign.FeignAutoConfiguration.CircuitBreakerPresentFeignTargeterConfiguration.AlphanumericCircuitBreakerNameResolver; +import org.springframework.cloud.openfeign.FeignAutoConfiguration.CircuitBreakerPresentFeignTargeterConfiguration.DefaultCircuitBreakerNameResolver; import org.springframework.cloud.openfeign.clientconfig.FeignClientConfigurer; import org.springframework.cloud.openfeign.support.AbstractFormWriter; import org.springframework.cloud.openfeign.support.FeignEncoderProperties; @@ -237,6 +240,18 @@ public Feign.Builder feignBuilder(Retryer retryer) { @ConditionalOnProperty("spring.cloud.openfeign.circuitbreaker.enabled") protected static class CircuitBreakerPresentFeignBuilderConfiguration { + @Autowired + private CircuitBreakerFactory circuitBreakerFactory; + + @Value("${spring.cloud.openfeign.circuitbreaker.group.enabled:false}") + private boolean circuitBreakerGroupEnabled; + + @Value("${spring.cloud.openfeign.circuitbreaker.alphanumeric-ids.enabled:true}") + private boolean alphanumericIdsEnabled; + + @Autowired + private ObjectProvider circuitBreakerNameResolver; + @Bean @Scope("prototype") @ConditionalOnMissingBean({ Feign.Builder.class, CircuitBreakerFactory.class }) @@ -249,7 +264,25 @@ public Feign.Builder defaultFeignBuilder(Retryer retryer) { @ConditionalOnMissingBean @ConditionalOnBean(CircuitBreakerFactory.class) public Feign.Builder circuitBreakerFeignBuilder() { - return FeignCircuitBreaker.builder(); + return circuitBreakerFeignBuilder(circuitBreakerFactory, circuitBreakerGroupEnabled, alphanumericIdsEnabled, + circuitBreakerNameResolver); + } + + public Feign.Builder circuitBreakerFeignBuilder(CircuitBreakerFactory circuitBreakerFactory, + boolean circuitBreakerGroupEnabled, boolean alphanumericIdsEnabled, + ObjectProvider circuitBreakerNameResolver) { + return FeignCircuitBreaker.builder() + .circuitBreakerFactory(circuitBreakerFactory) + .circuitBreakerGroupEnabled(circuitBreakerGroupEnabled) + .circuitBreakerNameResolver(circuitBreakerNameResolver + .getIfAvailable(() -> defaultCircuitBreakerNameResolver(alphanumericIdsEnabled))); + } + + private CircuitBreakerNameResolver defaultCircuitBreakerNameResolver(boolean alphanumericIdsEnabled) { + if (alphanumericIdsEnabled) { + return new AlphanumericCircuitBreakerNameResolver(); + } + return new DefaultCircuitBreakerNameResolver(); } } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignAutoConfigurationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignAutoConfigurationTests.java index 705a4153b..63158692a 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignAutoConfigurationTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignAutoConfigurationTests.java @@ -17,7 +17,12 @@ package org.springframework.cloud.openfeign; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Map; +import java.util.function.Supplier; +import feign.Feign; +import feign.InvocationHandlerFactory; import feign.Target; import org.assertj.core.api.Condition; import org.junit.jupiter.api.Test; @@ -25,15 +30,21 @@ import org.springframework.boot.autoconfigure.AutoConfigurations; import org.springframework.boot.test.context.assertj.AssertableApplicationContext; import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.cloud.client.circuitbreaker.CircuitBreaker; import org.springframework.cloud.client.circuitbreaker.CircuitBreakerFactory; import org.springframework.cloud.openfeign.FeignAutoConfiguration.CircuitBreakerPresentFeignTargeterConfiguration.AlphanumericCircuitBreakerNameResolver; +import org.springframework.cloud.openfeign.FeignAutoConfiguration.CircuitBreakerPresentFeignTargeterConfiguration.DefaultCircuitBreakerNameResolver; import org.springframework.cloud.openfeign.security.OAuth2AccessTokenInterceptor; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.security.oauth2.client.OAuth2AuthorizedClientService; import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; +import org.springframework.test.util.ReflectionTestUtils; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** * @author Tim Peeters @@ -91,6 +102,107 @@ void shouldInstantiateFeignCircuitBreakerTargeterWhenEnabledWithCustomCircuitBre }); } + @Test + void shouldKeepNoArgCircuitBreakerFeignBuilderSignature() throws NoSuchMethodException { + Method method = FeignClientsConfiguration.CircuitBreakerPresentFeignBuilderConfiguration.class + .getDeclaredMethod("circuitBreakerFeignBuilder"); + + assertThat(Modifier.isPublic(method.getModifiers())).isTrue(); + assertThat(method.getReturnType()).isEqualTo(Feign.Builder.class); + } + + @Test + void shouldConfigureCircuitBreakerFeignBuilderWhenUsedDirectly() { + CircuitBreakerFactory circuitBreakerFactory = mock(CircuitBreakerFactory.class); + new ApplicationContextRunner() + .withConfiguration(AutoConfigurations.of(FeignAutoConfiguration.class, FeignClientsConfiguration.class)) + .withPropertyValues("spring.cloud.openfeign.circuitbreaker.enabled=true", + "spring.cloud.openfeign.circuitbreaker.group.enabled=true", + "spring.cloud.openfeign.httpclient.hc5.enabled=false") + .withBean(CircuitBreakerFactory.class, () -> circuitBreakerFactory) + .run(ctx -> { + CircuitBreakerNameResolver circuitBreakerNameResolver = ctx.getBean(CircuitBreakerNameResolver.class); + Feign.Builder builder = ctx.getBean(Feign.Builder.class); + + assertThat(builder).isInstanceOf(FeignCircuitBreaker.Builder.class) + .hasFieldOrPropertyWithValue("circuitBreakerFactory", circuitBreakerFactory) + .hasFieldOrPropertyWithValue("circuitBreakerGroupEnabled", true) + .hasFieldOrPropertyWithValue("circuitBreakerNameResolver", circuitBreakerNameResolver); + }); + } + + @Test + void shouldConfigureCircuitBreakerFeignBuilderWithoutNameResolverBean() throws NoSuchMethodException { + CircuitBreakerFactory circuitBreakerFactory = mock(CircuitBreakerFactory.class); + new ApplicationContextRunner().withConfiguration(AutoConfigurations.of(FeignClientsConfiguration.class)) + .withPropertyValues("spring.cloud.openfeign.circuitbreaker.enabled=true", + "spring.cloud.openfeign.httpclient.hc5.enabled=false") + .withBean(CircuitBreakerFactory.class, () -> circuitBreakerFactory) + .run(ctx -> { + Feign.Builder builder = ctx.getBean(Feign.Builder.class); + CircuitBreakerNameResolver circuitBreakerNameResolver = (CircuitBreakerNameResolver) ReflectionTestUtils + .getField(builder, "circuitBreakerNameResolver"); + Method method = DirectCircuitBreakerClient.class.getMethod("get"); + Target target = new Target.HardCodedTarget<>( + DirectCircuitBreakerClient.class, "directClient", "http://localhost"); + + assertThat(builder).isInstanceOf(FeignCircuitBreaker.Builder.class) + .hasFieldOrPropertyWithValue("circuitBreakerFactory", circuitBreakerFactory) + .hasFieldOrPropertyWithValue("circuitBreakerNameResolver", circuitBreakerNameResolver); + assertThat(circuitBreakerNameResolver) + .isExactlyInstanceOf(AlphanumericCircuitBreakerNameResolver.class); + assertThat(circuitBreakerNameResolver.resolveCircuitBreakerName("directClient", target, method)) + .isEqualTo(Feign.configKey(target.type(), method).replaceAll("[^a-zA-Z0-9]", "")); + }); + } + + @Test + void shouldConfigureDefaultCircuitBreakerFeignBuilderNameResolverWhenAlphanumericIdsDisabled() + throws NoSuchMethodException { + CircuitBreakerFactory circuitBreakerFactory = mock(CircuitBreakerFactory.class); + new ApplicationContextRunner().withConfiguration(AutoConfigurations.of(FeignClientsConfiguration.class)) + .withPropertyValues("spring.cloud.openfeign.circuitbreaker.enabled=true", + "spring.cloud.openfeign.circuitbreaker.alphanumeric-ids.enabled=false", + "spring.cloud.openfeign.httpclient.hc5.enabled=false") + .withBean(CircuitBreakerFactory.class, () -> circuitBreakerFactory) + .run(ctx -> { + Feign.Builder builder = ctx.getBean(Feign.Builder.class); + CircuitBreakerNameResolver circuitBreakerNameResolver = (CircuitBreakerNameResolver) ReflectionTestUtils + .getField(builder, "circuitBreakerNameResolver"); + Method method = DirectCircuitBreakerClient.class.getMethod("get"); + Target target = new Target.HardCodedTarget<>( + DirectCircuitBreakerClient.class, "directClient", "http://localhost"); + + assertThat(builder).isInstanceOf(FeignCircuitBreaker.Builder.class) + .hasFieldOrPropertyWithValue("circuitBreakerFactory", circuitBreakerFactory) + .hasFieldOrPropertyWithValue("circuitBreakerNameResolver", circuitBreakerNameResolver); + assertThat(circuitBreakerNameResolver).isExactlyInstanceOf(DefaultCircuitBreakerNameResolver.class); + assertThat(circuitBreakerNameResolver.resolveCircuitBreakerName("directClient", target, method)) + .isEqualTo(Feign.configKey(target.type(), method)); + }); + } + + @Test + void shouldUseTargetNameWhenFeignClientNameIsNotSet() throws Throwable { + CircuitBreakerFactory circuitBreakerFactory = mock(CircuitBreakerFactory.class); + CircuitBreaker circuitBreaker = mock(CircuitBreaker.class); + Method method = DirectCircuitBreakerClient.class.getMethod("get"); + InvocationHandlerFactory.MethodHandler methodHandler = mock(InvocationHandlerFactory.MethodHandler.class); + Target target = new Target.HardCodedTarget<>(DirectCircuitBreakerClient.class, + "directClient", "http://localhost"); + FeignCircuitBreakerInvocationHandler handler = new FeignCircuitBreakerInvocationHandler(circuitBreakerFactory, + null, target, Map.of(method, methodHandler), null, true, + (feignClientName, targetType, targetMethod) -> feignClientName + "#" + targetMethod.getName()); + + when(circuitBreakerFactory.create("directClient#get", "directClient")).thenReturn(circuitBreaker); + when(circuitBreaker.run(any(Supplier.class))) + .thenAnswer(invocation -> invocation.>getArgument(0).get()); + when(methodHandler.invoke(null)).thenReturn("ok"); + + assertThat(handler.invoke(null, method, null)).isEqualTo("ok"); + verify(circuitBreakerFactory).create("directClient#get", "directClient"); + } + @Test void shouldInstantiateFeignOAuth2FeignRequestInterceptorWithoutInterceptors() { runner @@ -147,6 +259,12 @@ private void assertThatFeignCircuitBreakerTargeterHasSameCircuitBreakerNameResol assertThat(bean).isExactlyInstanceOf(beanClass); } + interface DirectCircuitBreakerClient { + + String get(); + + } + static class CustomCircuitBreakerNameResolver implements CircuitBreakerNameResolver { @Override