Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setErrorHandler is not called on plugin error #4204

Closed
2 tasks done
qlaffont opened this issue Aug 18, 2022 · 16 comments · Fixed by #4205
Closed
2 tasks done

setErrorHandler is not called on plugin error #4204

qlaffont opened this issue Aug 18, 2022 · 16 comments · Fixed by #4205
Labels
bug Confirmed bug

Comments

@qlaffont
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.5.0

Plugin version

4.2.0

Node.js version

16.17.0

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

22.04

Description

Hi,
When I try to return an error from fastify plugin, I see that setErrorHandler is not called.

I don't know why because I call my error handler plugin before to register my new plugin.

Thanks for the help !

Steps to Reproduce

Expected Behavior

Should return formated error from https://github.com/flexper/unify-fastify

@climba03003
Copy link
Member

climba03003 commented Aug 18, 2022

I don't see how it will trigger the error handler.
There is no route inside your example.

All yours request should go through not found handler.

@qlaffont
Copy link
Author

My plugin will return an error for any route if it is not wrote in config. So for any route it will return an unauthorized error.

(if route exist or not).

@climba03003
Copy link
Member

climba03003 commented Aug 18, 2022

Just one through, is it reproducible when using npm?

@qlaffont
Copy link
Author

I don't think it's due to package manager because package is loaded without any error.

@climba03003
Copy link
Member

climba03003 commented Aug 18, 2022

I don't think it's due to package manager because package is loaded without any error.

It could cause by how package resolve since, unify-errors exists in two package as dependency and you are using instanceof to check.

It depends on how require.cache to treats that two links and how the installed dependency structure look like.

The below one is what I have done to minimize the repro. It works just fine with npm.

import Fastify from 'fastify'
import { Unauthorized } from 'unify-errors'
import unifyFastify from 'unify-fastify'

const fastify = Fastify()

await fastify.register(unifyFastify, { hideContextOnProd: false })

fastify.addHook('preValidation', function(request, reply, done) {
  console.log('pre-validation hit')
  throw new Unauthorized()
})

fastify.setNotFoundHandler(function() {
  console.log('not found handler hit')
})

{
  const { statusCode, payload } = await fastify.inject('/')
  console.log(statusCode, payload)
}

@qlaffont
Copy link
Author

qlaffont commented Aug 18, 2022

I have done the same thing with npm.

npm install
npx ts-node test/core/real.ts

Same issue, so i can confirm that currently is not working.

As I see on your example, your preValidation is not in a fastify plugin, so it doesn't work.

A working example will be :

import fastify from 'fastify';
import fp from 'fastify-plugin';
import { Unauthorized } from 'unify-errors';
import unifyFastify from 'unify-fastify';

(async () => {
  const server = fastify();

  await server.register(unifyFastify, { hideContextOnProd: false });

  await server.register(
    fp(async (f) => {
      f.addHook('preValidation', async (request, reply) => {
        console.log('pre-validation hit');
        throw new Unauthorized();
      });
    }),
  );

  server.listen({ port: 3000 }).then(() => console.log('ready'));
})();

@climba03003
Copy link
Member

climba03003 commented Aug 18, 2022

I have tried both using fastify-plugin with or without it.
It doesn't affect the result, since it does not have encapsulation.

import Fastify from 'fastify'
import FastifyPlugin from 'fastify-plugin'
import { Unauthorized } from 'unify-errors'
import unifyFastify from 'unify-fastify'

const fastify = Fastify()

await fastify.register(unifyFastify, { hideContextOnProd: false })
await fastify.register(FastifyPlugin(function(fastify, _, done) {
  fastify.addHook('preValidation', function(request, reply, done) {
    console.log('pre-validation hit')
    throw new Unauthorized()
  })
  
  done()
}))


fastify.setNotFoundHandler(function() {
  console.log('not found handler hit')
})

{
  const { statusCode, payload } = await fastify.inject('/')
  console.log(statusCode, payload)
}

@climba03003
Copy link
Member

I see the problem now.
It still something related to the not found handler. The only different is that I have added a custom not found handler and it works.

Without it, it doesn't.

@climba03003 climba03003 added the bug Confirmed bug label Aug 18, 2022
@qlaffont
Copy link
Author

Okay i will push an update on unify-fastify to add it, but it seems anyway that there is a bug on fastify

@mcollina
Copy link
Member

Can you include a reproduction without external modules? Thanks

@qlaffont
Copy link
Author

import fastify from 'fastify';
import fp from 'fastify-plugin';

(async () => {
  const server = fastify();

  await server.register(fp(async(f) => {
    f.setErrorHandler((error, request, reply) => {
      console.log("error handler is called");
      reply.status(400).send("error");
    })
  }));

  await server.register(
    fp(async (f) => {
      f.addHook('preValidation', async (request, reply) => {
        console.log('pre-validation hit');
        throw new Error('test');
      });
    }),
  );

  server.listen({ port: 3000 }).then(() => console.log('ready'));
})();
pnpm dlx ts-node test.ts 

"error handler is called" will never be displayed

@climba03003
Copy link
Member

climba03003 commented Aug 18, 2022

The problem is more complicated than I though.
Here are the multiple way trigger the same problem.

  1. await plugin registration
import Fastify from 'fastify'

const fastify = Fastify()

// if it is not await, it works fine
await fastify.register(async function(fastify) {
})

fastify.setErrorHandler(function(error, request, reply) {
  return reply.code(401).send({"error":"Unauthorized"})
})

fastify.addHook('preValidation', async function(request, reply) {
  console.log('pre-validation')
  throw new Error()
})

{
  const { statusCode, payload } = await fastify.inject('/')
  console.log(statusCode, payload) // should be 401 {"error":"Unauthorized"}
}
  1. Set error handler within the plugin
import Fastify from 'fastify'
import FP from 'fastify-plugin'

const fastify = Fastify()

// it doesn't matter if this is awaited or not
fastify.register(FP(async function(fastify) {
  fastify.setErrorHandler(function(error, request, reply) {
    return reply.code(401).send({"error":"Unauthorized"})
  })
}))

fastify.addHook('preValidation', async function(request, reply) {
  console.log('pre-validation')
  throw new Error()
})

// provide a empty not found handle works fine
// fastify.setNotFoundHandler()

{
  const { statusCode, payload } = await fastify.inject('/')
  console.log(statusCode, payload) // should be 401 {"error":"Unauthorized"}
}

@budarin
Copy link
Contributor

budarin commented Aug 18, 2022

I have the same problem - throw an error in the async request decorator and I get the unhandledRejection error instead of processing it in setErrorHandler - spent half a day looking for a problem

@mcollina
Copy link
Member

I have the same problem - throw an error in the async request decorator and I get the unhandledRejection error instead of processing it in setErrorHandler - spent half a day looking for a problem

It's a different problem. Open a new issue with a minimum reproducible example.

@mcollina
Copy link
Member

@climba03003 I have a fix coming for this.

@mcollina mcollina mentioned this issue Aug 18, 2022
3 tasks
@mcollina
Copy link
Member

@qlaffont please check if #4205 solves this for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants