faeldt / redis-mock

Node.js redis client mock
112 stars 157 forks source link

hgetall callback has empty object as reply when called with a non-existent key #8

Closed snapfractalpop closed 11 years ago

snapfractalpop commented 11 years ago

The hgetall function should have a null reply object when called with a non-existent key. This is the case in the real version of the redis module (by mranney). In this mock version, the reply is {} for non-existent keys, which is "truthy". As a result, this breaks some tests.

lahabana commented 11 years ago

`Are you sure about this? I have just tried this code:

    var redis = require('node-redis');

    var r = redis.createClient("","","");

    r.hgetall("hello", function(err, result) {

      console.log(result);

    });

with the latest version of node-redis (0.1.6) and it outputs: []

This is also coherent with the redis doc:

redis> HGETALL foo
(empty list or set)
snapfractalpop commented 11 years ago

With redis v.0.8.3 I am getting null, with redis-mock v.0.1.6 I am getting {}. Even if I were to get an empty array, as you have, I believe it would still be broken (since it is not mocking the actual current behavior of redis). I believe is related to some functionality that was changed 2 years ago:

https://github.com/mranney/node_redis/issues/67

As an aside, I am wondering whether the conversation in the above thread continued elsewhere. It would be good to know whether we should be relying on the truthiness of arguments in the callback function. For example:

client.get("someKey", function (error, reply) {
  if (!error && reply) {  // can we trust the truthiness here?
    // do stuff with reply
   }
});
lahabana commented 11 years ago

Yes indeed it is my bad I was using the wrong package. I get confused quite often between npm packages and git repos. (the fact that the repo is called node_redis and the npm package redis doesn't help).

I get the same result as you. I'll send a patch for this in a bit.

For the truthfulness IMHO it seems node_redis tries to respect this, however this makes the replies different to what Redis should actually return (though I haven't developed node_redis at all). I personally use real checks and don't use truthfulness (reply === null is not this longer).

snapfractalpop commented 11 years ago

Thanks! Oops, I didn't mean to close it.. I pressed close by accident. But, I will close after I test the fix.

faeldt commented 11 years ago

Thanks guys. I'll publish to npm once this is closed and everyone is pleased.

snapfractalpop commented 11 years ago

Thanks everyone, especially for the fast turn-around. This solves the issue.

DanielTaika commented 2 years ago

Did this issue arise from the dead? I see {} when expecting null [Redis 7.x, NodeJS 16, Linux]

kerimembel commented 2 years ago

I was able check if it is null or not like this;

`redisClient.hGetAll('orders').then((data) => {

if(Object.keys(data).length) {
  console.log("Not empty");
} else {
  console.log("empty");
}

})`