Improve ProtoBuf.js version heuristic, add tests for oneof decoding

src/node/index.js

@@ -57,18 +57,53 @@ var protobuf_js_6_common = require('./src/protobuf_js_6_common');
 
 grpc.setDefaultRootsPem(fs.readFileSync(SSL_ROOTS_PATH, 'ascii'));
 
+/**
+ * Load a ProtoBuf.js object as a gRPC object. The options object can provide
+ * the following options:
+ * - binaryAsBase64: deserialize bytes values as base64 strings instead of
+ *   Buffers. Defaults to false
+ * - longsAsStrings: deserialize long values as strings instead of objects.
+ *   Defaults to true
+ * - deprecatedArgumentOrder: Use the beta method argument order for client
+ *   methods, with optional arguments after the callback. Defaults to false.
+ *   This option is only a temporary stopgap measure to smooth an API breakage.
+ *   It is deprecated, and new code should not use it.
+ * - protobufjsVersion: Available values are 5, 6, and 'detect'. 5 and 6
+ *   respectively indicate that an object from the corresponding version of
+ *   ProtoBuf.js is provided in the value argument. If the option is 'detect',
+ *   gRPC will guess what the version is based on the structure of the value.
+ *   Defaults to 'detect'.
+ * @param {Object} value The ProtoBuf.js reflection object to load
+ * @param {Object=} options Options to apply to the loaded file
+ * @return {Object<string, *>} The resulting gRPC object
+ */
 exports.loadObject = function loadObject(value, options) {
   options = _.defaults(options, common.defaultGrpcOptions);
-  if (value instanceof ProtoBuf.ReflectionObject) {
-    return protobuf_js_6_common.loadObject(value, options);
+  options = _.defaults(options, {'protobufjsVersion': 'detect'});
+  var protobufjsVersion;
+  if (options.protobufjsVersion === 'detect') {
+    if (protobuf_js_6_common.isProbablyProtobufJs6(value)) {
+      protobufjsVersion = 6;
+    } else if (protobuf_js_5_common.isProbablyProtobufJs5(value)) {
+      protobufjsVersion = 5;
+    } else {
+      var error_message = 'Could not detect ProtoBuf.js version. Please ' +
+          'specify the version number with the "protobufjs_version" option';
+      throw new Error(error_message);
+    }
   } else {
-    /* If value is not a ProtoBuf.js 6 reflection object, we assume that it is
-       a ProtoBuf.js 5 reflection object, for backwards compatibility */
+    protobufjsVersion = options.protobufjsVersion;
+  }
+  switch (protobufjsVersion) {
+    case 6: return protobuf_js_6_common.loadObject(value, options);
+    case 5:
     var deprecation_message = 'Calling grpc.loadObject with an object ' +
         'generated by ProtoBuf.js 5 is deprecated. Please upgrade to ' +
         'ProtoBuf.js 6.';
     common.log(grpc.logVerbosity.INFO, deprecation_message);
     return protobuf_js_5_common.loadObject(value, options);
+    default:
+    throw new Error('Unrecognized protobufjsVersion', protobufjsVersion);
   }
 };
 
@@ -90,9 +125,8 @@ function applyProtoRoot(filename, root) {
 /**
  * Load a gRPC object from a .proto file. The options object can provide the
  * following options:
- * - convertFieldsToCamelCase: Loads this file with that option on protobuf.js
- *   set as specified. See
- *   https://github.com/dcodeIO/protobuf.js/wiki/Advanced-options for details
+ * - convertFieldsToCamelCase: Load this file with field names in camel case
+ *   instead of their original case
  * - binaryAsBase64: deserialize bytes values as base64 strings instead of
  *   Buffers. Defaults to false
  * - longsAsStrings: deserialize long values as strings instead of objects.
@@ -113,6 +147,7 @@ exports.load = function load(filename, format, options) {
      still the possibility of adding other formats that would be loaded
      differently */
   options = _.defaults(options, common.defaultGrpcOptions);
+  options.protobufjs_version = 6;
   var root = new ProtoBuf.Root();
   var parse_options = {keepCase: !options.convertFieldsToCamelCase};
   return loadObject(root.loadSync(applyProtoRoot(filename, root),

src/node/src/protobuf_js_5_common.js

@@ -165,3 +165,16 @@ exports.loadObject = function loadObject(value, options) {
     return value;
   }
 };
+
+/**
+ * The primary purpose of this method is to distinguish between reflection
+ * objects from different versions of ProtoBuf.js. This is just a heuristic,
+ * checking for properties that are (currently) specific to this version of
+ * ProtoBuf.js
+ * @param {Object} obj The object to check
+ * @return {boolean} Whether the object appears to be a Protobuf.js 5
+ *   ReflectionObject
+ */
+exports.isProbablyProtobufJs5 = function isProbablyProtobufJs5(obj) {
+  return _.isArray(obj.children) && (typeof obj.build === 'function');
+};

src/node/src/protobuf_js_6_common.js

@@ -157,3 +157,16 @@ exports.loadObject = function loadObject(value, options) {
   // Otherwise, it's not something we need to change
   return value;
 };
+
+/**
+ * The primary purpose of this method is to distinguish between reflection
+ * objects from different versions of ProtoBuf.js. This is just a heuristic,
+ * checking for properties that are (currently) specific to this version of
+ * ProtoBuf.js
+ * @param {Object} obj The object to check
+ * @return {boolean} Whether the object appears to be a Protobuf.js 6
+ *   ReflectionObject
+ */
+exports.isProbablyProtobufJs6 = function isProbablyProtobufJs6(obj) {
+  return (typeof obj.root === 'object') && (typeof obj.resolve === 'function');
+};

src/node/src/server.js

@@ -781,18 +781,16 @@ Server.prototype.addService = function(service, implementation) {
  */
 Server.prototype.addProtoService = function(service, implementation) {
   var options;
+  var protobuf_js_5_common = require('./protobuf_js_5_common');
+  var protobuf_js_6_common = require('./protobuf_js_6_common');
   common.log(grpc.logVerbosity.INFO,
              'Server#addProtoService is deprecated. Use addService instead');
-  if (service.hasOwnProperty('children')) {
-    // Heuristically: this is a Protobuf.js 5 service object
-    var protobuf_js_5_common = require('./protobuf_js_5_common');
+  if (protobuf_js_5_common.isProbablyProtobufJs5(service)) {
     options = _.defaults(service.grpc_options, common.defaultGrpcOptions);
     this.addService(
         protobuf_js_5_common.getProtobufServiceAttrs(service, options),
         implementation);
-  } else if (service.hasOwnProperty('methods')) {
-    // Heuristically: this is a Protobuf.js 6 service object
-    var protobuf_js_6_common = require('./protobuf_js_6_common');
+  } else if (protobuf_js_6_common.isProbablyProtobufJs6(service)) {
     options = _.defaults(service.grpc_options, common.defaultGrpcOptions);
     this.addService(
         protobuf_js_6_common.getProtobufServiceAttrs(service, options),

src/node/test/common_test.js

@@ -155,3 +155,27 @@ describe('Proto message bytes serialize and deserialize', function() {
     assert.deepEqual(unpackedDeserialized, expectedDeserialize);
   });
 });
+describe('Proto message oneof serialize and deserialize', function() {
+  var oneofSerialize = serializeCls(messages_proto.OneOfValues);
+  var oneofDeserialize = deserializeCls(
+      messages_proto.OneOfValues, default_options);
+  it('Should have idempotent round trips', function() {
+    var test_message = {oneof_choice: 'int_choice', int_choice: 5};
+    var serialized1 = oneofSerialize(test_message);
+    var deserialized1 = oneofDeserialize(serialized1);
+    assert.equal(deserialized1.int_choice, 5);
+    var serialized2 = oneofSerialize(deserialized1);
+    var deserialized2 = oneofDeserialize(serialized2);
+    assert.deepEqual(deserialized1, deserialized2);
+  });
+  it('Should emit a property indicating which field was chosen', function() {
+    var test_message1 = {oneof_choice: 'int_choice', int_choice: 5};
+    var serialized1 = oneofSerialize(test_message1);
+    var deserialized1 = oneofDeserialize(serialized1);
+    assert.equal(deserialized1.oneof_choice, 'int_choice');
+    var test_message2 = {oneof_choice: 'string_choice', string_choice: 'abc'};
+    var serialized2 = oneofSerialize(test_message2);
+    var deserialized2 = oneofDeserialize(serialized2);
+    assert.equal(deserialized2.oneof_choice, 'int_choice');
+  });
+});

src/node/test/test_messages.proto

@@ -41,3 +41,10 @@ message SequenceValues {
   bytes bytes_field = 1;
   repeated int32 repeated_field = 2;
 }
+
+message OneOfValues {
+  oneof oneof_choice {
+    int32 int_choice = 1;
+    string string_choice = 2;
+  }
+}