-
Notifications
You must be signed in to change notification settings - Fork 33
Add (API Gateway) WebSockets Support to Swift for AWS Lambda Events #38
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
Add (API Gateway) WebSockets Support to Swift for AWS Lambda Events #38
Conversation
I think this is a fine approach. @dave-moser @sebsto opinions? |
// | ||
// This source file is part of the SwiftAWSLambdaRuntime open source project | ||
// | ||
// Copyright (c) YEARS Apple Inc. and the SwiftAWSLambdaRuntime project authors |
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.
// Copyright (c) YEARS Apple Inc. and the SwiftAWSLambdaRuntime project authors | |
// Copyright (c) 2023 Apple Inc. and the SwiftAWSLambdaRuntime project authors |
|
||
/// `APIGatewayWebSocketRequest` is a variation of the`APIGatewayV2Request` | ||
/// and contains data coming from the WebSockets API Gateway. | ||
public struct APIGatewayWebSocketRequest: Codable { |
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.
can any of this vibe shared with APIGatewayRequest
or not worth it?
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.
Oh sure ... much of it can be. I just wasn't sure what the correct approach should be. I kind of aimed for a "what's the minimum to make it work" approach. :)
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.
probably some shared struct they can both include as the underlying implementation
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.
If it would be helpful, I would be happy to look at the APIGatewayV2Request
event, as well as my proposed APIGatewayWebSocketRequest
event and extract those items shared between them as a struct
both can use…and then update this pull request.
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.
If it would be helpful, I would be happy to look at the APIGatewayV2Request
event, as well as my proposed APIGatewayWebSocketRequest
event and extract those items shared between them as a struct
both can use…and then update this pull request.
@tomerd @richwolf apologies for my late feedback as I am just back from PTO. I agree with @richwolf approach to not modify My approach would be to factor out all common elements between (and the same for the Thank you Rich for proposing this PR. |
No worries! I hope all is good on your end.
I don't want to step on any coding toes ... would you all like me to work on that? I am happy to defer to others if that would be best ... or proceed along those lines if that would free up everyone's time. |
I'm planning on handling some websocket events soonish this year, would love to see this progressed and happy to help if I can? I'll need to make something work regardless of this PR, but obviously it's better to have something that works for all users of this library. |
My apologies @jsonfry…I think this is on me. I've been meaning to return to this at some point but never got to it. I think what the group wanted was to see if it's possible to have a protocol that factored out code common to all flavors of the API Gateway v2 request. I started that work, just never finished it. Lemme see if I can fix it up in the next couple of days and update this PR. |
Thank you so much! |
@tomerd, @sebsto…in catching back up with this, I notice that |
Yes :D consider splitting to multiple PRs to make it easier to review and make progress |
Oh wow, that's really sage advice! Note to self: always try to make the reviewers' lives simpler. :) I'll follow up with a separate PR for |
} | ||
|
||
public let headers: HTTPHeaders? | ||
public let multiValueHeaders: HTTPMultiValueHeaders? |
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 also see
public let queryStringParameters: [String: String]?
public let multiValueQueryStringParameters: [String: [String]]?
come through on a CONNECT event type
Hi @sebsto !! Couple things ... First, my sincere apologies for letting this sit for so long. I truly regret not following up on it. I really should have. I would make a terrible AWS employee! :) Nextly, I am less familiar with Lambda authorizers for API Gateway ... but ... if I am understanding authorizers (in general) correctly ... say a developer were to choose "Cognito" as an authorizer for an resource/method in API Gateway (instead of "Lambda"), then wouldn't that still require what's in this PR ... or, generally speaking, the "traditional" V1 and V2 payloads? But ... and perhaps you would know more ... if AWS were deprecating "Cognito" as an authorizer choice in API Gateway, preferring "Lambda" in future development efforts, then I could see how it would make sense to focus on that. I would say, ultimately, it's really up to folks like you and @tomerd ... but my gut instinct is that the "traditional" V1 and V2 payloads would need to continue to supported to the fullest extent possible. If there are fields missing, my guess is that they should be added. But I wholeheartedly defer to your wisdom here. |
Hey @richwolf Thank you for keeping this updated.
What are the remaining questions / doubts / decisions to take to close the work on this ? |
@sebsto…actually, I feel it is I that owe you an apology here. I've let this sit idle for way too long. I started getting back into this mainly because I submitted this (WebSockets and Swift) as a talk idea for ServerSideSwift 2025…I wanted something to motivate me to get back into working on servers and Swift. So the main question I would have going forward is whether to try to implement what @tomerd asked about earlier…consolidating what is common amongst API Gateway Lambda integrations…v2, WebSockets. I had experimented with creating a struct… |
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
Adds support for API Gateway WebSocket events to the Swift AWS Lambda Events library, enabling Lambda functions to decode and respond to WebSocket frames.
- Introduces
APIGatewayWebSocketRequest
for decoding WebSocket event payloads. - Defines
APIGatewayWebSocketResponse
as an alias toAPIGatewayV2Response
. - Adds a unit test for decoding a
$connect
WebSocket event.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
Sources/AWSLambdaEvents/APIGateway+WebSockets.swift | Defines the WebSocket request struct, its nested types, and the response alias. |
Tests/AWSLambdaEventsTests/APIGateway+WebsocketsTests.swift | Adds a test case for decoding an example connect event. |
Comments suppressed due to low confidence (4)
Sources/AWSLambdaEvents/APIGateway+WebSockets.swift:15
- [nitpick] There’s a missing space between 'the' and the backticked type, and the phrase could be clarified. Consider changing to: '... is a variation of the
APIGatewayV2Request
.'
/// `APIGatewayWebSocketRequest` is a variation of the`APIGatewayV2Request`
Sources/AWSLambdaEvents/APIGateway+WebSockets.swift:55
- The comment references
APIGatewayV2Request
, but the alias actually targetsAPIGatewayV2Response
. Please update the comment to match the correct type.
/// `APIGatewayWebSocketResponse` is a type alias for `APIGatewayV2Request`.
Tests/AWSLambdaEventsTests/APIGateway+WebsocketsTests.swift:75
- Consider adding a complementary test case where the JSON contains a non-null
body
value to verify thatAPIGatewayWebSocketRequest.body
is decoded correctly.
XCTAssertNil(req?.body)
Tests/AWSLambdaEventsTests/APIGateway+WebsocketsTests.swift:1
- [nitpick] The file name uses 'Websockets' but the code uses 'WebSocket'. For consistency, consider renaming the file to 'APIGateway+WebSocketsTests.swift'.
//===----------------------------------------------------------------------===//
It would be great to meet you at Swift Server Conf ! I would suggest to move on and merge this, then evaluate refactoring opportunities. Here is what my assistant think. 1. HTTP Headers and Body Handling
2. Context/Identity Structures
3. Codable/Sendable Conformance
4. CodingKeys Mapping
5. Response Struct Pattern
Candidates for Code Sharing/Abstraction
Summary Table of Common Properties:
|
@sebsto…okay, sounds good! And I have my fingers crossed on the WebSockets talk (I also submitted another talk idea for SQLKit). I suppose I'll find out if the SSSwift folks think my ideas are good ones. I think the formatting thing is trivial…so I'll fix that as you suggest. Lastly, I think Copilot has a reasonable idea for consolidating commonality amongst the API Gateway Lambda integration types…but would something similar then need to be done for the other integrations (S3, DynamoDB, etc.)…to be consistent…or is API Gateway basically the "special case"? |
…om/richwolf/swift-aws-lambda-events into feature/add-websockets-event-types
@richwolf Thank you for your latest changes. I also merged the latest from There are still formatting issues. You can use Also, would you mind updating your test to use Swift Testing instead of XCTests? I just migrated all the tests to swift Testing, you'll find plenty of examples in the |
I think APIGateways have more opportunities for code simplifications because the three event payloads are very similar. I'm not sure there is much overlap between other types (S3 and DynamoDB for example) |
Understood. I know less about the other integration requests…but I "suspected" they were all different enough from API Gateway integrations to make it really not super worthwhile. Still, I thought it best to double-check with a pro. 😄 |
Sorry, I changed tabs to spaces, but I think the formatter wanted even more spacing…or it wasn't happy. I let it make the formatting changes it wanted to make.
Sure, that makes sense. I will do that. |
The |
Yeah, I noticed. It should be all good now. 🤞🏻 I think it's all good now. One test failed, but it's not a API Gateway Websockets test. |
Merged now, thank you @richwolf for your patience. |
Add APIGateway WebSockets Event Type
Motivation:
What I propose is adding WebSockets support to AWS Lambda Events.
Let me begin by stating outright that I am not sure this is the correct approach to take to bring WebSockets to AWS Lambda Events. Therefore, if this pull request is outright rejected, it won't hurt my feelings in the slightest.
API Gateway supports not only RESTful APIs, but also WebSockets. The way that it works is that API Gateway manages WebSockets sessions with clients. Whenever a client sends API Gateway some WebSockets data, API Gateway bundles it up in as an APIGatewayV2 request (at least, according to Amazon) and passes it along to a designated target…usually a Lambda function. This is what a bundled request looks like:
The problem, of course, is that the current
APIGatewayV2Request
type cannot decode that JSON because it is is missing a number of non-optional data values thatAPIGatewayV2Request
expects to exist (e.g.,version
,rawPath
, etc.).There are (at least as far as I can tell) two solutions to make this work. The first is simply to alter the current
APIGatewayV2Request
so that a number of its data values become optionals. I resisted suggesting this because I suspected it could easily break production code (forcing developers toif-let
things). I thought a better solution might simply be to create a new request/response type pair that could accommodate WebSockets APIs.Modifications:
I suggest adding a new event source file to AWS Lambda Events:
APIGateway+WebSockets.swift
containing two new types:APIGatewayWebSocketRequest
andAPIGatewayWebSocketResponse
.APIGatewayWebSocketResponse
would simply be a type alias (since responses require that no changes be made to that type);APIGatewayWebSocketRequest
would be capable of decoding the JSON listed above.A typical Lambda handler supporting WebSockets would look like this:
Note that responses to WebSockets clients (including, potentially, errors) are made through Amazon's
ApiGatewayManagementApi
. However, API Gateway itself always expects some kind of response…this can be a simple as always sending a 200 "OK" back to API Gateway.Result:
The Swift for AWS Lambda Runtime would be able to support API Gateway WebSockets applications.