failAction: detailedError to log, defaultError to response

[mahnunchik]

Support plan

• which support plan is this issue covered by? (e.g. Community, Core, Plus, or Enterprise): Community, Core
• is this issue currently blocking your project? (yes/no): yes
• is this issue affecting a production system? (yes/no): no

Context

• node version: v12 v14
• module version: 20.0.3
• environment (e.g. node, browser, native): node
• used with (e.g. hapi application, another framework, standalone, ...): hapi app
• any other relevant information: original issue failAction: detailedError to log, defaultError to response #4040

What problem are you trying to solve?

It is impossible to log detailedError to the console, defaultError to the response.

Current implementation allows:

failAction: 'error'
// defaultError to log and to response

failAction: (request, h, err) => { request.log('error', err); throw err }
// detailedError to log and to response

There is no way to access both detailedError and defaultError in the handler.

For example:

• defaultError: Error: Invalid request query input
• detailedError: Error [ValidationError]: "foobar" is not allowed

Do you have a new or modified API suggestion to solve the problem?

https://github.com/hapijs/hapi/blob/master/lib/toolkit.js#L117-L135

if (failAction === 'log-and-error') {
    request._log(options.tags, options.details || err);
    throw err;
}

[mahnunchik]

import Joi from 'joi'; // 17.4.0

server.route({
  method: 'GET',
  path: '/test',
  options: {
    validate: {
      query: Joi.object({
        foo: Joi.string().required(),
      }),
      failAction: 'error',
      // response: {"statusCode":400,"error":"Bad Request","message":"Invalid request query input"}
      failAction: 'log',
      // log to console: Error: Invalid request query input
      failAction(request, h, err) {
        request.log('error', err);
        throw err;
      },
      // log to console: Error [ValidationError]: "foo" is not allowed to be empty
      // response: {"statusCode":400,"error":"Bad Request","message":"\"foo\" is not allowed to be empty","validation":{"source":"query","keys":["foo"]}}
    },
  },
  handler() {
    return 'ok';
  },
});

There is no way to response by short error {"statusCode":400,"error":"Bad Request","message":"Invalid request query input"} and log to console detailed error Error [ValidationError]: "foo" is not allowed to be empty.

[sholladay]

Yeah, I always use the last one and I do it at the server level:

failAction(request, h, err) {
    request.log('error', err);
    throw err;
},

The other modes seem pretty pointless to me. I think you'd always want the detailed error in the logs and the only thing that really needs configuration is whether to return the detailed error in the response.

[Nargonath]

I believe this is a valid feature request. Even though you could throw a Boom.badRequest error as Eran mentioned you lose the message generated by Joi for the defaultError. This not a good solution if you have a global failAction setting in your server configuration IMO because there is no way to make an error message that is still a bit precise without providing the full information detailedError has.

We could handle this in a non-breaking way by adding args: [options.details || err, err] here. I'm not a big fan of it because the first arg could be either the detailedError or the defaultError. In the case where we don't have a detailedError both arg would be the same. At least this has the benefit of being a non-breaking change.

If we go that route and don't want to publish a breaking change, I'd suggest we also keep an issue open for the next major version release to replace it with: args: [options.details, err].

In the end the failAction signature would be:

failAction(request, h, detailedError | defaultError, defaultError) {}

[devinivy]

That's a good thought. Another approach could be to tack the default error onto the detailed error in this case so that it could be accessed:

failAction(request, h, err) {
    request.log('error', err);
    throw err.defaultError;
}

[sholladay]

Another somewhat related problem is how the default error message includes "request query input", which is useful to know, but the detailed error message doesn't include that source info.

So in my logs, for example, I have to choose between:

1. With failAction : 'log':

   Debug: validation, error, query 
       Error: Invalid request query input
       at Object.internals.input (/[redacted]/node_modules/@hapi/hapi/lib/validation.js:148:74)
       at async Request._lifecycle (/[redacted]/node_modules/@hapi/hapi/lib/request.js:370:32)
       at async Request._execute (/[redacted]/node_modules/@hapi/hapi/lib/request.js:279:9)

2. With failAction : (request, h, error) => { request.log('error', error); throw error; }:

   Debug: error 
       ValidationError: "ignoreNotFound" must be []

Except, option 1 isn't really an option for me, because failAction : 'log' disables enforcement of the validation, which is basically never what I want. Although, I see how it could be useful in some rare cases.

It's also kind of odd how option 1 includes a (relatively useless) stack trace while option 2 does not.

Personally, all of this stuff is my biggest pain point with hapi right now.

[Nargonath]

Good idea @devinivy . I didn't think of this one. Would you consider it a transitional step before a breaking change or the final implementation?

Thanks @sholladay . Interesting stuff to consider as well.

[mahnunchik]

I'm really happy that this feature has found followers. I'm looking forward to have it implemented in any from proposed option.

[ghdoergeloh]

This is exactly what I was looking for.

I implemented this behavior but it was a bit hacky without having the defaultError in the failAction method.

This implementation is TS and typesafe. That is the reason for some (maybe unnecessary) checks:

import {ValidationError} from "joi";

failAction: (request, _h, err): null => {
    if (Boom.isBoom(err)) {
      const validation = err.output.payload.validation as
        | { source?: string }
        | undefined;
      const source = validation?.source ?? '';
      if (err instanceof ValidationError) {
        request.log(['validation', 'error', source], err.message);
      }
      throw Boom.badRequest(`Invalid request ${source} input`);
    } else {
      // should never be the case, because a failAction is always called with a Boom error
      throw Boom.badRequest('Invalid request input');
    }
}

So in short maybe:

failAction: (request, h, err) => {
    const source = err.output.payload.validation?.source
    request.log(['validation', 'error', source], err.message);
    throw Boom.badRequest(`Invalid request ${source} input`);
}

[devinivy]

Resolved with v21 #4386