feat!: Modularize event streams & event streams auth #741

jbelkins · 2024-05-29T03:56:56Z

Issue #

awslabs/aws-sdk-swift#1384 (Modularize Event Streams)
awslabs/aws-sdk-swift#1385 (Modularize Event Streams Auth)

Description of changes

Event streams are modularized per SRA. Corresponding modules are created in the SDK for event streams as well. Other modules have been created and types have been moved into them, but may not be complete.

Creates the following modules:
- SmithyEventStreamsAPI for types supporting event streams.
- SmithyEventStreamsAuthAPI for types supporting message signing interfaces.
- SmithyEventStreams for the parts of SmithyEventStreamsAPI that have CRT dependencies.

Other changes:

HttpContext has been renamed to Context and moved to a new Smithy module for lightweight sharing across modules.
- Attributes and AttributeKey types have been moved as well.
- Specific attribute keys have been moved to various modules, and have been made private. Public accessors on context are provided in their place.
The ClientRuntime typealiases of Foundation types have been removed, and the corresponding Foundation types are used directly.
Middleware and Handler no longer have a Context associated type, since the Context type is used for all cases.

Scope

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…streams

jbelkins · 2024-05-29T05:08:30Z

Package.swift

+        .library(name: "SmithyEventStreamsAPI", targets: ["SmithyEventStreamsAPI"]),
+        .library(name: "SmithyEventStreamsAuthAPI", targets: ["SmithyEventStreamsAuthAPI"]),
+        .library(name: "SmithyEventStreams", targets: ["SmithyEventStreams"]),
+        .library(name: "SmithyChecksumsAPI", targets: ["SmithyChecksumsAPI"]),


Several new modules added for Smithy modularization.

jbelkins · 2024-05-29T05:10:46Z

Sources/ClientRuntime/Auth/HTTPAuth/DefaultIdentityResolverConfiguration.swift

+import struct Smithy.AttributeKey
+import protocol SmithyIdentityAPI.IdentityResolver
+import protocol SmithyIdentityAPI.IdentityResolverConfiguration
+


Many files contain superficial changes like this, that are related to modularizing the project:

moved or renamed file

changed or added imports

changed namespaces on types

changed visibility of methods or types (i.e. marking them "public")

I won't comment on these types of changes, since they happen frequently in this PR.

jbelkins · 2024-05-29T05:12:17Z

Sources/ClientRuntime/Idempotency/HttpContext+Idempotency.swift

