VB-2345 Don't retry POST or PUT requests (#594)

* Do not retry POST/PUT requests by default

ministryofjustice/hmpps-template-typescript#197

* Have sanitisedError always return an Error instance

ministryofjustice/hmpps-template-typescript#199

server/data/restClient.test.ts

@@ -0,0 +1,118 @@
+import nock from 'nock'
+import RestClient from './restClient'
+import { AgentConfig } from '../config'
+
+const restClient = new RestClient(
+  'name-1',
+  {
+    url: 'http://localhost:8080/api',
+    timeout: {
+      response: 1000,
+      deadline: 1000,
+    },
+    agent: new AgentConfig(1000),
+  },
+  'token-1',
+)
+
+describe('POST', () => {
+  it('Should return response body', async () => {
+    nock('http://localhost:8080', {
+      reqheaders: { authorization: 'Bearer token-1' },
+    })
+      .post('/api/test')
+      .reply(200, { success: true })
+
+    const result = await restClient.post({
+      path: '/test',
+    })
+
+    expect(nock.isDone()).toBe(true)
+
+    expect(result).toStrictEqual({
+      success: true,
+    })
+  })
+
+  it('Should return raw response body', async () => {
+    nock('http://localhost:8080', {
+      reqheaders: { authorization: 'Bearer token-1' },
+    })
+      .post('/api/test')
+      .reply(200, { success: true })
+
+    const result = await restClient.post({
+      path: '/test',
+      headers: { header1: 'headerValue1' },
+      raw: true,
+    })
+
+    expect(nock.isDone()).toBe(true)
+
+    expect(result).toMatchObject({
+      req: { method: 'POST' },
+      status: 200,
+      text: '{"success":true}',
+    })
+  })
+
+  it('Should not retry by default', async () => {
+    nock('http://localhost:8080', {
+      reqheaders: { authorization: 'Bearer token-1' },
+    })
+      .post('/api/test')
+      .reply(500)
+
+    await expect(
+      restClient.post({
+        path: '/test',
+        headers: { header1: 'headerValue1' },
+      }),
+    ).rejects.toThrow('Internal Server Error')
+
+    expect(nock.isDone()).toBe(true)
+  })
+
+  it('retries if configured to do so', async () => {
+    nock('http://localhost:8080', {
+      reqheaders: { authorization: 'Bearer token-1' },
+    })
+      .post('/api/test')
+      .reply(500)
+      .post('/api/test')
+      .reply(500)
+      .post('/api/test')
+      .reply(500)
+
+    await expect(
+      restClient.post({
+        path: '/test',
+        headers: { header1: 'headerValue1' },
+        retry: true,
+      }),
+    ).rejects.toThrow('Internal Server Error')
+
+    expect(nock.isDone()).toBe(true)
+  })
+
+  it('can recover through retries', async () => {
+    nock('http://localhost:8080', {
+      reqheaders: { authorization: 'Bearer token-1' },
+    })
+      .post('/api/test')
+      .reply(500)
+      .post('/api/test')
+      .reply(500)
+      .post('/api/test')
+      .reply(200, { success: true })
+
+    const result = await restClient.post({
+      path: '/test',
+      headers: { header1: 'headerValue1' },
+      retry: true,
+    })
+
+    expect(result).toStrictEqual({ success: true })
+    expect(nock.isDone()).toBe(true)
+  })
+})

server/data/restClient.ts

