Repository: solid/community-server (ts)

Issue:
Don't lose status codes in CompositeAsyncHandler
Current best example is the body parsers. These will throw an error with status code 415 if the media type is not supported, but the composite handler will merge these into an error with status code 400. Perhaps the composite handler should check if all errors thrown are of the same type and then use that one (not sure how best to do that yet).

Pull request description:
fix: Retain status codes when combining errors
Resolves #23 .

This does put some status code logic in the CompositeAsyncHandler but not sure how to do this otherwise since this is quite ingrained.

Patch:
diff --git a/src/util/CompositeAsyncHandler.ts b/src/util/CompositeAsyncHandler.ts
index b1fbe887c..ea21a6caf 100644
--- a/src/util/CompositeAsyncHandler.ts
+++ b/src/util/CompositeAsyncHandler.ts
@@ -1,4 +1,6 @@
 import type { AsyncHandler } from './AsyncHandler';
+import { HttpError } from './errors/HttpError';
+import { InternalServerError } from './errors/InternalServerError';
 import { UnsupportedHttpError } from './errors/UnsupportedHttpError';
 
 /**
@@ -70,7 +72,7 @@ export class CompositeAsyncHandler<TIn, TOut> implements AsyncHandler<TIn, TOut>
    * @returns A promise resolving to a handler that supports the data or otherwise rejecting.
    */
   private async findHandler(input: TIn): Promise<AsyncHandler<TIn, TOut>> {
-    const errors: Error[] = [];
+    const errors: HttpError[] = [];
 
     for (const handler of this.handlers) {
       try {
@@ -78,16 +80,28 @@ export class CompositeAsyncHandler<TIn, TOut> implements AsyncHandler<TIn, TOut>
 
         return handler;
       } catch (error: unknown) {
-        if (error instanceof Error) {
+        if (error instanceof HttpError) {
           errors.push(error);
+        } else if (error instanceof Error) {
+          errors.push(new InternalServerError(error.message));
         } else {
-          errors.push(new Error('Unknown error.'));
+          errors.push(new InternalServerError('Unknown error.'));
         }
       }
     }
 
     const joined = errors.map((error: Error): string => error.message).join(', ');
+    const message = `No handler supports the given input: [${joined}].`;
 
-    throw new UnsupportedHttpError(`No handler supports the given input: [${joined}].`);
+    // Check if all errors have the same status code
+    if (errors.every((error): boolean => error.statusCode === errors[0].statusCode)) {
+      throw new HttpError(errors[0].statusCode, errors[0].name, message);
+    }
+
+    // Find the error range (4xx or 5xx)
+    if (errors.some((error): boolean => error.statusCode >= 500)) {
+      throw new InternalServerError(message);
+    }
+    throw new UnsupportedHttpError(message);
   }
 }
diff --git a/src/util/errors/HttpError.ts b/src/util/errors/HttpError.ts
index f940ac30e..a443cca24 100644
--- a/src/util/errors/HttpError.ts
+++ b/src/util/errors/HttpError.ts
@@ -1,8 +1,8 @@
 /**
- * An abstract class for all errors that could be thrown by Solid.
+ * A class for all errors that could be thrown by Solid.
  * All errors inheriting from this should fix the status code thereby hiding the HTTP internals from other components.
  */
-export abstract class HttpError extends Error {
+export class HttpError extends Error {
   public statusCode: number;
 
   /**
@@ -11,7 +11,7 @@ export abstract class HttpError extends Error {
    * @param name - Error name. Useful for logging and stack tracing.
    * @param message - Message to be thrown.
    */
-  protected constructor(statusCode: number, name: string, message?: string) {
+  public constructor(statusCode: number, name: string, message?: string) {
     super(message);
     this.statusCode = statusCode;
     this.name = name;
diff --git a/src/util/errors/InternalServerError.ts b/src/util/errors/InternalServerError.ts
new file mode 100644
index 000000000..25c7c0843
--- /dev/null
+++ b/src/util/errors/InternalServerError.ts
@@ -0,0 +1,9 @@
+import { HttpError } from './HttpError';
+/**
+ * A generic error message, given when an unexpected condition was encountered and no more specific message is suitable.
+ */
+export class InternalServerError extends HttpError {
+  public constructor(message?: string) {
+    super(500, 'InternalServerError', message);
+  }
+}


Test patch:
diff --git a/test/unit/util/CompositeAsyncHandler.test.ts b/test/unit/util/CompositeAsyncHandler.test.ts
index 6dcaf73cf..268155c44 100644
--- a/test/unit/util/CompositeAsyncHandler.test.ts
+++ b/test/unit/util/CompositeAsyncHandler.test.ts
@@ -1,5 +1,7 @@
 import type { AsyncHandler } from '../../../src/util/AsyncHandler';
 import { CompositeAsyncHandler } from '../../../src/util/CompositeAsyncHandler';
+import { HttpError } from '../../../src/util/errors/HttpError';
+import { UnsupportedHttpError } from '../../../src/util/errors/UnsupportedHttpError';
 import { StaticAsyncHandler } from '../../util/StaticAsyncHandler';
 
 describe('A CompositeAsyncHandler', (): void => {
@@ -81,5 +83,44 @@ describe('A CompositeAsyncHandler', (): void => {
 
       await expect(handler.handleSafe(null)).rejects.toThrow('[Not supported., Not supported.]');
     });
+
+    it('throws an error with matching status code if all handlers threw the same.', async(): Promise<void> => {
+      handlerTrue.canHandle = async(): Promise<void> => {
+        throw new HttpError(401, 'UnauthorizedHttpError');
+      };
+      const handler = new CompositeAsyncHandler([ handlerTrue, handlerTrue ]);
+
+      await expect(handler.canHandle(null)).rejects.toMatchObject({
+        statusCode: 401,
+        name: 'UnauthorizedHttpError',
+      });
+    });
+
+    it('throws an internal server error if one of the handlers threw one.', async(): Promise<void> => {
+      handlerTrue.canHandle = async(): Promise<void> => {
+        throw new HttpError(401, 'UnauthorizedHttpError');
+      };
+      handlerFalse.canHandle = async(): Promise<void> => {
+        throw new Error('Server is crashing!');
+      };
+      const handler = new CompositeAsyncHandler([ handlerTrue, handlerFalse ]);
+
+      await expect(handler.canHandle(null)).rejects.toMatchObject({
+        statusCode: 500,
+        name: 'InternalServerError',
+      });
+    });
+
+    it('throws an UnsupportedHttpError if handlers throw different errors.', async(): Promise<void> => {
+      handlerTrue.canHandle = async(): Promise<void> => {
+        throw new HttpError(401, 'UnauthorizedHttpError');
+      };
+      handlerFalse.canHandle = async(): Promise<void> => {
+        throw new HttpError(415, 'UnsupportedMediaTypeHttpError');
+      };
+      const handler = new CompositeAsyncHandler([ handlerTrue, handlerFalse ]);
+
+      await expect(handler.canHandle(null)).rejects.toThrow(UnsupportedHttpError);
+    });
   });
 });