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

Author: Ruben Bridgewater

.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

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);
-    }
-
-    args = to_array(arguments);
-    if (typeof args[args.length - 1] === "function") {
-        callback = args[args.length - 1];
-        args.length -= 1;
-    } else {
-        callback = null;
+        return this.send_command("hmset", [key].concat(args), callback);
     }
-
-    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;
 };
 

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) {

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();
                     });

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");
 

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();
             });

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));
             });

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 () {

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);
+                    });
                 });
             });
         });

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 () {