Skip to content

Commit dd53b24

Browse files
mtdowlingmtdowling
authored
Run client protocol tests against the dynamic client too (#1233)
Client protocol tests previously exercised only the codegen path, so the document-backed DynamicClient serialization/deserialization went untested and several bugs went unnoticed. Each @HttpClientRequestTests and @HttpClientResponseTests case now runs twice — once per TestMode (codegen and dynamic). They use suffixed display names: `[codegen]` and `[dynamic]`. Harness: - Build a dynamic ApiOperation per operation (DynamicTestModels), mirroring DynamicClient, and run every request/response/error case through both models. - Isolate setup failures to a single attributable test (FailedGenerationTestCase) instead of aborting the whole method. - Bind the mock transport at execution time, not during stream generation, so the two invocations per case don't clobber each other's captured request. - Support per-mode suppression via a "[dynamic]"/"[codegen]" suffix on skipTests, plus @ProtocolTest(modes=...) to narrow a whole suite. Dynamic-path fixes surfaced by the new coverage: - Document.equals: compare DOCUMENT-typed values by content instead of always returning false, so document-typed members round-trip. - SchemaConverter: honor OriginalShapeIdTrait (with the Unit exception) so renamed input/output shapes serialize under their original wire name (e.g. the restXml root element XmlBlobsRequest), matching codegen. - SchemaGuidedDocumentBuilder: preserve null entries in @Sparse maps/lists and drop them otherwise, instead of failing to read a null as a typed value. - Comparison: compare Documents with NUMBER_PROMOTION so NaN/Infinity floats are tolerant, matching the typed comparators.
1 parent 351f916 commit dd53b24

22 files changed

Lines changed: 607 additions & 124 deletions

File tree

‎aws/client/aws-client-awsjson/src/it/java/software/amazon/smithy/java/client/aws/jsonprotocols/AwsJson1ProtocolTests.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ public class AwsJson1ProtocolTests {
2828

2929
// Like above, but in smithy-java we populate the defaults but don't change the nullability.
3030
"AwsJson10ClientIgnoresNonTopLevelDefaultsOnMembersWithClientOptional",
31+
32+
// Skipped only for the [dynamic] path; codegen still runs. The document-backed builder does not
33+
// populate modeled (nested) default values during error correction
34+
// (SchemaGuidedDocumentBuilder.errorCorrection is a no-op).
35+
"AwsJson10ClientPopulatesNestedDefaultValuesWhenMissing [dynamic]",
3136
},
3237
skipOperations = "aws.protocoltests.json10#OperationWithRequiredMembersWithDefaults")
3338
public void requestTest(DataStream expected, DataStream actual) {
@@ -48,6 +53,13 @@ public void requestTest(DataStream expected, DataStream actual) {
4853
skipTests = {
4954
"AwsJson10ClientPopulatesDefaultsValuesWhenMissingInResponse",
5055
"AwsJson10ClientIgnoresDefaultValuesIfMemberValuesArePresentInResponse",
56+
57+
// Skipped only for the [dynamic] (document-backed) path; codegen still runs. These rely on the
58+
// dynamic builder populating modeled defaults during error correction, which it does not yet do
59+
// (SchemaGuidedDocumentBuilder.errorCorrection is a no-op).
60+
"AwsJson10ClientPopulatesNestedDefaultsWhenMissingInResponseBody [dynamic]",
61+
"AwsJson10ClientPopulatesNestedDefaultValuesWhenMissing [dynamic]",
62+
"AwsJson10ClientErrorCorrectsWhenServerFailsToSerializeRequiredValues [dynamic]",
5163
},
5264
skipOperations = "aws.protocoltests.json10#OperationWithRequiredMembersWithDefaults")
5365
public void responseTest(Runnable test) {

‎aws/client/aws-client-awsquery/src/it/java/software/amazon/smithy/java/aws/client/awsquery/AwsQueryProtocolTests.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ public void requestTest(DataStream expected, DataStream actual) {
5050
@ProtocolTestFilter(
5151
skipTests = {
5252
"AwsQueryClientPopulatesDefaultsValuesWhenMissingInResponse",
53+
// Skipped only for the [dynamic] path: the document path doesn't tolerate the awsQuery result
54+
// wrapper element for an empty output (it sees ResponseMetadata instead). Codegen still runs.
55+
"QueryNoInputAndNoOutputWithResponseMetadata [dynamic]",
5356
})
5457
public void responseTest(Runnable test) {
5558
test.run();

‎aws/client/aws-client-restjson/src/it/java/software/amazon/smithy/java/client/aws/restjson/RestJson1ProtocolTests.java‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,22 @@
6060
"DuplexClientErrorOutput",
6161
// Client doesn't validate missing @required initial response members
6262
"MissingRequiredInitialResponseOutput",
63-
"DuplexMissingRequiredInitialResponseOutput"
63+
"DuplexMissingRequiredInitialResponseOutput",
64+
65+
// The following are skipped only for the [dynamic] (DynamicClient/document-backed) path; the codegen
66+
// versions still run. Each is a pre-existing dynamic-client limitation, not a regression in the
67+
// request/response serialization that dual-mode coverage was added to guard.
68+
// Streaming blobs: the document path resolves the @streaming @httpPayload member to a null/non-stream
69+
// value (the @streaming trait is on the target, not the member), and DataStreamDocument doesn't
70+
// support asBlob() for response comparison.
71+
"RestJsonStreamingTraitsWithBlob [dynamic]",
72+
"RestJsonStreamingTraitsWithMediaTypeWithBlob [dynamic]",
73+
"RestJsonStreamingTraitsWithNoBlobBody [dynamic]",
74+
"RestJsonStreamingTraitsRequireLengthWithNoBlobBody [dynamic]",
75+
// Nested default values are not populated by the document path when missing from the wire
76+
// (SchemaGuidedDocumentBuilder.errorCorrection is a no-op).
77+
"RestJsonClientPopulatesNestedDefaultsWhenMissingInResponseBody [dynamic]",
78+
"RestJsonClientPopulatesNestedDefaultValuesWhenMissing [dynamic]"
6479
})
6580
public class RestJson1ProtocolTests {
6681
private static final String EMPTY_BODY = "";

‎client/client-rpcv2-cbor/src/it/java/software/amazon/smithy/java/client/rpcv2/RpcV2CborProtocolTests.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ public class RpcV2CborProtocolTests {
2727
"RpcV2CborClientUsesExplicitlyProvidedMemberValues",
2828
"RpcV2CborClientIgnoresNonTopLevelDefaultsOnMembersWithClientOptional",
2929
"RpcV2CborClientUsesExplicitlyProvidedMemberValuesOverDefaults",
30+
// Skipped only for the [dynamic] path: an empty-input operation produces no CBOR body through the
31+
// document path, so there is nothing for the comparator to read. Codegen still runs.
32+
"no_input [dynamic]",
3033
})
3134
public void requestTest(DataStream expected, DataStream actual) {
3235
CborComparator.assertEquals(expected.asByteBuffer(), actual.asByteBuffer());

‎core/src/main/java/software/amazon/smithy/java/core/serde/document/Document.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -836,7 +836,16 @@ static boolean equals(Object left, Object right, int options) {
836836
}
837837
yield true;
838838
}
839-
default -> false; // unexpected type (DOCUMENT, MEMBER, OPERATION, SERVICE).
839+
case DOCUMENT -> {
840+
// An untyped document carries no schema-driven type of its own; compare the underlying values.
841+
// This also lets a typed-but-document-targeted value (e.g. a struct member whose shape is a
842+
// document) compare equal to an equivalent untyped document.
843+
if (r.type() != ShapeType.DOCUMENT) {
844+
yield false;
845+
}
846+
yield Objects.equals(l.asObject(), r.asObject());
847+
}
848+
default -> false; // unexpected type (MEMBER, OPERATION, SERVICE).
840849
};
841850
}
842851
return false;

