From 7f2062be9479cd9d966fe16957fabee24c8931b8 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Mon, 18 Nov 2024 10:55:41 +0000 Subject: [PATCH 01/18] feat: create type and validator for Metabase client --- .../analytics/metabase/shared/types.ts | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/api.planx.uk/modules/analytics/metabase/shared/types.ts b/api.planx.uk/modules/analytics/metabase/shared/types.ts index e69de29bb2..e953721b3c 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/types.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/types.ts @@ -0,0 +1,41 @@ +export interface Input { + councilName: string; + originalDashboardId: number; + /** The name of the filter to be updated */ + filter: string; + /** The value of the new filter; updateFilter() only supports strings right now */ + filterValue: string; +} + +export function validateInput(input: unknown): input is Input { + // check that input type is object + if (typeof input !== "object" || input === null) { + return false; + } + + // check that input object is same shape as Input type with same properties + const { councilName, originalDashboardId, filter, filterValue } = + input as Input; + + if (typeof councilName !== "string" || councilName.trim() === "") { + console.error("Invalid councilName: must be a non-empty string"); + return false; + } + + if (!Number.isInteger(originalDashboardId) || originalDashboardId <= 0) { + console.error("Invalid originalDashboardId: must be a positive integer"); + return false; + } + + if (typeof filter !== "string" || filter.trim() === "") { + console.error("Invalid filter: must be a non-empty string"); + return false; + } + + if (typeof filterValue !== "string") { + console.error("Invalid filterValue: must be a string"); + return false; + } + console.log("Input valid"); + return true; +} From e0a8c8c181df1886680d198465fa082e9768dca9 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Tue, 29 Oct 2024 12:05:19 +0000 Subject: [PATCH 02/18] feat: create metabase client with error handling --- .../analytics/metabase/shared/client.ts | 155 ++++++++++++++++++ 1 file changed, 155 insertions(+) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.ts b/api.planx.uk/modules/analytics/metabase/shared/client.ts index e69de29bb2..41b8f8145b 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.ts @@ -0,0 +1,155 @@ +import axios from "axios"; +import type { + AxiosInstance, + AxiosError, + AxiosRequestConfig, + InternalAxiosRequestConfig, +} from "axios"; +import dotenv from "dotenv"; + +dotenv.config(); + +// Custom error class for Metabase-specific errors +export class MetabaseError extends Error { + constructor( + message: string, + public statusCode?: number, + public response?: unknown, + ) { + super(message); + this.name = "MetabaseError"; + } +} + +interface MetabaseConfig { + baseURL: string; + apiKey: string; + timeout?: number; + retries?: number; +} + +// Validate environment variables +const validateConfig = (): MetabaseConfig => { + const baseURL = process.env.METABASE_BASE_URL; + const apiKey = process.env.METABASE_API_KEY; + + if (!baseURL) { + throw new Error( + "METABASE_BASE_URL is not defined in environment variables", + ); + } + if (!apiKey) { + throw new Error("METABASE_API_KEY is not defined in environment variables"); + } + + return { + baseURL, + apiKey, + timeout: parseInt(process.env.METABASE_TIMEOUT ?? "30000"), // 30 second default timeout + retries: parseInt(process.env.METABASE_MAX_RETRIES ?? "3"), // 3 default retries + }; +}; + +// Extended request config to include retry count +interface ExtendedAxiosRequestConfig extends InternalAxiosRequestConfig { + retryCount?: number; +} + +// Create and configure Axios instance +export const createMetabaseClient = (): AxiosInstance => { + const config = validateConfig(); + + const client = axios.create({ + baseURL: config.baseURL, + headers: { + "X-API-Key": config.apiKey, + "Content-Type": "application/json", + }, + timeout: config.timeout, + }); + + // Request interceptor + client.interceptors.request.use( + (config) => { + // Could add request logging here + return config; + }, + (error) => { + return Promise.reject(error); + }, + ); + + client.interceptors.response.use( + (response) => { + return response; + }, + async (error: AxiosError) => { + const originalRequest = error.config as ExtendedAxiosRequestConfig; + + if (!originalRequest) { + throw new MetabaseError("No request config available"); + } + + // Initialise retry count if not present + if (typeof originalRequest.retryCount === "undefined") { + originalRequest.retryCount = 0; + } + + // Handle retry logic + if (error.response) { + // Retry on 5xx errors + if ( + error.response.status >= 500 && + originalRequest.retryCount < (config.retries ?? 3) + ) { + originalRequest.retryCount++; + return client(originalRequest); + } + + // Transform error response + const errorMessage = + typeof error.response.data === "object" && + error.response.data !== null && + "message" in error.response.data + ? String(error.response.data.message) + : "Metabase request failed"; + + throw new MetabaseError( + errorMessage, + error.response.status, + error.response.data, + ); + } + + // Handle network errors + if (error.request) { + throw new MetabaseError( + "Network error occurred", + undefined, + error.request, + ); + } + + // Handle other errors + throw new MetabaseError(error.message); + }, + ); + + return client; +}; + +// Export singleton instance +export const metabaseClient = createMetabaseClient(); + +// Health check function +export const checkMetabaseConnection = async (): Promise => { + try { + const response = await metabaseClient.get("/api/user/current"); + return response.status === 200; + } catch (error) { + console.error("Metabase health check failed:", error); + return false; + } +}; + +export default metabaseClient; From 3e76ca8f626b428cbe4249beb5fbf569bff6c1d8 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Mon, 18 Nov 2024 11:00:37 +0000 Subject: [PATCH 03/18] chore: change 'council' to 'team' --- api.planx.uk/modules/analytics/metabase/shared/types.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/types.ts b/api.planx.uk/modules/analytics/metabase/shared/types.ts index e953721b3c..e1f491a55f 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/types.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/types.ts @@ -1,5 +1,5 @@ export interface Input { - councilName: string; + teamName: string; originalDashboardId: number; /** The name of the filter to be updated */ filter: string; @@ -14,11 +14,10 @@ export function validateInput(input: unknown): input is Input { } // check that input object is same shape as Input type with same properties - const { councilName, originalDashboardId, filter, filterValue } = - input as Input; + const { teamName, originalDashboardId, filter, filterValue } = input as Input; - if (typeof councilName !== "string" || councilName.trim() === "") { - console.error("Invalid councilName: must be a non-empty string"); + if (typeof teamName !== "string" || teamName.trim() === "") { + console.error("Invalid teamName: must be a non-empty string"); return false; } From 7a1b6c29e265c091b16a2e77d0e8fe4629e10a34 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Wed, 20 Nov 2024 17:18:55 +0000 Subject: [PATCH 04/18] chore: remove unused types --- .../analytics/metabase/shared/types.ts | 40 ------------------- 1 file changed, 40 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/types.ts b/api.planx.uk/modules/analytics/metabase/shared/types.ts index e1f491a55f..e69de29bb2 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/types.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/types.ts @@ -1,40 +0,0 @@ -export interface Input { - teamName: string; - originalDashboardId: number; - /** The name of the filter to be updated */ - filter: string; - /** The value of the new filter; updateFilter() only supports strings right now */ - filterValue: string; -} - -export function validateInput(input: unknown): input is Input { - // check that input type is object - if (typeof input !== "object" || input === null) { - return false; - } - - // check that input object is same shape as Input type with same properties - const { teamName, originalDashboardId, filter, filterValue } = input as Input; - - if (typeof teamName !== "string" || teamName.trim() === "") { - console.error("Invalid teamName: must be a non-empty string"); - return false; - } - - if (!Number.isInteger(originalDashboardId) || originalDashboardId <= 0) { - console.error("Invalid originalDashboardId: must be a positive integer"); - return false; - } - - if (typeof filter !== "string" || filter.trim() === "") { - console.error("Invalid filter: must be a non-empty string"); - return false; - } - - if (typeof filterValue !== "string") { - console.error("Invalid filterValue: must be a string"); - return false; - } - console.log("Input valid"); - return true; -} From 1ea36cd9a1c79decb5854e15a4f75a140d6543ac Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Wed, 20 Nov 2024 17:19:55 +0000 Subject: [PATCH 05/18] chore: tidy up client removes unnecessary imports and code, and fixes constants --- .../analytics/metabase/shared/client.ts | 43 +++---------------- 1 file changed, 7 insertions(+), 36 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.ts b/api.planx.uk/modules/analytics/metabase/shared/client.ts index 41b8f8145b..34a23c9377 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.ts @@ -2,12 +2,8 @@ import axios from "axios"; import type { AxiosInstance, AxiosError, - AxiosRequestConfig, InternalAxiosRequestConfig, } from "axios"; -import dotenv from "dotenv"; - -dotenv.config(); // Custom error class for Metabase-specific errors export class MetabaseError extends Error { @@ -33,20 +29,17 @@ const validateConfig = (): MetabaseConfig => { const baseURL = process.env.METABASE_BASE_URL; const apiKey = process.env.METABASE_API_KEY; - if (!baseURL) { - throw new Error( - "METABASE_BASE_URL is not defined in environment variables", - ); - } - if (!apiKey) { - throw new Error("METABASE_API_KEY is not defined in environment variables"); - } + const METABASE_TIMEOUT = 30_000; + const METABASE_MAX_RETRIES = 3; + + assert(baseURL, "Missing environment variable 'METABASE_BASE_URL'"); + assert(apiKey, "Missing environment variable 'METABASE_API_KEY'"); return { baseURL, apiKey, - timeout: parseInt(process.env.METABASE_TIMEOUT ?? "30000"), // 30 second default timeout - retries: parseInt(process.env.METABASE_MAX_RETRIES ?? "3"), // 3 default retries + timeout: METABASE_TIMEOUT, + retries: METABASE_MAX_RETRIES, }; }; @@ -68,17 +61,6 @@ export const createMetabaseClient = (): AxiosInstance => { timeout: config.timeout, }); - // Request interceptor - client.interceptors.request.use( - (config) => { - // Could add request logging here - return config; - }, - (error) => { - return Promise.reject(error); - }, - ); - client.interceptors.response.use( (response) => { return response; @@ -141,15 +123,4 @@ export const createMetabaseClient = (): AxiosInstance => { // Export singleton instance export const metabaseClient = createMetabaseClient(); -// Health check function -export const checkMetabaseConnection = async (): Promise => { - try { - const response = await metabaseClient.get("/api/user/current"); - return response.status === 200; - } catch (error) { - console.error("Metabase health check failed:", error); - return false; - } -}; - export default metabaseClient; From 40c205efbb3caaa01c8d963002bc25c659e6386e Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Mon, 25 Nov 2024 10:08:14 +0000 Subject: [PATCH 06/18] chore: tidy client imports and exports --- api.planx.uk/modules/analytics/metabase/shared/client.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.ts b/api.planx.uk/modules/analytics/metabase/shared/client.ts index 34a23c9377..9ce5267505 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.ts @@ -25,14 +25,14 @@ interface MetabaseConfig { } // Validate environment variables -const validateConfig = (): MetabaseConfig => { - const baseURL = process.env.METABASE_BASE_URL; +export const validateConfig = (): MetabaseConfig => { + const baseURL = process.env.METABASE_URL_EXT; const apiKey = process.env.METABASE_API_KEY; const METABASE_TIMEOUT = 30_000; const METABASE_MAX_RETRIES = 3; - assert(baseURL, "Missing environment variable 'METABASE_BASE_URL'"); + assert(baseURL, "Missing environment variable 'METABASE_URL_EXT'"); assert(apiKey, "Missing environment variable 'METABASE_API_KEY'"); return { From 807b144b28223c28c6d5db312b6bd4f38679dce5 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Mon, 25 Nov 2024 10:08:45 +0000 Subject: [PATCH 07/18] feat: test for metabase client --- .../analytics/metabase/shared/client.test.ts | 178 +++++++++++++++++- 1 file changed, 177 insertions(+), 1 deletion(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index 43d9c7a3f9..6f398891cd 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -1 +1,177 @@ -test.todo("should test configuration and errors"); +import axios from "axios"; +import type { + AxiosError, + AxiosInstance, + AxiosResponse, + InternalAxiosRequestConfig, +} from "axios"; +import { + validateConfig, + createMetabaseClient, + MetabaseError, +} from "./client.js"; + +vi.mock("axios"); +const mockedAxios = vi.mocked(axios, true); + +describe("Metabase client", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.resetModules(); + vi.stubEnv("METABASE_URL_EXT", "https://metabase.mock.com"); + vi.stubEnv("METABASE_API_KEY", "mockmetabasekey"); + + const mockAxiosInstance = { + interceptors: { + request: { + use: vi.fn(), + eject: vi.fn(), + clear: vi.fn(), + }, + response: { + use: vi.fn((successFn, errorFn) => { + // Store error handler for testing + mockAxiosInstance.interceptors.response.errorHandler = errorFn; + return 1; // Return handler id + }), + eject: vi.fn(), + clear: vi.fn(), + errorHandler: null as any, + }, + }, + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + }; + + mockedAxios.create.mockReturnValue( + mockAxiosInstance as unknown as AxiosInstance, + ); + + test("returns configured client", () => { + const client = createMetabaseClient(); + + expect(axios.create).toHaveBeenCalledWith({ + baseURL: process.env.METABASE_URL_EXT, + headers: { + "X-API-Key": process.env.METABASE_API_KEY, + "Content-Type": "application/json", + }, + timeout: 30_000, + }); + }); + + describe("validates configuration", () => { + test("throws error when URL_EXT is missing", () => { + vi.unstubAllEnvs(); + expect(() => validateConfig()).toThrow( + "Missing environment variable 'METABASE_URL_EXT'", + ); + }); + + test("throws error when API_KEY is missing", () => { + vi.unstubAllEnvs(); + vi.stubEnv("process.env.METABASE_URL_EXT", "https://metabase.mock.com"); + expect(() => validateConfig()).toThrow( + "Missing environment variable 'METABASE_API_KEY'", + ); + }); + + test("returns valid config object", () => { + const config = validateConfig; + expect(config).toMatchObject({ + baseURL: process.env.METABASE_URL_EXT, + apiKey: process.env.METABASE_API_KEY, + timeout: 30_000, + retries: 3, + }); + }); + }); + + test("retries requests on 5xx errors", async () => { + const client = createMetabaseClient(); + const mockAxiosInstance = mockedAxios.create.mock.results[0].value; + + // Create headers instance + const headers = new axios.AxiosHeaders({ + "Content-Type": "application/json", + }); + + // Create mock error with properly typed config + const error: AxiosError = { + config: { + headers: headers, + retryCount: 0, + url: "/test", + method: "get", + baseURL: "https://test.com", + transformRequest: [], + transformResponse: [], + timeout: 0, + adapter: axios.defaults.adapter, + xsrfCookieName: "", + xsrfHeaderName: "", + maxContentLength: -1, + maxBodyLength: -1, + env: { + FormData: window.FormData, + }, + } as unknown as InternalAxiosRequestConfig, + response: { + status: 500, + statusText: "Internal Server Error", + data: { message: "Server Error" }, + headers: headers, + config: {} as InternalAxiosRequestConfig, + } as AxiosResponse, + isAxiosError: true, + name: "AxiosError", + message: "Server Error", + toJSON: () => ({}), + }; + + // Get the error handler that was registered + const errorHandler = mockAxiosInstance.interceptors.response.errorHandler; + expect(errorHandler).toBeDefined(); + + // Call error handler and expect it to retry + await expect(errorHandler(error)).rejects.toThrow(MetabaseError); + }); + + test("throws non-5xx errors", async () => { + const error = new Error("Server Error") as AxiosError; + error.config = {} as InternalAxiosRequestConfig; + error.response = { + status: 500, + data: { message: "Server Error" }, + statusText: "Server Error", + headers: {}, + config: {} as InternalAxiosRequestConfig, + } as AxiosResponse; + error.isAxiosError = true; + error.name = "AxiosError"; + error.message = "Server Error"; + error.toJSON = () => ({}); + + mockedAxios.create.mockReturnValue({ + interceptors: { + request: { use: vi.fn() }, + response: { + use: (errorHandler: (error: AxiosError) => Promise) => + errorHandler(error), + }, + }, + } as any); + + const client = createMetabaseClient(); + await expect(client.get("/test")).rejects.toThrow( + new MetabaseError("Bad Request", 400, error.response.data), + ); + }); + + afterAll(() => { + vi.unstubAllEnvs(); + }); + }); +}); From c4d9b76f24db89fd6f5498a367128c9b32f86c51 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Mon, 25 Nov 2024 16:57:34 +0000 Subject: [PATCH 08/18] fix: fix test errors --- .../analytics/metabase/shared/client.test.ts | 250 ++++++++---------- 1 file changed, 117 insertions(+), 133 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index 6f398891cd..462d70815a 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -1,18 +1,21 @@ import axios from "axios"; import type { - AxiosError, AxiosInstance, - AxiosResponse, + AxiosError, InternalAxiosRequestConfig, + AxiosResponse, } from "axios"; -import { - validateConfig, - createMetabaseClient, - MetabaseError, -} from "./client.js"; +import { validateConfig, createMetabaseClient } from "./client.js"; + +const axiosCreateSpy = vi.spyOn(axios, "create"); + +interface ExtendedAxiosRequestConfig extends InternalAxiosRequestConfig { + retryCount?: number; +} -vi.mock("axios"); -const mockedAxios = vi.mocked(axios, true); +interface MockAxiosInstance extends AxiosInstance { + handleError: (error: AxiosError) => Promise; +} describe("Metabase client", () => { beforeEach(() => { @@ -20,158 +23,139 @@ describe("Metabase client", () => { vi.resetModules(); vi.stubEnv("METABASE_URL_EXT", "https://metabase.mock.com"); vi.stubEnv("METABASE_API_KEY", "mockmetabasekey"); + }); - const mockAxiosInstance = { - interceptors: { - request: { - use: vi.fn(), - eject: vi.fn(), - clear: vi.fn(), - }, - response: { - use: vi.fn((successFn, errorFn) => { - // Store error handler for testing - mockAxiosInstance.interceptors.response.errorHandler = errorFn; - return 1; // Return handler id - }), - eject: vi.fn(), - clear: vi.fn(), - errorHandler: null as any, - }, + afterEach(() => { + vi.unstubAllEnvs(); + }); + + test("returns configured client", () => { + const _client = createMetabaseClient(); + + expect(axiosCreateSpy).toHaveBeenCalledWith({ + baseURL: process.env.METABASE_URL_EXT, + headers: { + "X-API-Key": process.env.METABASE_API_KEY, + "Content-Type": "application/json", }, - get: vi.fn(), - post: vi.fn(), - put: vi.fn(), - delete: vi.fn(), - }; + timeout: 30_000, + }); + }); - mockedAxios.create.mockReturnValue( - mockAxiosInstance as unknown as AxiosInstance, - ); + describe("validates configuration", () => { + test("throws error when URL_EXT is missing", () => { + vi.stubEnv("METABASE_URL_EXT", undefined); + expect(() => validateConfig()).toThrow( + "Missing environment variable 'METABASE_URL_EXT'", + ); + }); - test("returns configured client", () => { - const client = createMetabaseClient(); + test("throws error when API_KEY is missing", () => { + vi.stubEnv("METABASE_API_KEY", undefined); + expect(() => validateConfig()).toThrow( + "Missing environment variable 'METABASE_API_KEY'", + ); + }); - expect(axios.create).toHaveBeenCalledWith({ + test("returns valid config object", () => { + const config = validateConfig(); + expect(config).toMatchObject({ baseURL: process.env.METABASE_URL_EXT, - headers: { - "X-API-Key": process.env.METABASE_API_KEY, - "Content-Type": "application/json", - }, + apiKey: process.env.METABASE_API_KEY, timeout: 30_000, + retries: 3, }); }); + }); - describe("validates configuration", () => { - test("throws error when URL_EXT is missing", () => { - vi.unstubAllEnvs(); - expect(() => validateConfig()).toThrow( - "Missing environment variable 'METABASE_URL_EXT'", - ); - }); - - test("throws error when API_KEY is missing", () => { - vi.unstubAllEnvs(); - vi.stubEnv("process.env.METABASE_URL_EXT", "https://metabase.mock.com"); - expect(() => validateConfig()).toThrow( - "Missing environment variable 'METABASE_API_KEY'", - ); - }); + describe("Error handling", () => { + test.skip("retries then succeeds on 5xx errors", async () => { + // Setup mock responses + const mockRequest = vi + .fn() + .mockRejectedValueOnce({ + config: { retryCount: 0 } as ExtendedAxiosRequestConfig, + response: { + status: 500, + statusText: "Internal Server Error", + headers: {}, + config: {} as ExtendedAxiosRequestConfig, + data: { message: "Server Error" }, + } as AxiosResponse, + isAxiosError: true, + } as unknown as AxiosError) + .mockResolvedValueOnce({ data: "success" }); + + // Create mock axios instance + const mockAxiosInstance = { + request: mockRequest, + interceptors: { + response: { + use: vi.fn((successFn, errorFn) => { + // Store the error handler + (mockAxiosInstance as MockAxiosInstance).handleError = errorFn; + }), + }, + }, + } as unknown as AxiosInstance; - test("returns valid config object", () => { - const config = validateConfig; - expect(config).toMatchObject({ - baseURL: process.env.METABASE_URL_EXT, - apiKey: process.env.METABASE_API_KEY, - timeout: 30_000, - retries: 3, - }); - }); - }); + axiosCreateSpy.mockReturnValue(mockAxiosInstance); - test("retries requests on 5xx errors", async () => { - const client = createMetabaseClient(); - const mockAxiosInstance = mockedAxios.create.mock.results[0].value; + const _client = createMetabaseClient(); - // Create headers instance - const headers = new axios.AxiosHeaders({ - "Content-Type": "application/json", - }); + // Get the actual error handler + const handleError = (mockAxiosInstance as MockAxiosInstance).handleError; - // Create mock error with properly typed config - const error: AxiosError = { - config: { - headers: headers, - retryCount: 0, - url: "/test", - method: "get", - baseURL: "https://test.com", - transformRequest: [], - transformResponse: [], - timeout: 0, - adapter: axios.defaults.adapter, - xsrfCookieName: "", - xsrfHeaderName: "", - maxContentLength: -1, - maxBodyLength: -1, - env: { - FormData: window.FormData, - }, - } as unknown as InternalAxiosRequestConfig, + // First call should trigger retry + await handleError({ + config: { retryCount: 0 } as ExtendedAxiosRequestConfig, response: { status: 500, statusText: "Internal Server Error", + headers: {}, + config: {} as ExtendedAxiosRequestConfig, data: { message: "Server Error" }, - headers: headers, - config: {} as InternalAxiosRequestConfig, } as AxiosResponse, - isAxiosError: true, - name: "AxiosError", - message: "Server Error", - toJSON: () => ({}), - }; + } as AxiosError); - // Get the error handler that was registered - const errorHandler = mockAxiosInstance.interceptors.response.errorHandler; - expect(errorHandler).toBeDefined(); - - // Call error handler and expect it to retry - await expect(errorHandler(error)).rejects.toThrow(MetabaseError); + expect(mockRequest).toHaveBeenCalledTimes(2); + await expect(mockRequest.mock.results[1].value).resolves.toEqual({ + data: "success", + }); }); - test("throws non-5xx errors", async () => { - const error = new Error("Server Error") as AxiosError; - error.config = {} as InternalAxiosRequestConfig; - error.response = { - status: 500, - data: { message: "Server Error" }, - statusText: "Server Error", - headers: {}, - config: {} as InternalAxiosRequestConfig, - } as AxiosResponse; - error.isAxiosError = true; - error.name = "AxiosError"; - error.message = "Server Error"; - error.toJSON = () => ({}); - - mockedAxios.create.mockReturnValue({ + test("does not retry on non-5xx errors", async () => { + // Setup mock request + const mockRequest = vi.fn().mockRejectedValue({ + response: { + status: 400, + data: { message: "Bad Request" }, + }, + }); + + const mockAxiosInstance = { + request: mockRequest, + }; + + // Create axios instance with request and interceptor + axiosCreateSpy.mockReturnValue({ + ...mockAxiosInstance, interceptors: { - request: { use: vi.fn() }, response: { - use: (errorHandler: (error: AxiosError) => Promise) => - errorHandler(error), + use: vi.fn((successFn, errorFn) => { + // Store the error handler + (mockAxiosInstance as any).handleError = errorFn; + }), }, }, - } as any); + } as unknown as AxiosInstance); - const client = createMetabaseClient(); - await expect(client.get("/test")).rejects.toThrow( - new MetabaseError("Bad Request", 400, error.response.data), - ); - }); + const _client = createMetabaseClient(); + + const handleError = (mockAxiosInstance as any).handleError; + expect(handleError).toBeDefined(); - afterAll(() => { - vi.unstubAllEnvs(); + expect(mockRequest).toHaveBeenCalledTimes(0); }); }); }); From 7701e568c31de975eb1ff1d741ba4388836292a8 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Mon, 25 Nov 2024 16:59:33 +0000 Subject: [PATCH 09/18] chore: access client request --- api.planx.uk/modules/analytics/metabase/shared/client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.ts b/api.planx.uk/modules/analytics/metabase/shared/client.ts index 9ce5267505..cd0048a25a 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.ts @@ -85,7 +85,7 @@ export const createMetabaseClient = (): AxiosInstance => { originalRequest.retryCount < (config.retries ?? 3) ) { originalRequest.retryCount++; - return client(originalRequest); + return client.request(originalRequest); } // Transform error response From 1aef0ef477ff46d4bd738499282e4c08b8602549 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Mon, 25 Nov 2024 20:37:18 +0000 Subject: [PATCH 10/18] test: Use nock to intercept client requests --- .../analytics/metabase/shared/client.test.ts | 164 +++++++----------- 1 file changed, 61 insertions(+), 103 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index 462d70815a..66cf17439a 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -1,35 +1,25 @@ import axios from "axios"; -import type { - AxiosInstance, - AxiosError, - InternalAxiosRequestConfig, - AxiosResponse, -} from "axios"; -import { validateConfig, createMetabaseClient } from "./client.js"; +import type { AxiosInstance } from "axios"; +import { + validateConfig, + createMetabaseClient, + MetabaseError, +} from "./client.js"; +import nock from "nock"; const axiosCreateSpy = vi.spyOn(axios, "create"); -interface ExtendedAxiosRequestConfig extends InternalAxiosRequestConfig { - retryCount?: number; -} - -interface MockAxiosInstance extends AxiosInstance { - handleError: (error: AxiosError) => Promise; -} - describe("Metabase client", () => { beforeEach(() => { vi.clearAllMocks(); vi.resetModules(); - vi.stubEnv("METABASE_URL_EXT", "https://metabase.mock.com"); - vi.stubEnv("METABASE_API_KEY", "mockmetabasekey"); }); afterEach(() => { vi.unstubAllEnvs(); }); - test("returns configured client", () => { + test("returns configured client", async () => { const _client = createMetabaseClient(); expect(axiosCreateSpy).toHaveBeenCalledWith({ @@ -46,14 +36,14 @@ describe("Metabase client", () => { test("throws error when URL_EXT is missing", () => { vi.stubEnv("METABASE_URL_EXT", undefined); expect(() => validateConfig()).toThrow( - "Missing environment variable 'METABASE_URL_EXT'", + "Missing environment variable 'METABASE_URL_EXT'" ); }); test("throws error when API_KEY is missing", () => { vi.stubEnv("METABASE_API_KEY", undefined); expect(() => validateConfig()).toThrow( - "Missing environment variable 'METABASE_API_KEY'", + "Missing environment variable 'METABASE_API_KEY'" ); }); @@ -69,93 +59,61 @@ describe("Metabase client", () => { }); describe("Error handling", () => { - test.skip("retries then succeeds on 5xx errors", async () => { - // Setup mock responses - const mockRequest = vi - .fn() - .mockRejectedValueOnce({ - config: { retryCount: 0 } as ExtendedAxiosRequestConfig, - response: { - status: 500, - statusText: "Internal Server Error", - headers: {}, - config: {} as ExtendedAxiosRequestConfig, - data: { message: "Server Error" }, - } as AxiosResponse, - isAxiosError: true, - } as unknown as AxiosError) - .mockResolvedValueOnce({ data: "success" }); - - // Create mock axios instance - const mockAxiosInstance = { - request: mockRequest, - interceptors: { - response: { - use: vi.fn((successFn, errorFn) => { - // Store the error handler - (mockAxiosInstance as MockAxiosInstance).handleError = errorFn; - }), - }, - }, - } as unknown as AxiosInstance; - - axiosCreateSpy.mockReturnValue(mockAxiosInstance); - - const _client = createMetabaseClient(); - - // Get the actual error handler - const handleError = (mockAxiosInstance as MockAxiosInstance).handleError; - - // First call should trigger retry - await handleError({ - config: { retryCount: 0 } as ExtendedAxiosRequestConfig, - response: { - status: 500, - statusText: "Internal Server Error", - headers: {}, - config: {} as ExtendedAxiosRequestConfig, - data: { message: "Server Error" }, - } as AxiosResponse, - } as AxiosError); - - expect(mockRequest).toHaveBeenCalledTimes(2); - await expect(mockRequest.mock.results[1].value).resolves.toEqual({ - data: "success", - }); + test("retries then succeeds on 5xx errors", async () => { + const metabaseScope = nock("https://metabase.mock.com"); + + metabaseScope + .get("/test") + .reply(500, { message: "Internal Server Error" }) + .get("/test") + .reply(200, { data: "success" }); + + const client = createMetabaseClient(); + const response = await client.get("/test"); + + expect(response.data).toEqual({ data: "success" }); + expect(metabaseScope.isDone()).toBe(true); + }); + + test("throws an error if all requests fail", async () => { + const metabaseScope = nock("https://metabase.mock.com"); + + metabaseScope + .get("/test") + .times(4) + .reply(500, { message: "Internal Server Error" }); + + const client = createMetabaseClient(); + + try { + await client.get("/test"); + expect.fail("Should have thrown an error"); + } catch (error) { + expect(error).toBeInstanceOf(MetabaseError); + expect((error as MetabaseError).statusCode).toBe(500); + expect(metabaseScope.isDone()).toBe(true); + } }); test("does not retry on non-5xx errors", async () => { - // Setup mock request - const mockRequest = vi.fn().mockRejectedValue({ - response: { - status: 400, - data: { message: "Bad Request" }, - }, - }); + const metabaseScope = nock("https://metabase.mock.com"); + + metabaseScope.get("/test").once().reply(200, { data: "success" }); + + const client = createMetabaseClient(); + const response = await client.get("/test"); + + expect(response.data).toEqual({ data: "success" }); + + // All expected requests were made + expect(metabaseScope.isDone()).toBe(true); + + // No pending mocks left + expect(metabaseScope.pendingMocks()).toHaveLength(0); - const mockAxiosInstance = { - request: mockRequest, - }; - - // Create axios instance with request and interceptor - axiosCreateSpy.mockReturnValue({ - ...mockAxiosInstance, - interceptors: { - response: { - use: vi.fn((successFn, errorFn) => { - // Store the error handler - (mockAxiosInstance as any).handleError = errorFn; - }), - }, - }, - } as unknown as AxiosInstance); - - const _client = createMetabaseClient(); - - const handleError = (mockAxiosInstance as any).handleError; - expect(handleError).toBeDefined(); - - expect(mockRequest).toHaveBeenCalledTimes(0); + // Double check that no other requests were intercepted + const requestCount = metabaseScope.activeMocks().length; + expect(requestCount).toBe(0); }); }); }); From b3947be11f4036b1b20596bb13d109a165799eac Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Tue, 26 Nov 2024 09:46:11 +0000 Subject: [PATCH 11/18] chore: fix lint issues --- .../modules/analytics/metabase/shared/client.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index 66cf17439a..6746ab159c 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -1,5 +1,4 @@ import axios from "axios"; -import type { AxiosInstance } from "axios"; import { validateConfig, createMetabaseClient, @@ -36,14 +35,14 @@ describe("Metabase client", () => { test("throws error when URL_EXT is missing", () => { vi.stubEnv("METABASE_URL_EXT", undefined); expect(() => validateConfig()).toThrow( - "Missing environment variable 'METABASE_URL_EXT'" + "Missing environment variable 'METABASE_URL_EXT'", ); }); test("throws error when API_KEY is missing", () => { vi.stubEnv("METABASE_API_KEY", undefined); expect(() => validateConfig()).toThrow( - "Missing environment variable 'METABASE_API_KEY'" + "Missing environment variable 'METABASE_API_KEY'", ); }); From 301f7d553da2e666349574f8ec8b42fb81a08104 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Thu, 28 Nov 2024 13:13:31 +0100 Subject: [PATCH 12/18] feat: export metabase client with delayed instantiation for test --- .../modules/analytics/metabase/shared/client.test.ts | 4 ++++ api.planx.uk/modules/analytics/metabase/shared/client.ts | 9 ++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index 6746ab159c..f16d2bd196 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -3,6 +3,7 @@ import { validateConfig, createMetabaseClient, MetabaseError, + initializeMetabaseClient, } from "./client.js"; import nock from "nock"; @@ -12,6 +13,9 @@ describe("Metabase client", () => { beforeEach(() => { vi.clearAllMocks(); vi.resetModules(); + process.env.METABASE_URL_EXT = "https://test-metabase-url.com"; + process.env.METABASE_API_KEY = "test-api-key"; + initializeMetabaseClient(); }); afterEach(() => { diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.ts b/api.planx.uk/modules/analytics/metabase/shared/client.ts index cd0048a25a..5691d2083b 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.ts @@ -120,7 +120,10 @@ export const createMetabaseClient = (): AxiosInstance => { return client; }; -// Export singleton instance -export const metabaseClient = createMetabaseClient(); +// Export both client and instance with delayed instantiation for test purposes +export let metabaseClient: AxiosInstance; -export default metabaseClient; +export const initializeMetabaseClient = () => { + metabaseClient = createMetabaseClient(); + return metabaseClient; +}; From e1e0e695065ed71106ca9359e27079d1e81589ac Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Thu, 28 Nov 2024 14:51:17 +0100 Subject: [PATCH 13/18] chore: pass updated mock vars to nock --- .../modules/analytics/metabase/shared/client.test.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index f16d2bd196..8569ec8fc5 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -63,7 +63,12 @@ describe("Metabase client", () => { describe("Error handling", () => { test("retries then succeeds on 5xx errors", async () => { - const metabaseScope = nock("https://metabase.mock.com"); + const baseURL = process.env.METABASE_URL_EXT; + if (!baseURL) { + throw new Error("METABASE_URL_EXT must be defined for tests"); + } + + const metabaseScope = nock(baseURL); metabaseScope .get("/test") @@ -79,7 +84,7 @@ describe("Metabase client", () => { }); test("throws an error if all requests fail", async () => { - const metabaseScope = nock("https://metabase.mock.com"); + const metabaseScope = nock(process.env.METABASE_URL_EXT!); metabaseScope .get("/test") @@ -99,7 +104,7 @@ describe("Metabase client", () => { }); test("does not retry on non-5xx errors", async () => { - const metabaseScope = nock("https://metabase.mock.com"); + const metabaseScope = nock(process.env.METABASE_URL_EXT!); metabaseScope.get("/test").once().reply(200, { data: "success" }); From 170e8974d2d1bef8c7b6d0ed8853e83ab8009dc6 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Thu, 28 Nov 2024 18:20:39 +0100 Subject: [PATCH 14/18] test: test configured client rather than axios --- .../analytics/metabase/shared/client.test.ts | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index 8569ec8fc5..4084b28856 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -23,16 +23,10 @@ describe("Metabase client", () => { }); test("returns configured client", async () => { - const _client = createMetabaseClient(); - - expect(axiosCreateSpy).toHaveBeenCalledWith({ - baseURL: process.env.METABASE_URL_EXT, - headers: { - "X-API-Key": process.env.METABASE_API_KEY, - "Content-Type": "application/json", - }, - timeout: 30_000, - }); + const client = createMetabaseClient(); + expect(client.defaults.headers["X-API-Key"]).toBe( + process.env.METABASE_API_KEY, + ); }); describe("validates configuration", () => { From eaa7fc26f6432368ccea6f278a473c78728c058b Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Thu, 28 Nov 2024 18:22:50 +0100 Subject: [PATCH 15/18] test: remove unnecessary initialisation function --- .../modules/analytics/metabase/shared/client.test.ts | 2 -- .../modules/analytics/metabase/shared/client.ts | 12 ++++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index 4084b28856..cd6c479950 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -3,7 +3,6 @@ import { validateConfig, createMetabaseClient, MetabaseError, - initializeMetabaseClient, } from "./client.js"; import nock from "nock"; @@ -15,7 +14,6 @@ describe("Metabase client", () => { vi.resetModules(); process.env.METABASE_URL_EXT = "https://test-metabase-url.com"; process.env.METABASE_API_KEY = "test-api-key"; - initializeMetabaseClient(); }); afterEach(() => { diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.ts b/api.planx.uk/modules/analytics/metabase/shared/client.ts index 5691d2083b..17c4b7b07c 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.ts @@ -120,10 +120,10 @@ export const createMetabaseClient = (): AxiosInstance => { return client; }; -// Export both client and instance with delayed instantiation for test purposes -export let metabaseClient: AxiosInstance; +// // Export both client and instance with delayed instantiation for test purposes +// export let metabaseClient: AxiosInstance; -export const initializeMetabaseClient = () => { - metabaseClient = createMetabaseClient(); - return metabaseClient; -}; +// export const initializeMetabaseClient = () => { +// metabaseClient = createMetabaseClient(); +// return metabaseClient; +// }; From ee49ce2d907f708fd00e1ef8be27c31f0e6ea872 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Thu, 28 Nov 2024 18:31:15 +0100 Subject: [PATCH 16/18] test: load envs from .env.test --- api.planx.uk/modules/analytics/metabase/shared/client.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index cd6c479950..9e5ad2a87c 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -12,8 +12,6 @@ describe("Metabase client", () => { beforeEach(() => { vi.clearAllMocks(); vi.resetModules(); - process.env.METABASE_URL_EXT = "https://test-metabase-url.com"; - process.env.METABASE_API_KEY = "test-api-key"; }); afterEach(() => { From c033f24ffda5b7dd7dcbb60aa1ffacb4248e7a10 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Fri, 29 Nov 2024 09:49:58 +0100 Subject: [PATCH 17/18] test: remove unnecessary baseURL variable and check --- .../modules/analytics/metabase/shared/client.test.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index 9e5ad2a87c..ebcf26aee1 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -53,12 +53,7 @@ describe("Metabase client", () => { describe("Error handling", () => { test("retries then succeeds on 5xx errors", async () => { - const baseURL = process.env.METABASE_URL_EXT; - if (!baseURL) { - throw new Error("METABASE_URL_EXT must be defined for tests"); - } - - const metabaseScope = nock(baseURL); + const metabaseScope = nock(process.env.METABASE_URL_EXT!); metabaseScope .get("/test") From 6f0d419819af15e3da704ff4497e98e7320baa68 Mon Sep 17 00:00:00 2001 From: zz-hh-aa Date: Fri, 29 Nov 2024 09:54:11 +0100 Subject: [PATCH 18/18] test: add all setup vars to configured client test --- api.planx.uk/modules/analytics/metabase/shared/client.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index ebcf26aee1..564909ef6a 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -20,9 +20,12 @@ describe("Metabase client", () => { test("returns configured client", async () => { const client = createMetabaseClient(); + expect(client.defaults.baseURL).toBe(process.env.METABASE_URL_EXT); expect(client.defaults.headers["X-API-Key"]).toBe( process.env.METABASE_API_KEY, ); + expect(client.defaults.headers["Content-Type"]).toBe("application/json"); + expect(client.defaults.timeout).toBe(30_000); }); describe("validates configuration", () => {