fix(logger): logger should display the right status code in case of e…

…rror (#1194)
ForestAdmin · Oct 15, 2024 · d328764 · d328764
1 parent 1fd3669
commit d328764
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 14 deletions.
diff --git a/packages/agent/src/routes/system/error-handling.ts b/packages/agent/src/routes/system/error-handling.ts
@@ -42,6 +42,9 @@ export default class ErrorHandling extends BaseRoute {
       if (!this.options.isProduction) {
         process.nextTick(() => this.debugLogError(context, e));
       }
+
+      // The error will be used by Logger
+      throw e;
     }
   }
 

diff --git a/packages/agent/src/routes/system/logger.ts b/packages/agent/src/routes/system/logger.ts
@@ -20,7 +20,6 @@ export default class Logger extends BaseRoute {
       await next();
     } catch (e) {
       error = e;
-      throw e;
     } finally {
       let logLevel: LoggerLevel = 'Info';
       if (context.response.status >= HttpCode.BadRequest) logLevel = 'Warn';

diff --git a/packages/agent/src/types.ts b/packages/agent/src/types.ts
@@ -47,8 +47,8 @@ export enum HttpCode {
 
 export enum RouteType {
   // Changing the values of this enum changes the order in which routes are loaded into koa-router.
-  ErrorHandler = 0,
-  LoggerHandler = 1,
+  LoggerHandler = 0,
+  ErrorHandler = 1,
   PublicRoute = 2,
   Authentication = 3,
   PrivateRoute = 4,

diff --git a/packages/agent/test/routes/index.test.ts b/packages/agent/test/routes/index.test.ts
@@ -27,6 +27,7 @@ import ScopeInvalidation from '../../src/routes/security/scope-invalidation';
 import ErrorHandling from '../../src/routes/system/error-handling';
 import HealthCheck from '../../src/routes/system/healthcheck';
 import Logger from '../../src/routes/system/logger';
+import { RouteType } from '../../src/types';
 import * as factories from '../__factories__';
 
 describe('Route index', () => {
@@ -81,6 +82,22 @@ describe('Route index', () => {
         const countCollectionRoutes = COLLECTION_ROUTES_CTOR.length * dataSource.collections.length;
         expect(routes.length).toEqual(ROOT_ROUTES_CTOR.length + countCollectionRoutes);
       });
+
+      test('should order routes correctly', async () => {
+        const dataSource = setupWithoutRelation();
+
+        const routes = makeRoutes(
+          dataSource,
+          factories.forestAdminHttpDriverOptions.build(),
+          factories.forestAdminHttpDriverServices.build(),
+        );
+
+        // with LoggerHandler in first
+        expect(routes[0].type).toEqual(RouteType.LoggerHandler);
+
+        // followed by ErrorHandler
+        expect(routes[1].type).toEqual(RouteType.ErrorHandler);
+      });
     });
 
     describe('when a data source with an one to many relation', () => {

diff --git a/packages/agent/test/routes/system/error-handling.test.ts b/packages/agent/test/routes/system/error-handling.test.ts
@@ -71,7 +71,7 @@ describe('ErrorHandling', () => {
       const context = createMockContext();
       const next = jest.fn().mockRejectedValue(new ValidationError('hello'));
 
-      await handleError.call(route, context, next);
+      await expect(handleError.call(route, context, next)).rejects.toThrow();
 
       expect(context.response.status).toStrictEqual(HttpCode.BadRequest);
       expect(context.response.body).toStrictEqual({
@@ -84,7 +84,7 @@ describe('ErrorHandling', () => {
       const context = createMockContext();
       const next = jest.fn().mockRejectedValue(new BadRequestError('message'));
 
-      await handleError.call(route, context, next);
+      await expect(handleError.call(route, context, next)).rejects.toThrow();
 
       expect(context.response.status).toStrictEqual(HttpCode.BadRequest);
       expect(context.response.body).toStrictEqual({
@@ -97,7 +97,7 @@ describe('ErrorHandling', () => {
       const context = createMockContext();
       const next = jest.fn().mockRejectedValue(new UnprocessableError('message'));
 
-      await handleError.call(route, context, next);
+      await expect(handleError.call(route, context, next)).rejects.toThrow();
 
       expect(context.response.status).toStrictEqual(HttpCode.Unprocessable);
       expect(context.response.body).toStrictEqual({
@@ -110,7 +110,7 @@ describe('ErrorHandling', () => {
       const context = createMockContext();
       const next = jest.fn().mockRejectedValue(new ForbiddenError('forbidden'));
 
-      await handleError.call(route, context, next);
+      await expect(handleError.call(route, context, next)).rejects.toThrow();
 
       expect(context.response.status).toStrictEqual(HttpCode.Forbidden);
       expect(context.response.body).toStrictEqual({
@@ -123,7 +123,7 @@ describe('ErrorHandling', () => {
       const context = createMockContext();
       const next = jest.fn().mockRejectedValue(new NotFoundError('message'));
 
-      await handleError.call(route, context, next);
+      await expect(handleError.call(route, context, next)).rejects.toThrow();
 
       expect(context.response.status).toStrictEqual(HttpCode.NotFound);
       expect(context.response.body).toStrictEqual({
@@ -138,7 +138,7 @@ describe('ErrorHandling', () => {
         .fn()
         .mockRejectedValue(new Error('Internal error with important data that should not leak'));
 
-      await handleError.call(route, context, next);
+      await expect(handleError.call(route, context, next)).rejects.toThrow();
 
       expect(context.response.status).toStrictEqual(HttpCode.InternalServerError);
       expect(context.response.body).toStrictEqual({
@@ -151,7 +151,7 @@ describe('ErrorHandling', () => {
       const context = createMockContext();
       const next = jest.fn().mockRejectedValue(new Error('My Error'));
 
-      await handleError.call(route, context, next);
+      await expect(handleError.call(route, context, next)).rejects.toThrow();
 
       expect(context.response.status).toStrictEqual(HttpCode.InternalServerError);
       expect(context.response.body).toStrictEqual({
@@ -165,7 +165,7 @@ describe('ErrorHandling', () => {
           const context = createMockContext();
           const next = jest.fn().mockRejectedValue(new FakePayloadError('message'));
 
-          await handleError.call(route, context, next);
+          await expect(handleError.call(route, context, next)).rejects.toThrow();
 
           expect(context.response.status).toStrictEqual(HttpCode.Forbidden);
           expect(context.response.body).toStrictEqual({
@@ -196,7 +196,7 @@ describe('ErrorHandling', () => {
 
           const next = jest.fn().mockRejectedValue(fakeErrorFromDependency);
 
-          await handleError.call(route, context, next);
+          await expect(handleError.call(route, context, next)).rejects.toThrow();
 
           expect(context.response.status).toStrictEqual(HttpCode.InternalServerError);
           expect(context.response.body).toStrictEqual({
@@ -233,7 +233,8 @@ describe('ErrorHandling', () => {
       const context = createMockContext({ method: 'POST' });
       const next = jest.fn().mockRejectedValue(new Error('Internal error'));
 
-      await handleError.call(route, context, next);
+      await expect(handleError.call(route, context, next)).rejects.toThrow();
+
       await new Promise(setImmediate);
 
       expect(console.error).toHaveBeenCalled();

diff --git a/packages/agent/test/routes/system/logger.test.ts b/packages/agent/test/routes/system/logger.test.ts
@@ -71,7 +71,8 @@ describe('Logger', () => {
       };
       const next = jest.fn().mockRejectedValue(error);
 
-      await expect(handleLog.call(route, context, next)).rejects.toThrow('RouteError');
+      await expect(handleLog.call(route, context, next)).toResolve();
+
       expect(options.logger).toHaveBeenCalledWith('Error', '[500] GET someUrl - 0ms', error);
     });
   });