[GH-67] - hgetall now returns null instead of {} on empty reply

README.md

@@ -206,7 +206,8 @@ to start over.
 
 ## Friendlier hash commands
 
-Most Redis commands take a single String or an Array of Strings as arguments, and replies are sent back as a single String or an Array of Strings.  When dealing with hash values, there are a couple of useful exceptions to this.
+Most Redis commands take a single String or an Array of Strings as arguments, and replies are sent back as a single String or an Array of Strings.
+When dealing with hash values, there are a couple of useful exceptions to this.
 
 ### client.hgetall(hash)
 
@@ -512,7 +513,7 @@ Defaults to 1.7.  The default initial connection retry is 250, so the second ret
 
 ## TODO
 
-Better tests for monitor mode, auth, disconnect/reconnect, and all combinations thereof.
+Better tests for auth, disconnect/reconnect, and all combinations thereof.
 
 Stream large set/get values into and out of Redis.  Otherwise the entire value must be in node's memory.
 

index.js

@@ -471,6 +471,23 @@ function try_callback(callback, reply) {
     }
 }
 
+// hgetall converts its replies to an Object.  If the reply is empty, null is returned.
+function reply_to_object(reply) {
+    var obj = {}, j, jl, key, val;
+
+    if (reply.length === 0) {
+        return null;
+    }
+
+    for (j = 0, jl = reply.length; j < jl; j += 2) {
+        key = reply[j].toString();
+        val = reply[j + 1];
+        obj[key] = val;
+    }
+
+    return obj;
+}
+
 RedisClient.prototype.return_reply = function (reply) {
     var command_obj, obj, i, len, key, val, type, timestamp, argindex, args, queue_len;
     
@@ -489,15 +506,9 @@ RedisClient.prototype.return_reply = function (reply) {
 
     if (command_obj && !command_obj.sub_command) {
         if (typeof command_obj.callback === "function") {
-            // HGETALL special case replies with keyed Buffers
+            // TODO - confusing and error-prone that hgetall is special cased in two places
             if (reply && 'hgetall' === command_obj.command.toLowerCase()) {
-                obj = {};
+                reply = reply_to_object(reply);
-                for (i = 0, len = reply.length; i < len; i += 2) {
-                    key = reply[i].toString();
-                    val = reply[i + 1];
-                    obj[key] = val;
-                }
-                reply = obj;
             }
 
             try_callback(command_obj.callback, reply);
@@ -901,15 +912,9 @@ Multi.prototype.exec = function (callback) {
                 reply = replies[i - 1];
                 args = self.queue[i];
 
-                // Convert HGETALL reply to object
+                // TODO - confusing and error-prone that hgetall is special cased in two places
                 if (reply && args[0].toLowerCase() === "hgetall") {
-                    obj = {};
+                    replies[i - 1] = reply = reply_to_object(reply);
-                    for (j = 0, jl = reply.length; j < jl; j += 2) {
-                        key = reply[j].toString();
-                        val = reply[j + 1];
-                        obj[key] = val;
-                    }
-                    replies[i - 1] = reply = obj;
                 }
 
                 if (typeof args[args.length - 1] === "function") {

test.js

@@ -670,9 +670,9 @@ tests.HGETALL = function () {
 tests.HGETALL_NULL = function () {
     var name = "HGETALL_NULL";
 
-    client.hgetall('missing', function (err, obj) {
+    client.hgetall("missing", function (err, obj) {
         assert.strictEqual(null, err);
-        assert.deepEqual([], obj);
+        assert.strictEqual(null, obj);
         next(name);
     });
 };