NPE in instrumentation/apache-httpclient/apache-httpclient-2.0/javaagent

[dgradecak]

If the httpclient executeMethod is used with 3 parameters like getClient(uri).executeMethod(null, request, null) which is a valid call
the v2 extensions does not add any traces since a NPE pops up at the line

url.append(hostConfiguration.getProtocol().getScheme());

Since this issues shows up only if the execution is using a different httpclient (apache commons) method signature => getClient(uri).executeMethod(null, request, null) instead of the tested getClient(uri).executeMethod(request)

I will provide a PR for this

[dgradecak]

after understanding how the build for different versions work, it seems I cannot make a logical test to make thinks work with http-client commons 2.0 and 3.1

This is an extract from buil.gradle.kts and it seems the extensions is meant to be used with those versions

dependencies {
  library("commons-httpclient:commons-httpclient:2.0")

  latestDepTestLibrary("commons-httpclient:commons-httpclient:3.+") // see apache-httpclient-4.0 module
}

since the signature(null, method, null) is not allowed in 2.0 but is in 3.1 and thus the test fails

furthermore when building with -PtestLatestDeps=true all tests are passing

could someone please point me to how this should be correctly tested so that the PR would be accepted?

should I rather create a supported extension for instrumentation/apache-httpclient/apache-httpclient-3.1?

[mateuszrzeszutek]

I think you could try passing the testLatestDeps param to test via system property:

tasks {
  test {
    systemProperty("testLatestDeps", findProperty("testLatestDeps") as Boolean)
  }
}

And then, you could use assumptions to skip that one particular test when you're not running latest deps.

[dgradecak]

thank you for the input @mateuszrzeszutek

I managed to do something, since I wanted the whole test class to be skipped

@IgnoreIf({ !Boolean.getBoolean("testLatestDeps") })
class CommonsHttpClientNPETest extends HttpClientTest<HttpMethod> implements AgentTestTrait {
 ...

is this something that is ok, since I could not see such a usage in the sources?

[mateuszrzeszutek]

It should be okay, I think -- just leave a comment that very briefly explains why it's being ignored and it'll be fine.

[laurit]

Resolved with #5949