From 764521f4ab61d53668f8b63af849d8bcae1ba0e1 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Wed, 7 Feb 2024 10:31:02 -0600 Subject: [PATCH] Fix regression that allowed suppressing ERROR There was a regression in #1890 that allowed ERROR events to be suppressed. This was not intentional and we do not want to remove this restriction. ERROR events indicate that the model is in an unusable state because it violates a MUST in the spec or some other necessary requirement to make it usable (like referring only to shapes that exist). New test cases have been added to guard against this regression in the future. Closes #2128 --- .../suppressions/ModelBasedEventDecorator.java | 5 +++++ .../model/knowledge/PropertyBindingIndexTest.java | 10 +++------- ...ppression-metadata-cannot-suppress-errors.errors | 1 + ...ppression-metadata-cannot-suppress-errors.smithy | 13 +++++++++++++ .../suppression-trait-cannot-suppress-errors.errors | 1 + .../suppression-trait-cannot-suppress-errors.smithy | 7 +++++++ .../model/knowledge/member-property-index.smithy | 7 ------- 7 files changed, 30 insertions(+), 14 deletions(-) create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-metadata-cannot-suppress-errors.errors create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-metadata-cannot-suppress-errors.smithy create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-trait-cannot-suppress-errors.errors create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-trait-cannot-suppress-errors.smithy diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/ModelBasedEventDecorator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/ModelBasedEventDecorator.java index a90ce944c28..889451ff635 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/ModelBasedEventDecorator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/ModelBasedEventDecorator.java @@ -151,6 +151,11 @@ private static ValidationEvent modifyEventSeverity( List suppressions, List severityOverrides ) { + // ERROR and SUPPRESSED events cannot be suppressed. + if (!event.getSeverity().canSuppress()) { + return event; + } + // Use a suppress trait if present. if (event.getShapeId().isPresent()) { ShapeId target = event.getShapeId().get(); diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/PropertyBindingIndexTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/PropertyBindingIndexTest.java index b51c20ac8a6..c707142bc41 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/PropertyBindingIndexTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/PropertyBindingIndexTest.java @@ -15,20 +15,18 @@ package software.amazon.smithy.model.knowledge; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasSize; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; 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.validation.Severity; import software.amazon.smithy.model.validation.ValidatedResult; +import software.amazon.smithy.model.validation.ValidatedResultException; public class PropertyBindingIndexTest { @@ -71,8 +69,6 @@ public void testIndex() { assertTrue(index.doesMemberShapeRequireProperty(model.expectShape( ShapeId.from("com.example#ChangeResourceOutput$id"), MemberShape.class))); - assertThat(vrmodel.getValidationEvents(Severity.SUPPRESSED), hasSize(1)); - assertThat(vrmodel.getValidationEvents(Severity.SUPPRESSED).get(0).getId(), - equalTo("ResourceOperationInputOutput")); + Assertions.assertThrows(ValidatedResultException.class, () -> vrmodel.unwrap()); } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-metadata-cannot-suppress-errors.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-metadata-cannot-suppress-errors.errors new file mode 100644 index 00000000000..2065c6e952a --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-metadata-cannot-suppress-errors.errors @@ -0,0 +1 @@ +[ERROR] foo.baz#MyString: Trait `idempotent` | TraitTarget diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-metadata-cannot-suppress-errors.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-metadata-cannot-suppress-errors.smithy new file mode 100644 index 00000000000..8bc35a09149 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-metadata-cannot-suppress-errors.smithy @@ -0,0 +1,13 @@ +$version: "2" + +metadata suppressions = [ + { + namespace: "foo.baz" + id: "TraitTarget" + } +] + +namespace foo.baz + +@idempotent // < this is invalid +string MyString diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-trait-cannot-suppress-errors.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-trait-cannot-suppress-errors.errors new file mode 100644 index 00000000000..2065c6e952a --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-trait-cannot-suppress-errors.errors @@ -0,0 +1 @@ +[ERROR] foo.baz#MyString: Trait `idempotent` | TraitTarget diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-trait-cannot-suppress-errors.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-trait-cannot-suppress-errors.smithy new file mode 100644 index 00000000000..195f18366b7 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-trait-cannot-suppress-errors.smithy @@ -0,0 +1,7 @@ +$version: "2" + +namespace foo.baz + +@suppress(["TraitTarget"]) +@idempotent // < this is invalid +string MyString diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/member-property-index.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/member-property-index.smithy index 57287636421..2c9860a95ee 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/member-property-index.smithy +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/member-property-index.smithy @@ -1,12 +1,5 @@ $version: "2.0" -metadata suppressions = [ - { - id: "ResourceOperationInputOutput", - namespace: "com.example" - } -] - namespace com.example resource Resource1 {