Skip to content

Commit

Permalink
Fix inconsistent command argument handling
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Ruben Bridgewater committed Sep 15, 2015
1 parent e24f056 commit c522ca1
Show file tree
Hide file tree
Showing 13 changed files with 208 additions and 76 deletions.
3 changes: 3 additions & 0 deletions .jshintrc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
127 changes: 72 additions & 55 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
}

Expand All @@ -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];
Expand Down Expand Up @@ -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;
};

Expand Down
10 changes: 10 additions & 0 deletions test/commands/blpop.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 10 additions & 1 deletion test/commands/client.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
2 changes: 1 addition & 1 deletion test/commands/dbsize.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down
14 changes: 14 additions & 0 deletions test/commands/del.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
5 changes: 5 additions & 0 deletions test/commands/exits.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
});
Expand Down
8 changes: 8 additions & 0 deletions test/commands/expire.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
48 changes: 31 additions & 17 deletions test/commands/flushdb.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
});
Expand Down
15 changes: 15 additions & 0 deletions test/commands/get.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
20 changes: 20 additions & 0 deletions test/commands/getset.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
Loading

0 comments on commit c522ca1

Please sign in to comment.