memcached icon indicating copy to clipboard operation
memcached copied to clipboard

add if (!value.hasOwnProperty(vKey)) continue; that occurred bugs!

Open XadillaX opened this issue 9 years ago • 5 comments

Before patching this patch, some functions attached to the object will be parsed and thrown errors!

For an example:

Array.prototype.foo = function() {};
var a = [];
for(var key in a) {
    console.log(key);
}

The result will be abc - this is not my expected result!

In your for ... in of util.js:

case Array:
  if (toString.call(value) !== '[object Array]') {
    err = 'Argument "' + key + '" is not a valid Array.';
  }
  if (!err && key === 'key') {
    for (var vKey in value) {
      ...

We should ignore abc this extended function.

XadillaX avatar Mar 02 '15 09:03 XadillaX

Some minor comments but LGTM.

3rd-Eden avatar Mar 06 '15 22:03 3rd-Eden

@ronkorving @3rd-Eden So you both prefer to use Object.keys instead of for ... in?

XadillaX avatar Mar 09 '15 01:03 XadillaX

I found another problem when I use Object.keys():

If the object is undefined, for(var i in undefined) will be ran as an empty object bug Object.keys(undefined) will throw an error!

This error makes mocha failed:

     TypeError: Cannot convert undefined or null to object
      at Function.keys (native)
      at Object.merge (/Users/XadillaX/code/node/node-memcached/lib/utils.js:113:10)
      at new Client (/Users/XadillaX/code/node/node-memcached/lib/memcached.js:58:9)
      at Context.<anonymous> (/Users/XadillaX/code/node/node-memcached/test/memcached-touch.test.js:21:21)
      at Test.Runnable.run (/usr/local/lib/node_modules/mocha/lib/runnable.js:217:15)
      at Runner.runTest (/usr/local/lib/node_modules/mocha/lib/runner.js:373:10)
      at /usr/local/lib/node_modules/mocha/lib/runner.js:451:12
      at next (/usr/local/lib/node_modules/mocha/lib/runner.js:298:14)
      at /usr/local/lib/node_modules/mocha/lib/runner.js:308:7
      at next (/usr/local/lib/node_modules/mocha/lib/runner.js:246:23)
      at Immediate._onImmediate (/usr/local/lib/node_modules/mocha/lib/runner.js:275:5)
      at processImmediate [as _immediateCallback] (timers.js:321:17)

So I think we still should use for ... in, or we should make sure object is not null at first: if(object !== undefined && object !== null).

@3rd-Eden @ronkorving

XadillaX avatar Mar 09 '15 03:03 XadillaX

Correct, Object.keys breaks on non-objects. A simple truthy check would address that, right? That's my preferred style. But I suggest you go with whatever style @3rd-Eden prefers.

if (obj) {
  var keys = Object.keys(obj);
}

ronkorving avatar Mar 09 '15 09:03 ronkorving

But doing a for in on an undefined is just as bad as doing an Object.keys on undefined. I would argue that a thrown error would be better as you're simply not receiving what you're expecting as the only proper use-case for a for in loop would be to iterate over an object.

If the code fails with Object.key there is a bigger problem with the code somewhere as it shouldn't even iterate on it.

3rd-Eden avatar Mar 09 '15 09:03 3rd-Eden