extract reusable rest client and allow greedy router path parsing #3232

johnduffell · 2025-11-19T11:10:08Z

This PR is a pre-refactoring to #3141
(via #3134 although that doesn't depend on this)

Background

There is no standard rest client at the moment, meaning that for each system we access (e.g. SF, Zuora, Mparticle) we have to

remember to implement request logging correctly
remember to use ZOD,
implement oauth header

In reality, it looks like we aren't consistent about those things across the clients.

This PR

extracts a standard rest client which handles all the above and provides convenient typed get/post/etc methods.
refactors the existing zuoraclient to inherit from the above (inheritance is not ideal but lets RestClient expose ready-made get/put/post etc methods.)
I also threw in a couple of other small refactors needed for the above PR - 1) Router support for + in path parameters, and 2) logging improvements.
I even wrote a scala style Try because I couldn't see the wood for the trees when doing zuoraclient error handling.

Tested in CODE, looks ok via discount-api (includes zuora GET, PUT and POST)
https://eu-west-1.console.aws.amazon.com/cloudwatch/home?region=eu-west-1#logsV2:log-groups/log-group/$252Faws$252Flambda$252Fdiscount-api-CODE/log-events/2025$252F11$252F19$252F$255B$2524LATEST$255D99fa80e816244237af0d7d3a1a31ec86

Future work

In the upcoming PR add google auth to the internal/staff only docs endpoints #3141 the AWS cognito client will use the client, as will the lambda-proxy client.
Refactor other clients e.g. SF to use it

johnduffell · 2025-11-19T11:20:05Z

handlers/product-switch-api/test/amendments.test.ts

+		headers: {
+			entries: () => [['Content-Type', 'application/json; charset=utf-8']],
+		},


headers are required for fetch - https://developer.mozilla.org/en-US/docs/Web/API/Response/headers
and TS types reflect that

so I think it's safe for us to require it in the restclient.

johnduffell · 2025-11-19T11:23:04Z

modules/routing/src/logger.ts

 		}
 		if (value instanceof Error) {
-			return value.stack ?? '';
+			return (value.stack ?? '') + '\n' + this.objectToPrettyString(value);


previously if we stashed extra fields into an error, they were not logged anywhere e.g. when the router catches an exception.
Since we are now throwing non-2xx as custom errors, this means the logs don't show all the information.

With this change, the full information is available after the stack trace.

johnduffell · 2025-11-19T11:52:30Z

modules/routing/src/logger.ts

+				(value.stack ?? '') +
+				'\n' +
+				this.objectToPrettyString(value) +
+				(value.cause ? '\nCaused by: ' + this.prettyPrint(value.cause) : '')


cause doesn't get printed by node's e.stack by default

johnduffell · 2025-11-19T11:53:24Z

modules/routing/src/logger.ts

+			return (
+				(value.stack ?? '') +
+				'\n' +
+				this.objectToPrettyString(value) +


any extra properties of the error don't get printed by node's e.stack by default

johnduffell · 2025-11-19T12:07:01Z

modules/routing/src/router.ts

 };

+/**
+ * if routeParts ends with a greedy `+`, batch together the last eventsParts accordingly


for some reason, when I try to write algorithmic code in TS it always ends up looking horrible.

johnduffell · 2025-11-19T14:02:29Z

modules/zuora/src/restClient.ts

@@ -0,0 +1,184 @@
+import { logger } from '@modules/routing/logger';


this file is mostly extracted from zuoraClient.ts

johnduffell · 2025-11-19T14:06:11Z

modules/zuora/src/restClient.ts

+
+		const json: unknown = JSON.parse(result.responseBody);
+		if (!this.isLogicalSuccess || this.isLogicalSuccess(json)) {
+			return schema.parse(json);


this throws a ZodError rather than RestClientError - this hasn't changed and isn't causing an issue, as usually we want to fail at that point, but wrapping it in a RestClientError would give us more infomartion about the http response that led to the parsing error

I'm not sure that isLogicalSuccess is meaningful in a general rest context, it would probably be better to just check the http code and return the result, then the calling code can take care of more domain specific checks.

yeah true I was just avoiding having an implementation of get, put, post etc in zuoraclient, it can just have one check to cover all methods. Will think about it again as I'm sure there's a better way and I agree this way is a bit odd.

just catching up on this, I've made it less intrusive by having a single assertValidResponse fire and forget method that does everything, rather than having two separate methods tangled into the logic. But I don't think there's a way to move it up a level without having a get/put/post etc method in ZuoraClient (which wouldn't be out of the question if we still prefer it)
This is the same situation we had with the scala version, the RestRequestMaker class called jsonIsSuccessful before parsing the json

support-service-lambdas/lib/restHttp/src/main/scala/com/gu/util/resthttp/RestRequestMaker.scala

Line 163 in f89ca55

_ <- if (skipCheck == WithoutCheck) ClientSuccess(()) else jsonIsSuccessful(bodyAsJson)

johnduffell · 2025-11-19T14:08:16Z

modules/zuora/src/restClient.ts

+	public async getRaw(path: string): Promise<RestResult> {
+		return await this.fetchRawBody(logger.getCallerInfo(1))(path, 'GET');
+	}


getRaw didn't used to exist, but it will be useful when reading from non-json endpoints i.e. proxying sensitive versions of things like https://user-benefits.code.dev-guardianapis.com/benefits/list

johnduffell · 2025-11-19T14:10:16Z

modules/zuora/src/restClient.ts

+		path: string,
+		method: string,
+		schema: T,
+		body?: string,


I feel like body should be an object (possibly with a schema to validate it) rather than expecting the caller to stringify it every time, but that hasn't changed and won't in this PR.

johnduffell · 2025-11-19T14:19:29Z

modules/zuora/src/restClient.ts

+	 * Provide any Authorization headers via this method.  They will be sent but not logged.
+	 * @protected
+	 */
+	protected abstract getAuthHeaders: () => Promise<Record<string, string>>;


It looks like zuora client uses BearerTokenProvider, not sure if that should have been used by everything that uses oauth, if so we should let people just pass in the client id/secret+URL and handle it all for free.

johnduffell · 2025-11-19T14:21:47Z

modules/zuora/test/restClient.test.ts

+		it('should handle paths with leading slash', async () => {
+			const schema = z.object({ data: z.string() });
+
+			mockFetchResponse({
+				ok: true,
+				status: 200,
+				body: { data: 'test' },
+				headers: {},
+			});
+
+			await client.get('/path/to/resource', schema);
+
+			expect(fetchMock).toHaveBeenCalledWith(
+				`${mockBaseUrl}/path/to/resource`,
+				expect.any(Object),
+			);
+		});
+
+		it('should handle paths without leading slash', async () => {
+			const schema = z.object({ data: z.string() });
+
+			mockFetchResponse({
+				ok: true,
+				status: 200,
+				body: { data: 'test' },
+				headers: {},
+			});
+
+			await client.get('path/to/resource', schema);
+
+			expect(fetchMock).toHaveBeenCalledWith(
+				`${mockBaseUrl}/path/to/resource`,
+				expect.any(Object),
+			);
+		});
+	});


