THRIFT-3409 NodeJS binary field issues

Client: Node.js
Patch: Nobuaki Sukegawa

This closes #681

4 files changed, 94 insertions, 37 deletions

diff --git a/lib/nodejs/lib/thrift/binary_protocol.js b/lib/nodejs/lib/thrift/binary_protocol.js
index a230291ec..25e5634ee 100644
--- a/lib/nodejs/lib/thrift/binary_protocol.js
+++ b/lib/nodejs/lib/thrift/binary_protocol.js
@@ -146,22 +146,10 @@ TBinaryProtocol.prototype.writeDouble = function(dub) {
   this.trans.write(binary.writeDouble(new Buffer(8), dub));
 };
 
-TBinaryProtocol.prototype.writeString = function(arg) {
+TBinaryProtocol.prototype.writeStringOrBinary = function(name, encoding, arg) {
   if (typeof(arg) === 'string') {
-    this.writeI32(Buffer.byteLength(arg, 'utf8'));
-    this.trans.write(arg, 'utf8');
-  } else if (arg instanceof Buffer) {
-    this.writeI32(arg.length);
-    this.trans.write(arg);
-  } else {
-    throw new Error('writeString called without a string/Buffer argument: ' + arg);
-  }
-};
-
-TBinaryProtocol.prototype.writeBinary = function(arg) {
-  if (typeof(arg) === 'string') {
-    this.writeI32(Buffer.byteLength(arg, 'utf8'));
-    this.trans.write(arg, 'utf8');
+    this.writeI32(Buffer.byteLength(arg, encoding));
+    this.trans.write(new Buffer(arg, encoding));
   } else if ((arg instanceof Buffer) ||
              (Object.prototype.toString.call(arg) == '[object Uint8Array]')) {
     // Buffers in Node.js under Browserify may extend UInt8Array instead of
@@ -170,10 +158,18 @@ TBinaryProtocol.prototype.writeBinary = function(arg) {
     this.writeI32(arg.length);
     this.trans.write(arg);
   } else {
-    throw new Error('writeBinary called without a string/Buffer argument: ' + arg);
+    throw new Error(name + ' called without a string/Buffer argument: ' + arg);
   }
 };
 
+TBinaryProtocol.prototype.writeString = function(arg) {
+  this.writeStringOrBinary('writeString', 'utf8', arg);
+};
+
+TBinaryProtocol.prototype.writeBinary = function(arg) {
+  this.writeStringOrBinary('writeBinary', 'binary', arg);
+};
+
 TBinaryProtocol.prototype.readMessageBegin = function() {
   var sz = this.readI32();
   var type, name, seqid;
@@ -279,11 +275,25 @@ TBinaryProtocol.prototype.readDouble = function() {
 
 TBinaryProtocol.prototype.readBinary = function() {
   var len = this.readI32();
+  if (len === 0) {
+    return new Buffer();
+  }
+
+  if (len < 0) {
+    throw new Thrift.TProtocolException(Thrift.TProtocolExceptionType.NEGATIVE_SIZE, "Negative binary size");
+  }
   return this.trans.read(len);
 };
 
 TBinaryProtocol.prototype.readString = function() {
   var len = this.readI32();
+  if (len === 0) {
+    return "";
+  }
+
+  if (len < 0) {
+    throw new Thrift.TProtocolException(Thrift.TProtocolExceptionType.NEGATIVE_SIZE, "Negative string size");
+  }
   return this.trans.readString(len);
 };
 
diff --git a/lib/nodejs/lib/thrift/compact_protocol.js b/lib/nodejs/lib/thrift/compact_protocol.js
index 8aee808f5..deecf4844 100644
--- a/lib/nodejs/lib/thrift/compact_protocol.js
+++ b/lib/nodejs/lib/thrift/compact_protocol.js
@@ -429,22 +429,30 @@ TCompactProtocol.prototype.writeDouble = function(v) {
   this.trans.write(buff);
 };
 
-TCompactProtocol.prototype.writeString = function(arg) {
-  this.writeBinary(arg);
-};
-
-TCompactProtocol.prototype.writeBinary = function(arg) {
+TCompactProtocol.prototype.writeStringOrBinary = function(name, encoding, arg) {
   if (typeof arg === 'string') {
-    this.writeVarint32(Buffer.byteLength(arg, 'utf8')) ;
-    this.trans.write(arg, 'utf8');
-  } else if (arg instanceof Buffer) {
+    this.writeVarint32(Buffer.byteLength(arg, encoding)) ;
+    this.trans.write(new Buffer(arg, encoding));
+  } else if (arg instanceof Buffer ||
+             Object.prototype.toString.call(arg) == '[object Uint8Array]') {
+    // Buffers in Node.js under Browserify may extend UInt8Array instead of
+    // defining a new object. We detect them here so we can write them
+    // correctly
     this.writeVarint32(arg.length);
     this.trans.write(arg);
   } else {
-    throw new Error('writeString/writeBinary called without a string/Buffer argument: ' + arg);
+    throw new Error(name + ' called without a string/Buffer argument: ' + arg);
   }
 };
 
+TCompactProtocol.prototype.writeString = function(arg) {
+  this.writeStringOrBinary('writeString', 'utf8', arg);
+};
+
+TCompactProtocol.prototype.writeBinary = function(arg) {
+  this.writeStringOrBinary('writeBinary', 'binary', arg);
+};
+
 
 //
 // Compact Protocol internal write methods
@@ -752,22 +760,28 @@ TCompactProtocol.prototype.readDouble = function() {
 
 TCompactProtocol.prototype.readBinary = function() {
   var size = this.readVarint32();
-  // Catch empty string case
   if (size === 0) {
-    return "";
+    return new Buffer();
   }
 
-  // Catch error cases
   if (size < 0) {
-    throw new Thrift.TProtocolException(Thrift.TProtocolExceptionType.NEGATIVE_SIZE, "Negative binary/string size");
+    throw new Thrift.TProtocolException(Thrift.TProtocolExceptionType.NEGATIVE_SIZE, "Negative binary size");
   }
-  var value = this.trans.readString(size);
-
-  return value;
+  return this.trans.read(size);
 };
 
 TCompactProtocol.prototype.readString = function() {
-  return this.readBinary();
+  var size = this.readVarint32();
+  // Catch empty string case
+  if (size === 0) {
+    return "";
+  }
+
+  // Catch error cases
+  if (size < 0) {
+    throw new Thrift.TProtocolException(Thrift.TProtocolExceptionType.NEGATIVE_SIZE, "Negative string size");
+  }
+  return this.trans.readString(size);
 };
 
 
diff --git a/lib/nodejs/lib/thrift/json_protocol.js b/lib/nodejs/lib/thrift/json_protocol.js
index 18fb0129e..436709c98 100644
--- a/lib/nodejs/lib/thrift/json_protocol.js
+++ b/lib/nodejs/lib/thrift/json_protocol.js
@@ -317,11 +317,19 @@ TJSONProtocol.prototype.writeDouble = function(dub) {
 };
 
 /** Serializes a string */
-TJSONProtocol.prototype.writeString = function(str) {
+TJSONProtocol.prototype.writeString = function(arg) {
   // We do not encode uri components for wire transfer:
-  if (str === null) {
+  if (arg === null) {
       this.tstack.push(null);
   } else {
+      if (typeof arg === 'string') {
+        var str = arg;
+      } else if (arg instanceof Buffer) {
+        var str = arg.toString('utf8');
+      } else {
+        throw new Error('writeString called without a string/Buffer argument: ' + arg);
+      }
+
       // concat may be slower than building a byte buffer
       var escapedString = '';
       for (var i = 0; i < str.length; i++) {
@@ -358,7 +366,15 @@ TJSONProtocol.prototype.writeString = function(str) {
 
 /** Serializes a string */
 TJSONProtocol.prototype.writeBinary = function(arg) {
-  this.writeString(arg);
+  if (typeof arg === 'string') {
+    var buf = new Buffer(arg, 'binary');
+  } else if (arg instanceof Buffer ||
+             Object.prototype.toString.call(arg) == '[object Uint8Array]')  {
+    var buf = arg;
+  } else {
+    throw new Error('writeBinary called without a string/Buffer argument: ' + arg);
+  }
+  this.tstack.push('"' + buf.toString('base64') + '"');
 };
 
 /**
@@ -680,7 +696,7 @@ TJSONProtocol.prototype.readDouble = function() {
 /** Returns the an object with a value property set to the
     next value found in the protocol buffer */
 TJSONProtocol.prototype.readBinary = function() {
-  return this.readString();
+  return new Buffer(this.readString(), 'base64');
 };
 
 /** Returns the an object with a value property set to the
diff --git a/lib/nodejs/test/test_driver.js b/lib/nodejs/test/test_driver.js
index 09439e55c..590d58337 100644
--- a/lib/nodejs/test/test_driver.js
+++ b/lib/nodejs/test/test_driver.js
@@ -57,6 +57,23 @@ exports.ThriftTestDriver = function(client, callback) {
     testCases.deep.forEach(makeAsserter(assert.deepEqual));
     testCases.deepUnordered.forEach(makeAsserter(makeUnorderedDeepEqual(assert)));
 
+    var arr = [];
+    for (var i = 0; i < 256; ++i) {
+      arr[i] = 255 - i;
+    }
+    var buf = new Buffer(arr);
+    client.testBinary(buf, function(err, response) {
+      assert.error(err, 'testBinary: no callback error');
+      assert.equal(response.length, 256, 'testBinary');
+      assert.deepEqual(response, buf, 'testBinary(Buffer)');
+    });
+    var buf = new Buffer(arr);
+    client.testBinary(buf.toString('binary'), function(err, response) {
+      assert.error(err, 'testBinary: no callback error');
+      assert.equal(response.length, 256, 'testBinary');
+      assert.deepEqual(response, buf, 'testBinary(string)');
+    });
+
     client.testMapMap(42, function(err, response) {
       var expected = {
         "4": {"1":1, "2":2, "3":3, "4":4},