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

feat!: Closure-based reader-writer serde for JSON, FormURL #1439

Merged
merged 39 commits into from
May 10, 2024
Merged

Conversation

jbelkins
Copy link
Contributor

@jbelkins jbelkins commented Apr 15, 2024

Issue #

#1443

Description of changes

Adapts the SDK to the changes made in smithy-lang/smithy-swift#696.

Specific changes:

  • Protocol generators for AWS protocols and their associated types are updated/simplified to match smithy-swift changes.
  • Base error types are provided for all AWS protocols.
  • Added a Smithy service with a RPC protocol (AWSJSON 1.0) and event streaming operations to the protocol-test-codegen-local target, so that initial event code can be generated & compiled along with protocol tests.

New/existing dependencies impact assessment, if applicable

No new dependencies were added to this change.

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

requestID2: String? = nil,
typeName: String?
) async throws -> Error {
public static func makeError<Base: BaseError>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to take a BaseError as its param


extension ClientRuntime.BaseError {

var requestID2: String? { nil }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extends BaseError with a default implementation for requestID2. XML/S3 will provide a concrete implementation for this field.

import class ClientRuntime.HttpResponse
import class SmithyJSON.Reader

public struct AWSJSONError: BaseError {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "base error" for use with AWS JSON 1.0 / 1.1. Provides values for the common error fields.

import class ClientRuntime.HttpResponse
import class SmithyXML.Reader

public struct AWSQueryError: BaseError {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "base error" for use with AWS Query. Provides values for the common error fields.

import class ClientRuntime.HttpResponse
import class SmithyXML.Reader

public struct EC2QueryError: BaseError {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "base error" for use with EC2Query. Provides values for the common error fields.

import SmithyReadWrite
import SmithyXML

public struct Ec2Error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted in favor of the EC2 base error.

import SmithyReadWrite
import SmithyXML

public struct Ec2Errors {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted in favor of the EC2 base error.

// SPDX-License-Identifier: Apache-2.0
//

public struct Ec2NarrowedError<T>: Decodable where T: Decodable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted in favor of the EC2 base error.

// SPDX-License-Identifier: Apache-2.0
//

public struct Ec2NarrowedResponse<T>: Decodable where T: Decodable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted in favor of the EC2 base error.

import class SmithyXML.Reader
import var ClientRuntime.responseDocumentBinding

public struct Ec2QueryError {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted in favor of the EC2 base error.

import SmithyXML
import ClientRuntime

public struct Ec2Response {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted in favor of the EC2 base error.

XCTFail("Request should have thrown a service error with code NotFound, instead the request succeeded")
} catch let error as AWSServiceError & ClientRuntime.HTTPError where error.errorCode == "NotFound" {
XCTFail("Request should have thrown a NotFound modeled service error, instead the request succeeded")
} catch let error as AWSS3.NotFound {
Copy link
Contributor Author

@jbelkins jbelkins Apr 22, 2024

Choose a reason for hiding this comment

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

Changed this error customization test so that it expects the genuine S3 NotFound modeled error instead of just matching the error code.

import ClientRuntime
import class SmithyXML.Reader

extension RestXMLError {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to be the base RestXML error. It's replaced by the new type that inherits from "BaseError".

The 404 customization in this type is now performed in the protocol generator's customization.

@@ -217,21 +219,22 @@ func addProtocolTests() {
.init(name: "S3TestSDK", sourcePath: "\(baseDir)/s3"),
.init(name: "rest_json_extras", sourcePath: "\(baseDirLocal)/rest_json_extras"),
.init(name: "AwsQueryExtras", sourcePath: "\(baseDirLocal)/AwsQueryExtras"),
.init(name: "EventStream", sourcePath: "\(baseDirLocal)/EventStream", buildOnly: true),
.init(name: "RPCEventStream", sourcePath: "\(baseDirLocal)/RPCEventStream", buildOnly: true),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the ability to add these two Smithy service definitions to local protocol tests.

These two Smithy services don't have any protocol tests, but at least this will build them & verify that the code compiles.

Had to add a little logic here to allow adding protocol test targets to the manifest that don't have actual test bundles.

@@ -191,11 +191,13 @@ func addProtocolTests() {
let name: String
let sourcePath: String
let testPath: String?
let buildOnly: Bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just generated from the Package.Base.swift above.

),
CodegenTest(
"aws.protocoltests.eventstream#RPCTestService",
"RPCEventStream"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code generate the non-RPC EventStream & RPC EventStream models with protocol-test-codegen-local.

@@ -0,0 +1,83 @@
namespace aws.protocoltests.eventstream
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is the same as eventstream.smithy except that the protocol is AWS JSON (i.e. a RPC protocol) instead of RestJSON.

@@ -1,4 +1,4 @@
namespace aws.protocoltests.restjson
namespace aws.protocoltests.eventstream
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the non-RPC EventStream model.

It is pre-existing, and was added when event streaming was first added to the SDK.

@@ -70,7 +70,19 @@ class MessageMarshallableGenerator(
eventPayloadBinding != null -> renderSerializeEventPayload(ctx, eventPayloadBinding, writer)
unbound.isNotEmpty() -> {
writer.addStringHeader(":content-type", payloadContentType)
renderPayloadSerialization(ctx, writer, variant)
Copy link
Contributor Author

@jbelkins jbelkins Apr 24, 2024

Choose a reason for hiding this comment

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

Added support here for unbound event members, which (according to the Smithy spec) are supposed to be serialized by

combining all members that are not marked with the eventHeader or eventPayload trait into a protocol-specific document

(https://smithy.io/2.0/spec/streaming.html#event-message-serialization)

So, I do that here.

@@ -175,14 +187,16 @@ class MessageMarshallableGenerator(
write("headers.append(.init(name: \$S, value: .string(\$S)))", name, value)
}

private fun renderPayloadSerialization(ctx: ProtocolGenerator.GenerationContext, writer: SwiftWriter, shape: Shape) {
private fun renderPayloadSerialization(ctx: ProtocolGenerator.GenerationContext, writer: SwiftWriter, memberShape: MemberShape) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to render serialization for the member shape so that other member info is available, not just the target shape.

@@ -170,7 +170,7 @@ class MessageUnmarshallableGenerator(val ctx: ProtocolGenerator.GenerationContex
// and then assign each deserialized payload member to the current builder instance
unbound.forEach {
renderReadToValue(writer, it)
writer.write("event.\$L = value", memberName)
writer.write("event.\$L = value", ctx.symbolProvider.toMemberName(it))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change needed to reference the correct member name.

@@ -29,11 +29,11 @@ extension EventStreamTestClientTypes.TestStream {
case .messagewithstruct(let value):
headers.append(.init(name: ":event-type", value: .string("MessageWithStruct")))
headers.append(.init(name: ":content-type", value: .string("application/json")))
payload = try SmithyJSON.Writer.write(value, rootNodeInfo: "TestStruct", with: EventStreamTestClientTypes.TestStruct.write(value:to:))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next few files are codegen test updates.

@@ -41,6 +41,7 @@ rm codegen/protocol-test-codegen/build/smithyprojections/protocol-test-codegen/s
rm codegen/protocol-test-codegen-local/build/smithyprojections/protocol-test-codegen-local/rest_json_extras/swift-codegen/Package.swift
rm codegen/protocol-test-codegen-local/build/smithyprojections/protocol-test-codegen-local/AwsQueryExtras/swift-codegen/Package.swift
rm codegen/protocol-test-codegen-local/build/smithyprojections/protocol-test-codegen-local/EventStream/swift-codegen/Package.swift
rm codegen/protocol-test-codegen-local/build/smithyprojections/protocol-test-codegen-local/RPCEventStream/swift-codegen/Package.swift
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated protocol test script for new target

@sichanyoo sichanyoo self-requested a review April 29, 2024 21:50
@jbelkins jbelkins merged commit 61031ff into main May 10, 2024
29 checks passed
@jbelkins jbelkins deleted the jbe/readwrite branch May 10, 2024 02:01
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