-
Notifications
You must be signed in to change notification settings - Fork 5
extract reusable rest client and allow greedy router path parsing #3232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
58a0cfe
cf5669a
a625620
4b6587f
6672107
6061b8d
6c37541
c1b54e2
99eb971
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,24 +80,34 @@ export class Logger { | |
| return String(value); | ||
| } | ||
| if (value instanceof Error) { | ||
| return value.stack ?? ''; | ||
| return ( | ||
| (value.stack ?? '') + | ||
| '\n' + | ||
| this.objectToPrettyString(value) + | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any extra properties of the error don't get printed by node's e.stack by default |
||
| (value.cause ? '\nCaused by: ' + this.prettyPrint(value.cause) : '') | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cause doesn't get printed by node's e.stack by default |
||
| ); | ||
| } | ||
| if (typeof value === 'object' || Array.isArray(value)) { | ||
| try { | ||
| const jsonString = JSON.stringify(value) | ||
| .replace(/"([^"]+)":/g, ' $1: ') // Remove quotes around keys | ||
| .replace(/}$/, ' }'); | ||
| if (jsonString.length <= 80) { | ||
| return jsonString; | ||
| } | ||
| return JSON.stringify(value, null, 2).replace(/"([^"]+)":/g, '$1:'); | ||
| } catch { | ||
| return String(value); | ||
| } | ||
| return this.objectToPrettyString(value); | ||
| } | ||
| return String(value); | ||
| }; | ||
|
|
||
| private objectToPrettyString(object: unknown) { | ||
| try { | ||
| const jsonString = JSON.stringify(object) | ||
| .replace(/"([^"]+)":/g, ' $1: ') // Remove quotes around keys | ||
| .replace(/}$/, ' }'); | ||
| if (jsonString.length <= 80) { | ||
| return jsonString; | ||
| } | ||
| return JSON.stringify(object, null, 2).replace(/"([^"]+)":/g, '$1:'); | ||
| } catch (e) { | ||
| console.error('caught error when trying to serialise log line', e); | ||
| return String(object); | ||
| } | ||
| } | ||
|
|
||
| /* eslint-enable @typescript-eslint/no-explicit-any */ | ||
| /* eslint-enable @typescript-eslint/no-unsafe-argument */ | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { mapPartition, zipAll } from '@modules/arrayFunctions'; | ||
| import { ValidationError } from '@modules/errors'; | ||
| import type { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda'; | ||
| import type { ZodTypeDef } from 'zod'; | ||
| import { z } from 'zod'; | ||
| import { logger } from '@modules/routing/logger'; | ||
|
|
||
|
|
@@ -21,8 +22,8 @@ export type Route<TPath, TBody> = { | |
| parsed: { path: TPath; body: TBody }, | ||
| ) => Promise<APIGatewayProxyResult>; | ||
| parser?: { | ||
| path?: z.Schema<TPath>; | ||
| body?: z.Schema<TBody>; | ||
| path?: z.Schema<TPath, ZodTypeDef, unknown>; | ||
| body?: z.Schema<TBody, ZodTypeDef, unknown>; | ||
|
Comment on lines
+25
to
+26
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this means that we can use .transform on the schema e.g. if we want to turn |
||
| }; | ||
| }; | ||
|
|
||
|
|
@@ -38,18 +39,50 @@ export const NotFoundResponse = { | |
| statusCode: 404, | ||
| }; | ||
|
|
||
| /** | ||
| * if routeParts ends with a greedy `+`, batch together the last eventsParts accordingly | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for some reason, when I try to write algorithmic code in TS it always ends up looking horrible. |
||
| */ | ||
| export function zipRouteWithEventPath( | ||
| routeParts: string[], | ||
| eventParts: string[], | ||
| ) { | ||
| const lastRoutePart: string | undefined = routeParts[routeParts.length - 1]; | ||
| const routeIsGreedy = lastRoutePart?.endsWith('+}'); | ||
| let adjustedEventParts; | ||
| let adjustedRouteParts; | ||
| if (lastRoutePart && routeIsGreedy && routeParts.length < eventParts.length) { | ||
| const excessParts = eventParts.slice(routeParts.length - 1); | ||
| const joinedGreedyValue = excessParts.join('/'); | ||
| adjustedEventParts = [ | ||
| ...eventParts.slice(0, routeParts.length - 1), | ||
| joinedGreedyValue, | ||
| ]; | ||
| const adjustedLastRoutePart = lastRoutePart.replace(/\+}/, '}'); | ||
| adjustedRouteParts = [ | ||
| ...routeParts.slice(0, routeParts.length - 1), | ||
| adjustedLastRoutePart, | ||
| ]; | ||
| } else if (routeParts.length === eventParts.length) { | ||
| adjustedEventParts = eventParts; | ||
| adjustedRouteParts = routeParts; | ||
| } else { | ||
| return undefined; | ||
| } | ||
| return zipAll(adjustedRouteParts, adjustedEventParts, '', ''); | ||
| } | ||
|
|
||
| function matchPath( | ||
| routePath: string, | ||
| eventPath: string, | ||
| ): { params: Record<string, string> } | undefined { | ||
| const routeParts = routePath.split('/').filter(Boolean); | ||
| const eventParts = eventPath.split('/').filter(Boolean); | ||
|
|
||
| if (routeParts.length !== eventParts.length) { | ||
| const routeEventPairs = zipRouteWithEventPath(routeParts, eventParts); | ||
| if (routeEventPairs === undefined) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const routeEventPairs = zipAll(routeParts, eventParts, '', ''); | ||
| const [matchers, literals] = mapPartition( | ||
| routeEventPairs, | ||
| ([routePart, eventPart]) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { zipRouteWithEventPath } from '../src/router'; | ||
|
|
||
| describe('zipRouteWithEventPath', () => { | ||
| test('matches route and event parts of equal length', () => { | ||
| const routeParts = ['benefits', '{benefitId}', 'users']; | ||
| const eventParts = ['benefits', '123', 'users']; | ||
| expect(zipRouteWithEventPath(routeParts, eventParts)).toEqual([ | ||
| ['benefits', 'benefits'], | ||
| ['{benefitId}', '123'], | ||
| ['users', 'users'], | ||
| ]); | ||
| }); | ||
|
|
||
| test('handles greedy route param at the end', () => { | ||
| const routeParts = ['files', '{path+}']; | ||
| const eventParts = ['files', 'a', 'b', 'c.txt']; | ||
| expect(zipRouteWithEventPath(routeParts, eventParts)).toEqual([ | ||
| ['files', 'files'], | ||
| ['{path}', 'a/b/c.txt'], | ||
| ]); | ||
| }); | ||
|
|
||
| test('returns undefined for mismatched lengths (non-greedy)', () => { | ||
| const routeParts = ['benefits', '{benefitId}', 'users']; | ||
| const eventParts = ['benefits', '123']; | ||
| expect(zipRouteWithEventPath(routeParts, eventParts)).toBeUndefined(); | ||
| }); | ||
|
|
||
| test('matches when both are empty', () => { | ||
| expect(zipRouteWithEventPath([], [])).toEqual([]); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| import { Try, Success, Failure } from './try'; | ||
|
|
||
| describe('Success', () => { | ||
| it('should return the value with get', () => { | ||
| const result = Success(42); | ||
| expect(result.get()).toBe(42); | ||
| }); | ||
|
|
||
| it('should return the value with getOrElse', () => { | ||
| const result = Success(42); | ||
| expect(result.getOrElse(100)).toBe(42); | ||
| }); | ||
|
|
||
| it('should have success true', () => { | ||
| const result = Success(42); | ||
| expect(result.success).toBe(true); | ||
| }); | ||
|
|
||
| it('should flatMap to a new Success', () => { | ||
| const result = Success(42); | ||
| const mapped = result.flatMap((n) => Success(n * 2)); | ||
| expect(mapped.success).toBe(true); | ||
| expect(mapped.get()).toBe(84); | ||
| }); | ||
|
|
||
| it('should flatMap to a Failure', () => { | ||
| const result = Success(42); | ||
| const error = new Error('flatMap error'); | ||
| const mapped = result.flatMap(() => Failure(error)); | ||
| expect(mapped.success).toBe(false); | ||
| expect((mapped as Failure<void>).failure).toBe(error); | ||
| }); | ||
|
|
||
| it('should not apply mapError', () => { | ||
| const result = Success(42); | ||
| const mapped = result.mapError(() => new Error('should not be called')); | ||
| expect(mapped.success).toBe(true); | ||
| expect(mapped.get()).toBe(42); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Failure', () => { | ||
| const error = new Error('test error'); | ||
|
|
||
| it('should throw error on get', () => { | ||
| const result = Failure<number>(error); | ||
| expect(() => result.get()).toThrow('test error'); | ||
| }); | ||
|
|
||
| it('should return default value with getOrElse', () => { | ||
| const result = Failure<number>(error); | ||
| expect(result.getOrElse(100)).toBe(100); | ||
| }); | ||
|
|
||
| it('should have success false', () => { | ||
| const result = Failure<number>(error); | ||
| expect(result.success).toBe(false); | ||
| }); | ||
|
|
||
| it('should expose the failure error', () => { | ||
| const result = Failure<number>(error); | ||
| expect(result.failure).toBe(error); | ||
| }); | ||
|
|
||
| it('should not execute flatMap', () => { | ||
| const result = Failure<number>(error); | ||
| const mapped = result.flatMap((n) => Success(n * 2)); | ||
| expect(mapped.success).toBe(false); | ||
| expect((mapped as Failure<number>).failure).toBe(error); | ||
| }); | ||
|
|
||
| it('should apply mapError', () => { | ||
| const result = Failure<number>(error); | ||
| const wrapperMessage = 'mapped error'; | ||
| const mapped = result.mapError( | ||
| (e) => new Error(wrapperMessage, { cause: e }), | ||
| ); | ||
| expect(mapped.success).toBe(false); | ||
| const finalFailure = (mapped as Failure<number>).failure; | ||
| expect(finalFailure.message).toBe(wrapperMessage); | ||
| expect(finalFailure.cause).toBe(error); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Try', () => { | ||
| it('should return Success for successful operation', () => { | ||
| const result = Try(() => 42); | ||
| expect(result.success).toBe(true); | ||
| expect(result.get()).toBe(42); | ||
| }); | ||
|
|
||
| it('should return Failure for throwing operation', () => { | ||
| const error = new Error('operation failed'); | ||
| const result = Try(() => { | ||
| throw error; | ||
| }); | ||
| expect(result.success).toBe(false); | ||
| expect((result as Failure<never>).failure).toBe(error); | ||
| }); | ||
|
|
||
| it('should chain multiple operations with flatMap', () => { | ||
| const result = Try(() => 10) | ||
| .flatMap((n) => Success(n * 2)) | ||
| .flatMap((n) => Success(n + 5)); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.get()).toBe(25); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.