Google-cloud-java: com.google.cloud.datastore.BaseKey.Builder is defined in a way that doesn't allow reflection.

[jrdoane]

The com.google.cloud.datastore.BaseKey.Builder subclass has the protected accessor applied to the sub-class itself. While this normally wouldn't cause a problem with regular use in Java, it does cause an IllegalArgumentException when reflection is done to dynamically call a method on the protected sub-class. This is particularly problematic when Clojure is used because most inter-op with Java involves reflection.

So, if a builder has already been created and the setKind method is dynamically called, this occurs:

IllegalArgumentException Can't call public method of non-public class: public com.google.cloud.datastore.BaseKey$Builder com.google.cloud.datastore.BaseKey$Builder.setKind(java.lang.String) clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:88)

This happens when attempting to use Clojure to call .setKind on an instance of com.google.cloud.datastore.KeyFactory produced by calling newKeyFactory on an instance of com.google.cloud.datastore.Datastore.

This can be solved by changing the sub-class accessor to public instead of being set to protected.

[vam-google]

Hi Jon. Let me clarify what will fix the issue you describe. So essentially changing this line from:

protected abstract static class Builder<B extends Builder<B>> {

to

public abstract static class Builder<B extends Builder<B>> {

Should fix the problem. Is that correct? If yes, then I'm happy to make/approve the change, as it is weird to have a protected builder for a public class, which is also inherited by multiple other public builders (for example)

[jrdoane]

@vam-google, that's correct. As I understand it, this is a known "bug" with the JVM since 1999 although, I do find it odd that it's protected without a real reason to be since it's intended to be used outside the scope of the parent class. The purpose might have been to prohibit public implementation of the interface while still allowing its usage but, I think that's probably not a good enough reason to keep it.

I initially figured this out from this StackOverflow article for a different sub-class.

[vam-google]

The fix is merged.