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

Use Swift HTTP types package for status and method #47

Merged
merged 2 commits into from
May 23, 2024

Conversation

jsonfry
Copy link
Contributor

@jsonfry jsonfry commented Mar 25, 2024

Use Swift HTTP types package for status and method

Motivation:

As mentioned in #46 there is some overlap between the definitions of http types in this library and in the recent Apple Swift HTTP Types library.

This means when working with Swift HTTP Types, you have to convert from the AWS Lambda Events representation of http types into Swift HTTP Types.

Modifications:

I have replaced the request method and response status Swift types in this library with those from Swift HTTP Types.

Result:

Now you must use Swift HTTP Types for request methods and response types.

Further discussion:

No Headers?

I initially indented to also replace the representation of headers to Swift HTTP Types' HTTPField, but this turned out to be... non-trivial. The way AWS encodes the different forms of headers in different situations and the way the Swift HTTP Types library handles encoding and decoding are both (of course) very different, so you can't just replace public let headers: [String: String] with public let header: HTTPFields.

I think I could have added manual encode and decode functions to the various types that have headers to support this, rather than relying on the synthized codable functions. But that seemed like overkill and a future maintenance burden that I am not in a place to decide about.

Extending Status and Method to add Codable support

In Swift HTTP Types, the higher level structs support Codable, but Method and Status themselves don't directly support Codable. Instead the structs that include Method and Status handle the encoding / decoding of those types. This means I've had to add a Codable implementation for Method and Status in this library. This seems a little odd to me. I wonder if that would be better placed in Swift HTTP Types?

@tomerd
Copy link
Contributor

tomerd commented Mar 25, 2024

@swift-server-bot test this please

@tomerd
Copy link
Contributor

tomerd commented Mar 25, 2024

seem like a great direction to me!

@tomerd tomerd requested a review from fabianfett March 25, 2024 16:54
@jsonfry
Copy link
Contributor Author

jsonfry commented May 14, 2024

Anything holding this up? I can see a soundness check failed but I can't see the details of that

@tomerd
Copy link
Contributor

tomerd commented May 15, 2024

@swift-server-bot test this please

@tomerd
Copy link
Contributor

tomerd commented May 15, 2024

yes, lets address the soundness checks violations. re-kicked it so we have a fresh copy

@tomerd
Copy link
Contributor

tomerd commented May 15, 2024

@swift-server-bot test this please

@tomerd
Copy link
Contributor

tomerd commented May 15, 2024

I am +1 to take this. @sebsto thoughts?

@sebsto
Copy link
Contributor

sebsto commented May 16, 2024

I think this is a good idea to align on the HTTP types package.

I would suggest two actions

  1. Open an issue against HTTP types project to check why / if they can add Codable to Status and Method (or even send them the PR) , Then, follow up depending on them. We might delete the Utils/HTTP.swift file if they implement Codable on their side.
  2. Open an issue here that will serve as TODO to track the later addition of the Headers

This is a breaking change, we will probably need to announce it on the forum and slack (that will allow us to see how many are using these)

I will update https://github.com/swift-server/swift-openapi-lambda when merged.

@jsonfry
Copy link
Contributor Author

jsonfry commented May 22, 2024

I've opened a PR, but there might be problems with this approach regarding encoding / decoding http status as the swift http struct for status also incldudes a reason phrase that would get lost in encoding, see apple/swift-http-types#53

@sebsto
Copy link
Contributor

sebsto commented May 22, 2024

Why would it be lost in encoding ? The Swift Lambda Events for APIGateway HTTP.Status also has a reason phrase

https://github.com/swift-server/swift-aws-lambda-events/blob/main/Sources/AWSLambdaEvents/Utils/HTTP.swift#L123

@jsonfry
Copy link
Contributor Author

jsonfry commented May 22, 2024

It would get lost as my Codable implementation doesn't encode it, it only encodes the status code number. I've added some more detail on the other PR.

@sebsto sebsto merged commit f015dbf into swift-server:main May 23, 2024
6 checks passed
@sebsto
Copy link
Contributor

sebsto commented May 23, 2024

As per discussion apple/swift-http-types#53, it is not a good idea to implement Codable on swift-http-type. Some coding (HTTP Status reason) is specific to the API Gateway.

@sebsto
Copy link
Contributor

sebsto commented 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.

3 participants