Use alternate name for static members with readonly property names

jimlloyd · jimlloyd · commit 98c9b79a23f2 · 2015-04-29T11:08:50.000-07:00
diff --git a/README.md b/README.md
@@ -86,7 +86,7 @@ _See testIntegration/webkit for a working example_
 
 ## Using node-java in existing maven projects
 
-When using node-java in existing maven projects, all the dependencies and the class files of the project have to be pushed to the classpath. 
+When using node-java in existing maven projects, all the dependencies and the class files of the project have to be pushed to the classpath.
 
 One possible solution would be:
 
@@ -571,7 +571,7 @@ __Example__
 
     var thread = java.newInstanceSync("java.lang.Thread", myProxy);
     thread.start();
- 
+
 <a name="javaisJvmCreated" />
 **java.isJvmCreated()**
 
@@ -697,6 +697,53 @@ ShutdownHookHelper.setShutdownHookSync(java.newProxy('java.lang.Runnable', {
 
 When you call a Java method through node-java, any arguments (V8/JavaScript objects) will be converted to Java objects  on the v8 main thread via a call to v8ToJava (found in utils.cpp). The JavaScript object is not held on to and can be garbage collected by v8. If this is an async call, the reference count on the Java objects will be incremented. The Java method will be invoked in a node.js async thread (see uv_queue_work). When the method returns, the resulting object will be returned to the main v8 thread and converted to JavaScript objects via a call to javaToV8 and the Java object's reference count will then be decremented to allow for garbage collection. The resulting v8 object will then be returned to the callers callback function.
 
+# Static member name conficts ('name', 'arguments', 'caller')
+
+The Javscript object returned by `java.import(classname)` is a Javascript constructor Function, implemented such that you can create instances of the Java class. For example:
+
+```javascript
+var Test = java.import('Test');
+var test = new Test();
+
+Test.someStaticMethod(function(err, result) { ... });
+
+var value1 = Test.NestedEnum.Value1;
+```
+
+But Javascript reserves a few property names of Function objects: `name`, `arguments`, and `caller`. If your class has public static members (either methods or fields) with these names, node-java is unable to create the necessary property to implement the class's API. For example, suppose your class `Test` implements a static method named `caller`, or has a `NestedEnum` with a value `name`:
+
+```java
+public class Test {
+    ...
+    public static String caller() { return "something"; }
+    public enum NestedEnum { foo, name };
+}
+```
+
+In Javascript, you would expect to be able to use those static members like this:
+
+```javascript
+var Test = java.import('Test');
+Test.caller(function(err, result) { ... });  // ERROR
+var value = Test.NestedEnum.name;  // ERROR
+```
+
+Node-java can't create those properties, so the above code won't work. Instead, node-java appends a suffix to the name. The default suffix is simpy an underscore `_`, but you can change the suffix using asyncOptions:
+
+```javascript
+var java = require('java');
+
+java.asyncOptions = {
+  asyncSuffix: "",
+  syncSuffix: "Sync",
+  ifReadOnlySuffix: "_alt"
+};
+
+var Test = java.import('Test');
+Test.caller_alt(function(err, result) { ... });  // OK
+var value = Test.NestedEnum.name_alt;  // OK
+```
+
 # Troubleshooting
 
 ## Error: Cannot find module '../build/jvm_dll_path.json'
diff --git a/lib/nodeJavaBridge.js b/lib/nodeJavaBridge.js
@@ -21,6 +21,7 @@ java.nativeBindingLocation = binaryPath;
 
 var syncSuffix = undefined;
 var asyncSuffix = undefined;
+var ifReadOnlySuffix = '_';
 
 var SyncCall = function(obj, method) {
   if (syncSuffix === undefined)
@@ -170,6 +171,10 @@ java.onJvmCreated = function() {
     } else {
       throw new Error('In asyncOptions, if either promiseSuffix or promisify is defined, both most be.');
     }
+
+    if (_.isString(java.asyncOptions.ifReadOnlySuffix) && java.asyncOptions.ifReadOnlySuffix !== '') {
+      ifReadOnlySuffix = java.asyncOptions.ifReadOnlySuffix;
+    }
   } else {
     syncSuffix = 'Sync';
     asyncSuffix = '';
@@ -179,19 +184,26 @@ java.onJvmCreated = function() {
 var MODIFIER_PUBLIC = 1;
 var MODIFIER_STATIC = 8;
 
-function isWritable(obj, prop) {
-  if (!prop || !obj) {
-    return false;
-  }
+function isWritable(prop) {
+  // If the property has no descriptor, or wasn't explicitly marked as not writable or not configurable, assume it is.
+  // We check both desc.writable and desc.configurable, since checking desc.writable alone is not sufficient
+  // (e.g. for either .caller or .arguments).
+  // It may be that checking desc.configurable is sufficient, but the specification doesn't make this definitive,
+  // and there is no harm in checking both.
+  var desc = Object.getOwnPropertyDescriptor(function() {}, prop) || {};
+  return desc.writable !== false &&  desc.configurable !== false;
+}
 
-  // If the property has no descriptor, or wasn't explicitly marked as not writable,
-  // assume it is.
-  return (Object.getOwnPropertyDescriptor(obj, prop) || {}).writable !== false;
+function usableName(name) {
+  if (!isWritable(name)) {
+    name = name + ifReadOnlySuffix;
+  }
+  return name;
 }
 
 java.import = function(name) {
   var clazz = java.findClassSync(name); // TODO: change to Class.forName when classloader issue is resolved.
-  var result = function() {
+  var result = function javaClassConstructorProxy() {
     var args = [name];
     for (var i = 0; i < arguments.length; i++) {
       args.push(arguments[i]);
@@ -209,10 +221,11 @@ java.import = function(name) {
     if (((modifiers & MODIFIER_PUBLIC) === MODIFIER_PUBLIC)
       && ((modifiers & MODIFIER_STATIC) === MODIFIER_STATIC)) {
       var fieldName = SyncCall(fields[i], 'getName')();
-      result.__defineGetter__(fieldName, function(name, fieldName) {
+      var jsfieldName = usableName(fieldName);
+      result.__defineGetter__(jsfieldName, function(name, fieldName) {
         return java.getStaticFieldValue(name, fieldName);
       }.bind(this, name, fieldName));
-      result.__defineSetter__(fieldName, function(name, fieldName, val) {
+      result.__defineSetter__(jsfieldName, function(name, fieldName, val) {
         java.setStaticFieldValue(name, fieldName, val);
       }.bind(this, name, fieldName));
     }
@@ -232,19 +245,19 @@ java.import = function(name) {
     if (((modifiers & MODIFIER_PUBLIC) === MODIFIER_PUBLIC)
       && ((modifiers & MODIFIER_STATIC) === MODIFIER_STATIC)) {
       var methodName = SyncCall(methods[i], 'getName')();
-      var syncName = methodName + syncSuffix;
-      var asyncName = methodName + asyncSuffix;
-      var promiseName = methodName + promiseSuffix;
 
-      if (isWritable(result, syncName)) {
+      if (_.isString(syncSuffix)) {
+        var syncName = usableName(methodName + syncSuffix);
         result[syncName] = java.callStaticMethodSync.bind(java, name, methodName);
       }
 
-      if (typeof asyncSuffix === 'string' && isWritable(result, asyncName)) {
+      if (_.isString(asyncSuffix)) {
+        var asyncName = usableName(methodName + asyncSuffix);
         result[asyncName] = java.callStaticMethod.bind(java, name, methodName);
       }
 
-      if (promisify && isWritable(result, promiseName)) {
+      if (promisify && _.isString(promiseSuffix)) {
+        var promiseName = usableName(methodName + promiseSuffix);
         result[promiseName] = promisify(java.callStaticMethod.bind(java, name, methodName));
       }
     }
diff --git a/test/Test$Enum.class b/test/Test$Enum.class
diff --git a/test/Test.class b/test/Test.class
diff --git a/test/Test.java b/test/Test.java
@@ -171,7 +171,23 @@ public static enum StaticEnum {
   public static String varArgsSignature(Long... args) { return "Long..."; }
   public static String varArgsSignature(Number... args) { return "Number..."; }
   public static String varArgsSignature(String... args) { return "String..."; }
-  // Test readonly properties on functions that throw errors in certain versions of
-  // node.
-  public static void name() {}
+
+  // The Javascript object returned by java.import(classname) is a Function object
+  // so that it can be used as a constructor.
+  // Javascript reserves some properties of Function as non-writable or non-configurable:
+  //  'name', 'arguments', 'caller'.
+  // This means we can't expose a static member such as 'name()' with that name.
+  // We instead append a suffix (asyncOptions.ifReadOnlySuffix)
+  // The following static members are used for unit tests involving these cases.
+
+  // For testing static member functions with reserved names
+  public static String name() { return "name"; }
+  public static String arguments() { return "arguments"; }
+  public static String caller() { return "caller"; }
+
+  // For testing public static fields with reserved names
+  public enum Enum {
+    foo, bar,                 // non-reserved
+    name, arguments, caller   // reserved
+  };
 }
diff --git a/test/importClass-test.js b/test/importClass-test.js
@@ -3,6 +3,7 @@
 var java = require("../testHelpers").java;
 var nodeunit = require("nodeunit");
 var util = require("util");
+var _ = require("lodash");
 
 exports['Import Class'] = nodeunit.testCase({
   tearDown: function (callback) {
@@ -25,5 +26,43 @@ exports['Import Class'] = nodeunit.testCase({
       test.equals(5, testObj.getIntSync());
       test.done();
     });
+  },
+
+  "import TestEnum with unsable name": function (test) {
+    test.expect(5);
+    var TestEnum = java.import('Test$Enum');
+
+    // 'foo' and 'bar' are valid enum names
+    test.strictEqual(TestEnum.foo.toStringSync(), "foo");
+    test.strictEqual(TestEnum.bar.toStringSync(), "bar");
+
+    _.forEach(['name', 'arguments', 'caller'], function(prop) {
+      test.throws(
+        function() {
+          // The enum also defines 'name', 'caller', and 'attributes', but Javascript prevents us from using them,
+          // since these are unwritable properties of Function.
+          var x = TestEnum[prop].toStringSync();
+        },
+        TypeError
+      );
+    });
+    test.done();
+  },
+
+  "import TestEnum and use alternate name": function (test) {
+    test.expect(5);
+    var TestEnum = java.import('Test$Enum');
+
+    // 'foo' and 'bar' are valid enum names
+    test.strictEqual(TestEnum.foo.toStringSync(), "foo");
+    test.strictEqual(TestEnum.bar.toStringSync(), "bar");
+
+    // 'name', 'caller', and 'arguments' are not, so we must use e.g. 'name_' to reference the enum.
+    // But note that the value is still e.g. "name".
+    test.strictEqual(TestEnum.name_.toStringSync(), "name");
+    test.strictEqual(TestEnum.arguments_.toStringSync(), "arguments");
+    test.strictEqual(TestEnum.caller_.toStringSync(), "caller");
+    test.done();
   }
+
 });
diff --git a/test/java-callStaticMethod-test.js b/test/java-callStaticMethod-test.js
@@ -268,7 +268,7 @@ exports['Java - Call Static Method'] = nodeunit.testCase({
     test.equal(str, "Value1");
     test.done();
   },
-  
+
   "Call static method with varargs": function(test) {
     var Test = java.import("Test");
 
@@ -279,6 +279,14 @@ exports['Java - Call Static Method'] = nodeunit.testCase({
     test.equal(str, "5abc");
 
     test.done();
-  }
+  },
 
+  "Call static method named name_": function(test) {
+    test.expect(1);
+    var Test = java.import("Test");
+    Test.name_(function(err) {
+      test.ifError(err);
+      test.done();
+    });
+  }
 });
diff --git a/testAsyncOptions/testAsyncSuffixSyncDefault.js b/testAsyncOptions/testAsyncSuffixSyncDefault.js
@@ -6,12 +6,35 @@ var java = require("../");
 var assert = require("assert");
 var _ = require('lodash');
 
-java.asyncOptions = {
-  syncSuffix: "",
-  asyncSuffix: "Async"
-};
-
 module.exports = {
+  launch: function(test) {
+    test.expect(4);
+    java.asyncOptions = {
+      syncSuffix: "",
+      asyncSuffix: "Async",
+      ifReadOnlySuffix: "_alt"
+    };
+
+    function before(callback) {
+      java.classpath.push('test/');
+      test.ok(!java.isJvmCreated());
+      callback();
+    }
+
+    function after(callback) {
+      test.ok(java.isJvmCreated());
+      callback();
+    }
+
+    java.registerClient(before, after);
+
+    java.ensureJvm(function(err) {
+      test.ifError(err);
+      test.ok(java.isJvmCreated());
+      test.done();
+    });
+  },
+
   testAPI: function(test) {
     test.expect(5);
     var arrayList = java.newInstanceSync("java.util.ArrayList");
@@ -82,5 +105,58 @@ module.exports = {
         });
       });
     });
-  }
+  },
+
+  // See testUnusableMethodName.js for the purpose of these last two tests.
+  // In that test, Test.name_alt() is an async method.
+  // In this test, it is a sync method.
+  testUnusableMethodNameThrows: function(test) {
+    test.expect(1);
+    var Test = java.import("Test");
+    test.ok(Test);
+    test.throws(
+      function() {
+        Test.name();
+      },
+      function(err) {
+        if (err instanceof TypeError) {
+          test.done();
+          return true;
+        } else {
+          test.done(err);
+          return false;
+        }
+      }
+    );
+  },
+
+  testAlternateMethodNameWorks: function(test) {
+    test.expect(4);
+    var Test = java.import("Test");
+    test.ok(Test);
+    test.strictEqual(Test.name_alt(), "name");
+    test.strictEqual(Test.caller_alt(), "caller");
+    test.strictEqual(Test.arguments_alt(), "arguments");
+    test.done();
+  },
+
+  testReservedFieldName: function(test) {
+    test.expect(7);
+    var TestEnum = java.import("Test$Enum");
+    test.ok(TestEnum);
+
+    // 'foo' and 'bar' are valid enum names
+    test.strictEqual(TestEnum.foo.toString(), "foo");
+    test.strictEqual(TestEnum.bar.toString(), "bar");
+
+    // TestEnum.name is actually the name of the proxy constructor function.
+    test.strictEqual(TestEnum.name, "javaClassConstructorProxy");
+
+    // Instead we need to acccess TestEnum.name_alt
+    test.strictEqual(TestEnum.name_alt.toString(), "name");
+    test.strictEqual(TestEnum.caller_alt.toString(), "caller");
+    test.strictEqual(TestEnum.arguments_alt.toString(), "arguments");
+
+    test.done();
+  },
 }
diff --git a/testAsyncOptions/testUnusableMethodName.js b/testAsyncOptions/testUnusableMethodName.js
