hapijs / hapi

The Simple, Secure Framework Developers Trust
https://hapi.dev
Other
14.61k stars 1.34k forks source link

hapi 13.5.1 (and 13.5.2) break if routes still use joi 8.x #3271

Closed rluba closed 8 years ago

rluba commented 8 years ago

I suddenly got lots of test timeouts when upgrading from hapi@13.5.0 to 13.5.1 (or 13.5.2). Looking at hapi’s diff, I noticed that it now depends on joi@9.x.

Using joi@8.x validation objects on routes with hapi@13.5.1 causes all malformed requests that previously returned 400 errors to suddenly time out instead of returning any response. So if you declare your dependencies using the default NPM semver ranges, you got yourself a broken app as soon as 13.5.1 was released:

  "dependencies": {
    "hapi": "^13.5.0",
    "joi": "^8.0.0"
…
  }

Upgrading the root dependency of joi solves the problem and all malformed requests return 400 with validation errors again:

  "dependencies": {
    "hapi": "^13.5.0",
    "joi": "^9.0.0"
…
  }

Therefore I’d consider the 13.5.1 changeset to be a breaking change => should be a major release.

akloeber commented 8 years ago

I'm also getting test timeouts since v13.5.2 while v13.5.0 was working fine. Furthermore the following stacktrace is logged:

Debug: internal, implementation, error
    TypeError: child.schema._getLabel is not a function
    at _base (/home/runner/project/node_modules/hapi/node_modules/joi/lib/object.js:157:93)
    at _validate (/home/runner/project/node_modules/hapi/node_modules/joi/lib/any.js:540:37)
    at _validateWithOptions (/home/runner/project/node_modules/hapi/node_modules/joi/lib/any.js:600:29)
    at root.validate (/home/runner/project/node_modules/hapi/node_modules/joi/lib/index.js:105:23)
    at Object.internals.input.postValidate [as input] (/home/runner/project/node_modules/hapi/lib/validation.js:137:20)
    at exports.payload (/home/runner/project/node_modules/hapi/lib/validation.js:42:22)
    at each (/home/runner/project/node_modules/hapi/lib/request.js:383:16)
    at iterate (/home/runner/project/node_modules/items/lib/index.js:36:13)
    at done (/home/runner/project/node_modules/items/lib/index.js:28:25)
    at internals.Auth.test.internals.Auth.payload.finalize (/home/runner/project/node_modules/hapi/lib/auth.js:223:16)
    at each (/home/runner/project/node_modules/hapi/lib/request.js:383:16)
    at iterate (/home/runner/project/node_modules/items/lib/index.js:36:13)
    at done (/home/runner/project/node_modules/items/lib/index.js:28:25)
    at onParsed (/home/runner/project/node_modules/hapi/lib/route.js:396:20)
    at /home/runner/project/node_modules/hapi/lib/route.js:417:20
    at next (/home/runner/project/node_modules/subtext/lib/index.js:43:16)
    at /home/runner/project/node_modules/subtext/lib/index.js:164:20
    at Object.internals.Parser.internals.Parser.parse.decoder.once.writeFile.internals.Parser.raw.decoder.once.internals.jsonParse (/home/runner/project/node_modules/subtext/lib/index.js:281:12)
    at Object.internals.Parser.internals.Parser.parse.decoder.once.writeFile.internals.Parser.raw.decoder.once.internals.object (/home/runner/project/node_modules/subtext/lib/index.js:254:26)
    at /home/runner/project/node_modules/subtext/lib/index.js:156:19
    at finish (/home/runner/project/node_modules/wreck/lib/index.js:328:20)
    at wrapped (/home/runner/project/node_modules/hoek/lib/index.js:871:20)
    at onReaderFinish (/home/runner/project/node_modules/wreck/lib/index.js:399:16)
    at g (events.js:260:16)
    at emitNone (events.js:72:20)
    at emit (events.js:166:7)
    at finishMaybe (_stream_writable.js:481:14)
    at afterWrite (_stream_writable.js:355:3)
    at nextTickCallbackWithManyArgs (node.js:464:18)
    at process._tickDomainCallback (node.js:403:17)

I also have a direct dependency to joi v8.4.2.

Environment: node 4.4.7 npm 3.8.3 hapi 13.5.2 joi 8.4.2

Marsup commented 8 years ago

In the meantime, just put Joi.object around your auto-built schemas, it should work.

EDIT: or even Joi.compile, at least you'd compile your schema with the correct version of joi.

AdriVanHoudt commented 8 years ago

Or update to Joi 9 if possible

We talked about this a bit on Gitter but there is no silver bullet on solving this I'm afraid.

I still propose adding it as a peer dep to at least get some warning (yes I know not everyone reads them but it is something) Releasing as a major would also help minimize the risk but it would still be able to break so yeah

hueniverse commented 8 years ago

v14 published and the previous two minor releases removed.

rluba commented 8 years ago

Great. But reverting the changes in 13.5.1 and 13.5.2 as a new 13.5.3 release would break less stuff than removing published releases. (Or is that what you meant, @hueniverse ?)

hueniverse commented 8 years ago

Sure. 13.5.3 published with a copy of 13.5.0