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

Add an encodable version of the APIGatewayResponse #86

Merged
merged 6 commits into from
Dec 21, 2024

Conversation

sebsto
Copy link
Contributor

@sebsto sebsto commented Dec 20, 2024

Add a convenience initializer to the APIGatewayResponse v2 that accepts an Encodable object

Motivation:

Most Lambda developers will return a JSON object when exposing their function through the API Gateway. The current initializer only accepts a String body as per the AWS message definition.

This obliges all developers to write two lines of code + error handling to encode their object and transform the Data to a string.

Modifications:

Add a new initializer that accepts a body as Encodable

public init<Input: Encodable> (
        statusCode: HTTPResponse.Status,
        headers: HTTPHeaders? = nil,
        body: Input,
        isBase64Encoded: Bool? = nil,
        cookies: [String]? = nil
) throws { ... }

Result:

Developers can now pass a Swift struct to the APIGatewayResponse.

let businessResponse = BusinessResponse(message: "Hello World", code: 200)
return APIGatewayV2Response(statusCode: .ok, body: businessResponse)

@sebsto sebsto added the semver/none No version bump required. label Dec 20, 2024
@sebsto sebsto requested review from adam-fowler and 0xTim December 20, 2024 09:55
@sebsto sebsto self-assigned this Dec 20, 2024
@sebsto
Copy link
Contributor Author

sebsto commented Dec 20, 2024

Thank you @adam-fowler
I simplified the code, removed the isBase64Encoded argument, and added the same initializer to APIGatewayResponse

func testResponseEncoding() {
let resp = APIGatewayResponse(
func testResponseEncoding() throws {
let resp = try APIGatewayResponse(
Copy link
Member

Choose a reason for hiding this comment

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

Have we broken/confused the API before? Since String automatically conforms to Encodable does the new API take precedence over the existing String one meaning it always throws when it doesn't need to? (And always add an extra hop through the encoder that isn't needed, or potentially double encode a JSON body)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah would be good to ensure we have a different signature for the Codable version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.
What type of difference would be acceptable ?

What about changing the order of the parameters. We sacrifice consistency for compatibility.

extension APIGatewayV2Response {

    public init<Input: Encodable>(
        body: Input,
        statusCode: HTTPResponse.Status,
        headers: HTTPHeaders? = nil,
        cookies: [String]? = nil
    ) throws {
        self.init(
            statusCode: statusCode,
            headers: headers,
            body: try body.string(),
            isBase64Encoded: nil,
            cookies: cookies
        )
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'd keep the ordering and make the body parameter different to be explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. I kept the original order and renamed body to encodableBody

@sebsto
Copy link
Contributor Author

sebsto commented Dec 20, 2024

@0xTim @adam-fowler would this be acceptable ? I changed the order of the args.

@adam-fowler
Copy link
Member

@0xTim @adam-fowler would this be acceptable ? I changed the order of the args.

I'd prefer you changed the name of the body parameter.

@sebsto sebsto merged commit 0025b1a into swift-server:main Dec 21, 2024
15 checks passed
@sebsto sebsto deleted the sebsto/APIGateway+Codable branch December 21, 2024 09:26
sebsto added a commit to swift-server/swift-aws-lambda-runtime that referenced this pull request Dec 24, 2024
…initializer (#437)

After the merge of
swift-server/swift-aws-lambda-events#86
I updated the `APIGatewayV2Response` example to use the new initializer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants