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

Make status and method directly codable #53

Closed
wants to merge 1 commit into from
Closed

Conversation

jsonfry
Copy link

@jsonfry jsonfry commented May 22, 2024

Closes #51

Over in the Swift AWS Lambda events, we are looking at using some of the Swift HTTP types, see this PR: swift-server/swift-aws-lambda-events#47

One thing I haven't handled in the Codable of HTTPResponse.Status is the reasonPhrase, as that wouldn't work decoding from a lot of places we find a status represented in AWS JSON as just a number. Which makes me thinks if this is even the right thing to do...

/cc @sebsto

@sebsto
Copy link

sebsto commented May 22, 2024

@jsonfry do you have exemples of AWS APIGateway v1 or v2 where status' reason phrase is a number ? Just curious and it would help me to understand

@jsonfry
Copy link
Author

jsonfry commented May 22, 2024

In the current swift aws lambda events implementation reasonPhrase is always set to nil.

All the responses that are encoded into JSON are encoded as just a number. See the current Codable implementation: https://github.com/swift-server/swift-aws-lambda-events/blob/main/Sources/AWSLambdaEvents/Utils/HTTP.swift#L212

Which is also how it should be encdoed, of course: https://docs.aws.amazon.com/lambda/latest/dg/services-apigateway.html#apigateway-types-transforms

The swift http type for Status is a struct that contains a code and an optional reason phrase: (reduced example)

public struct Status {
        public let code: Int
        public let reasonPhrase: String
}

The reason phrase according to the http standard has some reccomended strings, but can technically be anything. If we were to add the Codable implementation that I've done here, and someone added a reason phrase and encoded then decoded it, the reason phrase would be lost.

My concern is that in the aws events repo we have a particular way we want to encode and decode it that isn't neccesarily applicable to every situation of encoding and decoding http status, so maybe there shouldn't actually be a central way of doing it. But I don't maintain either repo so I'm just guessing 😄

@jsonfry
Copy link
Author

jsonfry commented May 22, 2024

I think what I'm saying is essentially the AWS JSON has a specific way it represents http response status, that may not be applicable to all situations, so we should probably handle the coding in the Swift AWS Lambda Events code rather that here.

What do you think? And what do you think that means for http request method?

@sebsto
Copy link

sebsto commented May 22, 2024

I find it frustrating that Lambda HTTP responses to a load balancer DO include a status description but not the responses to the APIGateway.

I'll ask the API Gateway team if the Lambda can provide one or not (or test this myself)

The Lambda .Net folks are having the same discussion
aws/aws-lambda-dotnet#507

@sebsto
Copy link

sebsto commented May 22, 2024

I made a few tests

When forcing a statusDescription in the Lambda response, the APIGateway returns "500 Internal server error"
This blog post mentions the problem too
https://serverless-training.com/articles/api-gateway-vs-application-load-balancer-technical-details/

Sample code in Python

    return {
        "statusCode": 200,
        "statusDescription": "200 OK",
        "body": json.dumps({
            "message": "hello world",
            # "location": ip.text.replace("\n", "")
        }),
    }

I reach out to the APIGateway service team for details.

@guoye-zhang
Copy link
Contributor

Reason phrase is not a part of modern HTTP versions 2 and 3, so it makes sense if some services ignore them. However from the types perspective, it's not great that we would lose some info when serializing it.

@sebsto
Copy link

sebsto commented May 23, 2024

@guoye-zhang Thank you for your explanation. Is there a reason to just add Codable to HTTP Method and not HTTP Status ?

I agree this library should stay application-agnostic and serve all versions of HTTP.

@jsonfry if you also agree, I let you close this PR and we will adopt the Amazon API Gateway-specific encoding that you proposed on swift-server/swift-aws-lambda-events#47

@jsonfry jsonfry closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Status and Method Directly Codable
3 participants