Orchestrator `ResponseDeserializer` codegen and auth #2494

jdisanti · 2023-03-24T00:34:27Z

Motivation and Context

This PR refactors the orchestrator's error handling and revises the code generator to implement the new ResponseDeserializer trait behind the enableNewSmithyRuntime codegen flag.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Co-authored-by: John DiSanti <[email protected]>

update: retry handling in orchestrator

…alled on

github-actions · 2023-03-24T02:05:38Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

github-actions · 2023-03-25T01:09:04Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

Velfi · 2023-03-27T15:29:04Z

rust-runtime/aws-smithy-runtime-api/src/type_erasure.rs

+use std::ops::{Deref, DerefMut};
+
+#[derive(Debug)]
+pub struct TypedBox<T> {


I would love to see docs for this. Why does this exist? What does it allow us to do that we couldn't otherwise?

Velfi · 2023-03-27T15:34:04Z

aws/rust-runtime/aws-http/src/request_id.rs

@@ -58,6 +58,12 @@ impl<B> RequestId for http::Response<B> {
    }
 }

+impl RequestId for HeaderMap {
+    fn request_id(&self) -> Option<&str> {
+        extract_request_id(self)


When should functions that return an option have a try prefix? I'd ask the same question for traits too. Do we expect to only ever have a fallible version of this trait? I can see that being the case but I wanted to confirm.

This one will definitely always be fallible.

Velfi · 2023-03-27T15:34:51Z

aws/rust-runtime/aws-inlineable/src/s3_request_id.rs

+    fn extended_request_id(&self) -> Option<&str> {
+        extract_extended_request_id(self)
+    }


What is an "extended" request ID? I'm not familiar with that phrasing.

Oh, this must be referring to that S3-specific way of sending it, right?

Yeah, this is the S3 extended request ID.

Velfi · 2023-03-27T15:38:11Z

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/HttpRequestChecksumDecorator.kt

+    // TODO(enableNewSmithyRuntime): Implement checksumming via interceptor and delete this decorator
+    private fun applies(codegenContext: ClientCodegenContext): Boolean =
+        !codegenContext.settings.codegenConfig.enableNewSmithyRuntime
+
    override fun operationCustomizations(
        codegenContext: ClientCodegenContext,
        operation: OperationShape,
        baseCustomizations: List<OperationCustomization>,
-    ): List<OperationCustomization> {
-        return baseCustomizations + HttpRequestChecksumCustomization(codegenContext, operation)
+    ): List<OperationCustomization> = baseCustomizations.letIf(applies(codegenContext)) {
+        it + HttpRequestChecksumCustomization(codegenContext, operation)
    }
 }


This adds code to make_operation but I'd assumed we'd no longer be generating/calling that. Are we using it somehow? If not, why generate the code for it?

I'm currently continuing to generate make_operation for now to keep things compiling. I haven't made enough codegen progress to stop generating some pieces.

Velfi · 2023-03-27T16:02:31Z

aws/sra-test/build.gradle.kts

Thanks for setting this up.

Velfi · 2023-03-27T16:17:29Z

...in/software/amazon/smithy/rust/codegen/client/smithy/protocols/HttpBoundProtocolGenerator.kt

+    private val orchestratorCodegenScope by lazy {
+        arrayOf(
+            "Error" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::interceptors::context::Error"),
+            "HttpResponse" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::orchestrator::HttpResponse"),
+            "Instrument" to CargoDependency.Tracing.toType().resolve("Instrument"),
+            "Output" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::interceptors::context::Output"),
+            "OutputOrError" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::interceptors::context::OutputOrError"),
+            "ResponseDeserializer" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::orchestrator::ResponseDeserializer"),
+            "SdkBody" to RuntimeType.sdkBody(runtimeConfig),
+            "SdkError" to RuntimeType.sdkError(runtimeConfig),
+            "TypedBox" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("type_erasure::TypedBox"),
+            "debug_span" to RuntimeType.Tracing.resolve("debug_span"),
+            "type_erase_result" to typeEraseResult(),
+        )
+    }


This is a teeny bit more readable with some aliasing.

Suggested change

private val orchestratorCodegenScope by lazy {

arrayOf(

"Error" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::interceptors::context::Error"),

"HttpResponse" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::orchestrator::HttpResponse"),

"Instrument" to CargoDependency.Tracing.toType().resolve("Instrument"),

"Output" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::interceptors::context::Output"),

"OutputOrError" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::interceptors::context::OutputOrError"),

"ResponseDeserializer" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::orchestrator::ResponseDeserializer"),

"SdkBody" to RuntimeType.sdkBody(runtimeConfig),

"SdkError" to RuntimeType.sdkError(runtimeConfig),

"TypedBox" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("type_erasure::TypedBox"),

"debug_span" to RuntimeType.Tracing.resolve("debug_span"),

"type_erase_result" to typeEraseResult(),

)

}

val InterceptorContext = CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::interceptors::context");

val Orchestrator = CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("client::orchestrator");

private val orchestratorCodegenScope by lazy {

arrayOf(

"Error" to InterceptorContext.resolve("Error"),

"HttpResponse" to Orchestrator.resolve("HttpResponse"),

"Instrument" to CargoDependency.Tracing.toType().resolve("Instrument"),

"Output" to InterceptorContext.resolve("Output"),

"OutputOrError" to InterceptorContext.resolve("OutputOrError"),

"ResponseDeserializer" to Orchestrator.resolve("ResponseDeserializer"),

"SdkBody" to RuntimeType.sdkBody(runtimeConfig),

"SdkError" to RuntimeType.sdkError(runtimeConfig),

"TypedBox" to CargoDependency.smithyRuntimeApi(runtimeConfig).toType().resolve("type_erasure::TypedBox"),

"debug_span" to RuntimeType.Tracing.resolve("debug_span"),

"type_erase_result" to typeEraseResult(),

)

}

These aliases could even be added to RuntimeType.kt

I was a little hesitant to add any conveniences to RuntimeType to avoid accidental inclusion in the old codegen paths.

Velfi · 2023-03-27T16:24:37Z

...in/software/amazon/smithy/rust/codegen/client/smithy/protocols/HttpBoundProtocolGenerator.kt

+    private fun typeEraseResult(): RuntimeType = ProtocolFunctions.crossOperationFn("type_erase_result") { fnName ->
+        rustTemplate(
+            """
+            pub(crate) fn $fnName<O, E>(result: Result<O, E>) -> Result<#{Output}, #{Error}>
+            where
+                O: Send + Sync + 'static,
+                E: Send + Sync + 'static,
+            {
+                result.map(|output| #{TypedBox}::new(output).erase())
+                    .map_err(|error| #{TypedBox}::new(error).erase())
+            }
+            """,
+            *orchestratorCodegenScope,
+        )
+    }


I think adding docs or some sort of breadcrumb for people that don't understand why we do type erasure would be helpful

Velfi · 2023-03-27T16:42:13Z

...in/software/amazon/smithy/rust/codegen/client/smithy/protocols/HttpBoundProtocolGenerator.kt

@@ -256,6 +403,43 @@ class HttpBoundProtocolTraitImplGenerator(
        val outputSymbol = symbolProvider.toSymbol(outputShape)
        val errorSymbol = symbolProvider.symbolForOperationError(operationShape)
        return protocolFunctions.deserializeFn(operationShape, fnNameSuffix = "http_response") { fnName ->
+            Attribute.AllowClippyUnnecessaryWraps.render(this)
+            rustBlockTemplate(
+                "pub fn $fnName(response: &mut #{http}::Response<#{SdkBody}>) -> std::result::Result<#{O}, #{E}>",


We should probably add #[doc(hidden)] for this

They're in a pub(crate) module.

Velfi · 2023-03-27T16:48:20Z

rust-runtime/aws-smithy-http/src/result.rs

I think we should document what the meaning/usage of all the sources are and why you'd choose one over the other. That, or we should create a TODO for that.

Ended up removing the modeled/unmodeled source changes to ServiceError since they're no longer needed.

Velfi · 2023-03-27T16:51:13Z

rust-runtime/aws-smithy-runtime-api/src/client/orchestrator.rs

This file is long enough that we should consider splitting it.

Velfi · 2023-03-27T16:55:13Z

rust-runtime/aws-smithy-runtime-api/src/lib.rs

+pub mod client;
+
 /// A typemap for storing configuration.
 pub mod config_bag;
-/// Smithy interceptors for smithy clients.
-///
-/// Interceptors are lifecycle hooks that can read/modify requests and responses.
-pub mod interceptors;
-/// Smithy code related to retry handling and token bucket.
-///
-/// This code defines when and how failed requests should be retried. It also defines the behavior
-/// used to limit the rate that requests are sent.
-pub mod retries;
-/// Runtime plugin type definitions.
-pub mod runtime_plugin;
+
+pub mod type_erasure;


Please add docs for these modules here, or in the module files

rust-runtime/aws-smithy-http/src/result.rs

github-actions · 2023-03-27T22:30:51Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

github-actions · 2023-03-27T23:12:04Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

* Convert interceptor fns to macro invocations Co-authored-by: John DiSanti <[email protected]> * Add unmodeled error to ServiceError and bring in EventLog Co-authored-by: John DiSanti <[email protected]> * Simplify and test `EventLog` * Attempt to integrate the event log with the orchestrator * fix: error type in integration test update: retry handling in orchestrator * update: set the runtime plugins to do nothing instead of panic when called on * Introduce a construct for type erasure * Eliminate several generics and add phased error handling * Code generate the `ResponseDeserializer` trait impls * CI fixes * Reorganize the new runtime crates * Create the `Identity` type * Reorganize orchestrator traits and accessors * Set up code generated test environment for the new runtime * Add initial auth orchestration implementation * Fix clippy lint * Incorporate feedback * Fix external types lint --------- Co-authored-by: Zelda Hessler <[email protected]>

Velfi and others added 9 commits March 20, 2023 14:22

Convert interceptor fns to macro invocations

34987d8

Co-authored-by: John DiSanti <[email protected]>

Add unmodeled error to ServiceError and bring in EventLog

9340e1d

Co-authored-by: John DiSanti <[email protected]>

Simplify and test EventLog

106fa6e

Attempt to integrate the event log with the orchestrator

8f08b28

fix: error type in integration test

36bbff3

update: retry handling in orchestrator

update: set the runtime plugins to do nothing instead of panic when c…

bd2cfa1

…alled on

Introduce a construct for type erasure

bdb80b8

Eliminate several generics and add phased error handling

ab354e6

Code generate the ResponseDeserializer trait impls

da58050

jdisanti mentioned this pull request Mar 24, 2023

Fix request ID logging for streaming operations #2495

Merged

2 tasks

jdisanti added 2 commits March 23, 2023 18:05

Merge remote-tracking branch 'origin/main' into pair-programming-session

bfb1bb0

CI fixes

d126797

smithy-lang deleted a comment from github-actions bot Mar 24, 2023

jdisanti marked this pull request as ready for review March 24, 2023 01:44

jdisanti requested review from a team as code owners March 24, 2023 01:44

jdisanti requested review from pose and david-perez March 24, 2023 01:44

jdisanti added the needs-sdk-review label Mar 24, 2023

jdisanti added 2 commits March 24, 2023 14:59

Reorganize the new runtime crates

9a8245a

Create the Identity type

484565e

ysaito1001 approved these changes Mar 24, 2023

View reviewed changes

jdisanti added 4 commits March 24, 2023 16:01

Reorganize orchestrator traits and accessors

692b64a

Set up code generated test environment for the new runtime

07307f5

Add initial auth orchestration implementation

08f8323

Merge remote-tracking branch 'origin/main' into pair-programming-session

fcc5c28

jdisanti changed the title ~~Generate ResponseDeserializer trait impls for the orchestrator~~ Orchestrator ResponseDeserializer codegen and auth Mar 25, 2023

Fix clippy lint

000c786

Velfi reviewed Mar 27, 2023

View reviewed changes

jdisanti commented Mar 27, 2023

View reviewed changes

rust-runtime/aws-smithy-http/src/result.rs Outdated Show resolved Hide resolved

jdisanti added 2 commits March 27, 2023 14:59

Incorporate feedback

3d64e43

Merge remote-tracking branch 'origin/main' into pair-programming-session

1c6879b

jdisanti enabled auto-merge March 27, 2023 22:06

Fix external types lint

6a6eb51

jdisanti added this pull request to the merge queue Mar 27, 2023

Merged via the queue into main with commit 6b21e46 Mar 27, 2023

jdisanti deleted the pair-programming-session branch March 27, 2023 23:39

Velfi removed the needs-sdk-review label Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Orchestrator `ResponseDeserializer` codegen and auth #2494

Orchestrator `ResponseDeserializer` codegen and auth #2494

jdisanti commented Mar 24, 2023

github-actions bot commented Mar 24, 2023

github-actions bot commented Mar 25, 2023

Velfi Mar 27, 2023 •

edited

Loading

Velfi Mar 27, 2023

jdisanti Mar 27, 2023

Velfi Mar 27, 2023

Velfi Mar 27, 2023

jdisanti Mar 27, 2023

Velfi Mar 27, 2023

jdisanti Mar 27, 2023

Velfi Mar 27, 2023

Velfi Mar 27, 2023

Velfi Mar 27, 2023

jdisanti Mar 27, 2023

Velfi Mar 27, 2023

Velfi Mar 27, 2023

jdisanti Mar 27, 2023

Velfi Mar 27, 2023

jdisanti Mar 27, 2023

Velfi Mar 27, 2023

Velfi Mar 27, 2023

github-actions bot commented Mar 27, 2023

github-actions bot commented Mar 27, 2023

Orchestrator ResponseDeserializer codegen and auth #2494

Orchestrator ResponseDeserializer codegen and auth #2494

Conversation

jdisanti commented Mar 24, 2023

Motivation and Context

github-actions bot commented Mar 24, 2023

github-actions bot commented Mar 25, 2023

Velfi Mar 27, 2023 • edited Loading

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

github-actions bot commented Mar 27, 2023

github-actions bot commented Mar 27, 2023

Orchestrator `ResponseDeserializer` codegen and auth #2494

Orchestrator `ResponseDeserializer` codegen and auth #2494

Velfi Mar 27, 2023 •

edited

Loading