Skip to content

Commit 37e8b0d

Browse files
kuhekuhe
authored
fix(core/protocols): rest-json default request body conditions (#7501)
1 parent e501b6c commit 37e8b0d

File tree

12 files changed

+168
-56
lines changed

12 files changed

+168
-56
lines changed

‎codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AddProtocolConfig.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,11 @@
3737
@SmithyInternalApi
3838
public final class AddProtocolConfig implements TypeScriptIntegration {
3939

40-
public AddProtocolConfig() {
40+
static {
41+
init();
42+
}
43+
44+
static void init() {
4145
List<ShapeId> allowed = List.of(
4246
AwsJson1_0Trait.ID,
4347
AwsJson1_1Trait.ID,

‎codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AddS3Config.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@
6161
import software.amazon.smithy.typescript.codegen.auth.http.integration.AddHttpSigningPlugin;
6262
import software.amazon.smithy.typescript.codegen.integration.RuntimeClientPlugin;
6363
import software.amazon.smithy.typescript.codegen.integration.TypeScriptIntegration;
64-
import software.amazon.smithy.typescript.codegen.schema.SchemaGenerationAllowlist;
6564
import software.amazon.smithy.utils.ListUtils;
6665
import software.amazon.smithy.utils.MapUtils;
6766
import software.amazon.smithy.utils.SetUtils;

‎codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AwsProtocolUtils.java‎

Lines changed: 29 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,9 @@ static boolean includeUnsignedPayloadSigV4Header(OperationShape operation) {
9696
* @param visitor A ShapeVisitor that generates a serde function for shapes.
9797
*/
9898
static void generateDocumentBodyShapeSerde(
99-
GenerationContext context,
100-
Set<Shape> shapes,
101-
ShapeVisitor<Void> visitor
99+
GenerationContext context,
100+
Set<Shape> shapes,
101+
ShapeVisitor<Void> visitor
102102
) {
103103
// Walk all the shapes within those in the document and generate for them as well.
104104
Walker shapeWalker = new Walker(NeighborProviderIndex.of(context.getModel()).getProvider());
@@ -123,7 +123,7 @@ static void generateJsonParseBodyWithQueryHeader(GenerationContext context) {
123123
TypeScriptWriter writer = context.getWriter();
124124
writer.addImport("HeaderBag", "__HeaderBag", TypeScriptDependency.SMITHY_TYPES);
125125
writer.write(IoUtils.readUtf8Resource(
126-
AwsProtocolUtils.class, "populate-body-with-query-compatibility-code-stub.ts"));
126+
AwsProtocolUtils.class, "populate-body-with-query-compatibility-code-stub.ts"));
127127
}
128128

129129
/**
@@ -176,9 +176,9 @@ static void generateBuildFormUrlencodedString(GenerationContext context) {
176176
writer.addImport("extendedEncodeURIComponent", "__extendedEncodeURIComponent",
177177
TypeScriptDependency.AWS_SMITHY_CLIENT);
178178
writer.openBlock("const buildFormUrlencodedString = (formEntries: Record<string, string>): "
179-
+ "string => Object.entries(formEntries).map(", ").join(\"&\");",
180-
() -> writer.write("([key, value]) => __extendedEncodeURIComponent(key) + '=' + "
181-
+ "__extendedEncodeURIComponent(value)"));
179+
+ "string => Object.entries(formEntries).map(", ").join(\"&\");",
180+
() -> writer.write("([key, value]) => __extendedEncodeURIComponent(key) + '=' + "
181+
+ "__extendedEncodeURIComponent(value)"));
182182
writer.write("");
183183
}
184184

@@ -251,7 +251,7 @@ static void writeIdempotencyAutofill(GenerationContext context, MemberShape memb
251251
TypeScriptWriter writer = context.getWriter();
252252
writer.addImport("v4", "generateIdempotencyToken", TypeScriptDependency.SMITHY_UUID);
253253
writer.openBlock("if ($L === undefined) {", "}", inputLocation, () ->
254-
writer.write("$L = generateIdempotencyToken();", inputLocation));
254+
writer.write("$L = generateIdempotencyToken();", inputLocation));
255255
}
256256
}
257257

@@ -266,10 +266,10 @@ static void writeIdempotencyAutofill(GenerationContext context, MemberShape memb
266266
* @return A string representing the proper value provider for this timestamp.
267267
*/
268268
static String getInputTimestampValueProvider(
269-
GenerationContext context,
270-
MemberShape memberShape,
271-
Format defaultFormat,
272-
String inputLocation
269+
GenerationContext context,
270+
MemberShape memberShape,
271+
Format defaultFormat,
272+
String inputLocation
273273
) {
274274
HttpBindingIndex httpIndex = HttpBindingIndex.of(context.getModel());
275275
TimestampFormatTrait.Format format = httpIndex.determineTimestampFormat(memberShape, DOCUMENT, defaultFormat);
@@ -278,9 +278,9 @@ static String getInputTimestampValueProvider(
278278

279279
static void generateProtocolTests(ProtocolGenerator generator, GenerationContext context) {
280280
new HttpProtocolTestGenerator(context,
281-
generator,
282-
AwsProtocolUtils::filterProtocolTests,
283-
AwsProtocolUtils::filterMalformedRequestTests).run();
281+
generator,
282+
AwsProtocolUtils::filterProtocolTests,
283+
AwsProtocolUtils::filterMalformedRequestTests).run();
284284
}
285285

286286
/**
@@ -307,28 +307,16 @@ static Map<String, TreeSet<String>> getErrorAliases(GenerationContext context,
307307
}
308308

309309
private static boolean filterProtocolTests(
310-
ServiceShape service,
311-
OperationShape operation,
312-
HttpMessageTestCase testCase,
313-
TypeScriptSettings settings
310+
ServiceShape service,
311+
OperationShape operation,
312+
HttpMessageTestCase testCase,
313+
TypeScriptSettings settings
314314
) {
315-
// TODO: Remove when upstream tests update to serialize empty headers.
316315
if (testCase.getId().contains("NullAndEmptyHeaders")) {
317-
return true;
318-
}
319-
320-
// TODO This test has an arbitrary root XML name NestedXmlMapWithXmlNameRequest
321-
// TODO which doesn't match the input structure. We will need to update
322-
// TODO the test comparator stub to ignore this if that is indeed intended.
323-
if (testCase.getId().contains("NestedXmlMapWithXmlNameSerializes")) {
324-
return true;
325-
}
326-
327-
// TODO: remove when Glacier AccountID hyphen customization is implemented: SMITHY-2614
328-
if (testCase.getId().equals("GlacierAccountId")) {
329-
return true;
316+
return settings.generateServerSdk();
330317
}
331318

319+
// JSv3 does not populate default values on the client side
332320
if (testCase.getTags().contains("defaults")) {
333321
return true;
334322
}
@@ -343,7 +331,7 @@ private static boolean filterProtocolTests(
343331
return true;
344332
}
345333

346-
// TODO: https://github.com/aws/aws-sdk-js-v3/issues/7169
334+
// todo(schema): can be enabled when dropping the legacy codegen private client.
347335
if (testCase.getId().equals("RestJsonHttpPayloadWithStructureAndEmptyResponseBody")) {
348336
return true;
349337
}
@@ -352,10 +340,10 @@ private static boolean filterProtocolTests(
352340
}
353341

354342
private static boolean filterMalformedRequestTests(
355-
ServiceShape service,
356-
OperationShape operation,
357-
HttpMalformedRequestTestCase testCase,
358-
TypeScriptSettings settings
343+
ServiceShape service,
344+
OperationShape operation,
345+
HttpMalformedRequestTestCase testCase,
346+
TypeScriptSettings settings
359347
) {
360348
// Handling overflow/underflow of longs in JS is extraordinarily tricky.
361349
// Numbers are actually all 62-bit floats, and so any integral number is
@@ -374,9 +362,8 @@ private static boolean filterMalformedRequestTests(
374362
return true;
375363
}
376364

377-
// TODO: fix in https://github.com/aws/aws-sdk-js-v3/issues/5545
378365
if (testCase.getId().equals("RestJsonMalformedUnionUnknownMember")) {
379-
return true;
366+
return settings.generateServerSdk();
380367
}
381368

382369
//TODO: reenable when the SSDK uses RE2 and not built-in regex for pattern constraints
@@ -405,15 +392,13 @@ private static boolean filterMalformedRequestTests(
405392
return true;
406393
}
407394

408-
// ToDo: https://github.com/aws/aws-sdk-js-v3/issues/6246
409395
if (testCase.getId().equals("RestJsonStringPayloadNoContentType")
410396
|| testCase.getId().equals("RestJsonWithBodyExpectsApplicationJsonContentTypeNoHeaders")) {
411-
return true;
397+
return settings.generateServerSdk();
412398
}
413399

414-
// ToDo: https://github.com/aws/aws-sdk-js-v3/issues/6907
415400
if (testCase.getId().equals("RestJsonWithoutBodyEmptyInputExpectsEmptyContentType")) {
416-
return true;
401+
return settings.generateServerSdk();
417402
}
418403

419404
return false;

‎packages/core/src/submodules/protocols/json/AwsRestJsonProtocol.spec.ts‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,5 +312,103 @@ describe(AwsRestJsonProtocol.name, () => {
312312
"query-default-date": "1970-01-01T00:00:00Z",
313313
});
314314
});
315+
316+
it("inserts a default body only when the payload is a structure shape", async () => {
317+
const operation = {
318+
namespace: "ns",
319+
name: "Operation",
320+
traits: 0,
321+
input: [
322+
3,
323+
"ns",
324+
"Input",
325+
0,
326+
["payload", "name"],
327+
[
328+
[21 satisfies BlobSchema, { httpPayload: 1 }],
329+
[0 satisfies StringSchema, { httpHeader: "name" }],
330+
],
331+
] satisfies StaticStructureSchema,
332+
output: "unit" as const,
333+
};
334+
335+
for (const body of ["", false, undefined, 0, new Uint8Array()]) {
336+
const request = await protocol.serializeRequest(
337+
operation,
338+
{
339+
name: "hi",
340+
payload: body,
341+
},
342+
context
343+
);
344+
if (!body) {
345+
expect(request.body).toBeFalsy();
346+
}
347+
expect(request.body).toEqual(body);
348+
}
349+
350+
for (const body of [undefined, null]) {
351+
const request = await protocol.serializeRequest(
352+
{
353+
...operation,
354+
input: [
355+
3,
356+
"ns",
357+
"Input",
358+
0,
359+
["payload", "name"],
360+
[
361+
[3, "ns", "PayloadStruct", { httpPayload: 1 }, ["a", "b"], [0, 0]] satisfies StaticStructureSchema,
362+
[0 satisfies StringSchema, { httpHeader: "name" }],
363+
],
364+
] satisfies StaticStructureSchema,
365+
},
366+
{
367+
payload: body,
368+
},
369+
context
370+
);
371+
expect(request.body).toEqual(`{}`);
372+
}
373+
});
374+
375+
it("should fill in the payload member of a response if the wire response was empty", async () => {
376+
const operation = {
377+
namespace: "ns",
378+
name: "Operation",
379+
traits: 0,
380+
input: "unit" as const,
381+
output: [
382+
3,
383+
"ns",
384+
"Output",
385+
0,
386+
["name", "payload"],
387+
[
388+
[0, { httpHeader: "name" }],
389+
[21, { httpPayload: 1 }],
390+
],
391+
] satisfies StaticStructureSchema,
392+
};
393+
394+
const output = await protocol.deserializeResponse(
395+
operation,
396+
context,
397+
new HttpResponse({
398+
statusCode: 200,
399+
headers: {},
400+
})
401+
);
402+
403+
expect(output).toEqual({
404+
$metadata: {
405+
cfId: undefined,
406+
extendedRequestId: undefined,
407+
httpStatusCode: 200,
408+
requestId: undefined,
409+
},
410+
payload: null,
411+
});
412+
});
315413
});
316414
});

‎packages/core/src/submodules/protocols/json/AwsRestJsonProtocol.ts‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import type {
99
HandlerExecutionContext,
1010
HttpRequest,
1111
HttpResponse,
12+
MetadataBearer,
1213
OperationSchema,
1314
ResponseMetadata,
1415
SerdeFunctions,
@@ -60,6 +61,9 @@ export class AwsRestJsonProtocol extends HttpBindingProtocol {
6061
super.setSerdeContext(serdeContext);
6162
}
6263

64+
/**
65+
* @override
66+
*/
6367
public async serializeRequest<Input extends object>(
6468
operationSchema: OperationSchema,
6569
input: Input,
@@ -75,7 +79,8 @@ export class AwsRestJsonProtocol extends HttpBindingProtocol {
7579
}
7680
}
7781

78-
if (request.headers["content-type"] && !request.body) {
82+
if (request.body == null && request.headers["content-type"] === this.getDefaultContentType()) {
83+
// if content type is blob or string shape, we don't set a default body.
7984
request.body = "{}";
8085
}
8186

@@ -84,6 +89,27 @@ export class AwsRestJsonProtocol extends HttpBindingProtocol {
8489
return request;
8590
}
8691

92+
/**
93+
* @override
94+
*/
95+
public async deserializeResponse<Output extends MetadataBearer>(
96+
operationSchema: OperationSchema,
97+
context: HandlerExecutionContext & SerdeFunctions,
98+
response: HttpResponse
99+
): Promise<Output> {
100+
const output: any & MetadataBearer = await super.deserializeResponse(operationSchema, context, response);
101+
const outputSchema = NormalizedSchema.of(operationSchema.output);
102+
for (const [name, member] of outputSchema.structIterator()) {
103+
if (member.getMemberTraits().httpPayload && !(name in output)) {
104+
output[name] = null;
105+
}
106+
}
107+
return output;
108+
}
109+
110+
/**
111+
* @override
112+
*/
87113
protected async handleError(
88114
operationSchema: OperationSchema,
89115
context: HandlerExecutionContext & SerdeFunctions,

‎packages/middleware-sdk-glacier/src/account-id-default.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export function accountIdDefaultMiddleware(): InitializeMiddleware<any, any> {
1111
return <Output extends MetadataBearer>(next: InitializeHandler<any, Output>): InitializeHandler<any, Output> =>
1212
async (args: InitializeHandlerArguments<any>): Promise<InitializeHandlerOutput<Output>> => {
1313
const { input } = args;
14-
if (input.accountId === undefined) {
14+
if (!input.accountId) {
1515
input.accountId = "-";
1616
}
1717
return next({ ...args, input });

‎private/aws-protocoltests-restjson-glacier/test/functional/restjson1.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ it("GlacierChecksums:Request", async () => {
282282
* hyphen (-) to indicate the current account. This should be default
283283
* behavior if the customer provides a null or empty string.
284284
*/
285-
it.skip("GlacierAccountId:Request", async () => {
285+
it("GlacierAccountId:Request", async () => {
286286
const client = new GlacierClient({
287287
...clientParams,
288288
requestHandler: new RequestSerializationTestHandler(),

‎private/aws-protocoltests-restjson-schema-glacier/test/functional/restjson1.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ it("GlacierChecksums:Request", async () => {
282282
* hyphen (-) to indicate the current account. This should be default
283283
* behavior if the customer provides a null or empty string.
284284
*/
285-
it.skip("GlacierAccountId:Request", async () => {
285+
it("GlacierAccountId:Request", async () => {
286286
const client = new GlacierClient({
287287
...clientParams,
288288
requestHandler: new RequestSerializationTestHandler(),

‎private/aws-protocoltests-restjson-schema/test/functional/restjson1.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7030,7 +7030,7 @@ it("RestJsonNoInputAndOutputNoPayload:Response", async () => {
70307030
/**
70317031
* Do not send null values, but do send empty strings and empty lists over the wire in headers
70327032
*/
7033-
it.skip("RestJsonNullAndEmptyHeaders:Request", async () => {
7033+
it("RestJsonNullAndEmptyHeaders:Request", async () => {
70347034
const client = new RestJsonProtocolClient({
70357035
...clientParams,
70367036
requestHandler: new RequestSerializationTestHandler(),

‎private/aws-protocoltests-restjson/test/functional/restjson1.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7030,7 +7030,7 @@ it("RestJsonNoInputAndOutputNoPayload:Response", async () => {
70307030
/**
70317031
* Do not send null values, but do send empty strings and empty lists over the wire in headers
70327032
*/
7033-
it.skip("RestJsonNullAndEmptyHeaders:Request", async () => {
7033+
it("RestJsonNullAndEmptyHeaders:Request", async () => {
70347034
const client = new RestJsonProtocolClient({
70357035
...clientParams,
70367036
requestHandler: new RequestSerializationTestHandler(),

0 commit comments

Comments
 (0)