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

Improves Developer Ergonomics for LambdaHandler Conformances #272

Closed
wants to merge 14 commits into from

Conversation

hectormatos2011
Copy link

@hectormatos2011 hectormatos2011 commented Sep 30, 2022

This PR aims to improve developer ergonomics for simple LambdaHandler conformances when trying to quickly put together a local server to test against.

Motivation:

Swift 5.7 added primary associated types for protocols where you can start using generic bracket syntax for protocols. An issue for this was opened not too long ago in this repo (#266) with the primary motivation of improving developer ergonomics around types conforming to LambdaHander. However, swift's type inference system should be able to handle the aforementioned issue just fine without using primary associated types (although support for that could still be useful when defining properties conforming to LambdaHandler when you want to constrain the types).

Swift's type inference system does not pick up the Event and Output types for objects conforming to LambdaHandler because those types are defined in the covariant protocol EventLoopLambdaHandler. This means that you need to add type aliases for Event and Output when conforming to LambdaHandler - resulting in less than ideal developer ergonomics when trying to spin up a quick little local server.

Additionally, the initializer for LambdaHandler is currently a required function in the protocol. This is super useful for setup logic for your Handlers but degrades the developer ergonomics of the most simple/minimal use-case for LambdaHandlers. Developers should be able to continue having the capability of writing simple lambda handlers like they could do in 0.5.2:

import AWSLambdaRuntime
import Foundation

Lambda.run { (context, input: Request, callback: @escaping (Result<String, Error>) -> Void) in
    callback(.success(...))
}

The current state of affairs in comparison is to explicitly define the Event and Output associated types of LambdaHandler when writing a simple lambda as well as being forced to include an init that isn't necessarily always required to successfully run a simple lambda:

import AWSLambdaRuntime
import Foundation

@main
struct EntryHandler: LambdaHandler {
    typealias Event = SomeCodableEventType
    typealias Output = SomeCodableOutputType
   
    init(context: AWSLambdaRuntimeCore.LambdaInitializationContext) async throws {
        // The body of this could be blank and the lambda would still work
    }
   
    func handle(_ event: Event, context: LambdaContext) async throws -> Output {
        // lambda logic
    }
}

With the changes on main, a developer loses the ease of writing a three-line lambda that they could do in 0.5.2.

On top of all of this, when needing to run a server locally in Xcode, a developer would need to add an environment variable to their local scheme. This isn't a much used feature in Xcode for developers coming from iOS who are looking to maybe start writing swift on the server. This isn't too big of a deal, but could be a bit of pain to do when trying to set yourself up with Lambda for the first time. It also introduces the possibility of having to manage two different environment variables if you're switching between debugging in Xcode or running the server locally in your Terminal. It would be nice to have the capability of defining this in code so a developer wouldn't have to open a separate window to toggle a server running locally.

I may be wrong about this (not a lot of experience writing swift on the server), but if a developer is running a local server in Xcode, it should be safe to assume that the server isn't running on an environment like AWS Lambda or Docker. I think that if a server is running their Lambda in Xcode, LOCAL_LAMBDA_SERVER_ENABLED should be enabled by default. Doing so would enable developers to package ready-to-go example local servers with their example mobile apps (maybe they want to demonstrate the usage of an SDK or something).

Currently, developers in this situation would have to instruct their users to set up this LOCAL_LAMBDA_SERVER_ENABLED environment variable. If a user is a swift developer already, they might have the urge to open that Package in Xcode and just hit run without knowing they need to set that variable up.

With the support of a variable for enabling running as a local server in Lambda directly, a developer could package a ready-to-go local server package with their example apps that would only require opening the package and running in Xcode with no extra set up.

Modifications:

  • Removes ambiguities between LambdaHandler, EventLoopLambdaHandler, and ByteBufferLambdaHandler. The handle(_:context:) functions shared across all three prevented the swift compiler from properly inferring the Event and Output types between LambdaHandler and EventLoopLambdaHandler. The handle(_:context:) functions are now more descriptive with their primary parameter to match the protocol they are associated with:
    • LambdaHandler.handle(_:context) has now been changed to LambdaHandler(request:context:)
    • EventLoopLambdaHandler.handle(_:context) has now been changed to EventLoopLambdaHandler(event:context:)
    • ByteBufferLambdaHandler.handle(_:context) has now been changed to ByteBufferLambdaHandler(buffer:context:)
  • Adds an init function to the LambdaHandler protocol
    • Adds a default implementation for init(context:) that calls through to init() - this allows fully qualified structs conforming to LambdaHandler to omit their initializers if no setup is required for the Lambda handler
  • Adds a static var isLocalServerEnabled to the ByteBufferLambdaHandler protocol
    • Default is true when running in Xcode, but still respects the LOCAL_LAMBDA_SERVER_ENABLED environment variable.
    • This defaults to false if running outside of Xcode
  • Updates tests with new changes

Result:

With all of the changes in this PR, a developer could package a local lambda handler Swift Package with an example app that could look like this:

@main
final class EntryHandler: LambdaHandler {
    func handle(request: YourDecodableRequest, context: LambdaContext) -> YourEncodableResponse {
        // lambda logic
    }
}

All previous logic is still retained but the optional code needed is now opt-in with sensible defaults.

Hector Matos added 8 commits September 14, 2022 13:00
…ng. Try putting the override as an instance function of the initialization context struct
…etter developer ergonomics for users that are trying to implement a simple Lambda but don't care to do any setup during initialization.
…tocol. This allows a developer to drop the requirement for defining their own typealiases in their LambdaHandler conformances
@swift-server-bot
Copy link

Can one of the admins verify this patch?

14 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

Backtrace.install()
var logger = Logger(label: "Lambda")
logger.logLevel = configuration.general.logLevel
) throws -> Int {
Copy link
Author

@hectormatos2011 hectormatos2011 Sep 30, 2022

Choose a reason for hiding this comment

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

The changes here is just code cleanup that should make the binary a little smaller. The logic here is exactly the same and removes the implicitly unwrapped optional we had for var result: Result<Int, Error>! I can revert this if you don't like it.

}
var general: General = .init()
var lifecycle: Lifecycle = .init()
var runtimeEngine: RuntimeEngine = .init()
Copy link
Author

