Use delegate in RetryableFeignLoadBalancer when url present. Fixes gh-494.

spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2013-2020 the original author or authors.
+ * Copyright 2013-2021 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.
@@ -45,6 +45,7 @@ import org.springframework.beans.factory.InitializingBean;
 import org.springframework.beans.factory.NoSuchBeanDefinitionException;
 import org.springframework.cloud.openfeign.clientconfig.FeignClientConfigurer;
 import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient;
+import org.springframework.cloud.openfeign.loadbalancer.RetryableFeignBlockingLoadBalancerClient;
 import org.springframework.cloud.openfeign.ribbon.LoadBalancerFeignClient;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
@@ -385,6 +386,12 @@ public class FeignClientFactoryBean implements FactoryBean<Object>, Initializing
 				// but Spring Cloud LoadBalancer is on the classpath, so unwrap
 				client = ((FeignBlockingLoadBalancerClient) client).getDelegate();
 			}
+			if (client instanceof RetryableFeignBlockingLoadBalancerClient) {
+				// not load balancing because we have a url,
+				// but Spring Cloud LoadBalancer is on the classpath, so unwrap
+				client = ((RetryableFeignBlockingLoadBalancerClient) client)
+						.getDelegate();
+			}
 			builder.client(client);
 		}
 		Targeter targeter = get(context, Targeter.class);

spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignHttpClientUrlTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2013-2020 the original author or authors.
+ * Copyright 2013-2021 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.
@@ -23,10 +23,9 @@ import feign.Client;
 import feign.Feign;
 import feign.Target;
 import feign.httpclient.ApacheHttpClient;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
 
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
@@ -36,7 +35,6 @@ import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
 import org.springframework.test.annotation.DirtiesContext;
-import org.springframework.test.context.junit4.SpringRunner;
 import org.springframework.util.ReflectionUtils;
 import org.springframework.util.SocketUtils;
 import org.springframework.web.bind.annotation.RequestMapping;
@@ -48,14 +46,14 @@ import static org.springframework.boot.test.context.SpringBootTest.WebEnvironmen
 
 /**
  * @author Spencer Gibb
+ * @author Olga Maciaszek-Sharma
  */
-@RunWith(SpringRunner.class)
 @SpringBootTest(classes = FeignHttpClientUrlTests.TestConfig.class,
 		webEnvironment = DEFINED_PORT,
 		value = { "spring.application.name=feignclienturltest",
 				"feign.hystrix.enabled=false", "feign.okhttp.enabled=false" })
 @DirtiesContext