@@ -21,6 +21,7 @@ interface PostRequest {
   responseType?: string
   data?: Record<string, unknown>
   raw?: boolean
+  retry?: boolean
 }
 
 interface PutRequest {
@@ -29,6 +30,7 @@ interface PutRequest {
   responseType?: string
   data?: Record<string, unknown>
   raw?: boolean
+  retry?: boolean
 }
 
 interface StreamRequest {
@@ -82,6 +84,7 @@ export default class RestClient {
     responseType = '',
     data = {},
     raw = false,
+    retry = false,
   }: PostRequest = {}): Promise<T> {
     logger.info(`Post using user credentials: calling ${this.name}: ${path}`)
     try {
@@ -90,6 +93,9 @@ export default class RestClient {
         .send(data)
         .agent(this.agent)
         .retry(2, (err, res) => {
+          if (retry === false) {
+            return false
+          }
           if (err) logger.info(`Retry handler found API error with ${err.code} ${err.message}`)
           return undefined // retry handler only for logging retries, not to influence retry logic
         })
@@ -106,14 +112,24 @@ export default class RestClient {
     }
   }
 
-  async put<T>({ path = null, headers = {}, responseType = '', data = {}, raw = false }: PutRequest = {}): Promise<T> {
+  async put<T>({
+    path = null,
+    headers = {},
+    responseType = '',
+    data = {},
+    raw = false,
+    retry = false,
+  }: PutRequest = {}): Promise<T> {
     logger.info(`Put using user credentials: calling ${this.name}: ${path}`)
     try {
       const result = await superagent
         .put(`${this.apiUrl()}${path}`)
         .send(data)
         .agent(this.agent)
         .retry(2, (err, res) => {
+          if (retry === false) {
+            return false
+          }
           if (err) logger.info(`Retry handler found API error with ${err.code} ${err.message}`)
           return undefined // retry handler only for logging retries, not to influence retry logic
         })

server/sanitisedError.test.ts

@@ -1,4 +1,4 @@
-import sanitisedError, { UnsanitisedError } from './sanitisedError'
+import sanitisedError, { UnsanitisedError, SanitisedError } from './sanitisedError'
 
 describe('sanitised error', () => {
   it('it should omit the request headers from the error object ', () => {
@@ -25,29 +25,32 @@ describe('sanitised error', () => {
       stack: 'stack description',
     } as unknown as UnsanitisedError
 
-    expect(sanitisedError(error)).toEqual({
-      headers: { date: 'Tue, 19 May 2020 15:16:20 GMT' },
-      message: 'Not Found',
-      stack: 'stack description',
-      status: 404,
-      text: { details: 'details' },
-      data: { content: 'hello' },
-    })
+    const e = new Error() as SanitisedError
+    e.message = 'Not Found'
+    e.text = 'details'
+    e.status = 404
+    e.headers = { date: 'Tue, 19 May 2020 15:16:20 GMT' }
+    e.data = { content: 'hello' }
+    e.stack = 'stack description'
+
+    expect(sanitisedError(error)).toEqual(e)
   })
 
-  it('it should return the error message ', () => {
+  it('it should return the error message', () => {
     const error = {
       message: 'error description',
     } as unknown as UnsanitisedError
-    expect(sanitisedError(error)).toEqual({
-      message: 'error description',
-    })
+
+    expect(sanitisedError(error)).toBeInstanceOf(Error)
+    expect(sanitisedError(error)).toHaveProperty('message', 'error description')
   })
 
-  it('it should return an empty object for an unknown error structure', () => {
+  it('it should return an empty Error instance for an unknown error structure', () => {
     const error = {
       property: 'unknown',
     } as unknown as UnsanitisedError
-    expect(sanitisedError(error)).toEqual({})
+
+    expect(sanitisedError(error)).toBeInstanceOf(Error)
+    expect(sanitisedError(error)).not.toHaveProperty('property')
   })
 })

server/sanitisedError.ts

@@ -1,6 +1,6 @@
 import type { ResponseError } from 'superagent'
 
-interface SanitisedError {
+export interface SanitisedError extends Error {
   text?: string
   status?: number
   headers?: unknown
@@ -12,18 +12,14 @@ interface SanitisedError {
 export type UnsanitisedError = ResponseError
 
 export default function sanitise(error: UnsanitisedError): SanitisedError {
+  const e = new Error() as SanitisedError
+  e.message = error.message
+  e.stack = error.stack
   if (error.response) {
-    return {
-      text: error.response.text,
-      status: error.response.status,
-      headers: error.response.headers,
-      data: error.response.body,
-      message: error.message,
-      stack: error.stack,
-    }
-  }
-  return {
-    message: error.message,
-    stack: error.stack,
+    e.text = error.response.text
+    e.status = error.response.status
+    e.headers = error.response.headers
+    e.data = error.response.body
   }
+  return e
 }