diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignAutoConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignAutoConfiguration.java index cb6b1f413..56f28cde9 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignAutoConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignAutoConfiguration.java @@ -86,7 +86,7 @@ @Configuration(proxyBeanMethods = false) @ConditionalOnClass(Feign.class) @EnableConfigurationProperties({ FeignClientProperties.class, FeignHttpClientProperties.class, - FeignEncoderProperties.class, FeignOAuth2Properties.class }) + FeignEncoderProperties.class, FeignOAuth2Properties.class, FeignCircuitBreakerProperties.class }) public class FeignAutoConfiguration { private static final Log LOG = LogFactory.getLog(FeignAutoConfiguration.class); @@ -184,14 +184,27 @@ public CircuitBreakerNameResolver alphanumericCircuitBreakerNameResolver() { return new AlphanumericCircuitBreakerNameResolver(); } + /** + * @deprecated in favor of + * {@link #circuitBreakerFeignTargeter(CircuitBreakerFactory, FeignCircuitBreakerProperties, CircuitBreakerNameResolver)}. + */ + @Deprecated + @SuppressWarnings("rawtypes") + public Targeter circuitBreakerFeignTargeter(CircuitBreakerFactory circuitBreakerFactory, + @Value("${spring.cloud.openfeign.circuitbreaker.group.enabled:false}") boolean circuitBreakerGroupEnabled, + CircuitBreakerNameResolver circuitBreakerNameResolver) { + return new FeignCircuitBreakerTargeter(circuitBreakerFactory, circuitBreakerGroupEnabled, + circuitBreakerNameResolver); + } + @SuppressWarnings("rawtypes") @Bean @ConditionalOnMissingBean @ConditionalOnBean(CircuitBreakerFactory.class) public Targeter circuitBreakerFeignTargeter(CircuitBreakerFactory circuitBreakerFactory, - @Value("${spring.cloud.openfeign.circuitbreaker.group.enabled:false}") boolean circuitBreakerGroupEnabled, + FeignCircuitBreakerProperties circuitBreakerProperties, CircuitBreakerNameResolver circuitBreakerNameResolver) { - return new FeignCircuitBreakerTargeter(circuitBreakerFactory, circuitBreakerGroupEnabled, + return circuitBreakerFeignTargeter(circuitBreakerFactory, circuitBreakerProperties.getGroup().isEnabled(), circuitBreakerNameResolver); } 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/FeignCircuitBreakerProperties.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerProperties.java new file mode 100644 index 000000000..7003a6a2c --- /dev/null +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerProperties.java @@ -0,0 +1,75 @@ +/* + * Copyright 2013-present 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; + +import org.springframework.boot.context.properties.ConfigurationProperties; + +/** + * Configuration properties for Feign circuit breaker support. + */ +@ConfigurationProperties("spring.cloud.openfeign.circuitbreaker") +public class FeignCircuitBreakerProperties { + + private Group group = new Group(); + + private AlphanumericIds alphanumericIds = new AlphanumericIds(); + + public Group getGroup() { + return group; + } + + public void setGroup(Group group) { + this.group = group; + } + + public AlphanumericIds getAlphanumericIds() { + return alphanumericIds; + } + + public void setAlphanumericIds(AlphanumericIds alphanumericIds) { + this.alphanumericIds = alphanumericIds; + } + + public static class Group { + + private boolean enabled; + + public boolean isEnabled() { + return enabled; + } + + public void setEnabled(boolean enabled) { + this.enabled = enabled; + } + + } + + public static class AlphanumericIds { + + private boolean enabled = true; + + public boolean isEnabled() { + return enabled; + } + + public void setEnabled(boolean enabled) { + this.enabled = enabled; + } + + } + +} 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..ae7792bd0 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 @@ -45,6 +45,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; @@ -244,12 +246,35 @@ public Feign.Builder defaultFeignBuilder(Retryer retryer) { return Feign.builder().retryer(retryer); } + /** + * @deprecated in favor of + * {@link #circuitBreakerFeignBuilder(CircuitBreakerFactory, FeignCircuitBreakerProperties, ObjectProvider)}. + */ + @Deprecated + public Feign.Builder circuitBreakerFeignBuilder() { + return FeignCircuitBreaker.builder(); + } + @Bean @Scope("prototype") @ConditionalOnMissingBean @ConditionalOnBean(CircuitBreakerFactory.class) - public Feign.Builder circuitBreakerFeignBuilder() { - return FeignCircuitBreaker.builder(); + public Feign.Builder circuitBreakerFeignBuilder(CircuitBreakerFactory circuitBreakerFactory, + FeignCircuitBreakerProperties circuitBreakerProperties, + ObjectProvider circuitBreakerNameResolver) { + return FeignCircuitBreaker.builder() + .circuitBreakerFactory(circuitBreakerFactory) + .circuitBreakerGroupEnabled(circuitBreakerProperties.getGroup().isEnabled()) + .circuitBreakerNameResolver(circuitBreakerNameResolver + .getIfAvailable(() -> defaultCircuitBreakerNameResolver(circuitBreakerProperties))); + } + + private CircuitBreakerNameResolver defaultCircuitBreakerNameResolver( + FeignCircuitBreakerProperties circuitBreakerProperties) { + if (circuitBreakerProperties.getAlphanumericIds().isEnabled()) { + 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..6ae23257d 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,23 +17,36 @@ 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; +import org.springframework.beans.factory.ObjectProvider; 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.context.annotation.Bean; 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 +104,130 @@ void shouldInstantiateFeignCircuitBreakerTargeterWhenEnabledWithCustomCircuitBre }); } + @Test + void shouldKeepNoArgCircuitBreakerFeignBuilderSignature() throws NoSuchMethodException { + Method noArgMethod = FeignClientsConfiguration.CircuitBreakerPresentFeignBuilderConfiguration.class + .getDeclaredMethod("circuitBreakerFeignBuilder"); + Method beanMethod = FeignClientsConfiguration.CircuitBreakerPresentFeignBuilderConfiguration.class + .getDeclaredMethod("circuitBreakerFeignBuilder", CircuitBreakerFactory.class, + FeignCircuitBreakerProperties.class, ObjectProvider.class); + + assertThat(Modifier.isPublic(noArgMethod.getModifiers())).isTrue(); + assertThat(noArgMethod.getReturnType()).isEqualTo(Feign.Builder.class); + assertThat(noArgMethod.isAnnotationPresent(Deprecated.class)).isTrue(); + assertThat(beanMethod.isAnnotationPresent(Bean.class)).isTrue(); + } + + @Test + void shouldKeepCircuitBreakerFeignTargeterSignature() throws NoSuchMethodException { + Method existingMethod = FeignAutoConfiguration.CircuitBreakerPresentFeignTargeterConfiguration.class + .getDeclaredMethod("circuitBreakerFeignTargeter", CircuitBreakerFactory.class, boolean.class, + CircuitBreakerNameResolver.class); + Method beanMethod = FeignAutoConfiguration.CircuitBreakerPresentFeignTargeterConfiguration.class + .getDeclaredMethod("circuitBreakerFeignTargeter", CircuitBreakerFactory.class, + FeignCircuitBreakerProperties.class, CircuitBreakerNameResolver.class); + + assertThat(Modifier.isPublic(existingMethod.getModifiers())).isTrue(); + assertThat(existingMethod.getReturnType()).isEqualTo(Targeter.class); + assertThat(existingMethod.isAnnotationPresent(Deprecated.class)).isTrue(); + assertThat(existingMethod.isAnnotationPresent(Bean.class)).isFalse(); + assertThat(beanMethod.isAnnotationPresent(Bean.class)).isTrue(); + } + + @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(FeignAutoConfiguration.class, 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(FeignAutoConfiguration.class, 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 +284,12 @@ private void assertThatFeignCircuitBreakerTargeterHasSameCircuitBreakerNameResol assertThat(bean).isExactlyInstanceOf(beanClass); } + interface DirectCircuitBreakerClient { + + String get(); + + } + static class CustomCircuitBreakerNameResolver implements CircuitBreakerNameResolver { @Override