Deprecate and warn on null / undefined arguments

Author: Ruben Bridgewater

README.md

@@ -211,6 +211,7 @@ limits total amount of connection tries. Setting this to 1 will prevent any reco
 * `auth_pass`: *null*; If set, client will run redis auth command on connect.
 * `family`: *IPv4*; You can force using IPv6 if you set the family to 'IPv6'. See Node.js [net](https://nodejs.org/api/net.html) or [dns](https://nodejs.org/api/dns.html) modules how to use the family type.
 * `disable_resubscribing`: *false*; If set to `true`, a client won't resubscribe after disconnecting
+* `to_empty_string`: *null*; convert any undefined or null command argument to an empty string. Be careful using this
 * `rename_commands`: *null*; pass a object with renamed commands to use those instead of the original functions. See the [redis security topics](http://redis.io/topics/security) for more info.
 * `tls`: an object containing options to pass to [tls.connect](http://nodejs.org/api/tls.html#tls_tls_connect_port_host_options_callback),
 to set up a TLS connection to Redis (if, for example, it is set up to be accessible via a tunnel).

index.js

@@ -719,39 +719,49 @@ RedisClient.prototype.send_command = function (command, args, callback) {
         callback = process.domain.bind(callback);
     }
 
-    if (command === 'set' || command === 'setex') {
-        // if the value is undefined or null and command is set or setx, need not to send message to redis
-        if (args[args.length - 1] === undefined || args[args.length - 1] === null) {
-            command = command.toUpperCase();
-            err = new Error('send_command: ' + command + ' value must not be undefined or null');
-            err.command = command;
-            this.callback_emit_error(callback, err);
-            // Singal no buffering
-            return true;
-        }
-    }
-
     for (i = 0; i < args.length; i += 1) {
-        if (Buffer.isBuffer(args[i])) {
+        if (typeof args[i] === 'string') {
+            // 30000 seemed to be a good value to switch to buffers after testing and checking the pros and cons
+            if (args[i].length > 30000) {
+                big_data = true;
+                args[i] = new Buffer(args[i], 'utf8');
+                if (this.pipeline !== 0) {
+                    this.pipeline += 2;
+                    this.writeDefault = this.writeBuffers;
+                }
+            }
+        } else if (args[i] === null) {
+            if (!this.options.to_empty_string) { // > 2  // ( 2 + 3 where 3 stands for both, null and undefined and 3 for null)
+                console.warn(
+                    'node_redis: Deprecated: The %s command contains a "null" argument.\n' +
+                    'This is converted to a "null" string now and will return an error from v.3.0 on.\n' +
+                    'If you wish to convert null to an empty string instead, please use the "to_empty_string" option.', command.toUpperCase()
+                );
+                args[i] = 'null'; // Backwards compatible :/
+            } else {
+                args[i] = '';
+            }
+        } else if (typeof args[i] === 'object') { // Buffer.isBuffer(args[i])) {
             buffer_args = true;
             if (this.pipeline !== 0) {
                 this.pipeline += 2;
                 this.writeDefault = this.writeBuffers;
             }
-        } else if (typeof args[i] !== 'string') {
-            args[i] = String(args[i]);
-        // 30000 seemed to be a good value to switch to buffers after testing and checking the pros and cons
-        } else if (args[i].length > 30000) {
-            big_data = true;
-            args[i] = new Buffer(args[i]);
-            if (this.pipeline !== 0) {
-                this.pipeline += 2;
-                this.writeDefault = this.writeBuffers;
+        } else if (args[i] === undefined) {
+            if (!this.options.to_empty_string) {
+                console.warn(
+                    'node_redis: Deprecated: The %s command contains a "undefined" argument.\n' +
+                    'This is converted to a "undefined" string now and will return an error from v.3.0 on.\n' +
+                    'If you wish to convert undefined to an empty string instead, please use the "to_empty_string" option.', command.toUpperCase()
+                );
+                args[i] = 'undefined'; // Backwards compatible :/
+            } else {
+                args[i] = '';
             }
         }
     }
 
-    command_obj = new Command(command, args, false, buffer_args, callback);
+    command_obj = new Command(command, args, buffer_args, callback);
 
     if (!this.ready && !this.send_anyway || !stream.writable) {
         if (this.closing || !this.enable_offline_queue) {
@@ -812,9 +822,9 @@ RedisClient.prototype.send_command = function (command, args, callback) {
 
         for (i = 0; i < args.length; i += 1) {
             arg = args[i];
-            if (typeof arg === 'string') {
+            if (typeof arg !== 'object') { // string; number; boolean
                 this.write('$' + Buffer.byteLength(arg) + '\r\n' + arg + '\r\n');
-            } else {
+            } else { // buffer
                 this.write('$' + arg.length + '\r\n');
                 this.write(arg);
                 this.write('\r\n');

lib/command.js

@@ -2,10 +2,9 @@
 
 // This Command constructor is ever so slightly faster than using an object literal, but more importantly, using
 // a named constructor helps it show up meaningfully in the V8 CPU profiler and in heap snapshots.
-function Command(command, args, sub_command, buffer_args, callback) {
+function Command(command, args, buffer_args, callback) {
     this.command = command;
     this.args = args;
-    this.sub_command = sub_command;
     this.buffer_args = buffer_args;
     this.callback = callback;
 }

test/commands/incr.spec.js

@@ -43,7 +43,6 @@ describe("The 'incr' method", function () {
             describe("when connected and a value in Redis", function () {
                 var client;
 
-                // Also, why tf were these disabled for hiredis? They work just fine.
                 before(function (done) {
                     /*
                         9007199254740992 -> 9007199254740992

test/commands/set.spec.js

@@ -113,7 +113,7 @@ describe("The 'set' method", function () {
                     describe("with undefined 'key' and missing 'value' parameter", function () {
                         it("emits an error without callback", function (done) {
                             client.on('error', function (err) {
-                                assert.equal(err.message, 'send_command: SET value must not be undefined or null');
+                                assert.equal(err.message, "ERR wrong number of arguments for 'set' command");
                                 assert.equal(err.command, 'SET');
                                 done();
                             });
@@ -132,13 +132,10 @@ describe("The 'set' method", function () {
 
                     it("emit an error without any parameters", function (done) {
                         client.once("error", function (err) {
-                            assert.equal(err.message, 'send_command: SET value must not be undefined or null');
+                            assert.equal(err.message, "ERR wrong number of arguments for 'set' command");
                             assert.equal(err.command, 'SET');
                             done();
                         });
-
-                        // This was not supported not to throw earlier and was added by the test refactoring
-                        // https://github.com/NodeRedis/node_redis/commit/eaca486ab1aecd1329f7452ad2f2255b1263606f
                         client.set();
                     });
                 });

test/commands/setex.spec.js

@@ -21,18 +21,14 @@ describe("The 'setex' method", function () {
 
             it('sets a key with an expiry', function (done) {
                 client.setex(["setex key", "100", "setex val"], helper.isString("OK"));
-                client.exists(["setex key"], helper.isNumber(1));
+                var buffering = client.exists(["setex key"], helper.isNumber(1));
+                assert(typeof buffering === 'boolean');
                 client.ttl(['setex key'], function (err, ttl) {
-                    assert.ok(ttl > 0);
+                    assert(ttl > 0);
                     return done();
                 });
             });
 
-            it('returns an error if no value is provided', function (done) {
-                var buffering = client.SETEX(["setex key", "100", undefined], helper.isError(done));
-                assert(typeof buffering === 'boolean');
-            });
-
             afterEach(function () {
                 client.end(true);
             });

test/connection.spec.js

@@ -90,7 +90,7 @@ describe("connection tests", function () {
                     });
 
                     client.on("reconnecting", function (params) {
-                        client.end();
+                        client.end(true);
                         setTimeout(done, 100);
                     });
                 });