arb / celebrate

A joi validation middleware for Express.
MIT License
1.34k stars 65 forks source link

Celebrate doesn't work with Joi `any.external` when throwing errors. #218

Closed kippllo closed 3 years ago

kippllo commented 3 years ago

Hello 👋🏻 I hope you're doing well.

For some reason I am getting a celebrate error when using a Joi any.external:

router.post(
        '/test',
        celebrate({
            body: Joi.object({
                test: Joi.string().external((value) => {
                    throw new Error('nope');
                }),
            }),
        }),
     ...

When I try this route with an HTTP POST that makes it to the external(), I get this error from Celebrate:

AssertionError [ERR_ASSERTION]: value must be a joi validation error
    at Map.set (C:\project\node_modules\celebrate\lib\CelebrateError.js:11:12)
    at C:\project\node_modules\celebrate\lib\celebrate.js:96:19
    at processTicksAndRejections (internal/process/task_queues.js:97:5) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

I'm not 100% what's wrong here. But I am fairly sure this is an issue in Celebrate because the Joi doc seems to allow for any.external to throw errors this way:

method - an async or sync function ... which can ... throw an error. (See: https://joi.dev/api/?v=17.4.0#anyexternalmethod-description)



The error messages from Celebrate seems to be coming from the following file: https://github.com/arb/celebrate/blob/d98db373f1bc6114b81d5183ba9894b5a9824b1e/lib/CelebrateError.js#L11



Here is a full project reproducing the error in a sandbox: https://github.com/kippllo/celebrate-iss-218


Thanks for the help!

arb commented 3 years ago

Can you try throwing ValidationError instead of a generic Error? https://joi.dev/api/?v=17.4.0#validationerror

kippllo commented 3 years ago

Yes, that does work. In fact, I thought the same thing and implemented it as a work-around.

But I was hoping that support for regular errors would be possible. Like the Joi documentation states.

Do you plan to add support for regular errors in the future? I'm a bit confused why Joi allows them, but they don't work in Celebrate.

arb commented 3 years ago

The reason it is this way is because I want to know for sure the errors inside the celebrate error are Joi errors when trying to format them for the HTTP response with the errors() middleware included with celebrate. I suppose I could make some changes to allow this but you'd be the first person asking for this in over 5 years 😅

kippllo commented 3 years ago

Aww that makes sense.

I totally understand if you'd rather leave this behavior as is. But I am curious if you'd be open to me trying to make a PR to alleviate the issue?

For example, perhaps we could wrap all any.external validation functions inside of something like this:

wrapExtrn(baseFnct) {
    return async (value, helpers) => {
        try {
            return await baseFnct(value, helpers);
        } catch (err) {
            throw new ValidationError(
                'External Error',
                [
                    {
                        message: err.message,
                        path: undefined,
                        type: undefined,
                        context: undefined,
                    },
                ],
                null
            );
        }
    }
}

That way it would convert all regular errors thrown inside of any.external into a ValidationError. And we would not have to modify any logic used inside of the errors() middleware.

arb commented 3 years ago

Alternatively, we could check for an error being a ValidationError or not during formatting and drop the check when adding it to the details Map. That way, users could put whatever the wanted into the details map, including standard Error objects.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.