New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OkHttp client #416

Merged
merged 26 commits into from Aug 3, 2016

Conversation

Projects
None yet
4 participants
@kiwiandroiddev
Contributor

kiwiandroiddev commented Jul 28, 2016

This introduces an alternative HTTP client for HAPI-FHIR utilising Square's OkHttp library. This is primarily intended as a replacement for the default Apache client on the Android platform as Apache HTTP is very problematic on Android.

This client was developed using Agfa Healthcare's JaxRs implementation as a reference - thanks very much to their team.

Tests
The GenericDstu2ClientTest suite has been copied and modified just enough to test this implementation. If it's possible, it seems like it'd be good to make this a purely generic suite of acceptance tests that aren't coupled to any particular HTTP client implementation, such that there is only one copy in the project. I'm not sure on how to implement this but am open to helping if there are any ideas.

Matthew Clarke added some commits Jul 18, 2016

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Jul 28, 2016

Hey Matt- Wow, this looks awesome!

I'm just getting ready to head out the door for an extended long weekend so I won't get to look at this until next week, but I'm looking forward to it already. :)

@markiantorno

This comment has been minimized.

Collaborator

markiantorno commented Jul 28, 2016

Looks good to me. Might want to do an auto-format to clean up some of the extra blank space.

Great stuff.

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Aug 2, 2016

Any idea why Travis CI is failing that one test method?


-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running ca.uhn.fhir.okhttp.GenericOkHttpClientDstu2Test
Tests run: 52, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 2.967 sec <<< FAILURE! - in ca.uhn.fhir.okhttp.GenericOkHttpClientDstu2Test
testSearchReturningDstu2Bundle(ca.uhn.fhir.okhttp.GenericOkHttpClientDstu2Test)  Time elapsed: 0.004 sec  <<< ERROR!
java.lang.NullPointerException
    at ca.uhn.fhir.okhttp.GenericOkHttpClientDstu2Test.testSearchReturningDstu2Bundle(GenericOkHttpClientDstu2Test.java:1430)
@kiwiandroiddev

This comment has been minimized.

Contributor

kiwiandroiddev commented Aug 2, 2016

Hi James, no ideas so far, I'm having trouble getting this failure to replicate on my machine (running all JUnit tests in the hapi-fhir-okhttp module directly from the IDE passes). And when I run mvn install to run all unit tests I get this:

Tests run: 17, Failures: 0, Errors: 4, Skipped: 3, Time elapsed: 2.382 sec <<< FAILURE! - in ca.uhn.fhir.jaxrs.server.example.JaxRsPatientProviderTest
testUpdateById(ca.uhn.fhir.jaxrs.server.example.JaxRsPatientProviderTest)  Time elapsed: 0.116 sec  <<< ERROR!
ca.uhn.fhir.rest.server.exceptions.InternalErrorException: HTTP 500 Internal Server Error: Failed to call access method
        at ca.uhn.fhir.jaxrs.server.example.JaxRsPatientProviderTest.testUpdateById(JaxRsPatientProviderTest.java:214)

As far as GenericOkHttpClientDstu2Test, this line seems to be the culprit:

String msg = IOUtils.toString(GenericOkHttpClientDstu2Test.class.getResourceAsStream("/bundle_orion.xml"));

I'm assuming the NPE means bundle_orion.xml can't be found for some reason but will keep looking into it...

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Aug 2, 2016

Ahh, that should be easy enough.. You need to copy the file bundle_orion.xml from hapi-fhir-structures-dstu2/src/test/resources to hapi-fhir-okhttp/src/test/resources

@coveralls

This comment has been minimized.

coveralls commented Aug 2, 2016

Coverage Status

Changes Unknown when pulling 835654c on orionhealth:okhttp-client into * on jamesagnew:master*.

@kiwiandroiddev

This comment has been minimized.

Contributor

kiwiandroiddev commented Aug 2, 2016

Awesome, that did it. Thanks James :-)

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Aug 3, 2016

Ah fantastic! How ironic that it was a file called orion that tripped it up. I think that was a test for a bug that David Hay reported.

So, I've finally had a chance to go over this, and it looks great. I'm going to merge now.

The main other question I have is what else to do with it. It seems obvious to me that the Android library should just use this by default (ironically @markiantorno and I were just talking last week about replacing the Apache client for Android and you beat us to it :) ). I'm kind of tempted to replace HttpClient entirely with this implementation though. It seems much more friendly to use than HttpClient. I guess the one thing holding us back would be the Java 7 dependency (most of HAPI still works on 6 so I'm trying to keep to that since there are people using 6 still). Maybe the right thing to do would be to switch when we stop supporting Java 6.. which surely will happen one of these days.

jamesagnew added a commit that referenced this pull request Aug 3, 2016

@jamesagnew jamesagnew merged commit 06b9059 into jamesagnew:master Aug 3, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 87.835%
Details
@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Aug 3, 2016

BTW- I'm going to rename the module from hapi-fhir-okhttp to hapi-fhir-client-okhttp, hope that's ok. HAPI's root project is starting to have so many submodules that it's getting unweildy, so I'm going to try and organize them a bit better.

@kiwiandroiddev

This comment has been minimized.

Contributor

kiwiandroiddev commented Aug 5, 2016

Thanks James, no worries about the module rename! Yeah OkHttp's API is very developer-friendly. If you think it would make sense as the default implementation at some stage that would be awesome. Something I was wondering about was if the hapi-fhir-android module at least could be configured to use this as its default http client, but I'm not hugely experienced with maven.
As for other improvements, I also noticed there is now a suite of http client tests for dstu3 - I'm guessing the OkHttp client should really be tested against these too at some point?
One other thing; currently the OkHttp client makes use of Constants.HEADER_ACCEPT_VALUE_XML_OR_JSON_LEGACY. I'm not sure how much of a problem this is (i.e. when should it be migrated to HEADER_ACCEPT_VALUE_XML_OR_JSON_NON_LEGACY)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment