Skip to content

Commit

Permalink
Forbid using requiresLength with output
Browse files Browse the repository at this point in the history
This commit does not allow using the requiresLength trait with streaming
blobs used in output shapes. We can lift this restriction in the future
if a use case ever arises, but by forbidding this use case now,
implementations are free to treat blobs marked with the requiresLength
trait as a specialization of a stream that affects the generated type.
  • Loading branch information
mtdowling authored and kstich committed Mar 24, 2022
1 parent 30300f1 commit 6ef1531
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 58 deletions.
7 changes: 3 additions & 4 deletions docs/source/1.0/spec/core/stream-traits.rst
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,15 @@ Summary
payload of a request. This can be useful for services that need to
determine if a request will be accepted based on its size or where to
store data based on the size of the stream.

.. note::

This trait only has an effect when used on blobs in input shapes.
Trait selector::
``blob[trait|streaming]``

*A blob shape marked with the streaming trait*
Value type
``structure``
Validation
* ``requiresLength`` shapes can only be referenced from top-level members
of operation input structures.

.. tabs::

Expand Down
42 changes: 3 additions & 39 deletions smithy-aws-protocol-tests/model/restJson1/streaming.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,7 @@ blob StreamingBlob
/// not a structure or a union type.
@http(uri: "/StreamingTraitsRequireLength", method: "POST")
operation StreamingTraitsRequireLength {
input: StreamingTraitsRequireLengthInputOutput,
output: StreamingTraitsRequireLengthInputOutput
input: StreamingTraitsRequireLengthInput
}

apply StreamingTraitsRequireLength @httpRequestTests([
Expand Down Expand Up @@ -147,43 +146,8 @@ apply StreamingTraitsRequireLength @httpRequestTests([
},
])

apply StreamingTraitsRequireLength @httpResponseTests([
{
id: "RestJsonStreamingTraitsRequireLengthWithBlob",
documentation: "Serializes a blob in the HTTP payload with a required length",
protocol: restJson1,
code: 200,
body: "blobby blob blob",
bodyMediaType: "application/octet-stream",
headers: {
"X-Foo": "Foo",
"Content-Type": "application/octet-stream"
},
requireHeaders: [
"Content-Length"
],
params: {
foo: "Foo",
blob: "blobby blob blob"
}
},
{
id: "RestJsonStreamingTraitsRequireLengthWithNoBlobBody",
documentation: "Serializes an empty blob in the HTTP payload",
protocol: restJson1,
code: 200,
body: "",
bodyMediaType: "application/octet-stream",
headers: {
"X-Foo": "Foo"
},
params: {
foo: "Foo"
}
}
])

structure StreamingTraitsRequireLengthInputOutput {
@input
structure StreamingTraitsRequireLengthInput {
@httpHeader("X-Foo")
foo: String,

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.stream.Collectors;
import software.amazon.smithy.model.Model;
Expand All @@ -26,7 +27,6 @@
import software.amazon.smithy.model.neighbor.NeighborProvider;
import software.amazon.smithy.model.neighbor.Relationship;
import software.amazon.smithy.model.neighbor.RelationshipDirection;
import software.amazon.smithy.model.neighbor.RelationshipType;
import software.amazon.smithy.model.neighbor.Walker;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.ServiceShape;
Expand All @@ -35,6 +35,7 @@
import software.amazon.smithy.model.shapes.UnionShape;
import software.amazon.smithy.model.traits.HttpPayloadTrait;
import software.amazon.smithy.model.traits.ProtocolDefinitionTrait;
import software.amazon.smithy.model.traits.RequiresLengthTrait;
import software.amazon.smithy.model.traits.StreamingTrait;
import software.amazon.smithy.model.validation.AbstractValidator;
import software.amazon.smithy.model.validation.ValidationEvent;
Expand Down Expand Up @@ -96,27 +97,47 @@ private List<ValidationEvent> validateStreamingTargets(Model model) {
List<ValidationEvent> events = new ArrayList<>();
NeighborProvider provider = NeighborProviderIndex.of(model).getReverseProvider();

// Find any containers that reference a streaming trait.
Set<Shape> streamingStructures = model.shapes(MemberShape.class)
.filter(member -> member.getMemberTrait(model, StreamingTrait.class).isPresent())
.map(member -> model.expectShape(member.getContainer()))
.collect(Collectors.toSet());

for (Shape shape : streamingStructures) {
for (Relationship rel : provider.getNeighbors(shape)) {
if (rel.getRelationshipType() != RelationshipType.INPUT
&& rel.getRelationshipType() != RelationshipType.OUTPUT
&& rel.getRelationshipType().getDirection() == RelationshipDirection.DIRECTED) {
events.add(error(rel.getShape(), String.format(
"This shape has an invalid `%s` relationship to a structure, `%s`, that contains "
+ "a stream", rel.getRelationshipType(), shape.getId())));
for (MemberShape member : model.getMemberShapes()) {
Shape target = model.expectShape(member.getTarget());
if (target.hasTrait(StreamingTrait.class)) {
Shape container = model.expectShape(member.getContainer());
for (Relationship rel : provider.getNeighbors(container)) {
validateStreamingTargetRel(container, target, rel, events);
}
}
}

return events;
}

private void validateStreamingTargetRel(
Shape container,
Shape target,
Relationship rel,
List<ValidationEvent> events
) {
if (rel.getRelationshipType().getDirection() == RelationshipDirection.DIRECTED) {
switch (rel.getRelationshipType()) {
case INPUT:
break;
case OUTPUT:
if (target.hasTrait(RequiresLengthTrait.class)) {
events.add(error(rel.getShape(), String.format(
"Structures that contain a reference to a stream marked with the "
+ "@requiresLength trait can only be used as operation inputs, but this "
+ "structure is referenced from `%s` as %s",
rel.getShape().getId(),
rel.getRelationshipType().toString().toLowerCase(Locale.ENGLISH))));
}
break;
default:
events.add(error(rel.getShape(), String.format(
"This shape has an invalid `%s` relationship to a structure, `%s`, that contains "
+ "a stream", rel.getRelationshipType(), container.getId())));
}
}
}

private List<ValidationEvent> validateAllEventStreamMembers(Model model) {
return model.shapes(UnionShape.class)
.filter(shape -> shape.hasTrait(StreamingTrait.class))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[ERROR] ns.foo#InvalidStreamingOperation: Structures that contain a reference to a stream marked with the @requiresLength trait can only be used as operation inputs, but this structure is referenced from `ns.foo#InvalidStreamingOperation` as output | StreamingTrait
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{
"smithy": "1.0",
"shapes": {
"ns.foo#Blob": {
"type": "blob"
},
"ns.foo#InvalidStreamingOperation": {
"type": "operation",
"input": {
"target": "ns.foo#InvalidStreamingOperationInput"
},
"output": {
"target": "ns.foo#InvalidStreamingOperationOutput"
}
},
"ns.foo#InvalidStreamingOperationInput": {
"type": "structure",
"members": {
"Body": {
"target": "ns.foo#StreamingBlob"
}
},
"traits": {
"smithy.api#input": {}
}
},
"ns.foo#InvalidStreamingOperationOutput": {
"type": "structure",
"members": {
"Body": {
"target": "ns.foo#StreamingBlob"
}
},
"traits": {
"smithy.api#output": {}
}
},
"ns.foo#StreamingBlob": {
"type": "blob",
"traits": {
"smithy.api#streaming": {},
"smithy.api#requiresLength": {}
}
}
}
}

0 comments on commit 6ef1531

Please sign in to comment.