diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java index ba17d806..6218afcf 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java @@ -18,6 +18,7 @@ package org.springframework.cloud.openfeign; import java.util.Map; import java.util.Objects; +import java.util.concurrent.TimeUnit; import feign.Client; import feign.Contract; @@ -52,6 +53,7 @@ import org.springframework.util.StringUtils; * @author Eko Kurniawan Khannedy * @author Gregor Zurowski * @author Matt King + * @author Olga Maciaszek-Sharma */ public class FeignClientFactoryBean implements FactoryBean, InitializingBean, ApplicationContextAware { @@ -81,15 +83,19 @@ public class FeignClientFactoryBean private Class fallbackFactory = void.class; + private int readTimeoutMillis = new Request.Options().readTimeoutMillis(); + + private int connectTimeoutMillis = new Request.Options().connectTimeoutMillis(); + @Override public void afterPropertiesSet() { - Assert.hasText(this.contextId, "Context id must be set"); - Assert.hasText(this.name, "Name must be set"); + Assert.hasText(contextId, "Context id must be set"); + Assert.hasText(name, "Name must be set"); } protected Feign.Builder feign(FeignContext context) { FeignLoggerFactory loggerFactory = get(context, FeignLoggerFactory.class); - Logger logger = loggerFactory.create(this.type); + Logger logger = loggerFactory.create(type); // @formatter:off Feign.Builder builder = get(context, Feign.Builder.class) @@ -119,7 +125,7 @@ public class FeignClientFactoryBean } protected void configureFeign(FeignContext context, Feign.Builder builder) { - FeignClientProperties properties = this.applicationContext + FeignClientProperties properties = applicationContext .getBean(FeignClientProperties.class); FeignClientConfigurer feignClientConfigurer = getOptional(context, @@ -132,15 +138,13 @@ public class FeignClientFactoryBean configureUsingProperties( properties.getConfig().get(properties.getDefaultConfig()), builder); - configureUsingProperties(properties.getConfig().get(this.contextId), - builder); + configureUsingProperties(properties.getConfig().get(contextId), builder); } else { configureUsingProperties( properties.getConfig().get(properties.getDefaultConfig()), builder); - configureUsingProperties(properties.getConfig().get(this.contextId), - builder); + configureUsingProperties(properties.getConfig().get(contextId), builder); configureUsingConfiguration(context, builder); } } @@ -168,7 +172,7 @@ public class FeignClientFactoryBean FeignErrorDecoderFactory errorDecoderFactory = getOptional(context, FeignErrorDecoderFactory.class); if (errorDecoderFactory != null) { - ErrorDecoder factoryErrorDecoder = errorDecoderFactory.create(this.type); + ErrorDecoder factoryErrorDecoder = errorDecoderFactory.create(type); builder.errorDecoder(factoryErrorDecoder); } } @@ -176,6 +180,8 @@ public class FeignClientFactoryBean Request.Options.class); if (options != null) { builder.options(options); + readTimeoutMillis = options.readTimeoutMillis(); + connectTimeoutMillis = options.connectTimeoutMillis(); } Map requestInterceptors = getInheritedAwareInstances( context, RequestInterceptor.class); @@ -187,7 +193,7 @@ public class FeignClientFactoryBean if (queryMapEncoder != null) { builder.queryMapEncoder(queryMapEncoder); } - if (this.decode404) { + if (decode404) { builder.decode404(); } ExceptionPropagationPolicy exceptionPropagationPolicy = getInheritedAwareOptional( @@ -208,10 +214,13 @@ public class FeignClientFactoryBean builder.logLevel(config.getLoggerLevel()); } - if (config.getConnectTimeout() != null && config.getReadTimeout() != null) { - builder.options(new Request.Options(config.getConnectTimeout(), - config.getReadTimeout())); - } + connectTimeoutMillis = config.getConnectTimeout() != null + ? config.getConnectTimeout() : connectTimeoutMillis; + readTimeoutMillis = config.getReadTimeout() != null ? config.getReadTimeout() + : readTimeoutMillis; + + builder.options(new Request.Options(connectTimeoutMillis, TimeUnit.MILLISECONDS, + readTimeoutMillis, TimeUnit.MILLISECONDS, true)); if (config.getRetryer() != null) { Retryer retryer = getOrInstantiate(config.getRetryer()); @@ -257,7 +266,7 @@ public class FeignClientFactoryBean private T getOrInstantiate(Class tClass) { try { - return this.applicationContext.getBean(tClass); + return applicationContext.getBean(tClass); } catch (NoSuchBeanDefinitionException e) { return BeanUtils.instantiateClass(tClass); @@ -265,16 +274,16 @@ public class FeignClientFactoryBean } protected T get(FeignContext context, Class type) { - T instance = context.getInstance(this.contextId, type); + T instance = context.getInstance(contextId, type); if (instance == null) { throw new IllegalStateException( - "No bean found of type " + type + " for " + this.contextId); + "No bean found of type " + type + " for " + contextId); } return instance; } protected T getOptional(FeignContext context, Class type) { - return context.getInstance(this.contextId, type); + return context.getInstance(contextId, type); } protected T getInheritedAwareOptional(FeignContext context, Class type) { @@ -282,17 +291,17 @@ public class FeignClientFactoryBean return getOptional(context, type); } else { - return context.getInstanceWithoutAncestors(this.contextId, type); + return context.getInstanceWithoutAncestors(contextId, type); } } protected Map getInheritedAwareInstances(FeignContext context, Class type) { if (inheritParentContext) { - return context.getInstances(this.contextId, type); + return context.getInstances(contextId, type); } else { - return context.getInstancesWithoutAncestors(this.contextId, type); + return context.getInstancesWithoutAncestors(contextId, type); } } @@ -320,22 +329,22 @@ public class FeignClientFactoryBean * information */ T getTarget() { - FeignContext context = this.applicationContext.getBean(FeignContext.class); + FeignContext context = applicationContext.getBean(FeignContext.class); Feign.Builder builder = feign(context); - if (!StringUtils.hasText(this.url)) { - if (!this.name.startsWith("http")) { - this.url = "http://" + this.name; + if (!StringUtils.hasText(url)) { + if (!name.startsWith("http")) { + url = "http://" + name; } else { - this.url = this.name; + url = name; } - this.url += cleanPath(); + url += cleanPath(); return (T) loadBalance(builder, context, - new HardCodedTarget<>(this.type, this.name, this.url)); + new HardCodedTarget<>(type, name, url)); } - if (StringUtils.hasText(this.url) && !this.url.startsWith("http")) { - this.url = "http://" + this.url; + if (StringUtils.hasText(url) && !url.startsWith("http")) { + url = "http://" + url; } String url = this.url + cleanPath(); Client client = getOptional(context, Client.class); @@ -349,7 +358,7 @@ public class FeignClientFactoryBean } Targeter targeter = get(context, Targeter.class); return (T) targeter.target(this, builder, context, - new HardCodedTarget<>(this.type, this.name, url)); + new HardCodedTarget<>(type, name, url)); } private String cleanPath() { @@ -367,7 +376,7 @@ public class FeignClientFactoryBean @Override public Class getObjectType() { - return this.type; + return type; } @Override @@ -376,7 +385,7 @@ public class FeignClientFactoryBean } public Class getType() { - return this.type; + return type; } public void setType(Class type) { @@ -384,7 +393,7 @@ public class FeignClientFactoryBean } public String getName() { - return this.name; + return name; } public void setName(String name) { @@ -392,7 +401,7 @@ public class FeignClientFactoryBean } public String getContextId() { - return this.contextId; + return contextId; } public void setContextId(String contextId) { @@ -400,7 +409,7 @@ public class FeignClientFactoryBean } public String getUrl() { - return this.url; + return url; } public void setUrl(String url) { @@ -408,7 +417,7 @@ public class FeignClientFactoryBean } public String getPath() { - return this.path; + return path; } public void setPath(String path) { @@ -416,7 +425,7 @@ public class FeignClientFactoryBean } public boolean isDecode404() { - return this.decode404; + return decode404; } public void setDecode404(boolean decode404) { @@ -432,7 +441,7 @@ public class FeignClientFactoryBean } public ApplicationContext getApplicationContext() { - return this.applicationContext; + return applicationContext; } @Override @@ -441,7 +450,7 @@ public class FeignClientFactoryBean } public Class getFallback() { - return this.fallback; + return fallback; } public void setFallback(Class fallback) { @@ -449,7 +458,7 @@ public class FeignClientFactoryBean } public Class getFallbackFactory() { - return this.fallbackFactory; + return fallbackFactory; } public void setFallbackFactory(Class fallbackFactory) { @@ -465,35 +474,31 @@ public class FeignClientFactoryBean return false; } FeignClientFactoryBean that = (FeignClientFactoryBean) o; - return Objects.equals(this.applicationContext, that.applicationContext) - && this.decode404 == that.decode404 - && this.inheritParentContext == that.inheritParentContext - && Objects.equals(this.fallback, that.fallback) - && Objects.equals(this.fallbackFactory, that.fallbackFactory) - && Objects.equals(this.name, that.name) - && Objects.equals(this.path, that.path) - && Objects.equals(this.type, that.type) - && Objects.equals(this.url, that.url); + return Objects.equals(applicationContext, that.applicationContext) + && decode404 == that.decode404 + && inheritParentContext == that.inheritParentContext + && Objects.equals(fallback, that.fallback) + && Objects.equals(fallbackFactory, that.fallbackFactory) + && Objects.equals(name, that.name) && Objects.equals(path, that.path) + && Objects.equals(type, that.type) && Objects.equals(url, that.url); } @Override public int hashCode() { - return Objects.hash(this.applicationContext, this.decode404, - this.inheritParentContext, this.fallback, this.fallbackFactory, this.name, - this.path, this.type, this.url); + return Objects.hash(applicationContext, decode404, inheritParentContext, fallback, + fallbackFactory, name, path, type, url); } @Override public String toString() { - return new StringBuilder("FeignClientFactoryBean{").append("type=") - .append(this.type).append(", ").append("name='").append(this.name) - .append("', ").append("url='").append(this.url).append("', ") - .append("path='").append(this.path).append("', ").append("decode404=") - .append(this.decode404).append(", ").append("inheritParentContext=") - .append(this.inheritParentContext).append(", ") - .append("applicationContext=").append(this.applicationContext) - .append(", ").append("fallback=").append(this.fallback).append(", ") - .append("fallbackFactory=").append(this.fallbackFactory).append("}") + return new StringBuilder("FeignClientFactoryBean{").append("type=").append(type) + .append(", ").append("name='").append(name).append("', ").append("url='") + .append(url).append("', ").append("path='").append(path).append("', ") + .append("decode404=").append(decode404).append(", ") + .append("inheritParentContext=").append(inheritParentContext).append(", ") + .append("applicationContext=").append(applicationContext).append(", ") + .append("fallback=").append(fallback).append(", ") + .append("fallbackFactory=").append(fallbackFactory).append("}") .toString(); } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientUsingPropertiesTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientUsingPropertiesTests.java index 1169112b..b97e2f54 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientUsingPropertiesTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientUsingPropertiesTests.java @@ -17,13 +17,19 @@ package org.springframework.cloud.openfeign; import java.io.IOException; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; import java.lang.reflect.Type; import java.net.SocketTimeoutException; +import java.util.ArrayList; import java.util.Collections; import java.util.Map; +import java.util.Objects; import javax.servlet.http.HttpServletRequest; +import feign.InvocationHandlerFactory; +import feign.Request; import feign.RequestInterceptor; import feign.RequestTemplate; import feign.RetryableException; @@ -47,6 +53,8 @@ import org.springframework.http.MediaType; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.TestPropertySource; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.test.util.ReflectionTestUtils; +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; @@ -57,7 +65,9 @@ import static org.springframework.boot.test.context.SpringBootTest.WebEnvironmen /** * @author Eko Kurniawan Khannedy + * @author Olga Maciaszek-Sharma */ +@SuppressWarnings("FieldMayBeFinal") @RunWith(SpringJUnit4ClassRunner.class) @SpringBootTest(classes = FeignClientUsingPropertiesTests.Application.class, webEnvironment = RANDOM_PORT) @@ -83,45 +93,45 @@ public class FeignClientUsingPropertiesTests { private FeignClientFactoryBean formFactoryBean; public FeignClientUsingPropertiesTests() { - this.fooFactoryBean = new FeignClientFactoryBean(); - this.fooFactoryBean.setContextId("foo"); - this.fooFactoryBean.setType(FeignClientFactoryBean.class); + fooFactoryBean = new FeignClientFactoryBean(); + fooFactoryBean.setContextId("foo"); + fooFactoryBean.setType(FeignClientFactoryBean.class); - this.barFactoryBean = new FeignClientFactoryBean(); - this.barFactoryBean.setContextId("bar"); - this.barFactoryBean.setType(FeignClientFactoryBean.class); + barFactoryBean = new FeignClientFactoryBean(); + barFactoryBean.setContextId("bar"); + barFactoryBean.setType(FeignClientFactoryBean.class); - this.unwrapFactoryBean = new FeignClientFactoryBean(); - this.unwrapFactoryBean.setContextId("unwrap"); - this.unwrapFactoryBean.setType(FeignClientFactoryBean.class); + unwrapFactoryBean = new FeignClientFactoryBean(); + unwrapFactoryBean.setContextId("unwrap"); + unwrapFactoryBean.setType(FeignClientFactoryBean.class); - this.formFactoryBean = new FeignClientFactoryBean(); - this.formFactoryBean.setContextId("form"); - this.formFactoryBean.setType(FeignClientFactoryBean.class); + formFactoryBean = new FeignClientFactoryBean(); + formFactoryBean.setContextId("form"); + formFactoryBean.setType(FeignClientFactoryBean.class); } public FooClient fooClient() { - this.fooFactoryBean.setApplicationContext(this.applicationContext); - return this.fooFactoryBean.feign(this.context).target(FooClient.class, - "http://localhost:" + this.port); + fooFactoryBean.setApplicationContext(applicationContext); + return fooFactoryBean.feign(context).target(FooClient.class, + "http://localhost:" + port); } public BarClient barClient() { - this.barFactoryBean.setApplicationContext(this.applicationContext); - return this.barFactoryBean.feign(this.context).target(BarClient.class, - "http://localhost:" + this.port); + barFactoryBean.setApplicationContext(applicationContext); + return barFactoryBean.feign(context).target(BarClient.class, + "http://localhost:" + port); } public UnwrapClient unwrapClient() { - this.unwrapFactoryBean.setApplicationContext(this.applicationContext); - return this.unwrapFactoryBean.feign(this.context).target(UnwrapClient.class, - "http://localhost:" + this.port); + unwrapFactoryBean.setApplicationContext(applicationContext); + return unwrapFactoryBean.feign(context).target(UnwrapClient.class, + "http://localhost:" + port); } public FormClient formClient() { - this.formFactoryBean.setApplicationContext(this.applicationContext); - return this.formFactoryBean.feign(this.context).target(FormClient.class, - "http://localhost:" + this.port); + formFactoryBean.setApplicationContext(applicationContext); + return formFactoryBean.feign(context).target(FormClient.class, + "http://localhost:" + port); } @Test @@ -149,6 +159,47 @@ public class FeignClientUsingPropertiesTests { assertThat(response).isEqualTo("Data"); } + @Test + public void readTimeoutShouldWorkWhenConnectTimeoutNotSet() { + FeignClientFactoryBean readTimeoutFactoryBean = new FeignClientFactoryBean(); + readTimeoutFactoryBean.setContextId("readTimeout"); + readTimeoutFactoryBean.setType(FeignClientFactoryBean.class); + readTimeoutFactoryBean.setApplicationContext(applicationContext); + + TimeoutClient client = readTimeoutFactoryBean.feign(context) + .target(TimeoutClient.class, "http://localhost:" + port); + + Request.Options options = getRequestOptions((Proxy) client); + + assertThat(options.readTimeoutMillis()).isEqualTo(1000); + assertThat(options.connectTimeoutMillis()).isEqualTo(5000); + } + + @Test + public void connectTimeoutShouldWorkWhenReadTimeoutNotSet() { + FeignClientFactoryBean readTimeoutFactoryBean = new FeignClientFactoryBean(); + readTimeoutFactoryBean.setContextId("connectTimeout"); + readTimeoutFactoryBean.setType(FeignClientFactoryBean.class); + readTimeoutFactoryBean.setApplicationContext(applicationContext); + + TimeoutClient client = readTimeoutFactoryBean.feign(context) + .target(TimeoutClient.class, "http://localhost:" + port); + + Request.Options options = getRequestOptions((Proxy) client); + + assertThat(options.connectTimeoutMillis()).isEqualTo(1000); + assertThat(options.readTimeoutMillis()).isEqualTo(5000); + } + + private Request.Options getRequestOptions(Proxy client) { + Object invocationHandler = ReflectionTestUtils.getField(client, "h"); + Map dispatch = (Map) ReflectionTestUtils + .getField(Objects.requireNonNull(invocationHandler), "dispatch"); + Method key = new ArrayList<>(dispatch.keySet()).get(0); + return (Request.Options) ReflectionTestUtils.getField(dispatch.get(key), + "options"); + } + protected interface FooClient { @RequestMapping(method = RequestMethod.GET, value = "/foo") @@ -178,6 +229,13 @@ public class FeignClientUsingPropertiesTests { } + protected interface TimeoutClient { + + @GetMapping("/timeouts") + String timeouts(); + + } + @Configuration(proxyBeanMethods = false) @EnableAutoConfiguration @RestController diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java index b22bcc58..a1a9f9d6 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java @@ -429,7 +429,7 @@ public class SpringMvcContractTests { assertThat(data.template().url()).isEqualTo("/test/{id}"); assertThat(data.template().method()).isEqualTo("GET"); - assertThat(data.template().headers().get("X-Foo").iterator().next()) + assertThat(data.template().headers().get("x-Foo").iterator().next()) .isEqualTo("bar"); } diff --git a/spring-cloud-openfeign-core/src/test/resources/feign-properties.properties b/spring-cloud-openfeign-core/src/test/resources/feign-properties.properties index 0caec544..e3248104 100644 --- a/spring-cloud-openfeign-core/src/test/resources/feign-properties.properties +++ b/spring-cloud-openfeign-core/src/test/resources/feign-properties.properties @@ -16,3 +16,5 @@ feign.client.config.form.encoder=org.springframework.cloud.openfeign.FeignClient feign.client.config.unwrap.connectTimeout=1000 feign.client.config.unwrap.readTimeout=1000 feign.client.config.unwrap.exceptionPropagationPolicy=unwrap +feign.client.config.readTimeout.readTimeout=1000 +feign.client.config.connectTimeout.connectTimeout=1000