HTTPCLIENT-1285: fixed inconsistent handling of default request config; added test cases

git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1427178 13f79535-47bb-0310-9956-ffa450edef68

httpclient/src/main/java/org/apache/http/client/HttpClient.java

@@ -32,6 +32,7 @@ import java.io.IOException;
 import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
 import org.apache.http.HttpResponse;
+import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.HttpUriRequest;
 import org.apache.http.conn.ClientConnectionManager;
 import org.apache.http.impl.client.HttpClientBuilder;
@@ -113,7 +114,10 @@ public interface HttpClient {
      * dependent objects in this client.
      *
      * @return  the default parameters
+     *
+     * @deprecated (4.3) use {@link RequestConfig}.
      */
+    @Deprecated
     HttpParams getParams();
 
     /**

httpclient/src/main/java/org/apache/http/impl/client/InternalHttpClient.java

@@ -30,7 +30,6 @@ package org.apache.http.impl.client;
 import java.io.Closeable;
 import java.io.IOException;
 import java.util.List;
-import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import org.apache.commons.logging.Log;
@@ -63,8 +62,8 @@ import org.apache.http.conn.routing.HttpRoutePlanner;
 import org.apache.http.conn.scheme.SchemeRegistry;
 import org.apache.http.cookie.CookieSpecProvider;
 import org.apache.http.impl.client.execchain.ClientExecChain;
-import org.apache.http.params.BasicHttpParams;
 import org.apache.http.params.HttpParams;
+import org.apache.http.params.HttpParamsNames;
 import org.apache.http.protocol.BasicHttpContext;
 import org.apache.http.protocol.HttpContext;
 import org.apache.http.util.Args;
@@ -88,7 +87,6 @@ class InternalHttpClient extends CloseableHttpClient {
     private final CredentialsProvider credentialsProvider;
     private final RequestConfig defaultConfig;
     private final List<Closeable> closeables;
-    private final BasicHttpParams params;
 
     public InternalHttpClient(
             final ClientExecChain execChain,
@@ -113,7 +111,6 @@ class InternalHttpClient extends CloseableHttpClient {
         this.credentialsProvider = credentialsProvider;
         this.defaultConfig = defaultConfig;
         this.closeables = closeables;
-        this.params = new BasicHttpParams();
     }
 
     private HttpRoute determineRoute(
@@ -128,9 +125,7 @@ class InternalHttpClient extends CloseableHttpClient {
         return this.routePlanner.determineRoute(host, request, context);
     }
 
-    private HttpClientContext setupContext(final HttpContext localContext) {
+    private void setupContext(final HttpClientContext context) {
-        HttpClientContext context = HttpClientContext.adapt(
-                localContext != null ? localContext : new BasicHttpContext());
         if (context.getAttribute(ClientContext.TARGET_AUTH_STATE) == null) {
             context.setAttribute(ClientContext.TARGET_AUTH_STATE, new AuthState());
         }
@@ -149,7 +144,9 @@ class InternalHttpClient extends CloseableHttpClient {
         if (context.getAttribute(ClientContext.CREDS_PROVIDER) == null) {
             context.setAttribute(ClientContext.CREDS_PROVIDER, this.credentialsProvider);
         }
-        return context;
+        if (context.getAttribute(ClientContext.REQUEST_CONFIG) == null) {
+            context.setAttribute(ClientContext.REQUEST_CONFIG, this.defaultConfig);
+        }
     }
 
     @Override
@@ -164,34 +161,33 @@ class InternalHttpClient extends CloseableHttpClient {
         }
         try {
             HttpRequestWrapper wrapper = HttpRequestWrapper.wrap(request);
-            HttpClientContext localcontext = setupContext(context);
+            HttpClientContext localcontext = HttpClientContext.adapt(
-            HttpRoute route = determineRoute(target, wrapper, localcontext);
+                    context != null ? context : new BasicHttpContext());
             RequestConfig config = null;
             if (request instanceof Configurable) {
                 config = ((Configurable) request).getConfig();
             }
             if (config == null) {
-                config = this.defaultConfig;
+                HttpParams params = request.getParams();
+                if (params instanceof HttpParamsNames) {
+                    if (!((HttpParamsNames) params).getNames().isEmpty()) {
+                        config = HttpClientParamConfig.getRequestConfig(params);
                     }
-            if (config == null) {
+                } else {
-                Set<String> names = params.getNames();
-                if (!names.isEmpty()) {
                     config = HttpClientParamConfig.getRequestConfig(params);
                 }
             }
             if (config != null) {
                 localcontext.setRequestConfig(config);
             }
+            setupContext(localcontext);
+            HttpRoute route = determineRoute(target, wrapper, localcontext);
             return this.execChain.execute(route, wrapper, localcontext, execAware);
         } catch (HttpException httpException) {
             throw new ClientProtocolException(httpException);
         }
     }
 
-    public HttpParams getParams() {
-        return this.params;
-    }
-
     public void close() {
         this.connManager.shutdown();
         if (this.closeables != null) {
@@ -205,6 +201,10 @@ class InternalHttpClient extends CloseableHttpClient {
         }
     }
 
+    public HttpParams getParams() {
+        throw new UnsupportedOperationException();
+    }
+
     public ClientConnectionManager getConnectionManager() {
 
         return new ClientConnectionManager() {

httpclient/src/test/java/org/apache/http/impl/client/TestCloseableHttpClient.java

@@ -0,0 +1,198 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation.  For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.http.impl.client;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import junit.framework.Assert;
+
+import org.apache.http.HttpEntity;
+import org.apache.http.HttpHost;
+import org.apache.http.HttpRequest;
+import org.apache.http.HttpResponse;
+import org.apache.http.client.ClientProtocolException;
+import org.apache.http.client.ResponseHandler;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.protocol.HttpContext;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+/**
+ *  Simple tests for {@link CloseableHttpClient}.
+ */
+public class TestCloseableHttpClient {
+
+    static abstract class NoopCloseableHttpClient extends CloseableHttpClient {
+
+        @Override
+        protected CloseableHttpResponse doExecute(
+                final HttpHost target,
+                final HttpRequest request,
+                final HttpContext context) throws IOException, ClientProtocolException {
+            return null;
+        }
+
+    }
+
+    private NoopCloseableHttpClient client;
+    private InputStream content;
+    private HttpEntity entity;
+    private CloseableHttpResponse response;
+
+    @Before
+    public void setup() throws Exception {
+        content = Mockito.mock(InputStream.class);
+        entity = Mockito.mock(HttpEntity.class);
+        response = Mockito.mock(CloseableHttpResponse.class);
+        Mockito.when(entity.getContent()).thenReturn(content);
+        Mockito.when(entity.isStreaming()).thenReturn(Boolean.TRUE);
+        Mockito.when(response.getEntity()).thenReturn(entity);
+        client = Mockito.mock(NoopCloseableHttpClient.class, Mockito.CALLS_REAL_METHODS);
+    }
+
+    @Test
+    public void testExecuteRequestAbsoluteURI() throws Exception {
+        HttpGet httpget = new HttpGet("https://somehost:444/stuff");
+        client.execute(httpget);
+
+        Mockito.verify(client).doExecute(
+                Mockito.eq(new HttpHost("somehost", 444, "https")),
+                Mockito.same(httpget),
+                (HttpContext) Mockito.isNull());
+    }
+
+    @Test
+    public void testExecuteRequestRelativeURI() throws Exception {
+        HttpGet httpget = new HttpGet("/stuff");
+        client.execute(httpget);
+
+        Mockito.verify(client).doExecute(
+                (HttpHost) Mockito.isNull(),
+                Mockito.same(httpget),
+                (HttpContext) Mockito.isNull());
+    }
+
+    @Test
+    public void testExecuteRequestInvalidHost() throws Exception {
+        HttpGet httpget = new HttpGet("http://_/stuff");
+        client.execute(httpget);
+
+        Mockito.verify(client).doExecute(
+                Mockito.eq(new HttpHost("_", -1, "http")),
+                Mockito.same(httpget),
+                (HttpContext) Mockito.isNull());
+    }
+
+    @Test(expected=ClientProtocolException.class)
+    public void testExecuteRequestInvalidHost2() throws Exception {
+        HttpGet httpget = new HttpGet("http://@/stuff");
+        client.execute(httpget);
+    }
+
+    @Test
+    public void testExecuteRequest() throws Exception {
+        HttpGet httpget = new HttpGet("https://somehost:444/stuff");
+
+        Mockito.when(client.doExecute(
+                new HttpHost("somehost", 444, "https"), httpget, null)).thenReturn(response);
+
+        CloseableHttpResponse result = client.execute(httpget);
+        Assert.assertSame(response, result);
+    }
+
+    @Test
+    @SuppressWarnings("unchecked")
+    public void testExecuteRequestHandleResponse() throws Exception {
+        HttpGet httpget = new HttpGet("https://somehost:444/stuff");
+
+        Mockito.when(client.doExecute(
+                new HttpHost("somehost", 444, "https"), httpget, null)).thenReturn(response);
+
+        ResponseHandler<HttpResponse> handler = Mockito.mock(ResponseHandler.class);
+
+        client.execute(httpget, handler);
+
+        Mockito.verify(client).doExecute(
+                Mockito.eq(new HttpHost("somehost", 444, "https")),
+                Mockito.same(httpget),
+                (HttpContext) Mockito.isNull());
+        Mockito.verify(handler).handleResponse(response);
+        Mockito.verify(content).close();
+    }
+
+    @Test(expected=IOException.class)
+    @SuppressWarnings("unchecked")
+    public void testExecuteRequestHandleResponseIOException() throws Exception {
+        HttpGet httpget = new HttpGet("https://somehost:444/stuff");
+
+        Mockito.when(client.doExecute(
+                new HttpHost("somehost", 444, "https"), httpget, null)).thenReturn(response);
+
+        ResponseHandler<HttpResponse> handler = Mockito.mock(ResponseHandler.class);
+
+        Mockito.when(handler.handleResponse(response)).thenThrow(new IOException());
+
+        try {
+            client.execute(httpget, handler);
+        } catch (IOException ex) {
+            Mockito.verify(client).doExecute(
+                    Mockito.eq(new HttpHost("somehost", 444, "https")),
+                    Mockito.same(httpget),
+                    (HttpContext) Mockito.isNull());
+            Mockito.verify(content).close();
+            throw ex;
+        }
+    }
+
+    @Test(expected=RuntimeException.class)
+    @SuppressWarnings("unchecked")
+    public void testExecuteRequestHandleResponseHttpException() throws Exception {
+        HttpGet httpget = new HttpGet("https://somehost:444/stuff");
+
+        Mockito.when(client.doExecute(
+                new HttpHost("somehost", 444, "https"), httpget, null)).thenReturn(response);
+
+        ResponseHandler<HttpResponse> handler = Mockito.mock(ResponseHandler.class);
+
+        Mockito.when(handler.handleResponse(response)).thenThrow(new RuntimeException());
+
+        try {
+            client.execute(httpget, handler);
+        } catch (RuntimeException ex) {
+            Mockito.verify(client).doExecute(
+                    Mockito.eq(new HttpHost("somehost", 444, "https")),
+                    Mockito.same(httpget),
+                    (HttpContext) Mockito.isNull());
+            Mockito.verify(content).close();
+            throw ex;
+        }
+    }
+
+}

httpclient/src/test/java/org/apache/http/impl/client/TestInternalHttpClient.java

@@ -0,0 +1,223 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation.  For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.http.impl.client;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.Arrays;
+
+import junit.framework.Assert;
+
+import org.apache.http.HttpException;
+import org.apache.http.HttpHost;
+import org.apache.http.auth.AuthSchemeProvider;
+import org.apache.http.client.ClientProtocolException;
+import org.apache.http.client.CookieStore;
+import org.apache.http.client.CredentialsProvider;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.methods.HttpRequestWrapper;
+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.config.Lookup;
+import org.apache.http.conn.HttpClientConnectionManager;
+import org.apache.http.conn.routing.HttpRoute;
+import org.apache.http.conn.routing.HttpRoutePlanner;
+import org.apache.http.cookie.CookieSpecProvider;
+import org.apache.http.impl.client.execchain.ClientExecChain;
+import org.apache.http.params.CoreConnectionPNames;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+
+/**
+ *  Simple tests for {@link InternalHttpClient}.
+ */
+@SuppressWarnings("deprecation")
+public class TestInternalHttpClient {
+
+    private ClientExecChain execChain;
+    private HttpClientConnectionManager connManager;
+    private HttpRoutePlanner routePlanner;
+    private Lookup<CookieSpecProvider> cookieSpecRegistry;
+    private Lookup<AuthSchemeProvider> authSchemeRegistry;
+    private CookieStore cookieStore;
+    private CredentialsProvider credentialsProvider;
+    private RequestConfig defaultConfig;
+    private Closeable closeable1;
+    private Closeable closeable2;
+
+    private InternalHttpClient client;
+
+    @SuppressWarnings("unchecked")
+    @Before
+    public void setup() throws Exception {
+        execChain = Mockito.mock(ClientExecChain.class);
+        connManager = Mockito.mock(HttpClientConnectionManager.class);
+        routePlanner = Mockito.mock(HttpRoutePlanner.class);
+        cookieSpecRegistry = Mockito.mock(Lookup.class);
+        authSchemeRegistry = Mockito.mock(Lookup.class);
+        cookieStore = Mockito.mock(CookieStore.class);
+        credentialsProvider = Mockito.mock(CredentialsProvider.class);
+        defaultConfig = RequestConfig.custom().build();
+        closeable1 = Mockito.mock(Closeable.class);
+        closeable2 = Mockito.mock(Closeable.class);
+
+        client = new InternalHttpClient(execChain, connManager, routePlanner,
+                cookieSpecRegistry, authSchemeRegistry, cookieStore, credentialsProvider,
+                defaultConfig, Arrays.asList(closeable1, closeable2));
+
+    }
+
+    @Test
+    public void testExecute() throws Exception {
+        HttpGet httpget = new HttpGet("http://somehost/stuff");
+        HttpRoute route = new HttpRoute(new HttpHost("somehost"));
+
+        ArgumentCaptor<HttpRequestWrapper> argcap = ArgumentCaptor.forClass(HttpRequestWrapper.class);
+        Mockito.when(routePlanner.determineRoute(
+                Mockito.eq(new HttpHost("somehost")),
+                argcap.capture(),
+                Mockito.<HttpClientContext>any())).thenReturn(route);
+
+        client.execute(httpget);
+
+        Assert.assertNotNull(argcap.getValue());
+        Assert.assertSame(httpget, argcap.getValue().getOriginal());
+
+        Mockito.verify(execChain).execute(
+                Mockito.same(route),
+                Mockito.<HttpRequestWrapper>any(),
+                Mockito.<HttpClientContext>any(),
+                Mockito.same(httpget));
+    }
+
+    @Test(expected=ClientProtocolException.class)
+    public void testExecuteHttpException() throws Exception {
+        HttpGet httpget = new HttpGet("http://somehost/stuff");
+        HttpRoute route = new HttpRoute(new HttpHost("somehost"));
+
+        Mockito.when(routePlanner.determineRoute(
+                Mockito.eq(new HttpHost("somehost")),
+                Mockito.<HttpRequestWrapper>any(),
+                Mockito.<HttpClientContext>any())).thenReturn(route);
+        Mockito.when(execChain.execute(
+                Mockito.same(route),
+                Mockito.<HttpRequestWrapper>any(),
+                Mockito.<HttpClientContext>any(),
+                Mockito.same(httpget))).thenThrow(new HttpException());
+
+        client.execute(httpget);
+    }
+
+    @Test
+    public void testExecuteDefaultContext() throws Exception {
+        HttpGet httpget = new HttpGet("http://somehost/stuff");
+        HttpClientContext context = HttpClientContext.create();
+        client.execute(httpget, context);
+
+        Assert.assertSame(cookieSpecRegistry, context.getCookieSpecRegistry());
+        Assert.assertSame(authSchemeRegistry, context.getAuthSchemeRegistry());
+        Assert.assertSame(cookieStore, context.getCookieStore());
+        Assert.assertSame(credentialsProvider, context.getCredentialsProvider());
+        Assert.assertSame(defaultConfig, context.getRequestConfig());
+    }
+
+    @Test
+    public void testExecuteRequestConfig() throws Exception {
+        HttpGet httpget = new HttpGet("http://somehost/stuff");
+
+        RequestConfig config = RequestConfig.custom().build();
+        httpget.setConfig(config);
+        HttpClientContext context = HttpClientContext.create();
+        client.execute(httpget, context);
+
+        Assert.assertSame(config, context.getRequestConfig());
+    }
+
+    @Test
+    public void testExecuteRequestConfigDeprecated() throws Exception {
+        HttpGet httpget = new HttpGet("http://somehost/stuff");
+        httpget.getParams().setParameter(CoreConnectionPNames.CONNECTION_TIMEOUT, new Integer(123));
+
+        HttpClientContext context = HttpClientContext.create();
+        client.execute(httpget, context);
+
+        RequestConfig config = context.getRequestConfig();
+
+        Assert.assertNotSame(config, defaultConfig);
+        Assert.assertEquals(123, config.getConnectionRequestTimeout());
+    }
+
+    @SuppressWarnings("unchecked")
+    @Test
+    public void testExecuteLocalContext() throws Exception {
+        HttpGet httpget = new HttpGet("http://somehost/stuff");
+        HttpClientContext context = HttpClientContext.create();
+
+        Lookup<CookieSpecProvider> localCookieSpecRegistry = Mockito.mock(Lookup.class);
+        Lookup<AuthSchemeProvider> localAuthSchemeRegistry = Mockito.mock(Lookup.class);
+        CookieStore localCookieStore = Mockito.mock(CookieStore.class);
+        CredentialsProvider localCredentialsProvider = Mockito.mock(CredentialsProvider.class);
+        RequestConfig localConfig = RequestConfig.custom().build();
+
+        context.setCookieSpecRegistry(localCookieSpecRegistry);
+        context.setAuthSchemeRegistry(localAuthSchemeRegistry);
+        context.setCookieStore(localCookieStore);
+        context.setCredentialsProvider(localCredentialsProvider);
+        context.setRequestConfig(localConfig);
+
+        client.execute(httpget, context);
+
+        Assert.assertSame(localCookieSpecRegistry, context.getCookieSpecRegistry());
+        Assert.assertSame(localAuthSchemeRegistry, context.getAuthSchemeRegistry());
+        Assert.assertSame(localCookieStore, context.getCookieStore());
+        Assert.assertSame(localCredentialsProvider, context.getCredentialsProvider());
+        Assert.assertSame(localConfig, context.getRequestConfig());
+    }
+
+    @Test
+    public void testClientClose() throws Exception {
+        client.close();
+
+        Mockito.verify(connManager).shutdown();
+        Mockito.verify(closeable1).close();
+        Mockito.verify(closeable2).close();
+    }
+
+    @Test
+    public void testClientCloseIOException() throws Exception {
+        Mockito.doThrow(new IOException()).when(closeable1).close();
+
+        client.close();
+
+        Mockito.verify(connManager).shutdown();
+        Mockito.verify(closeable1).close();
+        Mockito.verify(closeable2).close();
+    }
+
+}