Fix circuit breaker Feign builder direct usage

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);
 		}
 

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<Object> supplier = asSupplier(method, args);
 		if (this.nullableFallbackFactory != null) {
 			Function<Throwable, Object> fallbackFunction = throwable -> {

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;
+		}
+
+	}
+
+}

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> 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();
 		}
 
 	}

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<DirectCircuitBreakerClient> 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<DirectCircuitBreakerClient> 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<DirectCircuitBreakerClient> 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.<Supplier<?>>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