Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Remove OperationMissingInput|Output
Browse files Browse the repository at this point in the history
These validators nudged modelers to define explicit input and output
references for every operation, but added a lot of noise and don't
achieve the core objective of defining a dedicated input and output
shape for every operation. Better validation could be added through a
linter that discourages the use of Unit for operations at all.
mtdowling committed Nov 19, 2021
1 parent cd5be36 commit 403ed0b
Showing 42 changed files with 1 addition and 178 deletions.
5 changes: 0 additions & 5 deletions designs/operation-input-output-and-unit-types.md
Original file line number Diff line number Diff line change
@@ -180,11 +180,6 @@ structure GetFooOutput {}
To encourage models to utilize the `@input` and `@output` traits, built-in
validation will be added to Smithy.

When an operation does not define input or output explicitly a WARNING
validation event will be emitted with the ID `OperationMissingInput` or
`OperationMissingOutput`. This can be avoided by explicitly assigning the input
or output of an operation to a shape, including `smithy.api#Unit`.

If an operation input targets a shape other than `smithy.api#Unit` that is
not marked with the `@input` trait, a WARNING validation event is emitted with
the ID `OperationMissingInputTrait`. A similar event with an ID of
Original file line number Diff line number Diff line change
@@ -12,7 +12,6 @@ use smithy.test#httpResponseTests
/// The example tests how requests and responses are serialized when there's
/// no request or response payload because the operation has no input or output.
/// While this should be rare, code generators must support this.
@suppress(["OperationMissingInput", "OperationMissingOutput"])
operation NoInputAndNoOutput {}

apply NoInputAndNoOutput @httpRequestTests([
@@ -114,7 +113,6 @@ apply NoInputAndNoOutput @httpResponseTests([
/// no request or response payload because the operation has no input and the
/// output is empty. While this should be rare, code generators must support
/// this.
@suppress(["OperationMissingInput"])
operation NoInputAndOutput {
output: NoInputAndOutputOutput
}
Original file line number Diff line number Diff line change
@@ -20,5 +20,4 @@ use smithy.test#httpRequestTests
appliesTo: "client"
}
])
@suppress(["OperationMissingInput", "OperationMissingOutput"])
operation HostWithPathOperation {}
2 changes: 0 additions & 2 deletions smithy-aws-protocol-tests/model/awsJson1_0/endpoints.smithy
Original file line number Diff line number Diff line change
@@ -23,7 +23,6 @@ use smithy.test#httpRequestTests
}
])
@endpoint(hostPrefix: "foo.")
@suppress(["OperationMissingInput", "OperationMissingOutput"])
operation EndpointOperation {}


@@ -47,7 +46,6 @@ operation EndpointOperation {}
}
])
@endpoint(hostPrefix: "foo.{label}.")
@suppress(["OperationMissingOutput"])
operation EndpointWithHostLabelOperation {
input: EndpointWithHostLabelOperationInput,
}
14 changes: 0 additions & 14 deletions smithy-aws-protocol-tests/model/shared-types.smithy
Original file line number Diff line number Diff line change
@@ -33,20 +33,6 @@ metadata suppressions = [
reason: """
This is a temporary solution until we rewrite these tests to define input, output,
and use the `@input` and `@output` traits."""
},
{
id: "OperationMissingInput",
namespace: "*",
reason: """
This is a temporary solution until we rewrite these tests to define input, output,
and use the `@input` and `@output` traits."""
},
{
id: "OperationMissingOutput",
namespace: "*",
reason: """
This is a temporary solution until we rewrite these tests to define input, output,
and use the `@input` and `@output` traits."""
}
]

