Merge pull request #694 from snussbaumer/gh-691

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

@@ -29,29 +29,27 @@ import com.netflix.client.AbstractLoadBalancerAwareClient;
 import com.netflix.client.RequestSpecificRetryHandler;
 import com.netflix.client.RetryHandler;
 import com.netflix.client.config.CommonClientConfigKey;
+import com.netflix.client.config.DefaultClientConfigImpl;
 import com.netflix.client.config.IClientConfig;
 import com.netflix.loadbalancer.ILoadBalancer;
 
-import lombok.Setter;
-
 /**
  * @author Christian Lohmann
  */
 public class RibbonLoadBalancingHttpClient
-		extends AbstractLoadBalancerAwareClient<RibbonApacheHttpRequest, RibbonApacheHttpResponse> {
+		extends
+		AbstractLoadBalancerAwareClient<RibbonApacheHttpRequest, RibbonApacheHttpResponse> {
 	private final HttpClient delegate = HttpClientBuilder.create().build();
 
-	@Setter
 	private int connectTimeout;
 
-	@Setter
 	private int readTimeout;
 
-	@Setter
 	private boolean secure;
 
-	@Setter
-	private IClientConfig clientConfig;
+	private boolean followRedirects;
+
+	private boolean okToRetryOnAllOperations;
 
 	public RibbonLoadBalancingHttpClient() {
 		super(null);
@@ -64,9 +62,28 @@ public class RibbonLoadBalancingHttpClient
 	}
 
 	@Override
+	public void initWithNiwsConfig(IClientConfig clientConfig) {
+		super.initWithNiwsConfig(clientConfig);
+		this.connectTimeout = clientConfig.getPropertyAsInteger(
+				CommonClientConfigKey.ConnectTimeout,
+				DefaultClientConfigImpl.DEFAULT_CONNECT_TIMEOUT);
+		this.readTimeout = clientConfig.getPropertyAsInteger(
+				CommonClientConfigKey.ReadTimeout,
+				DefaultClientConfigImpl.DEFAULT_READ_TIMEOUT);
+		this.secure = clientConfig.getPropertyAsBoolean(CommonClientConfigKey.IsSecure,
+				false);
+		this.followRedirects = clientConfig.getPropertyAsBoolean(
+				CommonClientConfigKey.FollowRedirects,
+				DefaultClientConfigImpl.DEFAULT_FOLLOW_REDIRECTS);
+		this.okToRetryOnAllOperations = clientConfig.getPropertyAsBoolean(
+				CommonClientConfigKey.OkToRetryOnAllOperations,
+				DefaultClientConfigImpl.DEFAULT_OK_TO_RETRY_ON_ALL_OPERATIONS);
+	}
+
+	@Override
 	public RequestSpecificRetryHandler getRequestSpecificRetryHandler(
 			final RibbonApacheHttpRequest request, final IClientConfig requestConfig) {
-		if (this.clientConfig.get(CommonClientConfigKey.OkToRetryOnAllOperations, false)) {
+		if (this.okToRetryOnAllOperations) {
 			return new RequestSpecificRetryHandler(true, true, this.getRetryHandler(),
 					requestConfig);
 		}
@@ -90,10 +107,13 @@ public class RibbonLoadBalancingHttpClient
 					CommonClientConfigKey.ConnectTimeout, this.connectTimeout));
 			builder.setConnectionRequestTimeout(configOverride.get(
 					CommonClientConfigKey.ReadTimeout, this.readTimeout));
+			builder.setRedirectsEnabled(configOverride.get(
+					CommonClientConfigKey.FollowRedirects, this.followRedirects));
 		}
 		else {
 			builder.setConnectTimeout(this.connectTimeout);
 			builder.setConnectionRequestTimeout(this.readTimeout);
+			builder.setRedirectsEnabled(this.followRedirects);
 		}
 
 		final RequestConfig requestConfig = builder.build();
@@ -110,6 +130,7 @@ public class RibbonLoadBalancingHttpClient
 	}
 
 	private boolean isSecure(final IClientConfig config) {
-		return (config != null) ? config.get(CommonClientConfigKey.IsSecure) : secure;
+		return (config != null) ? config.get(CommonClientConfigKey.IsSecure)
+				: this.secure;
 	}
 }

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactory.java

@@ -16,13 +16,13 @@
 
 package org.springframework.cloud.netflix.zuul.filters.route.apache;
 
+import lombok.RequiredArgsConstructor;
+
 import org.springframework.cloud.netflix.ribbon.SpringClientFactory;
 import org.springframework.cloud.netflix.ribbon.apache.RibbonLoadBalancingHttpClient;
 import org.springframework.cloud.netflix.zuul.filters.route.RibbonCommandContext;
 import org.springframework.cloud.netflix.zuul.filters.route.RibbonCommandFactory;
 
-import lombok.RequiredArgsConstructor;
-
 /**
  * @author Christian Lohmann
  */
