Merge remote-tracking branch 'origin/1.4.x'

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RetryableRibbonLoadBalancingHttpClient.java

@@ -40,7 +40,6 @@ import org.springframework.retry.backoff.BackOffPolicy;
 import org.springframework.retry.backoff.NoBackOffPolicy;
 import org.springframework.retry.policy.NeverRetryPolicy;
 import org.springframework.retry.support.RetryTemplate;
-import org.springframework.web.util.UriComponentsBuilder;
 import com.netflix.client.RequestSpecificRetryHandler;
 import com.netflix.client.RetryHandler;
 import com.netflix.client.config.CommonClientConfigKey;
@@ -85,75 +84,55 @@ public class RetryableRibbonLoadBalancingHttpClient extends RibbonLoadBalancingH
 	}
 
 	@Override
-	public RibbonApacheHttpResponse execute(final RibbonApacheHttpRequest request,
-			final IClientConfig configOverride) throws Exception {
+	public RibbonApacheHttpResponse execute(final RibbonApacheHttpRequest request, final IClientConfig configOverride) throws Exception {
 		final RequestConfig.Builder builder = RequestConfig.custom();
 		IClientConfig config = configOverride != null ? configOverride : this.config;
-		builder.setConnectTimeout(
-				config.get(CommonClientConfigKey.ConnectTimeout, this.connectTimeout));
-		builder.setSocketTimeout(
-				config.get(CommonClientConfigKey.ReadTimeout, this.readTimeout));
-		builder.setRedirectsEnabled(
-				config.get(CommonClientConfigKey.FollowRedirects, this.followRedirects));
+		builder.setConnectTimeout(config.get(
+				CommonClientConfigKey.ConnectTimeout, this.connectTimeout));
+		builder.setSocketTimeout(config.get(
+				CommonClientConfigKey.ReadTimeout, this.readTimeout));
+		builder.setRedirectsEnabled(config.get(
+				CommonClientConfigKey.FollowRedirects, this.followRedirects));
 
 		final RequestConfig requestConfig = builder.build();
-		final LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryPolicyFactory
-				.create(this.getClientName(), this);
+		final LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryPolicyFactory.create(this.getClientName(), this);
 		RetryCallback retryCallback = new RetryCallback() {
 			@Override
-			public RibbonApacheHttpResponse doWithRetry(RetryContext context)
-					throws Exception {
-				// on retries the policy will choose the server and set it in the context
-				// extract the server and update the request being made
+			public RibbonApacheHttpResponse doWithRetry(RetryContext context) throws Exception {
+				//on retries the policy will choose the server and set it in the context
+				//extract the server and update the request being made
 				RibbonApacheHttpRequest newRequest = request;
-				if (context instanceof LoadBalancedRetryContext) {
-					ServiceInstance service = ((LoadBalancedRetryContext) context)
-							.getServiceInstance();
-					if (service != null) {
-						// Reconstruct the request URI using the host and port set in the
-						// retry context
-						newRequest = newRequest.withNewUri(new URI(
-								service.getUri().getScheme(),
-								newRequest.getURI().getUserInfo(), service.getHost(),
-								service.getPort(), newRequest.getURI().getPath(),
-								newRequest.getURI().getQuery(),
+				if(context instanceof LoadBalancedRetryContext) {
+					ServiceInstance service = ((LoadBalancedRetryContext)context).getServiceInstance();
+					if(service != null) {
+						//Reconstruct the request URI using the host and port set in the retry context
+						newRequest = newRequest.withNewUri(new URI(service.getUri().getScheme(),
+								newRequest.getURI().getUserInfo(), service.getHost(), service.getPort(),
+								newRequest.getURI().getPath(), newRequest.getURI().getQuery(),
 								newRequest.getURI().getFragment()));
 					}
 				}
-				if (isSecure(configOverride)) {
-					final URI secureUri = UriComponentsBuilder
-							.fromUri(newRequest.getUri()).scheme("https").build().toUri();
-					newRequest = newRequest.withNewUri(secureUri);
-				}
+				newRequest = getSecureRequest(request, configOverride);
 				HttpUriRequest httpUriRequest = newRequest.toRequest(requestConfig);
-				final HttpResponse httpResponse = RetryableRibbonLoadBalancingHttpClient.this.delegate
-						.execute(httpUriRequest);
-				if (retryPolicy.retryableStatusCode(
-						httpResponse.getStatusLine().getStatusCode())) {
-					if (CloseableHttpResponse.class.isInstance(httpResponse)) {
-						((CloseableHttpResponse) httpResponse).close();
+				final HttpResponse httpResponse = RetryableRibbonLoadBalancingHttpClient.this.delegate.execute(httpUriRequest);
+				if(retryPolicy.retryableStatusCode(httpResponse.getStatusLine().getStatusCode())) {
+					if(CloseableHttpResponse.class.isInstance(httpResponse)) {
+						((CloseableHttpResponse)httpResponse).close();
 					}
-					throw new RetryableStatusCodeException(
-							RetryableRibbonLoadBalancingHttpClient.this.clientName,
+					throw new RetryableStatusCodeException(RetryableRibbonLoadBalancingHttpClient.this.clientName,
 							httpResponse.getStatusLine().getStatusCode());
 				}
-				return new RibbonApacheHttpResponse(httpResponse,
-						httpUriRequest.getURI());
+				return new RibbonApacheHttpResponse(httpResponse, httpUriRequest.getURI());
 			}
 		};
 		return this.executeWithRetry(request, retryPolicy, retryCallback);
 	}
 
-	private RibbonApacheHttpResponse executeWithRetry(RibbonApacheHttpRequest request,
-			LoadBalancedRetryPolicy retryPolicy,
-			RetryCallback<RibbonApacheHttpResponse, IOException> callback)
-			throws Exception {
+	private RibbonApacheHttpResponse executeWithRetry(RibbonApacheHttpRequest request, LoadBalancedRetryPolicy retryPolicy, RetryCallback<RibbonApacheHttpResponse, IOException> callback) throws Exception {
 		RetryTemplate retryTemplate = new RetryTemplate();
-		boolean retryable = request.getContext() == null ? true
-				: BooleanUtils.toBooleanDefaultIfNull(request.getContext().getRetryable(),
-						true);
-		retryTemplate.setRetryPolicy(retryPolicy == null || !retryable
-				? new NeverRetryPolicy()
+		boolean retryable = request.getContext() == null ? true :
+				BooleanUtils.toBooleanDefaultIfNull(request.getContext().getRetryable(), true);
+		retryTemplate.setRetryPolicy(retryPolicy == null || !retryable ? new NeverRetryPolicy()
 				: new RetryPolicy(request, retryPolicy, this, this.getClientName()));
 		BackOffPolicy backOffPolicy = loadBalancedBackOffPolicyFactory.createBackOffPolicy(this.getClientName());
 		retryTemplate.setBackOffPolicy(backOffPolicy == null ? new NoBackOffPolicy() : backOffPolicy);
@@ -167,14 +146,12 @@ public class RetryableRibbonLoadBalancingHttpClient extends RibbonLoadBalancingH
 	}
 
 	@Override
-	public RequestSpecificRetryHandler getRequestSpecificRetryHandler(
-			RibbonApacheHttpRequest request, IClientConfig requestConfig) {
+	public RequestSpecificRetryHandler getRequestSpecificRetryHandler(RibbonApacheHttpRequest request, IClientConfig requestConfig) {
 		return new RequestSpecificRetryHandler(false, false, RetryHandler.DEFAULT, null);
 	}
 
 	static class RetryPolicy extends FeignRetryPolicy {
-		public RetryPolicy(HttpRequest request, LoadBalancedRetryPolicy policy,
-				ServiceInstanceChooser serviceInstanceChooser, String serviceName) {
+		public RetryPolicy(HttpRequest request, LoadBalancedRetryPolicy policy, ServiceInstanceChooser serviceInstanceChooser, String serviceName) {
 			super(request, policy, serviceInstanceChooser, serviceName);
 		}
 	}

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClient.java

@@ -77,11 +77,7 @@ public class RibbonLoadBalancingHttpClient extends
 				config.get(CommonClientConfigKey.FollowRedirects, this.followRedirects));
 
 		final RequestConfig requestConfig = builder.build();
-		if (isSecure(configOverride)) {
-			final URI secureUri = UriComponentsBuilder.fromUri(request.getUri())
-					.scheme("https").build().toUri();
-			request = request.withNewUri(secureUri);
-		}
+		request = getSecureRequest(request, configOverride);
 		final HttpUriRequest httpUriRequest = request.toRequest(requestConfig);
 		final HttpResponse httpResponse = this.delegate.execute(httpUriRequest);
 		return new RibbonApacheHttpResponse(httpResponse, httpUriRequest.getURI());
@@ -100,4 +96,13 @@ public class RibbonLoadBalancingHttpClient extends
 		return new RequestSpecificRetryHandler(false, false, RetryHandler.DEFAULT,
 				requestConfig);
 	}
+
+	protected RibbonApacheHttpRequest getSecureRequest(RibbonApacheHttpRequest request, IClientConfig configOverride) {
+		if (isSecure(configOverride)) {
+			final URI secureUri = UriComponentsBuilder.fromUri(request.getUri())
+					.scheme("https").build(true).toUri();
+			return request.withNewUri(secureUri);
+		}
+		return request;
+	}
 }

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java

@@ -17,8 +17,10 @@
 
 package org.springframework.cloud.netflix.ribbon.apache;
 
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.net.URI;
+import java.util.ArrayList;
 import org.apache.http.HttpResponse;
 import org.apache.http.StatusLine;
 import org.apache.http.client.HttpClient;
@@ -32,6 +34,8 @@ import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
 import org.springframework.cloud.client.loadbalancer.LoadBalancedBackOffPolicyFactory;
+
+import org.mockito.ArgumentMatcher;
 import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicyFactory;
 import org.springframework.cloud.commons.httpclient.HttpClientConfiguration;
 import org.springframework.cloud.netflix.ribbon.RibbonAutoConfiguration;
@@ -40,6 +44,8 @@ import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancedRetryPolicyFac
 import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancerContext;
 import org.springframework.cloud.netflix.ribbon.ServerIntrospector;
 import org.springframework.cloud.netflix.ribbon.SpringClientFactory;
+import org.springframework.cloud.netflix.ribbon.support.RibbonCommandContext;
+import org.springframework.cloud.netflix.ribbon.support.RibbonRequestCustomizer;
 import org.springframework.context.annotation.AnnotationConfigApplicationContext;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
@@ -49,6 +55,7 @@ import org.springframework.retry.backoff.BackOffContext;
 import org.springframework.retry.backoff.BackOffInterruptedException;
 import org.springframework.retry.backoff.BackOffPolicy;
 import org.springframework.test.util.ReflectionTestUtils;
+import org.springframework.util.LinkedMultiValueMap;
 
 import com.netflix.client.DefaultLoadBalancerRetryHandler;
 import com.netflix.client.RetryHandler;
@@ -65,6 +72,7 @@ import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.argThat;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
@@ -209,6 +217,16 @@ public class RibbonLoadBalancingHttpClientTests {
 																	   String serviceName, String host, int port,
 																	   CloseableHttpClient delegate, ILoadBalancer lb, String statusCodes,
 																	   LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory) throws Exception {
+		return setupClientForRetry(retriesNextServer, retriesSameServer, retryable, retryOnAllOps, serviceName, host, port,
+				delegate, lb, statusCodes, loadBalancedBackOffPolicyFactory, false);
+	}
+
+	private RetryableRibbonLoadBalancingHttpClient setupClientForRetry(int retriesNextServer, int retriesSameServer,
+																	   boolean retryable, boolean retryOnAllOps,
+																	   String serviceName, String host, int port,
+																	   CloseableHttpClient delegate, ILoadBalancer lb, String statusCodes,
+																	   LoadBalancedBackOffPolicyFactory loadBalancedBackOffPolicyFactory,
+																	   boolean isSecure) throws Exception {
 		ServerIntrospector introspector = mock(ServerIntrospector.class);
 		RetryHandler retryHandler = new DefaultLoadBalancerRetryHandler(retriesSameServer, retriesNextServer, retryable);
 		doReturn(new Server(host, port)).when(lb).chooseServer(eq(serviceName));
@@ -217,6 +235,7 @@ public class RibbonLoadBalancingHttpClientTests {
 		clientConfig.set(CommonClientConfigKey.MaxAutoRetriesNextServer, retriesNextServer);
 		clientConfig.set(CommonClientConfigKey.MaxAutoRetries, retriesSameServer);
 		clientConfig.set(RibbonLoadBalancedRetryPolicy.RETRYABLE_STATUS_CODES, statusCodes);
+		clientConfig.set(CommonClientConfigKey.IsSecure, isSecure);
 		clientConfig.setClientName(serviceName);
 		RibbonLoadBalancerContext context = new RibbonLoadBalancerContext(lb, clientConfig, retryHandler);
 		SpringClientFactory clientFactory = mock(SpringClientFactory.class);
@@ -333,6 +352,83 @@ public class RibbonLoadBalancingHttpClientTests {
 	}
 
 	@Test
+	public void testDoubleEncoding() throws Exception {
+		String serviceName = "foo";
+		String host = serviceName;
+		int port = 80;
+		HttpMethod method = HttpMethod.GET;
+		final URI uri = new URI("https://" + host + ":" + port + "/a%2Bb");
+		DefaultClientConfigImpl clientConfig = new DefaultClientConfigImpl();
+		clientConfig.setClientName(serviceName);
+		ServerIntrospector introspector = mock(ServerIntrospector.class);
+		RibbonCommandContext context = new RibbonCommandContext(serviceName, method.toString(), uri.toString(), false,
+				new LinkedMultiValueMap<String, String>(), new LinkedMultiValueMap<String, String>(),
+				new ByteArrayInputStream(new String("bar").getBytes()),
+				new ArrayList<RibbonRequestCustomizer>());
+		RibbonApacheHttpRequest request = new RibbonApacheHttpRequest(context);
+		CloseableHttpClient delegate = mock(CloseableHttpClient.class);
+		final CloseableHttpResponse response = mock(CloseableHttpResponse.class);
+		StatusLine statusLine = mock(StatusLine.class);
+		doReturn(200).when(statusLine).getStatusCode();
+		doReturn(statusLine).when(response).getStatusLine();
+		doReturn(response).
+				when(delegate).execute(any(HttpUriRequest.class));
+		RibbonLoadBalancingHttpClient client = new RibbonLoadBalancingHttpClient(delegate, clientConfig, introspector);
+		RibbonApacheHttpResponse returnedResponse = client.execute(request, null);
+		verify(response, times(0)).close();
+		verify(delegate, times(1)).execute(argThat(new ArgumentMatcher<HttpUriRequest>() {
+			@Override
+			public boolean matches(HttpUriRequest argument) {
+				if(argument instanceof HttpUriRequest) {
+					HttpUriRequest arg = (HttpUriRequest)argument;
+					return arg.getURI().equals(uri);
+				}
+				return false;
+			}
+		}));
+	}
+
+	@Test
+	public void testDoubleEncodingWithRetry() throws Exception {
+		int retriesNextServer = 0;
+		int retriesSameServer = 0;
+		boolean retryable = true;
+		boolean retryOnAllOps = true;
+		String serviceName = "foo";
+		String host = serviceName;
+		int port = 80;
+		HttpMethod method = HttpMethod.GET;
+		final URI uri = new URI("https://" + host + ":" + port + "/a%2Bb");
+		RibbonCommandContext context = new RibbonCommandContext(serviceName, method.toString(), uri.toString(), true,
+				new LinkedMultiValueMap<String, String>(), new LinkedMultiValueMap<String, String>(),
+				new ByteArrayInputStream(new String("bar").getBytes()),
+				new ArrayList<RibbonRequestCustomizer>());
+		RibbonApacheHttpRequest request = new RibbonApacheHttpRequest(context);
+		CloseableHttpClient delegate = mock(CloseableHttpClient.class);
+		final CloseableHttpResponse response = mock(CloseableHttpResponse.class);
+		StatusLine statusLine = mock(StatusLine.class);
+		doReturn(200).when(statusLine).getStatusCode();
+		doReturn(statusLine).when(response).getStatusLine();
+		doReturn(response).
+				when(delegate).execute(any(HttpUriRequest.class));
+		ILoadBalancer lb = mock(ILoadBalancer.class);
+		RetryableRibbonLoadBalancingHttpClient client = setupClientForRetry(retriesNextServer, retriesSameServer, retryable, retryOnAllOps,
+				serviceName, host, port, delegate, lb, "", loadBalancedBackOffPolicyFactory,true);
+		client.execute(request, null);
+		verify(response, times(0)).close();
+		verify(delegate, times(1)).execute(argThat(new ArgumentMatcher<HttpUriRequest>() {
+			@Override
+			public boolean matches(HttpUriRequest argument) {
+				if(argument instanceof HttpUriRequest) {
+					HttpUriRequest arg = (HttpUriRequest)argument;
+					return arg.getURI().equals(uri);
+				}
+				return false;
+			}
+		}));
+	}
+
+	@Test
 	public void testNoRetryOnPost() throws Exception {
 		int retriesNextServer = 1;
 		int retriesSameServer = 1;

spring-cloud-netflix-zuul/src/main/java/org/springframework/cloud/netflix/zuul/util/RequestContentDataExtractor.java

@@ -47,7 +47,7 @@ public class RequestContentDataExtractor {
 		for (Entry<String, String[]> entry : request.getParameterMap().entrySet()) {
 			String key = entry.getKey();
 
-			if (!queryParams.contains(key)) {
+			if (!queryParams.contains(key) && entry.getValue() != null) {
 				for (String value : entry.getValue()) {
 					builder.add(key, value);
 				}

spring-cloud-netflix-zuul/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilterTests.java

@@ -32,6 +32,7 @@ import java.util.zip.GZIPOutputStream;
 import javax.servlet.http.HttpServletResponse;
 
 import com.netflix.zuul.context.RequestContext;
+import com.netflix.zuul.monitoring.CounterFactory;
 import org.apache.http.HttpEntityEnclosingRequest;
 import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
@@ -44,6 +45,7 @@ import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
 import org.assertj.core.api.Assertions;
 import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.springframework.boot.SpringApplication;
@@ -59,6 +61,8 @@ import org.springframework.cloud.commons.httpclient.DefaultApacheHttpClientFacto
 import org.springframework.cloud.context.environment.EnvironmentChangeEvent;
 import org.springframework.cloud.netflix.zuul.filters.ProxyRequestHelper;
 import org.springframework.cloud.netflix.zuul.filters.ZuulProperties;
+import org.springframework.cloud.netflix.zuul.metrics.EmptyCounterFactory;
+import org.springframework.cloud.netflix.zuul.util.ZuulRuntimeException;
 import org.springframework.context.annotation.AnnotationConfigApplicationContext;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
@@ -98,11 +102,17 @@ public class SimpleHostRoutingFilterTests {
 	@LocalServerPort
 	private int port;
 
+	@Before
+	public void setup() {
+		CounterFactory.initialize(new EmptyCounterFactory());
+	}
+
 	@After
 	public void clear() {
 		if (this.context != null) {
 			this.context.close();
 		}
+		CounterFactory.initialize(null);
 	}
 
 	@Test
@@ -281,7 +291,7 @@ public class SimpleHostRoutingFilterTests {
 		assertEquals(100, getFilter().filterOrder());
 	}
 
-	@Test(expected = IllegalStateException.class)
+	@Test(expected = ZuulRuntimeException.class)
 	public void run() throws Exception {
 		setupContext();
 		MockHttpServletRequest request = new MockHttpServletRequest("POST", "/");