keenlabs / KeenClient-Java

Official Java client for the Keen IO API. Build analytics features directly into your Java apps.
https://keen.io/docs
MIT License
74 stars 43 forks source link

Add HTTP Proxy support. #25

Closed smurthas closed 10 years ago

smurthas commented 10 years ago

Lots of ideas and code from #13, thanks to @ryber for the starting point!

Still needs a solid test for the changes to UrlConnectionHttpHandler, but I figured I'd get this PR going so we can discuss the overall strategy in parallel. The basic idea is that each KeenClient instance has a private proxy variable and that is passed every time a Request object is created. If a Request has a proxy set, UrlConnectionHttpHandler.openConnection uses it.

joshed-io commented 10 years ago

Rad. Makes a lot of sense.

Geeber commented 10 years ago

I like the approach. It seems pretty clean. I'd like to see some more automated testing, if possible, and it would also be good to verify that it actually works (like, with a real app and a real proxy). Just a one time manual test is fine, I just want to validate the end-to-end.

I have some sample code (in both Android and plain Java) to exercise this SDK, but unfortunately it's not up-to-date on GitHub. If it would be helpful to get those in your hands, let me know and we can sync up on the best way to do that.

smurthas commented 10 years ago

I'm planning on adding an automated test as part of the core/.../UrlConnectionHttpHandlerTest.java file that verifies that the proxy gets used if it is set. Do you think it's worth going beyond that and fully mocking a proxy in the test suite?

I did verify it by hand with a real app and real proxy. It was a bit finicky because it required that the proxy support tunneling HTTPS traffic (which, I came to find out, many don't), however if they don't want that behavior, they can simply use the setBaseUrl functionality and point it at the proxy directly.

Also, should I add documentation to the readme? This seems like pretty advanced functionality, so I wasn't sure if the readme was the right place.

ryber commented 10 years ago

I think you will find that for a lot of larger corporate users or companies that have tight security, proxies are quite common. So I think it would be worth a mention on the readme..even if it's just one line with a link to a specific wiki page on how to use it.

Geeber commented 10 years ago

Hmm, I think that just verifying that it gets used is probably good enough for now. If we run into issues in the wild then we can flesh out the test behavior.

Glad to hear that you manually verified the behavior. That always makes me a lot more comfortable pushing something out :)

In terms of documentation, I think @ryber is right that it might be worth a mention. As I may have said before, I have some significant doc improvements in the works so I can try to handle this better whenever I get a day or two to push through and finish/deploy those. For now a simple mention in the main README should be fine. Thanks!

smurthas commented 10 years ago

I've been working on adding a test to ensure the Proxy gets used and I've come to a bit of a dead end. The issue is that the 5 lines in question end up calling methods of the URL object, which is a final class, so it can't be mocked and it isn't an implementation of an interface, so it can't simply be replaced with dependency injection.

Any other ideas on how to handle this or should we just put it aside and reconsider it if issues crop up in the future?

joshed-io commented 10 years ago

Ah, yuck. Seems like a lot of hassle for low return, I'd have no problem punting on it.

Geeber commented 10 years ago

Yeah, I feel like I've run into that problem as well. I'm OK punting on it, we'll just have to keep our ears open for any issues that people have.

Other than that, is this PR ready to go?

smurthas commented 10 years ago

Yep, good to go.

joshed-io commented 10 years ago

:+1:

Geeber commented 10 years ago

Thanks Simon. I'll need to do a bit of work to push a 2.0.3 release up to maven central. I'll try to get to that ASAP. (If you have interest in improving automation around releases, by the way, we can definitely talk about that as a project at some point ;) )

Geeber commented 10 years ago

FYI, 2.0.3 has been released. I'm going to work on some sample app stuff tomorrow as part of a documentation push, so I'll use that to smoke-screen the release as well.