the existing zuora client allows both with and without a slash, I feel like we should just pick one and stick with it?

Copilot

Pull Request Overview

This PR extracts a reusable RestClient base class to standardize HTTP client implementations across the codebase and adds support for greedy path parameters in the router. The changes refactor the existing ZuoraClient to use the new base class and improve logging of errors with causes.

Key changes:

Introduces a new RestClient abstract base class with standardized request/response handling, logging, and error generation
Refactors ZuoraClient to extend RestClient, reducing duplication and improving consistency
Adds router support for greedy path parameters (e.g., {path+}) to handle variable-length path segments

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
modules/zuora/src/restClient.ts	New reusable REST client base class with typed HTTP methods and error handling
modules/zuora/src/zuoraClient.ts	Refactored to extend RestClient, implementing custom error generation and auth headers
modules/zuora/test/restClient.test.ts	Comprehensive test coverage for the new RestClient base class
modules/zuora/test/zuoraClient.test.ts	Updated mock responses to use `text()` instead of `json()` for consistency
modules/zuora/test/mocks/mockZuoraClient.ts	Updated to reference `restServerUrl` instead of `zuoraServerUrl`
modules/routing/src/router.ts	Added `zipRouteWithEventPath` function to support greedy path parameters
modules/routing/test/zipRouteWithEventPath.test.ts	Test coverage for greedy path parameter handling
modules/routing/src/logger.ts	Enhanced error logging to include error causes and extracted object formatting
modules/routing/test/logger.test.ts	Updated tests for improved error logging with causes
handlers/product-switch-api/test/amendments.test.ts	Updated mock response to use `text()` method

modules/routing/src/logger.ts

rupertbates · 2025-11-19T15:22:18Z

modules/zuora/src/restClient.ts

