-
Notifications
You must be signed in to change notification settings - Fork 5
make a proper ApiGatewayToSqs event #3160
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
Conversation
| "ResponseTemplates": { | ||
| "application/json": "{ "status": "accepted" }", | ||
| }, | ||
| "SelectionPattern": "2\\d{2}", |
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.
this means it will only use 200 response if the SQS response is a 200.
Previously it was using 200 for everything, even if SQS rejected the request due to permissions or other issues
| { | ||
| "ResponseTemplates": { | ||
| "application/json": "{ "message": "Internal Server Error - could not queue message" }", | ||
| }, | ||
| "StatusCode": "500", | ||
| }, |
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.
by default for any non-2xx response from SQS it will now return a 500. e.g. if there is an SQS outage or credentials issue.
Hopefully this will do two new things:
- prompt ticket tailor to retry,
- trigger the existing API gateway 5xx alarm
(The only tricky thing is investigating - I'm not sure where API gateway logs the integration responses - but this is an existing issue)
| "application/json": "#set($json = "{") | ||
| #set($json = $json + """pathParameters"":{") | ||
| #set($allHeaders = $input.params().path) | ||
| #foreach($header in $allHeaders.keySet()) | ||
| #set($json = $json + """$header"":""$util.escapeJavaScript($allHeaders.get($header)).replaceAll(""\\\\'"",""'"")""") | ||
| #if($foreach.hasNext) | ||
| #set($json = $json + ",") | ||
| #end | ||
| #end | ||
| #set($json = $json + "},") | ||
| #set($json = $json + """headers"":{") | ||
| #set($allHeaders = $input.params().header) | ||
| #foreach($header in $allHeaders.keySet()) | ||
| #set($json = $json + """$header"":""$util.escapeJavaScript($allHeaders.get($header)).replaceAll(""\\\\'"",""'"")""") | ||
| #if($foreach.hasNext) | ||
| #set($json = $json + ",") | ||
| #end | ||
| #end | ||
| #set($json = $json + "},") | ||
| #set($json = $json + """queryStringParameters"":{") | ||
| #set($allHeaders = $input.params().querystring) | ||
| #foreach($header in $allHeaders.keySet()) | ||
| #set($json = $json + """$header"":""$util.escapeJavaScript($allHeaders.get($header)).replaceAll(""\\\\'"",""'"")""") | ||
| #if($foreach.hasNext) | ||
| #set($json = $json + ",") | ||
| #end | ||
| #end | ||
| #set($json = $json + "},") | ||
| #set($json = $json + """body"":""$util.escapeJavaScript($input.body).replaceAll(""\\\\'"",""'"")"",") | ||
| #set($json = $json + """httpMethod"":""$util.escapeJavaScript($context.httpMethod).replaceAll(""\\\\'"",""'"")"",") | ||
| #set($json = $json + """path"":""$util.escapeJavaScript($context.path).replaceAll(""\\\\'"",""'"")"",") | ||
| #set($json = $json + """mappingSource"": ""SrCDK""}") | ||
| Action=SendMessage&MessageBody=$util.urlEncode($json)", |
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.
this is a bit hard to read, it would be easier to look at the CDK.
In essence it's building a JSON payload similar to an APIGatewayProxyEvent to put into the queue
| function iterateParam(targetName: string, sourceName: string) { | ||
| return `#set($json = $json + """${targetName}"":{") | ||
| #set($allHeaders = $${sourceName}) | ||
| #foreach($header in $allHeaders.keySet()) | ||
| #set($json = $json + """$header"":""$util.escapeJavaScript($allHeaders.get($header)).replaceAll(""\\\\'"",""'"")""") | ||
| #if($foreach.hasNext) | ||
| #set($json = $json + ",") | ||
| #end | ||
| #end | ||
| #set($json = $json + "},")`; | ||
| } | ||
|
|
||
| function insertSingleProp(targetName: string, sourceName: string) { | ||
| return `#set($json = $json + """${targetName}"":""$util.escapeJavaScript($${sourceName}).replaceAll(""\\\\'"",""'"")"",")`; | ||
| } |
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.
these are utility methods to generate VTL that take values or objects from input variables and write them to the json safely
https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-mapping-template-reference.html
https://docs.aws.amazon.com/apigateway/latest/developerguide/rest-api-parameter-mapping-sources.html
| const apiGatewayToSqsEventSchema = z.object({ | ||
| pathParameters: z.record(z.string()), | ||
| headers: z.record(z.string()), | ||
| queryStringParameters: z.record(z.string()), | ||
| body: z.string(), | ||
| httpMethod: z.string(), | ||
| path: z.string(), | ||
| }); | ||
|
|
||
| export type ApiGatewayToSqsEvent = z.infer<typeof apiGatewayToSqsEventSchema>; |
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.
This would be generic for all lambdas using ApiGatewayToSqs, but for now let's leave it here.
| iterateParam(`pathParameters`, `input.params().path`), | ||
| iterateParam(`headers`, `input.params().header`), | ||
| iterateParam(`queryStringParameters`, `input.params().querystring`), | ||
| insertSingleProp(`body`, `input.body`), | ||
| insertSingleProp('httpMethod', 'context.httpMethod'), | ||
| insertSingleProp('path', 'context.path'), |
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.
as you can see I've added in a few bits we don't need for the existing lambda - we only need headers at the moment (in fact just one particular header)
The reason I added more is that this is a reusable construct and no one is going to want to fiddle about with VTL expressions just to put a queue in front of an existing lambda.
The idea of replicating the direct proxy lambda data structure (APIGatewayProxyEvent) is to make it as easy as possible to insert a queue in front of an existing lambda.
There would need to be a bit more tidying up of types before that's possible, but that can happen when it's needed.
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.
we could slightly simplify things if we were using API gateway V2 SQS service integration [1], but I think we would still need to use VTL to build the body correctly, and it would be more invasive to make that much of a change. We should assess whether v2 is something we should move to for some/all of our APIs as it seems cheaper and more lightweight for common/simple use cases.
[1] https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-aws-services-reference.html#SQS-SendMessage
| await processValidSqsRecord(parsedEvent); | ||
| } else { | ||
| console.error('Request failed validation. Processing terminated.'); | ||
| await putMetric('ticket-tailor-webhook-validation-failure'); |
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.
I suspect there was supposed to be an alarm on this value and it was missed
| { | ||
| "StatusCode": "500", | ||
| }, |
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.
we have to tell API gateway that 500 is an allowed response code, (otherwise it returns its own 500 error in place of the one we hard coded)
| export const apiGatewayToSqsEventSchema = z.object({ | ||
| pathParameters: z.record(z.string()), | ||
| headers: z.record(z.string()), | ||
| queryStringParameters: z.record(z.string()), | ||
| body: z.string(), | ||
| httpMethod: z.string(), | ||
| path: z.string(), | ||
| }); | ||
| export type ApiGatewayToSqsEvent = z.infer<typeof apiGatewayToSqsEventSchema>; |
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.
this is new the APIGatewayProxyEvent-like type. It can be moved to some kind of router module as and when it's needed in another lambda
| const parsedEvent: ApiGatewayToSqsEvent = | ||
| apiGatewayToSqsEventSchema.parse(JSON.parse(sqsRecord.body)); |
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.
rather than treating the sqs body as the http body, we actually parse the sqs body to an object with its own nested body (and headers etc).
|
|
||
| if (!(typeof signatureWithTs === 'string')) { | ||
| console.error( | ||
| if (!signatureWithTs) { |
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.
it's always a string now, but it could be missing
|
|
||
| export const emailAddress = `[email protected]`; | ||
|
|
||
| const validBody = `{"id":"wh_1072228","created_at":"2024-08-20 13:20:26","event":"ORDER.CREATED","resource_url":"https:\\/\\/api.tickettailor.com\\/v1\\/orders\\/or_46629271","payload":{"object":"order","id":"or_46629271","buyer_details":{"address":{"address_1":null,"address_2":null,"address_3":null,"postal_code":null},"custom_questions":[],"email":"${emailAddress}","first_name":"joe&^","last_name":"griffiths%","name":"joe&^ griffiths%","phone":null},"created_at":1724160016,"credited_out_amount":0,"currency":{"base_multiplier":100,"code":"gbp"},"event_summary":{"id":"ev_4467889","end_date":{"date":"2024-12-13","formatted":"Fri 13 Dec 2024 10:30 PM","iso":"2024-12-13T22:30:00+00:00","time":"22:30","timezone":"+00:00","unix":1734129000},"event_id":"ev_4467889","event_series_id":"es_1354460","name":"CODE","start_date":{"date":"2024-08-28","formatted":"Wed 28 Aug 2024 6:00 PM","iso":"2024-08-28T18:00:00+01:00","time":"18:00","timezone":"+01:00","unix":1724864400},"venue":{"name":null,"postal_code":null}},"issued_tickets":[{"object":"issued_ticket","id":"it_72697654","add_on_id":null,"barcode":"R59xesv","barcode_url":"https:\\/\\/cdn.tickettailor.com\\/userfiles\\/cache\\/barcode\\/st\\/attendee\\/72697654\\/ef31abaf2ddf8d484483.jpg","checked_in":"false","created_at":1724160026,"custom_questions":[],"description":"General Admission","email":"[email protected]","event_id":"ev_4467889","event_series_id":"es_1354460","first_name":"joe&^","full_name":"joe&^ griffiths%","group_ticket_barcode":null,"last_name":"griffiths%","order_id":"or_46629271","qr_code_url":"https:\\/\\/cdn.tickettailor.com\\/userfiles\\/cache\\/barcode\\/qr\\/attendee\\/72697654\\/9ef2823a01c811da7614.png","reference":null,"reservation":null,"source":"checkout","status":"valid","ticket_type_id":"tt_4328701","updated_at":1724160026,"voided_at":null}],"line_items":[{"object":"line_item","id":"li_96505270","booking_fee":0,"description":"General Admission","item_id":"tt_4328701","quantity":1,"total":0,"type":"ticket","value":0}],"marketing_opt_in":null,"meta_data":[],"payment_method":{"external_id":null,"id":null,"instructions":null,"name":null,"type":"no_cost"},"referral_tag":null,"refund_amount":0,"refunded_voucher_id":null,"status":"completed","status_message":null,"subtotal":0,"tax":0,"tax_treatment":"exclusive","total":0,"total_paid":0,"txn_id":"--"}}`; |
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.
extracted from validSQSRecord.body below
| headers: { | ||
| 'tickettailor-webhook-signature': | ||
| 't=1724160026,v1=a3dbd8cfb0f04a0a9b0dd9d2547f1dd1a51e60d528a4edaee3bc02085517bd50', | ||
| }, |
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.
tickettailor-webhook-signature is here instead of in messageAttributes below
| binaryListValues: [], | ||
| dataType: 'String', | ||
| }, | ||
| export const validSQSBody: ApiGatewayToSqsEvent = { |
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.
now these only need to be the simpler ApiGatewayToSqsEvent rather than the whole SQS record
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.
Pull Request Overview
This PR refactors the ticket-tailor-webhook lambda to improve the ApiGatewayToSqs pattern by moving webhook headers from SQS message attributes into the message body, implementing proper error handling with 500 responses, and replacing console.log with structured logging.
- Modified the API Gateway VTL template to construct a JSON event structure mimicking APIGatewayProxyEvent
- Updated the webhook handler to parse and validate the new event structure using Zod
- Replaced all console.log/console.error/console.warn calls with structured logger calls
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cdk/lib/cdk/ApiGatewayToSqs.ts | Refactored VTL template to build complete event JSON with headers, query parameters, and path parameters; added 500 error response handling |
| handlers/ticket-tailor-webhook/src/apiGatewayToSqsEvent.ts | Added new Zod schema and type definition for the ApiGatewayToSqs event structure |
| handlers/ticket-tailor-webhook/src/index.ts | Updated handler to parse and validate new event structure; replaced console.log with logger |
| handlers/ticket-tailor-webhook/src/validateRequest.ts | Changed to read signature from headers instead of message attributes; replaced console logging with logger |
| handlers/ticket-tailor-webhook/src/idapiService.ts | Replaced console.log/console.error with logger calls |
| handlers/ticket-tailor-webhook/test/testFixtures.ts | Refactored test fixtures to use new ApiGatewayToSqsEvent structure instead of SQS message attributes |
| handlers/ticket-tailor-webhook/test/validateRequest.test.ts | Updated test references to use new fixture names matching the event body structure |
| cdk/lib/snapshots/ticket-tailor-webhook.test.ts.snap | Updated snapshots to reflect new VTL template and error responses |
Co-authored-by: Copilot <[email protected]>
| ## How to Test | ||
|
|
||
| ### Local Testing | ||
| TODO create some runManual scripts to test locally and in CODE. |
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.
this is a TODO for another day, it's always nice when a lambda has
- a quick test script you can run after deploying to CODE, and
- a script to run the local code against CODE identity etc and log the output to test the integrations
| ); | ||
|
|
||
| console.log(`Fetching user type for provided email.`); | ||
| logger.log(`Fetching user type for provided email.`); |
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.
| async function processValidSqsRecord(sqsRecord: ApiGatewayToSqsEvent) { | ||
| const ticketTailorRequest = JSON.parse(sqsRecord.body) as TicketTailorRequest; | ||
| const email = ticketTailorRequest.payload.buyer_details.email; | ||
| logger.mutableAddContext(email); |
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.
| async (sqsRecord) => { | ||
| console.log( | ||
| logger.resetContext(); |
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.
I'm not sure if because it's async it might run the next iteration while it's waiting for the previous one to complete and therefore mix up the contexts?
# Conflicts: # handlers/ticket-tailor-webhook/src/index.ts


Having converted ticket tailor lambda to SrCDK in #3153
I noticed that there are a few issues
This PR aims to fix these issues in the ticket tailor lambda, making the ApiGatewayToSqs pattern more robust and reusable. Main changes:
Bonus updates:
See inline comments.
Tested in CODE
https://eu-west-1.console.aws.amazon.com/cloudwatch/home?region=eu-west-1#logsV2:log-groups/log-group/$252Faws$252Flambda$252Fticket-tailor-webhook-CODE/log-events/2025$252F11$252F21$252F$255B$2524LATEST$255Dc927c51551ae4816af123a0b2be7072f