Issue #620 PR2 break dependency on httpclient and replace with okhttp3

[xsteelej]

Closes #620

What has been done to verify that this works as intended?

I've tested this on a local VM Aggregate instance with both Digest and Basic authentication.
However, this has not yet been tested with an Ona server and therefore I'm not sure if this implementation suffers the same issue as described in this issue #2592.

Why is this the best possible solution? Were any other approaches considered?

The use of okhttp3 seemed to be the most obvious choice due to its popularity and ease of use. Most of the ground work has been done with #2257

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This change should not have any affect on users.

Do we need any specific form for testing your changes? If so, please attach one.

No

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

• run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[zestyping]

Hi @xsteelej, @lognaturel is on leave now; can you leave a note here when you believe you have addressed all of her outstanding questions and comments, and then I'll take a final look?

[xsteelej]

Hi @zestyping, thanks for your note and nice to meet you (virtually 😄)..

I'm just looking at the final bug which she spotted, which will also answer the question about my understanding of the caching as it's related.

Thanks

[xsteelej]

Hi @zestyping, I've completed the code updates and I feel it is ready for a review.

Just a couple of points:

• I'm not sure what the bug is that @lognaturel ran into when it was reported that "getting a form listing from Central: 11-02 04:50:25.506 3712-3859/org.odk.collect.android E/CollectServerClient: Parsing failed with Unexpected token.." - I was unable to reproduce.
• I haven't yet managed to test: "If an http server responds to a request with a 401 and requests basic auth without an https redirect, the client should not send auth headers" - I could probably do with some pointers on how to setup https on my aggregate vm (I tried following an example about how to configure Tomcat for SSL but it didn't work).

[xsteelej]

Hi @jd-alexander, do you have any time to take another look?

[shobhitagarwal1612]

@xsteelej I tried downloading blank forms & sending finalized forms using the default aggregate server and that worked perfectly.

[xsteelej]

Thanks for testing @shobhitagarwal1612

[grzesiek2010]

@zestyping do you have some time for reviewing?

[grzesiek2010]

I tried downloading blank forms & sending finalized forms using the default aggregate server and that worked perfectly.

@shobhitagarwal1612
Did you also have a chance to review the code or it was just manual testing?

[shobhitagarwal1612]

Did you also have a chance to review the code or it was just manual testing?

Yes, I did and it looks pretty clean to me.

[grzesiek2010]

@shobhitagarwal1612

Thanks!
I'm going to review this pr as well and hopefully, @zestyping and @ggalmazor will find some time too.

[yanokwa]

Hi folks! No need to review this one because I've asked @seadowg to review it. He is very familiar with OkHttp and once he looks at it, we can make a decision.

[seadowg]

Hey @xsteelej! I managed to have a look a this this evening. My interpretation of the aim of this PR is to switch the implementation of OpenRosaHttpInterface from HttpClientConnection to a new OkHttpConnection (that uses OkHttp). Given that goal I couldn't spot any problems in the code and the addition of OkHttp looks good. There are two thing I'd like to suggest though:

1. It is quite hard to to be confident that no minor changes in behavior have been introduced as there were no existing tests at an HTTP integration level for HttpClientConnection as far as I can see. I'll look into adding these so we can verify there are no minor differences between the two implementations of the interface. Given this I think what makes most sense is to change this PR so it doesn't remove the ol' HttpClientConnection (or its uses) but simply adds the new OkHttpConnection (which will be unused for the moment).

2. The Git commit history contains quite a lot of merge commits. Would it be possible to try and clean these up? I'd imagine it's possible to squash the commits down so the history is more readable and then force push the branch. As always SO to the rescue: https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

[xsteelej]

Hi @seadowg, thanks for the review!

1. What are your thoughts @yanokwa on the suggestion to merge this OkHttpClient and leave it unused for now? Another alternative could be that we merge it early in a release cycle and have it tested early.

2. I'll try and clean up the merge commits.. I did try, due to some previous comments, a while back and it didn't work out too well. However, I'll give it another go.

Thanks

[yanokwa]

@xsteelej I think merging unused for now is the right approach. It just gives us more flexibility as far as when exactly we turn it on. As to cleaning up the commits, you can make a branch off this current one to play around with with interactive rebasing.

https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history has a pretty good guide. https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History is also pretty good.

[yanokwa]

@xsteelej I'd like to get the requested changes merged a few weeks before our Feb 20th code freeze. Is that possible?

[xsteelej]

Hi @yanokwa, it should be yes - I've managed to squash most of the merge upstream/master commits on a branch, just looking at the latest conflicts and then i'll put back the httpConnection.

Thanks
John

[xsteelej]

To remove all the "Merge remote-tracking branch 'upstream/master'" commits I have created a new pull request: #2849 which replaces this one.

The new PR adds okhttp3 but it is not currently used.

[grzesiek2010]

closed in favor of #2849