make ConnectionImpl public

[mccraigmccraig]

when used from clojure a package-protected ConnectionImpl triggers this bug -

http://dev.clojure.org/jira/browse/CLJ-1243

which is perhaps a symptom of this underlying issue with reflection -

http://bugs.java.com/bugdatabase/view_bug.do?bug_id=4283544

the effect is that trying to call ConnectionImpl methods overridden from AbstractConnection, such as subscribeAsync, throws an exception

IllegalArgumentException Can't call public method of non-public class: public io.nats.client.AsyncSubscription io.nats.client.ConnectionImpl.subscribeAsync(java.lang.String)  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:88)

so this PR makes the ConnectionImpl class public, which solves the issue, and i can't see a downside

[pires]

Shouldn't the interface be called instead of the impl?

Connection conn = new ConnectionImpl(...);
conn. subscribeAsync(...);

[mccraigmccraig]

clojure's java interop facility uses vanilla java reflection to look for the method on the object instance presented, which is necessarily of the impl type, but neither the abstract connection type nor the impl type are explicitly referenced.

the connection is created here -

https://github.com/employeerepublic/clj-nats-async/blob/master/src/clj_nats_async/core.clj#L8

and subscribeAsync is called here -

https://github.com/employeerepublic/clj-nats-async/blob/master/src/clj_nats_async/core.clj#L27

[mcqueary]

Hi @mccraigmccraig -- Hmmm. I'm (we're) not completely against this idea, but would like to invest some time investigating a fix that doesn't require making ConnectionImpl public. Please bear with me.

[mccraigmccraig]

hi @mcqueary, no problem - i've got a private artefact so i'm not in any hurry

i could (but didn't try) have worked around the issue by creating a public java proxy class in my project, but since the issue will be encountered by anyone using reflection, e.g. jruby, groovy etc it seemed reasonable for jnats to have the solution - and a public ConnectionImpl is certainly a simple solution :)

[mcqueary]

@mccraigmccraig Totally understand your thought process, and it does appear to be the path of least resistance. I had not considered (obviously) nor tested for effects of reflection-based APIs. I wouldn't really want you to have to use a proxy; and any interface should be enforced by contract and documentation, not necessarily by Java scope enforcement. Perhaps this is one of the cases where that point of view wins.

[mcqueary]

@mccraigmccraig I relented :) I couldn't find a more elegant resolution that wouldn't involve a considerable refactoring effort. Although ConnectionImpl remains private by contract, it is now public in scope.

[mccraigmccraig]

pragmatism FTW! thanks @mcqueary :)