Smithy codegen Client Refactor #384

AllanZhengYP · 2019-10-02T18:53:35Z

A demo of refactored service client.

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

'apply' was used to adjust middleware stack and resolved configuration. It is not relative as we are moving to conposition configuration, and middleware stack should not be altered by configurations. Address: aws#94

codecov-io · 2019-10-02T19:09:31Z

Codecov Report

Merging #384 into smithy-codegen will decrease coverage by 0.3%.
The diff coverage is 92.98%.

@@                Coverage Diff                 @@
##           smithy-codegen     #384      +/-   ##
==================================================
- Coverage           97.28%   96.97%   -0.31%     
==================================================
  Files                  69       69              
  Lines                1177     1156      -21     
  Branches              215      211       -4     
==================================================
- Hits                 1145     1121      -24     
- Misses                 32       35       +3

Impacted Files	Coverage Δ
packages/middleware-stack/src/index.ts	`100% <ø> (ø)`	⬆️
packages/retry-middleware/src/defaultStrategy.ts	`100% <100%> (ø)`
packages/retry-middleware/src/retryDecider.ts	`100% <100%> (ø)`	⬆️
...ackages/node-http-handler/src/node-http-handler.ts	`96.42% <100%> (ø)`	⬆️
packages/stream-collector-node/src/index.ts	`100% <100%> (ø)`	⬆️
packages/retry-middleware/src/configurations.ts	`100% <100%> (ø)`
packages/retry-middleware/src/retryMiddleware.ts	`92% <71.42%> (-8%)`	⬇️
packages/signing-middleware/src/middleware.ts	`85.71% <85.71%> (ø)`
packages/middleware-header-default/src/index.ts
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8f5a3a...4c526b0. Read the comment docs.

AllanZhengYP · 2019-10-02T23:26:21Z

One more thought on the config: Now I put all the config in the config-resolver package. I think I would make more sense if I put AuthConfig to signingMiddleware package, put retryConfig to retryMiddleware package, put protocolConfig into httpProtocol package. This make codebase more modularized and show the relationship between middleware and its config

* use config interface as middleware parameter * use smithy command as super class of all commands so we can support use() in commands

* Serializers/Deserializers should take utils functions from client config first, if not available then fallback to generated dependencies. This allows configuring the runtime dependencies manually from client config when runtime-specific bundlers' decision is not accountable

AllanZhengYP · 2019-10-04T17:51:08Z

I added a (de)serializer utils function bag to the interface. Check this commit: 72a02d9. I make the utils interface general on purpose because (De)Serializer interfaces are suppose to be share with non-AWS services, so no pre-assumption there.

AllanZhengYP · 2019-10-04T18:10:21Z

We need to add browser client smoke test scripts to this demo client also.

trivikr

My recommendations for changing to arrow functions are non-blocking, and can be taken up separately.
I thought of suggesting them, as we're rewriting CodeGen anyway

trivikr · 2019-10-04T23:27:21Z

clients/node/client-rds-data-node/RDSDataClient.ts

+    const intermediaConfig_0 = ProtocolConfig.resolve({
+      ...RDSRuntimeConfiguration,
+      ...configuration
+    });
+    super(intermediaConfig_0);
+    let intermediaConfig_1 = RegionConfiguration.resolve(intermediaConfig_0);
+    let intermediaConfig_2 = AwsAuthConfiguration.resolve(intermediaConfig_1);
+    let intermediaConfig_3 = EndpointsConfig.resolve(intermediaConfig_2);
+    let intermediaConfig_4 = RetryConfig.resolve(intermediaConfig_3);
+    this.config = intermediaConfig_4;


To avoid usage of temporary variables, can we do this instead?

const initialConfig = ProtocolConfig.resolve({ ...RDSRuntimeConfiguration, ...configuration }); super(initialConfig); this.config = RetryConfig.resolve( EndpointsConfig.resolve( AwsAuthConfiguration.resolve(RegionConfiguration.resolve(initialConfig)) ) );

