fix: Make sure there is always a fallback for error handling

config/ldp/handler/components/error-handler.json

@@ -2,20 +2,16 @@
   "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld",
   "@graph": [
     {
-      "comment": "Changes an error into a valid representation to send as a response.",
+      "comment": "Wraps around the main error handler as a fallback in case something goes wrong.",
       "@id": "urn:solid-server:default:ErrorHandler",
-      "@type": "WaterfallHandler",
-      "handlers": [
-        {
+      "@type": "SafeErrorHandler",
+      "showStackTrace": { "@id": "urn:solid-server:default:variable:showStackTrace" },
+      "errorHandler": {
+        "comment": "Changes an error into a valid representation to send as a response.",
         "@type": "ConvertingErrorHandler",
         "converter": { "@id": "urn:solid-server:default:RepresentationConverter" },
         "showStackTrace": { "@id": "urn:solid-server:default:variable:showStackTrace" }
-        },
-        {
-          "@type": "TextErrorHandler",
-          "showStackTrace": { "@id": "urn:solid-server:default:variable:showStackTrace" }
-        }
-      ]
+      }
     }
   ]
 }

package-lock.json

@@ -103,7 +103,6 @@
         "nodemon": "^2.0.7",
         "rimraf": "^3.0.2",
         "set-cookie-parser": "^2.4.8",
-        "stream-to-string": "^1.2.0",
         "supertest": "^6.1.3",
         "ts-jest": "^27.0.3",
         "typedoc": "^0.21.2",

package.json

@@ -166,7 +166,6 @@
     "nodemon": "^2.0.7",
     "rimraf": "^3.0.2",
     "set-cookie-parser": "^2.4.8",
-    "stream-to-string": "^1.2.0",
     "supertest": "^6.1.3",
     "ts-jest": "^27.0.3",
     "typedoc": "^0.21.2",

src/index.ts

@@ -117,10 +117,10 @@ export * from './ldp/http/PreferenceParser';
 export * from './ldp/http/RawBodyParser';
 export * from './ldp/http/RequestParser';
 export * from './ldp/http/ResponseWriter';
+export * from './ldp/http/SafeErrorHandler';
 export * from './ldp/http/SparqlUpdateBodyParser';
 export * from './ldp/http/SparqlUpdatePatch';
 export * from './ldp/http/TargetExtractor';
-export * from './ldp/http/TextErrorHandler';
 
 // LDP/Operations
 export * from './ldp/operations/DeleteOperationHandler';

src/ldp/http/SafeErrorHandler.ts

@@ -1,4 +1,5 @@
-import { getStatusCode } from '../../util/errors/ErrorUtil';
+import { getLoggerFor } from '../../logging/LogUtil';
+import { createErrorMessage, getStatusCode } from '../../util/errors/ErrorUtil';
 import { guardedStreamFrom } from '../../util/StreamUtil';
 import { toLiteral } from '../../util/TermUtil';
 import { HTTP, XSD } from '../../util/Vocabularies';
@@ -9,17 +10,27 @@ import type { ResponseDescription } from './response/ResponseDescription';
 
 /**
  * Returns a simple text description of an error.
- * This class is mostly a failsafe in case all other solutions fail.
+ * This class is a failsafe in case the wrapped error handler fails.
  */
