Eliminate dependency on custom httpclient implementation

[batkinson]

As discussed in the recent issue #611, rather than maintaining our own custom http client, requiring its own maintenance and fixes for issues like #199, we should instead migrate to using a more "mainstream" http implementation for android.

The ultimate reason is to essentially to eliminate the need to worry about defects and security issues requiring maintenance to keep the custom library working properly.

[lognaturel]

And this is the repo the lib is currently built from.

[alxndrsn]

I'd be interested in taking a shot at this, but it might be tricky to test. Is there a server online that I can check basic functionality against?

[alxndrsn]

@mitchellsundt over on #611, you said:

Just a reminder - the hurdle is getting the "standard framework" to implement DigestAuth.

Any more info on this one?

[lognaturel]

@alxndrsn I think this may have at least part of the explanation.

For testing, what about using the Aggregate VM? Alternately, let us know what kind of server config you need and we can probably get one up for you.

[xsteelej]

I've spent some time looking at all the places in the collect code where it is dependant on httpclient.

Is the expectation to replace it with the standard HttpURLConnection or something else such Volley?

Also, any tips on how to test uploading? Is there a server that I can use to test form uploading?

[lognaturel]

Thanks for looking into it, @xsteelej. To be crystal clear, what's going on is that Collect has kept using HttpClient after it was deprecated and maintains this repo to hold the version constant regardless of Android version and patch it as needed (e.g. for #199). The challenge with migrating away from HttpClient is of course that a lot of mission-critical code will need to change but also that HTTP digest support is not as straightforward in modern libs.

From my quick reading about Volley, I'm not sure it would provide a ton of benefit in this context. It seems to make a big performance difference in apps that, for example, let users scroll through remotely-fetched content quickly or otherwise need to make lots of quick requests and cache content.

It seems it does provide nice APIs but would require changing the structure of the code quite a bit so I think some kind of tangible benefit would need to be provided.

HttpUrlConnection uses okhttp under the hood in Android 4.4+. There may be some benefits to using the okhttp APIs directly, though, so that could be another option. For example, https://github.com/rburgst/okhttp-digest seems like a nicely-vetted lib that makes digest auth easy with okhttp. Is that usable with HttpUrlConnection or does okhttp need to be included directly? Any lib that we would consider should not increase the binary size by much.

In terms of testing, for manual verification the default https://opendatakit.appspot.com server can be used. Is that what you were asking about? You can also set up a local Aggregate virtual machine - https://docs.opendatakit.org/aggregate-vm/. I think for automated testing mocks should be sufficient.

[xsteelej]

Thanks for the detailed response @lognaturel, after reading more about Volley I have come to the same conclusion as you - that the refactor would be too great for the benefit it provides.

I am currently targeting WebUtils.getXmlDocument() and have started using the standard HttpUrlconnection. But as you pointed out, digest authentication is not provided by default, only basic.

There is another library that I have found, that adds digest to HttpUrlConnection: bare-bones-digest. It may be worth considering that.

It looks like I need to do a bit more research into ok http and HttpUrlConnection + libraries to see which one would have the least impact on the current structure of the code.

Thanks for the tips on testing!

[xsteelej]

Work in progress

[xsteelej]

Work in progress

[xsteelej]

opendatakit-bot claim

[andreimarcut]

@xsteelej I think I could help, in case u need an extra hand..

[xsteelej]

@andreimarcut, thanks for the offer. I've got fairly far down the road with this one. I plan to make two pull requests:

1. A pull request that has refactored the code so that the rest of the app just uses an interface to make HTTP requests, with a concrete implementation that is still dependant on HTTPClient. Basically this will have reduced the dependency down to one place.

2. The second pull request will swap out the HTTP Client Concrete implementation with another that uses some other http library (favourite being ok-http at the moment).

I'm almost done with step 1 above, it would be handy for a number of eyes to look over that code and test.

At stage 2, I'd like to open up a discussion about which is the best http client to use. Help would be great at this stage, especially testing.

[andreimarcut]

@xsteelej I am available to help anywhere down the road, just notify me

[xsteelej]

Thanks @andreimarcut, I appreciate the offer 👍

[xsteelej]

@andreimarcut, I raised PR part 1 for this issue here: #2257. It is the refactor that will make swapping out httpclient easier. It would be great to get some feedback on this.

[andreimarcut]

Great, I'll have a look

[lognaturel]

The dependency on HTTPClient is now fully isolated in HttpClientConnection thanks to work by @xsteelej.

@xsteelej, I seem to remember reading in #2257 that you had started a branch swapping out the lib with okhttp. Is that something you'd like to complete and/or share a work in progress for?