Choose a reason for hiding this comment

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

This is probably unnecessary if you don't like it but I updated this to be easier to read. You don't necessarily need the init functions you had before if these are just structs. Updating to vars for these internal structs still keeps the same ability to initialize these structs with the default values you had defined in your previous init functions.

Let me know if you don't want this!

@@ -14,13 +14,12 @@

#if compiler(>=5.6)
@preconcurrency import Dispatch
@preconcurrency import Logging
@preconcurrency import NIOCore
Copy link
Author

Choose a reason for hiding this comment

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

Xcode 14 complains about these. I don't think they're needed anymore? If this breaks something before Xcode 14 I may have to remove this

#if compiler(>=5.6)
@preconcurrency import NIOCore
@preconcurrency import NIOHTTP1
#else
Copy link
Author

Choose a reason for hiding this comment

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

Same as before, Xcode 14 says these aren't needed. Will revert this change if it breaks something before Swift 5.7


init() {
self.storage = Storage()
}
Copy link
Author

Choose a reason for hiding this comment

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

More code cleanup. You don't need the init if all you're doing is providing a default value to the private storage variable

XCTAssertEqual(String(describing: shouldFailWithError!), String(describing: error), "expected error to mactch", file: file, line: line)
default:
XCTFail("invalid state")
func assertLambdaRuntimeResult(_ result: @autoclosure () throws -> Int, shouldHaveRun: Int = 0, shouldFailWithError: Error? = nil, file: StaticString = #file, line: UInt = #line) {
Copy link
Author

Choose a reason for hiding this comment

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

Using an autoclosure, we can guarantee that the result closure can get called with try without having to update the test function definitions to using throws.

This also takes advantage of swift error handling which allows us to clean up this logic. It's more readable in my opinion but I can revert this if you don't like it.

@hectormatos2011 hectormatos2011 marked this pull request as draft September 30, 2022 20:54
@hectormatos2011 hectormatos2011 marked this pull request as ready for review September 30, 2022 22:48
@@ -16,7 +16,7 @@ import NIOCore
extension EventLoopLambdaHandler where Event == String {
/// Implementation of a `ByteBuffer` to `String` decoding.
@inlinable
public func decode(buffer: ByteBuffer) throws -> String {
public func decode(buffer: ByteBuffer) throws -> Event {
Copy link
Author

Choose a reason for hiding this comment

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

These changes help to solve ambiguities in the swift compiler across default implementations of EventLoopLambdaHandler when importing AWSLambdaRuntime or running tests for the package in Xcode

@tomerd
Copy link
Contributor

tomerd commented Oct 4, 2022

thanks @hectormatos2011, @fabianfett and myself have been discussing some API changes to the main branch that should help address some of the points you bring up. should be able to put up a PR soon and then we can maybe see what else is missing and you can help get it over the finish line. wdyt?

@hectormatos2011
Copy link
Author

@tomerd I'd love to contribute! Thanks for taking a look. Would you like me to close this one? There's some good nuggets of code in here that should at least help with the type inference issue

@tomerd
Copy link
Contributor

tomerd commented Oct 7, 2022

lets keep it open and focus on landing #273. then we can see what else we need to bring over

@tomerd
Copy link
Contributor

tomerd commented Mar 10, 2023

hi @hectormatos2011 would you like to revisit this now that we have the 1.0 API in place?

@hectormatos2011
Copy link
Author

Let me take a look at 1.0 and close the PR if need be

@hectormatos2011
Copy link
Author

Closing this PR since most of the improvements have been implemented in the 1.0 release!

@tomerd Is it currently possible to have code in the Lambda that makes it always run with LOCAL_LAMBDA_SERVER_ENABLED or do we still have to put that in the target itself? I'd like to be able to deploy example documentation that a developer could just copy and paste into Xcode without having to finagle with Edit Schemes or anything like that

@tomerd
Copy link
Contributor

tomerd commented Mar 14, 2023

@tomerd Is it currently possible to have code in the Lambda that makes it always run with LOCAL_LAMBDA_SERVER_ENABLED or do we still have to put that in the target itself? I'd like to be able to deploy example documentation that a developer could just copy and paste into Xcode without having to finagle with Edit Schemes or anything like that

that is not possible right now, to protect users from accidentally deploying the mock / debug server

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