Skip to content

Commit

Permalink
codegen: Update generated serde to skip unmodeled input/outputs
Browse files Browse the repository at this point in the history 
Updates the SDK's generated serializers and deserializers to skip unmodeled
input and output shapes for operations.

This issue caused generated deserializers to attempt to deserialize
operation responses that were not modeled, resulting in an error.
Whereas the SDK should of ignored the response if one was present for
operations without a modeled output.

Related to aws/aws-sdk-go-v2#1047
  • Loading branch information
@jasdel
jasdel committed Jan 15, 2021
1 parent 655384d commit 18255cc
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 38 deletions.
1 change: 1 addition & 0 deletions ...smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/AddOperationShapes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public static Model execute(Model model, ShapeId serviceShapeId) {
private static StructureShape emptyOperationStructure(ShapeId opShapeId, String suffix) {
return StructureShape.builder()
.id(ShapeId.fromParts(CodegenUtils.getSyntheticTypeNamespace(), opShapeId.getName() + suffix))
.addTrait(SyntheticClone.builder().build())
.build();
}

Expand Down
17 changes: 17 additions & 0 deletions codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/CodegenUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.Optional;
import java.util.function.Predicate;
import java.util.logging.Logger;
import software.amazon.smithy.codegen.core.CodegenException;
Expand Down Expand Up @@ -145,6 +146,22 @@ public static String getSyntheticTypeNamespace() {
return CodegenUtils.SYNTHETIC_NAMESPACE;
}

/**
* Get if the passed in shape is decorated as a synthetic clone, but there is no other shape the clone is
* created from.
*
* @param shape the shape to check if its a stubbed synthetic clone without an archetype.
* @return if the shape is synthetic clone, but not based on a specific shape.
*/
public static boolean isStubSyntheticClone(Shape shape) {
Optional<SyntheticClone> optional = shape.getTrait(SyntheticClone.class);
if (!optional.isPresent()) {
return false;
}

SyntheticClone synthClone = optional.get();
return synthClone.getArchetype() == null;
}

/**
* Returns the operand decorated with an &amp; if the address of the shape type can be taken.
Expand Down
71 changes: 45 additions & 26 deletions ...main/java/software/amazon/smithy/go/codegen/integration/HttpBindingProtocolGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,11 @@ public void generateRequestSerializers(GenerationContext context) {
private void generateOperationSerializer(GenerationContext context, OperationShape operation) {
generateOperationSerializerMiddleware(context, operation);
generateOperationHttpBindingSerializer(context, operation);
generateOperationDocumentSerializer(context, operation);
addOperationDocumentShapeBindersForSerializer(context, operation);

if (!CodegenUtils.isStubSyntheticClone(ProtocolUtils.expectInput(context.getModel(), operation))) {
generateOperationDocumentSerializer(context, operation);
addOperationDocumentShapeBindersForSerializer(context, operation);
}
}

/**
Expand Down Expand Up @@ -240,32 +243,36 @@ private void generateOperationSerializerMiddleware(GenerationContext context, Op
writer.write("");
}

// document bindings vs payload bindings
HttpBindingIndex httpBindingIndex = model.getKnowledge(HttpBindingIndex.class);
boolean hasDocumentBindings = !httpBindingIndex
.getRequestBindings(operation, HttpBinding.Location.DOCUMENT)
.isEmpty();
Optional<HttpBinding> payloadBinding = httpBindingIndex.getRequestBindings(operation,
HttpBinding.Location.PAYLOAD).stream().findFirst();


if (hasDocumentBindings) {
// delegate the setup and usage of the document serializer function for the protocol
writeMiddlewareDocumentSerializerDelegator(context, operation, generator);

} else if (payloadBinding.isPresent()) {
// delegate the setup and usage of the payload serializer function for the protocol
MemberShape memberShape = payloadBinding.get().getMember();
writeMiddlewarePayloadSerializerDelegator(context, memberShape);
// Don't consider serializing the body if the input shape is a stubbed synthetic clone, without an
// archetype.
if (!CodegenUtils.isStubSyntheticClone(ProtocolUtils.expectInput(model, operation))) {
// document bindings vs payload bindings
HttpBindingIndex httpBindingIndex = model.getKnowledge(HttpBindingIndex.class);
boolean hasDocumentBindings = !httpBindingIndex
.getRequestBindings(operation, HttpBinding.Location.DOCUMENT)
.isEmpty();
Optional<HttpBinding> payloadBinding = httpBindingIndex.getRequestBindings(operation,
HttpBinding.Location.PAYLOAD).stream().findFirst();

if (hasDocumentBindings) {
// delegate the setup and usage of the document serializer function for the protocol
writeMiddlewareDocumentSerializerDelegator(context, operation, generator);

} else if (payloadBinding.isPresent()) {
// delegate the setup and usage of the payload serializer function for the protocol
MemberShape memberShape = payloadBinding.get().getMember();
writeMiddlewarePayloadSerializerDelegator(context, memberShape);
}
writer.write("");
}

writer.write("");
// Serialize HTTP request with payload, if set.
writer.openBlock("if request.Request, err = restEncoder.Encode(request.Request); err != nil {", "}", () -> {
writer.write("return out, metadata, &smithy.SerializationError{Err: err}");
});
// Ensure the request value is updated.
writer.write("in.Request = request");
writer.write("");

writer.write("return next.$L(ctx, in)", generator.getHandleMethodName());
});
}
Expand Down Expand Up @@ -331,12 +338,21 @@ private void generateOperationDeserializerMiddleware(GenerationContext context,
writer.write("");
}

// Output Shape Document Binding middleware generation
if (isShapeWithResponseBindings(model, operation, HttpBinding.Location.DOCUMENT)
// Discard without deserializing the response if the input shape is a stubbed synthetic clone
// without an archetype.
if (CodegenUtils.isStubSyntheticClone(ProtocolUtils.expectOutput(model, operation))) {
writer.addUseImports(SmithyGoDependency.IOUTIL);
writer.openBlock("if _, err = io.Copy(ioutil.Discard, response.Body); err != nil {", "}", () -> {
writer.openBlock("return out, metadata, &smithy.DeserializationError{", "}", () -> {
writer.write("Err: fmt.Errorf(\"failed to discard response body, %w\", err),");
});
});
} else if (isShapeWithResponseBindings(model, operation, HttpBinding.Location.DOCUMENT)
|| isShapeWithResponseBindings(model, operation, HttpBinding.Location.PAYLOAD)) {
// Output Shape Document Binding middleware generation
writeMiddlewareDocumentDeserializerDelegator(context, operation, generator);
writer.write("");
}
writer.write("");

writer.write("return out, metadata, err");
});
Expand Down Expand Up @@ -800,8 +816,11 @@ public void generateResponseDeserializers(GenerationContext context) {
for (OperationShape operation : getHttpBindingOperations(context)) {
generateOperationDeserializerMiddleware(context, operation);
generateHttpBindingDeserializer(context, operation);
generateOperationDocumentDeserializer(context, operation);
addOperationDocumentShapeBindersForDeserializer(context, operation);

if (!CodegenUtils.isStubSyntheticClone(ProtocolUtils.expectOutput(context.getModel(), operation))) {
generateOperationDocumentDeserializer(context, operation);
addOperationDocumentShapeBindersForDeserializer(context, operation);
}
}

for (StructureShape error : deserializingErrorShapes) {
Expand Down
35 changes: 28 additions & 7 deletions ...src/main/java/software/amazon/smithy/go/codegen/integration/HttpRpcProtocolGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import software.amazon.smithy.codegen.core.Symbol;
import software.amazon.smithy.codegen.core.SymbolProvider;
import software.amazon.smithy.go.codegen.ApplicationProtocol;
import software.amazon.smithy.go.codegen.CodegenUtils;
import software.amazon.smithy.go.codegen.GoStackStepMiddlewareGenerator;
import software.amazon.smithy.go.codegen.GoWriter;
import software.amazon.smithy.go.codegen.SmithyGoDependency;
Expand All @@ -38,7 +39,8 @@ public abstract class HttpRpcProtocolGenerator implements ProtocolGenerator {
/**
* Creates an Http RPC protocol generator.
*/
public HttpRpcProtocolGenerator() { }
public HttpRpcProtocolGenerator() {
}

@Override
public ApplicationProtocol getApplicationProtocol() {
Expand Down Expand Up @@ -111,6 +113,7 @@ private void generateOperationSerializer(GenerationContext context, OperationSha

// Cast the input parameters to the operation request type and check for errors.
writer.write("input, ok := in.Parameters.($P)", inputSymbol);
writer.write("_ = input");
writer.openBlock("if !ok {", "}", () -> {
writer.write("return out, metadata, "
+ "&smithy.SerializationError{Err: fmt.Errorf(\"unknown input parameters type %T\","
Expand All @@ -129,13 +132,18 @@ private void generateOperationSerializer(GenerationContext context, OperationSha

// delegate the setup and usage of the document serializer function for the protocol
serializeInputDocument(context, operation);
serializingDocumentShapes.add(ProtocolUtils.expectInput(model, operation));
// Skipping calling serializer method for the input shape is responsibility of the
// serializeInputDocument implementation.
if (!CodegenUtils.isStubSyntheticClone(ProtocolUtils.expectInput(context.getModel(), operation))) {
serializingDocumentShapes.add(ProtocolUtils.expectInput(model, operation));
}

writer.write("");

writer.openBlock("if request.Request, err = httpBindingEncoder.Encode(request.Request); err != nil {",
"}", () -> {
writer.write("return out, metadata, &smithy.SerializationError{Err: err}");
});
writer.write("return out, metadata, &smithy.SerializationError{Err: err}");
});
// Ensure the request value is updated if modified for a document.
writer.write("in.Request = request");

Expand Down Expand Up @@ -164,7 +172,8 @@ private void writeRequestHeaders(GenerationContext context, OperationShape opera
* @param operation The operation being generated.
* @param writer The writer to use.
*/
protected void writeDefaultHeaders(GenerationContext context, OperationShape operation, GoWriter writer) {}
protected void writeDefaultHeaders(GenerationContext context, OperationShape operation, GoWriter writer) {
}

/**
* Provides the request path for the operation.
Expand Down Expand Up @@ -256,8 +265,20 @@ private void generateOperationDeserializer(GenerationContext context, OperationS
writer.write("out.Result = output");
writer.write("");

deserializeOutputDocument(context, operation);
deserializingDocumentShapes.add(ProtocolUtils.expectOutput(model, operation));
// Discard without deserializing the response if the input shape is a stubbed synthetic clone
// without an archetype.
if (CodegenUtils.isStubSyntheticClone(ProtocolUtils.expectOutput(model, operation))) {
writer.addUseImports(SmithyGoDependency.IOUTIL);
writer.openBlock("if _, err = io.Copy(ioutil.Discard, response.Body); err != nil {", "}",
() -> {
writer.openBlock("return out, metadata, &smithy.DeserializationError{", "}", () -> {
writer.write("Err: fmt.Errorf(\"failed to discard response body, %w\", err),");
});
});
} else {
deserializeOutputDocument(context, operation);
deserializingDocumentShapes.add(ProtocolUtils.expectOutput(model, operation));
}
writer.write("");

writer.write("return out, metadata, err");
Expand Down
33 changes: 28 additions & 5 deletions ...hy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/AddOperationShapesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,14 @@ public void testOperationWithoutInputOutput() {
shape.isPresent(), Matchers.is(true));
MatcherAssert.assertThat(shapeId + " shape must have no members",
shape.get().members().size(), Matchers.equalTo(0));
if (shape.get().getTrait(SyntheticClone.class).isPresent()) {
MatcherAssert.assertThat("shape is not synthetic clone", false);

Optional<SyntheticClone> opSynthClone = shape.get().getTrait(SyntheticClone.class);
if (opSynthClone.isPresent()) {
SyntheticClone synthClone = opSynthClone.get();
MatcherAssert.assertThat(shapeId + " shape must not have archetype",
synthClone.getArchetype(), Matchers.equalTo(null));
} else {
MatcherAssert.assertThat(shapeId + " shape must be synthetic clone", false);
}
});
}
Expand Down Expand Up @@ -118,12 +124,13 @@ public void testOperationWithExistingInputOutput() {
MatcherAssert.assertThat("foo member present", fooMember.isPresent(), Matchers.is(true));

ShapeId id = fooMember.get().getId();
MatcherAssert.assertThat("foo is correct namespace", id.getNamespace(), Matchers.equalTo(shapeId.getNamespace()));
MatcherAssert.assertThat("foo is correct namespace", id.getNamespace(),
Matchers.equalTo(shapeId.getNamespace()));
MatcherAssert.assertThat("foo is correct parent", id.getName(), Matchers.equalTo(shapeId.getName()));

Optional<SyntheticClone> syntheticClone = shape.get().getTrait(SyntheticClone.class);
if (!syntheticClone.isPresent()) {
MatcherAssert.assertThat("shape must be marked as synthetic clone", false);
MatcherAssert.assertThat(shapeId + " shape must be marked as synthetic clone", false);
} else {
MatcherAssert.assertThat(syntheticClone.get().getArchetype().toString(),
Matchers.is(Matchers.oneOf(NAMESPACE + "#TestOperationRequest",
Expand Down Expand Up @@ -161,7 +168,7 @@ public void testOperationWithExistingInputOutputWithConflitcs() {
ShapeId expInputRename = ShapeId.fromParts(CodegenUtils.getSyntheticTypeNamespace(), "TestOperationInput");
ShapeId expOutputRename = ShapeId.fromParts(CodegenUtils.getSyntheticTypeNamespace(), "TestOperationOutput");

ListUtils.of(expInputRename, expOutputRename, inputConflict.getId(), outputConflict.getId())
ListUtils.of(inputConflict.getId(), outputConflict.getId())
.forEach(shapeId -> {
Optional<Shape> shape = processedModel.getShape(shapeId);
MatcherAssert.assertThat(shapeId + " shape must be present in model",
Expand All @@ -171,5 +178,21 @@ public void testOperationWithExistingInputOutputWithConflitcs() {
MatcherAssert.assertThat("shape must not be marked as synthetic clone", false);
}
});

ListUtils.of(expInputRename, expOutputRename)
.forEach(shapeId -> {
Optional<Shape> shape = processedModel.getShape(shapeId);
MatcherAssert.assertThat(shapeId + " shape must be present in model",
shape.isPresent(), Matchers.is(true));

Optional<SyntheticClone> opSynthClone = shape.get().getTrait(SyntheticClone.class);
if (opSynthClone.isPresent()) {
SyntheticClone synthClone = opSynthClone.get();
MatcherAssert.assertThat(shapeId + " shape must not have archetype",
synthClone.getArchetype(), Matchers.equalTo(null));
} else {
MatcherAssert.assertThat(shapeId + " shape must be synthetic clone", false);
}
});
}
}

0 comments on commit 18255cc

Please sign in to comment.