feat: Add WaiterTypedError type, conform operation errors to it #491

jbelkins · 2022-12-08T00:00:19Z

Description of changes

This PR merges to the waiters feature branch.

Adds a WaiterTypedError Swift protocol so that an error can provide its error type. This will be used in the implementation of the errorType acceptor for Smithy waiters, which will match the error based on an expected error type string that will match the errorCode property on a WaiterTypedError.

Conforms SdkError to WaiterTypedError
Conforms operation errors to WaiterTypedError
Conforms unknown HTTP errors to WaiterTypedError
Adds a test for correct codegen of WaiterTypedError compliance of an OperationError

As written, this PR generates WaiterTypedError compliance for every operation error, not just the ones with errorType waiters. When this work is integrated with Smithy acceptor work, WaiterTypedError compliance will be generated only for operations that have a waiter with an WaiterTypedError acceptor.

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.

jbelkins · 2022-12-08T00:03:54Z

...otlin/software/amazon/smithy/swift/codegen/integration/httpResponse/HttpResponseGenerator.kt

@@ -36,6 +36,7 @@ class HttpResponseGenerator(
        httpOperations.forEach {
            httpResponseBindingErrorGenerator.render(ctx, it)
            HttpResponseBindingErrorNarrowGenerator(ctx, it, unknownServiceErrorSymbol).render()
+            CodedErrorGenerator(ctx, it, unknownServiceErrorSymbol).render()


Rendering coded error compliance here does it for every operation. When this work is integrated with Smithy acceptors, coded error compliance will only be generated for operations that have a waiter with an errorType acceptor.

(Note that this branch merges to feat/waiters, not main)

Does that mean that the implementation of CodedErrorGenerator here will be removed? Assuming it will be implemented somewhere else where it is restricted to only operations that have a waiter with an errorType acceptor?

Yes, I'll remove it here, and I will conditionally perform this render when a waiter requires it.

I placed this render here so that I could render coded errors into the SDK for testing.

epau · 2022-12-08T14:55:30Z

Packages/ClientRuntime/Sources/Networking/CodedError.swift

+
+import Foundation
+
+public protocol CodedError: Error {


Can we add a doc comment for this type?

epau · 2022-12-08T14:57:41Z

Packages/ClientRuntime/Sources/Networking/CodedError.swift

+    /// The `String` error code that identifies the general type of this HTTP service error, or `nil` if the
+    /// error has no known code.
+    ///
+    /// The source of this code is the HTTP response to an API call, but what exact header, body field, etc.
+    /// it comes from is specific to the response encoding used.


We should refrain from documenting protocol methods with details from specific conforming types.

Suggested change

/// The `String` error code that identifies the general type of this HTTP service error, or `nil` if the

/// error has no known code.

///

/// The source of this code is the HTTP response to an API call, but what exact header, body field, etc.

/// it comes from is specific to the response encoding used.

/// The error code.

epau · 2022-12-08T14:58:45Z

Packages/ClientRuntime/Sources/Networking/Http/UnknownHttpServiceError.swift

@@ -5,6 +5,8 @@

 /// General Service Error structure used when exact error could not be deduced from the `HttpResponse`
 public struct UnknownHttpServiceError: HttpServiceError, Swift.Equatable {
+    public var _errorCode: String?


Unrelated to this PR:
I wonder why we prefix all the properties here with an underscore? 🤔
Especially since there properties are public

I don't know the purpose either; in the past I have used underscore on private properties to help keep them separate from a related public property.

When we overhaul our error system...

epau · 2022-12-08T14:59:23Z

Packages/ClientRuntime/Sources/Networking/Http/UnknownHttpServiceError.swift

+
+extension UnknownHttpServiceError: CodedError {
+
+    public var errorCode: String? { _errorCode }


Can we add docs as to what an error code is (or should be) for an UnknownHttpServiceError?

epau · 2022-12-08T14:59:39Z

Packages/ClientRuntime/Sources/Networking/Http/UnknownHttpServiceError.swift

@@ -19,9 +21,15 @@ public struct UnknownHttpServiceError: HttpServiceError, Swift.Equatable {
 }

 extension UnknownHttpServiceError {
-    public init(httpResponse: HttpResponse, message: String? = nil) {
+    public init(httpResponse: HttpResponse, message: String? = nil, errorCode: String? = nil) {


Mind adding docs for this initializer while you're here?

epau · 2022-12-08T14:59:57Z

Packages/ClientRuntime/Sources/Networking/SdkError.swift

+
+extension SdkError: CodedError {
+
+    public var errorCode: String? {


Can we add docs as to what an error code is (or should be) for a SdkError?

epau · 2022-12-08T15:03:15Z

.../kotlin/software/amazon/smithy/swift/codegen/integration/httpResponse/CodedErrorGenerator.kt

+            writer.declareSection(CodedErrorGeneratorSectionId, context) {
+                writer.openBlock("extension \$L: CodedError {", "}", operationErrorName) {
+                    writer.write("")
+                    writer.openBlock("public var errorCode: String? {", "}") {


Can we add docs for errorCode here?

epau · 2022-12-08T15:04:13Z

.../kotlin/software/amazon/smithy/swift/codegen/integration/httpResponse/CodedErrorGenerator.kt

+                            var errorShapeName = resolveErrorShapeName(errorShape)
+                            var errorShapeType = ctx.symbolProvider.toSymbol(errorShape).name
+                            var errorShapeEnumCase = errorShapeType.decapitalize()
+                            writer.write("case .\$L: return \$S", errorShapeEnumCase, errorShapeName)


Why do we return the error type as a string here? Should we return nil instead?

Returning the error type as a string here prevents me from having to extend every error on the operation with its own CodedError compliance... the GetWidgetOutputError knows the right error code for every possible error it can return.

Compare this to the output error initializer which switches over the expected values of errorCode to determine what exact error to create for the operation & this method makes more sense. Here's an (abridged) initializer for an operation from an AWS service:

extension ModifyDBInstanceOutputError { public init(errorType: Swift.String?, httpResponse: ClientRuntime.HttpResponse, decoder: ClientRuntime.ResponseDecoder? = nil, message: Swift.String? = nil, requestID: Swift.String? = nil) throws { switch errorType { case "AuthorizationNotFound" : self = .authorizationNotFoundFault(try AuthorizationNotFoundFault(httpResponse: httpResponse, decoder: decoder, message: message, requestID: requestID)) case "CertificateNotFound" : self = .certificateNotFoundFault(try CertificateNotFoundFault(httpResponse: httpResponse, decoder: decoder, message: message, requestID: requestID)) default : self = .unknown(UnknownAWSHttpServiceError(httpResponse: httpResponse, message: message, requestID: requestID)) } } }

AuthorizationNotFoundFault and CertificateNotFoundFault don't need to be CodedErrors because ModifyDBInstanceOutputError returns the right error code for them.

The extension I created is basically this switch statement in reverse... convert the enum case to a string instead of convert the string to an enum case.

epau

Looking really good! Just a few comments and questions

Packages/ClientRuntime/Sources/Networking/Http/UnknownHttpServiceError.swift

…iceError.swift Co-authored-by: Ed Paulosky <eeppaauu@amazon.com>

…o jbe/coded_error

epau

Changes look great!

* feat: Add waiter client runtime components (#450) * feat: Waiter extension & methods (#478) * feat: Add WaiterTypedError type, conform operation errors to it (#491) * feat: Code-generate Acceptors for waiters (#483) * fix: Use correct var name in AND expression (#493) * fix: Correct Swift compile errors for comparison expressions (#494)

jbelkins added 2 commits December 7, 2022 17:53

Add CodedError Swift type, conform all operation errors to it

bac3a7f

Remove duplicate doc comment

dbf525b

jbelkins commented Dec 8, 2022

View reviewed changes

Add a test for CodedError conformance

7762ace

jbelkins marked this pull request as ready for review December 8, 2022 01:31

jbelkins requested a review from epau December 8, 2022 01:31

epau reviewed Dec 8, 2022

View reviewed changes

Add comments to CodedError implementations

0351c6c

jbelkins requested a review from epau December 8, 2022 15:57

epau reviewed Dec 8, 2022

View reviewed changes

Packages/ClientRuntime/Sources/Networking/Http/UnknownHttpServiceError.swift Outdated Show resolved Hide resolved

jbelkins and others added 4 commits December 8, 2022 10:08

Update Packages/ClientRuntime/Sources/Networking/Http/UnknownHttpServ…

ab3f004

…iceError.swift Co-authored-by: Ed Paulosky <eeppaauu@amazon.com>

Fix test

d2386ae

Merge branch 'jbe/coded_error' of github.com:awslabs/smithy-swift int…

8d68f64

…o jbe/coded_error

Update CodedError to WaiterTypedError

579ee70

jbelkins changed the title ~~feat: Add CodedError type, conform operation errors to it~~ feat: Add WaiterTypedError type, conform operation errors to it Dec 8, 2022

epau approved these changes Dec 8, 2022

View reviewed changes

jbelkins merged commit e2b95fa into feat/waiters Dec 8, 2022

jbelkins deleted the jbe/coded_error branch December 8, 2022 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add WaiterTypedError type, conform operation errors to it #491

feat: Add WaiterTypedError type, conform operation errors to it #491

jbelkins commented Dec 8, 2022 •

edited

Loading

jbelkins Dec 8, 2022

jbelkins Dec 8, 2022

epau Dec 8, 2022

jbelkins Dec 8, 2022

epau Dec 8, 2022

jbelkins Dec 8, 2022

epau Dec 8, 2022

jbelkins Dec 8, 2022

epau Dec 8, 2022

jbelkins Dec 8, 2022

epau Dec 8, 2022

jbelkins Dec 8, 2022

epau Dec 8, 2022

jbelkins Dec 8, 2022

epau Dec 8, 2022

jbelkins Dec 8, 2022

epau Dec 8, 2022

jbelkins Dec 8, 2022

epau Dec 8, 2022

jbelkins Dec 8, 2022

jbelkins Dec 8, 2022

epau left a comment

epau left a comment


		extension UnknownHttpServiceError: CodedError {

		public var errorCode: String? { _errorCode }


		extension SdkError: CodedError {

		public var errorCode: String? {

feat: Add WaiterTypedError type, conform operation errors to it #491

feat: Add WaiterTypedError type, conform operation errors to it #491

Conversation

jbelkins commented Dec 8, 2022 • edited Loading

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

epau left a comment

Choose a reason for hiding this comment

epau left a comment

Choose a reason for hiding this comment

jbelkins commented Dec 8, 2022 •

edited

Loading