+import class Smithy.Context
+import class Smithy.ContextBuilder
+
+extension Context {


Broke these idempotency-related context extensions out to their own file.

jbelkins · 2024-05-29T14:28:00Z

Sources/ClientRuntime/Auth/HTTPAuthAPI/Signer.swift

-// All Rights Reserved.
-//
-// SPDX-License-Identifier: Apache-2.0
-//


Moved to the SmithyHTTPAuthAPI module

jbelkins · 2024-05-29T14:28:46Z

Sources/ClientRuntime/EventStream/MessageEncoderStream.swift

-// All Rights Reserved.
-//
-// SPDX-License-Identifier: Apache-2.0
-//


Moved to the SmithyEventStreamsAPI module

jbelkins · 2024-05-29T14:29:22Z

Sources/ClientRuntime/Idempotency/Context+Idempotency.swift

+
+import struct Smithy.AttributeKey
+import class Smithy.Context
+import class Smithy.ContextBuilder


Idempotency-related accessors that were broken out from the old HttpContext

jbelkins · 2024-05-29T14:34:37Z

Sources/ClientRuntime/Networking/Checksums/ChecksumAlgorithm.swift

 import AwsCommonRuntimeKit

 public enum HashError: Error {
    case invalidInput
    case hashingFailed(reason: String)
 }

-public enum ChecksumAlgorithm {
-    case crc32, crc32c, sha1, sha256, md5
+extension ChecksumAlgorithm {


ChecksumAlgorithm itself was moved out to SmithyHTTPAuthAPI. This extension remains here because it is dependent on CRT.

SEP says to place checksum things that depend on CRT into its own module, "checksums-crt" module. With our naming pattern, it would be SmithyChecksumsCRT. What was the reason for keeping this in ClientRuntime?

+1 to Chan's comment on moving them to their own module, but agree with keep ChecksumsCRT module in ClientRuntime as checksums themselves aren't AWS specific concepts

jbelkins · 2024-05-29T14:35:38Z

Sources/ClientRuntime/Networking/ClientError.swift

- * SPDX-License-Identifier: Apache-2.0.
- */
-
-import AwsCommonRuntimeKit


This file was split into ClientError in the Smithy module, and HTTPClientError in the SmithyHTTPAPI module (for the path & query cases.)

jbelkins · 2024-05-29T14:53:17Z

Sources/SmithyHTTPAPI/HTTPClientError.swift

+//
+
+/// Errors that may occur when creating a HTTP request.
+public enum HTTPClientError: Error {


Split these errors off of ClientError because these are HTTP-specific.

jbelkins · 2024-05-29T14:53:48Z

Sources/SmithyHTTPAPI/Headers.swift

@@ -233,25 +231,6 @@ extension Header: Comparable {
    }
 }

-extension Headers {


This CRT-dependent extension stayed behind.

jbelkins · 2024-05-29T14:54:16Z

Sources/SmithyHTTPAPI/HttpResponse.swift

@@ -35,33 +36,3 @@ extension HttpResponse: CustomDebugStringConvertible {
        return "\nStatus Code: \(statusCode.description) \n \(headers)"
    }
 }
-
-extension HttpResponse: WireDataProviding {


These extensions stayed behind in ClientRuntime.

jbelkins · 2024-05-29T14:54:44Z

Sources/SmithyHTTPAPI/SdkHttpRequest.swift

@@ -67,56 +73,6 @@ public final class SdkHttpRequest: RequestMessage {
    }
 }

-extension SdkHttpRequest {


CRT-specific code stayed behind.

jbelkins · 2024-05-29T14:56:15Z

Sources/SmithyHTTPAuthAPI/Signer.swift

+import class SmithyHTTPAPI.SdkHttpRequestBuilder
+import protocol SmithyIdentityAPI.Identity
+
+public protocol Signer {


Moved in from ClientRuntime, minus the event stream methods that were split off to MessageDataSigner.

jbelkins · 2024-05-29T14:56:47Z

Sources/SmithyIdentityAPI/AWSCredentialIdentity.swift

+/// A type representing AWS credentials for authenticating with an AWS service
+///
+/// For more information see [AWS security credentials](https://docs.aws.amazon.com/general/latest/gr/aws-security-credentials.html#AccessKeys)
+public struct AWSCredentialIdentity: Identity {


Moved in from SDK. Despite this being SDK-specific, SRA says it belongs here.

jbelkins · 2024-05-29T15:01:46Z

Tests/SmithyEventStreamsTests/DefaultMessageDecoderTests.swift

@@ -0,0 +1,82 @@
+//
+// Copyright Amazon.com Inc. or its affiliates.


Moved in from SDK

jbelkins · 2024-05-29T15:02:02Z

Tests/SmithyEventStreamsTests/DefaultMessageEncoderTests.swift

+// SPDX-License-Identifier: Apache-2.0
+//
+
+import SmithyEventStreams


Moved in from SDK

jbelkins · 2024-05-29T15:02:19Z

Tests/SmithyEventStreamsTests/EventStreamTestData.swift

+//
+
+import SmithyEventStreamsAPI
+import Foundation


Moved in from SDK

jbelkins · 2024-05-29T15:02:36Z

Tests/SmithyEventStreamsTests/MessageTypeTests.swift

+//
+
+import SmithyEventStreamsAPI
+import XCTest


Moved in from SDK

jbelkins · 2024-05-29T15:03:52Z

...-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/AuthSchemeResolverGenerator.kt

@@ -19,6 +19,8 @@ import software.amazon.smithy.swift.codegen.model.boxed
 import software.amazon.smithy.swift.codegen.model.buildSymbol
 import software.amazon.smithy.swift.codegen.model.defaultValue
 import software.amazon.smithy.swift.codegen.model.getTrait
+import software.amazon.smithy.swift.codegen.swiftmodules.SmithyHTTPAuthAPITypes
+import software.amazon.smithy.swift.codegen.swiftmodules.SmithyTypes


Codegen changes are generally about changing types & adding imports. No further comments on codegen or codegen tests.

sichanyoo

Couple small things & questions

sichanyoo · 2024-05-29T21:12:03Z

Package.swift

@@ -28,30 +28,51 @@ let package = Package(
        .watchOS(.v6)
    ],
    products: [
+        .library(name: "Smithy", targets: ["Smithy"]),


nit: It might be better to name this something like SmithyCommon

I am not currently sure about this module's name and exact scope... this is intended to be things that Smithy uses across all modules.

I might break it down further though to have such modules for clients and for models, since it seems some smithy-swift customers use them separately.

I'd like to leave it for now, and we can revisit it later in modularization.

sichanyoo · 2024-05-30T16:15:15Z

Sources/ClientRuntime/Networking/Checksums/ChecksumAlgorithm.swift

 import AwsCommonRuntimeKit

 public enum HashError: Error {
    case invalidInput
    case hashingFailed(reason: String)
 }

-public enum ChecksumAlgorithm {
-    case crc32, crc32c, sha1, sha256, md5
+extension ChecksumAlgorithm {


SEP says to place checksum things that depend on CRT into its own module, "checksums-crt" module. With our naming pattern, it would be SmithyChecksumsCRT. What was the reason for keeping this in ClientRuntime?

sichanyoo · 2024-05-30T16:24:45Z

Sources/ClientRuntime/Networking/Http/SdkHttpRequest+CRT.swift

@@ -0,0 +1,62 @@
+//


Q: This one is kept in ClientRuntime, but header+CRT is moved to new module at Sources/SmithyEventStreams/Header+CRT.swift. What's the criteria used for breaking things with CRT dependency into its own module vs keeping it in ClientRuntime?

Remember: I'm not completely modularizing anything except event streams & event streams auth.

So don't sweat things that should be in HTTP, HTTPAuth, etc but didn't get moved. But if something was moved to the wrong module, we should address that now.

Sources/SmithyEventStreams/Header+CRT.swift is related to event streams headers, not HTTP headers.

So this file goes into SmithyEventStreams because:

it is event stream related

it has a CRT dependency (i.e. it can't go into SmithyEventStreamsAPI)

sichanyoo · 2024-05-30T17:12:09Z

Sources/SmithyEventStreams/DefaultMessageEncoder.swift

+    /// Encodes a `Message` into a `Data` object
+    /// to be sent over the wire.
+    public struct DefaultMessageEncoder: MessageEncoder {
+        public init() {}
+
+        /// Encodes a `Message` into a `Data` object
+        public func encode(message: Message) throws -> Data {
+            let crtMessage = message.toCRTMessage()
+            return try crtMessage.getEncoded()
+        }
+    }


reminder: Extra indent for entire struct here as well, in addition to commented one in Sources/SmithyEventStreams/DefaultMessageEncoderStream.swift

Yeah, those will all be cleaned up later.

There will likely be some time spent on cleanup across the board after all the modules have been completed.

sichanyoo · 2024-05-30T17:24:41Z

Sources/SmithyHTTPAuthAPI/Context+HTTPAuth.swift

+private let authSchemesKey = AttributeKey<Attributes>(name: "AuthSchemes")
+private let authSchemeResolverKey = AttributeKey<AuthSchemeResolver>(name: "AuthSchemeResolver")
+private let selectedAuthSchemeKey = AttributeKey<SelectedAuthScheme>(name: "SelectedAuthScheme")
+private let signingAlgorithmKey = AttributeKey<SigningAlgorithm>(name: "SigningAlgorithmKey")


nit: There's duplicate of signingAlgorithmKey in Sources/SmithyHTTPAPI/Context+HTTP.swift.

This has been cleared up, the other extension was deleted.

…streams

dayaffe

Approved given Checksums is moved to its own module as Chan mentioned

jbelkins added 17 commits May 24, 2024 12:54

Split event streams-related modules out from ClientRuntime

f3a5979

Merge remote-tracking branch 'origin/main' into jbe/modularize_event_…

43724ef

…streams

Move AWSSigningAlgorithm out of smithy-swift

e802518

ClientRuntime builds & tests pass

1ceb2af

All Swift tests pass except WeatherSDK

1d76efb

Clean up SdkHttpRequestBuilder access

2186d53

WeatherSDK regenerated, builds, tests pass

c28efb4

Protocol tests pass

faa8961

Fix lint

35772ab

SDK builds

5ab04b4

Fix ktlint

79c4d37

Fix smithy-swift codegen tests

f0a8aa3

Regenerate WeatherSDK

2a9dc54

Fix types in initial event generator

6ac0242

Fix protocol tests

623e2a3

Fix event streams tests

3f74ad2

Add signingalgorithm extensions to Context

e15f2b8

jbelkins commented May 29, 2024

View reviewed changes

jbelkins and others added 4 commits May 29, 2024 08:54

Fix integration tests; code cleanup

4fbb5fa

Fix SmithyEventStreamsTests

6088055

Code cleanup

d54e356

Merge branch 'main' into jbe/modularize_event_streams

7c9e8d6

jbelkins changed the title ~~Jbe/modularize event streams~~ feat!: Modularize event streams & event streams auth May 29, 2024

jbelkins commented May 29, 2024

View reviewed changes

Tests/SmithyEventStreamsTests/MessageTypeTests.swift

//

import SmithyEventStreamsAPI

import XCTest

Copy link

Contributor Author

jbelkins May 29, 2024

Choose a reason for hiding this comment

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

Moved in from SDK

jbelkins commented May 29, 2024

View reviewed changes

Change scheme name for CI

a4ac939

jbelkins mentioned this pull request May 29, 2024

feat!: Modularize event streams & event streams auth awslabs/aws-sdk-swift#1530

Merged

jbelkins marked this pull request as ready for review May 29, 2024 15:19

jbelkins requested review from dayaffe and sichanyoo May 29, 2024 15:20

sichanyoo reviewed May 30, 2024

View reviewed changes

jbelkins added 3 commits May 30, 2024 17:06

Remove signing region accessor from SmithyHTTPAPI

e0a5dc3

Merge branch 'main' into jbe/modularize_event_streams

039fb4b

Merge remote-tracking branch 'origin/main' into jbe/modularize_event_…

bb2e6cb

…streams

dayaffe approved these changes May 31, 2024

View reviewed changes

jbelkins added 5 commits May 31, 2024 10:20

Incorporate requests/responses, fix tests & lint

75d973b

Adapt symbols to new declaration format

c3d4eba

Convert SigningAlgorithm back to a custom type

c86e96a

Fix SPI imports

cd615c3

Fix ktlint

6e347fe

sichanyoo self-requested a review May 31, 2024 20:58

sichanyoo approved these changes May 31, 2024

View reviewed changes

jbelkins merged commit dfa21d4 into main May 31, 2024
17 checks passed

jbelkins deleted the jbe/modularize_event_streams branch May 31, 2024 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Modularize event streams & event streams auth #741

feat!: Modularize event streams & event streams auth #741

jbelkins commented May 29, 2024 •

edited

Loading

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

sichanyoo May 30, 2024

dayaffe May 31, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

jbelkins May 29, 2024

sichanyoo left a comment

sichanyoo May 29, 2024

jbelkins May 31, 2024

sichanyoo May 30, 2024

sichanyoo May 30, 2024

jbelkins May 30, 2024

jbelkins May 30, 2024

sichanyoo May 30, 2024

jbelkins May 30, 2024

sichanyoo May 30, 2024

jbelkins May 31, 2024

dayaffe left a comment

		@@ -0,0 +1,82 @@
		//
		// Copyright Amazon.com Inc. or its affiliates.

feat!: Modularize event streams & event streams auth #741

feat!: Modularize event streams & event streams auth #741

Conversation

jbelkins commented May 29, 2024 • edited Loading

Issue #

Description of changes

Scope

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sichanyoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dayaffe left a comment

Choose a reason for hiding this comment

jbelkins commented May 29, 2024 •

edited

Loading