Use the path of remote url to check if the remote storage request is …

…made for a collection

Our check was using the full url that could include the query part as in for example "http://foo.com/bar?x=y/", case when we get a false positive

Also added UTs for checking. The problem is only on http client 4, the old one behaves as expected

Signed-off-by: Alin Dreghiciu <adreghiciu@gmail.com>

nexus/nexus-proxy/src/main/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorage.java

@@ -58,6 +58,7 @@
 import org.sonatype.nexus.proxy.storage.remote.RemoteStorageContext;
 import org.sonatype.nexus.proxy.storage.remote.http.QueryStringBuilder;
 import org.sonatype.nexus.proxy.utils.UserAgentBuilder;
+import com.google.common.annotations.VisibleForTesting;
 
 /**
  * Apache HTTP client (4) {@link RemoteRepositoryStorage} implementation.
@@ -135,7 +136,7 @@ public AbstractStorageItem retrieveItem( final ProxyRepository repository,
             appendQueryString( getAbsoluteUrlFromBase( baseUrl, request.getRequestPath() ), repository );
 
         final String url = remoteURL.toExternalForm();
-        if ( url.endsWith( "/" ) )
+        if ( remoteURL.getPath().endsWith( "/" ) )
         {
             // NEXUS-5125 we do not want to fetch any collection
             // Even though it is unlikely that we actually see a request for a collection here,
@@ -425,9 +426,10 @@ protected String getS3FlagKey()
      * @return response of making the request
      * @throws RemoteStorageException If an error occurred during execution of HTTP request
      */
