Skip to content
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

fix(adapter/aws-lambda): add alb event requestContext undefined check for testing convenience #3691

Merged
merged 3 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 63 additions & 2 deletions runtime-tests/lambda/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
import { Readable } from 'stream'
import { handle, streamHandle } from '../../src/adapter/aws-lambda/handler'
import type { LambdaEvent } from '../../src/adapter/aws-lambda/handler'
import {
ALBProcessor,
EventV1Processor,
EventV2Processor,
getProcessor,
handle,
streamHandle,
} from '../../src/adapter/aws-lambda/handler'
import type {
ALBProxyEvent,
APIGatewayProxyEventV2,
LambdaEvent,
} from '../../src/adapter/aws-lambda/handler'
import type {
ApiGatewayRequestContext,
ApiGatewayRequestContextV2,
Expand Down Expand Up @@ -967,3 +978,53 @@ describe('streamHandle function', () => {
expect(output).toContain(jsonResponsePrelude.replace(nullSequence, ''))
})
})

describe('getProcessor function', () => {
it('Should return ALBProcessor for an ALBProxyEvent event', () => {
const event: ALBProxyEvent = {
httpMethod: 'GET',
path: '/',
body: null,
isBase64Encoded: false,
requestContext: {
elb: {
targetGroupArn:
'arn:aws:elasticloadbalancing:us-east-1:123456789012:targetgroup/lambda-279XGJDqGZ5rsrHC2Fjr/49e9d65c45c6791a',
},
},
}

const processor = getProcessor(event)
yusukebe marked this conversation as resolved.
Show resolved Hide resolved
expect(processor).toBeInstanceOf(ALBProcessor)
})

it('Should return EventV1Processor for an event without requestContext', () => {
const event = {
httpMethod: 'GET',
path: '/',
body: null,
isBase64Encoded: false,
}

// while LambdaEvent RequestContext property is mandatory, it can be absent when testing through invoke-api or AWS Console
// in such cases, a V1 processor should be returned
const processor = getProcessor(event as unknown as LambdaEvent)
expect(processor).toBeInstanceOf(EventV1Processor)
})

it('Should return EventV2Processor for an APIGatewayProxyEventV2 event', () => {
const event: APIGatewayProxyEventV2 = {
version: '2.0',
routeKey: '$default',
headers: { 'content-type': 'text/plain' },
rawPath: '/',
rawQueryString: '',
body: null,
isBase64Encoded: false,
requestContext: testApiGatewayRequestContextV2,
}

const processor = getProcessor(event)
expect(processor).toBeInstanceOf(EventV2Processor)
})
})
15 changes: 9 additions & 6 deletions src/adapter/aws-lambda/handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@
}
}

abstract class EventProcessor<E extends LambdaEvent> {
export abstract class EventProcessor<E extends LambdaEvent> {
protected abstract getPath(event: E): string

protected abstract getMethod(event: E): string
Expand Down Expand Up @@ -272,7 +272,7 @@
}
}

class EventV2Processor extends EventProcessor<APIGatewayProxyEventV2> {
export class EventV2Processor extends EventProcessor<APIGatewayProxyEventV2> {
protected getPath(event: APIGatewayProxyEventV2): string {
return event.rawPath
}
Expand Down Expand Up @@ -315,7 +315,7 @@

const v2Processor: EventV2Processor = new EventV2Processor()

class EventV1Processor extends EventProcessor<Exclude<LambdaEvent, APIGatewayProxyEventV2>> {
export class EventV1Processor extends EventProcessor<Exclude<LambdaEvent, APIGatewayProxyEventV2>> {
protected getPath(event: Exclude<LambdaEvent, APIGatewayProxyEventV2>): string {
return event.path
}
Expand All @@ -333,9 +333,9 @@

protected getCookies(
// eslint-disable-next-line @typescript-eslint/no-unused-vars
event: Exclude<LambdaEvent, APIGatewayProxyEventV2>,

Check failure on line 336 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / Main

'event' is declared but its value is never read.

Check failure on line 336 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / Node.js v18.18.2

'event' is declared but its value is never read.

Check failure on line 336 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / Node.js v20.x

'event' is declared but its value is never read.

Check failure on line 336 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / workerd

'event' is declared but its value is never read.

Check failure on line 336 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / Node.js v22.x

'event' is declared but its value is never read.
// eslint-disable-next-line @typescript-eslint/no-unused-vars
headers: Headers

Check failure on line 338 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / Main

'headers' is declared but its value is never read.

Check failure on line 338 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / Node.js v18.18.2

'headers' is declared but its value is never read.

Check failure on line 338 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / Node.js v20.x

'headers' is declared but its value is never read.

Check failure on line 338 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / workerd

'headers' is declared but its value is never read.

Check failure on line 338 in src/adapter/aws-lambda/handler.ts

View workflow job for this annotation

GitHub Actions / Node.js v22.x

'headers' is declared but its value is never read.
): void {
// nop
}
Expand Down Expand Up @@ -375,7 +375,7 @@

const v1Processor: EventV1Processor = new EventV1Processor()

class ALBProcessor extends EventProcessor<ALBProxyEvent> {
export class ALBProcessor extends EventProcessor<ALBProxyEvent> {
protected getHeaders(event: ALBProxyEvent): Headers {
const headers = new Headers()
// if multiValueHeaders is present the ALB will use it instead of the headers field
Expand Down Expand Up @@ -407,7 +407,7 @@

protected getQueryString(event: ALBProxyEvent): string {
// In the case of ALB Integration either queryStringParameters or multiValueQueryStringParameters can be present not both
/*
/*
In other cases like when using the serverless framework, the event object does contain both queryStringParameters and multiValueQueryStringParameters:
Below is an example event object for this URL: /payment/b8c55e69?select=amount&select=currency
{
Expand Down Expand Up @@ -471,7 +471,10 @@
}

const isProxyEventALB = (event: LambdaEvent): event is ALBProxyEvent => {
return Object.hasOwn(event.requestContext, 'elb')
if (event.requestContext) {
return Object.hasOwn(event.requestContext, 'elb')
}
return false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not covered

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's from the report before I updated the PR with the tests. After adding the tests, the coverage report has not run as it's awaiting maintainer approval.

I ran the coverage locally (bun coverage) after adding the tests and it's all green.
hono-lambda-coverage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
New tests are not executed since approval needed

}

const isProxyEventV2 = (event: LambdaEvent): event is APIGatewayProxyEventV2 => {
Expand Down
Loading