-export class TextErrorHandler extends ErrorHandler {
+export class SafeErrorHandler extends ErrorHandler {
+  protected readonly logger = getLoggerFor(this);
+
+  private readonly errorHandler: ErrorHandler;
   private readonly showStackTrace: boolean;
 
-  public constructor(showStackTrace = false) {
+  public constructor(errorHandler: ErrorHandler, showStackTrace = false) {
     super();
+    this.errorHandler = errorHandler;
     this.showStackTrace = showStackTrace;
   }
 
-  public async handle({ error }: ErrorHandlerArgs): Promise<ResponseDescription> {
+  public async handle(input: ErrorHandlerArgs): Promise<ResponseDescription> {
+    try {
+      return await this.errorHandler.handleSafe(input);
+    } catch (error: unknown) {
+      this.logger.debug(`Recovering from error handler failure: ${createErrorMessage(error)}`);
+    }
+    const { error } = input;
     const statusCode = getStatusCode(error);
     const metadata = new RepresentationMetadata('text/plain');
     metadata.add(HTTP.terms.statusCodeNumber, toLiteral(statusCode, XSD.terms.integer));

test/unit/ldp/http/SafeErrorHandler.test.ts

@@ -0,0 +1,77 @@
+import 'jest-rdf';
+import { DataFactory } from 'n3';
+import type { ErrorHandler } from '../../../../src/ldp/http/ErrorHandler';
+import { SafeErrorHandler } from '../../../../src/ldp/http/SafeErrorHandler';
+import { BasicRepresentation } from '../../../../src/ldp/representation/BasicRepresentation';
+import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError';
+import { readableToString } from '../../../../src/util/StreamUtil';
+import { HTTP, XSD } from '../../../../src/util/Vocabularies';
+import literal = DataFactory.literal;
+
+describe('A SafeErrorHandler', (): void => {
+  let error: Error;
+  let stack: string | undefined;
+  let errorHandler: jest.Mocked<ErrorHandler>;
+  let handler: SafeErrorHandler;
+
+  beforeEach(async(): Promise<void> => {
+    error = new NotFoundHttpError('not here');
+    ({ stack } = error);
+
+    errorHandler = {
+      handleSafe: jest.fn().mockResolvedValue(new BasicRepresentation('<html>fancy error</html>', 'text/html')),
+    } as any;
+
+    handler = new SafeErrorHandler(errorHandler, true);
+  });
+
+  it('can handle everything.', async(): Promise<void> => {
+    await expect(handler.canHandle({} as any)).resolves.toBeUndefined();
+  });
+
+  it('sends the request to the stored error handler.', async(): Promise<void> => {
+    const prom = handler.handle({ error } as any);
+    await expect(prom).resolves.toBeDefined();
+    const result = await prom;
+    expect(result.metadata?.contentType).toBe('text/html');
+    await expect(readableToString(result.data!)).resolves.toBe('<html>fancy error</html>');
+  });
+
+  describe('where the wrapped error handler fails', (): void => {
+    beforeEach(async(): Promise<void> => {
+      errorHandler.handleSafe.mockRejectedValue(new Error('handler failed'));
+    });
+
+    it('creates a text representation of the error.', async(): Promise<void> => {
+      const prom = handler.handle({ error } as any);
+      await expect(prom).resolves.toBeDefined();
+      const result = await prom;
+      expect(result.statusCode).toBe(404);
+      expect(result.metadata?.get(HTTP.terms.statusCodeNumber)).toEqualRdfTerm(literal(404, XSD.terms.integer));
+      expect(result.metadata?.contentType).toBe('text/plain');
+      await expect(readableToString(result.data!)).resolves.toBe(`${stack}\n`);
+    });
+
+    it('concatenates name and message if there is no stack.', async(): Promise<void> => {
+      delete error.stack;
+      const prom = handler.handle({ error } as any);
+      await expect(prom).resolves.toBeDefined();
+      const result = await prom;
+      expect(result.statusCode).toBe(404);
+      expect(result.metadata?.get(HTTP.terms.statusCodeNumber)).toEqualRdfTerm(literal(404, XSD.terms.integer));
+      expect(result.metadata?.contentType).toBe('text/plain');
+      await expect(readableToString(result.data!)).resolves.toBe(`NotFoundHttpError: not here\n`);
+    });
+
+    it('hides the stack trace if the option is disabled.', async(): Promise<void> => {
+      handler = new SafeErrorHandler(errorHandler);
+      const prom = handler.handle({ error } as any);
+      await expect(prom).resolves.toBeDefined();
+      const result = await prom;
+      expect(result.statusCode).toBe(404);
+      expect(result.metadata?.get(HTTP.terms.statusCodeNumber)).toEqualRdfTerm(literal(404, XSD.terms.integer));
+      expect(result.metadata?.contentType).toBe('text/plain');
+      await expect(readableToString(result.data!)).resolves.toBe(`NotFoundHttpError: not here\n`);
+    });
+  });
+});

test/unit/ldp/http/TextErrorHandler.test.ts

@@ -1,60 +0,0 @@
-import 'jest-rdf';
-import { DataFactory } from 'n3';
-import stringifyStream from 'stream-to-string';
-import { TextErrorHandler } from '../../../../src/ldp/http/TextErrorHandler';
-import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError';
-import { HTTP, XSD } from '../../../../src/util/Vocabularies';
-import literal = DataFactory.literal;
-
-describe('A TextErrorHandler', (): void => {
-  // The error object can get modified by the handler
-  let error: Error;
-  let stack: string | undefined;
-  let handler: TextErrorHandler;
-
-  beforeEach(async(): Promise<void> => {
-    error = new NotFoundHttpError('not here');
-    ({ stack } = error);
-
-    handler = new TextErrorHandler(true);
-  });
-
-  it('can handle everything.', async(): Promise<void> => {
-    await expect(handler.canHandle({} as any)).resolves.toBeUndefined();
-  });
-
-  it('creates a text representation of the error.', async(): Promise<void> => {
-    const prom = handler.handle({ error } as any);
-    await expect(prom).resolves.toBeDefined();
-    const result = await prom;
-    expect(result.statusCode).toBe(404);
-    expect(result.metadata?.get(HTTP.terms.statusCodeNumber)).toEqualRdfTerm(literal(404, XSD.terms.integer));
-    expect(result.metadata?.contentType).toBe('text/plain');
-    const text = await stringifyStream(result.data!);
-    expect(text).toBe(`${stack}\n`);
-  });
-
-  it('concatenates name and message if there is no stack.', async(): Promise<void> => {
-    delete error.stack;
-    const prom = handler.handle({ error } as any);
-    await expect(prom).resolves.toBeDefined();
-    const result = await prom;
-    expect(result.statusCode).toBe(404);
-    expect(result.metadata?.get(HTTP.terms.statusCodeNumber)).toEqualRdfTerm(literal(404, XSD.terms.integer));
-    expect(result.metadata?.contentType).toBe('text/plain');
-    const text = await stringifyStream(result.data!);
-    expect(text).toBe(`NotFoundHttpError: not here\n`);
-  });
-
-  it('hides the stack trace if the option is disabled.', async(): Promise<void> => {
-    handler = new TextErrorHandler();
-    const prom = handler.handle({ error } as any);
-    await expect(prom).resolves.toBeDefined();
-    const result = await prom;
-    expect(result.statusCode).toBe(404);
-    expect(result.metadata?.get(HTTP.terms.statusCodeNumber)).toEqualRdfTerm(literal(404, XSD.terms.integer));
-    expect(result.metadata?.contentType).toBe('text/plain');
-    const text = await stringifyStream(result.data!);
-    expect(text).toBe(`NotFoundHttpError: not here\n`);
-  });
-});

test/unit/storage/conversion/QuadToRdfConverter.test.ts

@@ -1,6 +1,5 @@
 import { namedNode, triple } from '@rdfjs/data-model';
 import rdfSerializer from 'rdf-serialize';
-import stringifyStream from 'stream-to-string';
 import streamifyArray from 'streamify-array';
 import type { Representation } from '../../../../src/ldp/representation/Representation';
 import { RepresentationMetadata } from '../../../../src/ldp/representation/RepresentationMetadata';
@@ -8,6 +7,7 @@ import type { RepresentationPreferences } from '../../../../src/ldp/representati
 import type { ResourceIdentifier } from '../../../../src/ldp/representation/ResourceIdentifier';
 import { QuadToRdfConverter } from '../../../../src/storage/conversion/QuadToRdfConverter';
 import { INTERNAL_QUADS } from '../../../../src/util/ContentTypes';
+import { readableToString } from '../../../../src/util/StreamUtil';
 import { DC, PREFERRED_PREFIX_TERM } from '../../../../src/util/Vocabularies';
 
 describe('A QuadToRdfConverter', (): void => {
@@ -63,7 +63,7 @@ describe('A QuadToRdfConverter', (): void => {
       metadata: expect.any(RepresentationMetadata),
     });
     expect(result.metadata.contentType).toEqual('text/turtle');
-    await expect(stringifyStream(result.data)).resolves.toEqual(
+    await expect(readableToString(result.data)).resolves.toEqual(
       `<http://test.com/s> <http://test.com/p> <http://test.com/o>.
 `,
     );
@@ -83,7 +83,7 @@ describe('A QuadToRdfConverter', (): void => {
     const preferences: RepresentationPreferences = { type: { 'text/turtle': 1 }};
     const result = await converter.handle({ identifier, representation, preferences });
     expect(result.metadata.contentType).toEqual('text/turtle');
-    await expect(stringifyStream(result.data)).resolves.toEqual(
+    await expect(readableToString(result.data)).resolves.toEqual(
       `@prefix dc: <http://purl.org/dc/terms/>.
 @prefix test: <http://test.com/>.
 
@@ -104,7 +104,7 @@ test:s dc:modified test:o.
     const preferences: RepresentationPreferences = { type: { 'text/turtle': 1 }};
     const result = await converter.handle({ identifier, representation, preferences });
     expect(result.metadata.contentType).toEqual('text/turtle');
-    await expect(stringifyStream(result.data)).resolves.toEqual(
+    await expect(readableToString(result.data)).resolves.toEqual(
       `<> <#abc> <def/ghi>.
 `,
     );
@@ -127,7 +127,7 @@ test:s dc:modified test:o.
       metadata: expect.any(RepresentationMetadata),
     });
     expect(result.metadata.contentType).toEqual('application/ld+json');
-    await expect(stringifyStream(result.data)).resolves.toEqual(
+    await expect(readableToString(result.data)).resolves.toEqual(
       `[
   {
     "@id": "http://test.com/s",