From 5f261c5823d9062c43ebb9845c86d783d067ca68 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 25 Sep 2015 00:54:16 +0200 Subject: [PATCH] Minor changes Move utility functions in lib/utils.js Improve the js parser in cases the buffer is incomplete Rename lib/parser to lib/parsers Fix smaller issues with test suite and fix parser errors not being catched Fixed wrong test for the new .end flush parameter Fixed test suite options being partly mutated Add some more tests --- index.js | 64 +++++++-------------------- lib/{parser => parsers}/hiredis.js | 8 +++- lib/{parser => parsers}/javascript.js | 26 ++++++----- lib/to_array.js | 14 ------ lib/utils.js | 53 ++++++++++++++++++++++ package.json | 2 +- test/commands/geoadd.spec.js.future | 34 ++++++++++++++ test/commands/set.spec.js | 10 +++++ test/commands/sync.spec.js | 43 ++++++++++++++++++ test/helper.js | 6 +-- test/lib/config.js | 3 +- test/node_redis.spec.js | 49 ++++++++++++-------- test/parser/javascript.spec.js | 2 +- 13 files changed, 212 insertions(+), 102 deletions(-) rename lib/{parser => parsers}/hiredis.js (80%) rename lib/{parser => parsers}/javascript.js (96%) delete mode 100644 lib/to_array.js create mode 100644 lib/utils.js create mode 100644 test/commands/geoadd.spec.js.future create mode 100644 test/commands/sync.spec.js diff --git a/index.js b/index.js index ab7d11e335..dcdbaf96bb 100644 --- a/index.js +++ b/index.js @@ -3,8 +3,8 @@ var net = require("net"), URL = require("url"), util = require("util"), + utils = require("./lib/utils"), Queue = require("./lib/queue"), - to_array = require("./lib/to_array"), events = require("events"), parsers = [], // This static list of commands is updated from time to time. @@ -23,14 +23,13 @@ exports.debug_mode = /\bredis\b/i.test(process.env.NODE_DEBUG); // hiredis might not be installed try { - require("./lib/parser/hiredis"); - parsers.push(require("./lib/parser/hiredis")); + parsers.push(require("./lib/parsers/hiredis")); } catch (err) { /* istanbul ignore next: won't be reached with tests */ debug("Hiredis parser not installed."); } -parsers.push(require("./lib/parser/javascript")); +parsers.push(require("./lib/parsers/javascript")); function RedisClient(stream, options) { options = options || {}; @@ -135,6 +134,7 @@ RedisClient.prototype.flush_and_error = function (error) { while (command_obj = this.offline_queue.shift()) { if (typeof command_obj.callback === "function") { + error.command = command_obj.command.toUpperCase(); command_obj.callback(error); } } @@ -142,6 +142,7 @@ RedisClient.prototype.flush_and_error = function (error) { while (command_obj = this.command_queue.shift()) { if (typeof command_obj.callback === "function") { + error.command = command_obj.command.toUpperCase(); command_obj.callback(error); } } @@ -530,41 +531,6 @@ RedisClient.prototype.return_error = function (err) { } }; -// hgetall converts its replies to an Object. If the reply is empty, null is returned. -function reply_to_object(reply) { - var obj = {}, j, jl, key, val; - - if (reply.length === 0 || !Array.isArray(reply)) { - return null; - } - - for (j = 0, jl = reply.length; j < jl; j += 2) { - key = reply[j].toString('binary'); - val = reply[j + 1]; - obj[key] = val; - } - - return obj; -} - -function reply_to_strings(reply) { - var i; - - if (Buffer.isBuffer(reply)) { - return reply.toString(); - } - - if (Array.isArray(reply)) { - for (i = 0; i < reply.length; i++) { - // Recusivly call the function as slowlog returns deep nested replies - reply[i] = reply_to_strings(reply[i]); - } - return reply; - } - - return reply; -} - RedisClient.prototype.return_reply = function (reply) { var command_obj, len, type, timestamp, argindex, args, queue_len; @@ -598,12 +564,12 @@ RedisClient.prototype.return_reply = function (reply) { if (this.options.detect_buffers && command_obj.buffer_args === false) { // If detect_buffers option was specified, then the reply from the parser will be Buffers. // If this command did not use Buffer arguments, then convert the reply to Strings here. - reply = reply_to_strings(reply); + reply = utils.reply_to_strings(reply); } // TODO - confusing and error-prone that hgetall is special cased in two places if (reply && 'hgetall' === command_obj.command) { - reply = reply_to_object(reply); + reply = utils.reply_to_object(reply); } } @@ -614,7 +580,7 @@ RedisClient.prototype.return_reply = function (reply) { } else if (this.pub_sub_mode || command_obj && command_obj.sub_command) { if (Array.isArray(reply)) { if (!this.options.return_buffers && (!command_obj || this.options.detect_buffers && command_obj.buffer_args === false)) { - reply = reply_to_strings(reply); + reply = utils.reply_to_strings(reply); } type = reply[0].toString(); @@ -850,7 +816,7 @@ RedisClient.prototype.end = function (flush) { // Flush queue if wanted if (flush) { - this.flush_and_error("Redis connection ended."); + this.flush_and_error(new Error("The command can't be processed. The connection has already been closed.")); } this.connected = false; @@ -894,7 +860,7 @@ commands.forEach(function (fullCommand) { arg = [key].concat(arg); return this.send_command(command, arg, callback); } - return this.send_command(command, to_array(arguments)); + return this.send_command(command, utils.to_array(arguments)); }; RedisClient.prototype[command.toUpperCase()] = RedisClient.prototype[command]; @@ -910,7 +876,7 @@ commands.forEach(function (fullCommand) { } this.queue.push([command, key].concat(arg)); } else { - this.queue.push([command].concat(to_array(arguments))); + this.queue.push([command].concat(utils.to_array(arguments))); } return this; }; @@ -979,7 +945,7 @@ RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function (key, args, } return this.send_command("hmset", tmp_args, callback); } - return this.send_command("hmset", to_array(arguments)); + return this.send_command("hmset", utils.to_array(arguments)); }; Multi.prototype.hmset = Multi.prototype.HMSET = function (key, args, callback) { @@ -1008,7 +974,7 @@ Multi.prototype.hmset = Multi.prototype.HMSET = function (key, args, callback) { tmp_args.push(callback); } } else { - tmp_args = to_array(arguments); + tmp_args = utils.to_array(arguments); tmp_args.unshift("hmset"); } this.queue.push(tmp_args); @@ -1087,11 +1053,11 @@ Multi.prototype.execute_callback = function (err, replies) { replies[i].command = args[0].toUpperCase(); } else if (replies[i]) { if (this._client.options.detect_buffers && this.wants_buffers[i + 1] === false) { - replies[i] = reply_to_strings(replies[i]); + replies[i] = utils.reply_to_strings(replies[i]); } if (args[0] === "hgetall") { // TODO - confusing and error-prone that hgetall is special cased in two places - replies[i] = reply_to_object(replies[i]); + replies[i] = utils.reply_to_object(replies[i]); } } diff --git a/lib/parser/hiredis.js b/lib/parsers/hiredis.js similarity index 80% rename from lib/parser/hiredis.js rename to lib/parsers/hiredis.js index 765304bec5..5be6bf2ac9 100644 --- a/lib/parser/hiredis.js +++ b/lib/parsers/hiredis.js @@ -18,7 +18,13 @@ HiredisReplyParser.prototype.execute = function (data) { var reply; this.reader.feed(data); while (true) { - reply = this.reader.get(); + try { + reply = this.reader.get(); + } catch (err) { + // Protocol errors land here + this.send_error(err); + break; + } if (reply === undefined) { break; diff --git a/lib/parser/javascript.js b/lib/parsers/javascript.js similarity index 96% rename from lib/parser/javascript.js rename to lib/parsers/javascript.js index 4ce4168c43..dbbe658b6b 100644 --- a/lib/parser/javascript.js +++ b/lib/parsers/javascript.js @@ -30,14 +30,13 @@ ReplyParser.prototype._parseResult = function (type) { end = this._packetEndOffset() - 1; start = this._offset; - // include the delimiter - this._offset = end + 2; - if (end > this._buffer.length) { - this._offset = start; throw new IncompleteReadBuffer("Wait for more data."); } + // include the delimiter + this._offset = end + 2; + if (type === 45) { return new Error(this._buffer.toString(this._encoding, start, end)); } else if (this.return_buffers) { @@ -49,14 +48,13 @@ ReplyParser.prototype._parseResult = function (type) { end = this._packetEndOffset() - 1; start = this._offset; - // include the delimiter - this._offset = end + 2; - if (end > this._buffer.length) { - this._offset = start; throw new IncompleteReadBuffer("Wait for more data."); } + // include the delimiter + this._offset = end + 2; + // return the coerced numeric value return +this._buffer.toString('ascii', start, end); } else if (type === 36) { // $ @@ -74,14 +72,13 @@ ReplyParser.prototype._parseResult = function (type) { end = this._offset + packetHeader.size; start = this._offset; - // set the offset to after the delimiter - this._offset = end + 2; - if (end > this._buffer.length) { - this._offset = offset; throw new IncompleteReadBuffer("Wait for more data."); } + // set the offset to after the delimiter + this._offset = end + 2; + if (this.return_buffers) { return this._buffer.slice(start, end); } @@ -157,6 +154,11 @@ ReplyParser.prototype.execute = function (buffer) { ret = this._parseResult(type); this.send_reply(ret); + } else if (type === 10 || type === 13) { + break; + } else { + var err = new Error('Protocol error, got "' + String.fromCharCode(type) + '" as reply type byte'); + this.send_error(err); } } catch (err) { // catch the error (not enough data), rewind, and wait diff --git a/lib/to_array.js b/lib/to_array.js deleted file mode 100644 index 87804f8c3b..0000000000 --- a/lib/to_array.js +++ /dev/null @@ -1,14 +0,0 @@ -'use strict'; - -function to_array(args) { - var len = args.length, - arr = new Array(len), i; - - for (i = 0; i < len; i += 1) { - arr[i] = args[i]; - } - - return arr; -} - -module.exports = to_array; diff --git a/lib/utils.js b/lib/utils.js new file mode 100644 index 0000000000..42ae211c3a --- /dev/null +++ b/lib/utils.js @@ -0,0 +1,53 @@ +'use strict'; + +// hgetall converts its replies to an Object. If the reply is empty, null is returned. +function replyToObject(reply) { + var obj = {}, j, jl, key, val; + + if (reply.length === 0 || !Array.isArray(reply)) { + return null; + } + + for (j = 0, jl = reply.length; j < jl; j += 2) { + key = reply[j].toString('binary'); + val = reply[j + 1]; + obj[key] = val; + } + + return obj; +} + +function replyToStrings(reply) { + var i; + + if (Buffer.isBuffer(reply)) { + return reply.toString(); + } + + if (Array.isArray(reply)) { + for (i = 0; i < reply.length; i++) { + // Recusivly call the function as slowlog returns deep nested replies + reply[i] = replyToStrings(reply[i]); + } + return reply; + } + + return reply; +} + +function toArray(args) { + var len = args.length, + arr = new Array(len), i; + + for (i = 0; i < len; i += 1) { + arr[i] = args[i]; + } + + return arr; +} + +module.exports = { + reply_to_strings: replyToStrings, + reply_to_object: replyToObject, + to_array: toArray +}; diff --git a/package.json b/package.json index eb8438cc6e..208bdbbbb2 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "devDependencies": { "coveralls": "^2.11.2", "jshint": "^2.8.0", - "metrics": ">=0.1.5", + "metrics": "^0.1.9", "mocha": "^2.3.2", "nyc": "^3.2.2", "optional-dev-dependency": "^1.1.0", diff --git a/test/commands/geoadd.spec.js.future b/test/commands/geoadd.spec.js.future new file mode 100644 index 0000000000..710b528f80 --- /dev/null +++ b/test/commands/geoadd.spec.js.future @@ -0,0 +1,34 @@ +'use strict'; + +var config = require("../lib/config"); +var helper = require("../helper"); +var redis = config.redis; + +describe("The 'getoadd' method", function () { + + helper.allTests(function(parser, ip, args) { + + describe("using " + parser + " and " + ip, function () { + var client; + + beforeEach(function (done) { + client = redis.createClient.apply(redis.createClient, args); + client.once("connect", function () { + client.flushdb(done); + }); + }); + + it('returns 1 if the key exists', function (done) { + client.geoadd("mycity:21:0:location", "13.361389","38.115556","COR", function(err, res) { + console.log(err, res); + // geoadd is still in the unstable branch. As soon as it reaches the stable one, activate this test + done(); + }); + }); + + afterEach(function () { + client.end(); + }); + }); + }); +}); diff --git a/test/commands/set.spec.js b/test/commands/set.spec.js index c295af1d3d..dbdd3103e9 100644 --- a/test/commands/set.spec.js +++ b/test/commands/set.spec.js @@ -88,6 +88,16 @@ describe("The 'set' method", function () { }); }, 100); }); + + it("sets the value correctly with the array syntax", function (done) { + client.set([key, value]); + setTimeout(function () { + client.get(key, function (err, res) { + helper.isString(value)(err, res); + done(); + }); + }, 100); + }); }); describe("with undefined 'key' and missing 'value' parameter", function () { diff --git a/test/commands/sync.spec.js b/test/commands/sync.spec.js new file mode 100644 index 0000000000..7e7702c8b5 --- /dev/null +++ b/test/commands/sync.spec.js @@ -0,0 +1,43 @@ +'use strict'; + +var assert = require('assert'); +var config = require("../lib/config"); +var helper = require("../helper"); +var redis = config.redis; + +describe("The 'sync' method", function () { + + helper.allTests(function(parser, ip, args) { + + describe("using " + parser + " and " + ip, function () { + var client; + + beforeEach(function (done) { + client = redis.createClient.apply(redis.createClient, args); + client.once("connect", function () { + client.flushdb(done); + }); + }); + + // This produces a parser error + // Protocol error, got "K" as reply type byte + // I'm uncertain if this is correct behavior or not + it('try to sync with the server and fail other commands', function (done) { + client.on('error', function(err) { + assert.equal(err.message, 'Protocol error, got "K" as reply type byte'); + assert.equal(err.command, 'SET'); + done(); + }); + client.sync(function(err, res) { + assert.equal(err, null); + assert(!!res); + }); + client.set('foo', 'bar'); + }); + + afterEach(function () { + client.end(); + }); + }); + }); +}); diff --git a/test/helper.js b/test/helper.js index 776c2ca3ee..a9f66d4a40 100644 --- a/test/helper.js +++ b/test/helper.js @@ -31,12 +31,10 @@ module.exports = { redisProcess: function () { return rp; }, - stopRedis: function (done) { + stopRedis: function(done) { rp.stop(done); }, - startRedis: function (conf, done) { - startRedis(conf, done); - }, + startRedis: startRedis, isNumber: function (expected, done) { return function (err, results) { assert.strictEqual(null, err, "expected " + expected + ", got error: " + err); diff --git a/test/lib/config.js b/test/lib/config.js index d42dc21c9a..c6f1b25dae 100644 --- a/test/lib/config.js +++ b/test/lib/config.js @@ -13,7 +13,8 @@ var config = { }, configureClient: function (parser, ip, opts) { var args = []; - opts = opts || {}; + // Do not manipulate the opts => copy them each time + opts = opts ? JSON.parse(JSON.stringify(opts)) : {}; if (ip.match(/\.sock/)) { args.push(ip); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 2dabc68c8d..34ba79e3dd 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -156,8 +156,8 @@ describe("The node_redis client", function () { process.once('uncaughtException', function (err) { process.on('uncaughtException', mochaListener); - // assert(/ERR Protocol error/.test(err.message)); - // assert.equal(err.command, true); + assert(/ERR Protocol error/.test(err.message)); + assert.equal(err.command, true); assert.equal(err.code, 'ERR'); done(); }); @@ -177,25 +177,36 @@ describe("The node_redis client", function () { describe(".end", function () { it('used without flush', function(done) { - var err = null; - client.set('foo', 'bar'); - client.end(); - client.get('foo', function(err, res) { - err = new Error('failed'); - }); - setTimeout(function() { - done(err); - }, 200); + var end = helper.callFuncAfter(function() { + done(new Error('failed')); + }, 20); + var cb = function(err, res) { + assert.equal(err.message, "SET can't be processed. The connection has already been closed."); + end(); + }; + for (var i = 0; i < 20; i++) { + if (i === 10) { + client.end(); + } + client.set('foo', 'bar', cb); + } + setTimeout(done, 250); }); it('used with flush set to true', function(done) { - client.set('foo', 'bar'); - client.end(); - client.get('foo', function(err, res) { - assert.strictEqual(err.command, 'GET'); - assert.strictEqual(err.message, "GET can't be processed. The connection has already been closed."); + var end = helper.callFuncAfter(function() { done(); - }); + }, 20); + var cb = function(err, res) { + assert(/The connection has already been closed./.test(err.message)); + end(); + }; + for (var i = 0; i < 20; i++) { + if (i === 10) { + client.end(true); + } + client.set('foo', 'bar', cb); + } }); }); @@ -730,7 +741,7 @@ describe("The node_redis client", function () { describe('retry_max_delay', function () { var client; var args = config.configureClient(parser, ip, { - retry_max_delay: 1 + retry_max_delay: 1 // ms }); it("sets upper bound on how long client waits before reconnecting", function (done) { @@ -747,7 +758,7 @@ describe("The node_redis client", function () { } else { client.end(); var lasted = new Date().getTime() - time; - assert.ok(lasted < 1000); + assert.ok(lasted < 50); return done(); } }); diff --git a/test/parser/javascript.spec.js b/test/parser/javascript.spec.js index 19df991675..491a292ab1 100644 --- a/test/parser/javascript.spec.js +++ b/test/parser/javascript.spec.js @@ -1,7 +1,7 @@ 'use strict'; var assert = require('assert'); -var Parser = require("../../lib/parser/javascript").Parser; +var Parser = require("../../lib/parsers/javascript").Parser; var config = require("../lib/config"); var redis = config.redis;