Skip to content

Commit

Permalink
Apply streaming trait directly to shapes
Browse files Browse the repository at this point in the history
This updates the streaming trait to apply directly to shapes instead of
members. This was previously restricted to members because it made the
model easier to validate and grok at a glance. However, keeping it on
members introduces potential errors in code generation. A given blob
could have both the streaming trait and the media type trait, for
instance. Both of these traits could result in a new type being created,
but since the streaming trait is applied to a member that type is
implicit. The code generator would have to try to generate a name that
doesn't conflict with any existing names, which is very error prone and
likely would result in unfortunate names. Moving this trait to the
shape directly makes the naming concern an explicit part of modeling,
where it will only have to be handled once.

This also brings the trait in line with the general best practice of
having traits that impact the type generated apply directly to the
shapes they impact.
  • Loading branch information
JordonPhillips committed Apr 2, 2020
1 parent 5e2486a commit 32403cd
Show file tree
Hide file tree
Showing 7 changed files with 128 additions and 22 deletions.
15 changes: 9 additions & 6 deletions docs/source/spec/core.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3353,10 +3353,7 @@ Summary
not be stored in memory, or that the size of the data stored in the shape
is unknown at the start of a request.
Trait selector::
``operation -[input, output]-> structure > :test(member > blob)``

This trait may only be applied to top level members of operation inputs and
outputs.
``blob``
Value type
``structure``

Expand All @@ -3377,6 +3374,10 @@ optional members:
In an HTTP-based protocol, for instance, this indicates that the
``content-length`` header must be set.

Shapes targeted by this trait MAY NOT be used outside of top level operation
inputs and operation outputs. Additionally, only one member of a structure may
target a shape with this trait.

.. tabs::

.. code-tab:: smithy
Expand All @@ -3386,10 +3387,12 @@ optional members:
}

structure StreamingOutputWrapper {
@streaming
output: Blob,
output: StreamingBlob,
}

@streaming
blob StreamingBlob

Constraint traits
=================

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package software.amazon.smithy.model.validation.validators;

import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.OperationShape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.traits.StreamingTrait;
import software.amazon.smithy.model.validation.AbstractValidator;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.utils.OptionalUtils;
import software.amazon.smithy.utils.SetUtils;

