XRENDERING-337: Link checker cannot verify URL when XWiki is behind a HTTP proxy

xwiki-rendering-transformations/xwiki-rendering-transformation-linkchecker/pom.xml

@@ -31,7 +31,7 @@
   <name>XWiki Rendering - Transformation - Link Checker</name>
   <description>Verifies if external URLs are valid and decorate with parameters if not</description>
   <properties>
-    <xwiki.jacoco.instructionRatio>0.87</xwiki.jacoco.instructionRatio>
+    <xwiki.jacoco.instructionRatio>0.90</xwiki.jacoco.instructionRatio>
   </properties>
   <dependencies>
     <dependency>
@@ -45,12 +45,26 @@
       <version>${commons.version}</version>
     </dependency>
     <dependency>
-      <groupId>commons-httpclient</groupId>
-      <artifactId>commons-httpclient</artifactId>
+      <groupId>org.apache.httpcomponents</groupId>
+      <artifactId>httpclient</artifactId>
     </dependency>
     <dependency>
       <groupId>org.apache.commons</groupId>
       <artifactId>commons-lang3</artifactId>
     </dependency>
+    <!-- Test Dependencies -->
+    <dependency>
+      <groupId>com.github.tomakehurst</groupId>
+      <artifactId>wiremock</artifactId>
+      <version>1.46</version>
+      <scope>test</scope>
+      <exclusions>
+        <!-- We are using SLF4J -->
+        <exclusion>
+          <artifactId>log4j</artifactId>
+          <groupId>log4j</groupId>
+        </exclusion>
+      </exclusions>
+    </dependency>
   </dependencies>
 </project>

xwiki-rendering-transformations/xwiki-rendering-transformation-linkchecker/src/main/java/org/xwiki/rendering/internal/transformation/linkchecker/DefaultHTTPChecker.java

@@ -22,11 +22,13 @@
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
-import org.apache.commons.httpclient.HttpClient;
-import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager;
-import org.apache.commons.httpclient.cookie.CookiePolicy;
-import org.apache.commons.httpclient.methods.GetMethod;
-import org.apache.commons.httpclient.params.HttpMethodParams;
+import org.apache.http.client.config.CookieSpecs;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
 import org.slf4j.Logger;
 import org.xwiki.component.annotation.Component;
 import org.xwiki.component.phase.Initializable;
@@ -54,17 +56,30 @@ public class DefaultHTTPChecker implements HTTPChecker, Initializable
      * Note: If one day we wish to configure timeouts, here's some good documentation about it:
      * http://brian.olore.net/wp/2009/08/apache-httpclient-timeout/
      */
-    private HttpClient httpClient;
+    private CloseableHttpClient httpClient;
 
     @Override
     public void initialize() throws InitializationException
     {
-        this.httpClient = new HttpClient(new MultiThreadedHttpConnectionManager());
+        HttpClientBuilder httpClientBuilder = HttpClientBuilder.create();
+
+        // Make the Http Client reusable by several threads
+        PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager();
+        httpClientBuilder.setConnectionManager(connectionManager);
+
+        // Pre-configure with everything configured at JVM level (e.g. proxy setup).
+        httpClientBuilder.useSystemProperties();
 
         // Set our user agent to be a good citizen.
-        this.httpClient.getParams().setParameter(HttpMethodParams.USER_AGENT, "XWikiLinkChecker");
+        httpClientBuilder.setUserAgent("XWikiLinkChecker");
+
         // Ignore cookies since this can cause errors in logs and we don't need cookies when checking sites.
-        this.httpClient.getParams().setCookiePolicy(CookiePolicy.IGNORE_COOKIES);
+        RequestConfig config = RequestConfig.custom()
+            .setCookieSpec(CookieSpecs.IGNORE_COOKIES)
+            .build();
+        httpClientBuilder.setDefaultRequestConfig(config);
+
+        this.httpClient = httpClientBuilder.build();
     }
 
     @Override
@@ -72,13 +87,11 @@ public int check(String url)
     {
         int responseCode;
 
-        GetMethod method = null;
+        CloseableHttpResponse httpResponse = null;
         try {
-            method = new GetMethod(url);
-
-            // Execute the method.
-            responseCode = this.httpClient.executeMethod(method);
-
+            HttpGet httpGet = new HttpGet(url);
+            httpResponse = this.httpClient.execute(httpGet);
+            responseCode = httpResponse.getStatusLine().getStatusCode();
             this.logger.debug("Result of pinging [{}]: code = [{}]", url, responseCode);
         } catch (Exception e) {
             // Some error in the transport or in the passed URL, use a special response code (0) which isn't in the
@@ -86,8 +99,13 @@ public int check(String url)
             responseCode = 0;
             this.logger.debug("Error while checking [{}]", url, e);
         } finally {
-            if (method != null) {
-                method.releaseConnection();
+            if (httpResponse != null) {
+                try {
+                    httpResponse.close();
+                } catch (Exception ee) {
+                    // Failed to close, ignore but log the error.
+                    this.logger.error("Failed to close HTTP connection for [{}]", url, ee);
+                }
             }
         }
 

xwiki-rendering-transformations/xwiki-rendering-transformation-linkchecker/src/test/java/org/xwiki/rendering/internal/transformation/linkchecker/DefaultHTTPCheckerTest.java

+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package org.xwiki.rendering.internal.transformation.linkchecker;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.xwiki.test.mockito.MockitoComponentMockingRule;
+
+import com.github.tomakehurst.wiremock.junit.WireMockRule;
+
+import static com.github.tomakehurst.wiremock.client.RequestPatternBuilder.allRequests;
+import static com.github.tomakehurst.wiremock.client.WireMock.findAll;
+import static org.junit.Assert.*;
+
+/**
+ * Integration tests for {@link org.xwiki.rendering.internal.transformation.linkchecker.DefaultHTTPChecker}.
+ *
+ * @version $Id$
+ * @since 6.1M2
+ */
+public class DefaultHTTPCheckerTest
+{
+    @Rule
+    public WireMockRule proxyWireMockRule = new WireMockRule(8888);
+
+    @Rule
+    public MockitoComponentMockingRule<DefaultHTTPChecker> mocker =
+        new MockitoComponentMockingRule<>(DefaultHTTPChecker.class);
+
+    @Test
+    public void testProxy() throws Exception
+    {
+        // First call the link checker but since we haven't set up any proxy our Mock HTTP Server is not going to be
+        // called (since http://host will lead to nowhere...
+        assertEquals(0, this.mocker.getComponentUnderTest().check("http://host"));
+        assertTrue("The HTTP server was not called by the link checker", findAll(allRequests()).isEmpty());
+
+        // Second, setup a proxy by using System Properties, then call again the checker and this time it should
+        // succeed since http://host will go to the proxy which is pointing to our Mock HTTP Server!
+        System.setProperty("http.proxyHost", "localhost");
+        System.setProperty("http.proxyPort", "8888");
+        assertEquals(404, this.mocker.getComponentUnderTest().check("http://host"));
+        assertFalse("The HTTP server was called by the link checker", findAll(allRequests()).isEmpty());
+    }
+}