Do not retry POST requests by default (#197)

It doesn't really make sense to retry non-idempotent calls

Also moving sanitised error over to a real error rather than a object. Makes it a little bit easier to test these: 'expect(..).reject.throws' etc.. doesn't work if you don't have really errors

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

@@ -22,6 +22,7 @@ interface PostRequest {
   responseType?: string
   data?: Record<string, unknown>
   raw?: boolean
+  retry?: boolean
 }
 
 interface StreamRequest {
@@ -76,6 +77,7 @@ export default class RestClient {
     responseType = '',
     data = {},
     raw = false,
+    retry = false,
   }: PostRequest = {}): Promise<unknown> {
     logger.info(`Post using user credentials: calling ${this.name}: ${path}`)
     try {
@@ -85,6 +87,9 @@ export default class RestClient {
         .agent(this.agent)
         .use(restClientMetricsMiddleware)
         .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,14 +25,15 @@ 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 ', () => {

server/sanitisedError.ts

@@ -1,6 +1,6 @@
 import type { ResponseError } from 'superagent'
 
-interface SanitisedError {
+export interface SanitisedError {
   text?: string
   status?: number
   headers?: unknown
@@ -13,14 +13,14 @@ export type UnsanitisedError = ResponseError
 
 export default function sanitise(error: UnsanitisedError): SanitisedError {
   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,
-    }
+    const e = new Error(error.message) as SanitisedError
+    e.text = error.response.text
+    e.status = error.response.status
+    e.headers = error.response.headers
+    e.data = error.response.body
+    e.message = error.message
+    e.stack = error.stack
+    return e
   }
   return {
     message: error.message,