public class StreamingTraitValidator extends AbstractValidator {
@Override
public List<ValidationEvent> validate(Model model) {
Set<MemberShape> streamingMembers = model.shapes(MemberShape.class)
.filter(member -> member.getMemberTrait(model, StreamingTrait.class).isPresent())
.collect(Collectors.toSet());

List<ValidationEvent> events = validateShapesOnlyUsedAtTopLevel(model, streamingMembers);
events.addAll(validateOnlyOneStreamPerStructure(model, streamingMembers));
return events;
}

private List<ValidationEvent> validateShapesOnlyUsedAtTopLevel(Model model, Set<MemberShape> streamingMembers) {
Set<ShapeId> topLevelIoShapes = model.shapes(OperationShape.class)
.flatMap(operation -> SetUtils.of(operation.getInput(), operation.getOutput()).stream())
.flatMap(OptionalUtils::stream)
.collect(Collectors.toSet());

return streamingMembers.stream()
.filter(member -> !topLevelIoShapes.contains(member.getContainer()))
.map(member -> error(member, String.format(
"The shape %s has the smithy.api#streaming trait, and so may only be targeted by "
+ "top-level operation inputs and outputs.",
member.getTarget())))
.collect(Collectors.toList());
}

private List<ValidationEvent> validateOnlyOneStreamPerStructure(Model model, Set<MemberShape> streamingMembers) {
return streamingMembers.stream()
.collect(Collectors.groupingBy(MemberShape::getContainer)).entrySet().stream()
.filter(entry -> entry.getValue().size() > 1)
.map(entry -> {
String streamingTargets = entry.getValue().stream()
.sorted()
.map(member -> member.toShapeId().toString())
.collect(Collectors.joining(", "));
return error(model.expectShape(entry.getKey()), String.format(
"Structures may only target one shape with the smithy.api#streaming trait, but found [%s]",
streamingTargets));
})
.collect(Collectors.toList());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ software.amazon.smithy.model.validation.validators.ShapeIdConflictValidator
software.amazon.smithy.model.validation.validators.ShapeRecursionValidator
software.amazon.smithy.model.validation.validators.SingleOperationBindingValidator
software.amazon.smithy.model.validation.validators.SingleResourceBindingValidator
software.amazon.smithy.model.validation.validators.StreamingTraitValidator
software.amazon.smithy.model.validation.validators.TargetValidator
software.amazon.smithy.model.validation.validators.TraitConflictValidator
software.amazon.smithy.model.validation.validators.TraitTargetValidator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,7 @@ string since
/// Indicates that the the data stored in the shape is very large and should not
/// be stored in memory, or that the size of the data stored in the shape is
/// unknown at the start of a request
@trait(selector: "operation -[input, output]-> structure > :test(member > blob)",
structurallyExclusive: true)
@trait(selector: "blob")
@tags(["diff.error.const"])
structure streaming {
/// Indicates that the stream must have a known size.
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
[ERROR] ns.foo#InvalidStreamingStructure$Body: Trait `streaming` cannot be applied to `ns.foo#InvalidStreamingStructure$Body`. This trait may only be applied to shapes that match the following selector: operation -[input, output]-> structure > :test(member > blob) | TraitTarget
[ERROR] ns.foo#InvalidStreamingStructure$Body: The shape ns.foo#StreamingBlob has the smithy.api#streaming trait, and so may only be targeted by top-level operation inputs and outputs. | StreamingTrait
[ERROR] ns.foo#InvalidStreamingStructure2$Body: The shape ns.foo#StreamingBlob2 has the smithy.api#streaming trait, and so may only be targeted by top-level operation inputs and outputs. | StreamingTrait
[ERROR] ns.foo#InvalidStreamingOutput: Structures may only target one shape with the smithy.api#streaming trait, but found [ns.foo#InvalidStreamingOutput$StreamingBlob1, ns.foo#InvalidStreamingOutput$StreamingBlob2] | StreamingTrait
Original file line number Diff line number Diff line change
Expand Up @@ -17,32 +17,60 @@
"type": "structure",
"members": {
"Body": {
"target": "smithy.api#Blob",
"traits": {
"smithy.api#streaming": true
}
"target": "ns.foo#StreamingBlob"
}
}
},
"ns.foo#StreamingOutput": {
"type": "structure",
"members": {
"Body": {
"target": "smithy.api#Blob",
"traits": {
"smithy.api#streaming": true
}
"target": "ns.foo#StreamingBlob"
}
}
},
"ns.foo#InvalidStreamingStructure": {
"type": "structure",
"members": {
"Body": {
"target": "smithy.api#Blob",
"traits": {
"smithy.api#streaming": true
}
"target": "ns.foo#StreamingBlob"
}
}
},
"ns.foo#StreamingBlob": {
"type": "blob",
"traits": {
"smithy.api#streaming": true
}
},
"ns.foo#InvalidStreamingStructure2": {
"type": "structure",
"members": {
"Body": {
"target": "ns.foo#StreamingBlob2"
}
}
},
"ns.foo#StreamingBlob2": {
"type": "blob",
"traits": {
"smithy.api#streaming": true
}
},
"ns.foo#InvalidStreamingOperation": {
"type": "operation",
"output": {
"target": "ns.foo#InvalidStreamingOutput"
}
},
"ns.foo#InvalidStreamingOutput": {
"type": "structure",
"members": {
"StreamingBlob1": {
"target": "ns.foo#StreamingBlob"
},
"StreamingBlob2": {
"target": "ns.foo#StreamingBlob2"
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ operation StreamingOperation {

structure Output {
@httpPayload
@streaming
body: StreamingPayload,
}

@streaming
blob StreamingPayload

0 comments on commit 32403cd

Please sign in to comment.