Automattic / knox

S3 Lib
MIT License
1.74k stars 285 forks source link

canonicalizeResource Object.keys() breaks in node 0.11 #249

Closed tmarshall closed 9 years ago

tmarshall commented 10 years ago

My tests are working when using v0.10, but break in v0.11

TypeError: Object.keys called on non-object
    at Function.keys (native)
    at Object.exports.canonicalizeResource (/Users/mars/dev/node_modules/knox/lib/auth.js:210:10)
    at Client.request (/Users/mars/dev/node_modules/knox/lib/client.js:275:22)
    at Client.put (/Users/mars/dev/node_modules/knox/lib/client.js:326:15)
    at Client.putStream (/Users/mars/dev/node_modules/knox/lib/client.js:408:18)
    at /Users/mars/dev/api/v0/nodes/account.js:269:18
    at /Users/mars/dev/node_modules/async/lib/async.js:570:21
    at /Users/mars/dev/node_modules/async/lib/async.js:249:17
    at /Users/mars/dev/node_modules/async/lib/async.js:125:13
    at Array.forEach (native)

I logged url in /lib/auth.js's canonicalizeResource()

0.10:

{ protocol: null,
  slashes: null,
  auth: null,
  host: null,
  port: null,
  hostname: null,
  hash: null,
  search: '',
  query: {},
  pathname: '/dev/ifL1T/-JY6tjvunPCwVBdifL1T.png',
  path: '/dev/ifL1T/-JY6tjvunPCwVBdifL1T.png',
  href: '/dev/ifL1T/-JY6tjvunPCwVBdifL1T.png' }

0.11:

{ protocol: null,
  slashes: null,
  auth: null,
  host: null,
  port: null,
  hostname: null,
  hash: null,
  search: null,
  query: null,
  pathname: '/dev/ifL1T/-JY6tjvunPCwVBdifL1T.png',
  path: '/dev/ifL1T/-JY6tjvunPCwVBdifL1T.png',
  href: '/dev/ifL1T/-JY6tjvunPCwVBdifL1T.png' }

Object.keys(url.query) expects {} at minimum

kilianc commented 10 years ago

+1 this started happening on node v0.11.14 works just fine on v0.11.13

kilianc commented 10 years ago

lib/auth.js#L208

fixes it

kilianc commented 10 years ago

here for everyone else

$ npm install --save http://github.com/kilianc/knox/tarball/features/v0.11.14
pgherveou commented 9 years ago

Is there any chance this bug fix can be pushed to the master branch?

domenic commented 9 years ago

No, don't use unstable Node.

kilianc commented 9 years ago

@domenic that's a strong statement, while not supporting unstable node in third party libraries is ok, I think that as community members we are doing everyone a favor discovering new bugs and testing new versions of node.

domenic commented 9 years ago

Yep, and the bug was discovered in Node and fixed, so no need to change Knox.

pgherveou commented 9 years ago

Great thanks for your feedbacks guys

On Thu, Dec 4, 2014 at 7:00 AM, Domenic Denicola notifications@github.com wrote:

Yep, and the bug was discovered in Node and fixed, so no need to change Knox.

— Reply to this email directly or view it on GitHub https://github.com/LearnBoost/knox/issues/249#issuecomment-65643684.

PG

ELLIOTTCABLE commented 9 years ago

Do you have a reference to the bug on Node itself? I'm seeing similar behaviour elsewhere.