Mailing-List: contact commits-help@hc.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "HttpComponents Project" <dev@hc.apache.org>
Received-SPF: neutral (athena.apache.org: local policy)
Message-ID: <1361527991.2024.14.camel@ubuntu>
Subject: Re: svn commit: r1448936
From: Oleg Kalnichevski <olegk@apache.org>
To: HttpComponents Project <dev@hc.apache.org>
Cc: commits@hc.apache.org
Date: Fri, 22 Feb 2013 11:13:11 +0100
In-Reply-To: <20130222083636.DC14B23888D2@eris.apache.org>
References: <20130222083636.DC14B23888D2@eris.apache.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

On Fri, 2013-02-22 at 08:36 +0000, fx@apache.org wrote:
> Author: fx
> Date: Fri Feb 22 08:36:36 2013
> New Revision: 1448936
> 
> URL: http://svn.apache.org/r1448936
> Log:
> HTTPCLIENT-1325: Using a HttpRequest that is not an HttpUriRequest when host in URI is not target host results in an invalid request
> 

Francois-Xavier

I do not mean to be nasty, but please avoid mixing functional and
formatting changes in the same commit. It makes it more difficult to
review the changes. It happens to me all the time but generally this is
something we should _try_ to avoid. You should probably disable code
formatting on save option or some such in your Eclipse.

I also tend to do 'git diff --staged' or 'svn diff' prior to committing
local changes to make sure there is no unnecessary noise (whitespace
changes, formatting, etc).

Oleg


