Add basic error handling

Author: francisco-videira-nhs

lambdas/api-handler/src/contracts/errors.ts

@@ -0,0 +1,52 @@
+import { randomUUID } from 'crypto';
+
+export interface ApiError {
+  id: string;
+  code: ApiErrorCode;
+  links: {about: 'https://digital.nhs.uk/developer/api-catalogue/nhs-notify-supplier'};
+  status: ApiErrorStatus;
+  title: ApiErrorTitle;
+  detail: ApiErrorDetail | string;
+}
+
+export enum ApiErrorCode {
+  InternalServerError = 'NOTIFY_INTERNAL_SERVER_ERROR',
+  InvalidRequest = 'NOTIFY_INVALID_REQUEST',
+  NotFound = 'NOTIFY_LETTER_NOT_FOUND'
+}
+
+export enum ApiErrorTitle {
+  InternalServerError = 'Internal server error',
+  InvalidRequest = 'Invalid request',
+  NotFound = 'Not found'
+}
+
+export enum ApiErrorStatus {
+  InternalServerError = "500",
+  InvalidRequest = "400",
+  NotFound = "404"
+}
+
+export enum ApiErrorDetail {
+  NotFoundLetterId = 'The provided letter ID does not exist',
+  InvalidRequestMissingSupplierId = 'The supplier ID is missing from the request',
+  InvalidRequestMissingBody = 'The request is missing the body',
+  InvalidRequestMissingLetterIdPathParameter = 'The request is missing the letter id path parameter',
+  InvalidRequestLetterIdsMismatch = 'The letter ID in the request body does not match the letter ID path parameter'
+}
+
+export function buildApiError(params: {
+  code: ApiErrorCode;
+  status: ApiErrorStatus;
+  title: ApiErrorTitle;
+  detail: ApiErrorDetail | string;
+}): ApiError {
+  return {
+    id: randomUUID(),
+    code: params.code,
+    links: { about: 'https://digital.nhs.uk/developer/api-catalogue/nhs-notify-supplier' },
+    status: params.status,
+    title: params.title,
+    detail: params.detail,
+  };
+}

lambdas/api-handler/src/errors/index.ts

@@ -1,3 +1,17 @@
-export class NotFoundError extends Error {}
-export class ValidationError extends Error {}
-export class ConflictError extends Error {}
+import { ApiErrorDetail } from "../contracts/errors";
+
+export class NotFoundError extends Error {
+  detail: ApiErrorDetail;
+  constructor(detail: ApiErrorDetail) {
+    super(detail);
+    this.detail = detail;
+  }
+}
+
+export class ValidationError extends Error {
+  detail: ApiErrorDetail;
+  constructor(detail: ApiErrorDetail) {
+    super(detail);
+    this.detail = detail;
+  }
+}

lambdas/api-handler/src/handlers/__tests__/patch-letters.test.ts

@@ -1,12 +1,26 @@
 import { patchLetters } from '../../index';
-import type { Context } from 'aws-lambda';
+import type { APIGatewayProxyResult, Context } from 'aws-lambda';
 import { mockDeep } from 'jest-mock-extended';
 import { makeApiGwEvent } from './utils/test-utils';
 import * as letterService from '../../services/letter-operations';
 import { NotFoundError, ValidationError } from '../../errors';
 import { LetterApiDocument, LetterApiStatus } from '../../contracts/letter-api';
+import { ErrorResponse, mapErrorToResponse } from '../../mappers/error-mapper';
 
 jest.mock('../../services/letter-operations');
+jest.mock('../../mappers/error-mapper');
+
+const mockedMapErrorToResponse = jest.mocked(mapErrorToResponse);
+const expectedErrorResponse: APIGatewayProxyResult = {
+  statusCode: 400,
+  body: "Error"
+};
+mockedMapErrorToResponse.mockReturnValue(expectedErrorResponse);
+
+const mockedPatchLetterStatus = jest.mocked(letterService.patchLetterStatus);
+
+const letterApiDocument = makeLetterApiDocument("id1", "REJECTED");
+const requestBody = JSON.stringify(letterApiDocument, null, 2);
 
 function makeLetterApiDocument(id: string, status: LetterApiStatus) : LetterApiDocument {
   return {
@@ -23,13 +37,13 @@ function makeLetterApiDocument(id: string, status: LetterApiStatus) : LetterApiD
   };
 }
 
