fix review comments

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

packages/errors/package.json

@@ -30,7 +30,8 @@
   },
   "dependencies": {
     "@backstage/config": "^0.1.2",
-    "cross-fetch": "^3.0.6"
+    "cross-fetch": "^3.0.6",
+    "serialize-error": "^8.0.1"
   },
   "devDependencies": {
     "@backstage/cli": "^0.6.1",

packages/errors/src/error/server.ts

@@ -1,43 +0,0 @@
-/*
- * Copyright 2021 Spotify AB
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-import {
-  parseServerResponseErrorBody,
-  ServerResponseErrorBody,
-} from '../payload';
-
-/**
- * An error thrown as the result of a failed server request.
- *
- * The server is expected to respond on the ServerResponseErrorBody format.
- */
-export class ServerResponseError extends Error {
-  static async forResponse(response: Response): Promise<ServerResponseError> {
-    const body = await parseServerResponseErrorBody(response);
-    const status = body.error.statusCode || response.status;
-    const statusText = body.error.name || response.statusText;
-    const message = `Request failed with ${status} ${statusText}`;
-    return new ServerResponseError(message, body);
-  }
-
-  readonly body: ServerResponseErrorBody;
-
-  constructor(message: string, body: ServerResponseErrorBody) {
-    super(message);
-    this.name = 'ServerResponseError';
-    this.body = body;
-  }
-}

packages/errors/src/errors/ResponseError.test.ts

@@ -0,0 +1,42 @@
+/*
+ * Copyright 2021 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { ErrorResponse } from '../serialization';
+import { ResponseError } from './ResponseError';
+
+describe('ResponseError', () => {
+  it('constructs itself from a response', async () => {
+    const body: ErrorResponse = {
+      error: { name: 'Fours', message: 'Expected fives', stack: 'lines' },
+      request: { method: 'GET', url: '/' },
+      response: { statusCode: 444 },
+    };
+
+    const response: Partial<Response> = {
+      status: 444,
+      statusText: 'Fours',
+      text: async () => JSON.stringify(body),
+      headers: new Headers({ 'Content-Type': 'application/json' }),
+    };
+
+    const e = await ResponseError.fromResponse(response as Response);
+    expect(e.name).toEqual('ResponseError');
+    expect(e.message).toEqual('Request failed with 444 Fours');
+    expect(e.cause.name).toEqual('Fours');
+    expect(e.cause.message).toEqual('Expected fives');
+    expect(e.cause.stack).toEqual('lines');
+  });
+});

packages/errors/src/errors/ResponseError.ts

@@ -0,0 +1,88 @@
+/*
+ * Copyright 2021 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import {
+  parseErrorResponse,
+  ErrorResponse,
+  deserializeError,
+} from '../serialization';
+
+/**
+ * An error thrown as the result of a failed server request.
+ *
+ * The server is expected to respond on the ErrorResponse format.
+ */
+export class ResponseError extends Error {
+  /**
+   * The actual response, as seen by the client.
+   *
+   * Note that the body of this response is always consumed. Its parsed form is
+   * in the `body` field.
+   */
+  readonly response: Response;
+
+  /**
+   * The parsed JSON error body, as sent by the server.
+   */
+  readonly data: ErrorResponse;
+
+  /**
+   * The Error cause, as seen by the remote server. This is parsed out of the
+   * JSON error body.
+   *
+   * This error always has the plain Error constructor, however all
+   * serializable enumerable fields on the remote error including its name are
+   * preserved. Therefore, if you want to check the error type, use its name
+   * property rather than checking typeof or its constructor or prototype.
+   */
+  readonly cause: Error;
+
+  /**
+   * Constructs a ResponseError based on a failed response.
+   *
+   * Assumes that the response has already been checked to be not ok. This
+   * function consumes the body of the response, and assumes that it hasn't
+   * been consumed before.
+   */
+  static async fromResponse(response: Response): Promise<ResponseError> {
+    const data = await parseErrorResponse(response);
+
+    const status = data.response.statusCode || response.status;
+    const statusText = data.error.name || response.statusText;
+    const message = `Request failed with ${status} ${statusText}`;
+    const cause = deserializeError(data.error);
+
+    return new ResponseError({
+      message,
+      response,
+      data,
+      cause,
+    });
+  }
+
+  constructor(props: {
+    message: string;
+    response: Response;
+    data: ErrorResponse;
+    cause: Error;
+  }) {
+    super(props.message);
+    this.name = 'ResponseError';
+    this.response = props.response;
+    this.data = props.data;
+    this.cause = props.cause;
+  }
+}

packages/errors/src/errors/index.ts

@@ -15,11 +15,12 @@
  */
 
 export {
-  InputError,
   AuthenticationError,
+  ConflictError,
+  InputError,
   NotAllowedError,
   NotFoundError,
-  ConflictError,
   NotModifiedError,
 } from './common';
-export { ServerResponseError } from './server';
+export { CustomErrorBase } from './CustomErrorBase';
+export { ResponseError } from './ResponseError';

packages/errors/src/index.ts

@@ -14,5 +14,5 @@
  * limitations under the License.
  */
 
-export * from './error';
-export * from './payload';
+export * from './errors';
+export * from './serialization';

packages/errors/src/serialization/error.test.ts

@@ -0,0 +1,64 @@
+/*
+ * Copyright 2021 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { deserializeError, serializeError } from './error';
+
+class CustomError extends Error {
+  readonly customField: any;
+  constructor(message: string) {
+    super(message);
+    this.name = 'CustomError';
+    this.customField = { complex: 'data' };
+  }
+}
+
+describe('serialization', () => {
+  it('runs the happy path with stack', () => {
+    const before = new CustomError('m');
+    const after = deserializeError<CustomError>(
+      serializeError(before, { includeStack: true }),
+    );
+    expect(after.message).toEqual(before.message);
+    expect(after.name).toEqual(before.name);
+    expect(after.stack).toEqual(before.stack);
+    expect(after.customField).toEqual(before.customField);
+    // Does not reconstruct the actual class!
+    expect(after.constructor).not.toBe(before.constructor);
+  });
+
+  it('runs the happy path without stack', () => {
+    const before = new CustomError('m');
+    const after = deserializeError<CustomError>(
+      serializeError(before, { includeStack: false }),
+    );
+    expect(after.message).toEqual(before.message);
+    expect(after.name).toEqual(before.name);
+    expect(after.stack).not.toEqual(before.stack);
+    expect(after.customField).toEqual(before.customField);
+    // Does not reconstruct the actual class!
+    expect(after.constructor).not.toBe(before.constructor);
+  });
+
+  it('serializes stack traces only when allowed', () => {
+    const before = new CustomError('m');
+    const withStack = serializeError(before, { includeStack: true });
+    const withoutStack1 = serializeError(before, { includeStack: false });
+    const withoutStack2 = serializeError(before);
+    expect(withStack.stack).toEqual(before.stack);
+    expect(withoutStack1.stack).not.toBeDefined();
+    expect(withoutStack2.stack).not.toBeDefined();
+  });
+});

packages/errors/src/serialization/error.ts

@@ -0,0 +1,72 @@
+/*
+ * Copyright 2021 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { JsonObject } from '@backstage/config';
+import {
+  deserializeError as deserializeErrorInternal,
+  serializeError as serializeErrorInternal,
+} from 'serialize-error';
+
+/**
+ * The serialized form of an Error.
+ */
+export type SerializedError = JsonObject & {
+  /** The name of the exception that was thrown */
+  name: string;
+  /** The message of the exception that was thrown */
+  message: string;
+  /** A stringified stack trace; may not be present */
+  stack?: string;
+  /** A custom code (not necessarily the same as an HTTP response code); may not be present */
+  code?: string;
+};
+
+/**
+ * Serializes an error object to a JSON friendly form.
+ *
+ * @param error The error
+ * @param options.includeStackTraces: Include stack trace in the output (default false)
+ */
+export function serializeError(
+  error: Error,
+  options?: { includeStack?: boolean },
+): SerializedError {
+  const serialized = serializeErrorInternal(error);
+  const result: SerializedError = {
+    name: 'Unknown',
+    message: '<no reason given>',
+    ...serialized,
+  };
+
+  if (!options?.includeStack) {
+    delete result.stack;
+  }
+
+  return result;
+}
+
+/**
+ * Deserializes a serialized error object back to an Error.
+ */
+export function deserializeError<T extends Error = Error>(
+  data: SerializedError,
+): T {
+  const result = deserializeErrorInternal(data) as T;
+  if (!data.stack) {
+    result.stack = undefined;
+  }
+  return result;
+}

packages/errors/src/serialization/index.ts

@@ -14,5 +14,7 @@
  * limitations under the License.
  */
 
-export { parseServerResponseErrorBody } from './ServerResponseErrorBody';
-export type { ServerResponseErrorBody } from './ServerResponseErrorBody';
+export { deserializeError, serializeError } from './error';
+export type { SerializedError } from './error';
+export { parseErrorResponse } from './response';
+export type { ErrorResponse } from './response';

packages/errors/src/serialization/response.test.ts

@@ -14,24 +14,14 @@
  * limitations under the License.
  */
 
-import {
-  parseServerResponseErrorBody,
-  ServerResponseErrorBody,
-} from './ServerResponseErrorBody';
+import { parseErrorResponse, ErrorResponse } from './response';
 
-describe('parseServerResponseErrorBody', () => {
+describe('parseErrorResponse', () => {
   it('handles the happy path', async () => {
-    const body: ServerResponseErrorBody = {
-      error: {
-        statusCode: 444,
-        name: 'Fours',
-        message: 'Expected fives',
-        stack: 'lines',
-      },
-      request: {
-        method: 'GET',
-        url: '/',
-      },
+    const body: ErrorResponse = {
+      error: { name: 'Fours', message: 'Expected fives', stack: 'lines' },
+      request: { method: 'GET', url: '/' },
+      response: { statusCode: 444 },
     };
 
     const response: Partial<Response> = {
@@ -41,22 +31,16 @@ describe('parseServerResponseErrorBody', () => {
       headers: new Headers({ 'Content-Type': 'application/json' }),
     };
 
-    await expect(
-      parseServerResponseErrorBody(response as Response),
-    ).resolves.toEqual(body);
+    await expect(parseErrorResponse(response as Response)).resolves.toEqual(
+      body,
+    );
   });
 
   it('uses request header and text body when wrong content type, even if parsable', async () => {
-    const body: ServerResponseErrorBody = {
-      error: {
-        statusCode: 333,
-        name: 'Threes',
-        message: 'Expected twos',
-      },
-      request: {
-        method: 'GET',
-        url: '/',
-      },
+    const body: ErrorResponse = {
+      error: { name: 'Threes', message: 'Expected twos' },
+      request: { method: 'GET', url: '/' },
+      response: { statusCode: 333 },
     };
 
     const response: Partial<Response> = {
@@ -66,30 +50,22 @@ describe('parseServerResponseErrorBody', () => {
       headers: new Headers({ 'Content-Type': 'not-application/not-json' }),
     };
 
-    await expect(
-      parseServerResponseErrorBody(response as Response),
-    ).resolves.toEqual({
+    await expect(parseErrorResponse(response as Response)).resolves.toEqual({
       error: {
-        statusCode: 444,
         name: 'Unknown',
         message: `Request failed with status 444 Fours, ${JSON.stringify(
           body,
         )}`,
       },
+      response: { statusCode: 444 },
     });
   });
 
   it('uses request header and text body when not parsable', async () => {
-    const body: ServerResponseErrorBody = {
-      error: {
-        statusCode: 333,
-        name: 'Threes',
-        message: 'Expected twos',
-      },
-      request: {
-        method: 'GET',
-        url: '/',
-      },
+    const body: ErrorResponse = {
+      error: { name: 'Threes', message: 'Expected twos' },
+      request: { method: 'GET', url: '/' },
+      response: { statusCode: 333 },
     };
 
     const response: Partial<Response> = {
@@ -99,16 +75,14 @@ describe('parseServerResponseErrorBody', () => {
       headers: new Headers({ 'Content-Type': 'application/json' }),
     };
 
-    await expect(
-      parseServerResponseErrorBody(response as Response),
-    ).resolves.toEqual({
+    await expect(parseErrorResponse(response as Response)).resolves.toEqual({
       error: {
-        statusCode: 444,
         name: 'Unknown',
         message: `Request failed with status 444 Fours, ${JSON.stringify(
           body,
         ).substring(1)}`,
       },
+      response: { statusCode: 444 },
     });
   });
 
@@ -122,14 +96,12 @@ describe('parseServerResponseErrorBody', () => {
       headers: new Headers({ 'Content-Type': 'application/json' }),
     };
 
-    await expect(
-      parseServerResponseErrorBody(response as Response),
-    ).resolves.toEqual({
+    await expect(parseErrorResponse(response as Response)).resolves.toEqual({
       error: {
-        statusCode: 444,
         name: 'Unknown',
         message: `Request failed with status 444 Fours`,
       },
+      response: { statusCode: 444 },
     });
   });
 });

packages/errors/src/serialization/response.ts

@@ -14,43 +14,44 @@
  * limitations under the License.
  */
 
+import { SerializedError } from './error';
+
 /**
  * A standard shape of JSON data returned as the body of backend errors.
  */
-export type ServerResponseErrorBody = {
+export type ErrorResponse = {
   /** Details of the error that was caught */
-  error: {
-    /** The numeric HTTP status code that was returned */
-    statusCode: number;
-    /** The name of the exception that was thrown */
-    name: string;
-    /** The message of the exception that was thrown */
-    message: string;
-    /** A stringified stack trace, may not be present */
-    stack?: string;
-  };
+  error: SerializedError;
 
-  /** The incoming request */
+  /** Details about the incoming request */
   request?: {
     /** The HTTP method of the request */
     method: string;
     /** The URL of the request (excluding protocol and host/port) */
     url: string;
   };
+
+  /** Details about the response */
+  response: {
+    /** The numeric HTTP status code that was returned */
+    statusCode: number;
+  };
 };
 
 /**
- * Attempts to extract the ServerResponseErrorBody out of a server response.
- * This consumes the body of the response.
+ * Attempts to construct an ErrorResponse out of a failed server request.
+ * Assumes that the response has already been checked to be not ok. This
+ * function consumes the body of the response, and assumes that it hasn't
+ * been consumed before.
  *
  * The code is forgiving, and constructs a useful synthetic body as best it can
- * if the response wasn't on the expected form.
+ * if the response body wasn't on the expected form.
  *
  * @param response The response of a failed request
  */
-export async function parseServerResponseErrorBody(
+export async function parseErrorResponse(
   response: Response,
-): Promise<ServerResponseErrorBody> {
+): Promise<ErrorResponse> {
   try {
     const text = await response.text();
     if (text) {
@@ -59,7 +60,7 @@ export async function parseServerResponseErrorBody(
       ) {
         try {
           const body = JSON.parse(text);
-          if (body.error && body.request) {
+          if (body.error && body.response) {
             return body;
           }
         } catch {
@@ -69,10 +70,12 @@ export async function parseServerResponseErrorBody(
 
       return {
         error: {
-          statusCode: response.status,
           name: 'Unknown',
           message: `Request failed with status ${response.status} ${response.statusText}, ${text}`,
         },
+        response: {
+          statusCode: response.status,
+        },
       };
     }
   } catch {
@@ -81,9 +84,11 @@ export async function parseServerResponseErrorBody(
 
   return {
     error: {
-      statusCode: response.status,
       name: 'Unknown',
       message: `Request failed with status ${response.status} ${response.statusText}`,
     },
+    response: {
+      statusCode: response.status,
+    },
   };
 }