refactor: Split off ErrorResponseWriter

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",

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",

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",

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",

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",

config/presets/ldp.json

@@ -20,7 +20,7 @@
         "@id": "urn:solid-server:default:OperationHandler"
       },
       "AuthenticatedLdpHandler:_responseWriter": {
-        "@type": "BasicResponseWriter"
+        "@id": "urn:solid-server:default:ResponseWriter"
       }
     }
   ]

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"
+        }
+      ]
+    }
+  ]
+}

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';

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<void> {
-    if (!(input.result instanceof Error) && input.result.body && !input.result.body.binary) {
+    if ((input.result instanceof Error) || (input.result.body && !input.result.body.binary)) {
-      this.logger.warn('This writer can only write binary bodies and errors');
+      this.logger.warn('This writer can only write binary bodies');
-      throw new UnsupportedHttpError('Only binary results and errors are supported');
+      throw new UnsupportedHttpError('Only binary results are supported');
     }
   }
 
-  public async handle(input: { response: HttpResponse; result: ResponseDescription | Error }): Promise<void> {
+  public async handle(input: { response: HttpResponse; result: ResponseDescription }): Promise<void> {
-    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`);
-    } 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 (input.result.body) {
+      input.result.body.data.pipe(input.response);
+    } else {
       // If there is an input body the response will end once the input stream ends
       input.response.end();
     }
   }
-  }
 }

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<void> {
+    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<void> {
+    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`);
+  }
+}

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({

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,

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,

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({

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,

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<Operation, ResponseDescription>(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.
  */

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<void> => {
+  it('requires a description body, with a binary stream if there is data.', async(): Promise<void> => {
+    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<void> => {
-    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<void> => {
-    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<void> => {
-    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<void> => {
-    await writer.handle({ response, result: new Error('error') });
-    expect(response._isEndCalled()).toBeTruthy();
-    expect(response._getData().endsWith('\n')).toBeTruthy();
-  });
 });

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<any>;
+
+  beforeEach(async(): Promise<void> => {
+    response = createResponse({ eventEmitter: EventEmitter });
+  });
+
+  it('requires the input to be an error.', async(): Promise<void> => {
+    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<void> => {
+    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<void> => {
+    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<void> => {
+    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<void> => {
+    await writer.handle({ response, result: new Error('error') });
+    expect(response._isEndCalled()).toBeTruthy();
+    expect(response._getData().endsWith('\n')).toBeTruthy();
+  });
+});