Adding ClientFactory abstraction to allow alternate JAXRS implementations to be used

[jskjons]

We use RESTEasy in most of our applications/tooling and because the docker client currently uses the standard JAXRS client api it's hard to control which implementation gets used and it's impossible to customize the client if Resteasy gets picked. This abstractions allows us to just create RESTEasy clients and then only have 1 JAXRS implementation on our classpath. I've tested it with both Jersey and RESTEasy (not included) implementations and everything seems to work.

I also switched the JerseyClientFactory to directly use the JerseyClientBuilder so that it'll still use Jersey even if there's an alternate JAXRS implementation on the classpath.

[codecov-io]

Current coverage is 62.78% (diff: 75.78%)

Merging #340 into master will increase coverage by 16.44%

@@             master       #340   diff @@
==========================================
  Files           130         81     -49   
  Lines          4331       2940   -1391   
  Methods           0          0           
  Messages          0          0           
  Branches        635        403    -232   
==========================================
- Hits           2007       1846    -161   
+ Misses         2134       1094   -1040   
+ Partials        190          0    -190   

Powered by Codecov. Last update cbdd93b...128c40f

[jwgmeligmeyling]

Great work! The PR looks quite good. There are a few remarks from my side (as a contributor, not maintainer).

It seems the code is still dependent on org.glassfish.hk2.api.MultiException and org.glassfish.jersey.internal.util.Base64. Besides that this implies that (at least some) glassfish dependencies are still required on the classpath, I think that exceptions may not be handled properly when another HttpClient implementation is used.

I would love to see a few additions:

• Add resteasy-client (and its Jackson companion) as an optional and provided dependency:

<dependency>
    <groupId>org.jboss.resteasy</groupId>
    <artifactId>resteasy-client</artifactId>
    <version>3.0.16.Final</version>
    <scope>provided</scope>
    <optional>true</optional>
</dependency>

(In fact, this should also be done with the glassfish dependencies in my opinion, but that would be a breaking change for existing users, so lets stick to adding that as <exclusion> for now)

• Alter the intergration tests in such as way (either through test class inheritance or for example JUnit theories or parameterized) that the same tests are ran for both the glassfish and resteasy-client. This would address my concern that some exception handling (or for example any timeout vs. non-timeout issues) are not working properly.
• Some example usage of how to use another HttpClient can be used and specifically how the resteasy-client should be configured in practice would be nice!
• The PR should be up to date with master, so it can be merged :)

[jwgmeligmeyling]

ping @grexe

[jskjons]

I can add the optional dependencies but I'm not sure if it really adds much value since the user still has to add those dependencies themselves anyway. Unless you're going to fully support multiple JAXRS implementations I'm not sure how much value there is in kinda-sorta supporting it. I mainly wanted to clear the roadblock preventing usage of anything else.

If you want to full on support multiple implementations I think ideally there should be a few more breaking changes like extracting the dependencies and factories into separate modules.

I'm happy to add an example usage of using RESTEasy with this, should that go in user_manual.md?

[jwgmeligmeyling]

The dependencies should be included anyway in order to be able to test the Resteasy client. My opinion is still that this should be tested properly, including timeout and exception behaviour. So I would like to see the integration tests to be ran on both client implementations.

The optional inclusion would mostly be a cosmetic feature indicating it can be used with other clients.

[jskjons]

fair enough, I'll make that change today.

[jwgmeligmeyling]

Allright. I can have a look on Thursday to maybe add some tests if you'd like :)

[jskjons]

I added the ResteasyClientFactory implementation as well as deps + docs. It turned out there was a few of the features that we were using (streaming stuff in particular) that was a bit of a pain to get working in RESTEasy but the integration tests are passing now. The Events stuff currently directly relies on jersey so it would be more difficult to get working so I just left it for now and documented that fact.

[jwgmeligmeyling]

Whoa, great work! Will defenitely start using our fork 😄 Any idea on the last failing test case?

Tests in error: testEventStreamWithUntilTimeresteasy: The supplied component "org.jboss.resteasy.client.jaxrs.ResteasyClient" is not assignable from JerseyClient or JerseyWebTarget.

[jskjons]

that last test was failing because the events stuff doesn't work with resteasy so I needed to add

assumeThat(sut.getClient(), not(instanceOf(ResteasyClient.class)));

to all of those tests and missed one. Should be good now.

[jwgmeligmeyling]

Great! Waiting on response from the maintainers then.

[johnflavin]

That test failure on 1.10.3 is caused by the same problem as #412. That issue (or at least the test that failed because of the issue) was fixed in #415. Rebase this PR on master and that issue should go away.

[bitsofinfo]

+1

[mydeadlyvenoms]

Any updates on this?

[jskjons]

I haven't had time to keep the branch up to date with master and I haven't heard anything from the maintainers so it appears this isn't a priority or not a direction the maintainers want to go it. We're still internally using this branch so it still works.

[mydeadlyvenoms]

I am having trouble to use the library under Glassfish / Payara due to Jersey conflicts - this might be the solution.

[rmannibucau]

+1, i'd like to use CXF and JSONB instead of jackson, using portable API is really important