This manner is stupidly easy for codegen. The advice looks normal but might be hard to codegen.

trivikr · 2019-10-04T23:32:34Z

packages/smithy-client/src/command.ts

+  HandlerOptions as InjectOptions
+} from "@aws-sdk/types";
+
+export class Command<InputType extends object, OutputType extends object> {


Should this be called SmithyClientCommand or SmithyCommand?

Suggested change

export class Command<InputType extends object, OutputType extends object> {

export class SmithyClientCommand<InputType extends object, OutputType extends object> {

Resolved by changing SmithyClient to Client

trivikr · 2019-10-04T23:36:55Z

clients/node/client-rds-data-node/commands/ExecuteStatementCommand.ts

@@ -63,7 +41,7 @@ export class ExecuteStatementCommand {
    };

    return stack.resolve(
-      (request: FinalizeHandlerArguments<any>) => {return httpHandler.handle(request.request as HttpRequest, options || {})},
+      (request: FinalizeHandlerArguments<any>) => { return httpHandler.handle(request.request as HttpRequest, options || {}) },


nit: arrow functions

Suggested change

(request: FinalizeHandlerArguments<any>) => { return httpHandler.handle(request.request as HttpRequest, options || {}) },

(request: FinalizeHandlerArguments<any>) => (httpHandler.handle(request.request as HttpRequest, options || {})),

trivikr · 2019-10-04T23:49:08Z

clients/node/client-rds-data-node/protocol/AwsRestJson1_1.ts

-  for (let ColumnMetadata of input) {
-    list.push(columnMetadataAwsRestJson1_1Deserialize(ColumnMetadata));
+  if (input !== undefined) {
+    for (let ColumnMetadata of input) {
+      list.push(columnMetadataAwsRestJson1_1Deserialize(ColumnMetadata));
+    }
  }
  return list;
 }


nit: if interested, this method can be written in a functional programming way to reduce bugs

const columnMetadataListAwsRestJson1_1Deserialize = ( input: any ): Array<ColumnMetadata> => input && input.map(columnMetadata => columnMetadataAwsRestJson1_1Deserialize(columnMetadata) );

Ditto for other methods.

fixed in bed5267

trivikr · 2019-10-04T23:51:41Z

clients/node/client-rds-data-node/protocol/AwsRestJson1_1.ts

+function deserializeMetadata(output: HttpResponse): ResponseMetadata {
+  return {
+    httpStatusCode: output.statusCode,
+    httpHeaders: output.headers,
+    requestId: output.headers["x-amzn-requestid"]
+  };
+}


nit: arrow functions?

const deserializeMetadata = (output: HttpResponse): ResponseMetadata => ({ httpStatusCode: output.statusCode, httpHeaders: output.headers, requestId: output.headers["x-amzn-requestid"] });

fixed in bed5267

trivikr · 2019-10-04T23:53:41Z

packages/config-resolver/src/components.ts

+  export function resolve<T>(input: T & Input): T & Resolved {
+    return {
+      ...input,
+      maxRetries: input.maxRetries === undefined ? 3 : input.maxRetries


Suggested change

maxRetries: input.maxRetries === undefined ? 3 : input.maxRetries

maxRetries: input.maxRetries || 3

maxRetries can be set to 0.

trivikr · 2019-10-04T23:56:50Z

packages/smithy-client/src/client.ts

+  ) {
+    const { middleware, step, priority, tags } = injectable;
+    this.middlewareStack.add(
+      // @ts-ignore


Can you add the TypeScript Error code here?
There's no option to ignore specific errors (microsoft/TypeScript#19139), but it's better to document it

Ditto for other places where // @ts-ignore is added

trivikr · 2019-10-05T00:14:41Z

packages/stream-collector-node/src/index.ts

+export const streamCollector: StreamCollector = function streamCollector(
+  stream: Readable
+): Promise<Uint8Array> {
  return new Promise((resolve, reject) => {
    const collector = new Collector();


nit: Convert to arrow function?

export const streamCollector: StreamCollector = ( stream: Readable ): Promise<Uint8Array> => new Promise((resolve, reject) => { /* Code inside promise */ });

* signing middleware * retry middleware; Also update retry config interface by introducing RetryStrategy class

…llanFly120/aws-sdk-js-v3 into smithy-codegen-client-refactor

lock · 2019-10-15T18:04:48Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

AllanZhengYP added 6 commits October 2, 2019 11:47

feat: remove 'apply' entry from configuration definition

67f0792

'apply' was used to adjust middleware stack and resolved configuration. It is not relative as we are moving to conposition configuration, and middleware stack should not be altered by configurations. Address: aws#94

feat: complete PoC composable configuration

044e94d

feat: change config resolver to multiple client config components

5378332

feat: rename stack finalize step to finalizeRequest

f084181

feat: add use() to smithy client to inject middleware

6203448

fix: rename resolvedruntime configuration

2c3dc2c

AllanZhengYP added the smithy-codegen Changes regarding supporting smithy model. Will be merged to smithy-codegen branch label Oct 2, 2019

AllanZhengYP and others added 5 commits October 2, 2019 16:34

fix: remove exported reviouslyResolved interface

57c2c29

fix: request path

5d966ec

feat: add metadatabearer to shapes

5cae1e2

feat: parse derializing utils as parameters

7fbf9d0

* use config interface as middleware parameter * use smithy command as super class of all commands so we can support use() in commands

Chase Coalwell added 2 commits October 4, 2019 11:17

feat: add metadata deserializer

cbe55c3

fix: await body parsing

423a7d1

trivikr reviewed Oct 5, 2019

View reviewed changes

AllanZhengYP added 8 commits October 4, 2019 17:37

docs: add documentation for config properties

64ed628

feat: add defaultUserAgen config

a3808f0

feat: move some config components to middleware folder

a86e44f

* signing middleware * retry middleware; Also update retry config interface by introducing RetryStrategy class

feat: add input type proxy for better intellisense

4a76cc3

docs: add config doc block for retry config

1f1cd82

feat: add a user agent middleware to support custom useragent

a3fdc01

fix: address style suggestions--FP

7ee70a7

Merge branch 'smithy-codegen-client-refactor' of https://github.com/A…

a7332b8

…llanFly120/aws-sdk-js-v3 into smithy-codegen-client-refactor

AllanZhengYP marked this pull request as ready for review October 7, 2019 16:54

fix: use arrow functions

72c5dd2

AllanZhengYP force-pushed the smithy-codegen-client-refactor branch from bed5267 to 72c5dd2 Compare October 7, 2019 17:28

AllanZhengYP requested a review from mtdowling October 7, 2019 18:18

Merge branch 'smithy-codegen' into smithy-codegen-client-refactor

4c526b0

srchase merged commit 033f953 into aws:smithy-codegen Oct 8, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 15, 2019

	export class Command<InputType extends object, OutputType extends object> {
	export class SmithyClientCommand<InputType extends object, OutputType extends object> {

	(request: FinalizeHandlerArguments<any>) => { return httpHandler.handle(request.request as HttpRequest, options \|\| {}) },
	(request: FinalizeHandlerArguments<any>) => (httpHandler.handle(request.request as HttpRequest, options \|\| {})),

	maxRetries: input.maxRetries === undefined ? 3 : input.maxRetries
	maxRetries: input.maxRetries \|\| 3

Smithy codegen Client Refactor #384

Smithy codegen Client Refactor #384

Uh oh!

Conversation

AllanZhengYP commented Oct 2, 2019

Uh oh!

codecov-io commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AllanZhengYP commented Oct 2, 2019

Uh oh!

AllanZhengYP commented Oct 4, 2019

Uh oh!

AllanZhengYP commented Oct 4, 2019

Uh oh!

trivikr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lock bot commented Oct 15, 2019

Uh oh!

Uh oh!

codecov-io commented Oct 2, 2019 •

edited

Loading