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): handle multi value headers in AWS Lambda #2494

Merged
merged 8 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
70 changes: 69 additions & 1 deletion src/adapter/aws-lambda/handler.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { isContentTypeBinary, isContentEncodingBinary } from './handler'
import { Hono } from '../../hono'
import { isContentTypeBinary, isContentEncodingBinary, handle } from './handler'
import type { ALBProxyEvent } from './handler'

describe('isContentTypeBinary', () => {
it('Should determine whether it is binary', () => {
Expand Down Expand Up @@ -27,3 +29,69 @@ describe('isContentEncodingBinary', () => {
expect(isContentEncodingBinary('unknown')).toBe(false)
})
})

Copy link
Contributor

Choose a reason for hiding this comment

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

It is complicated, but it seems better to maintain runtime_tests/lambda instead of here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to d51f022
And added tests for APIGW V1 and V2 as well.

describe('handle', () => {
const dummyLambdaContext = {
awsRequestId: '',
callbackWaitsForEmptyEventLoop: false,
functionName: '',
functionVersion: '',
invokedFunctionArn: '',
logGroupName: '',
logStreamName: '',
memoryLimitInMB: '',
getRemainingTimeInMillis(): number {
return 0
},
}

describe('ALB', () => {
const app = new Hono().post('/', (c) => {
if (c.req.header('foo')?.includes('bar')) {
return c.json({ message: 'ok' })
}
return c.json({ message: 'fail' }, 400)
})
const handler = handle(app)

it('Should accept single value headers', async () => {
const event: ALBProxyEvent = {
body: '{}',
httpMethod: 'POST',
isBase64Encoded: false,
path: '/',
headers: {
host: 'localhost',
foo: 'bar',
},
requestContext: {
elb: {
targetGroupArn: '',
},
},
}
const response = await handler(event, dummyLambdaContext)
expect(response?.['statusCode']).toEqual(200)
})

it('Should accept multi value headers', async () => {
const event: ALBProxyEvent = {
body: '{}',
httpMethod: 'POST',
isBase64Encoded: false,
path: '/',
multiValueHeaders: {
host: ['localhost'],
foo: ['bar', 'buz'],
},
requestContext: {
elb: {
targetGroupArn: '',
},
},
}
const response = await handler(event, dummyLambdaContext)
expect(response?.['statusCode']).toEqual(200)
})
})
})
20 changes: 15 additions & 5 deletions src/adapter/aws-lambda/handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export interface APIGatewayProxyEventV2 {
version: string
routeKey: string
headers: Record<string, string | undefined>
multiValueHeaders?: undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

What is your rationale for this? The documentation does not appear to include multiValueHeaders in ver 2.0. https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html

Copy link
Contributor Author

@exoego exoego Apr 11, 2024

Choose a reason for hiding this comment

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

It simplifies code in my opinion.

It allows

event.multiValueHeaders?.['host']?.[0]

instead of

'multiValueHeaders' in event
      ? event.multiValueHeaders.['host']?.[0]
      : undefined

and it also allows

} else if (event.multiValueHeaders) {
  for (const [k, values] of
    Object.entries(event.multiValueHeaders)) {

instead of

 } else if ('multiValueHeaders' in event) {
  for (const [k, values] of
    Object.entries(event.multiValueHeaders || {})) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. The implementation is clean, and I think this is good.


Note.

Below is a note for when the need arises to completely remove 'multiValueHeaders'.

const createRequest = (event: LambdaEvent) => {
  const queryString = extractQueryString(event)
  const domainName =
    event.requestContext && 'domainName' in event.requestContext
      ? event.requestContext.domainName
      : getHostFromEvent(event)

  const path = isProxyEventV2(event) ? event.rawPath : event.path
  const urlPath = `https://${domainName}${path}`
  const url = queryString ? `${urlPath}?${queryString}` : urlPath

  const headers = new Headers()
  getCookies(event, headers)
  if (event.headers) {
    for (const [k, v] of Object.entries(event.headers)) {
      if (v) {
        headers.set(k, v)
      }
    }
  }
  if (isAPIGatewayProxyEvent(event) && event.multiValueHeaders) {
    for (const [k, values] of Object.entries(event.multiValueHeaders)) {
      if (values) {
        values.forEach((v) => headers.append(k, v))
      }
    }
  }

  const method = isProxyEventV2(event) ? event.requestContext.http.method : event.httpMethod
  const requestInit: RequestInit = {
    headers,
    method,
  }

  if (event.body) {
    requestInit.body = event.isBase64Encoded ? Buffer.from(event.body, 'base64') : event.body
  }

  return new Request(url, requestInit)
}

const getHostFromEvent = (event: LambdaEvent): string => {
  if (!event.headers) {
    return 'localhost'
  }

  if (isAPIGatewayProxyEvent(event)) {
    return event.headers['host'] ?? event.multiValueHeaders?.['host']?.[0] ?? 'localhost'
  } else {
    return event.headers['host'] ?? 'localhost'
  }
}
const isAPIGatewayProxyEvent = (event: LambdaEvent): event is APIGatewayProxyEvent => {
  return 'multiValueHeaders' in event
}

cookies?: string[]
rawPath: string
rawQueryString: string
Expand Down Expand Up @@ -63,7 +64,8 @@ export interface APIGatewayProxyEvent {
// When calling Lambda through an Application Load Balancer
export interface ALBProxyEvent {
httpMethod: string
headers: Record<string, string | undefined>
headers?: Record<string, string | undefined>
multiValueHeaders?: Record<string, string[] | undefined>
Copy link
Contributor

Choose a reason for hiding this comment

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

path: string
body: string | null
isBase64Encoded: boolean
Expand Down Expand Up @@ -198,16 +200,24 @@ const createRequest = (event: LambdaEvent) => {
const domainName =
event.requestContext && 'domainName' in event.requestContext
? event.requestContext.domainName
: event.headers['host']
: event.headers?.['host'] ?? event.multiValueHeaders?.['host']?.[0]
const path = isProxyEventV2(event) ? event.rawPath : event.path
const urlPath = `https://${domainName}${path}`
const url = queryString ? `${urlPath}?${queryString}` : urlPath

const headers = new Headers()
getCookies(event, headers)
for (const [k, v] of Object.entries(event.headers)) {
if (v) {
headers.set(k, v)
if (event.headers) {
for (const [k, v] of Object.entries(event.headers)) {
if (v) {
headers.set(k, v)
}
}
} else if (event.multiValueHeaders) {
for (const [k, values] of Object.entries(event.multiValueHeaders)) {
if (values) {
values.forEach((v) => headers.append(k, v))
}
}
}

Expand Down