From bd10256e5901320d9736310868bed1d820aba5d1 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Fri, 23 Jul 2021 11:55:27 +0200 Subject: [PATCH] fix: Make sure there is always a fallback for error handling --- .../ldp/handler/components/error-handler.json | 22 +++--- package-lock.json | 1 - package.json | 1 - src/index.ts | 2 +- ...extErrorHandler.ts => SafeErrorHandler.ts} | 21 +++-- test/unit/ldp/http/SafeErrorHandler.test.ts | 77 +++++++++++++++++++ test/unit/ldp/http/TextErrorHandler.test.ts | 60 --------------- .../conversion/QuadToRdfConverter.test.ts | 10 +-- 8 files changed, 108 insertions(+), 86 deletions(-) rename src/ldp/http/{TextErrorHandler.ts => SafeErrorHandler.ts} (56%) create mode 100644 test/unit/ldp/http/SafeErrorHandler.test.ts delete mode 100644 test/unit/ldp/http/TextErrorHandler.test.ts diff --git a/config/ldp/handler/components/error-handler.json b/config/ldp/handler/components/error-handler.json index 027c9f412..b8b12b918 100644 --- a/config/ldp/handler/components/error-handler.json +++ b/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": "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" } - } - ] + "@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" } + } } ] } diff --git a/package-lock.json b/package-lock.json index aa1ad437b..6e14e0ae2 100644 --- a/package-lock.json +++ b/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", diff --git a/package.json b/package.json index cc069bde1..2f91dfec2 100644 --- a/package.json +++ b/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", diff --git a/src/index.ts b/src/index.ts index 1b6893593..844cc3c10 100644 --- a/src/index.ts +++ b/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'; diff --git a/src/ldp/http/TextErrorHandler.ts b/src/ldp/http/SafeErrorHandler.ts similarity index 56% rename from src/ldp/http/TextErrorHandler.ts rename to src/ldp/http/SafeErrorHandler.ts index d93363a93..a49fee1cb 100644 --- a/src/ldp/http/TextErrorHandler.ts +++ b/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 { + public async handle(input: ErrorHandlerArgs): Promise { + 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)); diff --git a/test/unit/ldp/http/SafeErrorHandler.test.ts b/test/unit/ldp/http/SafeErrorHandler.test.ts new file mode 100644 index 000000000..6b92a6a19 --- /dev/null +++ b/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; + let handler: SafeErrorHandler; + + beforeEach(async(): Promise => { + error = new NotFoundHttpError('not here'); + ({ stack } = error); + + errorHandler = { + handleSafe: jest.fn().mockResolvedValue(new BasicRepresentation('fancy error', 'text/html')), + } as any; + + handler = new SafeErrorHandler(errorHandler, true); + }); + + it('can handle everything.', async(): Promise => { + await expect(handler.canHandle({} as any)).resolves.toBeUndefined(); + }); + + it('sends the request to the stored error handler.', async(): Promise => { + 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('fancy error'); + }); + + describe('where the wrapped error handler fails', (): void => { + beforeEach(async(): Promise => { + errorHandler.handleSafe.mockRejectedValue(new Error('handler failed')); + }); + + it('creates a text representation of the error.', async(): Promise => { + 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 => { + 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 => { + 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`); + }); + }); +}); diff --git a/test/unit/ldp/http/TextErrorHandler.test.ts b/test/unit/ldp/http/TextErrorHandler.test.ts deleted file mode 100644 index 7c9121f9e..000000000 --- a/test/unit/ldp/http/TextErrorHandler.test.ts +++ /dev/null @@ -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 => { - error = new NotFoundHttpError('not here'); - ({ stack } = error); - - handler = new TextErrorHandler(true); - }); - - it('can handle everything.', async(): Promise => { - await expect(handler.canHandle({} as any)).resolves.toBeUndefined(); - }); - - it('creates a text representation of the error.', async(): Promise => { - 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 => { - 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 => { - 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`); - }); -}); diff --git a/test/unit/storage/conversion/QuadToRdfConverter.test.ts b/test/unit/storage/conversion/QuadToRdfConverter.test.ts index 35a3131b7..9571e8ed0 100644 --- a/test/unit/storage/conversion/QuadToRdfConverter.test.ts +++ b/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( ` . `, ); @@ -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: . @prefix test: . @@ -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> . `, ); @@ -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",