-    private HttpResponse executeRequest( final ProxyRepository repository,
-                                         final ResourceStoreRequest request,
-                                         final HttpUriRequest httpRequest )
+    @VisibleForTesting
+    HttpResponse executeRequest( final ProxyRepository repository,
+                                 final ResourceStoreRequest request,
+                                 final HttpUriRequest httpRequest )
         throws RemoteStorageException
     {
         final URI methodUri = httpRequest.getURI();

nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/commonshttpclient/CommonsHttpClientRemoteStorageTest.java

@@ -0,0 +1,184 @@
+/**
+ * Sonatype Nexus (TM) Open Source Version
+ * Copyright (c) 2007-2012 Sonatype, Inc.
+ * All rights reserved. Includes the third-party code listed at http://links.sonatype.com/products/nexus/oss/attributions.
+ *
+ * This program and the accompanying materials are made available under the terms of the Eclipse Public License Version 1.0,
+ * which accompanies this distribution and is available at http://www.eclipse.org/legal/epl-v10.html.
+ *
+ * Sonatype Nexus (TM) Professional Version is available from Sonatype, Inc. "Sonatype" and "Sonatype Nexus" are trademarks
+ * of Sonatype, Inc. Apache Maven is a trademark of the Apache Software Foundation. M2eclipse is a trademark of the
+ * Eclipse Foundation. All other trademarks are the property of their respective owners.
+ */
+package org.sonatype.nexus.proxy.storage.remote.commonshttpclient;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.net.URL;
+
+import org.apache.commons.httpclient.HttpMethod;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.sonatype.nexus.ApplicationStatusSource;
+import org.sonatype.nexus.mime.MimeSupport;
+import org.sonatype.nexus.proxy.RemoteStorageException;
+import org.sonatype.nexus.proxy.ResourceStoreRequest;
+import org.sonatype.nexus.proxy.repository.ProxyRepository;
+import org.sonatype.nexus.proxy.storage.remote.RemoteItemNotFoundException;
+import org.sonatype.nexus.proxy.utils.UserAgentBuilder;
+import org.sonatype.sisu.litmus.testsupport.TestSupport;
+
+/**
+ * {@link CommonsHttpClientRemoteStorage} UTs.
+ *
+ * @since 2.2
+ */
+public class CommonsHttpClientRemoteStorageTest
+    extends TestSupport
+{
+
+    @Rule
+    public ExpectedException thrown = ExpectedException.none();
+
+    /**
+     * When retrieving an item with a path that ends with a "/" and without a query,
+     * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be
+     * thrown.
+     */
+    @Test
+    public void retrieveCollectionWhenPathEndsWithSlashAndNoQuery()
+        throws Exception
+    {
+        final CommonsHttpClientRemoteStorage underTest = new CommonsHttpClientRemoteStorage(
+            mock( UserAgentBuilder.class ),
+            mock( ApplicationStatusSource.class ),
+            mock( MimeSupport.class )
+        )
+        {
+            @Override
+            protected int executeMethod( final ProxyRepository repository, final ResourceStoreRequest request,
+                                         final HttpMethod method,
+                                         final URL remoteUrl )
+                throws RemoteStorageException
+            {
+                return 200;
+            }
+        };
+
+        thrown.expect( RemoteItemNotFoundException.class );
+        thrown.expectMessage( "The remoteURL we got to looks like is a collection" );
+
+        underTest.retrieveItem(
+            mock( ProxyRepository.class ),
+            new ResourceStoreRequest( "bar/" ),
+            "http://foo.com"
+        );
+    }
+
+    /**
+     * When retrieving an item with a path that ends with a "/" and a query string that ends with a "/",
+     * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be
+     * thrown.
+     */
+    @Test
+    public void retrieveCollectionWhenPathEndsWithSlashAndQueryEndsWithSlash()
+        throws Exception
+    {
+        final CommonsHttpClientRemoteStorage underTest = new CommonsHttpClientRemoteStorage(
+            mock( UserAgentBuilder.class ),
+            mock( ApplicationStatusSource.class ),
+            mock( MimeSupport.class )
+        )
+        {
+            @Override
+            protected int executeMethod( final ProxyRepository repository, final ResourceStoreRequest request,
+                                         final HttpMethod method,
+                                         final URL remoteUrl )
+                throws RemoteStorageException
+            {
+                return 200;
+            }
+        };
+
+        thrown.expect( RemoteItemNotFoundException.class );
+        thrown.expectMessage( "The remoteURL we got to looks like is a collection" );
+
+        underTest.retrieveItem(
+            mock( ProxyRepository.class ),
+            new ResourceStoreRequest( "bar/?param=x/" ),
+            "http://foo.com"
+        );
+    }
+
+    /**
+     * When retrieving an item with a path that ends with a "/" and a query string that does not end with a "/",
+     * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be
+     * thrown.
+     */
+    @Test
+    public void retrieveCollectionWhenPathEndsWithSlashAndQueryDoesNotEndWithSlash()
+        throws Exception
+    {
+        final CommonsHttpClientRemoteStorage underTest = new CommonsHttpClientRemoteStorage(
+            mock( UserAgentBuilder.class ),
+            mock( ApplicationStatusSource.class ),
+            mock( MimeSupport.class )
+        )
+        {
+            @Override
+            protected int executeMethod( final ProxyRepository repository, final ResourceStoreRequest request,
+                                         final HttpMethod method,
+                                         final URL remoteUrl )
+                throws RemoteStorageException
+            {
+                return 200;
+            }
+        };
+
+        thrown.expect( RemoteItemNotFoundException.class );
+        thrown.expectMessage( "The remoteURL we got to looks like is a collection" );
+
+        underTest.retrieveItem(
+            mock( ProxyRepository.class ),
+            new ResourceStoreRequest( "bar/?param=x" ),
+            "http://foo.com"
+        );
+    }
+
+    /**
+     * When retrieving an item with a path that does not end with a "/" and a query string that does not end with a "/",
+     * no exception should be thrown.
+     */
+    @Test
+    public void retrieveCollectionWhenPathDoesNotEndWithSlashAndQueryDoesNotEndWithSlash()
+        throws Exception
+    {
+        final CommonsHttpClientRemoteStorage underTest = new CommonsHttpClientRemoteStorage(
+            mock( UserAgentBuilder.class ),
+            mock( ApplicationStatusSource.class ),
+            mock( MimeSupport.class )
+        )
+        {
+            @Override
+            protected int executeMethod( final ProxyRepository repository, final ResourceStoreRequest request,
+                                         final HttpMethod method,
+                                         final URL remoteUrl )
+                throws RemoteStorageException
+            {
+                return 200;
+            }
+        };
+
+        final ProxyRepository repository = mock( ProxyRepository.class );
+        when( repository.getId() ).thenReturn( "foo" );
+
+        underTest.retrieveItem(
+            repository,
+            new ResourceStoreRequest( "bar?param=x" ),
+            "http://foo.com"
+        );
+    }
+
+}

nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorageTest.java

@@ -0,0 +1,168 @@
+/**
+ * Sonatype Nexus (TM) Open Source Version
+ * Copyright (c) 2007-2012 Sonatype, Inc.
+ * All rights reserved. Includes the third-party code listed at http://links.sonatype.com/products/nexus/oss/attributions.
+ *
+ * This program and the accompanying materials are made available under the terms of the Eclipse Public License Version 1.0,
+ * which accompanies this distribution and is available at http://www.eclipse.org/legal/epl-v10.html.
+ *
+ * Sonatype Nexus (TM) Professional Version is available from Sonatype, Inc. "Sonatype" and "Sonatype Nexus" are trademarks
+ * of Sonatype, Inc. Apache Maven is a trademark of the Apache Software Foundation. M2eclipse is a trademark of the
+ * Eclipse Foundation. All other trademarks are the property of their respective owners.
+ */
+package org.sonatype.nexus.proxy.storage.remote.httpclient;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.net.URL;
+
+import org.apache.commons.httpclient.HttpMethod;
+import org.apache.http.HttpEntity;
+import org.apache.http.HttpResponse;
+import org.apache.http.StatusLine;
+import org.apache.http.client.methods.HttpUriRequest;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.sonatype.nexus.ApplicationStatusSource;
+import org.sonatype.nexus.mime.MimeSupport;
+import org.sonatype.nexus.proxy.RemoteStorageException;
+import org.sonatype.nexus.proxy.ResourceStoreRequest;
+import org.sonatype.nexus.proxy.repository.ProxyRepository;
+import org.sonatype.nexus.proxy.storage.remote.RemoteItemNotFoundException;
+import org.sonatype.nexus.proxy.storage.remote.commonshttpclient.CommonsHttpClientRemoteStorage;
+import org.sonatype.nexus.proxy.storage.remote.http.QueryStringBuilder;
+import org.sonatype.nexus.proxy.utils.UserAgentBuilder;
+import org.sonatype.sisu.litmus.testsupport.TestSupport;
+
+/**
+ * {@link HttpClientRemoteStorage} UTs.
+ *
+ * @since 2.2
+ */
+public class HttpClientRemoteStorageTest
+    extends TestSupport
+{
+
+    @Rule
+    public ExpectedException thrown = ExpectedException.none();
+
+    /**
+     * When retrieving an item with a path that ends with a "/" and without a query,
+     * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be
+     * thrown.
+     */
+    @Test
+    public void retrieveCollectionWhenPathEndsWithSlashAndNoQuery()
+        throws Exception
+    {
+        final HttpClientRemoteStorage underTest = new HttpClientRemoteStorage(
+            mock( UserAgentBuilder.class ),
+            mock( ApplicationStatusSource.class ),
+            mock( MimeSupport.class ),
+            mock( QueryStringBuilder.class )
+        );
+
+        thrown.expect( RemoteItemNotFoundException.class );
+        thrown.expectMessage( "The remoteURL we got to looks like is a collection" );
+
+        underTest.retrieveItem(
+            mock( ProxyRepository.class ),
+            new ResourceStoreRequest( "bar/" ),
+            "http://foo.com"
+        );
+    }
+
+    /**
+     * When retrieving an item with a path that ends with a "/" and a query string that ends with a "/",
+     * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be
+     * thrown.
+     */
+    @Test
+    public void retrieveCollectionWhenPathEndsWithSlashAndQueryEndsWithSlash()
+        throws Exception
+    {
+        final HttpClientRemoteStorage underTest = new HttpClientRemoteStorage(
+            mock( UserAgentBuilder.class ),
+            mock( ApplicationStatusSource.class ),
+            mock( MimeSupport.class ),
+            mock( QueryStringBuilder.class )
+        );
+
+        thrown.expect( RemoteItemNotFoundException.class );
+        thrown.expectMessage( "The remoteURL we got to looks like is a collection" );
+
+        underTest.retrieveItem(
+            mock( ProxyRepository.class ),
+            new ResourceStoreRequest( "bar/?param=x/" ),
+            "http://foo.com"
+        );
+    }
+
+    /**
+     * When retrieving an item with a path that ends with a "/" and a query string that does not end with a "/",
+     * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be
+     * thrown.
+     */
+    @Test
+    public void retrieveCollectionWhenPathEndsWithSlashAndQueryDoesNotEndWithSlash()
+        throws Exception
+    {
+        final HttpClientRemoteStorage underTest = new HttpClientRemoteStorage(
+            mock( UserAgentBuilder.class ),
+            mock( ApplicationStatusSource.class ),
+            mock( MimeSupport.class ),
+            mock( QueryStringBuilder.class )
+        );
+
+        thrown.expect( RemoteItemNotFoundException.class );
+        thrown.expectMessage( "The remoteURL we got to looks like is a collection" );
+
+        underTest.retrieveItem(
+            mock( ProxyRepository.class ),
+            new ResourceStoreRequest( "bar/?param=x" ),
+            "http://foo.com"
+        );
+    }
+
+    /**
+     * When retrieving an item with a path that does not end with a "/" and a query string that does not end with a "/",
+     * no exception should be thrown.
+     */
+    @Test
+    public void retrieveCollectionWhenPathDoesNotEndWithSlashAndQueryDoesNotEndWithSlash()
+        throws Exception
+    {
+        final HttpClientRemoteStorage underTest = new HttpClientRemoteStorage(
+            mock( UserAgentBuilder.class ),
+            mock( ApplicationStatusSource.class ),
+            mock( MimeSupport.class ),
+            mock( QueryStringBuilder.class )
+        )
+        {
+            @Override
+            HttpResponse executeRequest( final ProxyRepository repository, final ResourceStoreRequest request,
+                                         final HttpUriRequest httpRequest )
+                throws RemoteStorageException
+            {
+                final HttpResponse httpResponse = mock( HttpResponse.class );
+                final StatusLine statusLine = mock( StatusLine.class );
+                when( httpResponse.getStatusLine() ).thenReturn( statusLine );
+                when( statusLine.getStatusCode() ).thenReturn( 200 );
+                when( httpResponse.getEntity() ).thenReturn( mock( HttpEntity.class ) );
+                return httpResponse;
+            }
+        };
+
+        final ProxyRepository repository = mock( ProxyRepository.class );
+        when( repository.getId() ).thenReturn( "foo" );
+
+        underTest.retrieveItem(
+            repository,
+            new ResourceStoreRequest( "bar?param=x" ),
+            "http://foo.com"
+        );
+    }
+
+}