‎core/src/test/java/software/amazon/smithy/java/core/serde/document/DocumentTest.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,40 @@ public static List<Arguments> inEqualDocumentsTestProvider() {
335335
Document.of(Map.of("a", Document.of("b")))));
336336
}
337337

338+
@Test
339+
public void documentTypedValuesCompareByContent() {
340+
// A document whose own type() is DOCUMENT (an untyped wrapper) must compare by its underlying contents,
341+
// not be treated as never-equal. This is what happens for a struct member modeled as a `document`.
342+
var a = new DocumentTypedWrapper(Document.of(Map.of("foo", Document.of("bar"))));
343+
var b = new DocumentTypedWrapper(Document.of(Map.of("foo", Document.of("bar"))));
344+
var c = new DocumentTypedWrapper(Document.of(Map.of("foo", Document.of("baz"))));
345+
346+
assertThat(a.type(), is(ShapeType.DOCUMENT));
347+
assertThat(Document.equals(a, b), is(true));
348+
assertThat(Document.equals(a, c), is(false));
349+
// A document-typed value is not equal to a non-document of the same content.
350+
assertThat(Document.equals(a, Document.of(Map.of("foo", Document.of("bar")))), is(false));
351+
}
352+
353+
// A document that reports its type as DOCUMENT but delegates content access, mimicking an untyped/document-target
354+
// value such as a struct member modeled as `document`.
355+
record DocumentTypedWrapper(Document delegate) implements Document {
356+
@Override
357+
public ShapeType type() {
358+
return ShapeType.DOCUMENT;
359+
}
360+
361+
@Override
362+
public Object asObject() {
363+
return delegate.asObject();
364+
}
365+
366+
@Override
367+
public void serializeContents(ShapeSerializer serializer) {
368+
delegate.serializeContents(serializer);
369+
}
370+
}
371+
338372
static final class DifferentDocument implements Document {
339373

340374
private final Document delegate;

‎dynamic-schemas/src/main/java/software/amazon/smithy/java/dynamicschemas/SchemaConverter.java‎

Lines changed: 41 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import software.amazon.smithy.model.shapes.ShapeId;
2323
import software.amazon.smithy.model.shapes.ShapeType;
2424
import software.amazon.smithy.model.traits.Trait;
25+
import software.amazon.smithy.model.traits.UnitTypeTrait;
26+
import software.amazon.smithy.model.traits.synthetic.OriginalShapeIdTrait;
2527

2628
/**
2729
* Converts {@link Shape}s to {@link Schema}s.
@@ -98,30 +100,31 @@ private Schema createSchema(Shape shape, Set<Shape> building) {
98100
}
99101

100102
private Schema createNonRecursiveSchema(Shape shape) {
103+
var id = schemaId(shape);
101104
return switch (shape.getType()) {
102-
case BLOB -> Schema.createBlob(shape.getId(), convertTraits(shape));
103-
case BOOLEAN -> Schema.createBoolean(shape.getId(), convertTraits(shape));
104-
case STRING -> Schema.createString(shape.getId(), convertTraits(shape));
105-
case TIMESTAMP -> Schema.createTimestamp(shape.getId(), convertTraits(shape));
106-
case BYTE -> Schema.createByte(shape.getId(), convertTraits(shape));
107-
case SHORT -> Schema.createShort(shape.getId(), convertTraits(shape));
108-
case INTEGER -> Schema.createInteger(shape.getId(), convertTraits(shape));
109-
case LONG -> Schema.createLong(shape.getId(), convertTraits(shape));
110-
case FLOAT -> Schema.createFloat(shape.getId(), convertTraits(shape));
111-
case DOCUMENT -> Schema.createDocument(shape.getId(), convertTraits(shape));
112-
case DOUBLE -> Schema.createDouble(shape.getId(), convertTraits(shape));
113-
case BIG_DECIMAL -> Schema.createBigDecimal(shape.getId(), convertTraits(shape));
114-
case BIG_INTEGER -> Schema.createBigInteger(shape.getId(), convertTraits(shape));
105+
case BLOB -> Schema.createBlob(id, convertTraits(shape));
106+
case BOOLEAN -> Schema.createBoolean(id, convertTraits(shape));
107+
case STRING -> Schema.createString(id, convertTraits(shape));
108+
case TIMESTAMP -> Schema.createTimestamp(id, convertTraits(shape));
109+
case BYTE -> Schema.createByte(id, convertTraits(shape));
110+
case SHORT -> Schema.createShort(id, convertTraits(shape));
111+
case INTEGER -> Schema.createInteger(id, convertTraits(shape));
112+
case LONG -> Schema.createLong(id, convertTraits(shape));
113+
case FLOAT -> Schema.createFloat(id, convertTraits(shape));
114+
case DOCUMENT -> Schema.createDocument(id, convertTraits(shape));
115+
case DOUBLE -> Schema.createDouble(id, convertTraits(shape));
116+
case BIG_DECIMAL -> Schema.createBigDecimal(id, convertTraits(shape));
117+
case BIG_INTEGER -> Schema.createBigInteger(id, convertTraits(shape));
115118
case ENUM -> Schema.createEnum(
116-
shape.getId(),
119+
id,
117120
new HashSet<>(shape.asEnumShape().orElseThrow().getEnumValues().values()),
118121
convertTraits(shape));
119122
case INT_ENUM -> Schema.createIntEnum(
120-
shape.getId(),
123+
id,
121124
new HashSet<>(shape.asIntEnumShape().orElseThrow().getEnumValues().values()),
122125
convertTraits(shape));
123-
case OPERATION -> Schema.createOperation(shape.getId(), convertTraits(shape));
124-
case SERVICE -> Schema.createService(shape.getId(), convertTraits(shape));
126+
case OPERATION -> Schema.createOperation(id, convertTraits(shape));
127+
case SERVICE -> Schema.createService(id, convertTraits(shape));
125128
default -> throw new UnsupportedOperationException("Unexpected shape: " + shape);
126129
};
127130
}
@@ -132,16 +135,33 @@ private static Trait[] convertTraits(Shape shape) {
132135
return traits;
133136
}
134137

138+
/**
139+
* Resolve the shape ID to use for a schema, honoring the {@link OriginalShapeIdTrait} left behind when model
140+
* transforms (e.g. {@code createDedicatedInputAndOutput}) rename a shape. Wire-facing behavior — such as the
141+
* restXml root element name, which derives from the schema's shape ID — must use the original modeled name, not
142+
* the transformed one. Mirrors the codegen schema generator. The {@code Unit} original ID is the exception:
143+
* synthesized dedicated inputs keep their generated ID rather than collapsing onto {@code smithy.api#Unit}.
144+
*/
145+
private static ShapeId schemaId(Shape shape) {
146+
var original = shape.getTrait(OriginalShapeIdTrait.class)
147+
.map(OriginalShapeIdTrait::getOriginalId)
148+
.orElse(null);
149+
if (original == null || UnitTypeTrait.UNIT.equals(original)) {
150+
return shape.getId();
151+
}
152+
return original;
153+
}
154+
135155
private SchemaBuilder getOrCreateRecursiveSchemaBuilder(Shape shape, Set<Shape> building) {
136156
SchemaBuilder builder;
137157
builder = recursiveBuilders.get(shape);
138158

139159
if (builder == null) {
140160
builder = switch (shape.getType()) {
141-
case LIST, SET -> Schema.listBuilder(shape.getId(), convertTraits(shape));
142-
case MAP -> Schema.mapBuilder(shape.getId(), convertTraits(shape));
143-
case STRUCTURE -> Schema.structureBuilder(shape.getId(), convertTraits(shape));
144-
case UNION -> Schema.unionBuilder(shape.getId(), convertTraits(shape));
161+
case LIST, SET -> Schema.listBuilder(schemaId(shape), convertTraits(shape));
162+
case MAP -> Schema.mapBuilder(schemaId(shape), convertTraits(shape));
163+
case STRUCTURE -> Schema.structureBuilder(schemaId(shape), convertTraits(shape));
164+
case UNION -> Schema.unionBuilder(schemaId(shape), convertTraits(shape));
145165
default -> throw new UnsupportedOperationException("Expected aggregate shape: " + shape);
146166
};
147167
SchemaBuilder previous = recursiveBuilders.putIfAbsent(shape, builder);

‎dynamic-schemas/src/main/java/software/amazon/smithy/java/dynamicschemas/SchemaGuidedDocumentBuilder.java‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,16 +103,28 @@ private Document deserialize(ShapeDeserializer decoder, Schema schema) {
103103
case BIG_DECIMAL -> new ContentDocument(Document.ofNumber(decoder.readBigDecimal(schema)), schema);
104104
case BIG_INTEGER -> new ContentDocument(Document.ofNumber(decoder.readBigInteger(schema)), schema);
105105
case LIST -> {
106+
var sparse = schema.hasTrait(TraitKey.SPARSE_TRAIT);
106107
var items = new SchemaList(schema.listMember());
107108
decoder.readList(schema, items, (it, memberDeserializer) -> {
108-
it.add(deserialize(memberDeserializer, it.schema));
109+
// A null element (only valid in @sparse lists) is retained as null; otherwise read the value.
110+
if (sparse && memberDeserializer.isNull()) {
111+
it.add(memberDeserializer.readNull());
112+
} else {
113+
it.add(deserialize(memberDeserializer, it.schema));
114+
}
109115
});
110116
yield new ContentDocument(Document.of(items), schema);
111117
}
112118
case MAP -> {
119+
var sparse = schema.hasTrait(TraitKey.SPARSE_TRAIT);
113120
var map = new SchemaMap(schema);
114121
decoder.readStringMap(schema, map, (state, mapKey, memberDeserializer) -> {
115-
state.put(mapKey, deserialize(memberDeserializer, state.schema.mapValueMember()));
122+
// A null value (only valid in @sparse maps) is retained as null; otherwise read the value.
123+
if (sparse && memberDeserializer.isNull()) {
124+
state.put(mapKey, memberDeserializer.readNull());
125+
} else {
126+
state.put(mapKey, deserialize(memberDeserializer, state.schema.mapValueMember()));
127+
}
116128
});
117129
yield new ContentDocument(Document.of(map), schema);
118130
}

‎protocol-test-harness/build.gradle.kts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ dependencies {
1212
implementation(project(":codegen:codegen-plugin"))
1313
implementation(libs.smithy.codegen)
1414
implementation(project(":client:client-core"))
15+
// Document-backed client models so protocol tests also exercise the DynamicClient path.
16+
implementation(project(":client:dynamic-client"))
17+
implementation(project(":dynamic-schemas"))
1518
implementation(libs.smithy.protocol.test.traits)
1619
implementation(project(":http:http-api"))
1720
implementation(project(":server:server-api"))

‎protocol-test-harness/src/main/java/software/amazon/smithy/java/protocoltests/harness/ComparisonUtils.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import java.util.Comparator;
1111
import org.assertj.core.api.recursive.comparison.RecursiveComparisonConfiguration;
1212
import software.amazon.smithy.java.core.serde.document.Document;
13+
import software.amazon.smithy.java.core.serde.document.DocumentEqualsFlags;
1314
import software.amazon.smithy.java.io.ByteBufferUtils;
1415
import software.amazon.smithy.java.io.datastream.DataStream;
1516
import software.amazon.smithy.java.retries.api.RetrySafety;
@@ -30,7 +31,11 @@ static RecursiveComparisonConfiguration getComparisonConfig() {
3031
// Compare doubles and floats as longs so NaN's will be equatable
3132
.withComparatorForType(nanPermittingDoubleComparator(), Double.class)
3233
.withComparatorForType(nanPermittingFloatComparator(), Float.class)
33-
.withComparatorForType((a, b) -> Document.equals(a, b) ? 0 : 1, Document.class)
34+
// Dynamic-path shapes are compared as Documents. Use NUMBER_PROMOTION so float/double values compare
35+
// with NaN/Infinity tolerance (mirroring the typed nanPermitting* comparators used for codegen shapes).
36+
.withComparatorForType(
37+
(a, b) -> Document.equals(a, b, DocumentEqualsFlags.NUMBER_PROMOTION) ? 0 : 1,
38+
Document.class)
3439
.withIgnoredFieldsOfTypes(RetrySafety.class)
3540
.build();
3641
}

0 commit comments

Comments
 (0)