Adobe-Marketing-Cloud / analytics-java-library

A Java client library for Analytics APIs
Apache License 2.0
15 stars 16 forks source link

Proxy support #2

Closed ByteFader closed 9 years ago

ByteFader commented 9 years ago

Is it possible to build in support for proxy usage?

Proxy proxy = new Proxy(Proxy.Type.HTTP, new InetSocketAddress("10.0.0.1", 8080)); conn = new URL(urlString).openConnection(proxy);

Changes needed for this:

trekawek commented 9 years ago

Good idea, it would be useful for debugging. Let's finish the #1 and then we can add the proxy support.

ByteFader commented 9 years ago

Do you have time to add the proxy support or should i make a PR for it? As we'll need it for a project asap if possible

trekawek commented 9 years ago

Could you take a look on the committed in the referenced branch? With this approach you can do something like this:

AnalyticsClient client = AnalyticsClient
    .authenticateWithSecret("my-username", "my-password", "api2.omniture.com")
    .withProxy("127.0.0.1", 8080);
ByteFader commented 9 years ago

Looks good but the authenticators should also get proxy support then. Otherwise you cannot run the whole library over a proxy.

trekawek commented 9 years ago

I missed that. Let me fix it...

trekawek commented 9 years ago

How about now? I used builders to avoid passing to many parameters to one method.

ByteFader commented 9 years ago

Looks good except for the fact that in the build() we do not check if the mandatory properties are filled for the corresponding type. This can be checked in the constructor of the authenticator or in the build method but it's harder to maintain if more auth method will be added. Maybe you could make a builder for each type of authenticator?

trekawek commented 9 years ago

I was thinking about this. User has to invoke one of the authenticateWith* methods, otherwise he'll get an IllegalStateException. Therefore, the only way to call AnalayticsClient#build() with authenticator chosen, but without required properties set is to pass null to the authenticateWith* method. In this case he'll probably get NPE somewhere from the authenticator - it's the same behaviour as in the current master.

I can add nullcheck to the AuthenticatorBuilder setters, so the feedback will be immediate - WDYT?

ByteFader commented 9 years ago

Well all mandatory properties should be nonnull for each authenticator. It's always better to throw an exception with an explanation then a nullpointer with a stacktrace in my opinion

trekawek commented 9 years ago

Added null checks to the setter methods.

trekawek commented 9 years ago

Merged.