fix NPE for commons-httpclient v3.1 (#5944) (#5949)

* fix NPE for commons-httpclient v3.1 (#5944)

* fix NPE for commons-httpclient v3.1 (#5944) - spotless fixes

* extracting abstract class with reused code (#5944)

instrumentation/apache-httpclient/apache-httpclient-2.0/javaagent/build.gradle.kts

@@ -15,3 +15,9 @@ dependencies {
 
   latestDepTestLibrary("commons-httpclient:commons-httpclient:3.+") // see apache-httpclient-4.0 module
 }
+
+tasks {
+  withType<Test>().configureEach {
+    systemProperty("testLatestDeps", findProperty("testLatestDeps"))
+  }
+}

instrumentation/apache-httpclient/apache-httpclient-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v2_0/ApacheHttpClientHttpAttributesGetter.java

@@ -88,7 +88,7 @@ final class ApacheHttpClientHttpAttributesGetter
   // mirroring implementation HttpMethodBase.getURI(), to avoid converting to URI and back to String
   private static String getUrl(HttpMethod request) {
     HostConfiguration hostConfiguration = request.getHostConfiguration();
-    if (hostConfiguration == null) {
+    if (hostConfiguration == null || hostConfiguration.getProtocol() == null) {
       String queryString = request.getQueryString();
       if (queryString == null) {
         return request.getPath();

instrumentation/apache-httpclient/apache-httpclient-2.0/javaagent/src/test/groovy/AbstractCommonsHttpClientTest.groovy

@@ -0,0 +1,109 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import io.opentelemetry.api.common.AttributeKey
+import io.opentelemetry.instrumentation.test.AgentTestTrait
+import io.opentelemetry.instrumentation.test.base.HttpClientTest
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
+import org.apache.commons.httpclient.HttpClient
+import org.apache.commons.httpclient.HttpConnectionManager
+import org.apache.commons.httpclient.HttpMethod
+import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager
+import org.apache.commons.httpclient.methods.DeleteMethod
+import org.apache.commons.httpclient.methods.GetMethod
+import org.apache.commons.httpclient.methods.HeadMethod
+import org.apache.commons.httpclient.methods.OptionsMethod
+import org.apache.commons.httpclient.methods.PostMethod
+import org.apache.commons.httpclient.methods.PutMethod
+import org.apache.commons.httpclient.methods.TraceMethod
+import spock.lang.Shared
+
+abstract class AbstractCommonsHttpClientTest extends HttpClientTest<HttpMethod> implements AgentTestTrait {
+  @Shared
+  HttpConnectionManager connectionManager = new MultiThreadedHttpConnectionManager()
+  @Shared
+  HttpClient client = buildClient(false)
+  @Shared
+  HttpClient clientWithReadTimeout = buildClient(true)
+
+  def buildClient(boolean readTimeout) {
+    HttpClient client = new HttpClient(connectionManager)
+    client.setConnectionTimeout(CONNECT_TIMEOUT_MS)
+    if (readTimeout) {
+      client.setTimeout(READ_TIMEOUT_MS)
+    }
+    return client
+  }
+
+  HttpClient getClient(URI uri) {
+    if (uri.toString().contains("/read-timeout")) {
+      return clientWithReadTimeout
+    }
+    return client
+  }
+
+  @Override
+  HttpMethod buildRequest(String method, URI uri, Map<String, String> headers) {
+    def request
+    switch (method) {
+      case "GET":
+        request = new GetMethod(uri.toString())
+        break
+      case "PUT":
+        request = new PutMethod(uri.toString())
+        break
+      case "POST":
+        request = new PostMethod(uri.toString())
+        break
+      case "HEAD":
+        request = new HeadMethod(uri.toString())
+        break
+      case "DELETE":
+        request = new DeleteMethod(uri.toString())
+        break
+      case "OPTIONS":
+        request = new OptionsMethod(uri.toString())
+        break
+      case "TRACE":
+        request = new TraceMethod(uri.toString())
+        break
+      default:
+        throw new IllegalStateException("Unsupported method: " + method)
+    }
+    headers.each { request.setRequestHeader(it.key, it.value) }
+    return request
+  }
+
+  @Override
+  boolean testCircularRedirects() {
+    false
+  }
+
+  @Override
+  boolean testReusedRequest() {
+    // apache commons throws an exception if the request is reused without being recycled first
+    // at which point this test is not useful (and requires re-populating uri)
+    false
+  }
+
+  @Override
+  boolean testCallback() {
+    false
+  }
+
+  @Override
+  boolean testReadTimeout() {
+    true
+  }
+
+  @Override
+  Set<AttributeKey<?>> httpAttributes(URI uri) {
+    Set<AttributeKey<?>> extra = [
+      SemanticAttributes.HTTP_SCHEME,
+      SemanticAttributes.HTTP_TARGET
+    ]
+    super.httpAttributes(uri) + extra
+  }
+}

instrumentation/apache-httpclient/apache-httpclient-2.0/javaagent/src/test/groovy/CommonsHttpClientLatestDepsTest.groovy

@@ -0,0 +1,25 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import org.apache.commons.httpclient.HttpMethod
+import spock.lang.IgnoreIf
+
+//this test will be ignored if not executed with -PtestLatestDeps=true
+//because the latest dependency commons-httpclient v3.1 allows a call to the executeMethod 
+//with some null parameters like HttpClient.executeMethod(null, request, null)  
+//but this construct is not allowed in commons-httpclient v2 that is used for regular otel testing
+@IgnoreIf({ !Boolean.getBoolean("testLatestDeps") })
+class CommonsHttpClientLatestDepsTest extends AbstractCommonsHttpClientTest {
+ 
+  @Override
+  int sendRequest(HttpMethod request, String method, URI uri, Map<String, String> headers) {
+    try {      
+      getClient(uri).executeMethod(null, request, null)      
+      return request.getStatusCode()
+    } finally {
+      request.releaseConnection()
+    }
+  }
+}

instrumentation/apache-httpclient/apache-httpclient-2.0/javaagent/src/test/groovy/CommonsHttpClientTest.groovy

@@ -3,78 +3,9 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import io.opentelemetry.api.common.AttributeKey
-import io.opentelemetry.instrumentation.test.AgentTestTrait
-import io.opentelemetry.instrumentation.test.base.HttpClientTest
-import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
-import org.apache.commons.httpclient.HttpClient
-import org.apache.commons.httpclient.HttpConnectionManager
 import org.apache.commons.httpclient.HttpMethod
-import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager
-import org.apache.commons.httpclient.methods.DeleteMethod
-import org.apache.commons.httpclient.methods.GetMethod
-import org.apache.commons.httpclient.methods.HeadMethod
-import org.apache.commons.httpclient.methods.OptionsMethod
-import org.apache.commons.httpclient.methods.PostMethod
-import org.apache.commons.httpclient.methods.PutMethod
-import org.apache.commons.httpclient.methods.TraceMethod
-import spock.lang.Shared
 
-class CommonsHttpClientTest extends HttpClientTest<HttpMethod> implements AgentTestTrait {
-  @Shared
-  HttpConnectionManager connectionManager = new MultiThreadedHttpConnectionManager()
-  @Shared
-  HttpClient client = buildClient(false)
-  @Shared
-  HttpClient clientWithReadTimeout = buildClient(true)
-
-  def buildClient(boolean readTimeout) {
-    HttpClient client = new HttpClient(connectionManager)
-    client.setConnectionTimeout(CONNECT_TIMEOUT_MS)
-    if (readTimeout) {
-      client.setTimeout(READ_TIMEOUT_MS)
-    }
-    return client
-  }
-
-  HttpClient getClient(URI uri) {
-    if (uri.toString().contains("/read-timeout")) {
-      return clientWithReadTimeout
-    }
-    return client
-  }
-
-  @Override
-  HttpMethod buildRequest(String method, URI uri, Map<String, String> headers) {
-    def request
-    switch (method) {
-      case "GET":
-        request = new GetMethod(uri.toString())
-        break
-      case "PUT":
-        request = new PutMethod(uri.toString())
-        break
-      case "POST":
-        request = new PostMethod(uri.toString())
-        break
-      case "HEAD":
-        request = new HeadMethod(uri.toString())
-        break
-      case "DELETE":
-        request = new DeleteMethod(uri.toString())
-        break
-      case "OPTIONS":
-        request = new OptionsMethod(uri.toString())
-        break
-      case "TRACE":
-        request = new TraceMethod(uri.toString())
-        break
-      default:
-        throw new IllegalStateException("Unsupported method: " + method)
-    }
-    headers.each { request.setRequestHeader(it.key, it.value) }
-    return request
-  }
+class CommonsHttpClientTest extends AbstractCommonsHttpClientTest {
 
   @Override
   int sendRequest(HttpMethod request, String method, URI uri, Map<String, String> headers) {
@@ -85,35 +16,4 @@ class CommonsHttpClientTest extends HttpClientTest<HttpMethod> implements AgentT
       request.releaseConnection()
     }
   }
-
-  @Override
-  boolean testCircularRedirects() {
-    false
-  }
-
-  @Override
-  boolean testReusedRequest() {
-    // apache commons throws an exception if the request is reused without being recycled first
-    // at which point this test is not useful (and requires re-populating uri)
-    false
-  }
-
-  @Override
-  boolean testCallback() {
-    false
-  }
-
-  @Override
-  boolean testReadTimeout() {
-    true
-  }
-
-  @Override
-  Set<AttributeKey<?>> httpAttributes(URI uri) {
-    Set<AttributeKey<?>> extra = [
-      SemanticAttributes.HTTP_SCHEME,
-      SemanticAttributes.HTTP_TARGET
-    ]
-    super.httpAttributes(uri) + extra
-  }
 }