From 5f6789bfdc43db0454085935bc46df54c5a79f4b Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 14 Mar 2024 15:57:08 +0000 Subject: [PATCH 1/7] feat: allow commands to be constructed without arg if all arg fields optional --- .changeset/six-doors-speak.md | 6 ++++++ packages/smithy-client/src/command.spec.ts | 21 +++++++++++++++++++++ packages/smithy-client/src/command.ts | 8 ++++++-- packages/types/src/client.ts | 4 ++-- packages/types/src/util.ts | 8 ++++++++ 5 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 .changeset/six-doors-speak.md diff --git a/.changeset/six-doors-speak.md b/.changeset/six-doors-speak.md new file mode 100644 index 00000000000..1bcf208f688 --- /dev/null +++ b/.changeset/six-doors-speak.md @@ -0,0 +1,6 @@ +--- +"@smithy/smithy-client": patch +"@smithy/types": patch +--- + +allow command constructor argument to be omitted if no required members diff --git a/packages/smithy-client/src/command.spec.ts b/packages/smithy-client/src/command.spec.ts index 59d6773a877..251049046d5 100644 --- a/packages/smithy-client/src/command.spec.ts +++ b/packages/smithy-client/src/command.spec.ts @@ -1,6 +1,27 @@ import { Command } from "./command"; describe(Command.name, () => { + it("has optional argument if the input type has no required members", async () => { + type OptionalInput = { + key?: string; + optional?: string; + }; + + type RequiredInput = { + key: string | undefined; + optional?: string; + }; + + class WithRequiredInputCommand extends Command.classBuilder() + .build() {} + + class WithOptionalInputCommand extends Command.classBuilder() + .build() {} + + new WithRequiredInputCommand({ key: "1" }); + + new WithOptionalInputCommand(); // expect no type error. + }); it("implements a classBuilder", async () => { class MyCommand extends Command.classBuilder() .ep({ diff --git a/packages/smithy-client/src/command.ts b/packages/smithy-client/src/command.ts index 7f265f064ce..9d39aeba452 100644 --- a/packages/smithy-client/src/command.ts +++ b/packages/smithy-client/src/command.ts @@ -11,6 +11,7 @@ import type { Logger, MetadataBearer, MiddlewareStack as IMiddlewareStack, + OptionalParameter, Pluggable, RequestHandler, SerdeContext, @@ -217,7 +218,7 @@ class ClassBuilder< * @returns a Command class with the classBuilder properties. */ public build(): { - new (input: I): CommandImpl; + new (...[input]: OptionalParameter): CommandImpl; getEndpointParameterInstructions(): EndpointParameterInstructions; } { // eslint-disable-next-line @typescript-eslint/no-this-alias @@ -225,6 +226,8 @@ class ClassBuilder< let CommandRef: any; return (CommandRef = class extends Command { + public readonly input: I; + /** * @public */ @@ -235,8 +238,9 @@ class ClassBuilder< /** * @public */ - public constructor(readonly input: I) { + public constructor(...[input]: OptionalParameter) { super(); + this.input = input ?? (({} as unknown) as I); closure._init(this); } diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index e8d7fcbad01..576b2220c8d 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -1,7 +1,7 @@ import { Command } from "./command"; import { MiddlewareStack } from "./middleware"; import { MetadataBearer } from "./response"; -import { Exact } from "./util"; +import { OptionalParameter } from "./util"; /** * @public @@ -9,7 +9,7 @@ import { Exact } from "./util"; * A type which checks if the client configuration is optional. * If all entries of the client configuration are optional, it allows client creation without passing any config. */ -export type CheckOptionalClientConfig = Exact, T> extends true ? [] | [T] : [T]; +export type CheckOptionalClientConfig = OptionalParameter; /** * @public diff --git a/packages/types/src/util.ts b/packages/types/src/util.ts index bebb7775669..2518482f843 100644 --- a/packages/types/src/util.ts +++ b/packages/types/src/util.ts @@ -181,3 +181,11 @@ export interface RetryStrategy { args: FinalizeHandlerArguments ) => Promise>; } + +/** + * @public + * + * Indicates the parameter may be omitted if the parameter object T + * is equivalent to a Partial, i.e. all properties optional. + */ +export type OptionalParameter = Exact, T> extends true ? [] | [T] : [T] \ No newline at end of file From f5b89a6ca786e9bc0eed1b109c6ee1a7f07bc814 Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 14 Mar 2024 17:55:53 +0000 Subject: [PATCH 2/7] chore: formatting --- packages/smithy-client/src/command.spec.ts | 6 ++---- packages/types/src/util.ts | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/smithy-client/src/command.spec.ts b/packages/smithy-client/src/command.spec.ts index 251049046d5..651a608229a 100644 --- a/packages/smithy-client/src/command.spec.ts +++ b/packages/smithy-client/src/command.spec.ts @@ -12,11 +12,9 @@ describe(Command.name, () => { optional?: string; }; - class WithRequiredInputCommand extends Command.classBuilder() - .build() {} + class WithRequiredInputCommand extends Command.classBuilder().build() {} - class WithOptionalInputCommand extends Command.classBuilder() - .build() {} + class WithOptionalInputCommand extends Command.classBuilder().build() {} new WithRequiredInputCommand({ key: "1" }); diff --git a/packages/types/src/util.ts b/packages/types/src/util.ts index 2518482f843..101df49de03 100644 --- a/packages/types/src/util.ts +++ b/packages/types/src/util.ts @@ -188,4 +188,4 @@ export interface RetryStrategy { * Indicates the parameter may be omitted if the parameter object T * is equivalent to a Partial, i.e. all properties optional. */ -export type OptionalParameter = Exact, T> extends true ? [] | [T] : [T] \ No newline at end of file +export type OptionalParameter = Exact, T> extends true ? [] | [T] : [T]; From db8bfe6d54d4fbe2606ff29bed45c0ecf4cfcbb1 Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 14 Mar 2024 18:17:14 +0000 Subject: [PATCH 3/7] test: add type test for OptionalParameter transform --- packages/types/src/util.spec.ts | 40 +++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 packages/types/src/util.spec.ts diff --git a/packages/types/src/util.spec.ts b/packages/types/src/util.spec.ts new file mode 100644 index 00000000000..1b98b80a7ea --- /dev/null +++ b/packages/types/src/util.spec.ts @@ -0,0 +1,40 @@ +import type { Exact, OptionalParameter } from "./util"; + +type Assignable = [RHS] extends [LHS] ? true : false; + +type OptionalInput = { + key?: string; + optional?: string; +}; + +type RequiredInput = { + key: string | undefined; + optional?: string; +}; + +{ + // optional parameter transform of an optional input is not equivalent to exactly 1 parameter. + type A = [...OptionalParameter]; + type B = [OptionalInput]; + type C = [OptionalInput] | []; + + const assert1: Exact = false as const; + const assert2: Exact = true as const; + + const assert3: Assignable = true as const; + const assert4: A = []; + + const assert5: Assignable = true as const; + const assert6: A = [{ key: "" }]; +} + +{ + // optional parameter transform of a required input is equivalent to exactly 1 parameter. + type A = [...OptionalParameter]; + type B = [RequiredInput]; + + const assert1: Exact = true as const; + const assert2: Assignable = false as const; + const assert3: Assignable = true as const; + const assert4: A = [{ key: "" }]; +} From 0905982f5996b95a606488cf82e2260cd62d358f Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 14 Mar 2024 19:58:53 +0000 Subject: [PATCH 4/7] feat: java codegen for optional command arg --- .../model/weather/main.smithy | 26 +++++++------- .../ServiceAggregatedClientGenerator.java | 34 ++++++++++++------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/smithy-typescript-codegen-test/model/weather/main.smithy b/smithy-typescript-codegen-test/model/weather/main.smithy index 32a4d662333..e8abc79201c 100644 --- a/smithy-typescript-codegen-test/model/weather/main.smithy +++ b/smithy-typescript-codegen-test/model/weather/main.smithy @@ -167,17 +167,17 @@ apply GetCity @httpResponseTests([ code: 200 body: """ { - "name": "Seattle", - "coordinates": { - "latitude": 12.34, - "longitude": -56.78 - }, - "city": { - "cityId": "123", - "name": "Seattle", - "number": "One", - "case": "Upper" - } + "name": "Seattle", + "coordinates": { + "latitude": 12.34, + "longitude": -56.78 + }, + "city": { + "cityId": "123", + "name": "Seattle", + "number": "One", + "case": "Upper" + } }""" bodyMediaType: "application/json" params: { @@ -259,8 +259,8 @@ apply NoSuchResource @httpResponseTests([ code: 404 body: """ { - "resourceType": "City", - "message": "Your custom message" + "resourceType": "City", + "message": "Your custom message" }""" bodyMediaType: "application/json" params: { resourceType: "City", message: "Your custom message" } diff --git a/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java b/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java index ff01e97116f..3441ece6fc3 100644 --- a/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java +++ b/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java @@ -94,19 +94,27 @@ public void run() { writer.writeDocs( "@see {@link " + operationSymbol.getName() + "}" ); - writer.write("$L(\n" - + " args: $T,\n" - + " options?: $T,\n" - + "): Promise<$T>;", methodName, input, applicationProtocol.getOptionsType(), output); - writer.write("$L(\n" - + " args: $T,\n" - + " cb: (err: any, data?: $T) => void\n" - + "): void;", methodName, input, output); - writer.write("$L(\n" - + " args: $T,\n" - + " options: $T,\n" - + " cb: (err: any, data?: $T) => void\n" - + "): void;", methodName, input, applicationProtocol.getOptionsType(), output); + writer.addImport("OptionalParameter", null, TypeScriptDependency.SMITHY_TYPES); + writer.write(""" + $L( + ...[args]: OptionalParameter<$T>, + ): Promise<$T>;""", methodName, input, output); + writer.write(""" + $L( + args: $T, + options?: $T, + ): Promise<$T>;""", methodName, input, applicationProtocol.getOptionsType(), output); + writer.write(""" + $L( + args: $T, + cb: (err: any, data?: $T) => void + ): void;""", methodName, input, output); + writer.write(""" + $L( + args: $T, + options: $T, + cb: (err: any, data?: $T) => void + ): void;""", methodName, input, applicationProtocol.getOptionsType(), output); writer.write(""); } }); From ba79f060132fe126c4c78984614616da580bb164 Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 14 Mar 2024 20:00:03 +0000 Subject: [PATCH 5/7] feat: java codegen for optional command arg --- packages/smithy-client/src/command.ts | 1 + .../codegen/ServiceAggregatedClientGenerator.java | 14 +++++++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/smithy-client/src/command.ts b/packages/smithy-client/src/command.ts index 9d39aeba452..93c9a3b1acc 100644 --- a/packages/smithy-client/src/command.ts +++ b/packages/smithy-client/src/command.ts @@ -218,6 +218,7 @@ class ClassBuilder< * @returns a Command class with the classBuilder properties. */ public build(): { + new (input: I): CommandImpl; new (...[input]: OptionalParameter): CommandImpl; getEndpointParameterInstructions(): EndpointParameterInstructions; } { diff --git a/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java b/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java index 3441ece6fc3..e6dea65f116 100644 --- a/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java +++ b/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java @@ -22,6 +22,7 @@ import software.amazon.smithy.codegen.core.SymbolProvider; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.knowledge.TopDownIndex; +import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.OperationShape; import software.amazon.smithy.model.shapes.ServiceShape; import software.amazon.smithy.utils.SmithyInternalApi; @@ -94,11 +95,14 @@ public void run() { writer.writeDocs( "@see {@link " + operationSymbol.getName() + "}" ); - writer.addImport("OptionalParameter", null, TypeScriptDependency.SMITHY_TYPES); - writer.write(""" - $L( - ...[args]: OptionalParameter<$T>, - ): Promise<$T>;""", methodName, input, output); + boolean inputOptional = model.getShape(operation.getInputShape()).map( + shape -> shape.getAllMembers().values().stream().noneMatch(MemberShape::isRequired) + ).orElse(true); + if (inputOptional) { + writer.addImport("OptionalParameter", null, TypeScriptDependency.SMITHY_TYPES); + writer.write(""" + $L(): Promise<$T>;""", methodName, output); + } writer.write(""" $L( args: $T, From aaf0c59615efa64001d949624f8e2d40613ed070 Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 14 Mar 2024 20:42:09 +0000 Subject: [PATCH 6/7] chore: undo formatting --- .../model/weather/main.smithy | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/smithy-typescript-codegen-test/model/weather/main.smithy b/smithy-typescript-codegen-test/model/weather/main.smithy index e8abc79201c..32a4d662333 100644 --- a/smithy-typescript-codegen-test/model/weather/main.smithy +++ b/smithy-typescript-codegen-test/model/weather/main.smithy @@ -167,17 +167,17 @@ apply GetCity @httpResponseTests([ code: 200 body: """ { - "name": "Seattle", - "coordinates": { - "latitude": 12.34, - "longitude": -56.78 - }, - "city": { - "cityId": "123", - "name": "Seattle", - "number": "One", - "case": "Upper" - } + "name": "Seattle", + "coordinates": { + "latitude": 12.34, + "longitude": -56.78 + }, + "city": { + "cityId": "123", + "name": "Seattle", + "number": "One", + "case": "Upper" + } }""" bodyMediaType: "application/json" params: { @@ -259,8 +259,8 @@ apply NoSuchResource @httpResponseTests([ code: 404 body: """ { - "resourceType": "City", - "message": "Your custom message" + "resourceType": "City", + "message": "Your custom message" }""" bodyMediaType: "application/json" params: { resourceType: "City", message: "Your custom message" } From bfa254c57dfae7454490777c2d0a4d38b8f034a3 Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 14 Mar 2024 20:47:03 +0000 Subject: [PATCH 7/7] chore: use java text block --- .../ServiceAggregatedClientGenerator.java | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java b/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java index e6dea65f116..cde823dc291 100644 --- a/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java +++ b/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/ServiceAggregatedClientGenerator.java @@ -99,26 +99,24 @@ public void run() { shape -> shape.getAllMembers().values().stream().noneMatch(MemberShape::isRequired) ).orElse(true); if (inputOptional) { - writer.addImport("OptionalParameter", null, TypeScriptDependency.SMITHY_TYPES); - writer.write(""" - $L(): Promise<$T>;""", methodName, output); + writer.write("$L(): Promise<$T>;", methodName, output); } writer.write(""" - $L( - args: $T, - options?: $T, - ): Promise<$T>;""", methodName, input, applicationProtocol.getOptionsType(), output); - writer.write(""" - $L( - args: $T, - cb: (err: any, data?: $T) => void - ): void;""", methodName, input, output); - writer.write(""" - $L( - args: $T, - options: $T, - cb: (err: any, data?: $T) => void - ): void;""", methodName, input, applicationProtocol.getOptionsType(), output); + $1L( + args: $2T, + options?: $3T, + ): Promise<$4T>; + $1L( + args: $2T, + cb: (err: any, data?: $4T) => void + ): void; + $1L( + args: $2T, + options: $3T, + cb: (err: any, data?: $4T) => void + ): void;""", + methodName, input, applicationProtocol.getOptionsType(), output + ); writer.write(""); } });