From 02948657a400ad57789d415003cda5e702774bc6 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Thu, 16 Jun 2016 11:45:52 -0600 Subject: [PATCH] Don't send 4xx errors to /error fixes gh-1110 --- .../filters/route/RibbonRoutingFilter.java | 9 ------- .../route/SimpleHostRoutingFilter.java | 8 ------ .../zuul/SampleZuulProxyApplicationTests.java | 6 +++-- .../cloud/netflix/zuul/ZuulProxyTestBase.java | 25 ++++++++++++++----- 4 files changed, 23 insertions(+), 25 deletions(-) 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 3903822b..02305106 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 @@ -78,7 +78,6 @@ public class RibbonRoutingFilter extends ZuulFilter { RibbonCommandContext commandContext = buildCommandContext(context); ClientHttpResponse response = forward(commandContext); setResponse(response); - setErrorCodeFor4xx(context, response); return response; } catch (ZuulException ex) { @@ -94,14 +93,6 @@ public class RibbonRoutingFilter extends ZuulFilter { return null; } - private void setErrorCodeFor4xx(RequestContext context, ClientHttpResponse response) - throws IOException { - HttpStatus httpStatus = response.getStatusCode(); - if (httpStatus.is4xxClientError()) { - context.set(ERROR_STATUS_CODE, httpStatus.value()); - } - } - protected RibbonCommandContext buildCommandContext(RequestContext context) { HttpServletRequest request = context.getRequest(); 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 ae1d3f43..4b8e9020 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 @@ -175,7 +175,6 @@ public class SimpleHostRoutingFilter extends ZuulFilter { HttpResponse response = forward(this.httpClient, verb, uri, request, headers, params, requestEntity); setResponse(response); - setErrorCodeFor4xx(context, response); } catch (Exception ex) { context.set(ERROR_STATUS_CODE, @@ -185,13 +184,6 @@ public class SimpleHostRoutingFilter extends ZuulFilter { return null; } - private void setErrorCodeFor4xx(RequestContext context, HttpResponse response) { - HttpStatus httpStatus = HttpStatus.valueOf(response.getStatusLine().getStatusCode()); - if (httpStatus.is4xxClientError()) { - context.set(ERROR_STATUS_CODE, httpStatus.value()); - } - } - protected PoolingHttpClientConnectionManager newConnectionManager() { try { final SSLContext sslContext = SSLContext.getInstance("SSL"); diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/SampleZuulProxyApplicationTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/SampleZuulProxyApplicationTests.java index 35eb1281..dc5bb9ee 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/SampleZuulProxyApplicationTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/SampleZuulProxyApplicationTests.java @@ -113,11 +113,13 @@ public class SampleZuulProxyApplicationTests extends ZuulProxyTestBase { public void simpleHostRouteWithNonExistentUrl() { this.routes.addRoute("/self/**", "http://localhost:" + this.port + "/"); this.endpoint.reset(); + String uri = "/self/nonExistentUrl"; + this.myErrorController.setUriToMatch(uri); ResponseEntity result = new TestRestTemplate().exchange( - "http://localhost:" + this.port + "/self/nonExistentUrl", HttpMethod.GET, + "http://localhost:" + this.port + uri, HttpMethod.GET, new HttpEntity<>((Void) null), String.class); assertEquals(HttpStatus.NOT_FOUND, result.getStatusCode()); - assertTrue(this.myErrorController.wasControllerUsed()); + assertFalse(this.myErrorController.wasControllerUsed()); } @Test diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/ZuulProxyTestBase.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/ZuulProxyTestBase.java index 33fc36bc..85afec6b 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/ZuulProxyTestBase.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/ZuulProxyTestBase.java @@ -43,7 +43,6 @@ import com.netflix.zuul.context.RequestContext; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; /** * @author Spencer Gibb @@ -155,8 +154,10 @@ public abstract class ZuulProxyTestBase { @Test public void ribbonRouteWithSpace() { + String uri = "/simple/spa ce"; + this.myErrorController.setUriToMatch(uri); ResponseEntity result = new TestRestTemplate().exchange( - "http://localhost:" + this.port + "/simple/spa ce", HttpMethod.GET, + "http://localhost:" + this.port + uri, HttpMethod.GET, new HttpEntity<>((Void) null), String.class); assertEquals(HttpStatus.OK, result.getStatusCode()); assertEquals("Hello space", result.getBody()); @@ -165,11 +166,13 @@ public abstract class ZuulProxyTestBase { @Test public void ribbonRouteWithNonExistentUri() { + String uri = "/simple/nonExistent"; + this.myErrorController.setUriToMatch(uri); ResponseEntity result = new TestRestTemplate().exchange( - "http://localhost:" + this.port + "/simple/nonExistent", HttpMethod.GET, + "http://localhost:" + this.port + uri, HttpMethod.GET, new HttpEntity<>((Void) null), String.class); assertEquals(HttpStatus.NOT_FOUND, result.getStatusCode()); - assertTrue(myErrorController.wasControllerUsed()); + assertFalse(myErrorController.wasControllerUsed()); } @Test @@ -325,6 +328,7 @@ class AnotherRibbonClientConfiguration { } class MyErrorController extends BasicErrorController { + ThreadLocal uriToMatch = new ThreadLocal<>(); AtomicBoolean controllerUsed = new AtomicBoolean(); @@ -334,10 +338,19 @@ class MyErrorController extends BasicErrorController { @Override public ResponseEntity> error(HttpServletRequest request) { - controllerUsed.set(true); + String errorUri = (String) request.getAttribute("javax.servlet.error.request_uri"); + + if (errorUri != null && errorUri.equals(this.uriToMatch.get())) { + controllerUsed.set(true); + } + this.uriToMatch.remove(); return super.error(request); } + public void setUriToMatch(String uri) { + this.uriToMatch.set(uri); + } + public boolean wasControllerUsed() { return this.controllerUsed.get(); } @@ -345,4 +358,4 @@ class MyErrorController extends BasicErrorController { public void clear() { this.controllerUsed.set(false); } -} \ No newline at end of file +}