-public class FeignHttpClientUrlTests {
+class FeignHttpClientUrlTests {
 
 	static int port;
 
@@ -68,19 +66,19 @@ public class FeignHttpClientUrlTests {
 	@Autowired
 	private BeanUrlClient beanClient;
 
-	@BeforeClass
-	public static void beforeClass() {
+	@BeforeAll
+	static void beforeClass() {
 		port = SocketUtils.findAvailableTcpPort();
 		System.setProperty("server.port", String.valueOf(port));
 	}
 
-	@AfterClass
-	public static void afterClass() {
+	@AfterAll
+	static void afterClass() {
 		System.clearProperty("server.port");
 	}
 
 	@Test
-	public void testUrlHttpClient() {
+	void testUrlHttpClient() {
 		assertThat(this.urlClient).as("UrlClient was null").isNotNull();
 		Hello hello = this.urlClient.getHello();
 		assertThat(hello).as("hello was null").isNotNull();
@@ -89,7 +87,7 @@ public class FeignHttpClientUrlTests {
 	}
 
 	@Test
-	public void testBeanUrl() {
+	void testBeanUrl() {
 		Hello hello = this.beanClient.getHello();
 		assertThat(hello).as("hello was null").isNotNull();
 		assertThat(hello).as("first hello didn't match")
@@ -97,7 +95,7 @@ public class FeignHttpClientUrlTests {
 	}
 
 	@Test
-	public void testBeanUrlNoProtocol() {
+	void testBeanUrlNoProtocol() {
 		Hello hello = this.beanClientNoProtocol.getHello();
 		assertThat(hello).as("hello was null").isNotNull();
 		assertThat(hello).as("first hello didn't match")
@@ -183,6 +181,7 @@ public class FeignHttpClientUrlTests {
 		private String message;
 
 		public Hello() {
+
 		}
 
 		public Hello(String message) {

spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignHttpClientUrlTestsWithLoadBalancer.java

@@ -0,0 +1,221 @@
+/*
+ * Copyright 2013-2021 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 java.lang.reflect.Field;
+import java.util.Objects;
+
+import feign.Client;
+import feign.Feign;
+import feign.Target;
+import feign.httpclient.ApacheHttpClient;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.cloud.openfeign.test.NoSecurityConfiguration;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Import;
+import org.springframework.test.annotation.DirtiesContext;
+import org.springframework.util.ReflectionUtils;
+import org.springframework.util.SocketUtils;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.bind.annotation.RequestMethod;
+import org.springframework.web.bind.annotation.RestController;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.DEFINED_PORT;
+
+/**
+ * @author Spencer Gibb
+ * @author Olga Maciaszek-Sharma
+ */
+@SpringBootTest(classes = FeignHttpClientUrlTestsWithLoadBalancer.TestConfig.class,
+		webEnvironment = DEFINED_PORT,
+		value = { "spring.application.name=feignclienturlwithloadbalancertest",
+				"feign.hystrix.enabled=false", "feign.okhttp.enabled=false",
+				"spring.cloud.loadbalancer.ribbon.enabled=false",
+				"spring.cloud.loadbalancer.retry.enabled=false" })
+@DirtiesContext
+class FeignHttpClientUrlTestsWithLoadBalancer {
+
+	static int port;
+
+	@Autowired
+	BeanUrlClientNoProtocol beanClientNoProtocol;
+
+	@Autowired
+	private UrlClient urlClient;
+
+	@Autowired
+	private BeanUrlClient beanClient;
+
+	@BeforeAll
+	static void beforeClass() {
+		port = SocketUtils.findAvailableTcpPort();
+		System.setProperty("server.port", String.valueOf(port));
+	}
+
+	@AfterAll
+	static void afterClass() {
+		System.clearProperty("server.port");
+	}
+
+	@Test
+	void testUrlHttpClient() {
+		assertThat(this.urlClient).as("UrlClient was null").isNotNull();
+		Hello hello = this.urlClient.getHello();
+		assertThat(hello).as("hello was null").isNotNull();
+		assertThat(hello).as("first hello didn't match")
+				.isEqualTo(new Hello("hello world 1"));
+	}
+
+	@Test
+	void testBeanUrl() {
+		Hello hello = this.beanClient.getHello();
+		assertThat(hello).as("hello was null").isNotNull();
+		assertThat(hello).as("first hello didn't match")
+				.isEqualTo(new Hello("hello world 1"));
+	}
+
+	@Test
+	void testBeanUrlNoProtocol() {
+		Hello hello = this.beanClientNoProtocol.getHello();
+		assertThat(hello).as("hello was null").isNotNull();
+		assertThat(hello).as("first hello didn't match")
+				.isEqualTo(new Hello("hello world 1"));
+	}
+
+	// this tests that
+	@FeignClient(name = "localappurl", url = "http://localhost:${server.port}/")
+	protected interface UrlClient {
+
+		@RequestMapping(method = RequestMethod.GET, value = "/hello")
+		Hello getHello();
+
+	}
+
+	@FeignClient(name = "beanappurl", url = "#{SERVER_URL}path")
+	protected interface BeanUrlClient {
+
+		@RequestMapping(method = RequestMethod.GET, value = "/hello")
+		Hello getHello();
+
+	}
+
+	@FeignClient(name = "beanappurlnoprotocol", url = "#{SERVER_URL_NO_PROTOCOL}path")
+	protected interface BeanUrlClientNoProtocol {
+
+		@RequestMapping(method = RequestMethod.GET, value = "/hello")
+		Hello getHello();
+
+	}
+
+	@Configuration(proxyBeanMethods = false)
+	@EnableAutoConfiguration
+	@RestController
+	@EnableFeignClients(clients = { UrlClient.class, BeanUrlClient.class,
+			BeanUrlClientNoProtocol.class })
+	@Import(NoSecurityConfiguration.class)
+	protected static class TestConfig {
+
+		@GetMapping("/hello")
+		public Hello getHello() {
+			return new Hello("hello world 1");
+		}
+
+		@GetMapping("/path/hello")
+		public Hello getHelloWithPath() {
+			return getHello();
+		}
+
+		@Bean(name = "SERVER_URL")
+		public String serverUrl() {
+			return "http://localhost:" + port + "/";
+		}
+
+		@Bean(name = "SERVER_URL_NO_PROTOCOL")
+		public String serverUrlNoProtocol() {
+			return "localhost:" + port + "/";
+		}
+
+		@Bean
+		public Targeter feignTargeter() {
+			return new Targeter() {
+				@Override
+				public <T> T target(FeignClientFactoryBean factory, Feign.Builder feign,
+						FeignContext context, Target.HardCodedTarget<T> target) {
+					Field field = ReflectionUtils.findField(Feign.Builder.class,
+							"client");
+					ReflectionUtils.makeAccessible(field);
+					Client client = (Client) ReflectionUtils.getField(field, feign);
+					if (target.name().equals("localappurl")) {
+						assertThat(client).isInstanceOf(ApacheHttpClient.class)
+								.as("client was wrong type");
+					}
+					return feign.target(target);
+				}
+			};
+		}
+
+	}
+
+	public static class Hello {
+
+		private String message;
+
+		public Hello() {
+
+		}
+
+		public Hello(String message) {
+			this.message = message;
+		}
+
+		public String getMessage() {
+			return this.message;
+		}
+
+		public void setMessage(String message) {
+			this.message = message;
+		}
+
+		@Override
+		public boolean equals(Object o) {
+			if (this == o) {
+				return true;
+			}
+			if (o == null || getClass() != o.getClass()) {
+				return false;
+			}
+			Hello that = (Hello) o;
+			return Objects.equals(this.message, that.message);
+		}
+
+		@Override
+		public int hashCode() {
+			return Objects.hash(this.message);
+		}
+
+	}
+
+}

spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignHttpClientUrlTestsWithRetryableLoadBalancer.java

@@ -0,0 +1,221 @@
+/*
+ * Copyright 2013-2021 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 java.lang.reflect.Field;
+import java.util.Objects;
+
+import feign.Client;
+import feign.Feign;
+import feign.Target;
+import feign.httpclient.ApacheHttpClient;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.cloud.openfeign.test.NoSecurityConfiguration;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Import;
+import org.springframework.test.annotation.DirtiesContext;
+import org.springframework.util.ReflectionUtils;
+import org.springframework.util.SocketUtils;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.bind.annotation.RequestMethod;
+import org.springframework.web.bind.annotation.RestController;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.DEFINED_PORT;
+
+/**
+ * @author Spencer Gibb
+ * @author Olga Maciaszek-Sharma
+ */
+@SpringBootTest(
+		classes = FeignHttpClientUrlTestsWithRetryableLoadBalancer.TestConfig.class,
+		webEnvironment = DEFINED_PORT,
+		value = { "spring.application.name=feignclienturlwithretryableloadbalancertest",
+				"feign.hystrix.enabled=false", "feign.okhttp.enabled=false",
+				"spring.cloud.loadbalancer.ribbon.enabled=false" })
+@DirtiesContext
+class FeignHttpClientUrlTestsWithRetryableLoadBalancer {
+
+	static int port;
+
+	@Autowired
+	BeanUrlClientNoProtocol beanClientNoProtocol;
+
+	@Autowired
+	private UrlClient urlClient;
+
+	@Autowired
+	private BeanUrlClient beanClient;
+
+	@BeforeAll
+	static void beforeClass() {
+		port = SocketUtils.findAvailableTcpPort();
+		System.setProperty("server.port", String.valueOf(port));
+	}
+
+	@AfterAll
+	static void afterClass() {
+		System.clearProperty("server.port");
+	}
+
+	@Test
+	void testUrlHttpClient() {
+		assertThat(this.urlClient).as("UrlClient was null").isNotNull();
+		Hello hello = this.urlClient.getHello();
+		assertThat(hello).as("hello was null").isNotNull();
+		assertThat(hello).as("first hello didn't match")
+				.isEqualTo(new Hello("hello world 1"));
+	}
+
+	@Test
+	void testBeanUrl() {
+		Hello hello = this.beanClient.getHello();
+		assertThat(hello).as("hello was null").isNotNull();
+		assertThat(hello).as("first hello didn't match")
+				.isEqualTo(new Hello("hello world 1"));
+	}
+
+	@Test
+	void testBeanUrlNoProtocol() {
+		Hello hello = this.beanClientNoProtocol.getHello();
+		assertThat(hello).as("hello was null").isNotNull();
+		assertThat(hello).as("first hello didn't match")
+				.isEqualTo(new Hello("hello world 1"));
+	}
+
+	// this tests that
+	@FeignClient(name = "localappurl", url = "http://localhost:${server.port}/")
+	protected interface UrlClient {
+
+		@RequestMapping(method = RequestMethod.GET, value = "/hello")
+		Hello getHello();
+
+	}
+
+	@FeignClient(name = "beanappurl", url = "#{SERVER_URL}path")
+	protected interface BeanUrlClient {
+
+		@RequestMapping(method = RequestMethod.GET, value = "/hello")
+		Hello getHello();
+
+	}
+
+	@FeignClient(name = "beanappurlnoprotocol", url = "#{SERVER_URL_NO_PROTOCOL}path")
+	protected interface BeanUrlClientNoProtocol {
+
+		@RequestMapping(method = RequestMethod.GET, value = "/hello")
+		Hello getHello();
+
+	}
+
+	@Configuration(proxyBeanMethods = false)
+	@EnableAutoConfiguration
+	@RestController
+	@EnableFeignClients(clients = { UrlClient.class, BeanUrlClient.class,
+			BeanUrlClientNoProtocol.class })
+	@Import(NoSecurityConfiguration.class)
+	protected static class TestConfig {
+
+		@GetMapping("/hello")
+		public Hello getHello() {
+			return new Hello("hello world 1");
+		}
+
+		@GetMapping("/path/hello")
+		public Hello getHelloWithPath() {
+			return getHello();
+		}
+
+		@Bean(name = "SERVER_URL")
+		public String serverUrl() {
+			return "http://localhost:" + port + "/";
+		}
+
+		@Bean(name = "SERVER_URL_NO_PROTOCOL")
+		public String serverUrlNoProtocol() {
+			return "localhost:" + port + "/";
+		}
+
+		@Bean
+		public Targeter feignTargeter() {
+			return new Targeter() {
+				@Override
+				public <T> T target(FeignClientFactoryBean factory, Feign.Builder feign,
+						FeignContext context, Target.HardCodedTarget<T> target) {
+					Field field = ReflectionUtils.findField(Feign.Builder.class,
+							"client");
+					ReflectionUtils.makeAccessible(field);
+					Client client = (Client) ReflectionUtils.getField(field, feign);
+					if (target.name().equals("localappurl")) {
+						assertThat(client).isInstanceOf(ApacheHttpClient.class)
+								.as("client was wrong type");
+					}
+					return feign.target(target);
+				}
+			};
+		}
+
+	}
+
+	public static class Hello {
+
+		private String message;
+
+		public Hello() {
+
+		}
+
+		public Hello(String message) {
+			this.message = message;
+		}
+
+		public String getMessage() {
+			return this.message;
+		}
+
+		public void setMessage(String message) {
+			this.message = message;
+		}
+
+		@Override
+		public boolean equals(Object o) {
+			if (this == o) {
+				return true;
+			}
+			if (o == null || getClass() != o.getClass()) {
+				return false;
+			}
+			Hello that = (Hello) o;
+			return Objects.equals(this.message, that.message);
+		}
+
+		@Override
+		public int hashCode() {
+			return Objects.hash(this.message);
+		}
+
+	}
+
+}

src/checkstyle/checkstyle-suppressions.xml

@@ -16,6 +16,7 @@
 	<suppress files=".*ProtobufSpringEncoderTest.*" checks="LineLengthCheck"/>
 	<suppress files=".*ProtobufTest.*" checks="LineLengthCheck"/>
 	<suppress files=".*Hello.*" checks="RedundantModifier"/>
+	<suppress files=".*FeignHttp.*" checks="RedundantModifier"/>
 	<!-- Important -->
 	<suppress files=".*HttpEncoding.*" checks="InterfaceIsTypeCheck"/>
 </suppressions>