-
Notifications
You must be signed in to change notification settings - Fork 113
Codable support for APIGateway V2 request and response payloads #129
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
Can one of the admins verify this patch? |
3 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Hi @eneko, thanks for bringing this up and providing a PR.
The problem you're running into is the following. The body is by default encoded as un utf8 string, even if it contains json. That mean's you can not decode the whole structure in one go anyway. The json you receive looks like this: {
"otherProperty": "foo",
"body": "{\"some\":\"json\"}",
"otherProperty": "bar"
} As you can see the While you raise a valid point, I'm unsure if copy and pasting the existing Personally I'd prefer to see extensions on the original event types that provide such functionality. If the original event types need to be open up for those extensions I'm all for it. |
Oh, that makes total sense, thank you @fabianfett! I agree 100%. Knowing how the data is structure, providing a I've updated this PR with these changes, let me know what you think. |
I've added some unit tests and inline documentation. If this looks like a good approach, I'll move the PR to "Ready for Review" |
return nil | ||
} | ||
let data = Data(bodyString.utf8) | ||
return try JSONDecoder().decode(Payload.self, from: data) |
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.
how about:
public func decodedBody<Payload: Codable>(decoder:JSONDecoder = JSONDecoder()) throws -> Payload? {
This way the user has can customize the decoder and provide their own. For example, to change the dateDecodingStrategy.
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.
(Same suggestion for the JSONEncoder usage below)
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.
That's a good point but also yet another instance where "TopLevelDecoder" types would be the right thing to go with... Since we'd like to allow using various coder implementations, not marry the API to Foundation's impl only.
We could do them ad-hoc here in the lambda lib again as Combine does, but that's growing the problem a bit; Or we ignore and go along with this for now, but eventually try to resolve it once we get to it.
// fyi @tomerd
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.
Thanks for the feedback. I've updated the PR to inject both JSONEncoder
and JSONDecoder
.
I agree a "TopLevelDecoder" type would be ideal, let me know if I should do any further changes.
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.
Another idea would be to see if it's possible to push the functions out into an "AWSLambdaFoundationCompat" module or something similar?
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.
@ktoso I was looking at Lambda+Codable.swift
from AWSLambdaRuntime
and it seems like encoding/decoding work could be moved to a separate module, like you suggest. I haven't tried it, though.
Would you be open for that work being done on a separate pull request? While I think it is valuable, it feels out of the initial scope of this work.
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.
Sorry, I don't know the internals here, based on a github search I don't see too many instances of "import Foundation". Would that mean that the lambda runtime wouldn't depend on Foundation
? That might be nice for performance, right?
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.
} | ||
|
||
func testRquestPayloadDecoding() throws { |
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.
typo
/// | ||
/// - Throws: `DecodingError` if body contains a value that couldn't be decoded | ||
/// - Returns: Decoded payload. Returns `nil` if body property is `nil`. | ||
public func decodedBody<Payload: Codable>(decoder: JSONDecoder = JSONDecoder()) throws -> Payload? { |
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.
rename Payload -> 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.
I wonder if instead of decoder: JSONDecoder = JSONDecoder()
which should do something like decoder: (Body) -> String
and have a default impl that uses JSONDecoder
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.
following on that line of thinking, @fabianfett maybe knows if this should be (Body) ->[UInt8]
, or String is good enough
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.
The decoder should be either (String) throws -> Body
or (String.UTF8View) throws -> Body
, since we are decoding the content of the body
property. Will update.
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've given this a shot, but kind of defeats the purpose of the pull request.
Here is what the extension would look like:
public func decodedBody<Body: Codable>(decoder: (String.UTF8View) throws -> Body) throws -> Body? {
guard let utf8 = body?.utf8 else {
return nil
}
return try decoder(utf8)
}
And what the callee would look like:
let decoder: (String.UTF8View) throws -> Body = { utf8 in try JSONDecoder().decode(Body.self, from: Data(utf8)) }
let body: Body? = try request.decodedBody(decoder: decoder)
At that point, using this extension would yield close to no benefit.
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.
@eneko I meant the library would offer a default impl as well, for example (non optimized):
public func decodedBody<Body: Decodable>(decoder: @escaping (String) throws -> Body = Self.jsonDecoder) throws -> Body? {
try self.body.map(decoder)
}
public static func jsonDecoder<T: Decodable>(json: String) throws -> T {
try JSONDecoder().decode(T.self, from: Data(json.utf8))
}
/// - body: `Codable` response payload | ||
/// - cookies: Response cookies | ||
/// - Throws: `EncodingError` if payload could not be encoded into a JSON string | ||
public init<Payload: 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.
rename Payload -> Body
multiValueHeaders: HTTPMultiValueHeaders? = nil, | ||
body: Payload? = nil, | ||
cookies: [String]? = nil, | ||
encoder: JSONEncoder = JSONEncoder() |
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.
tbd.. same comment like the decoder above
@swift-server-bot test this please |
…neko/swift-aws-lambda-runtime into feature/codable-api-gateway-v2
Hi @eneko, if I understand you correctly, your primary goad is to have a one liner to get the json payload out of the The real issue imho is that people can't just use the JSONDecoder to decode strings right away. One thing that would solve this is to make the request decoding a one liner, could be an approach like this: // MARK: - Run Lambda
Lambda.run(APIGatewayProxyLambda())
// MARK: - Handler, Request and Response
struct APIGatewayProxyLambda: EventLoopLambdaHandler {
typealias In = APIGateway.V2.JSONRequest
typealias Out = APIGateway.V2.JSONResponse
struct Request: Codable {
let name: String
}
struct Response: Codable {
let message: String
}
func handle(context: Lambda.Context, event: In, callback: @escaping (Result<Out, Error>) -> Void) {
do {
let jsonRequest = try self.decoder.decode(Request.self, from: event.body)
// ^-- magic here
} catch {
return .success(.init(status: .badRequest))
}
// unimportant right now
}
} There are other JSON libraries out there with which you can do this out of the box, since they use Please also note that we use the default decoder of the lambda handler to increase performance. So I propose the following we add an extension to extension JSONDecoder {
func decode<T: Decodable>(_ type: T.Type, from string: String) throws -> T {
guard let data = Data(from.utf8) else {
preconditionFailure("must never happen")
}
return try self.decode(type, from: data)
}
} This way we would add more convenience to For the encoder we could go into a similar direction with the encoder supporting encoding into a String. @eneko would this work for you? |
Hi @fabianfett, From what I see, that solution could work well. I agree using the default encoder/decoder form Before getting further down this path, let's take a step back. I think it is important to clarify the goal, so we can find the best solution.
I have a bit higher expectations: the real goal, in my opinion, would be to have a zero-liner solution. Let me explain. This is the most basic private struct Request: Codable {
let name: String
}
private struct Response: Codable {
let message: String
}
// In this example we are receiving and responding with JSON using Codable
Lambda.run { (context, request: Request, callback) in
callback(.success(Response(message: "Hello, \(request.name)")))
} The simplicity of this code is great. We should be able to provide the same level of simplicity when working with any type from
I'm not sure about I'm hoping I'm making sense here. The goal, to summarize, would be to automatically decode the body payload, when content type is JSON, and provide this decoded body to the lambda handler method, so users do not have to worry about it. Rewriting the code above using private struct Request: Codable {
let name: String
}
private struct Response: Codable {
let message: String
}
// In this example we are receiving and responding with JSON using Codable
Lambda.run { (context, event: APIGateway.V2.Request<Request>, callback: @escaping (Result<APIGateway.V2.Response<Response>, Error>) -> Void) in
let body = Response(message: "Hello, \(event.body.name)")
callback(.success(APIGateway.V2.Response(statusCode: .ok, body: body)))
} This code would need some tweaks, probably to handle encoding/decoding errors, but maybe we could abstract that too. Again, the goal would be to have a fully decoded body prior to invocation of the handler. This should be viable. It might require exposing Let me know if the makes sense. I appreciate each comment on this review, great feedback, I'm learning a lot. |
I think this would be a useful and nice addition
I tend to agree this would be the optimal user experience |
I'm sure such an In general purpose frameworks this task would require some registering of BodyParsers that then emit create types. This should imho be done as part of a framework that builds on top of AWSLambdaRuntime that should also offer routing. |
@enemo just fyi: not forgotten this PR, hope to share a proposal soon |
Hi @eneko, thank you for you patience. @fabianfett and myself discussed and experimented with various alternatives, and would like to bring up some of the ideas and potential future directions for wider discussion. The desire to have a strongly typed lambda function clearly resonates. Being able to define a As you mention, several AWS event payloads provide a free-from body: e.g. API Gateway, SNS, and SQS. The body could be JSON, and its often is, but it could also be any other format e.g. XML, or a binary blob encoded as base64. As such, in order for the lambda runtime library to utility for body encoding/decoding it's not enough to know the expected type, but we also need to know the expected encoder/decoder. Of course, we can default to JSON to make the API easier, but we need to allow extension points to configure alternatives. Following this line of thinking, the only generic implementation option we could come up with would be to define a new type that represents the body and provide easy-to-use encoding/decoding APIs that default to JSON. For example (illustration purposes only): public struct AWSBody: RawRepresentable, Equatable {
public let rawValue: String
public init?(rawValue: String) {
self.rawValue = rawValue
}
}
extension AWSBody {
public func decode<Body: Decodable>(_ type: Body.Type, decoder: (String) throws -> Body = Self.defaultDecoder) throws -> Body {
try decoder(self.rawValue)
}
public static func defaultDecoder<Body: Decodable>(string: String) throws -> Body {
return try Lambda.defaultJSONDecoder.decode(Body.self, from: string.data(using: .utf8)!) // safe since String.data should never return nil
}
}
extension AWSBody {
public init<Body: Encodable>(_ body: Body, encoder: (Body) throws -> String = Self.defaultEncoder) throws {
self.init(rawValue: try encoder(body))! // safe since AWSBody::init never returns nil
}
public static func defaultEncoder<Body: Encodable>(body: Body) throws -> String {
return try String(data: Lambda.defaultJSONDecoder.encode(body), encoding: .utf8)! // safe since String(data:) from JSON should never return nil
}
} This means the API will not end up being the desired Another approach could be to create a dedicated protocol APIGatewayLambdaHandler: EventLoopLambdaHandler {
associatedtype Request: Decodable
associatedtype Response: Encodable
func handle(context: Lambda.Context, request: Request?) -> EventLoopFuture<Response>
func encode(response: Response) throws -> String?
func decode(request: String) throws -> Request
}
extension APIGatewayLambdaHandler {
func handle(context: Lambda.Context, event: APIGateway.V2.Request) -> EventLoopFuture<APIGateway.V2.Response> {
let requestBody: Request?
do {
requestBody = try event.body.map(self.decode)
} catch {
return context.eventLoop.makeFailedFuture(error)
}
return self.handle(context: context, request: requestBody).flatMapThrowing { response in
let responseBody = try self.encode(response: response)
return APIGateway.V2.Response(statusCode: .ok, body: responseBody)
}
}
}
extension APIGatewayLambdaHandler {
func encode(response: Response) throws -> String? {
try String(data: Lambda.defaultJSONEncoder.encode(response), encoding: .utf8)
}
func decode(request: String) throws -> Request {
try Lambda.defaultJSONDecoder.decode(Request.self, from: request.data(using: .utf8)!)
}
} This approach is much more heavy weight but has the advantage of setting the foundation for providing additional functionality around certain event types. Its especially compelling for api-gateway integration where its common to need other middleware-like functionality such as routing. The challenge with this approach is how far we want to extend the runtime library, or IOW should we leave this kind of more opinionated functionality to libraries that the community builds on top of the runtime library? Of course the approaches are not mutually exclusive - the second can be built on top of the first. WDYT? |
This is great, @tomerd, thanks for getting back. Personally, I like the second approach, with associated types. It looks pretty similar to other code I've worked with, were we have Probably at this point, the best option is to make this a separate Swift Package. Then, we could merge it into this one if it had enough support and met all needs/requirements. I'd be happy to work on that, but might take some time. |
Closing in favor: #144 |
Hi there!
I'm trying to update
APIGateway.V2
request/response so their body can be any payload conforming toCodable
.Since API requests can be many formats other than JSON, I've added
APIGateway.V2.JSONRequest
andAPIGateway.V2.JSONResponse
as separate, generic, types that can be used on Lambda function handlers as In/Out. These are basically a copy ofAPIGateway.V2.Request
andAPIGateway.V2.Response
, but made generic with aCodable
body property.For example, given the following
Request
andResponse
payloads:We could use them with AWS API Gateway V2 as follows:
This code, while it compiles fine, is giving me an error when invoking the function. Here is what I see in CloudWatch:
Thoughts? I think having Codable support for API Gateway V2 payloads is a big plus.
Hopefully we can get it to work, any help would be appreciated.