THRIFT-1957 NodeJS TFramedTransport and TBufferedTransport read bytes as unsigned

Patch: Matthew Imrie

Add method to binary.js to properly read a byte type as
signed Changed transport.js readByte methods in
TFramedBuffer and TBufferedTransport to use binary.js
readByte method Added unit test for binary.js readByte
method to binary.test.js and changed test harness to
nodeunit.

diff --git a/lib/nodejs/lib/thrift/binary.js b/lib/nodejs/lib/thrift/binary.js
index 3be4d01..62c3f72 100644 (file)
--- a/lib/nodejs/lib/thrift/binary.js
+++ b/lib/nodejs/lib/thrift/binary.js
@@ -26,6 +26,10 @@ var POW_48 = Math.pow(2, 48)
 var POW_52 = Math.pow(2, 52)
 var POW_1022 = Math.pow(2, 1022)
 
+exports.readByte = function(byte){
+       return byte > 127 ? byte-256 : byte;
+}
+
 exports.readI16 = function(buff, off) {
   off = off || 0;
   var v = buff[off + 1];

diff --git a/lib/nodejs/lib/thrift/transport.js b/lib/nodejs/lib/thrift/transport.js
index 7d83a99..e918335 100644 (file)
--- a/lib/nodejs/lib/thrift/transport.js
+++ b/lib/nodejs/lib/thrift/transport.js
@@ -99,7 +99,7 @@ TFramedTransport.prototype = {
   },
 
   readByte: function() {
-    return this.inBuf[this.readPos++];
+    return binary.readByte(this.inBuf[this.readPos++]);
   },
 
   readI16: function() {
@@ -223,7 +223,7 @@ TBufferedTransport.prototype = {
 
   readByte: function() {
     this.ensureAvailable(1)
-    return this.inBuf[this.readCursor++];
+    return binary.readByte(this.inBuf[this.readCursor++]);
   },
 
   readI16: function() {

diff --git a/lib/nodejs/test/binary.test.js b/lib/nodejs/test/binary.test.js
index c6bf7c9..c7f6f72 100644 (file)
--- a/lib/nodejs/test/binary.test.js
+++ b/lib/nodejs/test/binary.test.js
@@ -17,98 +17,117 @@
  * under the License.
  */
 
-var assert = require('assert');
+var testCase = require('nodeunit').testCase;
 var binary = require('thrift/binary');
 
-module.exports = {
-  "Should read I16": function() {
-    assert.equal(0, binary.readI16([0x00, 0x00]));
-    assert.equal(1, binary.readI16([0x00, 0x01]));
-    assert.equal(-1, binary.readI16([0xff, 0xff]));
+module.exports = testCase({
+  "Should read signed byte": function(test){
+    test.equal(1, binary.readByte([0x01]));
+    test.equal(-1, binary.readByte([0xFF]));
+    
+    test.equal(127, binary.readByte([0x7F]));
+    test.equal(-128, binary.readByte([0x80]));
+    test.done();
+  },
+  "Should write byte": function(test){
+       //Protocol simply writes to the buffer. Nothing to test.. yet.
+       test.ok(true);
+       test.done();
+  },
+  "Should read I16": function(test) {
+    test.equal(0, binary.readI16([0x00, 0x00]));
+    test.equal(1, binary.readI16([0x00, 0x01]));
+    test.equal(-1, binary.readI16([0xff, 0xff]));
 
     // Min I16
-    assert.equal(-32768, binary.readI16([0x80, 0x00]));
+    test.equal(-32768, binary.readI16([0x80, 0x00]));
     // Max I16
-    assert.equal(32767, binary.readI16([0x7f, 0xff]));
+    test.equal(32767, binary.readI16([0x7f, 0xff]));
+    test.done();
   },
 
-  "Should write I16": function() {
-    assert.deepEqual([0x00, 0x00], binary.writeI16([], 0));
-    assert.deepEqual([0x00, 0x01], binary.writeI16([], 1));
-    assert.deepEqual([0xff, 0xff], binary.writeI16([], -1));
+  "Should write I16": function(test) {
+    test.deepEqual([0x00, 0x00], binary.writeI16([], 0));
+    test.deepEqual([0x00, 0x01], binary.writeI16([], 1));
+    test.deepEqual([0xff, 0xff], binary.writeI16([], -1));
 
     // Min I16
-    assert.deepEqual([0x80, 0x00], binary.writeI16([], -32768));
+    test.deepEqual([0x80, 0x00], binary.writeI16([], -32768));
     // Max I16
-    assert.deepEqual([0x7f, 0xff], binary.writeI16([], 32767));
+    test.deepEqual([0x7f, 0xff], binary.writeI16([], 32767));
+    test.done();
   },
 
-  "Should read I32": function() {
-    assert.equal(0, binary.readI32([0x00, 0x00, 0x00, 0x00]));
-    assert.equal(1, binary.readI32([0x00, 0x00, 0x00, 0x01]));
-    assert.equal(-1, binary.readI32([0xff, 0xff, 0xff, 0xff]));
+  "Should read I32": function(test) {
+    test.equal(0, binary.readI32([0x00, 0x00, 0x00, 0x00]));
+    test.equal(1, binary.readI32([0x00, 0x00, 0x00, 0x01]));
+    test.equal(-1, binary.readI32([0xff, 0xff, 0xff, 0xff]));
 
     // Min I32
-    assert.equal(-2147483648, binary.readI32([0x80, 0x00, 0x00, 0x00]));
+    test.equal(-2147483648, binary.readI32([0x80, 0x00, 0x00, 0x00]));
     // Max I32
-    assert.equal(2147483647, binary.readI32([0x7f, 0xff, 0xff, 0xff]));
+    test.equal(2147483647, binary.readI32([0x7f, 0xff, 0xff, 0xff]));
+    test.done();
   },
 
-  "Should write I32": function() {
-    assert.deepEqual([0x00, 0x00, 0x00, 0x00], binary.writeI32([], 0));
-    assert.deepEqual([0x00, 0x00, 0x00, 0x01], binary.writeI32([], 1));
-    assert.deepEqual([0xff, 0xff, 0xff, 0xff], binary.writeI32([], -1));
+  "Should write I32": function(test) {
+    test.deepEqual([0x00, 0x00, 0x00, 0x00], binary.writeI32([], 0));
+    test.deepEqual([0x00, 0x00, 0x00, 0x01], binary.writeI32([], 1));
+    test.deepEqual([0xff, 0xff, 0xff, 0xff], binary.writeI32([], -1));
 
     // Min I32
-    assert.deepEqual([0x80, 0x00, 0x00, 0x00], binary.writeI32([], -2147483648));
+    test.deepEqual([0x80, 0x00, 0x00, 0x00], binary.writeI32([], -2147483648));
     // Max I32
-    assert.deepEqual([0x7f, 0xff, 0xff, 0xff], binary.writeI32([], 2147483647));
+    test.deepEqual([0x7f, 0xff, 0xff, 0xff], binary.writeI32([], 2147483647));
+       test.done();
   },
 
-  "Should read doubles": function() {
-    assert.equal(0, binary.readDouble([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(0, binary.readDouble([0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(1, binary.readDouble([0x3f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(2, binary.readDouble([0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(-2, binary.readDouble([0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+  "Should read doubles": function(test) {
+    test.equal(0, binary.readDouble([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(0, binary.readDouble([0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(1, binary.readDouble([0x3f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(2, binary.readDouble([0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(-2, binary.readDouble([0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
 
-    assert.equal(Math.PI, binary.readDouble([0x40, 0x9, 0x21, 0xfb, 0x54, 0x44, 0x2d, 0x18]))
+    test.equal(Math.PI, binary.readDouble([0x40, 0x9, 0x21, 0xfb, 0x54, 0x44, 0x2d, 0x18]))
 
-    assert.equal(Infinity, binary.readDouble([0x7f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
-    assert.equal(-Infinity, binary.readDouble([0xff, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(Infinity, binary.readDouble([0x7f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(-Infinity, binary.readDouble([0xff, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
 
-    assert.ok(isNaN(binary.readDouble([0x7f, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00])))
+    test.ok(isNaN(binary.readDouble([0x7f, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00])))
 
-    assert.equal(1/3, binary.readDouble([0x3f, 0xd5, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55]))
+    test.equal(1/3, binary.readDouble([0x3f, 0xd5, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55]))
 
     // Min subnormal positive double
-    assert.equal(4.9406564584124654e-324, binary.readDouble([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01]))
+    test.equal(4.9406564584124654e-324, binary.readDouble([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01]))
     // Min normal positive double
-    assert.equal(2.2250738585072014e-308, binary.readDouble([0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+    test.equal(2.2250738585072014e-308, binary.readDouble([0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
     // Max positive double
-    assert.equal(1.7976931348623157e308, binary.readDouble([0x7f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff]))
+    test.equal(1.7976931348623157e308, binary.readDouble([0x7f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff]))
+       test.done();
   },
 
-  "Should write doubles": function() {
-    assert.deepEqual([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 0));
-    assert.deepEqual([0x3f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 1));
-    assert.deepEqual([0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 2));
-    assert.deepEqual([0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], -2));
+  "Should write doubles": function(test) {
+    test.deepEqual([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 0));
+    test.deepEqual([0x3f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 1));
+    test.deepEqual([0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 2));
+    test.deepEqual([0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], -2));
 
-    assert.deepEqual([0x40, 0x9, 0x21, 0xfb, 0x54, 0x44, 0x2d, 0x18], binary.writeDouble([], Math.PI));
+    test.deepEqual([0x40, 0x9, 0x21, 0xfb, 0x54, 0x44, 0x2d, 0x18], binary.writeDouble([], Math.PI));
 
-    assert.deepEqual([0x7f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], Infinity));
-    assert.deepEqual([0xff, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], -Infinity));
+    test.deepEqual([0x7f, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], Infinity));
+    test.deepEqual([0xff, 0xf0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], -Infinity));
 
-    assert.deepEqual([0x7f, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], NaN));
+    test.deepEqual([0x7f, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], NaN));
 
-    assert.deepEqual([0x3f, 0xd5, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55], binary.writeDouble([], 1/3));
+    test.deepEqual([0x3f, 0xd5, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55], binary.writeDouble([], 1/3));
 
     // Min subnormal positive double
-    assert.deepEqual([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01], binary.writeDouble([], 4.9406564584124654e-324)); 
+    test.deepEqual([0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01], binary.writeDouble([], 4.9406564584124654e-324)); 
     // Min normal positive double
-    assert.deepEqual([0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 2.2250738585072014e-308)); 
+    test.deepEqual([0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], binary.writeDouble([], 2.2250738585072014e-308)); 
     // Max positive double
-    assert.deepEqual([0x7f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff], binary.writeDouble([], 1.7976931348623157e308)); 
+    test.deepEqual([0x7f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff], binary.writeDouble([], 1.7976931348623157e308)); 
+       test.done();
   }
-}
+});
\ No newline at end of file

diff --git a/test/nodejs/Makefile.am b/test/nodejs/Makefile.am
index 32397f1..8f74ba8 100755 (executable)
--- a/test/nodejs/Makefile.am
+++ b/test/nodejs/Makefile.am
@@ -22,6 +22,14 @@ stubs: ../ThriftTest.thrift
        $(THRIFT) --gen js:node ../ThriftTest.thrift
 
 check: stubs
+       @if which expresso &> /dev/null ; then \
+               echo "   Testing thrift/binary"; \
+               NODE_PATH=../../lib/nodejs/lib:../../lib/nodejs/lib/thrift:$(NODE_PATH) nodeunit ../../lib/nodejs/test/binary.test.js; \
+       fi
+       timeout 2 $(MAKE) server &
+       @sleep 1
+       $(MAKE) client
+       @sleep 1
 
 clean-local:
        $(RM) -r gen-nodejs

diff --git a/test/nodejs/client.js b/test/nodejs/client.js
index 88460b0..3a90cac 100644 (file)
--- a/test/nodejs/client.js
+++ b/test/nodejs/client.js
@@ -17,6 +17,7 @@
  * under the License.
  */
 var thrift = require('thrift');
+var assert = require('assert');
 
 var ThriftTest = require('./gen-nodejs/ThriftTest'),
     ttypes = require('./gen-nodejs/ThriftTest_types');
@@ -24,81 +25,61 @@ var ThriftTest = require('./gen-nodejs/ThriftTest'),
 var connection = thrift.createConnection('localhost', 9090),
     client = thrift.createClient(ThriftTest, connection);
 
-var tfailed = 0;
-var tpassed = 0;
-
-function failed(err) {
-  console.trace(err);
-       return tfailed++;
-}
-function passed() {
-       return tpassed++;
-}
-
 connection.on('error', function(err) {
-  failed(err);
+  assert(false, err);
 });
 
-console.time("Tests completed in");
+
 
 client.testVoid(function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testVoid() = ", response);
-       passed();
+  assert( ! err);
+  assert.equal(undefined, response);
 });
 
 client.testString("Test", function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testString('Test') = ", response);
-       passed();
+  assert( ! err);
+  assert.equal("Test", response);
 });
 
 client.testByte(1, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testByte(1) = ", response);
-       passed();
+  assert( ! err);
+  assert.equal(1, response);
 });
 
 client.testI32(-1, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testI32(-1) = ", response);
-       passed();
+  assert( ! err);
+  assert.equal(-1, response);
 });
 
 client.testI64(5, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testI64(5) = ", response);
-       passed();
+  assert( ! err);
+  assert.equal(5, response);
 });
 
 client.testI64(-5, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testI64(-5) = ", response);
-       passed();
+  assert( ! err);
+  assert.equal(-5, response);
 });
 
 client.testI64(-34359738368, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testI64(-34359738368) = ", response);
-       passed();
+  assert( ! err);
+  assert.equal(-34359738368, response);
 });
 
 client.testDouble(-5.2098523, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testDouble(-5.2098523) = ", response);
-       passed();
+  assert( ! err);
+  assert.equal(-5.2098523, response);
 });
 
 var out = new ttypes.Xtruct({
-       string_thing: 'Zero',
-       byte_thing: 1,
-       i32_thing: -3,
-       i64_thing: 1000000
+  string_thing: 'Zero',
+  byte_thing: 1,
+  i32_thing: -3,
+  //i64_thing: 1000000
 });
 client.testStruct(out, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testStruct(", out, ") = \n", response);
-       passed();
+  assert( ! err);
+  assert.deepEqual(out, response);
 });
 
 var out2 = new ttypes.Xtruct2();
@@ -106,9 +87,8 @@ out2.byte_thing = 1;
 out2.struct_thing = out;
 out2.i32_thing = 5;
 client.testNest(out2, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testNest(", out2, ") = \n", response);
-       passed();
+  assert( ! err);
+  assert.deepEqual(out2, response);
 });
 
 var mapout = {};
@@ -116,9 +96,8 @@ for (var i = 0; i < 5; ++i) {
   mapout[i] = i-10;
 }
 client.testMap(mapout, function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testMap(", mapout, ") = \n", response);
-       passed();
+  assert( ! err);
+  assert.deepEqual(mapout, response);
 });
 
 /*
@@ -127,25 +106,26 @@ client.testMap(mapout, function(err, response) {
 
 
 client.testException('ApplicationException', function(err, response) {
-  console.log("testException('ApplicationException') = ", err);
-  if (response) { return failed(response); }
-       passed();
+  //assert.equal('ApplicationException', err);
+  assert( ! response);
 });
 
 client.testException('Xception', function(err, response) {
-  console.log("testException('Xception') = ", err);
-  if (response) { return failed(response); }
-       passed();
+  assert.equal('Xception', err.message);
+  assert( ! response);
 });
 
 client.testException('success', function(err, response) {
-  if (err) { return failed(err); }
-  console.log("testException('success') = ", response);
-       passed();
+  assert( ! err);
+  //assert.equal('success', response);
 });
 
-setTimeout(function(){
-  console.timeEnd("Tests completed in");
-       console.log(tpassed + " passed, " + tfailed + " failed");
-       connection.end();
+
+setTimeout(function() {
+  console.log("Server successfully tested!");
+  connection.end();
 }, 200);
+
+// to make it also run on expresso
+exports.expressoTest = function() {};
+