From c522ca126443ac715d162694870c723869aad10f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 14 Sep 2015 15:07:32 +0200 Subject: [PATCH] Fix inconsistent command argument handling Earlier multi.command and client.command diverged a lot in the way they accepted arguments. This is now consistent This will also fix some bugs like using multi.hmset with arrays --- .jshintrc | 3 + index.js | 127 +++++++++++++++++++--------------- test/commands/blpop.spec.js | 10 +++ test/commands/client.spec.js | 11 ++- test/commands/dbsize.spec.js | 2 +- test/commands/del.spec.js | 14 ++++ test/commands/exits.spec.js | 5 ++ test/commands/expire.spec.js | 8 +++ test/commands/flushdb.spec.js | 48 ++++++++----- test/commands/get.spec.js | 15 ++++ test/commands/getset.spec.js | 20 ++++++ test/commands/hgetall.spec.js | 4 +- test/commands/multi.spec.js | 17 +++++ 13 files changed, 208 insertions(+), 76 deletions(-) diff --git a/.jshintrc b/.jshintrc index 814239e345..c5caf777dd 100644 --- a/.jshintrc +++ b/.jshintrc @@ -16,6 +16,9 @@ "node": true, // Enable globals available when code is running inside of the NodeJS runtime environment. "mocha": true, + // Relaxing options + "boss": true, // Accept things like `while (command = keys.shift()) { ... }` + "overrides": { "examples/*.js": { "unused": false diff --git a/index.js b/index.js index efca586644..60ec1f4e04 100644 --- a/index.js +++ b/index.js @@ -863,8 +863,16 @@ RedisClient.prototype.end = function () { function Multi(client, args) { this._client = client; this.queue = [["multi"]]; + var command, tmp_args; if (Array.isArray(args)) { - this.queue = this.queue.concat(args); + while (tmp_args = args.shift()) { + command = tmp_args.shift(); + if (Array.isArray(command)) { + this[command[0]].apply(this, command.slice(1).concat(tmp_args)); + } else { + this[command].apply(this, tmp_args); + } + } } } @@ -878,16 +886,32 @@ commands.forEach(function (fullCommand) { return; } - RedisClient.prototype[command] = function (args, callback) { - if (Array.isArray(args)) { - return this.send_command(command, args, callback); + RedisClient.prototype[command] = function (key, arg, callback) { + if (Array.isArray(key)) { + return this.send_command(command, key, arg); + } + if (Array.isArray(arg)) { + arg.unshift(key); + return this.send_command(command, arg, callback); } return this.send_command(command, to_array(arguments)); }; RedisClient.prototype[command.toUpperCase()] = RedisClient.prototype[command]; - Multi.prototype[command] = function () { - this.queue.push([command].concat(to_array(arguments))); + Multi.prototype[command] = function (key, arg, callback) { + if (Array.isArray(key)) { + if (arg) { + key.push(arg); + } + this.queue.push([command].concat(key)); + } else if (Array.isArray(arg)) { + if (callback) { + arg.push(callback); + } + this.queue.push([command, key].concat(arg)); + } else { + this.queue.push([command].concat(to_array(arguments))); + } return this; }; Multi.prototype[command.toUpperCase()] = Multi.prototype[command]; @@ -919,70 +943,63 @@ RedisClient.prototype.auth = RedisClient.prototype.AUTH = function (pass, callba } }; -RedisClient.prototype.hmget = RedisClient.prototype.HMGET = function (arg1, arg2, arg3) { - if (Array.isArray(arg2) && typeof arg3 === "function") { - return this.send_command("hmget", [arg1].concat(arg2), arg3); - } else if (Array.isArray(arg1) && typeof arg2 === "function") { - return this.send_command("hmget", arg1, arg2); - } else { - return this.send_command("hmget", to_array(arguments)); +RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function (key, args, callback) { + var field, tmp_args; + if (Array.isArray(key)) { + return this.send_command("hmset", key, args); } -}; - -RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function (args, callback) { - var tmp_args, tmp_keys, i, il, key; - if (Array.isArray(args)) { - return this.send_command("hmset", args, callback); + return this.send_command("hmset", [key].concat(args), callback); } - - args = to_array(arguments); - if (typeof args[args.length - 1] === "function") { - callback = args[args.length - 1]; - args.length -= 1; - } else { - callback = null; - } - - if (args.length === 2 && (typeof args[0] === "string" || typeof args[0] === "number") && typeof args[1] === "object") { + if (typeof args === "object") { // User does: client.hmset(key, {key1: val1, key2: val2}) // assuming key is a string, i.e. email address // if key is a number, i.e. timestamp, convert to string - if (typeof args[0] === "number") { - args[0] = args[0].toString(); + // TODO: This seems random and no other command get's the key converted => either all or none should behave like this + if (typeof key !== "string") { + key = key.toString(); } - - tmp_args = [ args[0] ]; - tmp_keys = Object.keys(args[1]); - for (i = 0, il = tmp_keys.length; i < il ; i++) { - key = tmp_keys[i]; - tmp_args.push(key); - tmp_args.push(args[1][key]); + tmp_args = [key]; + var fields = Object.keys(args); + while (field = fields.shift()) { + tmp_args.push(field, args[field]); } - args = tmp_args; + return this.send_command("hmset", tmp_args, callback); } - - return this.send_command("hmset", args, callback); + return this.send_command("hmset", to_array(arguments)); }; -Multi.prototype.hmset = Multi.prototype.HMSET = function () { - var args = to_array(arguments), tmp_args; - if (args.length >= 2 && typeof args[0] === "string" && typeof args[1] === "object") { - tmp_args = [ "hmset", args[0] ]; - Object.keys(args[1]).map(function (key) { - tmp_args.push(key); - tmp_args.push(args[1][key]); - }); - if (args[2]) { - tmp_args.push(args[2]); +Multi.prototype.hmset = Multi.prototype.HMSET = function (key, args, callback) { + var tmp_args, field; + if (Array.isArray(key)) { + if (args) { + key.push(args); + } + tmp_args = ['hmset'].concat(key); + } else if (Array.isArray(args)) { + if (callback) { + args.push(callback); + } + tmp_args = ['hmset', key].concat(args); + } else if (typeof args === "object") { + tmp_args = ["hmset", key]; + if (typeof key !== "string") { + key = key.toString(); + } + var fields = Object.keys(args); + while (field = fields.shift()) { + tmp_args.push(field); + tmp_args.push(args[field]); + } + if (callback) { + tmp_args.push(callback); } - args = tmp_args; } else { - args.unshift("hmset"); + tmp_args = to_array(arguments); + tmp_args.unshift("hmset"); } - - this.queue.push(args); + this.queue.push(tmp_args); return this; }; diff --git a/test/commands/blpop.spec.js b/test/commands/blpop.spec.js index 66a85dd3b4..7f1aeb1947 100644 --- a/test/commands/blpop.spec.js +++ b/test/commands/blpop.spec.js @@ -33,6 +33,16 @@ describe("The 'blpop' method", function () { }); }); + it('pops value immediately if list contains values using array notation', function (done) { + bclient = redis.createClient.apply(redis.createClient, args); + client.rpush(["blocking list", "initial value"], helper.isNumber(1)); + bclient.blpop(["blocking list", 0], function (err, value) { + assert.strictEqual(value[0], "blocking list"); + assert.strictEqual(value[1], "initial value"); + return done(err); + }); + }); + it('waits for value if list is not yet populated', function (done) { bclient = redis.createClient.apply(redis.createClient, args); bclient.blpop("blocking list 2", 5, function (err, value) { diff --git a/test/commands/client.spec.js b/test/commands/client.spec.js index 1d47db708d..5636df976c 100644 --- a/test/commands/client.spec.js +++ b/test/commands/client.spec.js @@ -37,8 +37,17 @@ describe("The 'client' method", function () { }); }); + it("lists connected clients when invoked with array syntax on client", function (done) { + client.multi().client(["list"]).exec(function(err, results) { + assert(pattern.test(results[0]), "expected string '" + results + "' to match " + pattern.toString()); + return done(); + }); + }); + it("lists connected clients when invoked with multi's array syntax", function (done) { - client.multi().client("list").exec(function(err, results) { + client.multi([ + ['client', 'list'] + ]).exec(function(err, results) { assert(pattern.test(results[0]), "expected string '" + results + "' to match " + pattern.toString()); return done(); }); diff --git a/test/commands/dbsize.spec.js b/test/commands/dbsize.spec.js index 845a4adaa1..d3e8c07daf 100644 --- a/test/commands/dbsize.spec.js +++ b/test/commands/dbsize.spec.js @@ -71,7 +71,7 @@ describe("The 'dbsize' method", function () { var oldSize; beforeEach(function (done) { - client.dbsize([], function (err, res) { + client.dbsize(function (err, res) { helper.isType.number()(err, res); assert.strictEqual(res, 0, "Initial db size should be 0"); diff --git a/test/commands/del.spec.js b/test/commands/del.spec.js index 152e9c1e45..69f3930c29 100644 --- a/test/commands/del.spec.js +++ b/test/commands/del.spec.js @@ -36,6 +36,20 @@ describe("The 'del' method", function () { client.get('apple', helper.isNull(done)); }); + it('allows multiple keys to be deleted with the array syntax', function (done) { + client.mset('foo', 'bar', 'apple', 'banana'); + client.del(['foo', 'apple'], helper.isNumber(2)); + client.get('foo', helper.isNull()); + client.get('apple', helper.isNull(done)); + }); + + it('allows multiple keys to be deleted with the array syntax and no callback', function (done) { + client.mset('foo', 'bar', 'apple', 'banana'); + client.del(['foo', 'apple']); + client.get('foo', helper.isNull()); + client.get('apple', helper.isNull(done)); + }); + afterEach(function () { client.end(); }); diff --git a/test/commands/exits.spec.js b/test/commands/exits.spec.js index 3e7074268f..1d6d778b39 100644 --- a/test/commands/exits.spec.js +++ b/test/commands/exits.spec.js @@ -24,6 +24,11 @@ describe("The 'exits' method", function () { client.EXISTS('foo', helper.isNumber(1, done)); }); + it('returns 1 if the key exists with array syntax', function (done) { + client.set('foo', 'bar'); + client.EXISTS(['foo'], helper.isNumber(1, done)); + }); + it('returns 0 if the key does not exist', function (done) { client.exists('bar', helper.isNumber(0, done)); }); diff --git a/test/commands/expire.spec.js b/test/commands/expire.spec.js index 3e700206e6..b0480b6c1d 100644 --- a/test/commands/expire.spec.js +++ b/test/commands/expire.spec.js @@ -20,6 +20,14 @@ describe("The 'expire' method", function () { }); it('expires key after timeout', function (done) { + client.set(['expiry key', 'bar'], helper.isString("OK")); + client.EXPIRE("expiry key", "1", helper.isNumber(1)); + setTimeout(function () { + client.exists(["expiry key"], helper.isNumber(0, done)); + }, 1100); + }); + + it('expires key after timeout with array syntax', function (done) { client.set(['expiry key', 'bar'], helper.isString("OK")); client.EXPIRE(["expiry key", "1"], helper.isNumber(1)); setTimeout(function () { diff --git a/test/commands/flushdb.spec.js b/test/commands/flushdb.spec.js index 3ba5214040..c6f3c83e19 100644 --- a/test/commands/flushdb.spec.js +++ b/test/commands/flushdb.spec.js @@ -58,36 +58,50 @@ describe("The 'flushdb' method", function () { describe("when there is data in Redis", function () { beforeEach(function (done) { - var end = helper.callFuncAfter(function () { - client.flushdb(helper.isString("OK", done)); - }, 2); - client.mset(key, uuid.v4(), key2, uuid.v4(), helper.isNotError(end)); + client.mset(key, uuid.v4(), key2, uuid.v4(), helper.isNotError()); client.dbsize([], function (err, res) { helper.isType.positiveNumber()(err, res); assert.equal(res, 2, 'Two keys should have been inserted'); - end(); + done(); }); }); it("deletes all the keys", function (done) { - client.mget(key, key2, function (err, res) { - assert.strictEqual(null, err, "Unexpected error returned"); - assert.strictEqual(true, Array.isArray(res), "Results object should be an array."); - assert.strictEqual(2, res.length, "Results array should have length 2."); - assert.strictEqual(null, res[0], "Redis key should have been flushed."); - assert.strictEqual(null, res[1], "Redis key should have been flushed."); - done(err); + client.flushdb(function(err, res) { + assert.equal(res, 'OK'); + client.mget(key, key2, function (err, res) { + assert.strictEqual(null, err, "Unexpected error returned"); + assert.strictEqual(true, Array.isArray(res), "Results object should be an array."); + assert.strictEqual(2, res.length, "Results array should have length 2."); + assert.strictEqual(null, res[0], "Redis key should have been flushed."); + assert.strictEqual(null, res[1], "Redis key should have been flushed."); + done(err); + }); }); }); it("results in a db size of zero", function (done) { - client.dbsize([], function (err, res) { - helper.isNotError()(err, res); - helper.isType.number()(err, res); - assert.strictEqual(0, res, "Flushing db should result in db size 0"); - done(); + client.flushdb(function(err, res) { + client.dbsize([], function (err, res) { + helper.isNotError()(err, res); + helper.isType.number()(err, res); + assert.strictEqual(0, res, "Flushing db should result in db size 0"); + done(); + }); }); }); + + it("results in a db size of zero without a callback", function (done) { + client.flushdb(); + setTimeout(function(err, res) { + client.dbsize([], function (err, res) { + helper.isNotError()(err, res); + helper.isType.number()(err, res); + assert.strictEqual(0, res, "Flushing db should result in db size 0"); + done(); + }); + }, 25); + }); }); }); }); diff --git a/test/commands/get.spec.js b/test/commands/get.spec.js index 1a6f641532..014df57446 100644 --- a/test/commands/get.spec.js +++ b/test/commands/get.spec.js @@ -70,6 +70,21 @@ describe("The 'get' method", function () { done(err); }); }); + + it("gets the value correctly with array syntax and the callback being in the array", function (done) { + client.GET([key, function (err, res) { + helper.isString(value)(err, res); + done(err); + }]); + }); + + it("should not throw on a get without callback (even if it's not useful", function (done) { + client.GET(key); + client.on('error', function(err) { + throw err; + }); + setTimeout(done, 50); + }); }); describe("when the key does not exist in Redis", function () { diff --git a/test/commands/getset.spec.js b/test/commands/getset.spec.js index e650ccaa5f..41c2dc3a8e 100644 --- a/test/commands/getset.spec.js +++ b/test/commands/getset.spec.js @@ -74,6 +74,26 @@ describe("The 'getset' method", function () { }); }); }); + + it("gets the value correctly with array syntax", function (done) { + client.GETSET([key, value2], function (err, res) { + helper.isString(value)(err, res); + client.get(key, function (err, res) { + helper.isString(value2)(err, res); + done(err); + }); + }); + }); + + it("gets the value correctly with array syntax style 2", function (done) { + client.GETSET(key, [value2], function (err, res) { + helper.isString(value)(err, res); + client.get(key, function (err, res) { + helper.isString(value2)(err, res); + done(err); + }); + }); + }); }); describe("when the key does not exist in Redis", function () { diff --git a/test/commands/hgetall.spec.js b/test/commands/hgetall.spec.js index 74b506e6f0..e45995aa35 100644 --- a/test/commands/hgetall.spec.js +++ b/test/commands/hgetall.spec.js @@ -67,7 +67,7 @@ describe("The 'hgetall' method", function () { it('returns binary results', function (done) { client.hmset(["bhosts", "mjr", "1", "another", "23", "home", "1234", new Buffer([0xAA, 0xBB, 0x00, 0xF0]), new Buffer([0xCC, 0xDD, 0x00, 0xF0])], helper.isString("OK")); - client.HGETALL(["bhosts"], function (err, obj) { + client.HGETALL(["bhosts", function (err, obj) { assert.strictEqual(4, Object.keys(obj).length); assert.strictEqual("1", obj.mjr.toString()); assert.strictEqual("23", obj.another.toString()); @@ -75,7 +75,7 @@ describe("The 'hgetall' method", function () { assert.strictEqual((new Buffer([0xAA, 0xBB, 0x00, 0xF0])).toString('binary'), Object.keys(obj)[3]); assert.strictEqual((new Buffer([0xCC, 0xDD, 0x00, 0xF0])).toString('binary'), obj[(new Buffer([0xAA, 0xBB, 0x00, 0xF0])).toString('binary')].toString('binary')); return done(err); - }); + }]); }); }); diff --git a/test/commands/multi.spec.js b/test/commands/multi.spec.js index 45c3ab3f1a..4f78abf8ec 100644 --- a/test/commands/multi.spec.js +++ b/test/commands/multi.spec.js @@ -165,6 +165,23 @@ describe("The 'multi' method", function () { }); }); + it('allows multiple commands to work the same as normal to be performed using a chaining API', function (done) { + client.multi() + .mset(['some', '10', 'keys', '20']) + .incr('some') + .incr('keys') + .mget('some', 'keys') + .exec(function (err, replies) { + assert.strictEqual(null, err); + assert.equal('OK', replies[0]); + assert.equal(11, replies[1]); + assert.equal(21, replies[2]); + assert.equal(11, replies[3][0].toString()); + assert.equal(21, replies[3][1].toString()); + return done(); + }); + }); + it('allows an array to be provided indicating multiple operations to perform', function (done) { // test nested multi-bulk replies with nulls. client.multi([