Update to Apache HttpClient 4.5.x

[proski]

The commits before "Update to Apache httpclient 4.5.2" are cleanups
"Update to Apache httpclient 4.5.2" has been salvaged from nemccarthy#103
The next commit removes EasySSLProtocolSocketFactory, which is no longer needed
The final commit contains additional cleanups.

[proski]

The original PR: nemccarthy#103

I tested all 3 used methods (GET, POST, DELETE), everything appears to be OK. The error handling is reasonable. If multiple PR builders try to merge the same PR, one succeeds to merge, others fail to merge with HTTP error 409 Conflict and report successful build to Stash as expected.

[proski]

More improvements for the exception handling. RuntimeError is never thrown, there are no SEVERE messages anymore. An error is either logged or thrown as an exception, not both. Casts to long have been removed in a separate commit.

[jakub-bochenski]

The changes look good, but I'm worried about doing this with no test coverage.

Could you consider adding some client tests against a mock server (e.g. http://www.mock-server.com/) ?

[proski]

The changes look good, but I'm worried about doing this with no test coverage.

Could you consider adding some client tests against a mock server (e.g. http://www.mock-server.com/) ?

Good idea. I'll try to add the tests when I have a chance.

[jakub-bochenski]

see above

[proski]

It looks like the StashApiClient class needs to be split to be testable. The low-level class (let's call it StashHttpClient) should just do the HTTP requests with plugin-specific parameters. The high-level class should use those requests for Stash specific operations, such as posting PR comments.

If we remove running requests in a separate thread, we can test the code by mocking the Apache HTTP code without any mock server.

As a first step, I'm trying to get all cleanups outside the main commit (HTTP client update) and get them merged separately. The main commit should not be doing any cleanups to be easy to review.

[proski]

All changes below the @loicalbertin's commit have been submitted as #83, they don't need to wait.

I've checked other plugins. StashNotifier encoded the context into the client, which is probably a good idea, which would reduce the changes. See https://github.com/jenkinsci/stashnotifier-plugin/blob/39671348994312f6da45ac3a04639e1aa99edbe0/src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java#L422

I'm also not convinced if we should run HTTP requests in a separate thread, especially considering that we only use it to enforce the request timeout. There should be an easier way to do it.

[proski]

Updated to remove an extra dependency on httpclient 4.5.2. The dependency is already in the master branch.

[jakub-bochenski]

FTR I'm sill waiting for the TA coverage here

[proski]

FTR I'm sill waiting for the TA coverage here

I know, it should be easier now.

[jakub-bochenski]

Bump?
Just checking if I missed it, but I think we still have no tests here. I would really prefer to have some before doing any changes to the stash client.

If you have trouble setting it up I can try to hack something together.

[proski]

I started experimenting with wiremock, but didn't have a chance to write proper tests yet.

[proski]

Rebased on top of #109, which includes extensive tests for StashApiClient. Once #109 is merged, this PR should be ready for merging.

I dropped changes to log and exception messages. Those change should be redone in a separate PR with unit tests to demonstrate that the messages are correct and easy to understand.

[proski]

Using HttpClient 4.5.9, the latest version. Removed dependency on commons-codec, it's not used directly in the code. No code changes were needed.

[proski]

I was able to avoid using HttpContext while keeping preemptive basic authentication, as suggested in
https://stackoverflow.com/questions/2014700/

That makes the change shorter and easier to review. I moved the original author's name to the comment, as the commit has changed quite a lot from the original version.

We can avoid using preemptive authentication, but then we should avoid the full authentication for requests other than the first one. It can be done by reusing the CloseableHttpClient object between requests, and that's definitely a topic for a separate PR.

[jakub-bochenski]

Removed dependency on commons-codec, it's not used directly in the code.

Just checking: given the nature of this dependency, are you sure it's not needed during runtime?

[proski]

We don't use that dependency directly. As I understand, we specified a relatively new version 1.10 to resolve a version conflict. The new httpclient requires an even newer version 1.11. That takes care of the conflict. I think it's better not to get involved in version resolution if we don't have to, especially since we are not direct users of the dependency.

[proski]

I took a closer look at the dependencies.

When Maven resolves versions, it uses two rules

• Shorter dependency chain wins
• Between the dependency chains of the same length, the one introduced earlier wins.

More information here: https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html

Without this PR, i.e. on the master branch

• commons-httpclient 3.1 (i.e. the old httpclient) is the first dependency, it requires commons-codec 1.2
• commons-codec 1.10 is required explicitly
• httpclient 4.5.2 (i.e. the new httpclient) requires commons-codec 1.2

The commons-codes dependency wins because it's the shortest (direct). Other dependencies are OK with commons-codec 1.10.

mvn dependency:analyze shows that the dependency on commons-codec is unused.

If I remove the dependency on commons-codec, I have a conflict, as commons-httpclient has its way and sets commons-codec version to 1.2, breaking httpclient.

However, if I put commons-httpclient below httpclient in pom.xml, the dependency issue is resolved. That would downgrade commons-codec from 1.10 to 1.9, but all dependencies are OK with it.

With this PR as originally submitted

• commons-httpclient dependency is removed
• commons-codec 1.10 is required explicitly and determines the resolution
• httpclient 4.5.2 requires commons-codec 1.8, so it's OK with 1.10
• other dependencies are OK with 1.10

With this PR as it is now

• commons-httpclient dependency is removed
• commons-codec 1.10 is required explicitly
• httpclient 4.5.2 requires commons-codec 1.11 and wins the resolution
• other dependencies are OK with commons-codec 1.11

[proski]

Just checking: given the nature of this dependency, are you sure it's not needed during runtime?

Given that commons-codec is not our direct dependency, it's up to out dependencies to decide whether commons-codec is needed at the runtime.

I did check the plugin build this PR on a Jenkins server, it works fine. Basic authentication works over HTTP and HTTPS. Builds work, that involves all methods we have in the code (GET, DELETE, POST).

[jakub-bochenski]

Given that commons-codec is not our direct dependency, it's up to out dependencies to decide whether commons-codec is needed at the runtime.

I meant that sometimes codec-like dependencies are optional. It's up to the user to add them if needed.

If you checked it on a jenkins server that's enough to verify it.

[jakub-bochenski]

httpclient 4.5.2 requires commons-codec 1.11 and wins the resolution

We shouldn't depend on dependecy order in pom.xml. This is too birttle. Ideally we would pass a full dependecy covergence check
For now, we should specify the commons-codec in a <dependencyManagement> section instead. This will set the version without declaring a dependency on commons-codec

[proski]

I've removed the upgrade to httpclient 4.5.9 from this PR, including the commons-codec removal. I wanted to use the latest client to avoid depending on any obsolete methods. But everything is working with httpclient 4.5.2.

Dependency cleanup can be done in a separate PR.

[proski]

We shouldn't depend on dependecy order in pom.xml. This is too birttle.

I agree.

Ideally we would pass a full dependecy covergence check

I tried that and I don't like the result. I had to declare 28 dependencies in dependencyManagement. I don't see any easy way to keep that information up to date if we update some dependencies. I don't get a warning about incorrect entries (the dependency in not used at all) or useless entries (there is no version conflict).

In most cases I just need the oldest version that satisfies all dependencies. I wish I could just tell Maven to use that logic instead of what it does now. dependencyManagement should be reserved for rare non-trivial conflicts and to force newer version of the dependencies, perhaps to fix security issues.

That said, I found some issues that could be easily fixed and submitted them as #121.

[proski]

If #120 is merged first, this PR would make us use HttpClient 4.5.5. So I'm not trying to document the exact version of HttpClient anymore.

While at that, I added a little warning fix found by SpotBugs. We should not declare two exceptions in the throws clause if one is subclass of another.

[proski]

The same variable response was used for two different things, the reason (e.g. Not found) and for the actual response (a JSON file). Rectified that without making the change longer (it's actually shorter now). The unit tests are not affected.