Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modifies ExamplesTraitValidator to handle cases where both output and error are defined #1599

Merged
merged 4 commits into from
Feb 23, 2023
Merged

Modifies ExamplesTraitValidator to handle cases where both output and error are defined #1599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1d8386
Fix behavior of examples with output and error
jvschneid Feb 1, 2023
05a300a
Fix errors in examples trait docs
jvschneid Feb 1, 2023
eade383
Add entry to docs for new example behavior
jvschneid Feb 2, 2023
b2125c5
Fix else-if chain
jvschneid Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/source-1.0/spec/core/documentation-traits.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,9 @@ compatible with the shapes and constraints of the corresponding structure.
These values use the same semantics and format as
:ref:`custom trait values <trait-node-values>`.

A value for ``output`` or ``error`` SHOULD be provided. However, both
MUST NOT be defined for the same example.

.. tabs::

.. code-tab:: smithy
Expand Down
7 changes: 4 additions & 3 deletions docs/source-2.0/spec/documentation-traits.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ compatible with the shapes and constraints of the corresponding structure.
These values use the same semantics and format as
:ref:`custom trait values <trait-node-values>`.

A value for ``output`` or ``error`` SHOULD be provided. However, both
MUST NOT be defined for the same example.

.. code-block:: smithy
@readonly
Expand Down Expand Up @@ -175,9 +178,7 @@ These values use the same semantics and format as
{
title: "Error example for MyOperation"
input: {
foo: "!",
}
foo: 1
foo: "!"
}
error: {
shapeId: MyOperationError
Expand Down
8 changes: 4 additions & 4 deletions smithy-model/src/main/java/software/amazon/smithy/model/traits/ExamplesTrait.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ public ObjectNode getInput() {
/**
* @return Gets the output object.
*/
public ObjectNode getOutput() {
return output;
public Optional<ObjectNode> getOutput() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of a breaking change... but maybe using this trait in code is fringe enough to not matter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about keeping it as is and checking if example.getOutput().isEmpty() to check if it was defined, but it opened the possibility of the example below being valid, which I think shouldn't be, if it's present it should conform, even if empty:

$version: "2.0"

namespace example.hello

@examples([
    {
        title: "Unhappy path",
        output: {},
        error: {
            shapeId: YouShallNotPass
            content: {
                "message": "Go back to the Shadow"
            }
        }
    }
])
operation Hello {
    output: Greeting,
    errors: [YouShallNotPass]
}

structure Greeting {
    @required
    message: String
}

@error("client")
structure YouShallNotPass {
    @required
    message: String
}

return Optional.ofNullable(output);
}

/**
Expand All @@ -154,7 +154,7 @@ public Node toNode() {
if (!input.isEmpty()) {
builder.withMember("input", input);
}
if (!output.isEmpty()) {
if (this.getOutput().isPresent()) {
builder.withMember("output", output);
}

Expand All @@ -177,7 +177,7 @@ public static final class Builder implements SmithyBuilder<Example> {
private String title;
private String documentation;
private ObjectNode input = Node.objectNode();
private ObjectNode output = Node.objectNode();
private ObjectNode output;
private ErrorExample error;

@Override
Expand Down
21 changes: 14 additions & 7 deletions .../main/java/software/amazon/smithy/model/validation/validators/ExamplesTraitValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,20 @@ private List<ValidationEvent> validateExamples(Model model, OperationShape shape
events.addAll(input.accept(validator));
});

model.getShape(shape.getOutputShape()).ifPresent(output -> {
NodeValidationVisitor validator = createVisitor(
"output", example.getOutput(), model, shape, example);
events.addAll(output.accept(validator));
});
boolean isOutputDefined = example.getOutput().isPresent();
boolean isErrorDefined = example.getError().isPresent();

if (example.getError().isPresent()) {
if (isOutputDefined && isErrorDefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the behaviour we want, this needs to be mentioned in the doc as well.
https://smithy.io/2.0/spec/documentation-traits.html?highlight=example#examples-trait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point!

events.add(error(shape, trait, String.format(
"Example: `%s` has both output and error defined, only one should be present.",
example.getTitle())));
} else if (isOutputDefined) {
model.getShape(shape.getOutputShape()).ifPresent(output -> {
NodeValidationVisitor validator = createVisitor(
"output", example.getOutput().get(), model, shape, example);
events.addAll(output.accept(validator));
});
} else if (isErrorDefined) {
ExamplesTrait.ErrorExample errorExample = example.getError().get();
Optional<Shape> errorShape = model.getShape(errorExample.getShapeId());
if (errorShape.isPresent() && shape.getErrors().contains(errorExample.getShapeId())) {
Expand All @@ -68,7 +75,7 @@ private List<ValidationEvent> validateExamples(Model model, OperationShape shape
events.addAll(errorShape.get().accept(validator));
} else {
events.add(error(shape, trait, String.format(
"Error parameters provided for operation without the `%s` error: `%s`",
"Error parameters provided for operation without the `%s` error: `%s`",
errorExample.getShapeId(), example.getTitle())));
}
}
Expand Down
7 changes: 4 additions & 3 deletions ...ources/software/amazon/smithy/model/errorfiles/validators/examples-trait-validator.errors
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[WARNING] ns.foo#Operation2: Example input of `Testing 3`: Invalid structure member `foo` found for `smithy.api#Unit` | ExamplesTrait
[WARNING] ns.foo#Operation2: Example output of `Testing 3`: Invalid structure member `bam` found for `smithy.api#Unit` | ExamplesTrait
[ERROR] ns.foo#Operation2: Error parameters provided for operation without the `ns.foo#OperationError` error: `Testing 3` | ExamplesTrait
[ERROR] ns.foo#Operation2: Error parameters provided for operation without the `ns.foo#OperationError` error: `Testing 5` | ExamplesTrait
[WARNING] ns.foo#Operation2: Example input of `Testing 4`: Invalid structure member `foo` found for `smithy.api#Unit` | ExamplesTrait
[WARNING] ns.foo#Operation2: Example output of `Testing 4`: Invalid structure member `bam` found for `smithy.api#Unit` | ExamplesTrait
[ERROR] ns.foo#Operation: Example: `Testing 3` has both output and error defined, only one should be present. | ExamplesTrait
[ERROR] ns.foo#Operation: Example input of `Testing 2`: Missing required structure member `foo` for `ns.foo#OperationInput` | ExamplesTrait
[WARNING] ns.foo#Operation: Example output of `Testing 2`: Invalid structure member `additional` found for `ns.foo#OperationOutput` | ExamplesTrait
[ERROR] ns.foo#Operation: Example output of `Testing 2`: Missing required structure member `bam` for `ns.foo#OperationOutput` | ExamplesTrait
Expand Down
25 changes: 20 additions & 5 deletions ...esources/software/amazon/smithy/model/errorfiles/validators/examples-trait-validator.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@
"input": {
"foo": "value1"
},
"output": {
"bam": "value2"
},
"error": {
"shapeId": "ns.foo#OperationError",
"content": {
Expand All @@ -41,6 +38,21 @@
"output": {
"additional": "value"
}
},
{
"title": "Testing 3",
"input": {
"foo": "value1"
},
"output": {
"bam": "value2"
},
"error": {
"shapeId": "ns.foo#OperationError",
"content": {
"bat": "baz"
}
}
}
]
}
Expand Down Expand Up @@ -93,13 +105,16 @@
"smithy.api#readonly": {},
"smithy.api#examples": [
{
"title": "Testing 3",
"title": "Testing 4",
"input": {
"foo": "baz"
},
"output": {
"bam": "baz"
},
}
},
{
"title": "Testing 5",
"error": {
"shapeId": "ns.foo#OperationError",
"content": {
Expand Down
6 changes: 4 additions & 2 deletions ...c/main/java/software/amazon/smithy/openapi/fromsmithy/protocols/AbstractRestProtocol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,8 @@ private Map<String, Node> createExamplesForMembersWithHttpTraits(
Optional<ExamplesTrait> examplesTrait = operationOrError.getTrait(ExamplesTrait.class);
for (ExamplesTrait.Example example
: examplesTrait.map(ExamplesTrait::getExamples).orElse(Collections.emptyList())) {
ObjectNode inputOrOutput = type == MessageType.REQUEST ? example.getInput() : example.getOutput();
ObjectNode inputOrOutput = type == MessageType.REQUEST ? example.getInput()
: example.getOutput().orElse(Node.objectNode());
String name = operationOrError.getId().getName() + "_example" + uniqueNum++;

// this if condition is needed to avoid errors when converting examples of response.
Expand Down Expand Up @@ -327,7 +328,8 @@ private Map<String, Node> createBodyExamples(
: examplesTrait.map(ExamplesTrait::getExamples).orElse(Collections.emptyList())) {
// get members included in bindings
ObjectNode values = getMembersWithHttpBindingTrait(bindings,
type == MessageType.REQUEST ? example.getInput() : example.getOutput());
type == MessageType.REQUEST ? example.getInput()
: example.getOutput().orElse(Node.objectNode()));
String name = operationOrError.getId().getName() + "_example" + uniqueNum++;
// this if condition is needed to avoid errors when converting examples of response.
if (!example.getError().isPresent() || type == MessageType.REQUEST) {
Expand Down