HAAR-335: upgrade dependencies

package.json

@@ -28,8 +28,8 @@
     "clean": "rm -rf dist build node_modules stylesheets"
   },
   "engines": {
-    "node": "^14",
-    "npm": "^6.4.1"
+    "node": "^16",
+    "npm": "^8"
   },
   "jest": {
     "preset": "ts-jest",
@@ -84,32 +84,35 @@
     ]
   },
   "dependencies": {
-    "@ministryofjustice/frontend": "^1.0.1",
-    "agentkeepalive": "^4.2.0",
-    "applicationinsights": "^2.2.1",
-    "body-parser": "^1.19.1",
+    "@ministryofjustice/frontend": "^1.3.1",
+    "agentkeepalive": "^4.2.1",
+    "applicationinsights": "^2.3.1",
+    "body-parser": "^1.20.0",
     "bunyan": "^1.8.15",
     "bunyan-format": "^0.2.1",
     "compression": "^1.7.4",
     "connect-flash": "^0.1.1",
-    "connect-redis": "^6.0.0",
+    "connect-redis": "^6.1.3",
     "cookie-session": "^2.0.0",
     "csurf": "^1.11.0",
     "dotenv": "^16.0.0",
-    "express": "^4.17.2",
+    "eslint-plugin-no-only-tests": "^2.6.0",
+    "express": "^4.17.3",
     "express-request-id": "^1.4.1",
     "express-session": "^1.17.2",
-    "govuk-frontend": "^3.14.0",
+    "govuk-frontend": "^4.0.1",
     "helmet": "^5.0.2",
     "http-errors": "^2.0.0",
     "jquery": "^3.6.0",
     "jwt-decode": "^3.1.2",
-    "nocache": "^3.0.1",
+    "nocache": "^3.0.3",
     "nunjucks": "^3.2.3",
     "passport": "^0.5.2",
     "passport-oauth2": "^1.6.1",
-    "redis": "^3.1.2",
-    "superagent": "^7.1.1"
+    "prom-client": "^14.0.1",
+    "redis": "^4.0.6",
+    "superagent": "^7.1.1",
+    "url-value-parser": "^2.1.0"
   },
   "devDependencies": {
     "@types/bunyan": "^1.8.8",
@@ -122,40 +125,40 @@
     "@types/express-request-id": "^1.4.3",
     "@types/express-session": "^1.17.4",
     "@types/http-errors": "^1.8.2",
-    "@types/jest": "^27.4.0",
+    "@types/jest": "^27.4.1",
     "@types/jsonwebtoken": "^8.5.8",
-    "@types/node": "^17.0.14",
+    "@types/node": "^17.0.23",
     "@types/nunjucks": "^3.2.1",
     "@types/passport": "^1.0.7",
     "@types/passport-oauth2": "^1.4.11",
-    "@types/redis": "^2.8.32",
+    "@types/redis": "^4.0.10",
     "@types/superagent": "^4.1.15",
-    "@types/supertest": "^2.0.11",
-    "@typescript-eslint/eslint-plugin": "^5.10.2",
-    "@typescript-eslint/parser": "^5.10.2",
-    "concurrently": "^7.0.0",
-    "cypress": "^9.4.1",
+    "@types/supertest": "^2.0.12",
+    "@typescript-eslint/eslint-plugin": "^5.19.0",
+    "@typescript-eslint/parser": "^5.19.0",
+    "concurrently": "^7.1.0",
+    "cypress": "^9.5.4",
     "cypress-multi-reporters": "^1.5.0",
-    "eslint": "^8.8.0",
+    "eslint": "^8.13.0",
     "eslint-config-airbnb-base": "^15.0.0",
-    "eslint-config-prettier": "^8.3.0",
-    "eslint-import-resolver-typescript": "^2.5.0",
+    "eslint-config-prettier": "^8.5.0",
+    "eslint-import-resolver-typescript": "^2.7.1",
     "eslint-plugin-cypress": "^2.12.1",
-    "eslint-plugin-import": "2.25.4",
+    "eslint-plugin-import": "2.26.0",
     "eslint-plugin-prettier": "^4.0.0",
     "husky": "^7.0.4",
-    "jest": "^27.4.7",
+    "jest": "^27.5.1",
     "jest-html-reporter": "^3.4.2",
-    "jest-junit": "^13.0.0",
+    "jest-junit": "^13.1.0",
     "jsonwebtoken": "^8.5.1",
-    "lint-staged": "^12.3.3",
+    "lint-staged": "^12.3.7",
     "mocha-junit-reporter": "^2.0.2",
-    "nock": "^13.2.2",
+    "nock": "^13.2.4",
     "nodemon": "^2.0.15",
-    "prettier": "^2.5.1",
-    "sass": "^1.49.7",
+    "prettier": "^2.6.2",
+    "sass": "^1.50.0",
     "supertest": "^6.2.2",
-    "ts-jest": "^27.1.3",
-    "typescript": "^4.5.5"
+    "ts-jest": "^27.1.4",
+    "typescript": "^4.6.3"
   }
 }

