fix: setErrorHandler must be registered before route modules
Fastify resolves each encapsulated child context's error handler at
the time that context is registered. app.ts called
app.setErrorHandler()/setNotFoundHandler() AFTER bootstrapRoutes()
had already registered every domain module's routes (each
app.register(someRoutes) call creates its own child context, since
none of the route modules use fastify-plugin). A handler set on the
parent afterwards does not retroactively apply to already-registered
children, so every module's routes were silently falling back to
Fastify's default {statusCode, error, message} error shape instead
of this app's {success:false, error:{code,message,details},
requestId} envelope, for any thrown error -- not specific to any one
feature. Discovered while building and manually verifying the
002-saas-integration feature's inbound endpoint.
Also fixed the generic error-handler branch to preserve a framework
error's own client-facing statusCode (e.g. 400 for malformed JSON)
instead of always reporting 500.
Added a regression test in tests/unit/app.test.ts that fails without
this fix and passes with it (verified both ways).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
000a8bcb6b
commit
5444fb7ef3
+27
-4
@@ -15,10 +15,10 @@ export async function buildApp(): Promise<FastifyInstance> {
|
|||||||
// Register Bootstrapped Plugins
|
// Register Bootstrapped Plugins
|
||||||
await bootstrapPlugins(app);
|
await bootstrapPlugins(app);
|
||||||
|
|
||||||
// Register Bootstrapped Global Routes
|
// Global Error Handler — MUST be registered before any route module is (bootstrapRoutes
|
||||||
await bootstrapRoutes(app);
|
// below), because Fastify resolves each encapsulated child context's error handler at the
|
||||||
|
// time that context is registered. A handler set after a child module has already been
|
||||||
// Global Error Handler
|
// registered will not retroactively apply to that module's routes/hooks.
|
||||||
app.setErrorHandler(
|
app.setErrorHandler(
|
||||||
(error: Error | AppError | ZodError, request: FastifyRequest, reply: FastifyReply) => {
|
(error: Error | AppError | ZodError, request: FastifyRequest, reply: FastifyReply) => {
|
||||||
const requestId = request.reqContext?.requestId || (request.id as string) || 'unknown';
|
const requestId = request.reqContext?.requestId || (request.id as string) || 'unknown';
|
||||||
@@ -52,6 +52,25 @@ export async function buildApp(): Promise<FastifyInstance> {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Framework-level errors (e.g. Fastify's own body-parser/payload errors) already carry
|
||||||
|
// their own client-facing statusCode/code — preserve those rather than reporting every
|
||||||
|
// non-AppError/ZodError as a 500, which would misclassify a client mistake as a server
|
||||||
|
// failure.
|
||||||
|
const frameworkStatusCode = (error as { statusCode?: number }).statusCode;
|
||||||
|
const frameworkCode = (error as { code?: string }).code;
|
||||||
|
if (frameworkStatusCode && frameworkStatusCode >= 400 && frameworkStatusCode < 500) {
|
||||||
|
logger.warn({ requestId, statusCode: frameworkStatusCode }, error.message);
|
||||||
|
return reply.status(frameworkStatusCode).send({
|
||||||
|
success: false,
|
||||||
|
error: {
|
||||||
|
code: frameworkCode ?? 'BAD_REQUEST',
|
||||||
|
message: error.message,
|
||||||
|
details: null,
|
||||||
|
},
|
||||||
|
requestId,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
logger.error({ error, requestId }, 'Unhandled Server Error');
|
logger.error({ error, requestId }, 'Unhandled Server Error');
|
||||||
|
|
||||||
const responseMessage =
|
const responseMessage =
|
||||||
@@ -85,5 +104,9 @@ export async function buildApp(): Promise<FastifyInstance> {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Register Bootstrapped Global Routes — MUST come after the handlers above (see comment
|
||||||
|
// on setErrorHandler).
|
||||||
|
await bootstrapRoutes(app);
|
||||||
|
|
||||||
return app;
|
return app;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -7,4 +7,32 @@ describe('App Factory Unit Test', () => {
|
|||||||
expect(app).toBeDefined();
|
expect(app).toBeDefined();
|
||||||
await app.close();
|
await app.close();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('applies the custom error envelope to errors raised inside a registered route module, not just root-level routes', async () => {
|
||||||
|
// Regression test: app.setErrorHandler() must be called BEFORE any route module is
|
||||||
|
// registered (bootstrapRoutes), because Fastify resolves each encapsulated child
|
||||||
|
// context's error handler at registration time — a handler set afterwards does not
|
||||||
|
// retroactively apply to already-registered child modules. This was discovered while
|
||||||
|
// building specs/002-saas-integration: every domain module's routes (registered via
|
||||||
|
// `app.register(someModuleRoutes)`, which creates a child encapsulation context) were
|
||||||
|
// silently falling back to Fastify's default `{statusCode, error, message}` shape
|
||||||
|
// instead of this app's `{success:false, error:{code,message,details}, requestId}`
|
||||||
|
// envelope. Uses malformed JSON against a real registered route (not a synthetic
|
||||||
|
// top-level route) so the error is raised by Fastify's own body parser inside that
|
||||||
|
// module's encapsulation context, with no database/Redis dependency — a regression
|
||||||
|
// here (e.g. someone moving setErrorHandler back after bootstrapRoutes) fails this test.
|
||||||
|
const app = await buildApp();
|
||||||
|
const response = await app.inject({
|
||||||
|
method: 'POST',
|
||||||
|
url: '/v1/support/requests',
|
||||||
|
headers: { 'content-type': 'application/json' },
|
||||||
|
payload: 'not-valid-json{{{',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(response.statusCode).toBe(400);
|
||||||
|
const body = response.json();
|
||||||
|
expect(body).toMatchObject({ success: false, error: { code: expect.any(String) } });
|
||||||
|
expect(body.requestId).toBeDefined();
|
||||||
|
await app.close();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user