+
+		const json: unknown = JSON.parse(result.responseBody);
+		if (!this.isLogicalSuccess || this.isLogicalSuccess(json)) {
+			return schema.parse(json);


I'm not sure that isLogicalSuccess is meaningful in a general rest context, it would probably be better to just check the http code and return the result, then the calling code can take care of more domain specific checks.

rupertbates · 2025-11-19T15:25:22Z

modules/zuora/src/zuoraClient.ts

 import { zuoraServerUrl } from './utils';

-export class ZuoraClient {
+export class ZuoraClient extends RestClient {


Might it be better to just pass a RestClient in to ZuoraClient rather than using inheritance?

yeah will do, that will hide implementation details more effectively and be easier to follow

actually we can't unless we want to have "passthrough" get/put/post/etc methods on ZuoraClient, which I would prefer to avoid if we can just implement them once only.

johnduffell · 2025-11-19T20:48:35Z

modules/routing/src/router.ts

+		path?: z.Schema<TPath, ZodTypeDef, unknown>;
+		body?: z.Schema<TBody, ZodTypeDef, unknown>;


this means that we can use .transform on the schema e.g. if we want to turn string into UUID or something

johnduffell · 2025-11-19T20:55:26Z

modules/zuora/src/errors/zuoraErrorHandler.ts

 	// Fallback: unknown error format
-	const statusText = response.statusText || 'Zuora API Error';
-	return new ZuoraError(statusText, response.status, []);
+	const statusText = 'Zuora API Error';


I don't think there was a need for the status text, it was added in the original PR
https://github.com/guardian/support-service-lambdas/pull/2117/files#diff-dd8bf5368c55cff37954c8734389b29820f7baf1d164604d874cce76a5e78306R56
and it should always be standard text for the error code.

… RestClient

johnduffell · 2025-11-19T21:03:36Z

modules/zuora/src/errors/zuoraErrorHandler.ts

 	// Fallback: unknown error format
-	const statusText = response.statusText || 'Zuora API Error';
-	return new ZuoraError(statusText, response.status, []);
+	return new ZuoraError('Zuora API Error', response.statusCode, []);


I don't think there was a need for the status text, it was added in the original PR
https://github.com/guardian/support-service-lambdas/pull/2117/files#diff-dd8bf5368c55cff37954c8734389b29820f7baf1d164604d874cce76a5e78306R56
and it should always be standard text for the error code.

johnduffell · 2025-11-19T21:08:52Z

modules/zuora/src/zuoraClient.ts

+		if (result.statusCode === 429) {
+			logger.log(
+				`Received a 429 rate limit response with response headers ${JSON.stringify(result.responseHeaders)}`,
+			);
+		}


I've kept this here but in reality all the headers are extracted into the RestClientError and will be logged by the router, so this may be redundant.

johnduffell · 2025-11-19T21:09:47Z

modules/zuora/test/mocks/mockZuoraClient.ts


 		super('stage' as Stage, mockTokenProvider);
-		this.zuoraServerUrl = 'https://mock.zuora.com';
+		// @ts-expect-error override for the test


didn't want to make the field public just for the test

extract reusable rest client and allow router path parsing

58a0cfe

johnduffell mentioned this pull request Nov 19, 2025

extract reusable rest client and allow router path parsing #3231

Closed

johnduffell commented Nov 19, 2025

View reviewed changes

include chained exceptions in logging

cf5669a

johnduffell commented Nov 19, 2025

View reviewed changes

extract event chunking function

a625620

johnduffell commented Nov 19, 2025

View reviewed changes

johnduffell added 2 commits November 19, 2025 12:40

fix and add tests for zipRoute function

4b6587f

add basic tests for restClient

6672107

johnduffell changed the title ~~extract reusable rest client and allow router path parsing~~ extract reusable rest client and allow greedy router path parsing Nov 19, 2025

johnduffell force-pushed the jd/extract-restclient-rebase branch from ebcd2f2 to 1a23ebc Compare November 19, 2025 13:57

lint

6061b8d

johnduffell force-pushed the jd/extract-restclient-rebase branch from 1a23ebc to 6061b8d Compare November 19, 2025 14:00

johnduffell commented Nov 19, 2025

View reviewed changes

johnduffell requested a review from Copilot November 19, 2025 14:24

johnduffell marked this pull request as ready for review November 19, 2025 14:24

Copilot AI reviewed Nov 19, 2025

View reviewed changes

modules/routing/src/logger.ts Outdated Show resolved Hide resolved

modules/routing/src/logger.ts Outdated Show resolved Hide resolved

shtukas approved these changes Nov 19, 2025

View reviewed changes

lint

6c37541

rupertbates reviewed Nov 19, 2025

View reviewed changes

johnduffell force-pushed the jd/extract-restclient-rebase branch from 840787c to 670ea8d Compare November 19, 2025 20:47

johnduffell commented Nov 19, 2025

View reviewed changes

johnduffell force-pushed the jd/extract-restclient-rebase branch from 670ea8d to dc5cf38 Compare November 19, 2025 21:02

reduce the intrusiveness of zuora specific response processing in the…

c1b54e2

… RestClient

johnduffell force-pushed the jd/extract-restclient-rebase branch from dc5cf38 to c1b54e2 Compare November 19, 2025 21:03

johnduffell commented Nov 19, 2025

View reviewed changes

add tests for Try

99eb971

This was referenced Nov 20, 2025

extract rest client from zuora client using composition #3241

Closed

compare two zuoraclient/restclient approaches #3242

Open

		@@ -0,0 +1,184 @@
		import { logger } from '@modules/routing/logger';

		path?: z.Schema<TPath, ZodTypeDef, unknown>;
		body?: z.Schema<TBody, ZodTypeDef, unknown>;

extract reusable rest client and allow greedy router path parsing #3232

Are you sure you want to change the base?

extract reusable rest client and allow greedy router path parsing #3232

Uh oh!

Conversation

johnduffell commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

This PR

Future work

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnduffell Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnduffell Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johnduffell commented Nov 19, 2025 •

edited

Loading

johnduffell Nov 19, 2025 •

edited

Loading

johnduffell Nov 19, 2025 •

edited

Loading