Added AWSLambdaEvents library #35

fabianfett · 2020-03-12T16:22:24Z

Motivation

Lambda has a number of triggers within the AWS ecosystem, these triggers create events that are then handed to Lambda. To allow faster development with Swift and Lambda we should include as many events as possible with our Lambda.

In Go the batteries are included as well: https://github.com/aws/aws-lambda-go/tree/master/events

Changes

Added a library AWSLambaEvents
Added test target AWSLambaEventsTests
The AWSLambaEvents vends the fabianfett/swift-base64-kit Base64 implementation that is also vended in SwiftNIOWebsocket
AWSLambaEvents links Foundation (primarily for Date and DateFormatter)

Open ends

Currently Foundation JSONEncoder and JSONDecoder is backed into the code, I would like to change this before merge

fabianfett · 2020-03-12T23:17:21Z

Local sanity is happy:

$> ./scripts/sanity.sh                                                  
=> Checking linux tests... okay.
=> Checking format... okay.
=> Checking license headers
   * swift-or-c... okay.
   * bash... okay.
   * dtrace... okay.

Sources/AWSLambdaEvents/APIGateway.swift

Sources/AWSLambdaEvents/AWSNumber.swift

tomerd · 2020-03-14T02:56:30Z

Sources/AWSLambdaEvents/S3.swift

+import Foundation
+import NIO
+
+// https://github.com/aws/aws-lambda-go/blob/master/events/s3.go


no better/official doc?

let's see: https://twitter.com/fabianfett/status/1240646471059476480?s=20

tomerd · 2020-03-14T02:59:31Z

Sources/AWSLambdaEvents/SQS.swift

+        case .binary(let byteBuffer):
+            let base64 = byteBuffer.withUnsafeReadableBytes { (pointer) -> String in
+                String(base64Encoding: pointer)
+            }


@weissi not gonna like this :D

if this is a pattern we need for base64 maybe we add a ByteBuffer extension in that area of the code to keep it DRY and minimize chances of getting unsafe access wrong

tomerd · 2020-03-14T03:01:17Z

Sources/AWSLambdaEvents/Utils/Base64.swift

+
+//===----------------------------------------------------------------------===//
+// This is a vendored version from:
+// https://github.com/fabianfett/swift-base64-kit


faster than foundation's?

tldr: yes.

https://github.com/fabianfett/swift-base64-kit#performance

https://github.com/fabianfett/swift-base64-kit/runs/364930115 (Expand Run test)

okay. was this reviewed by the nio team already, or should I review?

the encoding was reviewed afaik, the decoding wasn't.

Tests/AWSLambdaEventsTests/ALBTests+XCTest.swift

tomerd · 2020-03-14T03:04:25Z

Tests/AWSLambdaEventsTests/APIGatewayTests.swift