Original file line number Diff line number Diff line change
@@ -1 +0,0 @@
[WARNING] ns.foo#PutObject: This operation does not define output | OperationMissingOutput
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
[WARNING] ns.foo#DescribeEndpoints: This operation does not define output | OperationMissingOutput
[ERROR] ns.foo#DescribeEndpoints: Endpoint discovery operation output must only contain an `Endpoints` member, but found: [] | ClientEndpointDiscovery
Original file line number Diff line number Diff line change
@@ -15,7 +15,7 @@ structure NoBehaviorInput {}
@httpChecksum(
requestChecksumRequired: true,
)
@suppress(["UnstableTrait", "OperationMissingInput"])
@suppress(["UnstableTrait"])
operation NoInput {
output: Unit
}
Original file line number Diff line number Diff line change
@@ -202,8 +202,6 @@
},
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"},
{"id": "OperationInputOutputName", "namespace": "ns.foo"}
],
"validators": [
Original file line number Diff line number Diff line change
@@ -106,8 +106,6 @@
},
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"},
{"id": "OperationMissingInputTrait", "namespace": "ns.foo"},
{"id": "OperationMissingOutputTrait", "namespace": "ns.foo"}
],
Original file line number Diff line number Diff line change
@@ -256,10 +256,6 @@
}
},
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"}
],
"validators": [
{
"name": "MissingPaginatedTrait"
Original file line number Diff line number Diff line change
@@ -42,10 +42,6 @@
}
},
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"}
],
"validators": [
{
"name": "StandardOperationVerb",
Original file line number Diff line number Diff line change
@@ -246,15 +246,6 @@ private void loadOperation(ShapeId id, ObjectNode node, FullyResolvedModelFile m
.id(id)
.source(node.getSourceLocation())
.addErrors(loadOptionalTargetList(modelFile, id, node, ERRORS));

if (!node.getMember("input").isPresent()) {
modelFile.events().add(LoaderUtils.createOperationMissingInput(id, node.getSourceLocation()));
}

if (!node.getMember("output").isPresent()) {
modelFile.events().add(LoaderUtils.createOperationMissingOutput(id, node.getSourceLocation()));
}

loadOptionalTarget(modelFile, id, node, "input").ifPresent(builder::input);
loadOptionalTarget(modelFile, id, node, "output").ifPresent(builder::output);
modelFile.onShape(builder);
Original file line number Diff line number Diff line change
@@ -549,15 +549,6 @@ private void parseOperationStatement(ShapeId id, SourceLocation location) {
ObjectNode node = IdlNodeParser.parseObjectNode(this, id.toString());
LoaderUtils.checkForAdditionalProperties(node, id, OPERATION_PROPERTY_NAMES, modelFile.events());
modelFile.onShape(builder);

if (!node.getMember("input").isPresent()) {
modelFile.events().add(LoaderUtils.createOperationMissingInput(id, location));
}

if (!node.getMember("output").isPresent()) {
modelFile.events().add(LoaderUtils.createOperationMissingOutput(id, location));
}

optionalId(node, "input", builder::input);
optionalId(node, "output", builder::output);
optionalIdList(node, ERRORS_KEYS, builder::addError);
Original file line number Diff line number Diff line change
@@ -110,42 +110,4 @@ static boolean containsErrorEvents(List<ValidationEvent> events) {
}
return false;
}

/**
* Helper method to create a validation event when input is not explicitly defined.
*
* @param id Shape ID of the operation.
* @param location Where the operation is defined.
* @return Returns the event.
*/
static ValidationEvent createOperationMissingInput(ShapeId id, SourceLocation location) {
return ValidationEvent.builder()
.id("OperationMissingInput")
.severity(Severity.WARNING)
.shapeId(id)
.sourceLocation(location)
.message("This operation does not define input, so it defaults to smithy.api#Unit. Defining input "
+ "for an operation, even if you don't need it today, future-proofs the operation by "
+ "allowing input members to be added later.")
.build();
}

/**
* Helper method to create a validation event when output is not explicitly defined.
*
* @param id Shape ID of the operation.
* @param location Where the operation is defined.
* @return Returns the event.
*/
static ValidationEvent createOperationMissingOutput(ShapeId id, SourceLocation location) {
return ValidationEvent.builder()
.id("OperationMissingOutput")
.severity(Severity.WARNING)
.shapeId(id)
.sourceLocation(location)
.message("This operation does not define output, so it defaults to smithy.api#Unit. Defining output "
+ "for an operation, even if you don't need it today, future-proofs the operation by "
+ "allowing output members to be added later.")
.build();
}
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
{
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"}
]
},
"shapes": {
"ns.foo#InvalidShapeId": {
"type": "operation",
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
{
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"}
]
},
"shapes": {
"ns.foo#Valid1": {
"type": "operation",
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
{
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"}
]
},
"shapes": {
"ns.foo#Service": {
"type": "service",
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
{
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"}
]
},
"shapes": {
"ns.foo#Operation": {
"type": "operation",
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"},
{"id": "OperationMissingInputTrait", "namespace": "*"},
{"id": "OperationMissingOutputTrait", "namespace": "*"}
]
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"},
{"id": "OperationMissingInputTrait", "namespace": "*"},
{"id": "OperationMissingOutputTrait", "namespace": "*"}
]
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
{
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"}
]
},
"shapes": {
"ns.foo#A": {
"type": "operation",
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"},
{"id": "OperationMissingInputTrait", "namespace": "ns.foo"},
{"id": "OperationMissingOutputTrait", "namespace": "ns.foo"}
]
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
{
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"}
]
},
"shapes": {
"ns.foo#A": {
"type": "operation",
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"},
{"id": "OperationMissingInputTrait", "namespace": "*"},
{"id": "OperationMissingOutputTrait", "namespace": "*"}
]
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"},
{"id": "OperationMissingInputTrait", "namespace": "*"},
{"id": "OperationMissingOutputTrait", "namespace": "*"}
]
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"},
{"id": "OperationMissingInputTrait", "namespace": "*"},
{"id": "OperationMissingOutputTrait", "namespace": "*"}
]
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"},
{"id": "OperationMissingInputTrait", "namespace": "*"},
{"id": "OperationMissingOutputTrait", "namespace": "*"}
]
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"},
{"id": "OperationMissingInputTrait", "namespace": "*"},
{"id": "OperationMissingOutputTrait", "namespace": "*"}
]
Original file line number Diff line number Diff line change
@@ -128,9 +128,6 @@
"type": "operation",
"input": {
"target": "ns.foo#OperationBInput"
},
"traits": {
"smithy.api#suppress": ["OperationMissingOutput"]
}
},
"ns.foo#OperationBInput": {
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"},
{"id": "OperationMissingInputTrait", "namespace": "*"},
{"id": "OperationMissingOutputTrait", "namespace": "*"}
]
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"},
{"id": "OperationMissingInputTrait", "namespace": "*"},
{"id": "OperationMissingOutputTrait", "namespace": "*"}
]
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
{
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "*"},
{"id": "OperationMissingOutput", "namespace": "*"}
]
},
"shapes": {
"ns.foo#MyService": {
"type": "service",
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
{
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"}
]
},
"shapes": {
"ns.foo#MyService": {
"type": "service",
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@
"smithy": "1.0",
"metadata": {
"suppressions": [
{"id": "OperationMissingInput", "namespace": "ns.foo"},
{"id": "OperationMissingOutput", "namespace": "ns.foo"},
{"id": "OperationMissingInputTrait", "namespace": "ns.foo"},
{"id": "OperationMissingOutputTrait", "namespace": "ns.foo"}
]
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
$version: "1.0"

metadata suppressions = [
{id: "OperationMissingInput", namespace: "aws.iotjobs"},
{id: "OperationMissingInputTrait", namespace: "aws.iotjobs"},
{id: "OperationMissingOutput", namespace: "aws.iotjobs"},
{id: "OperationMissingOutputTrait", namespace: "aws.iotjobs"},
]

Original file line number Diff line number Diff line change
@@ -2,7 +2,6 @@ namespace smithy.example

// Missing input for {foo} property.
@smithy.mqtt#publish("events1/{foo}")
@suppress(["OperationMissingInput", "OperationMissingOutput"])
operation Operation1 {}


@@ -51,7 +50,6 @@ structure Operation4Input {

// No errors.
@smithy.mqtt#publish("events5/{foo}")
@suppress(["OperationMissingOutput"])
operation Operation5 {
input: Operation5Input
}
Original file line number Diff line number Diff line change
@@ -3,5 +3,4 @@
namespace smithy.example

@smithy.mqtt#subscribe("events")
@suppress(["OperationMissingInput", "OperationMissingOutput"])
operation Foo {}
Original file line number Diff line number Diff line change
@@ -13,7 +13,6 @@ use smithy.waiters#waitable
]
}
)
@suppress(["OperationMissingOutput"])
operation StreamingInput {
input: StreamingInputInput
}
@@ -41,7 +40,6 @@ structure SuccessMessage {}
]
}
)
@suppress(["OperationMissingInput"])
operation StreamingOutput {
output: StreamingOutputOutput
}
Original file line number Diff line number Diff line change
@@ -19,7 +19,6 @@ use smithy.waiters#waitable
]
}
)
@suppress(["OperationMissingInput"])
operation A {
output: AOutput
}
Original file line number Diff line number Diff line change
@@ -19,7 +19,6 @@ use smithy.waiters#waitable
]
}
)
@suppress(["OperationMissingOutput"])
operation A {
input: AInput
}
Original file line number Diff line number Diff line change
@@ -18,5 +18,4 @@ use smithy.waiters#waitable
]
}
)
@suppress(["OperationMissingInput", "OperationMissingOutput"])
operation A {}

0 comments on commit 403ed0b

Please sign in to comment.