From c02910b07ae492098d7d0c1bca747fbcad742393 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 17 Feb 2016 12:59:26 -0800 Subject: [PATCH 1/3] Node: add options to modify ProtoBuf behavior --- src/node/index.js | 40 +++++++++++++++++++++++++--------------- src/node/src/client.js | 6 ++++-- src/node/src/common.js | 29 ++++++++++++++++++++++++----- src/node/src/server.js | 7 ++++++- 4 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/node/index.js b/src/node/index.js index 7eacdc67b1d..4e4d12d9e45 100644 --- a/src/node/index.js +++ b/src/node/index.js @@ -56,17 +56,18 @@ var grpc = require('./src/grpc_extension'); /** * Load a gRPC object from an existing ProtoBuf.Reflect object. * @param {ProtoBuf.Reflect.Namespace} value The ProtoBuf object to load. + * @param {Object=} options Options to apply to the loaded object * @return {Object} The resulting gRPC object */ -exports.loadObject = function loadObject(value) { +exports.loadObject = function loadObject(value, options) { var result = {}; if (value.className === 'Namespace') { _.each(value.children, function(child) { - result[child.name] = loadObject(child); + result[child.name] = loadObject(child, options); }); return result; } else if (value.className === 'Service') { - return client.makeProtobufClientConstructor(value); + return client.makeProtobufClientConstructor(value, options); } else if (value.className === 'Message' || value.className === 'Enum') { return value.build(); } else { @@ -78,27 +79,36 @@ var loadObject = exports.loadObject; /** * Load a gRPC object from a .proto file. - * @param {string} filename The file to load + * @param {string|{root: string, file: string}} filename The file to load * @param {string=} format The file format to expect. Must be either 'proto' or * 'json'. Defaults to 'proto' + * @param {Object=} options Options to apply to the loaded file * @return {Object} The resulting gRPC object */ -exports.load = function load(filename, format) { +exports.load = function load(filename, format, options) { if (!format) { format = 'proto'; } + var convertFieldsToCamelCaseOriginal = ProtoBuf.convertFieldsToCamelCase; + if(options && options.hasOwnProperty('convertFieldsToCamelCase')) { + ProtoBuf.convertFieldsToCamelCase = options.convertFieldsToCamelCase; + } var builder; - switch(format) { - case 'proto': - builder = ProtoBuf.loadProtoFile(filename); - break; - case 'json': - builder = ProtoBuf.loadJsonFile(filename); - break; - default: - throw new Error('Unrecognized format "' + format + '"'); + try { + switch(format) { + case 'proto': + builder = ProtoBuf.loadProtoFile(filename); + break; + case 'json': + builder = ProtoBuf.loadJsonFile(filename); + break; + default: + throw new Error('Unrecognized format "' + format + '"'); + } + } finally { + ProtoBuf.convertFieldsToCamelCase = convertFieldsToCamelCaseOriginal; } - return loadObject(builder.ns); + return loadObject(builder.ns, options); }; /** diff --git a/src/node/src/client.js b/src/node/src/client.js index b5247a69ee0..5523d6b9a47 100644 --- a/src/node/src/client.js +++ b/src/node/src/client.js @@ -698,13 +698,15 @@ exports.waitForClientReady = function(client, deadline, callback) { * Creates a constructor for clients for the given service * @param {ProtoBuf.Reflect.Service} service The service to generate a client * for + * @param {Object=} options Options to apply to the client * @return {function(string, Object)} New client constructor */ -exports.makeProtobufClientConstructor = function(service) { - var method_attrs = common.getProtobufServiceAttrs(service, service.name); +exports.makeProtobufClientConstructor = function(service, options) { + var method_attrs = common.getProtobufServiceAttrs(service, service.name, options); var Client = exports.makeClientConstructor( method_attrs, common.fullyQualifiedName(service)); Client.service = service; + Client.service.grpc_options = options; return Client; }; diff --git a/src/node/src/common.js b/src/node/src/common.js index 2e6c01c4d74..3e6609ab107 100644 --- a/src/node/src/common.js +++ b/src/node/src/common.js @@ -44,9 +44,20 @@ var _ = require('lodash'); /** * Get a function that deserializes a specific type of protobuf. * @param {function()} cls The constructor of the message type to deserialize + * @param {bool=} binaryAsBase64 Deserialize bytes fields as base64 instead of binary. + * Defaults to false + * @param {bool=} longsAsStrings Deserialize long values as strings instead of doubles. + * Defaults to true * @return {function(Buffer):cls} The deserialization function */ -exports.deserializeCls = function deserializeCls(cls) { +exports.deserializeCls = function deserializeCls(cls, binaryAsBase64, + longsAsStrings) { + if (binaryAsBase64 === undefined || binaryAsBase64 === null) { + binaryAsBase64 = false; + } + if (longsAsStrings === undefined || longsAsStrings === null) { + longsAsStrings = true; + } /** * Deserialize a buffer to a message object * @param {Buffer} arg_buf The buffer to deserialize @@ -55,7 +66,7 @@ exports.deserializeCls = function deserializeCls(cls) { return function deserialize(arg_buf) { // Convert to a native object with binary fields as Buffers (first argument) // and longs as strings (second argument) - return cls.decode(arg_buf).toRaw(false, true); + return cls.decode(arg_buf).toRaw(binaryAsBase64, longsAsStrings); }; }; @@ -119,19 +130,27 @@ exports.wrapIgnoreNull = function wrapIgnoreNull(func) { /** * Return a map from method names to method attributes for the service. * @param {ProtoBuf.Reflect.Service} service The service to get attributes for + * @param {Object=} options Options to apply to these attributes * @return {Object} The attributes map */ -exports.getProtobufServiceAttrs = function getProtobufServiceAttrs(service) { +exports.getProtobufServiceAttrs = function getProtobufServiceAttrs(service, options) { var prefix = '/' + fullyQualifiedName(service) + '/'; + var binaryAsBase64, longsAsStrings; + if (options) { + binaryAsBase64 = options.binaryAsBase64; + longsAsStrings = options.longsAsStrings; + } return _.object(_.map(service.children, function(method) { return [_.camelCase(method.name), { path: prefix + method.name, requestStream: method.requestStream, responseStream: method.responseStream, requestSerialize: serializeCls(method.resolvedRequestType.build()), - requestDeserialize: deserializeCls(method.resolvedRequestType.build()), + requestDeserialize: deserializeCls(method.resolvedRequestType.build(), + binaryAsBase64, longsAsStrings), responseSerialize: serializeCls(method.resolvedResponseType.build()), - responseDeserialize: deserializeCls(method.resolvedResponseType.build()) + responseDeserialize: deserializeCls(method.resolvedResponseType.build(), + binaryAsBase64, longsAsStrings) }]; })); }; diff --git a/src/node/src/server.js b/src/node/src/server.js index e5aadcd5658..0cf7ba34246 100644 --- a/src/node/src/server.js +++ b/src/node/src/server.js @@ -737,7 +737,12 @@ Server.prototype.addService = function(service, implementation) { * method implementation for the provided service. */ Server.prototype.addProtoService = function(service, implementation) { - this.addService(common.getProtobufServiceAttrs(service), implementation); + var options; + if (service.grpc_options) { + options = service.grpc_options; + } + this.addService(common.getProtobufServiceAttrs(service, options), + implementation); }; /** From 654d2549b752dfd74500f0b5882abaf306cd62c1 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 17 Feb 2016 15:36:28 -0800 Subject: [PATCH 2/3] Add tests and documentation for new options --- src/node/index.js | 10 ++++++- src/node/src/client.js | 3 +- src/node/src/common.js | 11 +++---- src/node/test/common_test.js | 50 ++++++++++++++++++++++++++++++- src/node/test/test_messages.proto | 5 ++++ 5 files changed, 71 insertions(+), 8 deletions(-) diff --git a/src/node/index.js b/src/node/index.js index 4e4d12d9e45..1c197729d77 100644 --- a/src/node/index.js +++ b/src/node/index.js @@ -78,7 +78,15 @@ exports.loadObject = function loadObject(value, options) { var loadObject = exports.loadObject; /** - * Load a gRPC object from a .proto file. + * 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 + * - 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 * @param {string|{root: string, file: string}} filename The file to load * @param {string=} format The file format to expect. Must be either 'proto' or * 'json'. Defaults to 'proto' diff --git a/src/node/src/client.js b/src/node/src/client.js index 5523d6b9a47..c02c44730e5 100644 --- a/src/node/src/client.js +++ b/src/node/src/client.js @@ -702,7 +702,8 @@ exports.waitForClientReady = function(client, deadline, callback) { * @return {function(string, Object)} New client constructor */ exports.makeProtobufClientConstructor = function(service, options) { - var method_attrs = common.getProtobufServiceAttrs(service, service.name, options); + var method_attrs = common.getProtobufServiceAttrs(service, service.name, + options); var Client = exports.makeClientConstructor( method_attrs, common.fullyQualifiedName(service)); Client.service = service; diff --git a/src/node/src/common.js b/src/node/src/common.js index 3e6609ab107..e5217608ecd 100644 --- a/src/node/src/common.js +++ b/src/node/src/common.js @@ -44,10 +44,10 @@ var _ = require('lodash'); /** * Get a function that deserializes a specific type of protobuf. * @param {function()} cls The constructor of the message type to deserialize - * @param {bool=} binaryAsBase64 Deserialize bytes fields as base64 instead of binary. - * Defaults to false - * @param {bool=} longsAsStrings Deserialize long values as strings instead of doubles. - * Defaults to true + * @param {bool=} binaryAsBase64 Deserialize bytes fields as base64 strings + * instead of Buffers. Defaults to false + * @param {bool=} longsAsStrings Deserialize long values as strings instead of + * objects. Defaults to true * @return {function(Buffer):cls} The deserialization function */ exports.deserializeCls = function deserializeCls(cls, binaryAsBase64, @@ -133,7 +133,8 @@ exports.wrapIgnoreNull = function wrapIgnoreNull(func) { * @param {Object=} options Options to apply to these attributes * @return {Object} The attributes map */ -exports.getProtobufServiceAttrs = function getProtobufServiceAttrs(service, options) { +exports.getProtobufServiceAttrs = function getProtobufServiceAttrs(service, + options) { var prefix = '/' + fullyQualifiedName(service) + '/'; var binaryAsBase64, longsAsStrings; if (options) { diff --git a/src/node/test/common_test.js b/src/node/test/common_test.js index 08ba429ed7f..c57b7388f67 100644 --- a/src/node/test/common_test.js +++ b/src/node/test/common_test.js @@ -42,7 +42,7 @@ var ProtoBuf = require('protobufjs'); var messages_proto = ProtoBuf.loadProtoFile( __dirname + '/test_messages.proto').build(); -describe('Proto message serialize and deserialize', function() { +describe('Proto message long int serialize and deserialize', function() { var longSerialize = common.serializeCls(messages_proto.LongValues); var longDeserialize = common.deserializeCls(messages_proto.LongValues); var pos_value = '314159265358979'; @@ -87,4 +87,52 @@ describe('Proto message serialize and deserialize', function() { assert.strictEqual(longDeserialize(serialized).sfixed_64.toString(), neg_value); }); + it('should deserialize as a number with the right option set', function() { + var longNumDeserialize = common.deserializeCls(messages_proto.LongValues, + false, false); + var serialized = longSerialize({int_64: pos_value}); + assert.strictEqual(typeof longDeserialize(serialized).int_64, 'string'); + /* With the longsAsStrings option disabled, long values are represented as + * objects with 3 keys: low, high, and unsigned */ + assert.strictEqual(typeof longNumDeserialize(serialized).int_64, 'object'); + }); +}); +describe('Proto message bytes serialize and deserialize', function() { + var sequenceSerialize = common.serializeCls(messages_proto.SequenceValues); + var sequenceDeserialize = common.deserializeCls( + messages_proto.SequenceValues); + var sequenceBase64Deserialize = common.deserializeCls( + messages_proto.SequenceValues, true); + var buffer_val = new Buffer([0x69, 0xb7]); + var base64_val = 'abc='; + it('should preserve a buffer', function() { + var serialized = sequenceSerialize({bytes_field: buffer_val}); + var deserialized = sequenceDeserialize(serialized); + assert.strictEqual(deserialized.bytes_field.compare(buffer_val), 0); + }); + it('should accept base64 encoded strings', function() { + var serialized = sequenceSerialize({bytes_field: base64_val}); + var deserialized = sequenceDeserialize(serialized); + assert.strictEqual(deserialized.bytes_field.compare(buffer_val), 0); + }); + it('should output base64 encoded strings with an option set', function() { + var serialized = sequenceSerialize({bytes_field: base64_val}); + var deserialized = sequenceBase64Deserialize(serialized); + assert.strictEqual(deserialized.bytes_field, base64_val); + }); + /* The next two tests are specific tests to verify that issue + * https://github.com/grpc/grpc/issues/5174 has been fixed. They are skipped + * because they will not pass until a protobuf.js release has been published + * with a fix for https://github.com/dcodeIO/protobuf.js/issues/390 */ + it.skip('should serialize a repeated field as packed by default', function() { + var expected_serialize = new Buffer([0x12, 0x01, 0x01, 0x0a]); + var serialized = sequenceSerialize({repeated_field: [10]}); + assert.strictEqual(expected_serialize.compare(serialized), 0); + }); + it.skip('should deserialize packed or unpacked repeated', function() { + var serialized = new Buffer([0x12, 0x01, 0x01, 0x0a]); + assert.doesNotThrow(function() { + sequenceDeserialize(serialized); + }); + }); }); diff --git a/src/node/test/test_messages.proto b/src/node/test/test_messages.proto index c77a937d3f4..d1ffcf996df 100644 --- a/src/node/test/test_messages.proto +++ b/src/node/test/test_messages.proto @@ -36,3 +36,8 @@ message LongValues { fixed64 fixed_64 = 4; sfixed64 sfixed_64 = 5; } + +message SequenceValues { + bytes bytes_field = 1; + repeated int32 repeated_field = 2; +} \ No newline at end of file From 5acbb9c1e9e93b3fc085cb52e5e6207470bd938a Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 17 Feb 2016 15:42:01 -0800 Subject: [PATCH 3/3] Sanitize files --- src/node/test/common_test.js | 2 +- src/node/test/test_messages.proto | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node/test/common_test.js b/src/node/test/common_test.js index c57b7388f67..66a4205f82c 100644 --- a/src/node/test/common_test.js +++ b/src/node/test/common_test.js @@ -1,6 +1,6 @@ /* * - * Copyright 2015, Google Inc. + * Copyright 2015-2016, Google Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without diff --git a/src/node/test/test_messages.proto b/src/node/test/test_messages.proto index d1ffcf996df..9b8cb875eeb 100644 --- a/src/node/test/test_messages.proto +++ b/src/node/test/test_messages.proto @@ -40,4 +40,4 @@ message LongValues { message SequenceValues { bytes bytes_field = 1; repeated int32 repeated_field = 2; -} \ No newline at end of file +}