From 8b7d4a84496feaab49f9896de57e6373f0727a73 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 17 Sep 2015 22:51:42 +0200 Subject: [PATCH] Remove bad .eval implementation The implementation is not really good as mentioned in #722 and we pipline our commands. That way we can't just replace the eval function as it was. This could result in violating the order of execution! If we want to include a function like this we should not break the order of execution and also not recalculate the sha1 hash each time. --- index.js | 32 -------------------------------- test/commands/eval.spec.js | 31 +++++++++++++++++++++---------- 2 files changed, 21 insertions(+), 42 deletions(-) diff --git a/index.js b/index.js index 010e4a8c59..be91dc3fc3 100644 --- a/index.js +++ b/index.js @@ -6,7 +6,6 @@ var net = require("net"), Queue = require("./lib/queue"), to_array = require("./lib/to_array"), events = require("events"), - crypto = require("crypto"), parsers = [], // This static list of commands is updated from time to time. // ./lib/commands.js can be updated with generate_commands.js @@ -1083,37 +1082,6 @@ RedisClient.prototype.multi = RedisClient.prototype.MULTI = function (args) { return new Multi(this, args); }; -// stash original eval method -var eval_orig = RedisClient.prototype.eval; -// hook eval with an attempt to evalsha for cached scripts -RedisClient.prototype.eval = RedisClient.prototype.EVAL = function () { - var self = this, - args = to_array(arguments), - callback; - - if (typeof args[args.length - 1] === "function") { - callback = args.pop(); - } - - if (Array.isArray(args[0])) { - args = args[0]; - } - - // replace script source with sha value - var source = args[0]; - args[0] = crypto.createHash("sha1").update(source).digest("hex"); - - self.evalsha(args, function (err, reply) { - if (err && /NOSCRIPT/.test(err.message)) { - args[0] = source; - eval_orig.call(self, args, callback); - - } else if (callback) { - callback(err, reply); - } - }); -}; - var createClient_unix = function(path, options){ var cnxOptions = { path: path diff --git a/test/commands/eval.spec.js b/test/commands/eval.spec.js index df075f3d29..d11fc50911 100644 --- a/test/commands/eval.spec.js +++ b/test/commands/eval.spec.js @@ -12,6 +12,7 @@ describe("The 'eval' method", function () { describe("using " + parser + " and " + ip, function () { var client; + var source = "return redis.call('set', 'sha', 'test')"; beforeEach(function (done) { client = redis.createClient.apply(redis.createClient, args); @@ -94,24 +95,25 @@ describe("The 'eval' method", function () { }); }); - describe('evalsha', function () { - var source = "return redis.call('get', 'sha test')"; - var sha = crypto.createHash('sha1').update(source).digest('hex'); + it('allows a script to be executed that accesses the redis API without callback', function (done) { + helper.serverVersionAtLeast.call(this, client, [2, 5, 0]); + client.eval(source, 0); + client.get('sha', helper.isString('test', done)); + }); - beforeEach(function (done) { - client.set("sha test", "eval get sha test", function (err, res) { - return done(err); - }); - }); + describe('evalsha', function () { + var sha = crypto.createHash('sha1').update(source).digest('hex'); it('allows a script to be executed that accesses the redis API', function (done) { helper.serverVersionAtLeast.call(this, client, [2, 5, 0]); - client.eval(source, 0, helper.isString('eval get sha test', done)); + client.eval(source, 0, helper.isString('OK')); + client.get('sha', helper.isString('test', done)); }); it('can execute a script if the SHA exists', function (done) { helper.serverVersionAtLeast.call(this, client, [2, 5, 0]); - client.evalsha(sha, 0, helper.isString('eval get sha test', done)); + client.evalsha(sha, 0, helper.isString('OK')); + client.get('sha', helper.isString('test', done)); }); it('returns an error if SHA does not exist', function (done) { @@ -119,6 +121,15 @@ describe("The 'eval' method", function () { client.evalsha('ffffffffffffffffffffffffffffffffffffffff', 0, helper.isError(done)); }); + it('emit an error if SHA does not exist without any callback', function (done) { + helper.serverVersionAtLeast.call(this, client, [2, 5, 0]); + client.evalsha('ffffffffffffffffffffffffffffffffffffffff', 0); + client.on('error', function(err) { + assert(/NOSCRIPT No matching script. Please use EVAL./.test(err.message)); + done(); + }); + }); + it('emits an error if SHA does not exist and no callback has been provided', function (done) { client.on('error', function (err) { assert.equal(err.message, 'NOSCRIPT No matching script. Please use EVAL.');