From e84d0fa670bb2f9bfdb45220198312824db5e031 Mon Sep 17 00:00:00 2001 From: Luc Talatinian Date: Thu, 6 Aug 2026 02:42:16 -0400 Subject: [PATCH 1/2] fixup json snaps and deser --- .../SerdeResponseSnapshotTests.java | 338 +++++++++++------- .../integration/SerdeSnapshotTests.java | 79 +++- .../integration/SnapshotInputGenerator.java | 118 +++++- .../integration/SnapshotOutputGenerator.java | 5 +- .../codegen/serde2/StructureSerializer.java | 3 +- schema.go | 36 +- testing/struct.go | 16 + 7 files changed, 428 insertions(+), 167 deletions(-) diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SerdeResponseSnapshotTests.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SerdeResponseSnapshotTests.java index 7135e8834..ce6d38ed6 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SerdeResponseSnapshotTests.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SerdeResponseSnapshotTests.java @@ -15,14 +15,11 @@ package software.amazon.smithy.go.codegen.integration; -import static software.amazon.smithy.go.codegen.GoWriter.goTemplate; - import java.util.ArrayList; import java.util.Comparator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Set; import software.amazon.smithy.aws.traits.protocols.AwsJson1_0Trait; import software.amazon.smithy.aws.traits.protocols.AwsJson1_1Trait; import software.amazon.smithy.aws.traits.protocols.AwsQueryTrait; @@ -47,15 +44,25 @@ import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.traits.ErrorTrait; import software.amazon.smithy.model.traits.HttpErrorTrait; +import software.amazon.smithy.model.traits.HttpHeaderTrait; +import software.amazon.smithy.model.traits.HttpLabelTrait; +import software.amazon.smithy.model.traits.HttpPrefixHeadersTrait; +import software.amazon.smithy.model.traits.HttpQueryParamsTrait; +import software.amazon.smithy.model.traits.HttpQueryTrait; import software.amazon.smithy.protocol.traits.Rpcv2CborTrait; -import software.amazon.smithy.utils.MapUtils; import software.amazon.smithy.utils.StringUtils; +import static software.amazon.smithy.go.codegen.GoWriter.goTemplate; public class SerdeResponseSnapshotTests implements GoIntegration { - private static final Set SKIP_OPERATIONS = Set.of( - // Predict has a customization where an input member goes into the URL. The check path still builds a - // request to drive the deserialize, so that customization complicates things here too -- just skip it. - "com.amazonaws.machinelearning#Predict" + // SDK customizations that aren't visible in the model, so they can't be + // detected by inspecting traits + // + // shape id -> reason + private static final Map SKIP_OPERATIONS = Map.of( + "com.amazonaws.machinelearning#Predict", "endpoint customization", + "com.amazonaws.sqs#ReceiveMessage", "md5 customization", + "com.amazonaws.sqs#SendMessage", "md5 customization", + "com.amazonaws.sqs#SendMessageBatch", "md5 customization" ); @Override @@ -73,7 +80,7 @@ public void writeAdditionalFiles( } var serviceSchemaRef = "schemas." + StringUtils.capitalize(service.getId().getName(service)); - var generator = new SnapshotOutputGenerator(model, symbolProvider); + var generator = new SnapshotOutputGenerator(model, symbolProvider, settings); // The Check tests are ALWAYS generated: they only read a fixture, inject it, run the real deserialize path, // and compare the output. They have no dependency on output Serialize/schemas, so they compile and run whether @@ -82,7 +89,7 @@ public void writeAdditionalFiles( goDelegator.useFileWriter("response_snapshot_test.go", settings.getModuleName(), writer -> { writer.addBuildTag("response_snapshot"); writer.write(checkCommonSource()); - writer.write(checks(model, service, symbolProvider, generator)); + writer.write(checks(settings, model, service, symbolProvider, generator)); }); // The Update tests (fixture generation) serialize the output value via a throwaway protocol, which requires @@ -92,17 +99,13 @@ public void writeAdditionalFiles( goDelegator.useFileWriter("response_snapshot_update_test.go", settings.getModuleName(), writer -> { writer.addBuildTag("response_snapshot"); writer.addImport(settings.getModuleName() + "/schemas", "schemas"); - writer.write(updateCommonSource()); + writer.write(updateCommonSource(settings.getProtocol())); writer.write(updaters(model, service, symbolProvider, generator, serviceSchemaRef, protoNew, - errorFraming(settings.getProtocol()))); + settings.getProtocol())); }); } } - // Resolves the throwaway protocol constructor used to serialize output values into wire responses. Returns null - // only for protocols we don't recognize (the file is then not emitted). Every schema-serde protocol is wired up; - // the response-snapshot files are still only emitted for a service once it migrates to schema-serde (the Update - // half is gated on !useLegacySerde), so this generalizes without any manual per-protocol flip. private Symbol resolveProtocolCtor(ShapeId protocol) { if (Rpcv2CborTrait.ID.equals(protocol)) { return SmithyGoDependency.SMITHY_PROTOCOL_RPCV2.func("NewCBOR"); @@ -122,32 +125,6 @@ private Symbol resolveProtocolCtor(ShapeId protocol) { return null; } - // How the modeled-error discriminator + status are framed onto the captured wire response, per protocol. The - // success path is protocol-agnostic (serialize output, capture headers+body, status 200); only errors need - // protocol-specific framing because the throwaway SerializeRequest emits just the error members, not the - // protocol's error envelope/discriminator that the deserializer keys on. - private enum ErrorFraming { - // rpcv2Cbor: discriminator is "__type" inside the CBOR body map (no header form). - CBOR_BODY, - // awsJson1_0/1_1 + restJson1: deserializers resolve the code from the X-Amzn-ErrorType header first, so we - // set that and leave the serialized JSON members as the body (which the error deserializer then reads). - JSON_HEADER, - // restXml/awsQuery/ec2Query: deserializers parse an ...... - // envelope. awsQuery/ec2Query serialize requests as form-urlencoded rather than XML, so for those two the - // captured body is not XML members. - XML_ENVELOPE - } - - private ErrorFraming errorFraming(ShapeId protocol) { - if (Rpcv2CborTrait.ID.equals(protocol)) { - return ErrorFraming.CBOR_BODY; - } else if (RestXmlTrait.ID.equals(protocol) || AwsQueryTrait.ID.equals(protocol) - || Ec2QueryTrait.ID.equals(protocol)) { - return ErrorFraming.XML_ENVELOPE; - } - return ErrorFraming.JSON_HEADER; - } - // HTTP status for an error fixture: explicit @httpError code, else Smithy's default (400 client / 500 server). private int errorStatus(StructureShape error) { if (error.hasTrait(HttpErrorTrait.class)) { @@ -266,8 +243,12 @@ func serdeRespClient(status int, header http.Header, body []byte) *Client { // updateCommonSource emits helpers used only by the (schema-serde-gated) Update tests: fixture directory creation // and the fixture writer. serdeRespSSPath / serdeRespSSPrefix live in the always-generated Check file. - private Writable updateCommonSource() { + private Writable updateCommonSource(ShapeId protocol) { return writer -> { + writer.addUseImports(SmithyGoDependency.TESTING); + writer.addUseImports(SmithyGoDependency.SMITHY_CBOR); + writer.addUseImports(SmithyGoDependency.JSON); + writer.addUseImports(SmithyGoDependency.BYTES); writer.addUseImports(SmithyGoDependency.OS); writer.addUseImports(SmithyGoDependency.FS); writer.addUseImports(SmithyGoDependency.IO); @@ -286,6 +267,11 @@ func serdeRespCreatePath(path string) (*os.File, error) { } func serdeRespWriteSnapshot(op string, status int, header http.Header, body []byte) error { + if es, eh, eb, err := serdeRespReadSnapshot(op); err == nil && + es == status && serdeRespHeaderEqual(eh, header) && bytes.Equal(body, eb) { + return nil + } + f, err := serdeRespCreatePath(serdeRespSSPath(op)) if err != nil { return err @@ -314,11 +300,64 @@ func serdeRespWriteSnapshot(op string, status int, header http.Header, body []by return err } - // serdeRespXMLErrorEnvelope wraps serialized error members in the XML - // error envelope the restXml/query deserializers parse: - // CODEMEMBERS. - // It strips the serialized body's outer root element and re-parents the - // members under . + func serdeRespHeaderEqual(a, b http.Header) bool { + if len(a) != len(b) { + return false + } + for k, av := range a { + bv, ok := b[k] + if !ok || !slices.Equal(av, bv) { + return false + } + } + return true + } + + // inject __type into cbor since our error "serializers" don't like a real response would have + func serdeRespSpliceCBORType(t *testing.T, body []byte, code string) []byte { + pair := append( + smithycbor.Encode(smithycbor.String("__type")), + smithycbor.Encode(smithycbor.String(code))..., + ) + if len(body) == 0 { + return append(append([]byte{0xbf}, pair...), 0xff) + } + + if body[0] != 0xbf { + t.Fatalf("expected cbor indefinite map header, got %#x", body[0]) + } + + out := make([]byte, 0, len(body)+len(pair)) + out = append(out, 0xbf) + out = append(out, pair...) + return append(out, body[1:]...) + } + + // inject __type into json since our error "serializers" don't like a real response would have + func serdeRespSpliceJSONType(t *testing.T, body []byte, code string) []byte { + quoted, err := json.Marshal(code) + if err != nil { + t.Fatal(err) + } + + entry := append([]byte(`"__type":`), quoted...) + trimmed := bytes.TrimLeft(body, " \\t\\r\\n") + if len(trimmed) == 0 { + return append(append([]byte{'{'}, entry...), '}') + } + if trimmed[0] != '{' { + t.Fatalf("expected json object body, got %q", trimmed[0]) + } + + rest := bytes.TrimLeft(trimmed[1:], " \\t\\r\\n") + out := append([]byte{'{'}, entry...) + if len(rest) > 0 && rest[0] != '}' { + out = append(out, ',') + } + return append(out, rest...) + } + + // inject the xml envelope since our error "serializers" don't like a real response woul have func serdeRespXMLErrorEnvelope(body []byte, code string) []byte { inner := "" s := strings.TrimSpace(string(body)) @@ -339,15 +378,23 @@ func serdeRespXMLErrorEnvelope(body []byte, code string) []byte { } private Writable checks( - Model model, ServiceShape service, SymbolProvider symbolProvider, SnapshotOutputGenerator generator + GoSettings settings, Model model, ServiceShape service, SymbolProvider symbolProvider, + SnapshotOutputGenerator generator ) { var eventStreamIndex = EventStreamIndex.of(model); + var inputs = new SnapshotInputGenerator(model, symbolProvider, settings, false); var writables = new ArrayList(); var operations = sortedOperations(model, service, eventStreamIndex); - for (var operation : operations) { - var inputSymbol = symbolProvider.toSymbol(model.expectShape(operation.getInputShape())); + for (var operation : allOperations(model, service)) { + var reason = skipReason(model, operation, eventStreamIndex); + if (reason != null) { + writables.add(writeSkip( + "TestCheckResponseSnapshot_" + symbolProvider.toSymbol(operation).getName(), reason)); + continue; + } + var inputValue = inputs.generateCases(operation).get(0).input(); for (var testCase : generator.generateCases(operation)) { - writables.add(writeCheck(operation, testCase, symbolProvider, inputSymbol)); + writables.add(writeCheck(operation, testCase, symbolProvider, inputValue)); } } // Modeled errors run the same ReadStruct/ReadUnion deserialize machinery as success outputs (the @@ -358,21 +405,47 @@ private Writable checks( for (var entry : errorRepOps(model, operations).entrySet()) { var errorShape = entry.getKey(); var repOp = entry.getValue(); - var inputSymbol = symbolProvider.toSymbol(model.expectShape(repOp.getInputShape())); + var inputValue = inputs.generateCases(repOp).get(0).input(); var errorSymbol = symbolProvider.toSymbol(errorShape); + if (isAsymmetric(errorShape)) { + writables.add(writeSkip( + "TestCheckResponseSnapshot_Error_" + errorSymbol.getName(), "asymmetric")); + continue; + } for (var errorCase : generator.generateCasesForError(errorShape)) { - writables.add(writeErrorCheck(repOp, errorCase, symbolProvider, inputSymbol, errorSymbol)); + writables.add(writeErrorCheck(repOp, errorCase, symbolProvider, inputValue, errorSymbol)); } } return ChainWritable.of(writables).compose(); } + // Every operation the service contains, sorted by shape id, skipped or not. + private List allOperations(Model model, ServiceShape service) { + var operations = new ArrayList<>(TopDownIndex.of(model).getContainedOperations(service)); + operations.sort(Comparator.comparing(o -> o.getId().toString())); + return operations; + } + + // A test that exists only to record, in the generated SDK, that an operation or error has no snapshot coverage. + private Writable writeSkip(String testName, String reason) { + return goTemplate(""" + func $name:L(t *$testingT:T) { + t.Skip($reason:S) + } + """, + Map.of( + "name", testName, + "reason", reason, + "testingT", GoStdlibTypes.Testing.T + )); + } + // Returns the service's non-skipped operations sorted by shape id, so the representative-operation choice for // each error (and thus fixture/test names) is stable across regenerations. private List sortedOperations(Model model, ServiceShape service, EventStreamIndex esi) { var operations = new ArrayList(); for (var operation : TopDownIndex.of(model).getContainedOperations(service)) { - if (!skip(operation, esi)) { + if (!skip(model, operation, esi)) { operations.add(operation); } } @@ -405,7 +478,7 @@ private LinkedHashMap errorRepOps( private Writable updaters( Model model, ServiceShape service, SymbolProvider symbolProvider, SnapshotOutputGenerator generator, - String serviceSchemaRef, Symbol protoNew, ErrorFraming framing + String serviceSchemaRef, Symbol protoNew, ShapeId protocol ) { var eventStreamIndex = EventStreamIndex.of(model); var writables = new ArrayList(); @@ -415,8 +488,8 @@ private Writable updaters( var opSchemaRef = SchemaGenerator.getSchemaRef(operation, service); var outSchemaRef = SchemaGenerator.getSchemaRef(outputShape, service); for (var testCase : generator.generateCases(operation)) { - writables.add(writeUpdate( - operation, testCase, symbolProvider, serviceSchemaRef, protoNew, opSchemaRef, outSchemaRef)); + writables.add(writeUpdate(operation, testCase, symbolProvider, serviceSchemaRef, protoNew, + opSchemaRef, outSchemaRef)); } } for (var entry : errorRepOps(model, operations).entrySet()) { @@ -428,21 +501,66 @@ private Writable updaters( for (var errorCase : generator.generateCasesForError(errorShape)) { writables.add(writeErrorUpdate( repOp, errorCase, symbolProvider, serviceSchemaRef, protoNew, opSchemaRef, - errSchemaRef, errorSymbol, framing, errorStatus(errorShape))); + errSchemaRef, errorSymbol, errorShape, protocol, errorStatus(errorShape))); } } return ChainWritable.of(writables).compose(); } - private boolean skip(OperationShape operation, EventStreamIndex eventStreamIndex) { - return SKIP_OPERATIONS.contains(operation.getId().toString()) - || eventStreamIndex.getInputInfo(operation).isPresent() - || eventStreamIndex.getOutputInfo(operation).isPresent(); + private boolean skip(Model model, OperationShape operation, EventStreamIndex eventStreamIndex) { + return skipReason(model, operation, eventStreamIndex) != null; + } + + private String skipReason(Model model, OperationShape operation, EventStreamIndex eventStreamIndex) { + var staticReason = SKIP_OPERATIONS.get(operation.getId().toString()); + if (staticReason != null) { + return staticReason; + } + + if (eventStreamIndex.getInputInfo(operation).isPresent() || eventStreamIndex.getOutputInfo(operation).isPresent()) { + return "event stream operation"; + } + + var output = model.expectShape(operation.getOutputShape(), StructureShape.class); + if (isAsymmetric(output)) { + return "asymmetric"; + } + + return null; + } + + // since we are basically just flipping our serializer around to make responses, there are a number of things that + // don't translate symmetrically and result in broken snapshots, so we have to just not cover those for now + private boolean isAsymmetric(StructureShape shape) { + for (var member : shape.getAllMembers().values()) { + // an actual response serializer would ignore these + if (member.hasTrait(HttpQueryTrait.class) || member.hasTrait(HttpQueryParamsTrait.class) || member.hasTrait(HttpLabelTrait.class)) { + return true; + } + + // doesn't translate because there's special handling w/ the ContentLength field + if (member.hasTrait(HttpHeaderTrait.class)) { + var header = member.expectTrait(HttpHeaderTrait.class); + if (header.getValue().equalsIgnoreCase("Content-Length")) { + return true; + } + } + + // steals any headers from the protocol serializer because no prefix means everything + if (member.hasTrait(HttpPrefixHeadersTrait.class)) { + var headers = member.expectTrait(HttpPrefixHeadersTrait.class); + if (headers.getValue().isEmpty()) { + return true; + } + } + } + + return false; } private Writable writeCheck( OperationShape operation, SnapshotInputGenerator.TestCase testCase, SymbolProvider symbolProvider, - Symbol inputSymbol + Writable inputValue ) { var opName = symbolProvider.toSymbol(operation).getName(); return goTemplate(""" @@ -456,7 +574,7 @@ private Writable writeCheck( t.Fatal(err) } svc := serdeRespClient(status, header, body) - got, err := svc.$op:L($ctx:T(), &$input:T{}) + got, err := svc.$op:L($ctx:T(), $input:W) if err != nil { t.Fatal(err) } @@ -465,12 +583,12 @@ private Writable writeCheck( } } """, - MapUtils.of( + Map.of( "name", opName, "fixture", opName + ".response", "op", opName, "output", testCase.input(), - "input", inputSymbol, + "input", inputValue, "testingT", GoStdlibTypes.Testing.T, "ctx", GoStdlibTypes.Context.Background, "compare", SmithyGoDependency.SMITHY_TESTING.func("CompareValues") @@ -505,17 +623,17 @@ private Writable writeUpdate( } } """, - MapUtils.of( - "name", opName, - "fixture", opName + ".response", - "output", testCase.input(), - "protoNew", protoNew, - "service", serviceSchemaRef, - "op", opSchemaRef, - "out", outSchemaRef, - "newOpSchema", SmithyGoDependency.SMITHY.func("NewOperationSchema"), - "testingT", GoStdlibTypes.Testing.T, - "ctx", GoStdlibTypes.Context.Background + Map.ofEntries( + Map.entry("name", opName), + Map.entry("fixture", opName + ".response"), + Map.entry("output", testCase.input()), + Map.entry("protoNew", protoNew), + Map.entry("service", serviceSchemaRef), + Map.entry("op", opSchemaRef), + Map.entry("out", outSchemaRef), + Map.entry("newOpSchema", SmithyGoDependency.SMITHY.func("NewOperationSchema")), + Map.entry("testingT", GoStdlibTypes.Testing.T), + Map.entry("ctx", GoStdlibTypes.Context.Background) )); } @@ -523,7 +641,7 @@ private Writable writeUpdate( // errors.As into the modeled error type, and compares against the deterministic expected value. private Writable writeErrorCheck( OperationShape operation, SnapshotInputGenerator.TestCase testCase, SymbolProvider symbolProvider, - Symbol inputSymbol, Symbol errorSymbol + Writable inputValue, Symbol errorSymbol ) { var opName = symbolProvider.toSymbol(operation).getName(); var errName = errorSymbol.getName(); @@ -540,7 +658,7 @@ private Writable writeErrorCheck( t.Fatal(err) } svc := serdeRespClient(status, header, body) - _, opErr := svc.$op:L($ctx:T(), &$input:T{}) + _, opErr := svc.$op:L($ctx:T(), $input:W) if opErr == nil { t.Fatal("expected error, got nil") } @@ -553,12 +671,12 @@ private Writable writeErrorCheck( } } """, - MapUtils.of( + Map.of( "name", testName, "op", opName, "fixture", fixtureName, "want", testCase.input(), - "input", inputSymbol, + "input", inputValue, "err", errorSymbol, "testingT", GoStdlibTypes.Testing.T, "ctx", GoStdlibTypes.Context.Background, @@ -572,7 +690,7 @@ private Writable writeErrorCheck( private Writable writeErrorUpdate( OperationShape operation, SnapshotInputGenerator.TestCase testCase, SymbolProvider symbolProvider, String serviceSchemaRef, Symbol protoNew, String opSchemaRef, String errSchemaRef, Symbol errorSymbol, - ErrorFraming framing, int status + StructureShape errorShape, ShapeId protocol, int status ) { var errName = errorSymbol.getName(); var testName = "Error_" + errName; @@ -610,54 +728,24 @@ private Writable writeErrorUpdate( Map.entry("op", opSchemaRef), Map.entry("err", errSchemaRef), Map.entry("status", status), - Map.entry("frame", errorFrameWritable(framing)), + Map.entry("frame", errorFrameWritable(protocol)), Map.entry("newOpSchema", SmithyGoDependency.SMITHY.func("NewOperationSchema")), Map.entry("testingT", GoStdlibTypes.Testing.T), Map.entry("ctx", GoStdlibTypes.Context.Background) )); } - // Protocol-specific fragment that stamps the error discriminator onto the captured wire response. Runs with - // `built` (the built request), `body` (its captured body), and `want` (the modeled error value) in scope; - // mutates `body` and/or `built.Header`. `want.ErrorCode()` is exactly the key the client's type registry uses. - private Writable errorFrameWritable(ErrorFraming framing) { - switch (framing) { - case CBOR_BODY: - return goTemplate(""" - // Inject the CBOR error discriminator into the body map so the deserializer routes to the - // modeled error type. - var m $cborMap:T - if len(body) > 0 { - v, err := $cborDecode:T(body) - if err != nil { - t.Fatal(err) - } - mm, ok := v.($cborMap:T) - if !ok { - t.Fatalf("expected cbor map body, got %T", v) - } - m = mm - } else { - m = $cborMap:T{} - } - m["__type"] = $cborString:T(want.ErrorCode()) - body = $cborEncode:T(m)""", - Map.of( - "cborMap", SmithyGoDependency.SMITHY_CBOR.valueSymbol("Map"), - "cborString", SmithyGoDependency.SMITHY_CBOR.valueSymbol("String"), - "cborDecode", SmithyGoDependency.SMITHY_CBOR.func("Decode"), - "cborEncode", SmithyGoDependency.SMITHY_CBOR.func("Encode") - )); - case XML_ENVELOPE: - return goTemplate(""" - // Wrap the serialized members in the XML error envelope the deserializer parses. - body = serdeRespXMLErrorEnvelope(body, want.ErrorCode())"""); - case JSON_HEADER: - default: - return goTemplate(""" - // Route to the modeled error via the standard error-type header; the serialized JSON members - // remain the body for the error deserializer. - built.Header.Set("X-Amzn-ErrorType", want.ErrorCode())"""); + private Writable errorFrameWritable(ShapeId protocol) { + if (Rpcv2CborTrait.ID.equals(protocol)) { + return goTemplate(""" + body = serdeRespSpliceCBORType(t, body, want.ErrorCode())"""); } + if (RestXmlTrait.ID.equals(protocol) || AwsQueryTrait.ID.equals(protocol) || Ec2QueryTrait.ID.equals(protocol)) { + return goTemplate(""" + body = serdeRespXMLErrorEnvelope(body, want.ErrorCode())"""); + } + // json + return goTemplate(""" + body = serdeRespSpliceJSONType(t, body, want.ErrorCode())"""); } } diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SerdeSnapshotTests.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SerdeSnapshotTests.java index 4cfe7923f..9efeffba7 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SerdeSnapshotTests.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SerdeSnapshotTests.java @@ -4,6 +4,9 @@ import java.util.ArrayList; import java.util.Set; +import software.amazon.smithy.aws.traits.protocols.AwsJson1_0Trait; +import software.amazon.smithy.aws.traits.protocols.AwsJson1_1Trait; +import software.amazon.smithy.aws.traits.protocols.RestJson1Trait; import software.amazon.smithy.codegen.core.SymbolProvider; import software.amazon.smithy.go.codegen.ChainWritable; import software.amazon.smithy.go.codegen.GoDelegator; @@ -32,32 +35,37 @@ public void writeAdditionalFiles( goDelegator.useFileWriter("request_snapshot_test.go", settings.getModuleName(), writer -> { writer.addBuildTag("request_snapshot"); writer.write(commonSource()); - writer.write(bodyEqual(isCbor(settings))); + writer.write(bodyEqual(settings)); var service = settings.getService(model); - var generator = new SnapshotInputGenerator(model, symbolProvider); + var generator = new SnapshotInputGenerator(model, symbolProvider, settings, false); writer.write(snapshotTests(model, service, symbolProvider, generator)); writer.write(snapshotUpdaters(model, service, symbolProvider, generator)); }); } - private static boolean isCbor(GoSettings settings) { - return Rpcv2CborTrait.ID.equals(settings.getProtocol()); + // Emits serdeBodyEqual, the request-body comparator, per protocol. Every variant falls back to a byte compare + // when either side won't decode, so a malformed golden fails loudly rather than silently passing. + private Writable bodyEqual(GoSettings settings) { + var protocol = settings.getProtocol(); + if (Rpcv2CborTrait.ID.equals(protocol)) { + return cborBodyEqual(); + } else if (AwsJson1_0Trait.ID.equals(protocol) || AwsJson1_1Trait.ID.equals(protocol) + || RestJson1Trait.ID.equals(protocol)) { + return jsonBodyEqual(); + } + // restXml has the same member-ordering divergence in XML element form, and awsQuery/ec2Query bodies are + // form-urlencoded. Those are out of scope for now (JSON snapshots only), so they stay on a byte compare. + return goTemplate(""" + func serdeBodyEqual(got, expected []byte) bool { + return bytes.Equal(got, expected) + } + """); } - // Emits serdeBodyEqual, the request-body comparator. Most protocols serialize - // deterministically, so a raw byte compare is correct. rpcv2Cbor encodes struct - // fields as a CBOR map and the encoder emits map entries in Go map iteration - // order, so the same input produces different byte orderings across runs. For - // that protocol we compare decoded CBOR values, which is order-independent. - private Writable bodyEqual(boolean isCbor) { - if (!isCbor) { - return goTemplate(""" - func serdeBodyEqual(got, expected []byte) bool { - return bytes.Equal(got, expected) - } - """); - } + // rpcv2Cbor encodes struct fields as a CBOR map and the encoder emits map entries in Go map iteration order, so + // the same input produces different byte orderings across runs. Comparing decoded values is order-independent. + private Writable cborBodyEqual() { return writer -> { writer.addUseImports(SmithyGoDependency.REFLECT); writer.addUseImports(SmithyGoDependency.SMITHY_CBOR); @@ -77,6 +85,43 @@ func serdeBodyEqual(got, expected []byte) bool { }; } + // JSON object member order isn't semantically meaningful, and legacy serde orders members by ShapeId + // (case-insensitive, via TreeSet) while schema-serde orders them by member name (byte order). A byte compare + // therefore reports thousands of semantically identical bodies as mismatches for the whole legacy -> + // schema-serde transition. + private Writable jsonBodyEqual() { + return writer -> { + writer.addUseImports(SmithyGoDependency.REFLECT); + writer.addUseImports(SmithyGoDependency.JSON); + writer.write(""" + func serdeBodyEqual(got, expected []byte) bool { + if len(got) == 0 || len(expected) == 0 { + return bytes.Equal(got, expected) + } + gv, gok := serdeDecodeJSON(got) + ev, eok := serdeDecodeJSON(expected) + if !gok || !eok { + return bytes.Equal(got, expected) + } + return reflect.DeepEqual(gv, ev) + } + + // serdeDecodeJSON decodes a body for structural comparison. Numbers are kept as + // json.Number rather than float64 so a large int64 doesn't lose precision (which would + // mask a real difference) and so numeric formatting differences still show up. + func serdeDecodeJSON(b []byte) (any, bool) { + d := json.NewDecoder(bytes.NewReader(b)) + d.UseNumber() + var v any + if err := d.Decode(&v); err != nil { + return nil, false + } + return v, true + } + """); + }; + } + private Writable commonSource() { return writer -> { writer.addUseImports(SmithyGoDependency.OS); diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SnapshotInputGenerator.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SnapshotInputGenerator.java index 20dc6eaae..e2a5f1873 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SnapshotInputGenerator.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SnapshotInputGenerator.java @@ -15,12 +15,17 @@ package software.amazon.smithy.go.codegen.integration; -import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; +import software.amazon.smithy.aws.traits.protocols.AwsJson1_0Trait; +import software.amazon.smithy.aws.traits.protocols.AwsJson1_1Trait; +import software.amazon.smithy.aws.traits.protocols.RestJson1Trait; import software.amazon.smithy.codegen.core.SymbolProvider; +import software.amazon.smithy.go.codegen.GoSettings; +import software.amazon.smithy.go.codegen.GoStdlibTypes; import software.amazon.smithy.go.codegen.GoWriter; +import software.amazon.smithy.go.codegen.ProtocolDocumentGenerator; import software.amazon.smithy.go.codegen.SmithyGoDependency; import software.amazon.smithy.go.codegen.Writable; import software.amazon.smithy.go.codegen.knowledge.GoPointableIndex; @@ -31,9 +36,15 @@ import software.amazon.smithy.model.shapes.OperationShape; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.shapes.ShapeType; import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.shapes.UnionShape; import software.amazon.smithy.model.traits.EnumTrait; +import software.amazon.smithy.model.traits.ErrorTrait; +import software.amazon.smithy.model.traits.HostLabelTrait; +import software.amazon.smithy.model.traits.HttpHeaderTrait; +import software.amazon.smithy.model.traits.HttpResponseCodeTrait; +import software.amazon.smithy.model.traits.JsonNameTrait; import software.amazon.smithy.model.traits.StreamingTrait; /** @@ -52,11 +63,22 @@ public final class SnapshotInputGenerator { private final Model model; private final SymbolProvider symbolProvider; private final GoPointableIndex pointableIndex; + private final GoSettings settings; + private final boolean responseMode; - public SnapshotInputGenerator(Model model, SymbolProvider symbolProvider) { + /** + * @param responseMode When true, values are generated for a shape deserialized from a RESPONSE: + * {@code @httpResponseCode} takes the fixture's status, and a member occupying the protocol's + * error-code slot takes the error's shape name. + */ + public SnapshotInputGenerator( + Model model, SymbolProvider symbolProvider, GoSettings settings, boolean responseMode + ) { this.model = model; this.symbolProvider = symbolProvider; this.pointableIndex = GoPointableIndex.of(model); + this.settings = settings; + this.responseMode = responseMode; } /** @@ -86,6 +108,67 @@ public List generateCasesForShape(StructureShape shape) { private static final int MAX_DEPTH = 10; + private static final String ERROR_TYPE_HEADER = "x-amzn-errortype"; + private static final String ERROR_CODE_MEMBER = "code"; + + // legacy json resolves errortype header -> code -> __type + // schema-serde resolves errortype header -> __type -> code + // + // the latter is objectively more correct because some services e.g. chime actually have a separate modeled code + // field which would _not_ have the error, meaning if errortype header was not set there we'd actually resolve the + // wrong thing + // + // to make the old deserializer figure out the "correct" code, we just set any modeled code member to the error + // shape name as well + private String errorCodeValue(MemberShape member) { + if (!responseMode || !isJsonProtocol()) { + return null; + } + var container = model.expectShape(member.getContainer()); + if (!container.hasTrait(ErrorTrait.class)) { + return null; + } + if (member.hasTrait(HttpHeaderTrait.class) + && member.expectTrait(HttpHeaderTrait.class).getValue().equalsIgnoreCase(ERROR_TYPE_HEADER)) { + return container.getId().getName(); + } + if (wireName(member).equalsIgnoreCase(ERROR_CODE_MEMBER)) { + return container.getId().getName(); + } + return null; + } + + private String wireName(MemberShape member) { + return member.hasTrait(JsonNameTrait.class) + ? member.expectTrait(JsonNameTrait.class).getValue() + : member.getMemberName(); + } + + private boolean isJsonProtocol() { + var protocol = settings.getProtocol(); + return AwsJson1_0Trait.ID.equals(protocol) + || AwsJson1_1Trait.ID.equals(protocol) + || RestJson1Trait.ID.equals(protocol); + } + + // Success fixtures are written with HTTP 200, so an @httpResponseCode member deserializes to exactly that. + private boolean isSuccessResponseCode(MemberShape member) { + return responseMode && member.hasTrait(HttpResponseCodeTrait.class); + } + + // A streaming UNION is an event stream, whose framing the plain serializer doesn't produce. Those operations are + // skipped wholesale by the snapshot integrations; this is belt-and-braces for a shape reached some other way. + private boolean isEventStream(Shape target) { + return target.hasTrait(StreamingTrait.class) && target.getType() != ShapeType.BLOB; + } + + // A streaming blob gets a deterministic reader: the request serializer routes it into the body, the response + // deserializer hands the body back as the stream, and CompareValues short-circuits on io.Reader and compares + // contents. So it's an assertion about the body rather than a skipped member. + private boolean isStreamingBlob(Shape target) { + return target.getType() == ShapeType.BLOB && target.hasTrait(StreamingTrait.class); + } + private void writeStructure(GoWriter writer, StructureShape shape, Set visited, UnionChoice choice, boolean pointable) { if (!visited.add(shape.getId()) || visited.size() > MAX_DEPTH) { @@ -103,7 +186,7 @@ private void writeStructure(GoWriter writer, StructureShape shape, Set writer.indent(); for (var member : shape.getAllMembers().values()) { var target = model.expectShape(member.getTarget()); - if (target.hasTrait(StreamingTrait.class)) { + if (isEventStream(target)) { continue; } var memberName = symbolProvider.toMemberName(member); @@ -121,6 +204,18 @@ private void writeMemberValue( GoWriter writer, MemberShape member, Shape target, Set visited, UnionChoice choice ) { boolean needsPointer = pointableIndex.isPointable(member); + if (isSuccessResponseCode(member)) { + writeScalar(writer, needsPointer, "200", SmithyGoDependency.SMITHY_PTR, "Int32"); + return; + } + if (isStreamingBlob(target)) { + var memberName = symbolProvider.toMemberName(member); + writer.writeInline("$T($T([]byte($S)))", + SmithyGoDependency.IO.valueSymbol("NopCloser"), + GoStdlibTypes.Bytes.NewReader, + "__" + memberName + "__"); + return; + } switch (target.getType()) { case BOOLEAN -> writeScalar(writer, needsPointer, "true", SmithyGoDependency.SMITHY_PTR, "Bool"); case BYTE -> writeScalar(writer, needsPointer, "1", SmithyGoDependency.SMITHY_PTR, "Int8"); @@ -144,7 +239,10 @@ private void writeMemberValue( writer.writeInline("$T", memberSymbol); } else { var memberName = symbolProvider.toMemberName(member); - if (member.hasTrait(software.amazon.smithy.model.traits.HostLabelTrait.class)) { + var errorCode = errorCodeValue(member); + if (errorCode != null) { + writeStringValue(writer, needsPointer, errorCode); + } else if (member.hasTrait(HostLabelTrait.class)) { writeStringValue(writer, needsPointer, memberName + "-value"); } else { writeStringValue(writer, needsPointer, "__" + memberName + "__"); @@ -154,7 +252,11 @@ private void writeMemberValue( case ENUM -> { var enumSymbol = symbolProvider.toSymbol(target); var members = target.getAllMembers().values(); - if (members.isEmpty()) { + var errorCode = errorCodeValue(member); + if (errorCode != null) { + // The slot holds the code, which is a shape name rather than one of the enum's own values. + writer.writeInline("$T($S)", enumSymbol, errorCode); + } else if (members.isEmpty()) { writer.writeInline("$T(\"\")", enumSymbol); } else { var firstMember = members.iterator().next(); @@ -192,7 +294,11 @@ private void writeMemberValue( case STRUCTURE -> writeStructure(writer, target.asStructureShape().get(), visited, choice, needsPointer); case UNION -> writeUnion(writer, target.asUnionShape().get(), visited, choice); - case DOCUMENT -> writer.writeInline("nil"); + case DOCUMENT -> { + var newLazyDocument = ProtocolDocumentGenerator.Utilities.getDocumentSymbolBuilder( + settings, ProtocolDocumentGenerator.NEW_LAZY_DOCUMENT).build(); + writer.writeInline("$T($S)", newLazyDocument, "__Document__"); + } default -> writer.writeInline("nil"); } } diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SnapshotOutputGenerator.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SnapshotOutputGenerator.java index 1d2b76f41..93b01f2e7 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SnapshotOutputGenerator.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/SnapshotOutputGenerator.java @@ -17,6 +17,7 @@ import java.util.List; import software.amazon.smithy.codegen.core.SymbolProvider; +import software.amazon.smithy.go.codegen.GoSettings; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.OperationShape; import software.amazon.smithy.model.shapes.StructureShape; @@ -30,9 +31,9 @@ public final class SnapshotOutputGenerator { private final Model model; private final SnapshotInputGenerator values; - public SnapshotOutputGenerator(Model model, SymbolProvider symbolProvider) { + public SnapshotOutputGenerator(Model model, SymbolProvider symbolProvider, GoSettings settings) { this.model = model; - this.values = new SnapshotInputGenerator(model, symbolProvider); + this.values = new SnapshotInputGenerator(model, symbolProvider, settings, true); } /** diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/serde2/StructureSerializer.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/serde2/StructureSerializer.java index c92097252..4339dba6f 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/serde2/StructureSerializer.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/serde2/StructureSerializer.java @@ -103,7 +103,8 @@ private void generateSerializeMember(GoWriter writer, MemberShape member, Shape writer.write("if $2L != nil { s.WriteStruct($1L)\n$2L.SerializeMembers(s)\ns.CloseStruct() }", schemaName, ident); case DOCUMENT -> { writer.addUseImports(SmithyGoDependency.SMITHY_DOCUMENT); - writer.write("s.WriteDocument($L, &smithydocument.Opaque{Value: $L})", schemaName, ident); + writer.write("if $2L != nil { s.WriteDocument($1L, &smithydocument.Opaque{Value: $2L}) }", + schemaName, ident); } // FUTURE(602) diff --git a/schema.go b/schema.go index 6293d34b1..8e9c209a5 100644 --- a/schema.go +++ b/schema.go @@ -71,8 +71,9 @@ type Schema struct { directMask uint64 // bitmask: bit i set means indexed[i] was declared directly on this schema targetID ShapeID // for member schemas, the target's shape ID - listMember *Schema - mapKey, mapValue *Schema + // resolved on the fly and cached + listMember atomic.Pointer[Schema] + mapKey, mapValue atomic.Pointer[Schema] ext [numExtensionSlots]unsafe.Pointer // lazily-computed codec extensions, accessed atomically } @@ -126,9 +127,6 @@ func (s *Schema) AddMember(name string, target *Schema, ts ...Trait) *Schema { traits: cloneTraits(target.traits), directMask: 0, // inherited traits are not direct targetID: target.id, - listMember: target.listMember, - mapKey: target.mapKey, - mapValue: target.mapValue, } // member-declared traits override and are direct @@ -143,14 +141,6 @@ func (s *Schema) AddMember(name string, target *Schema, ts ...Trait) *Schema { atomic.StorePointer(&s.ext[i], nil) } - switch name { - case "member": - s.listMember = m - case "key": - s.mapKey = m - case "value": - s.mapValue = m - } return m } @@ -176,17 +166,31 @@ func cloneTraits(src map[ShapeID]Trait) map[ShapeID]Trait { // ListMember returns the "member" schema for list types. func (s *Schema) ListMember() *Schema { - return s.listMember + return s.lookup(&s.listMember, "member") } // MapKey returns the "key" schema for map types. func (s *Schema) MapKey() *Schema { - return s.mapKey + return s.lookup(&s.mapKey, "key") } // MapValue returns the "value" schema for map types. func (s *Schema) MapValue() *Schema { - return s.mapValue + return s.lookup(&s.mapValue, "value") +} + +func (s *Schema) lookup(cached *atomic.Pointer[Schema], name string) *Schema { + if v := cached.Load(); v != nil { + return v + } + + m, ok := s.members[name] + if !ok { + return nil + } + + cached.Store(m) + return m } // MemberName returns the member component of the schema's shape ID. diff --git a/testing/struct.go b/testing/struct.go index 9aae8fe83..a8950b4fc 100644 --- a/testing/struct.go +++ b/testing/struct.go @@ -52,6 +52,22 @@ func deepEqual(expect, actual reflect.Value, path string) error { } switch expect.Kind() { + case reflect.Interface: + // union, document + expect = deref(expect) + actual = deref(actual) + ek, ak := expect.Kind(), actual.Kind() + if ek == reflect.Invalid || ak == reflect.Invalid { + // one side was a nil interface, so they both must be nil + if ek == ak { + return nil + } + return fmt.Errorf("%s: %s != %s", path, fmtNil(ek), fmtNil(ak)) + } + if expect.Type() != actual.Type() { + return fmt.Errorf("%s: type mismatch: %s != %s", path, expect.Type(), actual.Type()) + } + return deepEqual(expect, actual, path) case reflect.Pointer: if expect.Type() != actual.Type() { return fmt.Errorf("%s: type mismatch", path) From f5cc5f12eda25ee487c6df772f25b703542332ab Mon Sep 17 00:00:00 2001 From: Luc Talatinian Date: Thu, 6 Aug 2026 11:45:31 -0400 Subject: [PATCH 2/2] changelog --- .changelog/2e107c2caba94fc7a7ea87a9e259466d.json | 8 ++++++++ .changelog/95eb090821094264bc1c5a718b3dc64a.json | 8 ++++++++ 2 files changed, 16 insertions(+) create mode 100644 .changelog/2e107c2caba94fc7a7ea87a9e259466d.json create mode 100644 .changelog/95eb090821094264bc1c5a718b3dc64a.json diff --git a/.changelog/2e107c2caba94fc7a7ea87a9e259466d.json b/.changelog/2e107c2caba94fc7a7ea87a9e259466d.json new file mode 100644 index 000000000..2783b2fc3 --- /dev/null +++ b/.changelog/2e107c2caba94fc7a7ea87a9e259466d.json @@ -0,0 +1,8 @@ +{ + "id": "2e107c2c-aba9-4fc7-a7ea-87a9e259466d", + "type": "bugfix", + "description": "Don't serialize unset JSON documents as `nil` in structure members.", + "modules": [ + "." + ] +} \ No newline at end of file diff --git a/.changelog/95eb090821094264bc1c5a718b3dc64a.json b/.changelog/95eb090821094264bc1c5a718b3dc64a.json new file mode 100644 index 000000000..0c5842f4f --- /dev/null +++ b/.changelog/95eb090821094264bc1c5a718b3dc64a.json @@ -0,0 +1,8 @@ +{ + "id": "95eb0908-2109-4264-bc1c-5a718b3dc64a", + "type": "bugfix", + "description": "Fix a deserialization panic around collection members in recursive shape configs.", + "modules": [ + "." + ] +} \ No newline at end of file