Merge remote-tracking branch 'origin/feature/CCM-11602_get_endpoint' into feature/CCM-11188

Author: francisco-videira-nhs

infrastructure/terraform/components/api/README.md

@@ -18,9 +18,11 @@ No requirements.
 | <a name="input_force_lambda_code_deploy"></a> [force\_lambda\_code\_deploy](#input\_force\_lambda\_code\_deploy) | If the lambda package in s3 has the same commit id tag as the terraform build branch, the lambda will not update automatically. Set to True if making changes to Lambda code from on the same commit for example during development | `bool` | `false` | no |
 | <a name="input_group"></a> [group](#input\_group) | The group variables are being inherited from (often synonmous with account short-name) | `string` | n/a | yes |
 | <a name="input_kms_deletion_window"></a> [kms\_deletion\_window](#input\_kms\_deletion\_window) | When a kms key is deleted, how long should it wait in the pending deletion state? | `string` | `"30"` | no |
+| <a name="input_letter_table_ttl_hours"></a> [letter\_table\_ttl\_hours](#input\_letter\_table\_ttl\_hours) | Number of hours to set as TTL on letters table | `number` | `24` | no |
 | <a name="input_log_level"></a> [log\_level](#input\_log\_level) | The log level to be used in lambda functions within the component. Any log with a lower severity than the configured value will not be logged: https://docs.python.org/3/library/logging.html#levels | `string` | `"INFO"` | no |
 | <a name="input_log_retention_in_days"></a> [log\_retention\_in\_days](#input\_log\_retention\_in\_days) | The retention period in days for the Cloudwatch Logs events to be retained, default of 0 is indefinite | `number` | `0` | no |
 | <a name="input_manually_configure_mtls_truststore"></a> [manually\_configure\_mtls\_truststore](#input\_manually\_configure\_mtls\_truststore) | Manually manage the truststore used for API Gateway mTLS (e.g. for prod environment) | `bool` | `false` | no |
+| <a name="input_max_get_limit"></a> [max\_get\_limit](#input\_max\_get\_limit) | Default limit to apply to GET requests that support pagination | `number` | `2500` | no |
 | <a name="input_parent_acct_environment"></a> [parent\_acct\_environment](#input\_parent\_acct\_environment) | Name of the environment responsible for the acct resources used, affects things like DNS zone. Useful for named dev environments | `string` | `"main"` | no |
 | <a name="input_project"></a> [project](#input\_project) | The name of the tfscaffold project | `string` | n/a | yes |
 | <a name="input_region"></a> [region](#input\_region) | The AWS Region | `string` | n/a | yes |

infrastructure/terraform/components/api/module_lambda_get_letters.tf

@@ -35,7 +35,9 @@ module "get_letters" {
   log_destination_arn       = local.destination_arn
   log_subscription_role_arn = local.acct.log_subscription_role_arn
 
-  lambda_env_vars = merge(local.common_db_access_lambda_env_vars, {})
+  lambda_env_vars = merge(local.common_db_access_lambda_env_vars, {
+    MAX_LIMIT = var.max_get_limit
+  })
 }
 
 data "aws_iam_policy_document" "get_letters_lambda" {

infrastructure/terraform/components/api/variables.tf

@@ -99,9 +99,20 @@ variable "enable_backups" {
   default     = false
 }
 
-
 variable "ca_pem_filename" {
   type        = string
   description = "Filename for the CA truststore file within the s3 bucket"
   default     = null
 }
+
+variable "letter_table_ttl_hours" {
+  type        = number
+  description = "Number of hours to set as TTL on letters table"
+  default     = 24
+}
+
+variable "max_get_limit" {
+  type        = number
+  description = "Default limit to apply to GET requests that support pagination"
+  default     = 2500
+}

internal/datastore/src/types.md

@@ -10,19 +10,19 @@ The schemas are generated from Zod definitions and provide a visual representati
 erDiagram
     Letter {
         string id
-        string supplierId "ref: Supplier"
+        string status "enum: PENDING, ACCEPTED, REJECTED, PRINTED, ENCLOSED, CANCELLED, DISPATCHED, FAILED, RETURNED, DESTROYED, FORWARDED, DELIVERED"
         string specificationId
         string groupId
+        number reasonCode
+        string reasonText
+        string supplierId
         string url "url"
-        string status "enum: PENDING, ACCEPTED, REJECTED, PRINTED, ENCLOSED, CANCELLED, DISPATCHED, FAILED, RETURNED, DESTROYED, FORWARDED, DELIVERED"
         string createdAt
         string updatedAt
         string supplierStatus
+        string supplierStatusSk
         number ttl "min: -9007199254740991, max: 9007199254740991"
     }
-    Supplier {
-    }
-    Letter }o--|| Supplier : "supplierId"
 ```
 
 ## MI schema

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

@@ -4,6 +4,9 @@ import { mockDeep } from 'jest-mock-extended';
 import { makeApiGwEvent } from './utils/test-utils';
 import * as letterService from '../../services/letter-operations';
 import { mapErrorToResponse } from '../../mappers/error-mapper';
+import { getEnvars } from '../get-letters';
+import { ValidationError } from '../../errors';
+import * as errors from '../../contracts/errors';
 
 jest.mock('../../mappers/error-mapper');
 const mockedMapErrorToResponse = jest.mocked(mapErrorToResponse);
@@ -13,6 +16,7 @@ const expectedErrorResponse: APIGatewayProxyResult = {
 };
 mockedMapErrorToResponse.mockReturnValue(expectedErrorResponse);
 
+
 jest.mock('../../services/letter-operations');
 
 jest.mock("../../config/lambda-config", () => ({
@@ -23,8 +27,21 @@ jest.mock("../../config/lambda-config", () => ({
 
 describe('API Lambda handler', () => {
 
+  const originalEnv = process.env;
+
   beforeEach(() => {
     jest.clearAllMocks();
+    jest.resetModules();
+    process.env = { ...originalEnv };
+    process.env.MAX_LIMIT = '2500';
+  });
+
+  afterEach(() => {
+    process.env = originalEnv;
+  });
+
+  it('uses process.env.MAX_LIMIT for max limit set', async () => {
+    expect(getEnvars().maxLimit).toBe(2500);
   });
 
   it('returns 200 OK with basic paginated resources', async () => {
@@ -94,10 +111,11 @@ describe('API Lambda handler', () => {
     const callback = jest.fn();
     const result = await getLetters(event, context, callback);
 
+    expect(mockedMapErrorToResponse).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitNotANumber));
     expect(result).toEqual(expectedErrorResponse);
   });
 
-  it("returns 400 if the limit parameter is not positive", async () => {
+  it("returns 400 if the limit parameter is negative", async () => {
     const event = makeApiGwEvent({
       path: "/letters",
       queryStringParameters: { limit: "-1" },
@@ -107,6 +125,49 @@ describe('API Lambda handler', () => {
     const callback = jest.fn();
     const result = await getLetters(event, context, callback);
 
+    expect(mockedMapErrorToResponse).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitNotInRange));
+    expect(result).toEqual(expectedErrorResponse);
+  });
+
+  it("returns 400 if the limit parameter is zero", async () => {
+    const event = makeApiGwEvent({
+      path: "/letters",
+      queryStringParameters: { limit: "0" },
+      headers: {'nhsd-supplier-id': 'supplier1'}
+    });
+    const context = mockDeep<Context>();
+    const callback = jest.fn();
+    const result = await getLetters(event, context, callback);
+
+    expect(mockedMapErrorToResponse).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitNotInRange));
+    expect(result).toEqual(expectedErrorResponse);
+  });
+
+  it("returns 400 if the limit parameter is higher than max limit", async () => {
+    const event = makeApiGwEvent({
+      path: "/letters",
+      queryStringParameters: { limit: "2501" },
+      headers: {'nhsd-supplier-id': 'supplier1'}
+    });
+    const context = mockDeep<Context>();
+    const callback = jest.fn();
+    const result = await getLetters(event, context, callback);
+
+    expect(mockedMapErrorToResponse).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitNotInRange));
+    expect(result).toEqual(expectedErrorResponse);
+  });
+
+  it("returns 400 if unknown parameters are present", async () => {
+    const event = makeApiGwEvent({
+      path: "/letters",
+      queryStringParameters: { max: "2000" },
+      headers: {'nhsd-supplier-id': 'supplier1'}
+    });
+    const context = mockDeep<Context>();
+    const callback = jest.fn();
+    const result = await getLetters(event, context, callback);
+
+    expect(mockedMapErrorToResponse).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitOnly));
     expect(result).toEqual(expectedErrorResponse);
   });
 
@@ -116,6 +177,7 @@ describe('API Lambda handler', () => {
     const callback = jest.fn();
     const result = await getLetters(event, context, callback);
 
+    expect(mockedMapErrorToResponse).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestMissingSupplierId));
     expect(result).toEqual(expectedErrorResponse);
   });
 
@@ -125,6 +187,7 @@ describe('API Lambda handler', () => {
     const callback = jest.fn();
     const result = await getLetters(event, context, callback);
 
+    expect(mockedMapErrorToResponse).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestMissingSupplierId));
     expect(result).toEqual(expectedErrorResponse);
   });
 });

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

@@ -9,18 +9,27 @@ import pino from 'pino';
 import { mapErrorToResponse } from "../mappers/error-mapper";
 import { ValidationError } from "../errors";
 import { mapLetterBaseToApiResource } from "../mappers/letter-mapper";
+import util from "util";
 
 const letterRepo = createLetterRepository();
+
 const log = pino();
 
 // The endpoint should only return pending letters for now
 const status = "PENDING";
 
+export const getEnvars = (): { maxLimit: number } => ({
+  maxLimit: parseInt(process.env.MAX_LIMIT!)
+});
+
 export const getLetters: APIGatewayProxyHandler = async (event) => {
 
+  const { maxLimit } = getEnvars();
+
   try {
+    assertNotEmpty(event.headers, errors.ApiErrorDetail.InvalidRequestMissingSupplierId);
     const supplierId = assertNotEmpty(event.headers[lambdaConfig.SUPPLIER_ID_HEADER], errors.ApiErrorDetail.InvalidRequestMissingSupplierId);
-    const limitNumber = validateLimit(event.queryStringParameters);
+    const limitNumber = getLimitOrDefault(event.queryStringParameters, maxLimit);
 
     const letters = await getLettersForSupplier(
       supplierId,
@@ -51,38 +60,56 @@ export const getLetters: APIGatewayProxyHandler = async (event) => {
   }
 };
 
-function validateLimit(queryStringParameters: APIGatewayProxyEventQueryStringParameters | null) : number {
+function getLimitOrDefault(queryStringParameters: APIGatewayProxyEventQueryStringParameters | null, maxLimit: number) : number {
 
-  let limit = queryStringParameters?.limit;
-
-  if (!limit) {
-    limit = "10";
-  }
-
-  let limitNumber = Number(limit);
-
-  assertIsNumber(limitNumber, limit);
-  assertIsPositive(limitNumber, limit);
-
-  return limitNumber;
+  validateLimitParamOnly(queryStringParameters);
+  return getLimit(queryStringParameters?.limit, maxLimit);
 }
 
-function assertIsNumber(limitNumber: number, limit: string) {
+function assertIsNumber(limitNumber: number) {
   if (isNaN(limitNumber)) {
     log.info({
       description: "limit parameter is not a number",
-      limit,
+      limitNumber,
     });
     throw new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitNotANumber);
   }
 }
 
-function assertIsPositive(limitNumber: number, limit?: string) {
-  if (limitNumber < 0) {
+function assertLimitInRange(limitNumber: number, maxLimit: number) {
+  if (limitNumber <= 0 || limitNumber > maxLimit) {
     log.info({
-      description: "limit parameter is not positive",
-      limit,
+      description: "Limit value is invalid",
+      limitNumber,
     });
-    throw new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitNotPositive);
+    throw new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitNotInRange);
+  }
+}
+
+function validateLimitParamOnly(queryStringParameters: APIGatewayProxyEventQueryStringParameters | null) {
+  if (
+    queryStringParameters &&
+    Object.keys(queryStringParameters).some(
+      (key) => key !== "limit"
+    )
+  ) {
+    log.info({
+      description: "Unexpected query parameter(s) present",
+      queryStringParameters: queryStringParameters,
+    });
+    throw new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitOnly);
+  }
+}
+
+function getLimit(limit: string | undefined, maxLimit: number) {
+  let result;
+  if (limit) {
+    let limitParam = limit;
+    result = Number(limitParam);
+    assertIsNumber(result);
+    assertLimitInRange(result, maxLimit);
+  } else {
+    result = maxLimit;
   }
+  return result;
 }

specification/api/components/endpoints/listLetters.yml

@@ -11,6 +11,8 @@ description: The key use of this endpoint is to query letters which are ready to
 responses:
   '200':
     $ref: "../responses/getLetters200.yml"
+  '400':
+    $ref: "../responses/errors/listLetters/listLettersBadRequest.yml"
   '404':
     $ref: "../responses/errors/resourceNotFound.yml"
   '429':

specification/api/components/examples/errors/responses/listLetters/limitInvalidValue.json

@@ -0,0 +1,14 @@
+{
+  "errors": [
+    {
+      "code": "NOTIFY_INVALID_REQUEST",
+      "detail": "Invalid Request: limit parameter must be a positive number not greater than 2500",
+      "id": "rrt-1931948104716186917-c-geu2-10664-3111479-3.0",
+      "links": {
+        "about": "https://digital.nhs.uk/developer/api-catalogue/nhs-notify-supplier"
+      },
+      "status": "400",
+      "title": "Invalid request"
+    }
+  ]
+}

specification/api/components/examples/errors/responses/listLetters/unknownParameter.json

@@ -0,0 +1,14 @@
+{
+  "errors": [
+    {
+      "code": "NOTIFY_INVALID_REQUEST",
+      "detail": "Invalid Request: Only 'limit' query parameter is supported",
+      "id": "rrt-1931948104716186917-c-geu2-10664-3111479-3.0",
+      "links": {
+        "about": "https://digital.nhs.uk/developer/api-catalogue/nhs-notify-supplier"
+      },
+      "status": "400",
+      "title": "Invalid request"
+    }
+  ]
+}

specification/api/components/parameters/limit.yml

@@ -1,7 +1,7 @@
 name: limit
 in: query
 description: |-
-  The maximum number of items to return in a single request
+  The maximum number of items to return in a single request, max 2500
 schema:
   type: number
-  example: 100
+  example: 2500