> Added:
>     httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/execchain/TestProtocolExec.java   (with props)
> Modified:
>     httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/execchain/ProtocolExec.java
> 
> Modified: httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/execchain/ProtocolExec.java
> URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/execchain/ProtocolExec.java?rev=1448936&r1=1448935&r2=1448936&view=diff
> ==============================================================================
> --- httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/execchain/ProtocolExec.java (original)
> +++ httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/execchain/ProtocolExec.java Fri Feb 22 08:36:36 2013
> @@ -67,18 +67,15 @@ public class ProtocolExec implements Cli
>      private final ClientExecChain requestExecutor;
>      private final HttpProcessor httpProcessor;
>  
> -    public ProtocolExec(
> -            final ClientExecChain requestExecutor,
> -            final HttpProcessor httpProcessor) {
> +    public ProtocolExec(final ClientExecChain requestExecutor, final HttpProcessor httpProcessor) {
>          Args.notNull(requestExecutor, "HTTP client request executor");
>          Args.notNull(httpProcessor, "HTTP protocol processor");
>          this.requestExecutor = requestExecutor;
>          this.httpProcessor = httpProcessor;
>      }
>  
> -    private void rewriteRequestURI(
> -            final HttpRequestWrapper request,
> -            final HttpRoute route) throws ProtocolException {
> +    private void rewriteRequestURI(final HttpRequestWrapper request, final HttpRoute route)
> +        throws ProtocolException {
>          try {
>              URI uri = request.getURI();
>              if (uri != null) {
> @@ -101,16 +98,13 @@ public class ProtocolExec implements Cli
>                  request.setURI(uri);
>              }
>          } catch (final URISyntaxException ex) {
> -            throw new ProtocolException("Invalid URI: " +
> -                    request.getRequestLine().getUri(), ex);
> +            throw new ProtocolException("Invalid URI: " + request.getRequestLine().getUri(), ex);
>          }
>      }
>  
> -    public CloseableHttpResponse execute(
> -            final HttpRoute route,
> -            final HttpRequestWrapper request,
> -            final HttpClientContext context,
> -            final HttpExecutionAware execAware) throws IOException, HttpException {
> +    public CloseableHttpResponse execute(final HttpRoute route, final HttpRequestWrapper request,
> +        final HttpClientContext context, final HttpExecutionAware execAware) throws IOException,
> +        HttpException {
>          Args.notNull(route, "HTTP route");
>          Args.notNull(request, "HTTP request");
>          Args.notNull(context, "HTTP context");
> @@ -122,8 +116,8 @@ public class ProtocolExec implements Cli
>              if (uri != null) {
>                  final String userinfo = uri.getUserInfo();
>                  if (userinfo != null) {
> -                    targetAuthState.update(
> -                            new BasicScheme(), new UsernamePasswordCredentials(userinfo));
> +                    targetAuthState.update(new BasicScheme(), new UsernamePasswordCredentials(
> +                        userinfo));
>                  }
>              }
>          }
> @@ -136,8 +130,9 @@ public class ProtocolExec implements Cli
>          // HTTPCLIENT-1092 - add the port if necessary
>          if (virtualHost != null && virtualHost.getPort() == -1) {
>              final int port = route.getTargetHost().getPort();
> -            if (port != -1){
> -                virtualHost = new HttpHost(virtualHost.getHostName(), port, virtualHost.getSchemeName());
> +            if (port != -1) {
> +                virtualHost = new HttpHost(virtualHost.getHostName(), port,
> +                    virtualHost.getSchemeName());
>              }
>              if (this.log.isDebugEnabled()) {
>                  this.log.debug("Using virtual host" + virtualHost);
> @@ -149,11 +144,20 @@ public class ProtocolExec implements Cli
>              target = virtualHost;
>          } else {
>              final HttpRequest original = request.getOriginal();
> +            URI uri = null;
>              if (original instanceof HttpUriRequest) {
> -                final URI uri = ((HttpUriRequest) original).getURI();
> -                if (uri.isAbsolute()) {
> -                    target = new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme());
> +                uri = ((HttpUriRequest) original).getURI();
> +            } else {
> +                final String uriString = original.getRequestLine().getUri();
> +                try {
> +                    uri = URI.create(uriString);
> +                } catch (IllegalArgumentException e) {
> +                    log.debug("Could not parse " + uriString + " as a URI to set Host header");
>                  }
> +
> +            }
> +            if (uri != null && uri.isAbsolute() && uri.getHost() != null) {
> +                target = new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme());
>              }
>          }
>          if (target == null) {
> @@ -167,7 +171,8 @@ public class ProtocolExec implements Cli
>  
>          this.httpProcessor.process(request, context);
>  
> -        final CloseableHttpResponse response = this.requestExecutor.execute(route, request, context, execAware);
> +        final CloseableHttpResponse response = this.requestExecutor.execute(route, request,
> +            context, execAware);
>          try {
>              // Run response protocol interceptors
>              context.setAttribute(ExecutionContext.HTTP_RESPONSE, response);
> 
> Added: httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/execchain/TestProtocolExec.java
> URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/execchain/TestProtocolExec.java?rev=1448936&view=auto
> ==============================================================================
> --- httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/execchain/TestProtocolExec.java (added)
> +++ httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/execchain/TestProtocolExec.java Fri Feb 22 08:36:36 2013
> @@ -0,0 +1,84 @@
> +/*
> + * ====================================================================
> + * 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.execchain;
> +
> +import org.apache.http.HttpHost;
> +import org.apache.http.client.methods.HttpExecutionAware;
> +import org.apache.http.client.methods.HttpRequestWrapper;
> +import org.apache.http.client.protocol.HttpClientContext;
> +import org.apache.http.conn.routing.HttpRoute;
> +import org.apache.http.message.BasicHttpRequest;
> +import org.apache.http.params.BasicHttpParams;
> +import org.apache.http.params.HttpParams;
> +import org.apache.http.protocol.ExecutionContext;
> +import org.apache.http.protocol.HttpProcessor;
> +import org.junit.Assert;
> +import org.junit.Before;
> +import org.junit.Test;
> +import org.mockito.Mockito;
> +
> +public class TestProtocolExec {
> +
> +    private ClientExecChain requestExecutor;
> +    private HttpProcessor httpProcessor;
> +    private ProtocolExec protocolExec;
> +    private HttpClientContext context;
> +    private HttpRequestWrapper request;
> +    private HttpExecutionAware execAware;
> +    private HttpRoute route;
> +    private HttpParams params = new BasicHttpParams();
> +
> +    @Before
> +    public void setup() throws Exception {
> +        requestExecutor = Mockito.mock(ClientExecChain.class);
> +        httpProcessor = Mockito.mock(HttpProcessor.class);
> +        protocolExec = new ProtocolExec(requestExecutor, httpProcessor);
> +        route = new HttpRoute(new HttpHost("foo", 8080));
> +        context = new HttpClientContext();
> +        execAware = Mockito.mock(HttpExecutionAware.class);
> +    }
> +
> +    @Test
> +    public void testHostHeaderWhenNonUriRequest() throws Exception {
> +        request = HttpRequestWrapper.wrap(new BasicHttpRequest("GET", "http://bar/test"));
> +        protocolExec.execute(route, request, context, execAware);
> +        // ProtocolExect should have extracted the host from request URI
> +        Assert.assertEquals(new HttpHost("bar", -1, "http"),
> +            context.getAttribute(ExecutionContext.HTTP_TARGET_HOST));
> +    }
> +
> +    @Test
> +    public void testHostHeaderWhenNonUriRequestAndInvalidUri() throws Exception {
> +        request = HttpRequestWrapper.wrap(new BasicHttpRequest("GET", "http://bar/test|"));
> +        protocolExec.execute(route, request, context, execAware);
> +        // ProtocolExect should have fall back to physical host as request URI
> +        // is not parseable
> +        Assert.assertEquals(new HttpHost("foo", 8080, "http"),
> +            context.getAttribute(ExecutionContext.HTTP_TARGET_HOST));
> +    }
> +
> +}
> 
> Propchange: httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/execchain/TestProtocolExec.java
> ------------------------------------------------------------------------------
>     svn:executable = *
> 
> 
>