Smithy Document Shape Support #310

skmcgrail · 2021-06-28T18:19:38Z

Adds support for Smithy Document shapes and supporting types for protocols to implement support.

AWS Protocol Support: aws/aws-sdk-go-v2#1320

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java

...n/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ShapeValueGenerator.java

document/document.go

...n/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ShapeValueGenerator.java

document/json/encoder.go

document/json/encoder_test.go

document/json/json.go

document/json/decoder_test.go

...en/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/StructureGenerator.java

mtdowling · 2021-06-30T23:20:36Z

...n/src/main/java/software/amazon/smithy/go/codegen/integration/DocumentShapeDeserVisitor.java

@@ -411,7 +411,7 @@ protected final void generateDeserFunction(
            BiConsumer<GenerationContext, Shape> functionBody
    ) {
        SymbolProvider symbolProvider = context.getSymbolProvider();
-        GoWriter writer = context.getWriter();
+        GoWriter writer = context.getWriter().get();


I see this a lot. Did this method get changed to be Optional? Why?

The ProtocolGenerator context is kind of a bit of a grab bag currently, in some instances a writer is to be expected, in other instances the delegator that is stored on the context is used and no writer is expected. This was just to make that contact a bit clearer that it may not be there.

This was just something that I noticed when I was refactoring context to make it immutable after construction.

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/ProtocolUtils.java

mtdowling · 2021-06-30T23:24:17Z

document/document.go

+	"strconv"
+)
+
+type SmithyDocumentMarshaler interface {


Do these kinds of interfaces generally have godocs?

Yes, next revision will include documentation, just wanted to get this out for initial feedback on the overall implementation.

mtdowling · 2021-06-30T23:27:25Z

document/internal/serde/serde.go

+	return v.Interface()
+}
+
+func IsZeroValue(v reflect.Value) bool {


Is this here to prevent zero values from being serialized? If so, I don't think that's the correct behavior for document types. They're meant to carry whatever input they were given, so I expect if someone populates a 0 or [] or whatever, then it's serialized.

This is here to detect if a value is a zero-value, this doesn't prevent serialization of the zero value unless the user instructed the document marshaler via a struct tag likedocument:",omitempty", where omitempty indicates that zero-values should not be serialized.

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java

mtdowling · 2021-07-01T17:40:56Z

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java

+/**
+ * Generates the Smithy document type for the service client.
+ */
+public final class ProtocolDocumentGenerator {


Does this class need to be public?

Yes, these exported constants are needed so that protocol implementation parts make symbol references to the types.

mtdowling · 2021-07-01T17:41:51Z

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java

+import software.amazon.smithy.model.shapes.DocumentShape;
+
+/**
+ * Generates the Smithy document type for the service client.


Can you give more info on what this generates? Do you generate a document type per type defined in the model? Do you generate a single document type used by every document type, and this mostly just generates the serde code specific to the protocol?

To add onto this, it would be generally nice to have example usage.

Will add some clarifying code examples sections to this class documentation. To summarize what is being created here: Each service client generates its own document type interface definition. This ensures that document types that are sent or received from the service are bound to the client package and it's associated client protocol. This prevents the situation where a user receives a set of protocol document bytes in service client A and tries to round-trip it by plugging it directly into service client B (which may be a different protocol and where the raw captured bytes can't be used). User who want to plug a document from service client A into client B would need to first unmarshal the document into a Go type, and then pass that Go type to the NewLazyDcoument constructor in the service client B package.

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java

codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/CodegenVisitor.java

JordonPhillips · 2021-07-05T13:50:17Z

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java

+import software.amazon.smithy.model.shapes.DocumentShape;
+
+/**
+ * Generates the Smithy document type for the service client.


To add onto this, it would be generally nice to have example usage.

document/document.go

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java

...n/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ShapeValueGenerator.java

document/document.go

document/json/encoder.go

document/json/decoder.go

skmcgrail · 2021-07-13T21:50:16Z

I believe all outstanding feedback has been capture in the Implement Feedback commit that I have force-pushed. Please review when you have the chance, where applicable I left some conversations unresolved to give you the opportunity to read the response, feel free to resolve if satisfied.

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java

document/document.go

document/internal/serde/field.go

skmcgrail · 2021-07-19T21:51:54Z

Rebased on main.

Adds support for Smithy Document Shape Types for JSON protocols. Depends on: aws/smithy-go#310

skmcgrail requested review from jasdel, JordonPhillips and mtdowling June 28, 2021 18:19

skmcgrail mentioned this pull request Jun 28, 2021

Smithy Document Shape Support aws/aws-sdk-go-v2#1320

Merged

skmcgrail requested a review from skotambkar June 29, 2021 15:55

jasdel reviewed Jun 30, 2021

View reviewed changes

mtdowling reviewed Jul 1, 2021

View reviewed changes

JordonPhillips requested changes Jul 5, 2021

View reviewed changes

skmcgrail force-pushed the documentTypes branch 2 times, most recently from 7fc9824 to 09269c5 Compare July 13, 2021 21:05

skmcgrail requested review from mtdowling, jasdel and JordonPhillips July 13, 2021 21:22

skmcgrail force-pushed the documentTypes branch 2 times, most recently from cb032bb to a87ef15 Compare July 13, 2021 21:49

jasdel approved these changes Jul 16, 2021

View reviewed changes

...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/ProtocolDocumentGenerator.java Show resolved Hide resolved

document/document.go Show resolved Hide resolved

document/internal/serde/field.go Show resolved Hide resolved

JordonPhillips approved these changes Jul 19, 2021

View reviewed changes

skmcgrail added 2 commits July 19, 2021 10:53

Smithy Document Shape Support

2b63427

Implement Feedback

4970553

skmcgrail force-pushed the documentTypes branch from a87ef15 to 4970553 Compare July 19, 2021 17:54

skmcgrail requested a review from kstich July 19, 2021 23:00

Implement kstich feedback

f19a016

jasdel merged commit d3a155d into main Jul 29, 2021

jasdel deleted the documentTypes branch July 29, 2021 22:24

jasdel pushed a commit to aws/aws-sdk-go-v2 that referenced this pull request Jul 29, 2021

Add Smithy Document Shape Support (#1320)

17a3908

Adds support for Smithy Document Shape Types for JSON protocols. Depends on: aws/smithy-go#310

jrichard8 pushed a commit to jrichard8/aws-sdk-go-v2 that referenced this pull request Feb 14, 2022

Add Smithy Document Shape Support (aws#1320)

d6a5651

Adds support for Smithy Document Shape Types for JSON protocols. Depends on: aws/smithy-go#310

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smithy Document Shape Support #310

Smithy Document Shape Support #310

skmcgrail commented Jun 28, 2021 •

edited

Loading

mtdowling Jun 30, 2021

skmcgrail Jul 8, 2021

mtdowling Jun 30, 2021

skmcgrail Jul 8, 2021

mtdowling Jun 30, 2021

skmcgrail Jul 8, 2021

mtdowling Jul 1, 2021

skmcgrail Jul 8, 2021

mtdowling Jul 1, 2021

JordonPhillips Jul 5, 2021

skmcgrail Jul 8, 2021 •

edited

Loading

JordonPhillips Jul 5, 2021

skmcgrail commented Jul 13, 2021

skmcgrail commented Jul 19, 2021

Smithy Document Shape Support #310

Smithy Document Shape Support #310

Conversation

skmcgrail commented Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skmcgrail Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skmcgrail commented Jul 13, 2021

skmcgrail commented Jul 19, 2021

skmcgrail commented Jun 28, 2021 •

edited

Loading

skmcgrail Jul 8, 2021 •

edited

Loading