@@ -35,10 +35,9 @@ public class HttpClientRibbonCommandFactory implements
 	@Override
 	public HttpClientRibbonCommand create(final RibbonCommandContext context) {
 		final String serviceId = context.getServiceId();
-		final RibbonLoadBalancingHttpClient client = clientFactory.getClient(serviceId,
-				RibbonLoadBalancingHttpClient.class);
+		final RibbonLoadBalancingHttpClient client = this.clientFactory.getClient(
+				serviceId, RibbonLoadBalancingHttpClient.class);
 		client.setLoadBalancer(this.clientFactory.getLoadBalancer(serviceId));
-		client.setClientConfig(this.clientFactory.getClientConfig(serviceId));
 
 		final HttpClientRibbonCommand httpClientRibbonCommand = new HttpClientRibbonCommand(
 				serviceId, client, context.getVerb(), context.getUri(),

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

+/*
+ * Copyright 2013-2015 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
+ *
+ *      http://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.netflix.ribbon.apache;
+
+import org.apache.http.HttpResponse;
+import org.apache.http.client.HttpClient;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.HttpUriRequest;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.springframework.cloud.netflix.ribbon.SpringClientFactory;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import com.netflix.client.config.CommonClientConfigKey;
+import com.netflix.client.config.DefaultClientConfigImpl;
+import com.netflix.client.config.IClientConfig;
+
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertThat;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+/**
+ * @author Sébastien Nussbaumer
+ */
+public class RibbonLoadBalancingHttpClientTests {
+
+	@Test
+	public void testRequestConfigUseDefaultsNoOverride() throws Exception {
+		RequestConfig result = getBuiltRequestConfig(UseDefaults.class, null);
+
+		assertThat(result.isRedirectsEnabled(), is(false));
+	}
+
+	@Test
+	public void testRequestConfigDoNotFollowRedirectsNoOverride() throws Exception {
+		RequestConfig result = getBuiltRequestConfig(DoNotFollowRedirects.class, null);
+
+		assertThat(result.isRedirectsEnabled(), is(false));
+	}
+
+	@Test
+	public void testRequestConfigFollowRedirectsNoOverride() throws Exception {
+		RequestConfig result = getBuiltRequestConfig(FollowRedirects.class, null);
+
+		assertThat(result.isRedirectsEnabled(), is(true));
+	}
+
+	@Test
+	public void testRequestConfigDoNotFollowRedirectsOverrideWithFollowRedirects()
+			throws Exception {
+
+		DefaultClientConfigImpl override = new DefaultClientConfigImpl();
+		override.set(CommonClientConfigKey.FollowRedirects, true);
+		override.set(CommonClientConfigKey.IsSecure, false);
+
+		RequestConfig result = getBuiltRequestConfig(DoNotFollowRedirects.class, override);
+
+		assertThat(result.isRedirectsEnabled(), is(true));
+	}
+
+	@Test
+	public void testRequestConfigFollowRedirectsOverrideWithDoNotFollowRedirects()
+			throws Exception {
+
+		DefaultClientConfigImpl override = new DefaultClientConfigImpl();
+		override.set(CommonClientConfigKey.FollowRedirects, false);
+		override.set(CommonClientConfigKey.IsSecure, false);
+
+		RequestConfig result = getBuiltRequestConfig(FollowRedirects.class, override);
+
+		assertThat(result.isRedirectsEnabled(), is(false));
+	}
+
+	@Configuration
+	protected static class UseDefaults {
+
+	}
+
+	@Configuration
+	protected static class FollowRedirects {
+		@Bean
+		public IClientConfig clientConfig() {
+			DefaultClientConfigImpl config = new DefaultClientConfigImpl();
+			config.set(CommonClientConfigKey.FollowRedirects, true);
+			return config;
+		}
+	}
+
+	@Configuration
+	protected static class DoNotFollowRedirects {
+		@Bean
+		public IClientConfig clientConfig() {
+			DefaultClientConfigImpl config = new DefaultClientConfigImpl();
+			config.set(CommonClientConfigKey.FollowRedirects, false);
+			return config;
+		}
+	}
+
+	private RequestConfig getBuiltRequestConfig(Class<?> defaultConfigurationClass,
+			IClientConfig configOverride) throws Exception {
+
+		SpringClientFactory factory = new SpringClientFactory();
+		factory.setApplicationContext(new AnnotationConfigApplicationContext(
+				defaultConfigurationClass));
+		HttpClient delegate = mock(HttpClient.class);
+		RibbonLoadBalancingHttpClient client = factory.getClient("service",
+				RibbonLoadBalancingHttpClient.class);
+
+		ReflectionTestUtils.setField(client, "delegate", delegate);
+		given(delegate.execute(any(HttpUriRequest.class))).willReturn(
+				mock(HttpResponse.class));
+		RibbonApacheHttpRequest request = mock(RibbonApacheHttpRequest.class);
+		given(request.toRequest(any(RequestConfig.class))).willReturn(
+				mock(HttpUriRequest.class));
+
+		client.execute(request, configOverride);
+
+		ArgumentCaptor<RequestConfig> requestConfigCaptor = ArgumentCaptor
+				.forClass(RequestConfig.class);
+		verify(request).toRequest(requestConfigCaptor.capture());
+		return requestConfigCaptor.getValue();
+	}
+
+}