+        )
+
+        do {
+            let data = try JSONEncoder().encodeAsByteBuffer(resp, allocator: ByteBufferAllocator())


XCTAssertNoThrows, but then reading the value becomes a bit odd ... XCTAssertNoThrows should really be returning T

yes, that's why most of my tests looks like this. one can of course do

func testBla() throws { let a = try blub() }

But if an error is thrown the error isn't printed anywhere in a simple and easy to read way. That's why I decided to go the expressive/repetitive way.

I always write these tests like that now:

func testFoo() /* no throws! */ { var maybeThing: Thing? XCTAssertNoThrow(maybeThing = try makeThing()) guard let thing = maybeThing else { XCTFail("couldn't make thing") return } }

example: https://github.com/apple/swift-nio/blob/52a4cae14d252920b421034fcf28bd6885b32513/Tests/NIOTests/SALChannelTests.swift#L209

I do almost the same as @weissi

var thing: Thing! XCTAssertNoThrow(thing = try makeThing()) ...

@tomerd do you want me to change the tests to your format?

low priority, but would be nicer imo. throwing tests are a bit of a smell

weissi · 2020-03-16T12:05:41Z

Tests/AWSLambdaEventsTests/SQSTests.swift

+        let data = SQSTests.testPayload.data(using: .utf8)!
+        do {
+            let decoder = JSONDecoder()
+            let event = try decoder.decode(SQS.Event.self, from: data)


in this case you could just write

var event: Event? XCTAssertNoThrow(event = try decoder.decode(...) XCTAssertEqual(1, event?.records.first) guard let message = event?.records.first else { XCTFail(...) return }

tomerd · 2020-03-18T04:17:03Z

Sources/AWSLambdaEvents/APIGateway.swift

+
+// https://github.com/aws/aws-lambda-go/blob/master/events/apigw.go
+
+public enum APIGateway {


should we namespace these with AWS to make sure generic terms like APIGateway that may refer to user's own code or other SDK that may be integrated into this will have enough "space"?

We do already have a namespace (the package-name), don't we?

tomerd · 2020-03-18T04:17:32Z

Sources/AWSLambdaEvents/ALB.swift

+            public let elb: ELBContext
+        }
+
+        public let httpMethod: HTTPMethod


should we explore NIO types like this?

Interesting question. As you might have noticed that this is code copied over from my lambda runtime. The reason I choose to expose those NIO types is. I wanted to build something barebone, allowing as many use-cases as possible in an environment that people might be aware of while using only needed Foundation types.

I don't know what API design goal we have in mind for this library? AsyncHTTPClient uses the same nio types btw.

I'd argue that AsyncHTTPClient and this library are at a different level of abstraction and target different audiences. I dont see a strong reason for the target audience of Lambda to need and learn about NIO in general or ByteBuffer specifically, while Data (or [Int8] if we want to avoid Foundation) is a much more natural type for them. when it comes to HTTPMethod and HTTPHeaders, I would argue that we should not leak NIO types as this is an implementation detail leaking to user's APIs, and we can easily wrap them with our own.

I don't know if it makes so much sense to reimplement: HTTPMethod, HTTPStatusCode and HTTPHeaders. What do you think exporting those with:

@_exported import struct NIOHTTP1.HTTPMethod

Otherwise for some code we would just duplicate it I guess.

tomerd · 2020-03-18T04:18:38Z

Sources/AWSLambdaEvents/Utils/DecodableBody.swift

+import struct Foundation.Data
+import class Foundation.JSONDecoder
+
+public protocol DecodableBody {


does this need to be public?

tomerd · 2020-03-18T04:19:26Z

Sources/AWSLambdaEvents/ALB.swift

+    }
+
+    public struct TargetGroupResponse {
+        public let statusCode: HTTPResponseStatus


same question on exposing NIO types

tomerd · 2020-03-18T04:19:34Z

Sources/AWSLambdaEvents/ALB.swift

+    public struct TargetGroupResponse {
+        public let statusCode: HTTPResponseStatus
+        public let statusDescription: String?
+        public let headers: HTTPHeaders?


tomerd · 2020-03-18T04:19:52Z

Sources/AWSLambdaEvents/ALB.swift

+            statusCode: HTTPResponseStatus,
+            statusDescription: String? = nil,
+            headers: HTTPHeaders? = nil,
+            body: String = "",


isn't it optional?

tomerd · 2020-03-18T04:21:41Z

Sources/AWSLambdaEvents/ALB.swift

+        self.isBase64Encoded = try container.decode(Bool.self, forKey: .isBase64Encoded)
+
+        let body = try container.decode(String.self, forKey: .body)
+        self.body = body != "" ? body : nil


why empty string comparison?

tomerd · 2020-03-18T04:21:59Z

Sources/AWSLambdaEvents/ALB.swift

+// MARK: - Response -
+
+extension ALB.TargetGroupResponse: Encodable {
+    public static let MultiValueHeadersEnabledKey =


needs to be public?

tomerd · 2020-03-18T04:23:55Z

Sources/AWSLambdaEvents/ALB.swift

+                singleValueHeaders[name] = value
+            }
+            try container.encode(singleValueHeaders, forKey: .headers)
+        }


maybe you can sue reduce in some of these headers transformations

tomerd · 2020-03-18T04:25:14Z

Sources/AWSLambdaEvents/ALB.swift

+        encoder: JSONEncoder = JSONEncoder()
+    ) throws {
+        var headers = headers ?? HTTPHeaders()
+        headers.add(name: "Content-Type", value: "application/json")


can you shed some light on why this is needed

This is to signal that the content is json. The Application Load Balancer (ALB)/APIGatway api is quite interesting:

You send back a json that mimics the properties of an http response. That's why you've properties like headers, statusCode and payload. So the ALB.TargetGroupResponse here is the json equivalent of a HTTPResponse.

If you want to send json back to the client you will have a json structure that is your httpresponse (ALB.TargetGroupResponse) and within this structure you will have your actual json payload and headers. Thats why I set http headers here within the json payload.

so the outer json payload and http headers (from the actual http response) are there to communicate with the control plane, whereas the inner http headers and payload are communicated back to the user making the request against the apigateway/application load balancer.

Does that make sense?

kinda. since you are receiving headers from the caller, I think we should check that content type is not set before writing on top of it potentially?

tomerd · 2020-03-18T04:47:30Z

Sources/AWSLambdaEvents/SNS.swift

+
+extension SNS.Message: DecodableBody {
+    public var body: String? {
+        return self.message != "" ? self.message : nil


why the empty string?

tomerd · 2020-03-18T04:48:06Z

Sources/AWSLambdaEvents/SNS.swift

+    public struct Message {
+        public enum Attribute {
+            case string(String)
+            case binary(ByteBuffer)


ditto re ByteBuffer

Sources/AWSLambdaEvents/SNS.swift

tomerd · 2020-03-18T04:49:21Z

Sources/AWSLambdaEvents/SNS.swift

+            try container.encode("Binary", forKey: .dataType)
+            try container.encode(base64, forKey: .dataValue)
+        case .string(let string):
+            try container.encode("String", forKey: .dataType)


using const/enum for string/binary?

tomerd · 2020-03-18T04:49:37Z

Sources/AWSLambdaEvents/SQS.swift

+        /// https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_MessageAttributeValue.html
+        public enum Attribute {
+            case string(String)
+            case binary(ByteBuffer)


ditto re ByteBuffer

tomerd · 2020-03-18T04:50:17Z

Sources/AWSLambdaEvents/SQS.swift

+            buffer.moveReaderIndex(to: bytes.count)
+
+            self = .binary(buffer)
+        default:


const to enum for String/Number/Binary

Sources/AWSLambdaEvents/SQS.swift

tomerd · 2020-03-18T04:57:14Z

Sources/AWSLambdaEvents/Utils/DecodableBody.swift

+            data = Data(bytes)
+        } else {
+            // TBD: Can this ever fail? I wouldn't think so...
+            data = payload.data(using: .utf8)!


a guard would be better than force unwrap

tomerd · 2020-03-18T04:57:38Z

Sources/AWSLambdaEvents/Utils/HTTPHeaders+Codable.swift

+            values.forEach { value in
+                nioHeaders.append((key, value))
+            }
+        }


maybe can use reduce?

we could do that, but i'm afraid that this could lead to some ref count overhead since the array would be copied around quite a lot:

we get the target array as input

we need to create a var output = input (ref count)

we write to the output.append() - still referenced by the input, this means CoW

we return output (ref count?!)

maybe if we make all our headers [String: [String]] this becomes simpler / redundant

tomerd

@fabianfett awesome work, very useful addition. comments and ideas inline, did not review base64 code or tests yet

tomerd · 2020-03-20T05:53:08Z

Sources/AWSLambdaEvents/ALB.swift

+        self.statusDescription = statusDescription
+        self.headers = headers
+
+        var buffer = try encoder.encodeAsByteBuffer(payload, allocator: ByteBufferAllocator())


would be nice to find a way to provide an allocator into all such methods

tomerd · 2020-03-20T05:54:34Z

Sources/AWSLambdaEvents/APIGateway.swift

+            public let resourceId: String
+            public let apiId: String
+            public let resourcePath: String
+            public let httpMethod: String


here we are using String and not the nio type. maybe we can do the same in other places, or come up with our own enum?

fixed a stupid bug Remove bytebuffer usages.

fabianfett requested a review from tomerd March 12, 2020 16:22

fabianfett force-pushed the add-lambda-events branch 3 times, most recently from 7864e00 to 1f26995 Compare March 12, 2020 23:16

tomerd reviewed Mar 14, 2020

View reviewed changes

Sources/AWSLambdaEvents/APIGateway.swift Outdated Show resolved Hide resolved

tomerd reviewed Mar 14, 2020

View reviewed changes

Sources/AWSLambdaEvents/APIGateway.swift Outdated Show resolved Hide resolved

tomerd reviewed Mar 14, 2020

View reviewed changes

Sources/AWSLambdaEvents/AWSNumber.swift Outdated Show resolved Hide resolved

tomerd reviewed Mar 14, 2020

View reviewed changes

Tests/AWSLambdaEventsTests/ALBTests+XCTest.swift Outdated Show resolved Hide resolved

tomerd reviewed Mar 14, 2020

View reviewed changes

fabianfett force-pushed the add-lambda-events branch 5 times, most recently from 3ec24d8 to 5020b90 Compare March 16, 2020 10:58

weissi reviewed Mar 16, 2020

View reviewed changes

tomerd mentioned this pull request Mar 18, 2020

Moved Foundation to AWSLambdaRuntime; AWSLambdaRuntime renamed to AWSLambdaRuntimeCore #41

Merged

tomerd reviewed Mar 18, 2020

View reviewed changes

Sources/AWSLambdaEvents/SNS.swift Outdated Show resolved Hide resolved

tomerd reviewed Mar 18, 2020

View reviewed changes

Sources/AWSLambdaEvents/SNS.swift Outdated Show resolved Hide resolved

tomerd reviewed Mar 18, 2020

View reviewed changes

Sources/AWSLambdaEvents/SQS.swift Outdated Show resolved Hide resolved

tomerd reviewed Mar 18, 2020

View reviewed changes

fabianfett force-pushed the add-lambda-events branch 4 times, most recently from f775b5e to db7f9e6 Compare March 19, 2020 16:31

tomerd reviewed Mar 20, 2020

View reviewed changes

fabianfett force-pushed the add-lambda-events branch 9 times, most recently from fb799a6 to 0764f2e Compare March 27, 2020 09:52

Added AWSLambdaEvents library

ee65380

fixed a stupid bug Remove bytebuffer usages.

fabianfett force-pushed the add-lambda-events branch from 0764f2e to ee65380 Compare March 27, 2020 12:52

fabianfett closed this Apr 15, 2020

fabianfett deleted the add-lambda-events branch April 23, 2020 06:36


		// https://github.com/aws/aws-lambda-go/blob/master/events/apigw.go

		public enum APIGateway {

Added AWSLambdaEvents library #35

Added AWSLambdaEvents library #35

Uh oh!

Conversation

fabianfett commented Mar 12, 2020

Motivation

Changes

Open ends

Uh oh!

fabianfett commented Mar 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomerd Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fabianfett Mar 14, 2020 •

edited

Loading

tomerd Mar 14, 2020 •

edited

Loading

tomerd Mar 18, 2020 •

edited

Loading

tomerd Mar 20, 2020 •

edited

Loading

tomerd Mar 18, 2020 •

edited

Loading

tomerd Mar 18, 2020 •

edited

Loading

fabianfett Mar 19, 2020 •

edited

Loading