-const letterApiDocument = makeLetterApiDocument("id1", "REJECTED");
-
-const requestBody = JSON.stringify(letterApiDocument, null, 2);
+beforeEach(() => {
+  jest.clearAllMocks();
+});
 
 describe('patchLetters API Handler', () => {
-  it('returns 200 OK with updated resource', async () => {
 
+  it('returns 200 OK with updated resource', async () => {
     const event = makeApiGwEvent({
       path: '/letters/id1',
       body: requestBody,
@@ -38,7 +52,6 @@ describe('patchLetters API Handler', () => {
     const context = mockDeep<Context>();
     const callback = jest.fn();
 
-    const mockedPatchLetterStatus = letterService.patchLetterStatus as jest.Mock;
     mockedPatchLetterStatus.mockResolvedValue(letterApiDocument);
 
     const result = await patchLetters(event, context, callback);
@@ -49,22 +62,20 @@ describe('patchLetters API Handler', () => {
     });
   });
 
-  it('returns 400 Bad Request as there is no body', async () => {
+  it('returns error response when there is no body', async () => {
     const event = makeApiGwEvent({
       path: '/letters/id1',
       pathParameters: {id: "id1"},
       headers: {'app-supplier-id': 'supplier1'}});
     const context = mockDeep<Context>();
     const callback = jest.fn();
+
     const result = await patchLetters(event, context, callback);
 
-    expect(result).toEqual({
-      statusCode: 400,
-      body: 'Bad Request: Missing request body',
-    });
+    expect(result).toEqual(expectedErrorResponse);
   });
 
-  it('returns 404 Not Found as path parameter is not found', async () => {
+  it('returns error response when path parameter letterId is not found', async () => {
     const event = makeApiGwEvent({
       path: '/letters/',
       body: requestBody,
@@ -73,15 +84,11 @@ describe('patchLetters API Handler', () => {
     const callback = jest.fn();
     const result = await patchLetters(event, context, callback);
 
-    expect(result).toEqual({
-      statusCode: 404,
-      body: 'Not Found: The requested resource does not exist',
-    });
+    expect(result).toEqual(expectedErrorResponse);
   });
 
-  it('returns 400 Bad Request when ValidationError is thrown by service', async () => {
-    const mockedPatchLetterStatus = letterService.patchLetterStatus as jest.Mock;
-    mockedPatchLetterStatus.mockRejectedValue(new ValidationError('Validation failed'));
+  it('returns error response when error is thrown by service', async () => {
+    mockedPatchLetterStatus.mockRejectedValue(new Error('Service error'));
 
     const event = makeApiGwEvent({
       path: '/letters/id1',
@@ -94,61 +101,19 @@ describe('patchLetters API Handler', () => {
 
     const result = await patchLetters(event, context, callback);
 
-    expect(result).toEqual({
-      statusCode: 400,
-      body: 'Validation failed'
-    });
+    expect(result).toEqual(expectedErrorResponse);
   });
 
-  it('returns 404 Not Found when NotFoundError is thrown by service', async () => {
-    const mockedPatchLetterStatus = letterService.patchLetterStatus as jest.Mock;
-    mockedPatchLetterStatus.mockRejectedValue(new NotFoundError('Letter not found'));
-
-    const event = makeApiGwEvent({
-      path: '/letters/id1',
-      body: requestBody,
-      pathParameters: {id: "id1"},
-      headers: {'app-supplier-id': 'supplier1'}
-    });
-    const context = mockDeep<Context>();
-    const callback = jest.fn();
-
-    const result = await patchLetters(event, context, callback);
-
-    expect(result).toEqual({
-      statusCode: 404,
-      body: 'Letter not found'
-    });
-  });
-
-  it('throws unexpected errors from service', async () => {
-    const mockedPatchLetterStatus = letterService.patchLetterStatus as jest.Mock;
-    mockedPatchLetterStatus.mockRejectedValue(new Error('Unexpected error'));
-
-    const event = makeApiGwEvent({
-      path: '/letters/id1',
-      body: requestBody,
-      pathParameters: {id: "id1"},
-      headers: {'app-supplier-id': 'supplier1'}
-    });
-    const context = mockDeep<Context>();
-    const callback = jest.fn();
-
-    await expect(patchLetters(event, context, callback)).rejects.toThrow('Unexpected error');
-  });
-
-  it('returns 400 Bad Request: Missing supplier ID if header app-supplier-id ', async () => {
+  it('returns error when app-supplier-id is missing', async () => {
     const event = makeApiGwEvent({
       path: '/letters/id1',
       body: requestBody,
       pathParameters: {id: "id1"}});
     const context = mockDeep<Context>();
     const callback = jest.fn();
+
     const result = await patchLetters(event, context, callback);
 
-    expect(result).toEqual({
-      statusCode: 400,
-      body: 'Bad Request: Missing supplier ID',
-    });
+    expect(result).toEqual(expectedErrorResponse);
   });
 });

lambdas/api-handler/src/handlers/patch-letters.ts

@@ -2,61 +2,35 @@ import { APIGatewayProxyHandler } from 'aws-lambda';
 import { createLetterRepository } from '../infrastructure/letter-repo-factory';
 import { patchLetterStatus } from '../services/letter-operations';
 import { LetterApiDocument } from '../contracts/letter-api';
-import { NotFoundError, ValidationError } from '../errors';
+import * as errors from '../contracts/errors';
+import { ValidationError } from '../errors';
+import { mapErrorToResponse } from '../mappers/error-mapper';
 
 const letterRepo = createLetterRepository();
 export const patchLetters: APIGatewayProxyHandler = async (event) => {
 
-  const supplierId = event.headers['app-supplier-id'];
-
-  if (!supplierId) {
-    return {
-      statusCode: 400,
-      body: "Bad Request: Missing supplier ID"
-    };
-  }
-
-  const letterId = event.pathParameters?.id;
-
-  if (!letterId) {
-    return {
-      statusCode: 404,
-      body: "Not Found: The requested resource does not exist"
-    };
-  }
-
-  if (!event.body)
-  {
-    return {
-      statusCode: 400,
-      body: "Bad Request: Missing request body"
-    }
-  }
-
-  const patchLetterRequest: LetterApiDocument = JSON.parse(event.body);
-
   try {
+    const supplierId = assertNotEmpty(event.headers['app-supplier-id'], errors.ApiErrorDetail.InvalidRequestMissingSupplierId);
+    const letterId = assertNotEmpty( event.pathParameters?.id, errors.ApiErrorDetail.InvalidRequestMissingLetterIdPathParameter);
+    const body = assertNotEmpty(event.body, errors.ApiErrorDetail.InvalidRequestMissingBody);
 
-    // TODO CCM-11188: Is it worth retrieving the letter first to check if the status is different?
+    const patchLetterRequest: LetterApiDocument = JSON.parse(body);
 
-    const result = await patchLetterStatus(patchLetterRequest.data, letterId, supplierId, letterRepo);
+    const result = await patchLetterStatus(patchLetterRequest.data, letterId!, supplierId!, letterRepo);
 
     return {
       statusCode: 200,
       body: JSON.stringify(result, null, 2)
     };
+
   } catch (error) {
-    if (error instanceof ValidationError) {
-      return {
-        statusCode: 400,
-        body: error.message
-      };
-    } else if (error instanceof NotFoundError) {
-      return {
-        statusCode: 404,
-        body: error.message
-      };
-    }
-    throw error;
+    return mapErrorToResponse(error);
   }
 };
+
+function assertNotEmpty(value: string | null | undefined, detail: errors.ApiErrorDetail): string {
+  if (!value || value.trim() === '') {
+    throw new ValidationError(detail);
+  }
+  return value;
+}