From 86199b013e9e70e1d580e9bf78ef655188147741 Mon Sep 17 00:00:00 2001 From: JordonPhillips Date: Fri, 15 May 2020 15:51:33 -0700 Subject: [PATCH 1/2] Fix bug allowing validation to be skipped This fixes a bug where any `SourceException`s rasied after the model assembler began validation would result in validation getting skipped. This was due to the fact that the assembler calls `onEnd` in its visitor before validation AND whenever it catches that class of exception. `onEnd` will cache its result, so you can't simply call `onError` and guarantee it will be in the output. --- .../openapi/integrations-without-credentials.json | 12 ++---------- .../smithy/aws/apigateway/openapi/integrations.json | 12 ++---------- .../amazon/smithy/model/loader/ModelAssembler.java | 6 ++++-- .../smithy/model/loader/ModelAssemblerTest.java | 12 ++++++++++++ 4 files changed, 20 insertions(+), 22 deletions(-) diff --git a/smithy-aws-apigateway-openapi/src/test/resources/software/amazon/smithy/aws/apigateway/openapi/integrations-without-credentials.json b/smithy-aws-apigateway-openapi/src/test/resources/software/amazon/smithy/aws/apigateway/openapi/integrations-without-credentials.json index 3e8cb723b07..1b804be4989 100644 --- a/smithy-aws-apigateway-openapi/src/test/resources/software/amazon/smithy/aws/apigateway/openapi/integrations-without-credentials.json +++ b/smithy-aws-apigateway-openapi/src/test/resources/software/amazon/smithy/aws/apigateway/openapi/integrations-without-credentials.json @@ -165,7 +165,8 @@ }, "traits": { "smithy.api#error": "client", - "smithy.api#httpError": 302 + "smithy.api#httpError": 302, + "smithy.api#suppress": ["HttpResponseCodeSemantics"] } }, "smithy.example#OperationInput": { @@ -198,14 +199,5 @@ } } } - }, - "metadata": { - "suppressions": [ - { - "ids": ["HttpResponseCodeSemantics"], - "shapes": ["smithy.example#Redirect"], - "reason": "model redirect as error" - } - ] } } diff --git a/smithy-aws-apigateway-openapi/src/test/resources/software/amazon/smithy/aws/apigateway/openapi/integrations.json b/smithy-aws-apigateway-openapi/src/test/resources/software/amazon/smithy/aws/apigateway/openapi/integrations.json index 262f6076775..84dbc4c2e83 100644 --- a/smithy-aws-apigateway-openapi/src/test/resources/software/amazon/smithy/aws/apigateway/openapi/integrations.json +++ b/smithy-aws-apigateway-openapi/src/test/resources/software/amazon/smithy/aws/apigateway/openapi/integrations.json @@ -166,7 +166,8 @@ }, "traits": { "smithy.api#error": "client", - "smithy.api#httpError": 302 + "smithy.api#httpError": 302, + "smithy.api#suppress": ["HttpResponseCodeSemantics"] } }, "smithy.example#OperationInput": { @@ -199,14 +200,5 @@ } } } - }, - "metadata": { - "suppressions": [ - { - "ids": ["HttpResponseCodeSemantics"], - "shapes": ["smithy.example#Redirect"], - "reason": "model redirect as error" - } - ] } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java index 470e5d25865..d9e913a7546 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java @@ -477,8 +477,10 @@ public ValidatedResult assemble() { try { return doAssemble(visitor); } catch (SourceException e) { - visitor.onError(ValidationEvent.fromSourceException(e)); - return visitor.onEnd(); + ValidatedResult modelResult = visitor.onEnd(); + List events = new ArrayList<>(modelResult.getValidationEvents()); + events.add(ValidationEvent.fromSourceException(e)); + return new ValidatedResult<>(modelResult.getResult().orElse(null), events); } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java index 51032c0ee2c..035965f0a6d 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java @@ -481,4 +481,16 @@ public void canDisableValidation() { assertFalse(result.isBroken()); } + + @Test + public void canHandleBadSuppressions() { + String document = "namespace foo.baz\n" + + "@idempotent\n" // < this is invalid + + "string MyString\n"; + ValidatedResult result = new ModelAssembler() + .addUnparsedModel("foo.smithy", document) + .putMetadata("suppressions", Node.parse("[{}]")) + .assemble(); + assertTrue(result.isBroken()); + } } From 07fabc70a3961bca4d5c39279cc75ee9b262bad7 Mon Sep 17 00:00:00 2001 From: JordonPhillips Date: Fri, 15 May 2020 16:08:11 -0700 Subject: [PATCH 2/2] Fix typo in suppress trait docs --- docs/source/1.0/spec/core/model-validation.rst | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/source/1.0/spec/core/model-validation.rst b/docs/source/1.0/spec/core/model-validation.rst index 89832df1cae..8878ee31cba 100644 --- a/docs/source/1.0/spec/core/model-validation.rst +++ b/docs/source/1.0/spec/core/model-validation.rst @@ -179,18 +179,18 @@ Suppressions ------------ Suppressions are used to suppress specific validation events. -Suppressions are created using the :ref:`suppression-trait` and +Suppressions are created using the :ref:`suppress-trait` and :ref:`suppressions metadata `. -.. _suppression-trait: +.. _suppress-trait: -``suppression`` trait +``suppress`` trait ===================== Summary - The suppression trait is used to suppress validation events(s) for a - specific shape. Each value in the ``suppression`` trait is a + The suppress trait is used to suppress validation events(s) for a + specific shape. Each value in the ``suppress`` trait is a validation event ID to suppress for the shape. Trait selector ``*`` @@ -208,7 +208,7 @@ for the ``smithy.example#MyString`` shape: namespace smithy.example - @suppression(["Foo", "Bar"]) + @suppress(["Foo", "Bar"]) string MyString