Test route handler rejection with non-Error #441

sebdeckers · 2017-11-07T15:43:31Z

I noticed that Fastify does type checking (err instanceof Error) when a promise is rejected (async function throws). This is problematic when the thing thrown does not actually inherit from Error.

It's not entirely clear to me how to address this.

Expose a Reply.prototype.error() method
Add a parameter or option to Reply.prototype.send() to mark the payload as error
Replace the non-Error err with a new Error() instance (losing the original object)
Set a private (Symbol) property to mark the payload as an error.

Suggestions?

sebdeckers · 2017-11-07T16:00:48Z

PS: Is it possible to disable the pre-commit hook? I find it a bit annoying to wait ~20s on every commit, and to be unable to commit a failing test (TDD).

StarpTech · 2017-11-07T16:13:01Z

Due to spec we have to support it but we should log a warn message like in bluebird so +1 for this PR.

StarpTech · 2017-11-07T16:14:18Z

@sebdeckers you can do git commit --no-verify or git commit -n

jsumners · 2017-11-07T21:20:53Z

It seems wrong to me to throw something that isn't in the Error prototype chain. What is the use case for doing so?

mcollina · 2017-11-08T09:41:24Z

@jsumners it is odd, but the spec supports that, and such an error can bubble up from the prototype chain.

@sebdeckers good catch, would you like to try a fix? We should just change the function
passed to catch in https://github.com/fastify/fastify/blob/master/lib/reply.js#L68.

sebdeckers · 2017-11-08T09:52:19Z

@jsumners I encountered this as some library threw an "error" object that had the Error interface but was not an instanceof Error.

Another use case would be error objects passed between workers in a cluster, where they are serialised as JSON. Even if the prototype chain were somehow preserved, they would be instances of different Error constructors (this is a common scenario in cross-window browser contexts more than in Node.js).

@mcollina 🤔 Ooh, interesting. Thanks for the hint!

sebdeckers · 2017-11-08T11:13:05Z

Upon closer inspection this had already been solved in wrapReplyEnd so I have copied that solution to the preHandlerCallback promise rejection handler.

StarpTech · 2017-11-08T11:22:35Z

@sebdeckers this looks really wrong to me to pass any value as the first argument to the Error object. The payload should be passed one-to-one be aware of this.

Otherwise, all validation plugins only can respond to a string.

sebdeckers · 2017-11-08T11:51:37Z

@StarpTech If it's an error the payload is passed one-to-one. If it's not an error, we eventually cast it to a string anyway. (Note this is the same approach as taken in the existing code just below.)

mcollina · 2017-11-08T11:51:51Z

lib/handleRequest.js

+      if (err instanceof Error) {
+        this.reply.send(err)
+      } else {
+        this.reply.send(new Error(err || ''))


I'm not not very happy about this part, can you do a check if err is a string and use that with new Error, or log the object with a warning?

Is there a reason why it must be a string? I think the Error constructor does this already. See: https://tc39.github.io/ecma262/#sec-error-message

FYI the error-like objects that I was getting from my app's dependency actually have a toString method so this fix works.

I want to avoid:

> new Error({ hello: 'world' }) Error: [object Object] at repl:1:1 at ContextifyScript.Script.runInThisContext (vm.js:50:33) at REPLServer.defaultEval (repl.js:240:29) at bound (domain.js:301:14) at REPLServer.runBound [as eval] (domain.js:314:12) at REPLServer.onLine (repl.js:441:10) at emitOne (events.js:121:20) at REPLServer.emit (events.js:211:7) at REPLServer.Interface._onLine (readline.js:282:10) at REPLServer.Interface._line (readline.js:631:8)

We can check if the object has a toString method that is not Object.prototype.toString.

Actually I feel like this type checking and coercion is premature here. It should take place after handleError (and any reply.context.errorHandler hooks) have processed the raw error.

@sebdeckers all of what you have mentioned assumes an error is an Error. Refactoring that would be a major undertaking, and I do not think it is worth it to support this edge case.

StarpTech · 2017-11-08T11:59:00Z

@sebdeckers we shouldn't cast anything since any value can be thrown as an error.

sebdeckers · 2017-11-08T12:23:14Z

@StarpTech Agreed; will undo this commit. But I don't see how better to solve this problem. Suggestions?

mcollina · 2017-11-08T12:26:39Z

@StarpTech I think the solution in #441 (comment) could get it done?

IMHO we have two issues, when a non-error is catched:

return 500
return the proper error message from toString()

StarpTech · 2017-11-08T12:38:25Z

@mcollina I think about to handle that very neutral. When the developer passes an error it's fine but when not we should just pass it as it is. It doesn't look like a responsibility of fastify to enforce that interface (since any value is valid)

function wrapReplyEnd (context, req, res, statusCode, payload) {
  const reply = new context.Reply(req, res, context)
  if (payload instanceof Error) {
    reply.code(statusCode).send(payload)
  } else {
    reply.code(statusCode).send(payload) // fastify can not know what it is so let's the developer decide
  }
  return
}

mcollina · 2017-11-08T12:42:00Z

@StarpTech So you propose to do:

    }).catch((err) => {
      if (err instanceof Error) {
        this.reply.send(err)
      } else {
        this.reply.code(500).send(err)
      }
    })

StarpTech · 2017-11-08T12:42:57Z

@mcollina yes!

StarpTech · 2017-11-08T12:45:54Z

@mcollina we have to mark this payload as an error even when it is not from type Error this need some refactoring.

sebdeckers · 2017-11-08T14:09:05Z

@StarpTech I've done another attempt. This is using a private flag on the reply. Thoughts?

Notes:

The message is not yet being set. It's a separate question: What should the message be for each case?
The setErrorHandler callback gets the raw object but I struggled to get the async tests to work properly.

delvedor · 2017-11-08T14:54:54Z

lib/reply.js

@@ -10,6 +10,8 @@ const flatstr = require('flatstr')
 const fastseries = require('fastseries')
 const runHooks = fastseries()

+const isError = Symbol.for('is-error')


You should use just Symbol for performances reasons. Since every symbol is different you must declare it in one file and export it.
@mcollina thoughts?

there are no performance reasons to do that, more keeping it private. I agree.

sebdeckers · 2017-11-08T18:29:46Z

@delvedor @mcollina I've changed it to a Symbol that is expoted by reply.js.

Now the message property on the JSON response is omitted if err is not an instance of Error. This seems safer than potentially leaking unintended data. (Open to other suggestions though: String(err), JSON.stringify(err), ...)

mcollina · 2017-11-09T18:27:03Z

lib/reply.js

@@ -59,7 +61,7 @@ Reply.prototype.send = function (payload) {
    return
  }

-  if (payload instanceof Error) {
+  if (payload instanceof Error || this[isError] === true) {


can you please do the this[isError] check only once and store it in a variable?

Thanks, good catch. Done in 67e0ed5

mcollina

LGTM

StarpTech · 2017-11-09T18:55:13Z

LGTM

allevo

Changing reply hidden class could be a problem. Another problem is on using Simbol for checking isError.

Maybe it is better to add isError property to Reply class (false as default)

mcollina · 2017-11-09T23:11:41Z

@StarpTech you just need to add this[isError] = false in the Reply constructor.

sebdeckers · 2017-11-09T23:54:06Z

@mcollina @allevo 🤷🏻‍♂️ I'm getting confused here. Which do you prefer?

reply.isError (public) -> this needs to be documented/tested at least as much as reply.sent
reply[isError] (private, Symbol)
reply._isError (private, prefix)
other...?

Your wish is my command.

delvedor · 2017-11-10T06:59:14Z

lib/reply.js

@@ -203,7 +208,7 @@ function handleError (reply, err, cb) {

  cb(reply, Object.assign({
    error: statusCodes[reply.res.statusCode + ''],
-    message: err.message,
+    message: (err || undefined) && err.message,


Maybe I'm missing something here, why err || undefined?

Since err can be any value this ensures any falsy evaluation of err sets the message property to undefined. That way it will not be output into the JSON response. Otherwise the falsy value of err (e.g. null, false, 0, '') would be sent.

I think the message property should be always there.
If you don't have a value to write, just use an empty string.

@delvedor I've added a few extra test cases to demonstrate the behaviour in 6a69afa. Notice that it expects the message property of the response to not exist.

@delvedor Sorry, didn't see your second comment in time. I've updated (34ffb0d) to output an empty string as the default error message.

mcollina · 2017-11-11T08:51:16Z

lib/reply.js

@@ -10,6 +10,8 @@ const flatstr = require('flatstr')
 const fastseries = require('fastseries')
 const runHooks = fastseries()

+const isError = Symbol('is-error')
+
 function Reply (req, res, context, request) {
  this.res = res
  this.context = context


can you set this[isError] = false here?

Okay, I can do that. Was afraid that extra statement would add some performance cost.

I guess it also needs to be added to buildReply?

Yes of course.

mcollina · 2017-11-12T22:35:49Z

Perfect... @allevo @delvedor can you have another look?

allevo

LGTM Only a few nits.

allevo · 2017-11-12T22:47:13Z

test/reply-error.test.js

+        )
+      })
+
+      fastify.setErrorHandler((err, reply) => {


Could you move this before request injection?

allevo · 2017-11-12T22:48:14Z

test/reply-error.test.js

+
+      fastify.setErrorHandler((err, reply) => {
+        t.strictEqual(err, nonErr)
+        t.end()


The end of the test should be in the injection callback, not here

This handler fires last. Not sure how I can end the test before it runs?

How could it be possible? The inject callback has to be called after the setErrorHadler. @mcollina

@allevo 😓 Upon closer inspection, it appears I still misunderstood tap's parallel subtests. I'll refactor again. WIP.

allevo · 2017-11-12T22:49:31Z

test/reply-error.test.js

+      })
+    })
+  }
+  t.end()


Could you use t.plan(objs.length) instead?

mcollina · 2017-11-14T10:15:02Z

Ping, I would love to get this out asap.

delvedor

LGTM

sebdeckers · 2017-11-14T12:59:48Z

That's weird... tests are passing locally. Same Node 9.1.0

...
test/reply-error.test.js .......................... 168/168
test/route-prefix.test.js ........................... 10/10
test/route.test.js .................................. 18/18
test/routes.test.js ................................. 22/22
test/stream.test.js ................................... 9/9
test/throw.test.js .................................. 13/13
(node:11252) ExperimentalWarning: The http2 module is an experimental API.
test/http2/http2.test.js ............................ 24/24
test/https/https.test.js .............................. 7/7
test/internals/all.test.js ............................ 7/7
test/internals/decorator.test.js ...................... 9/9
test/internals/handleRequest.test.js ................ 17/17
test/internals/hooks.test.js ........................ 16/16
test/internals/logger.test.js ....................... 26/26
test/internals/reply.test.js ........................ 35/35
test/internals/validation.test.js ................... 16/16
total ........................................... 1506/1506

  1506 passing (15s)

  ok

mcollina · 2017-11-14T14:16:41Z

The test fails in the exact same way on my machine.

sebdeckers · 2017-11-14T15:15:07Z

@mcollina Sorry about that. Forgot to rebase. 🙇🏼‍♂️

mcollina · 2017-11-14T16:04:15Z

@allevo are you ok with this? Seems ready to me.

mcollina · 2017-11-15T11:46:44Z

@allevo the nits have been addressed, I'm landing.

github-actions · 2022-02-23T00:19:39Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mcollina reviewed Nov 8, 2017

View reviewed changes

sebdeckers force-pushed the master branch from f18b396 to 947098d Compare November 8, 2017 14:05

delvedor requested changes Nov 8, 2017

View reviewed changes

delvedor added the enhancement label Nov 9, 2017

mcollina reviewed Nov 9, 2017

View reviewed changes

mcollina approved these changes Nov 9, 2017

View reviewed changes

allevo suggested changes Nov 9, 2017

View reviewed changes

delvedor requested changes Nov 10, 2017

View reviewed changes

mcollina reviewed Nov 11, 2017

View reviewed changes

allevo suggested changes Nov 12, 2017

View reviewed changes

delvedor approved these changes Nov 14, 2017

View reviewed changes

sebdeckers added 9 commits November 14, 2017 22:56

Test route handler rejection with non-Error

9a2c90f

set a flag on the reply to mark as error (WIP)

ea2e475

omit message if error is not instanceof Error

05e16aa

use a variable to cache property access

11de1bf

falsy error rejection test cases

4f78021

default error message is empty string

6ce4b4b

default the isError hidden flag to false

eda4203

reply error test plan & ordering

fc58466

bumped plan counter to expect another assertion

90fe14c

sebdeckers force-pushed the master branch from c939665 to 90fe14c Compare November 14, 2017 15:04

mcollina merged commit 49bdad9 into fastify:master Nov 15, 2017

github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2022

Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023

Test route handler rejection with non-Error #441

Test route handler rejection with non-Error #441

Conversation

sebdeckers commented Nov 7, 2017

sebdeckers commented Nov 7, 2017

StarpTech commented Nov 7, 2017 • edited

StarpTech commented Nov 7, 2017 • edited

jsumners commented Nov 7, 2017

mcollina commented Nov 8, 2017

sebdeckers commented Nov 8, 2017 • edited

sebdeckers commented Nov 8, 2017

StarpTech commented Nov 8, 2017 • edited

sebdeckers commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarpTech commented Nov 8, 2017

sebdeckers commented Nov 8, 2017

mcollina commented Nov 8, 2017

StarpTech commented Nov 8, 2017 • edited

mcollina commented Nov 8, 2017

StarpTech commented Nov 8, 2017

StarpTech commented Nov 8, 2017 • edited

sebdeckers commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebdeckers commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

StarpTech commented Nov 9, 2017

allevo left a comment

Choose a reason for hiding this comment

mcollina commented Nov 9, 2017

sebdeckers commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Nov 12, 2017

allevo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Nov 14, 2017

delvedor left a comment

Choose a reason for hiding this comment

sebdeckers commented Nov 14, 2017

mcollina commented Nov 14, 2017

sebdeckers commented Nov 14, 2017

mcollina commented Nov 14, 2017

mcollina commented Nov 15, 2017

github-actions bot commented Feb 23, 2022

StarpTech commented Nov 7, 2017 •

edited

StarpTech commented Nov 7, 2017 •

edited

sebdeckers commented Nov 8, 2017 •

edited

StarpTech commented Nov 8, 2017 •

edited

StarpTech commented Nov 8, 2017 •

edited

StarpTech commented Nov 8, 2017 •

edited