From c8fded80d5b159df4f6231e89be7d61ed3269597 Mon Sep 17 00:00:00 2001 From: lowzj Date: Thu, 9 Feb 2017 13:58:30 +0800 Subject: [PATCH 1/7] fix gh-1672, support @RequestParam without a value --- .../feign/support/SpringMvcContract.java | 5 +---- .../feign/support/SpringMvcContractTests.java | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java index 8a023630..fd7cfeb3 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java @@ -28,7 +28,6 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import org.springframework.beans.BeanUtils; import org.springframework.cloud.netflix.feign.AnnotatedParameterProcessor; import org.springframework.cloud.netflix.feign.annotation.PathVariableParameterProcessor; import org.springframework.cloud.netflix.feign.annotation.RequestHeaderParameterProcessor; @@ -324,9 +323,7 @@ public class SpringMvcContract extends Contract.BaseContract // has a parameter name return parameterNames != null && parameterNames.length > parameterIndex // has a type - && parameterTypes != null && parameterTypes.length > parameterIndex - // and it is a simple property - && BeanUtils.isSimpleProperty(parameterTypes[parameterIndex].getClass()); + && parameterTypes != null && parameterTypes.length > parameterIndex; } private class SimpleAnnotatedParameterContext diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringMvcContractTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringMvcContractTests.java index ecb11426..edb44acb 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringMvcContractTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringMvcContractTests.java @@ -297,6 +297,19 @@ public class SpringMvcContractTests { assertNotNull(data.indexToExpander().get(0)); } + @Test + public void testProcessAnnotations_ListParamsWithoutName() throws Exception { + Method method = TestTemplate_ListParamsWithoutName.class.getDeclaredMethod("getTest", + List.class); + MethodMetadata data = this.contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertEquals("/test", data.template().url()); + assertEquals("GET", data.template().method()); + assertEquals("[{id}]", data.template().queries().get("id").toString()); + assertNotNull(data.indexToExpander().get(0)); + } + @Test public void testProcessAnnotations_MapParams() throws Exception { Method method = TestTemplate_MapParams.class.getDeclaredMethod("getTest", @@ -457,6 +470,11 @@ public class SpringMvcContractTests { ResponseEntity getTest(@RequestParam("id") List id); } + public interface TestTemplate_ListParamsWithoutName { + @RequestMapping(value = "/test", method = RequestMethod.GET) + ResponseEntity getTest(@RequestParam List id); + } + public interface TestTemplate_MapParams { @RequestMapping(value = "/test", method = RequestMethod.GET) ResponseEntity getTest(@RequestParam Map params); From 3e7b196950c5f1aec6899fc35089b7c7be7bc72d Mon Sep 17 00:00:00 2001 From: Johannes Edmeier Date: Fri, 10 Feb 2017 18:12:26 +0100 Subject: [PATCH 2/7] Close response on errors writing to the outputStream (#1372) Currently all errors during writing to the outputStream are ignored. So the response is read to the end. This is a problem when the response is infite or very long running (`hystrix.stream` for example) and results in the socket never being closed and eating up threads (on zuul and the proxied backend). With this commit the errors during writing are not ignored resulting in closing the response. --- .../zuul/filters/post/SendResponseFilter.java | 28 ++++++---- .../filters/route/RibbonRoutingFilter.java | 1 + .../route/SimpleHostRoutingFilter.java | 18 ++++--- .../filters/post/SendResponseFilterTests.java | 52 ++++++++++++++++--- 4 files changed, 74 insertions(+), 25 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilter.java index 606dbd68..6ac172bb 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilter.java @@ -17,6 +17,7 @@ package org.springframework.cloud.netflix.zuul.filters.post; import java.io.ByteArrayInputStream; +import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -184,28 +185,33 @@ public class SendResponseFilter extends ZuulFilter { } } finally { + /** + * Closing the wrapping InputStream itself has no effect on closing the underlying tcp connection since it's a wrapped stream. I guess for http + * keep-alive. When closing the wrapping stream it tries to reach the end of the current request, which is impossible for infinite http streams. So + * instead of closing the InputStream we close the HTTP response. + * + * @author Johannes Edmeier + */ try { - if (is != null) { - is.close(); + Object zuulResponse = RequestContext.getCurrentContext() + .get("zuulResponse"); + if (zuulResponse instanceof Closeable) { + ((Closeable) zuulResponse).close(); } outStream.flush(); // The container will close the stream for us } catch (IOException ex) { + log.warn("Error while sending response to client: " + ex.getMessage()); } } } private void writeResponse(InputStream zin, OutputStream out) throws Exception { - try { - byte[] bytes = buffers.get(); - int bytesRead = -1; - while ((bytesRead = zin.read(bytes)) != -1) { - out.write(bytes, 0, bytesRead); - } - } - catch(IOException ioe) { - log.warn("Error while sending response to client: "+ioe.getMessage()); + byte[] bytes = buffers.get(); + int bytesRead = -1; + while ((bytesRead = zin.read(bytes)) != -1) { + out.write(bytes, 0, bytesRead); } } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonRoutingFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonRoutingFilter.java index 2ef434fb..749e7462 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonRoutingFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonRoutingFilter.java @@ -218,6 +218,7 @@ public class RibbonRoutingFilter extends ZuulFilter { protected void setResponse(ClientHttpResponse resp) throws ClientException, IOException { + RequestContext.getCurrentContext().set("zuulResponse", resp); this.helper.setResponse(resp.getStatusCode().value(), resp.getBody() == null ? null : resp.getBody(), resp.getHeaders()); } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java index 90bba011..9fec0b08 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java @@ -42,10 +42,10 @@ import org.apache.http.HttpHost; import org.apache.http.HttpRequest; import org.apache.http.HttpResponse; import org.apache.http.ProtocolException; -import org.apache.http.client.HttpClient; import org.apache.http.client.RedirectStrategy; import org.apache.http.client.config.CookieSpecs; import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpPatch; import org.apache.http.client.methods.HttpPost; import org.apache.http.client.methods.HttpPut; @@ -192,8 +192,8 @@ public class SimpleHostRoutingFilter extends ZuulFilter { this.helper.addIgnoredHeaders(); try { - HttpResponse response = forward(this.httpClient, verb, uri, request, headers, - params, requestEntity); + CloseableHttpResponse response = forward(this.httpClient, verb, uri, request, + headers, params, requestEntity); setResponse(response); } catch (Exception ex) { @@ -278,8 +278,8 @@ public class SimpleHostRoutingFilter extends ZuulFilter { }).build(); } - private HttpResponse forward(HttpClient httpclient, String verb, String uri, - HttpServletRequest request, MultiValueMap headers, + private CloseableHttpResponse forward(CloseableHttpClient httpclient, String verb, + String uri, HttpServletRequest request, MultiValueMap headers, MultiValueMap params, InputStream requestEntity) throws Exception { Map info = this.helper.debug(verb, uri, headers, params, @@ -301,7 +301,8 @@ public class SimpleHostRoutingFilter extends ZuulFilter { try { log.debug(httpHost.getHostName() + " " + httpHost.getPort() + " " + httpHost.getSchemeName()); - HttpResponse zuulResponse = forwardRequest(httpclient, httpHost, httpRequest); + CloseableHttpResponse zuulResponse = forwardRequest(httpclient, httpHost, + httpRequest); this.helper.appendDebug(info, zuulResponse.getStatusLine().getStatusCode(), revertHeaders(zuulResponse.getAllHeaders())); return zuulResponse; @@ -379,8 +380,8 @@ public class SimpleHostRoutingFilter extends ZuulFilter { return list.toArray(new BasicHeader[0]); } - private HttpResponse forwardRequest(HttpClient httpclient, HttpHost httpHost, - HttpRequest httpRequest) throws IOException { + private CloseableHttpResponse forwardRequest(CloseableHttpClient httpclient, + HttpHost httpHost, HttpRequest httpRequest) throws IOException { return httpclient.execute(httpHost, httpRequest); } @@ -407,6 +408,7 @@ public class SimpleHostRoutingFilter extends ZuulFilter { } private void setResponse(HttpResponse response) throws IOException { + RequestContext.getCurrentContext().set("zuulResponse", response); this.helper.setResponse(response.getStatusLine().getStatusCode(), response.getEntity() == null ? null : response.getEntity().getContent(), revertHeaders(response.getAllHeaders())); diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilterTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilterTests.java index 9d0ca919..b8469ef9 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilterTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilterTests.java @@ -16,9 +16,26 @@ package org.springframework.cloud.netflix.zuul.filters.post; -import java.io.ByteArrayInputStream; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; +import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.isA; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.springframework.cloud.netflix.zuul.filters.support.FilterConstants.X_ZUUL_DEBUG_HEADER; +import static org.hamcrest.Matchers.equalTo; +import java.io.ByteArrayInputStream; +import java.io.Closeable; +import java.io.IOException; +import java.lang.reflect.UndeclaredThrowableException; + +import javax.servlet.ServletOutputStream; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import org.junit.After; import org.junit.Before; @@ -34,11 +51,6 @@ import com.netflix.zuul.constants.ZuulConstants; import com.netflix.zuul.context.Debug; import com.netflix.zuul.context.RequestContext; -import static org.hamcrest.Matchers.equalTo; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.springframework.cloud.netflix.zuul.filters.support.FilterConstants.X_ZUUL_DEBUG_HEADER; - /** * @author Spencer Gibb */ @@ -96,6 +108,34 @@ public class SendResponseFilterTests { assertThat("wrong origin content length", contentLength, equalTo("6")); } + @Test + public void closeResponseOutpusStreamError() throws Exception { + HttpServletResponse response = mock(HttpServletResponse.class); + + RequestContext context = new RequestContext(); + context.setRequest(new MockHttpServletRequest()); + context.setResponse(response); + context.setResponseDataStream(new ByteArrayInputStream("Hello\n".getBytes("UTF-8"))); + Closeable zuulResponse = mock(Closeable.class); + context.set("zuulResponse", zuulResponse); + RequestContext.testSetCurrentContext(context); + + SendResponseFilter filter = new SendResponseFilter(); + + ServletOutputStream zuuloutputstream = mock(ServletOutputStream.class); + doThrow(new IOException("Response to client closed")).when(zuuloutputstream).write(isA(byte[].class), anyInt(), anyInt()); + + when(response.getOutputStream()).thenReturn(zuuloutputstream); + + try { + filter.run(); + } catch (UndeclaredThrowableException ex) { + assertThat(ex.getUndeclaredThrowable().getMessage(), is("Response to client closed")); + } + + verify(zuulResponse).close(); + } + private void runFilter(String characterEncoding, String content, boolean streamContent) throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); SendResponseFilter filter = createFilter(content, characterEncoding, response, streamContent); From 7237275f5dc2d367fcda84ad6cd2bc770ea56bef Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Fri, 10 Feb 2017 10:14:12 -0700 Subject: [PATCH 3/7] polish --- .../filters/post/SendResponseFilterTests.java | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilterTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilterTests.java index b8469ef9..e5389e58 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilterTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/post/SendResponseFilterTests.java @@ -16,18 +16,6 @@ package org.springframework.cloud.netflix.zuul.filters.post; -import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.mockito.Matchers.anyInt; -import static org.mockito.Matchers.isA; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; -import static org.springframework.cloud.netflix.zuul.filters.support.FilterConstants.X_ZUUL_DEBUG_HEADER; -import static org.hamcrest.Matchers.equalTo; - import java.io.ByteArrayInputStream; import java.io.Closeable; import java.io.IOException; @@ -51,6 +39,18 @@ import com.netflix.zuul.constants.ZuulConstants; import com.netflix.zuul.context.Debug; import com.netflix.zuul.context.RequestContext; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; +import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.isA; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.springframework.cloud.netflix.zuul.filters.support.FilterConstants.X_ZUUL_DEBUG_HEADER; + /** * @author Spencer Gibb */ @@ -108,33 +108,33 @@ public class SendResponseFilterTests { assertThat("wrong origin content length", contentLength, equalTo("6")); } - @Test + @Test public void closeResponseOutpusStreamError() throws Exception { - HttpServletResponse response = mock(HttpServletResponse.class); + HttpServletResponse response = mock(HttpServletResponse.class); - RequestContext context = new RequestContext(); - context.setRequest(new MockHttpServletRequest()); - context.setResponse(response); - context.setResponseDataStream(new ByteArrayInputStream("Hello\n".getBytes("UTF-8"))); - Closeable zuulResponse = mock(Closeable.class); - context.set("zuulResponse", zuulResponse); - RequestContext.testSetCurrentContext(context); + RequestContext context = new RequestContext(); + context.setRequest(new MockHttpServletRequest()); + context.setResponse(response); + context.setResponseDataStream(new ByteArrayInputStream("Hello\n".getBytes("UTF-8"))); + Closeable zuulResponse = mock(Closeable.class); + context.set("zuulResponse", zuulResponse); + RequestContext.testSetCurrentContext(context); - SendResponseFilter filter = new SendResponseFilter(); + SendResponseFilter filter = new SendResponseFilter(); - ServletOutputStream zuuloutputstream = mock(ServletOutputStream.class); - doThrow(new IOException("Response to client closed")).when(zuuloutputstream).write(isA(byte[].class), anyInt(), anyInt()); + ServletOutputStream zuuloutputstream = mock(ServletOutputStream.class); + doThrow(new IOException("Response to client closed")).when(zuuloutputstream).write(isA(byte[].class), anyInt(), anyInt()); - when(response.getOutputStream()).thenReturn(zuuloutputstream); + when(response.getOutputStream()).thenReturn(zuuloutputstream); - try { - filter.run(); - } catch (UndeclaredThrowableException ex) { - assertThat(ex.getUndeclaredThrowable().getMessage(), is("Response to client closed")); - } + try { + filter.run(); + } catch (UndeclaredThrowableException ex) { + assertThat(ex.getUndeclaredThrowable().getMessage(), is("Response to client closed")); + } - verify(zuulResponse).close(); - } + verify(zuulResponse).close(); + } private void runFilter(String characterEncoding, String content, boolean streamContent) throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); From c795bd16a7748749af2d84ad1520caa481bb1fe2 Mon Sep 17 00:00:00 2001 From: Roman Terentiev Date: Fri, 10 Feb 2017 19:25:27 +0200 Subject: [PATCH 4/7] #1203 Improve filter performance by using only one instance of AllEncompassingFormHttpMessageConverter (#1681) --- .../zuul/filters/pre/FormBodyWrapperFilter.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/FormBodyWrapperFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/FormBodyWrapperFilter.java index 8e6bb258..175c5cfb 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/FormBodyWrapperFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/FormBodyWrapperFilter.java @@ -31,6 +31,7 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.HttpOutputMessage; import org.springframework.http.InvalidMediaTypeException; import org.springframework.http.MediaType; +import org.springframework.http.converter.FormHttpMessageConverter; import org.springframework.http.converter.support.AllEncompassingFormHttpMessageConverter; import org.springframework.util.Assert; import org.springframework.util.MultiValueMap; @@ -52,10 +53,16 @@ import static org.springframework.cloud.netflix.zuul.filters.support.FilterConst */ public class FormBodyWrapperFilter extends ZuulFilter { + private FormHttpMessageConverter formHttpMessageConverter; private Field requestField; private Field servletRequestField; public FormBodyWrapperFilter() { + this(new AllEncompassingFormHttpMessageConverter()); + } + + public FormBodyWrapperFilter(FormHttpMessageConverter formHttpMessageConverter) { + this.formHttpMessageConverter = formHttpMessageConverter; this.requestField = ReflectionUtils.findField(HttpServletRequestWrapper.class, "req", HttpServletRequest.class); this.servletRequestField = ReflectionUtils.findField(ServletRequestWrapper.class, @@ -139,8 +146,6 @@ public class FormBodyWrapperFilter extends ZuulFilter { private int contentLength; - private AllEncompassingFormHttpMessageConverter converter = new AllEncompassingFormHttpMessageConverter(); - public FormBodyRequestWrapper(HttpServletRequest request) { super(request); this.request = request; @@ -184,7 +189,7 @@ public class FormBodyWrapperFilter extends ZuulFilter { this.contentType = MediaType.valueOf(this.request.getContentType()); data.getHeaders().setContentType(this.contentType); - this.converter.write(builder, this.contentType, data); + FormBodyWrapperFilter.this.formHttpMessageConverter.write(builder, this.contentType, data); // copy new content type including multipart boundary this.contentType = data.getHeaders().getContentType(); this.contentData = data.getInput(); From d78da7b99451425f32dd0446877a65354871c489 Mon Sep 17 00:00:00 2001 From: Erik Kringen Date: Fri, 10 Feb 2017 12:37:17 -0600 Subject: [PATCH 5/7] Add support for Feign.SetterFactory (#1673) Fixes gh-1414 --- .../main/asciidoc/spring-cloud-netflix.adoc | 1 + .../cloud/netflix/feign/HystrixTargeter.java | 15 +++- .../netflix/feign/valid/FeignClientTests.java | 82 ++++++++++++++++--- 3 files changed, 87 insertions(+), 11 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-netflix.adoc b/docs/src/main/asciidoc/spring-cloud-netflix.adoc index 110fb6a9..00b26bf5 100644 --- a/docs/src/main/asciidoc/spring-cloud-netflix.adoc +++ b/docs/src/main/asciidoc/spring-cloud-netflix.adoc @@ -976,6 +976,7 @@ Spring Cloud Netflix _does not_ provide the following beans by default for feign * `ErrorDecoder` * `Request.Options` * `Collection` +* `SetterFactory` Creating a bean of one of those type and placing it in a `@FeignClient` configuration (such as `FooConfiguration` above) allows you to override each one of the beans described. Example: diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/HystrixTargeter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/HystrixTargeter.java index 66106c70..cfbe3659 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/HystrixTargeter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/HystrixTargeter.java @@ -17,14 +17,17 @@ package org.springframework.cloud.netflix.feign; +import org.springframework.util.Assert; + import feign.Feign; import feign.Target; import feign.hystrix.FallbackFactory; import feign.hystrix.HystrixFeign; -import org.springframework.util.Assert; +import feign.hystrix.SetterFactory; /** * @author Spencer Gibb + * @author Erik Kringen */ @SuppressWarnings("unchecked") class HystrixTargeter implements Targeter { @@ -36,6 +39,11 @@ class HystrixTargeter implements Targeter { return feign.target(target); } feign.hystrix.HystrixFeign.Builder builder = (feign.hystrix.HystrixFeign.Builder) feign; + SetterFactory setterFactory = getOptional(factory.getName(), context, + SetterFactory.class); + if (setterFactory != null) { + builder.setterFactory(setterFactory); + } Class fallback = factory.getFallback(); if (fallback != void.class) { return targetWithFallback(factory.getName(), context, target, builder, fallback); @@ -95,4 +103,9 @@ class HystrixTargeter implements Targeter { } return (T) fallbackInstance; } + + private T getOptional(String feignClientName, FeignContext context, + Class beanType) { + return context.getInstance(feignClientName, beanType); + } } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientTests.java index 903fad03..d928b28c 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientTests.java @@ -25,6 +25,7 @@ import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.text.ParseException; import java.util.ArrayList; @@ -68,14 +69,19 @@ import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; import com.netflix.hystrix.HystrixCommand; +import com.netflix.hystrix.HystrixCommandGroupKey; +import com.netflix.hystrix.HystrixCommandKey; import com.netflix.loadbalancer.Server; import com.netflix.loadbalancer.ServerList; import feign.Client; +import feign.Feign; import feign.Logger; import feign.RequestInterceptor; import feign.RequestTemplate; +import feign.Target; import feign.hystrix.FallbackFactory; +import feign.hystrix.SetterFactory; import lombok.AllArgsConstructor; import lombok.Data; import lombok.NoArgsConstructor; @@ -85,6 +91,7 @@ import rx.Single; /** * @author Spencer Gibb * @author Jakub Narloch + * @author Erik Kringen */ @RunWith(SpringJUnit4ClassRunner.class) @SpringBootTest(classes = FeignClientTests.Application.class, webEnvironment = WebEnvironment.RANDOM_PORT, value = { @@ -124,6 +131,9 @@ public class FeignClientTests { @Qualifier("localapp3FeignClient") HystrixClient namedHystrixClient; + @Autowired + HystrixSetterFactoryClient hystrixSetterFactoryClient; + protected enum Arg { A, B; @@ -298,18 +308,47 @@ public class FeignClientTests { } } + @FeignClient(name = "localapp5", configuration = TestHystrixSetterFactoryClientConfig.class) + protected interface HystrixSetterFactoryClient { + @RequestMapping(method = RequestMethod.GET, path = "/hellos") + HystrixCommand> getHellosHystrix(); + } + + public static class TestHystrixSetterFactoryClientConfig { + public static final String SETTER_PREFIX = "SETTER-"; + @Bean + public SetterFactory commandKeyIsRequestLineSetterFactory() { + return new SetterFactory() { + @Override public HystrixCommand.Setter create(Target target, + Method method) { + String groupKey = SETTER_PREFIX + target.name(); + RequestMapping requestMapping = method + .getAnnotation(RequestMapping.class); + String commandKey = + SETTER_PREFIX + requestMapping.method()[0] + " " + requestMapping + .path()[0]; + return HystrixCommand.Setter + .withGroupKey(HystrixCommandGroupKey.Factory.asKey(groupKey)) + .andCommandKey(HystrixCommandKey.Factory.asKey(commandKey)); + } + }; + } + } + @Configuration @EnableAutoConfiguration @RestController @EnableFeignClients(clients = { TestClientServiceId.class, TestClient.class, - DecodingTestClient.class, HystrixClient.class, HystrixClientWithFallBackFactory.class }, - defaultConfiguration = TestDefaultFeignConfig.class) + DecodingTestClient.class, HystrixClient.class, HystrixClientWithFallBackFactory.class, + HystrixSetterFactoryClient.class}, + defaultConfiguration = TestDefaultFeignConfig.class) @RibbonClients({ - @RibbonClient(name = "localapp", configuration = LocalRibbonClientConfiguration.class), - @RibbonClient(name = "localapp1", configuration = LocalRibbonClientConfiguration.class), - @RibbonClient(name = "localapp2", configuration = LocalRibbonClientConfiguration.class), - @RibbonClient(name = "localapp3", configuration = LocalRibbonClientConfiguration.class), - @RibbonClient(name = "localapp4", configuration = LocalRibbonClientConfiguration.class) + @RibbonClient(name = "localapp", configuration = LocalRibbonClientConfiguration.class), + @RibbonClient(name = "localapp1", configuration = LocalRibbonClientConfiguration.class), + @RibbonClient(name = "localapp2", configuration = LocalRibbonClientConfiguration.class), + @RibbonClient(name = "localapp3", configuration = LocalRibbonClientConfiguration.class), + @RibbonClient(name = "localapp4", configuration = LocalRibbonClientConfiguration.class), + @RibbonClient(name = "localapp5", configuration = LocalRibbonClientConfiguration.class) }) protected static class Application { @@ -525,12 +564,16 @@ public class FeignClientTests { } @Test - public void testHystrixCommand() { + public void testHystrixCommand() throws NoSuchMethodException { HystrixCommand> command = this.testClient.getHellosHystrix(); assertNotNull("command was null", command); assertEquals( - "Hystrix command group name should match the name of the feign client", - "localapp", command.getCommandGroup().name()); + "Hystrix command group name should match the name of the feign client", + "localapp", command.getCommandGroup().name()); + String configKey = Feign.configKey(TestClient.class, + TestClient.class.getMethod("getHellosHystrix", (Class[]) null)); + assertEquals("Hystrix command key name should match the feign config key", + configKey, command.getCommandKey().name()); List hellos = command.execute(); assertNotNull("hellos was null", hellos); assertEquals("hellos didn't match", hellos, getHelloList()); @@ -658,6 +701,25 @@ public class FeignClientTests { assertNotNull("namedHystrixClient was null", this.namedHystrixClient); } + @Test + public void testHystrixSetterFactory() { + HystrixCommand> command = this.hystrixSetterFactoryClient + .getHellosHystrix(); + assertNotNull("command was null", command); + String setterPrefix = TestHystrixSetterFactoryClientConfig.SETTER_PREFIX; + assertEquals( + "Hystrix command group name should match the name of the feign client with a prefix of " + + setterPrefix, setterPrefix + "localapp5", + command.getCommandGroup().name()); + assertEquals( + "Hystrix command key name should match the request method (space) request path with a prefix of " + + setterPrefix, setterPrefix + "GET /hellos", + command.getCommandKey().name()); + List hellos = command.execute(); + assertNotNull("hellos was null", hellos); + assertEquals("hellos didn't match", hellos, getHelloList()); + } + @Data @AllArgsConstructor @NoArgsConstructor From 731c8bf0fde6b597299ec20ff937e81f8b677abf Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Tue, 14 Feb 2017 13:59:25 -0500 Subject: [PATCH 6/7] Add the ability to provide a default ZuulFallbackProvider. Fixes #1506 --- .../main/asciidoc/spring-cloud-netflix.adoc | 50 +++++++++ .../support/AbstractRibbonCommandFactory.java | 14 ++- .../HttpClientRibbonCommandFallbackTests.java | 2 +- .../OkHttpRibbonCommandFallbackTests.java | 2 +- .../RestClientRibbonCommandFallbackTests.java | 2 +- .../support/RibbonCommandFallbackTests.java | 105 +++++++++++++++++- 6 files changed, 168 insertions(+), 7 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-netflix.adoc b/docs/src/main/asciidoc/spring-cloud-netflix.adoc index 00b26bf5..55881390 100644 --- a/docs/src/main/asciidoc/spring-cloud-netflix.adoc +++ b/docs/src/main/asciidoc/spring-cloud-netflix.adoc @@ -1755,6 +1755,56 @@ zuul: customers: /customers/** ---- +If you would like to provide a default fallback for all routes than you can create a bean of +type `ZuulFallbackProvider` and have the `getRoute` method return `*` or `null`. + +[source,java] +---- +class MyFallbackProvider implements ZuulFallbackProvider { + @Override + public String getRoute() { + return "*"; + } + + @Override + public ClientHttpResponse fallbackResponse() { + return new ClientHttpResponse() { + @Override + public HttpStatus getStatusCode() throws IOException { + return HttpStatus.OK; + } + + @Override + public int getRawStatusCode() throws IOException { + return 200; + } + + @Override + public String getStatusText() throws IOException { + return "OK"; + } + + @Override + public void close() { + + } + + @Override + public InputStream getBody() throws IOException { + return new ByteArrayInputStream("fallback".getBytes()); + } + + @Override + public HttpHeaders getHeaders() { + HttpHeaders headers = new HttpHeaders(); + headers.setContentType(MediaType.APPLICATION_JSON); + return headers; + } + }; + } +} +---- + [[zuul-developer-guide]] === Zuul Developer Guide diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommandFactory.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommandFactory.java index 11c97a21..cc8c4e03 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommandFactory.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommandFactory.java @@ -30,15 +30,25 @@ import org.springframework.cloud.netflix.zuul.filters.route.ZuulFallbackProvider public abstract class AbstractRibbonCommandFactory implements RibbonCommandFactory { private Map fallbackProviderCache; + private ZuulFallbackProvider defaultFallbackProvider = null; public AbstractRibbonCommandFactory(Set fallbackProviders){ this.fallbackProviderCache = new HashMap<>(); for(ZuulFallbackProvider provider : fallbackProviders) { - fallbackProviderCache.put(provider.getRoute(), provider); + String route = provider.getRoute(); + if("*".equals(route) || route == null) { + defaultFallbackProvider = provider; + } else { + fallbackProviderCache.put(route, provider); + } } } protected ZuulFallbackProvider getFallbackProvider(String route) { - return fallbackProviderCache.get(route); + ZuulFallbackProvider provider = fallbackProviderCache.get(route); + if(provider == null) { + provider = defaultFallbackProvider; + } + return provider; } } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFallbackTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFallbackTests.java index 43a94807..085e12ae 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFallbackTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFallbackTests.java @@ -33,7 +33,7 @@ import static org.springframework.boot.test.context.SpringBootTest.WebEnvironmen * @author Ryan Baxter */ @RunWith(SpringRunner.class) -@SpringBootTest(classes = HttpClientRibbonCommandIntegrationTests.TestConfig.class, webEnvironment = RANDOM_PORT, +@SpringBootTest(classes = RibbonCommandFallbackTests.TestConfig.class, webEnvironment = RANDOM_PORT, properties = { "zuul.routes.simple: /simple/**", "zuul.routes.another: /another/twolevel/**", "ribbon.ReadTimeout: 1"}) diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/okhttp/OkHttpRibbonCommandFallbackTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/okhttp/OkHttpRibbonCommandFallbackTests.java index 864e5504..fe07b64e 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/okhttp/OkHttpRibbonCommandFallbackTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/okhttp/OkHttpRibbonCommandFallbackTests.java @@ -31,7 +31,7 @@ import com.netflix.zuul.context.RequestContext; * @author Ryan Baxter */ @RunWith(SpringJUnit4ClassRunner.class) -@SpringBootTest(classes = OkHttpRibbonCommandIntegrationTests.TestConfig.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, value = { +@SpringBootTest(classes = RibbonCommandFallbackTests.TestConfig.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, value = { "zuul.routes.simple: /simple/**", "zuul.routes.another: /another/twolevel/**", "ribbon.ReadTimeout: 1"}) @DirtiesContext diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/restclient/RestClientRibbonCommandFallbackTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/restclient/RestClientRibbonCommandFallbackTests.java index ca9784f3..8496dbe4 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/restclient/RestClientRibbonCommandFallbackTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/restclient/RestClientRibbonCommandFallbackTests.java @@ -31,7 +31,7 @@ import com.netflix.zuul.context.RequestContext; * @author Ryan Baxter */ @RunWith(SpringJUnit4ClassRunner.class) -@SpringBootTest(classes = RestClientRibbonCommandIntegrationTests.TestConfig.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, value = { +@SpringBootTest(classes = RibbonCommandFallbackTests.TestConfig.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, value = { "zuul.routes.simple: /simple/**", "zuul.routes.another: /another/twolevel/**", "ribbon.ReadTimeout: 1"}) @DirtiesContext diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandFallbackTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandFallbackTests.java index 205204f4..03c768e3 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandFallbackTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandFallbackTests.java @@ -18,13 +18,35 @@ package org.springframework.cloud.netflix.zuul.filters.route.support; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.util.Collections; +import java.util.Set; import org.junit.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.autoconfigure.web.ErrorAttributes; import org.springframework.boot.context.embedded.LocalServerPort; import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.cloud.netflix.ribbon.RibbonClient; +import org.springframework.cloud.netflix.ribbon.RibbonClients; +import org.springframework.cloud.netflix.ribbon.SpringClientFactory; +import org.springframework.cloud.netflix.zuul.EnableZuulProxy; +import org.springframework.cloud.netflix.zuul.filters.ZuulProperties; +import org.springframework.cloud.netflix.zuul.filters.route.RibbonCommandFactory; +import org.springframework.cloud.netflix.zuul.filters.route.ZuulFallbackProvider; +import org.springframework.cloud.netflix.zuul.filters.route.apache.HttpClientRibbonCommandFactory; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; import org.springframework.http.HttpEntity; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.http.client.ClientHttpResponse; +import org.springframework.web.bind.annotation.RestController; import static org.junit.Assert.assertEquals; @@ -47,11 +69,90 @@ public abstract class RibbonCommandFallbackTests { } @Test - public void noFallback() { + public void defaultFallback() { String uri = "/another/twolevel/slow"; ResponseEntity result = new TestRestTemplate().exchange( "http://localhost:" + this.port + uri, HttpMethod.GET, new HttpEntity<>((Void) null), String.class); - assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, result.getStatusCode()); + assertEquals(HttpStatus.OK, result.getStatusCode()); + assertEquals("default fallback", result.getBody()); + } + + // Don't use @SpringBootApplication because we don't want to component scan + @Configuration + @EnableAutoConfiguration + @RestController + @EnableZuulProxy + @RibbonClients({ + @RibbonClient(name = "simple", configuration = ZuulProxyTestBase.SimpleRibbonClientConfiguration.class), + @RibbonClient(name = "another", configuration = ZuulProxyTestBase.AnotherRibbonClientConfiguration.class)}) + public static class TestConfig extends ZuulProxyTestBase.AbstractZuulProxyApplication { + + @Autowired(required = false) + private Set zuulFallbackProviders = Collections.emptySet(); + + + @Bean + public RibbonCommandFactory ribbonCommandFactory( + final SpringClientFactory clientFactory) { + return new HttpClientRibbonCommandFactory(clientFactory, new ZuulProperties(), + zuulFallbackProviders); + } + + @Bean + public ZuulProxyTestBase.MyErrorController myErrorController( + ErrorAttributes errorAttributes) { + return new ZuulProxyTestBase.MyErrorController(errorAttributes); + } + + @Bean + public ZuulFallbackProvider defaultFallbackProvider() { + return new DefaultFallbackProvider(); + } + } + + public static class DefaultFallbackProvider implements ZuulFallbackProvider { + + @Override + public String getRoute() { + return "*"; + } + + @Override + public ClientHttpResponse fallbackResponse() { + return new ClientHttpResponse() { + @Override + public HttpStatus getStatusCode() throws IOException { + return HttpStatus.OK; + } + + @Override + public int getRawStatusCode() throws IOException { + return 200; + } + + @Override + public String getStatusText() throws IOException { + return null; + } + + @Override + public void close() { + + } + + @Override + public InputStream getBody() throws IOException { + return new ByteArrayInputStream("default fallback".getBytes()); + } + + @Override + public HttpHeaders getHeaders() { + HttpHeaders headers = new HttpHeaders(); + headers.setContentType(MediaType.TEXT_HTML); + return headers; + } + }; + } } } From 52ab4d3ecd1a1c8f0993b23119e6bbfb89cc39fd Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Tue, 14 Feb 2017 14:50:55 -0500 Subject: [PATCH 7/7] Adds a check for an empty URI in updateToHttpsIfNeeded. Fixes #1690. --- .../springframework/cloud/netflix/ribbon/RibbonUtils.java | 2 +- .../cloud/netflix/ribbon/RibbonUtilsTests.java | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java index fb172027..35e85e32 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java @@ -71,7 +71,7 @@ public class RibbonUtils { public static URI updateToHttpsIfNeeded(URI uri, IClientConfig config, ServerIntrospector serverIntrospector, Server server) { String scheme = uri.getScheme(); - if (!"https".equals(scheme) && isSecure(config, serverIntrospector, server)) { + if (!"".equals(uri.toString()) && !"https".equals(scheme) && isSecure(config, serverIntrospector, server)) { UriComponentsBuilder uriComponentsBuilder = UriComponentsBuilder.fromUri(uri).scheme("https"); if (uri.getRawQuery() != null) { // When building the URI, UriComponentsBuilder verify the allowed characters and does not diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java index 92548e7f..f4d63dda 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java @@ -110,6 +110,14 @@ public class RibbonUtilsTests { "https://foo/%20bar?hello=1%202"))); } + @Test + public void emptyStringUri() throws URISyntaxException { + URI original = new URI(""); + URI updated = updateToHttpsIfNeeded(original, SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("URI should be the emptry string", updated, is(new URI( + ""))); + } + static DefaultClientConfigImpl getConfig(boolean value) { DefaultClientConfigImpl config = new DefaultClientConfigImpl(); config.setProperty(CommonClientConfigKey.IsSecure, value);