From de45d141cb72b699f1c5129d0bd4ec01fd2da30d Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Mon, 31 Dec 2012 17:04:50 +0000 Subject: [PATCH] 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 --- .../org/apache/http/client/HttpClient.java | 4 + .../http/impl/client/InternalHttpClient.java | 38 +-- .../impl/client/TestCloseableHttpClient.java | 198 ++++++++++++++++ .../impl/client/TestInternalHttpClient.java | 223 ++++++++++++++++++ 4 files changed, 444 insertions(+), 19 deletions(-) create mode 100644 httpclient/src/test/java/org/apache/http/impl/client/TestCloseableHttpClient.java create mode 100644 httpclient/src/test/java/org/apache/http/impl/client/TestInternalHttpClient.java diff --git a/httpclient/src/main/java/org/apache/http/client/HttpClient.java b/httpclient/src/main/java/org/apache/http/client/HttpClient.java index 1da5866d4..1324209db 100644 --- a/httpclient/src/main/java/org/apache/http/client/HttpClient.java +++ b/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(); /** diff --git a/httpclient/src/main/java/org/apache/http/impl/client/InternalHttpClient.java b/httpclient/src/main/java/org/apache/http/impl/client/InternalHttpClient.java index 4ee4c0e62..9566eb098 100644 --- a/httpclient/src/main/java/org/apache/http/impl/client/InternalHttpClient.java +++ b/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 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) { - HttpClientContext context = HttpClientContext.adapt( - localContext != null ? localContext : new BasicHttpContext()); + private void setupContext(final HttpClientContext context) { 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); - HttpRoute route = determineRoute(target, wrapper, localcontext); + HttpClientContext localcontext = HttpClientContext.adapt( + context != null ? context : new BasicHttpContext()); RequestConfig config = null; if (request instanceof Configurable) { config = ((Configurable) request).getConfig(); } if (config == null) { - config = this.defaultConfig; - } - if (config == null) { - Set names = params.getNames(); - if (!names.isEmpty()) { + HttpParams params = request.getParams(); + if (params instanceof HttpParamsNames) { + if (!((HttpParamsNames) params).getNames().isEmpty()) { + config = HttpClientParamConfig.getRequestConfig(params); + } + } else { 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() { diff --git a/httpclient/src/test/java/org/apache/http/impl/client/TestCloseableHttpClient.java b/httpclient/src/test/java/org/apache/http/impl/client/TestCloseableHttpClient.java new file mode 100644 index 000000000..4070cbfb1 --- /dev/null +++ b/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 + * . + * + */ +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 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 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 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; + } + } + +} diff --git a/httpclient/src/test/java/org/apache/http/impl/client/TestInternalHttpClient.java b/httpclient/src/test/java/org/apache/http/impl/client/TestInternalHttpClient.java new file mode 100644 index 000000000..2f1317d9e --- /dev/null +++ b/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 + * . + * + */ +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 cookieSpecRegistry; + private Lookup 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 argcap = ArgumentCaptor.forClass(HttpRequestWrapper.class); + Mockito.when(routePlanner.determineRoute( + Mockito.eq(new HttpHost("somehost")), + argcap.capture(), + Mockito.any())).thenReturn(route); + + client.execute(httpget); + + Assert.assertNotNull(argcap.getValue()); + Assert.assertSame(httpget, argcap.getValue().getOriginal()); + + Mockito.verify(execChain).execute( + Mockito.same(route), + Mockito.any(), + Mockito.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.any(), + Mockito.any())).thenReturn(route); + Mockito.when(execChain.execute( + Mockito.same(route), + Mockito.any(), + Mockito.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 localCookieSpecRegistry = Mockito.mock(Lookup.class); + Lookup 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(); + } + +}