server/config.ts

@@ -35,7 +35,7 @@ export default {
   https: production,
   staticResourceCacheDuration: 20,
   redis: {
-    host: process.env.REDIS_HOST,
+    host: get('REDIS_HOST', 'localhost', requiredInProduction),
     port: parseInt(process.env.REDIS_PORT, 10) || 6379,
     password: process.env.REDIS_AUTH_TOKEN,
     tls_enabled: get('REDIS_TLS_ENABLED', 'false'),

server/data/redisClient.ts

@@ -0,0 +1,18 @@
+import { createClient } from 'redis'
+
+import config from '../config'
+
+export type RedisClient = ReturnType<typeof createClient>
+
+const url =
+  config.redis.tls_enabled === 'true'
+    ? `rediss://${config.redis.host}:${config.redis.port}`
+    : `redis://${config.redis.host}:${config.redis.port}`
+
+export const createRedisClient = (legacyMode = false): RedisClient => {
+  return createClient({
+    url,
+    password: config.redis.password,
+    legacyMode,
+  })
+}

server/data/restClient.ts

@@ -6,6 +6,7 @@ import logger from '../../logger'
 import sanitiseError from '../sanitisedError'
 import { ApiConfig } from '../config'
 import type { UnsanitisedError } from '../sanitisedError'
+import { restClientMetricsMiddleware } from './restClientMetricsMiddleware'
 
 interface GetRequest {
   path?: string
@@ -19,7 +20,7 @@ interface PostRequest {
   path?: string
   headers?: Record<string, string>
   responseType?: string
-  data?: Record<string, unknown> | string
+  data?: Record<string, unknown>
   raw?: boolean
 }
 
@@ -44,12 +45,13 @@ export default class RestClient {
     return this.config.timeout
   }
 
-  async get({ path = null, query = '', headers = {}, responseType = '', raw = false }: GetRequest): Promise<unknown> {
+  async get<T>({ path = null, query = '', headers = {}, responseType = '', raw = false }: GetRequest): Promise<T> {
     logger.info(`Get using user credentials: calling ${this.name}: ${path} ${query}`)
     try {
       const result = await superagent
         .get(`${this.apiUrl()}${path}`)
         .agent(this.agent)
+        .use(restClientMetricsMiddleware)
         .retry(2, (err, res) => {
           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
@@ -75,12 +77,13 @@ export default class RestClient {
     data = {},
     raw = false,
   }: PostRequest = {}): Promise<T> {
-    logger.info(`Post using user credentials: calling ${this.name}: ${this.apiUrl()}${path}`)
+    logger.info(`Post using user credentials: calling ${this.name}: ${path}`)
     try {
       const result = await superagent
         .post(`${this.apiUrl()}${path}`)
         .send(data)
         .agent(this.agent)
+        .use(restClientMetricsMiddleware)
         .retry(2, (err, res) => {
           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
@@ -98,13 +101,14 @@ export default class RestClient {
     }
   }
 
-  async stream({ path = null, headers = {} }: StreamRequest = {}): Promise<unknown> {
+  async stream({ path = null, headers = {} }: StreamRequest = {}): Promise<Readable> {
     logger.info(`Get using user credentials: calling ${this.name}: ${path}`)
     return new Promise((resolve, reject) => {
       superagent
         .get(`${this.apiUrl()}${path}`)
         .agent(this.agent)
         .auth(this.token, { type: 'bearer' })
+        .use(restClientMetricsMiddleware)
         .retry(2, (err, res) => {
           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/data/restClientMetricsMiddleware.test.ts

@@ -0,0 +1,72 @@
+import superagent from 'superagent'
+import {
+  restClientMetricsMiddleware,
+  normalizePath,
+  requestHistogram,
+  timeoutCounter,
+} from './restClientMetricsMiddleware'
+
+describe('restClientMetricsMiddleware', () => {
+  afterEach(() => {
+    jest.clearAllMocks()
+  })
+
+  describe('normalizePath', () => {
+    it('removes the query params from the URL path', () => {
+      const result = normalizePath('https://httpbin.org/?foo=bar')
+      expect(result).toBe('/')
+    })
+
+    it('normalises recall ids', () => {
+      const result = normalizePath(
+        'https://manage-recalls-dev.hmpps.service.justice.gov.uk/recalls/15e4cccf-cc7b-4946-aa22-a82086735ec2/view-recall'
+      )
+      expect(result).toBe('/recalls/#val/view-recall')
+    })
+
+    it('normalises nomis ids', () => {
+      const result = normalizePath('https://manage-recalls-dev.hmpps.service.justice.gov.uk/prisoner/A7826DY')
+      expect(result).toBe('/prisoner/#val')
+    })
+  })
+
+  describe('request timers', () => {
+    it('times the whole request', async () => {
+      const requestHistogramLabelsSpy = jest.spyOn(requestHistogram, 'labels').mockReturnValue(requestHistogram)
+      const requestHistogramStartSpy = jest.spyOn(requestHistogram, 'observe')
+
+      let code: number
+
+      await superagent
+        .get('https://httpbin.org/')
+        .use(restClientMetricsMiddleware)
+        .set('accept', 'json')
+        .then(res => {
+          code = res.statusCode
+        })
+
+      expect(code).toBe(200)
+      expect(requestHistogramLabelsSpy).toHaveBeenCalledTimes(1)
+      expect(requestHistogramLabelsSpy).toHaveBeenCalledWith('httpbin.org', 'GET', '/', '200')
+      expect(requestHistogramStartSpy).toHaveBeenCalledTimes(1)
+    })
+  })
+
+  describe('timeout errors', () => {
+    // FIXME: For some reason this test just doesn't work, but the code really does count the timeouts...
+    it.skip('increment the timeoutCounter', async () => {
+      const timeoutCounterLabelsSpy = jest.spyOn(timeoutCounter, 'labels').mockReturnValue(timeoutCounter)
+      const timeoutCounterIncSpy = jest.spyOn(timeoutCounter, 'inc')
+
+      await superagent
+        .get('https://httpbin.org/delay/5') // implements a delay of 5 seconds
+        .use(restClientMetricsMiddleware)
+        .set('accept', 'json')
+        .timeout(100) // Wait 100ms for the server to start sending
+        .end()
+
+      expect(timeoutCounterLabelsSpy).toHaveBeenCalledWith('httpbin.org', 'GET', '/delay/5')
+      expect(timeoutCounterIncSpy).toHaveBeenCalledTimes(1)
+    })
+  })
+})

server/data/restClientMetricsMiddleware.ts

@@ -0,0 +1,49 @@
+import { Socket } from 'net'
+import promClient from 'prom-client'
+import { Response, SuperAgentRequest } from 'superagent'
+import UrlValueParser from 'url-value-parser'
+
+const requestHistogram = new promClient.Histogram({
+  name: 'http_client_requests_seconds',
+  help: 'Timings and counts of http client requests',
+  buckets: [0.5, 0.75, 0.95, 0.99, 1],
+  labelNames: ['clientName', 'method', 'uri', 'status'],
+})
+
+const timeoutCounter = new promClient.Counter({
+  name: 'http_client_requests_timeout_total',
+  help: 'Count of http client request timeouts',
+  labelNames: ['clientName', 'method', 'uri'],
+})
+
+function restClientMetricsMiddleware(agent: SuperAgentRequest) {
+  agent.on('request', ({ req }) => {
+    const { hostname } = new URL(agent.url)
+    const normalizedPath = normalizePath(agent.url)
+    const startTime = Date.now()
+
+    req.on('socket', (socket: Socket) => {
+      socket.on('timeout', () => {
+        timeoutCounter.labels(hostname, req.method, normalizedPath).inc()
+      })
+    })
+
+    req.on('response', (res: Response, err: Error) => {
+      res.on('end', () => {
+        const responseTime = Date.now() - startTime
+        requestHistogram.labels(hostname, req.method, normalizedPath, String(res.statusCode)).observe(responseTime)
+      })
+    })
+  })
+
+  return agent
+}
+
+function normalizePath(url: string) {
+  const { pathname } = new URL(url)
+  const urlPathReplacement = '#val'
+  const urlValueParser = new UrlValueParser({ extraMasks: [/^[A-Z|0-9]+/] })
+  return urlValueParser.replacePathValues(pathname, urlPathReplacement)
+}
+
+export { restClientMetricsMiddleware, normalizePath, requestHistogram, timeoutCounter }

server/data/tokenStore.test.ts

@@ -1,11 +1,13 @@
-import { RedisClient } from 'redis'
+import { RedisClient } from './redisClient'
 import TokenStore from './tokenStore'
 
 const redisClient = {
-  on: jest.fn(),
   get: jest.fn(),
   set: jest.fn(),
-}
+  on: jest.fn(),
+  connect: jest.fn(),
+  isOpen: true,
+} as unknown as jest.Mocked<RedisClient>
 
 describe('tokenStore', () => {
   let tokenStore: TokenStore
@@ -18,17 +20,37 @@ describe('tokenStore', () => {
     jest.resetAllMocks()
   })
 
-  it('Can retrieve token', async () => {
-    redisClient.get.mockImplementation((key, callback) => callback(null, 'token-1'))
+  describe('get token', () => {
+    it('Can retrieve token', async () => {
+      redisClient.get.mockResolvedValue('token-1')
+
+      await expect(tokenStore.getToken('user-1')).resolves.toBe('token-1')
+
+      expect(redisClient.get).toHaveBeenCalledWith('systemToken:user-1')
+    })
 
-    await expect(tokenStore.getToken('user-1')).resolves.toBe('token-1')
+    it('Connects when no connection calling getToken', async () => {
+      ;(redisClient as unknown as Record<string, boolean>).isOpen = false
 
-    expect(redisClient.get).toHaveBeenCalledWith('user-1', expect.any(Function))
+      await tokenStore.getToken('user-1')
+
+      expect(redisClient.connect).toHaveBeenCalledWith()
+    })
   })
 
-  it('Can set token', async () => {
-    await tokenStore.setToken('user-1', 'token-1', 10)
+  describe('set token', () => {
+    it('Can set token', async () => {
+      await tokenStore.setToken('user-1', 'token-1', 10)
+
+      expect(redisClient.set).toHaveBeenCalledWith('systemToken:user-1', 'token-1', { EX: 10 })
+    })
+
+    it('Connects when no connection calling set token', async () => {
+      ;(redisClient as unknown as Record<string, boolean>).isOpen = false
+
+      await tokenStore.setToken('user-1', 'token-1', 10)
 
-    expect(redisClient.set).toHaveBeenCalledWith('user-1', 'token-1', 'EX', 10, expect.any(Function))
+      expect(redisClient.connect).toHaveBeenCalledWith()
+    })
   })
 })