hmget breaks on null values if used with detect_buffers #344

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

shin- commented Dec 12, 2012

Hi!

The following code snippet reproduces the problem:

var redis = require('redis');

var clt = redis.createClient(6379, 'localhost', { detect_buffers : true });

clt.del('foo_129000', function(err) {
    if (err) throw err;
    clt.hmget('foo_129000', 'bar', 'baz', function(err, arr) {
        if (err) throw err;
        console.log(arr);
    });
});

The pull request patches reply_to_strings() to avoid calling toString() on null/undefined values

Ready to merge checklist

  • test(s) in test.js
  • tests will fail without the PR, but succeed once applied
  • docs, if applicable
  • merges cleanly
  • coding style (4-space indents, looks similar to other code)

cblage commented on 1c70232 Jan 17, 2013

I think your condition is too broad as it will make toString not be called on 0 and false. I think the below code would be a better solution:

            if (null !== reply[i]) {
                reply[i] = reply[i].toString();
            }
Contributor

shin- commented Jan 17, 2013

Thanks @cblage, you're right - Added another commit to check for both null and undefined values, but only these.

Contributor

DTrejo commented Feb 18, 2013

Could you add the repro code to tests.js?
Thank you,
D

index.js
@@ -551,7 +551,9 @@ function reply_to_strings(reply) {
if (Array.isArray(reply)) {
for (i = 0; i < reply.length; i++) {
- reply[i] = reply[i].toString();
+ if (reply[i] !== null && reply[i] !== undefined) {
+ reply[i] = reply[i].toString();
@DTrejo

DTrejo Feb 18, 2013

Contributor

you have an extra space after the equal sign, watch out!

brycebaril added a commit that referenced this pull request Mar 16, 2013

Owner

brycebaril commented Mar 16, 2013

Thank you! Merged into master and will be on the next npm release.

@brycebaril brycebaril closed this Mar 16, 2013

mciparelli added a commit to mciparelli/node_redis that referenced this pull request Mar 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment