Exception from hook not handled

[thomasthiebaud]

I would like to validate my request (get, post, update or delete), and if the request is not valid, I would like to return an error (status 400 or 500 depending on the error). For the moment I'm using hooks for this task like this

const fortune = require('fortune');
const status = require('./model').status;
const { errors: { BadRequestError } } = fortune;

function input(context, record, update) {
  switch (context.request.method) {
    case 'create':
      if (status.indexOf(record.status) === -1) {
        throw new BadRequestError('Invalid status');
      } else {
        return record;
      }
    case 'update':
      if (!update || !update.replace || !update.replace.status) {
        return update;
      }

      if (status.indexOf(update.replace.status) === -1) {
        throw new BadRequestError('Invalid status');
      } else {
        return update;
      }
    default: return null;
  }
}

The important line here is throw new BadRequestError('Invalid status');. This line makes fortune returning a 400 error with invalid status as a content.

Even if I get the desire response, it seems that the error is not fully catch, and so I also get the error in the catch of the listener.

app.use((req, res) => createListener(req, res).catch(err => logger.error(err)));

So here are my questions:

• Is it a desire behaviour ?
• Is it the correct way to validate an entry in the hook ?
• Is it possible to handle exceptions from hooks into fortunejs ?

[gr0uch]

It's intended behavior that the error appears again from the listener function. Internally the error gets caught and thrown again. This makes it possible to correlate the Node.js HTTP request with the error handled by Fortune.js.

That looks like how errors are meant to be handled. After throwing an error there's nothing more that can be done, what Fortune.js will do is immediately end the transaction if there is one and reject the request.

[thomasthiebaud]

Thank you very much for your answer

[thomasthiebaud]

One more thing, even if I catch the error on this line

app.use((req, res) => createListener(req, res).catch(err => /*Do something here*/));

I can still see

(node:20623) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): BadRequestError: Invalid status

in the logs. Also if I replace this line

app.use((req, res) => createListener(req, res).catch(err => logger.error(err)));

by this one

app.use(createListener);

I can see in the logs the error twice.

(node:20694) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): BadRequestError: Invalid status
(node:20694) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): BadRequestError: Invalid status

[gr0uch]

That would probably be a bug somewhere in your application code. The tests for fortune-http work fine and don't emit warnings. Here's the server from the test:

const server = http.createServer((request, response) =>
  listener(request, response)
  .catch(error => stderr.error(error)))

Is it possible that the framework you're using for app accepts a return value from use and does something with it?

[thomasthiebaud]

I'm using express. I setup it using the example here

const http = require('fortune-http');

const store = fortune({
  // Put models here
}, {
  hooks: {
    customer: [customerHooks.input],
    rule: [ruleHooks.input],
    user: [userHooks.input, userHooks.output],
  },
  adapter,
});

const api = http(store, {
  serializers: [
    [jsonApiSerializer],
  ],
});

const app = express();

/*
 * Use different middleware 
 * app.use(...);
 * app.use(...);
 */

app.use((req, res) => api(req, res).catch(err => logger.error(err)));

[gr0uch]

Hmm, maybe you could try two things:

• Use http.createServer directly just to see if the warning still occurs.
• Wrap the arrow function with brackets to avoid returning the promise to use, i.e. app.use((req, res) => { ... })

[thomasthiebaud]

The problem remains the same. I tried :

const http = require('http');
const app = http.createServer((request, response) =>
  api(request, response)
  // Make sure to catch Promise rejections.
    .catch(error => {
      console.error(error.stack)
    }))

and I get

(node:30556) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): BadRequestError: Invalid status
BadRequestError: Invalid status
    at /home/thomasthiebaud/Combain/mate-v2-server/node_modules/fortune/lib/dispatch/update.js:100:13
    at process._tickCallback (internal/process/next_tick.js:103:7)

[gr0uch]

I can't reproduce the warning you're getting, throwing errors is working fine. Can you tell me which version you're using?

[thomasthiebaud]

I think I'm using the latest version

"fortune": "5.0.2",
"fortune-http": "1.0.3",
"fortune-json-api": "2.1.0",
"fortune-postgres": "1.4.7",

[kketch]

@thomasthiebaud I tried to reproduce your issue with a similar code and did not get the uncaught exception, with node v7.2.0 : https://gist.github.com/kketch/b7a84bb830bd071e672fbe1c52cfd071

And your stack trace seems weird: https://github.com/fortunejs/fortune/blob/5.0.2/lib/dispatch/update.js#L100

As you can see this line do not throw a BadRequestError with a "Invalid status" message. Is it your complete stack trace ? this error seems to be thrown in the application code you shared earlier in the issue, we only see fortune

[thomasthiebaud]

You are right, the stack trace is strange. I will try to investigate on this strange stack trace next week. Thank you very much for your feedbacks.

[cooperka]

@thomasthiebaud did you ever find the cause of the warnings? I'm getting the same thing:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): BadRequestError: The value of "state" is invalid, it must be a "RequestState".

with very similar code as you (using http instead of express). The original error is being thrown by Fortune in this case during type validation; I don't expect I should need to handle that sort of thing.

The warnings are also reproducible with https://github.com/redoPop/fortunejs-example/.

---

Debug notes:

Trace when using --trace-warnings:

BadRequestError
    at checkValue (/home/demo/node_modules/fortune/lib/record_type/enforce.js:128:28)
    at enforce (/home/demo/node_modules/fortune/lib/record_type/enforce.js:77:12)
    at /home/demo/node_modules/fortune/lib/dispatch/create.js:86:9
    at map (/home/demo/node_modules/fortune/lib/common/array/map.js:14:14)
    at /home/demo/node_modules/fortune/lib/dispatch/create.js:81:26
    at <anonymous>

If I comment out the re-throw in lib/dispatch/index.js, the warning changes slightly:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: Cannot read property 'payload' of undefined
    at JsonApiSerializer.processResponse (/home/demo/node_modules/fortune-json-api/lib/index.js:100:36)
    at /home/demo/node_modules/fortune-http/lib/index.js:357:12
    at <anonymous>

[kketch]

Hi @cooperka, what should I do to reproduce exactly ? (with your example repository)

[gr0uch]

OK, I'll do a more thorough investigation then, the interesting part is TypeError: Cannot read property 'payload' of undefined.

[cooperka]

@kketch with that example repo, simply start the server and hit it with any invalid request that causes some sort of error to be thrown and you'll see the warnings in your console.

@daliwali to add more info, it looks like the follow-up warning is thrown in fortune-json-api. I've added the full stack above. It may not be relevant since the error is supposed to have been re-thrown; more just useful to know that dispatch/index.js may be where the unhandled error is originating from.

[gr0uch]

I found the problem: the listener returns a promise that doesn't get a catch. The readme in fortune-http states:

const server = http.createServer((request, response) =>
  listener(request, response)
  // Make sure to catch Promise rejections.
  .catch(error => {
    console.error(error.stack)
  }))

I'm not sure what to do about it. I think it's somewhat unexpected for there to be a return value from the listener, I guess I could remove it but it would be a breaking change. Or I can make sure that this listener never throws.

Though I would be in favor of removing it and bumping major version, not sure what I was thinking with this, it makes dealing with HTTP-specific requests easier but it's kind of unexpected.

[cooperka]

Hmm, in my opinion it makes sense as-is, no changes to the libraries needed -- I updated my server code to match your example above and it works great!

If you're good with it, I'm happy to make a PR to that example app to encourage others to follow the same pattern. I see now that this pattern is already in an example in the official guide, I just hadn't noticed the difference and blindly followed the boilerplate app. Thank you for your help!