From e8fdcb0ad08d608ed5e6eda58ad7236a844cbe1c Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Mon, 26 Oct 2020 12:05:39 +0100 Subject: [PATCH] refactor: Split off ErrorResponseWriter --- config/config-default.json | 1 + config/config-file.json | 1 + config/config-path-routing.json | 1 + config/config-rdf-to-sparql-endpoint.json | 1 + config/config-sparql-endpoint.json | 1 + config/presets/ldp.json | 2 +- config/presets/ldp/response-writer.json | 17 ++++++ index.ts | 1 + src/ldp/http/BasicResponseWriter.ts | 46 ++++++---------- src/ldp/http/ErrorResponseWriter.ts | 32 +++++++++++ .../AuthenticatedDataAccessorBasedConfig.ts | 4 +- test/configs/BasicConfig.ts | 5 +- test/configs/BasicHandlersConfig.ts | 4 +- test/configs/BasicHandlersWithAclConfig.ts | 4 +- test/configs/DataAccessorBasedConfig.ts | 4 +- test/configs/Util.ts | 10 ++++ .../unit/ldp/http/BasicResponseWriter.test.ts | 34 ++---------- .../unit/ldp/http/ErrorResponseWriter.test.ts | 54 +++++++++++++++++++ 18 files changed, 150 insertions(+), 72 deletions(-) create mode 100644 config/presets/ldp/response-writer.json create mode 100644 src/ldp/http/ErrorResponseWriter.ts create mode 100644 test/unit/ldp/http/ErrorResponseWriter.test.ts diff --git a/config/config-default.json b/config/config-default.json index 7c0c0f14a..094e4c137 100644 --- a/config/config-default.json +++ b/config/config-default.json @@ -8,6 +8,7 @@ "files-scs:config/presets/ldp/metadata-handler.json", "files-scs:config/presets/ldp/operation-handler.json", "files-scs:config/presets/ldp/permissions-extractor.json", + "files-scs:config/presets/ldp/response-writer.json", "files-scs:config/presets/ldp/request-parser.json", "files-scs:config/presets/logging.json", "files-scs:config/presets/representation-conversion.json", diff --git a/config/config-file.json b/config/config-file.json index 3bf4e9372..5de3a1139 100644 --- a/config/config-file.json +++ b/config/config-file.json @@ -8,6 +8,7 @@ "files-scs:config/presets/ldp/metadata-handler.json", "files-scs:config/presets/ldp/operation-handler.json", "files-scs:config/presets/ldp/permissions-extractor.json", + "files-scs:config/presets/ldp/response-writer.json", "files-scs:config/presets/ldp/request-parser.json", "files-scs:config/presets/logging.json", "files-scs:config/presets/representation-conversion.json", diff --git a/config/config-path-routing.json b/config/config-path-routing.json index 3187a91d0..efadc2c72 100644 --- a/config/config-path-routing.json +++ b/config/config-path-routing.json @@ -8,6 +8,7 @@ "files-scs:config/presets/ldp/metadata-handler.json", "files-scs:config/presets/ldp/operation-handler.json", "files-scs:config/presets/ldp/permissions-extractor.json", + "files-scs:config/presets/ldp/response-writer.json", "files-scs:config/presets/ldp/request-parser.json", "files-scs:config/presets/logging.json", "files-scs:config/presets/representation-conversion.json", diff --git a/config/config-rdf-to-sparql-endpoint.json b/config/config-rdf-to-sparql-endpoint.json index f262919d3..0b2ec8d82 100644 --- a/config/config-rdf-to-sparql-endpoint.json +++ b/config/config-rdf-to-sparql-endpoint.json @@ -8,6 +8,7 @@ "files-scs:config/presets/ldp/metadata-handler.json", "files-scs:config/presets/ldp/operation-handler.json", "files-scs:config/presets/ldp/permissions-extractor.json", + "files-scs:config/presets/ldp/response-writer.json", "files-scs:config/presets/ldp/request-parser.json", "files-scs:config/presets/logging.json", "files-scs:config/presets/representation-conversion.json", diff --git a/config/config-sparql-endpoint.json b/config/config-sparql-endpoint.json index 174d35c18..4f5ea3389 100644 --- a/config/config-sparql-endpoint.json +++ b/config/config-sparql-endpoint.json @@ -8,6 +8,7 @@ "files-scs:config/presets/ldp/metadata-handler.json", "files-scs:config/presets/ldp/operation-handler.json", "files-scs:config/presets/ldp/permissions-extractor.json", + "files-scs:config/presets/ldp/response-writer.json", "files-scs:config/presets/ldp/request-parser.json", "files-scs:config/presets/logging.json", "files-scs:config/presets/representation-conversion.json", diff --git a/config/presets/ldp.json b/config/presets/ldp.json index 0471a27dd..9ad544f54 100644 --- a/config/presets/ldp.json +++ b/config/presets/ldp.json @@ -20,7 +20,7 @@ "@id": "urn:solid-server:default:OperationHandler" }, "AuthenticatedLdpHandler:_responseWriter": { - "@type": "BasicResponseWriter" + "@id": "urn:solid-server:default:ResponseWriter" } } ] diff --git a/config/presets/ldp/response-writer.json b/config/presets/ldp/response-writer.json new file mode 100644 index 000000000..94a92f34d --- /dev/null +++ b/config/presets/ldp/response-writer.json @@ -0,0 +1,17 @@ +{ + "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld", + "@graph": [ + { + "@id": "urn:solid-server:default:ResponseWriter", + "@type": "CompositeAsyncHandler", + "CompositeAsyncHandler:_handlers": [ + { + "@type": "ErrorResponseWriter" + }, + { + "@type": "BasicResponseWriter" + } + ] + } + ] +} diff --git a/index.ts b/index.ts index 3108e053e..35c80c9e9 100644 --- a/index.ts +++ b/index.ts @@ -28,6 +28,7 @@ export * from './src/ldp/http/BasicRequestParser'; export * from './src/ldp/http/BasicResponseWriter'; export * from './src/ldp/http/BasicTargetExtractor'; export * from './src/ldp/http/BodyParser'; +export * from './src/ldp/http/ErrorResponseWriter'; export * from './src/ldp/http/Patch'; export * from './src/ldp/http/PreferenceParser'; export * from './src/ldp/http/RawBodyParser'; diff --git a/src/ldp/http/BasicResponseWriter.ts b/src/ldp/http/BasicResponseWriter.ts index f62e5b3f6..2870f95a0 100644 --- a/src/ldp/http/BasicResponseWriter.ts +++ b/src/ldp/http/BasicResponseWriter.ts @@ -1,49 +1,37 @@ import { getLoggerFor } from '../../logging/LogUtil'; import type { HttpResponse } from '../../server/HttpResponse'; -import { HttpError } from '../../util/errors/HttpError'; import { UnsupportedHttpError } from '../../util/errors/UnsupportedHttpError'; import type { ResponseDescription } from '../operations/ResponseDescription'; import { ResponseWriter } from './ResponseWriter'; /** - * Writes to an {@link HttpResponse} based on the incoming {@link ResponseDescription} or error. + * Writes to an {@link HttpResponse} based on the incoming {@link ResponseDescription}. * Still needs a way to write correct status codes for successful operations. */ export class BasicResponseWriter extends ResponseWriter { protected readonly logger = getLoggerFor(this); public async canHandle(input: { response: HttpResponse; result: ResponseDescription | Error }): Promise { - if (!(input.result instanceof Error) && input.result.body && !input.result.body.binary) { - this.logger.warn('This writer can only write binary bodies and errors'); - throw new UnsupportedHttpError('Only binary results and errors are supported'); + if ((input.result instanceof Error) || (input.result.body && !input.result.body.binary)) { + this.logger.warn('This writer can only write binary bodies'); + throw new UnsupportedHttpError('Only binary results are supported'); } } - public async handle(input: { response: HttpResponse; result: ResponseDescription | Error }): Promise { - if (input.result instanceof Error) { - let code = 500; - if (input.result instanceof HttpError) { - code = input.result.statusCode; - } - input.response.setHeader('content-type', 'text/plain'); - input.response.writeHead(code); - input.response.end(typeof input.result.stack === 'string' ? - `${input.result.stack}\n` : - `${input.result.name}: ${input.result.message}\n`); + public async handle(input: { response: HttpResponse; result: ResponseDescription }): Promise { + input.response.setHeader('location', input.result.identifier.path); + if (input.result.body) { + const contentType = input.result.body.metadata.contentType ?? 'text/plain'; + input.response.setHeader('content-type', contentType); + } + + input.response.writeHead(200); + + if (input.result.body) { + input.result.body.data.pipe(input.response); } else { - input.response.setHeader('location', input.result.identifier.path); - if (input.result.body) { - const contentType = input.result.body.metadata.contentType ?? 'text/plain'; - input.response.setHeader('content-type', contentType); - input.result.body.data.pipe(input.response); - } - - input.response.writeHead(200); - - if (!input.result.body) { - // If there is an input body the response will end once the input stream ends - input.response.end(); - } + // If there is an input body the response will end once the input stream ends + input.response.end(); } } } diff --git a/src/ldp/http/ErrorResponseWriter.ts b/src/ldp/http/ErrorResponseWriter.ts new file mode 100644 index 000000000..f84e0b07a --- /dev/null +++ b/src/ldp/http/ErrorResponseWriter.ts @@ -0,0 +1,32 @@ +import { getLoggerFor } from '../../logging/LogUtil'; +import type { HttpResponse } from '../../server/HttpResponse'; +import { HttpError } from '../../util/errors/HttpError'; +import { UnsupportedHttpError } from '../../util/errors/UnsupportedHttpError'; +import type { ResponseDescription } from '../operations/ResponseDescription'; +import { ResponseWriter } from './ResponseWriter'; + +/** + * Writes to an {@link HttpResponse} based on the incoming Error. + */ +export class ErrorResponseWriter extends ResponseWriter { + protected readonly logger = getLoggerFor(this); + + public async canHandle(input: { response: HttpResponse; result: ResponseDescription | Error }): Promise { + if (!(input.result instanceof Error)) { + this.logger.warn('This writer can only write errors'); + throw new UnsupportedHttpError('Only errors are supported'); + } + } + + public async handle(input: { response: HttpResponse; result: Error }): Promise { + let code = 500; + if (input.result instanceof HttpError) { + code = input.result.statusCode; + } + input.response.setHeader('content-type', 'text/plain'); + input.response.writeHead(code); + input.response.end(typeof input.result.stack === 'string' ? + `${input.result.stack}\n` : + `${input.result.name}: ${input.result.message}\n`); + } +} diff --git a/test/configs/AuthenticatedDataAccessorBasedConfig.ts b/test/configs/AuthenticatedDataAccessorBasedConfig.ts index a39c77887..5949457a3 100644 --- a/test/configs/AuthenticatedDataAccessorBasedConfig.ts +++ b/test/configs/AuthenticatedDataAccessorBasedConfig.ts @@ -5,7 +5,6 @@ import type { } from '../../index'; import { AuthenticatedLdpHandler, - BasicResponseWriter, CompositeAsyncHandler, MethodPermissionsExtractor, RdfToQuadConverter, @@ -19,6 +18,7 @@ import { getOperationHandler, getWebAclAuthorizer, getDataAccessorStore, + getResponseWriter, } from './Util'; /** @@ -50,7 +50,7 @@ export class AuthenticatedDataAccessorBasedConfig implements ServerConfig { const operationHandler = getOperationHandler(this.store); - const responseWriter = new BasicResponseWriter(); + const responseWriter = getResponseWriter(); const authorizer = getWebAclAuthorizer(this.store, this.base); const handler = new AuthenticatedLdpHandler({ diff --git a/test/configs/BasicConfig.ts b/test/configs/BasicConfig.ts index ec22b9b1f..6948bb7fe 100644 --- a/test/configs/BasicConfig.ts +++ b/test/configs/BasicConfig.ts @@ -3,12 +3,11 @@ import type { HttpHandler, import { AllowEverythingAuthorizer, AuthenticatedLdpHandler, - BasicResponseWriter, MethodPermissionsExtractor, UnsecureWebIdExtractor, } from '../../index'; import type { ServerConfig } from './ServerConfig'; -import { getOperationHandler, getInMemoryResourceStore, getBasicRequestParser } from './Util'; +import { getOperationHandler, getInMemoryResourceStore, getBasicRequestParser, getResponseWriter } from './Util'; /** * BasicConfig works with @@ -33,7 +32,7 @@ export class BasicConfig implements ServerConfig { const operationHandler = getOperationHandler(this.store); - const responseWriter = new BasicResponseWriter(); + const responseWriter = getResponseWriter(); const handler = new AuthenticatedLdpHandler({ requestParser, diff --git a/test/configs/BasicHandlersConfig.ts b/test/configs/BasicHandlersConfig.ts index c37e34d96..dec1f7a55 100644 --- a/test/configs/BasicHandlersConfig.ts +++ b/test/configs/BasicHandlersConfig.ts @@ -3,7 +3,6 @@ import type { HttpHandler, import { AllowEverythingAuthorizer, AuthenticatedLdpHandler, - BasicResponseWriter, CompositeAsyncHandler, MethodPermissionsExtractor, QuadToRdfConverter, @@ -21,6 +20,7 @@ import { getConvertingStore, getPatchingStore, getBasicRequestParser, + getResponseWriter, } from './Util'; /** @@ -56,7 +56,7 @@ export class BasicHandlersConfig implements ServerConfig { const operationHandler = getOperationHandler(this.store); - const responseWriter = new BasicResponseWriter(); + const responseWriter = getResponseWriter(); const handler = new AuthenticatedLdpHandler({ requestParser, diff --git a/test/configs/BasicHandlersWithAclConfig.ts b/test/configs/BasicHandlersWithAclConfig.ts index 49b280e1c..49215944c 100644 --- a/test/configs/BasicHandlersWithAclConfig.ts +++ b/test/configs/BasicHandlersWithAclConfig.ts @@ -2,7 +2,6 @@ import type { HttpHandler, ResourceStore } from '../../index'; import { AuthenticatedLdpHandler, - BasicResponseWriter, CompositeAsyncHandler, MethodPermissionsExtractor, RdfToQuadConverter, @@ -16,6 +15,7 @@ import { getBasicRequestParser, getOperationHandler, getWebAclAuthorizer, + getResponseWriter, } from './Util'; /** @@ -46,7 +46,7 @@ export class BasicHandlersWithAclConfig implements ServerConfig { const operationHandler = getOperationHandler(this.store); - const responseWriter = new BasicResponseWriter(); + const responseWriter = getResponseWriter(); const authorizer = getWebAclAuthorizer(this.store); const handler = new AuthenticatedLdpHandler({ diff --git a/test/configs/DataAccessorBasedConfig.ts b/test/configs/DataAccessorBasedConfig.ts index 36e982188..43ca3a397 100644 --- a/test/configs/DataAccessorBasedConfig.ts +++ b/test/configs/DataAccessorBasedConfig.ts @@ -6,7 +6,6 @@ import type { import { AllowEverythingAuthorizer, AuthenticatedLdpHandler, - BasicResponseWriter, CompositeAsyncHandler, MethodPermissionsExtractor, QuadToRdfConverter, @@ -20,6 +19,7 @@ import { getConvertingStore, getBasicRequestParser, getDataAccessorStore, + getResponseWriter, } from './Util'; /** @@ -50,7 +50,7 @@ export class DataAccessorBasedConfig implements ServerConfig { const authorizer = new AllowEverythingAuthorizer(); const operationHandler = getOperationHandler(this.store); - const responseWriter = new BasicResponseWriter(); + const responseWriter = getResponseWriter(); const handler = new AuthenticatedLdpHandler({ requestParser, diff --git a/test/configs/Util.ts b/test/configs/Util.ts index 529c2259a..b6c7634fe 100644 --- a/test/configs/Util.ts +++ b/test/configs/Util.ts @@ -6,16 +6,20 @@ import type { RepresentationConverter, ResourceStore, ResponseDescription, + HttpResponse, + ResponseWriter, } from '../../index'; import { AcceptPreferenceParser, BasicMetadataExtractor, BasicRequestParser, + BasicResponseWriter, BasicTargetExtractor, CompositeAsyncHandler, ContentTypeParser, DataAccessorBasedStore, DeleteOperationHandler, + ErrorResponseWriter, GetOperationHandler, HeadOperationHandler, InMemoryDataAccessor, @@ -113,6 +117,12 @@ export const getOperationHandler = (store: ResourceStore): CompositeAsyncHandler return new CompositeAsyncHandler(handlers); }; +export const getResponseWriter = (): ResponseWriter => + new CompositeAsyncHandler<{ response: HttpResponse; result: ResponseDescription | Error }, void>([ + new ErrorResponseWriter(), + new BasicResponseWriter(), + ]); + /** * Creates a BasicMetadataExtractor with parsers for content-type, slugs and link types. */ diff --git a/test/unit/ldp/http/BasicResponseWriter.test.ts b/test/unit/ldp/http/BasicResponseWriter.test.ts index 9dcf5df26..2a4fde3d3 100644 --- a/test/unit/ldp/http/BasicResponseWriter.test.ts +++ b/test/unit/ldp/http/BasicResponseWriter.test.ts @@ -16,7 +16,9 @@ describe('A BasicResponseWriter', (): void => { response = createResponse({ eventEmitter: EventEmitter }); }); - it('requires the description body to be a string or binary stream if present.', async(): Promise => { + it('requires a description body, with a binary stream if there is data.', async(): Promise => { + await expect(writer.canHandle({ response, result: new Error('error') })) + .rejects.toThrow(UnsupportedHttpError); await expect(writer.canHandle({ response, result: { body: { binary: false }} as ResponseDescription })) .rejects.toThrow(UnsupportedHttpError); await expect(writer.canHandle({ response, result: { body: { binary: true }} as ResponseDescription })) @@ -72,34 +74,4 @@ describe('A BasicResponseWriter', (): void => { await writer.handle({ response, result: { identifier: { path: 'path' }, body }}); await end; }); - - it('responds with 500 if an error if there is an error.', async(): Promise => { - await writer.handle({ response, result: new Error('error') }); - expect(response._isEndCalled()).toBeTruthy(); - expect(response._getStatusCode()).toBe(500); - expect(response._getData()).toMatch('Error: error'); - }); - - it('responds with the given statuscode if there is an HttpError.', async(): Promise => { - const error = new UnsupportedHttpError('error'); - await writer.handle({ response, result: error }); - expect(response._isEndCalled()).toBeTruthy(); - expect(response._getStatusCode()).toBe(error.statusCode); - expect(response._getData()).toMatch('UnsupportedHttpError: error'); - }); - - it('responds with the error name and message when no stack trace is lazily generated.', async(): Promise => { - const error = new Error('error'); - error.stack = undefined; - await writer.handle({ response, result: error }); - expect(response._isEndCalled()).toBeTruthy(); - expect(response._getStatusCode()).toBe(500); - expect(response._getData()).toMatch('Error: error'); - }); - - it('ends its response with a newline if there is an error.', async(): Promise => { - await writer.handle({ response, result: new Error('error') }); - expect(response._isEndCalled()).toBeTruthy(); - expect(response._getData().endsWith('\n')).toBeTruthy(); - }); }); diff --git a/test/unit/ldp/http/ErrorResponseWriter.test.ts b/test/unit/ldp/http/ErrorResponseWriter.test.ts new file mode 100644 index 000000000..e348a5f49 --- /dev/null +++ b/test/unit/ldp/http/ErrorResponseWriter.test.ts @@ -0,0 +1,54 @@ +import { EventEmitter } from 'events'; +import type { MockResponse } from 'node-mocks-http'; +import { createResponse } from 'node-mocks-http'; +import { ErrorResponseWriter } from '../../../../src/ldp/http/ErrorResponseWriter'; +import type { ResponseDescription } from '../../../../src/ldp/operations/ResponseDescription'; +import { UnsupportedHttpError } from '../../../../src/util/errors/UnsupportedHttpError'; + +describe('An ErrorResponseWriter', (): void => { + const writer = new ErrorResponseWriter(); + let response: MockResponse; + + beforeEach(async(): Promise => { + response = createResponse({ eventEmitter: EventEmitter }); + }); + + it('requires the input to be an error.', async(): Promise => { + await expect(writer.canHandle({ response, result: new Error('error') })) + .resolves.toBeUndefined(); + await expect(writer.canHandle({ response, result: { body: { binary: false }} as ResponseDescription })) + .rejects.toThrow(UnsupportedHttpError); + await expect(writer.canHandle({ response, result: { body: { binary: true }} as ResponseDescription })) + .rejects.toThrow(UnsupportedHttpError); + }); + + it('responds with 500 if an error if there is an error.', async(): Promise => { + await writer.handle({ response, result: new Error('error') }); + expect(response._isEndCalled()).toBeTruthy(); + expect(response._getStatusCode()).toBe(500); + expect(response._getData()).toMatch('Error: error'); + }); + + it('responds with the given statuscode if there is an HttpError.', async(): Promise => { + const error = new UnsupportedHttpError('error'); + await writer.handle({ response, result: error }); + expect(response._isEndCalled()).toBeTruthy(); + expect(response._getStatusCode()).toBe(error.statusCode); + expect(response._getData()).toMatch('UnsupportedHttpError: error'); + }); + + it('responds with the error name and message when no stack trace is lazily generated.', async(): Promise => { + const error = new Error('error'); + error.stack = undefined; + await writer.handle({ response, result: error }); + expect(response._isEndCalled()).toBeTruthy(); + expect(response._getStatusCode()).toBe(500); + expect(response._getData()).toMatch('Error: error'); + }); + + it('ends its response with a newline if there is an error.', async(): Promise => { + await writer.handle({ response, result: new Error('error') }); + expect(response._isEndCalled()).toBeTruthy(); + expect(response._getData().endsWith('\n')).toBeTruthy(); + }); +});