From fc4594e0f3140d3e1cdf96acc0d0e350e27328ff Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Mon, 24 Oct 2022 17:06:32 -0700 Subject: [PATCH] Add support for hierarchical event IDs Many of the validators in Smithy and Smithy Diff today emit events for various reasons. A single validator might event different events with different severities, and there is currently no way to suppress just one specific type of event for a given validator. For example, the ChangedNullability SmithyDiff validator emits events for various reasons, some of which different tools may choose to suppress. However, without inspecting the message of the event, there's no way to know what you're suppressing. To give existing validators the ability to emit more granular validation events without breaking current validators, this change introduces event ID hierarchies and hierarchical suppression IDs using dots (.). For example, ChangedNullability.RemovedRequiredTrait is now emitted, and tools can check for "ChangedNullability" to match all events, or more granularly only match "ChangedNullability.RemovedRequiredTrait". This can also be used in suppressions so that existing Smithy validators can be updated to emit more granular events. --- docs/source-2.0/spec/model-validation.rst | 52 ++++++++++ .../diff/evaluators/ChangedNullability.java | 89 +++++++++-------- .../evaluators/ChangedNullabilityTest.java | 14 +-- .../smithy/diff/evaluators/TestHelper.java | 2 +- .../model/validation/ValidationEvent.java | 25 +++++ .../suppressions/MetadataSuppression.java | 2 +- .../suppressions/TraitSuppression.java | 12 ++- .../model/validation/ValidationEventTest.java | 35 +++++++ .../suppressions/MetadataSuppressionTest.java | 65 +++++++++++++ .../suppressions/TraitSuppressionTest.java | 68 +++++++++++++ .../loader/suppression-hierarchy.errors | 7 ++ .../loader/suppression-hierarchy.smithy | 95 +++++++++++++++++++ 12 files changed, 418 insertions(+), 48 deletions(-) create mode 100644 smithy-model/src/test/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppressionTest.java create mode 100644 smithy-model/src/test/java/software/amazon/smithy/model/validation/suppressions/TraitSuppressionTest.java create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-hierarchy.errors create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-hierarchy.smithy diff --git a/docs/source-2.0/spec/model-validation.rst b/docs/source-2.0/spec/model-validation.rst index 37dc9e6f450..d1cd42422ea 100644 --- a/docs/source-2.0/spec/model-validation.rst +++ b/docs/source-2.0/spec/model-validation.rst @@ -60,6 +60,11 @@ objects that are used to constrain a model. Each object in the If ``id`` is not specified, it will default to the ``name`` property of the validator definition. + + IDs that contain dots (.) are hierarchical. For example, the ID + "Foo.Bar" contains the ID "Foo". Event ID hierarchies can be leveraged + to group validation events and allow more granular + :ref:`suppressions `. * - message - ``string`` - Provides a custom message to use when emitting validation events. The @@ -262,6 +267,53 @@ ID of ``OverlyBroadValidator``: ] +Matching suppression event IDs +============================== + +Both the :ref:`suppress-trait` and suppressions +:ref:`defined in metadata ` match events hierarchically +based on dot (.) segments. For example, given a validation event ID of +"Foo.Bar", and a suppression ID of "Foo", the suppression ID matches the +event ID because "Foo.Bar" begins with the segment "Foo". However, a suppression +ID of "Foo.Bar" does not match an event ID of "Foo" because "Foo.Bar" is more +specific. Further, a suppression ID of "ABC" does not match an event ID of +"ABC123" because "ABC" is not a segment of the event ID. + +.. list-table:: + :header-rows: 1 + + * - Event ID + - Suppression ID + - Is match + * - ``Foo`` + - ``Foo`` + - Yes + * - ``Foo.Bar`` + - ``Foo`` + - Yes + * - ``Foo.Bar.Baz`` + - ``Foo`` + - Yes + * - ``Foo.`` + - ``Foo.`` + - Yes + * - ``Foo.`` + - ``Foo`` + - Yes + * - ``Foo`` + - ``Foo.`` + - No + * - ``Foosball`` + - ``Foo`` + - No + * - ``Foo`` + - ``Foo.Bar`` + - No + * - ``Abc.Foo.Bar`` + - ``Foo.Bar`` + - No + + ------------------- Built-in validators ------------------- diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java index 3915617983e..148462b2bbd 100644 --- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java +++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java @@ -18,7 +18,6 @@ import java.util.ArrayList; import java.util.List; import java.util.Objects; -import java.util.StringJoiner; import java.util.stream.Stream; import software.amazon.smithy.diff.ChangedShape; import software.amazon.smithy.diff.Differences; @@ -26,6 +25,7 @@ import software.amazon.smithy.model.knowledge.NullableIndex; import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.traits.ClientOptionalTrait; import software.amazon.smithy.model.traits.DefaultTrait; @@ -45,24 +45,23 @@ public class ChangedNullability extends AbstractDiffEvaluator { public List evaluate(Differences differences) { NullableIndex oldIndex = NullableIndex.of(differences.getOldModel()); NullableIndex newIndex = NullableIndex.of(differences.getNewModel()); - List events = new ArrayList<>(); - Stream> changed = Stream.concat( + Stream.concat( // Get members that changed. differences.changedShapes(MemberShape.class), // Get members of structures that added/removed the input trait. changedInputMembers(differences) - ); - - changed.map(change -> { + ).forEach(change -> { // If NullableIndex says the nullability of a member changed, then that's a breaking change. MemberShape oldShape = change.getOldShape(); MemberShape newShape = change.getNewShape(); boolean wasNullable = oldIndex.isMemberNullable(oldShape); boolean isNowNullable = newIndex.isMemberNullable(newShape); - return wasNullable == isNowNullable ? null : createError(differences, change, wasNullable); - }).filter(Objects::nonNull).forEach(events::add); + if (wasNullable != isNowNullable) { + createErrors(differences, change, wasNullable, events); + } + }); return events; } @@ -79,10 +78,11 @@ private Stream> changedInputMembers(Differences differ .filter(Objects::nonNull)); } - private ValidationEvent createError( + private void createErrors( Differences differences, ChangedShape change, - boolean wasNullable + boolean wasNullable, + List events ) { MemberShape oldMember = change.getOldShape(); MemberShape newMember = change.getNewShape(); @@ -90,67 +90,80 @@ private ValidationEvent createError( oldMember.getMemberName(), wasNullable ? "nullable" : "non-nullable", wasNullable ? "non-nullable" : "nullable"); - StringJoiner joiner = new StringJoiner("; ", message, ""); boolean oldHasInput = hasInputTrait(differences.getOldModel(), oldMember); boolean newHasInput = hasInputTrait(differences.getNewModel(), newMember); + ShapeId shape = change.getShapeId(); Shape newTarget = differences.getNewModel().expectShape(newMember.getTarget()); - Severity severity = null; + int currentEventSize = events.size(); if (oldHasInput && !newHasInput) { // If there was an input trait before, but not now, then the nullability must have // changed from nullable to non-nullable. - joiner.add("The @input trait was removed from " + newMember.getContainer()); - severity = Severity.ERROR; + events.add(emit(Severity.ERROR, "RemovedInputTrait", shape, message, + "The @input trait was removed from " + newMember.getContainer())); } else if (!oldHasInput && newHasInput) { // If there was no input trait before, but there is now, then the nullability must have // changed from non-nullable to nullable. - joiner.add("The @input trait was added to " + newMember.getContainer()); - severity = Severity.DANGER; + events.add(emit(Severity.DANGER, "AddedInputTrait", shape, message, + "The @input trait was added to " + newMember.getContainer())); } else if (!newHasInput) { // Can't add nullable to a preexisting required member. if (change.isTraitAdded(ClientOptionalTrait.ID) && change.isTraitInBoth(RequiredTrait.ID)) { - joiner.add("The @nullable trait was added to a @required member."); - severity = Severity.ERROR; + events.add(emit(Severity.ERROR, "AddedNullableTrait", shape, message, + "The @nullable trait was added to a @required member.")); } // Can't add required to a member unless the member is marked as nullable. if (change.isTraitAdded(RequiredTrait.ID) && !newMember.hasTrait(ClientOptionalTrait.ID)) { - joiner.add("The @required trait was added to a member that is not marked as @nullable."); - severity = Severity.ERROR; + events.add(emit(Severity.ERROR, "AddedRequiredTrait", shape, message, + "The @required trait was added to a member that is not marked as @nullable.")); } // Can't add the default trait to a member unless the member was previously required. if (change.isTraitAdded(DefaultTrait.ID) && !change.isTraitRemoved(RequiredTrait.ID)) { - joiner.add("The @default trait was added to a member that was not previously @required."); - severity = Severity.ERROR; + events.add(emit(Severity.ERROR, "AddedDefaultTrait", shape, message, + "The @default trait was added to a member that was not previously @required.")); } // Can only remove the required trait if the member was nullable or replaced by the default trait. if (change.isTraitRemoved(RequiredTrait.ID) && !newMember.hasTrait(DefaultTrait.ID) && !oldMember.hasTrait(ClientOptionalTrait.ID)) { if (newTarget.isStructureShape() || newTarget.isUnionShape()) { - severity = severity == null ? Severity.WARNING : severity; - joiner.add("The @required trait was removed from a member that targets a " + newTarget.getType() - + ". This is backward compatible in generators that always treat structures and " - + "unions as optional (e.g., AWS generators)"); + events.add(emit(Severity.WARNING, "RemovedRequiredTrait.StructureOrUnion", shape, message, + "The @required trait was removed from a member that targets a " + + newTarget.getType() + ". This is backward compatible in generators that " + + "always treat structures and unions as optional (e.g., AWS generators)")); } else { - joiner.add("The @required trait was removed and not replaced with the @default trait and " - + "@addedDefault trait."); - severity = Severity.ERROR; + events.add(emit(Severity.ERROR, "RemovedRequiredTrait", shape, message, + "The @required trait was removed and not replaced with the @default trait and " + + "@addedDefault trait.")); } } } - // If no explicit severity was detected, then assume an error. - severity = severity == null ? Severity.ERROR : severity; - - return ValidationEvent.builder() - .id(getEventId()) - .shapeId(change.getNewShape()) - .severity(severity) - .message(joiner.toString()) - .build(); + // If not specific event was emitted, emit a generic event. + if (events.size() == currentEventSize) { + events.add(emit(Severity.ERROR, null, shape, null, message)); + } } private boolean hasInputTrait(Model model, MemberShape member) { return model.getShape(member.getContainer()).filter(shape -> shape.hasTrait(InputTrait.ID)).isPresent(); } + + private ValidationEvent emit( + Severity severity, + String eventIdSuffix, + ShapeId shape, + String prefixMessage, + String message + ) { + String actualId = eventIdSuffix == null ? getEventId() : (getEventId() + '.' + eventIdSuffix); + String actualMessage = prefixMessage == null ? message : (prefixMessage + "; " + message); + return ValidationEvent.builder() + .id(actualId) + .shapeId(shape) + .message(actualMessage) + .severity(severity) + .message(message) + .build(); + } } diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java index 8c7ea877b61..47d11ab77f8 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java @@ -90,7 +90,7 @@ public void detectsInvalidAdditionOfDefaultTrait() { assertThat(result.isDiffBreaking(), is(true)); assertThat(result.getDiffEvents().stream() .filter(event -> event.getSeverity() == Severity.ERROR) - .filter(event -> event.getId().equals("ChangedNullability")) + .filter(event -> event.getId().equals("ChangedNullability.AddedDefaultTrait")) .filter(event -> event.getShapeId().get().equals(a.getAllMembers().get("foo").getId())) .filter(event -> event.getMessage().contains("The @default trait was added to a member that " + "was not previously @required")) @@ -115,7 +115,7 @@ public void removingTheRequiredTraitOnInputStructureIsOk() { ModelDiff.Result result = ModelDiff.builder().oldModel(model1).newModel(model2).compare(); assertThat(result.getDiffEvents().stream() - .filter(event -> event.getId().equals("ChangedNullability")) + .filter(event -> event.getId().equals("ChangedNullability.RemovedRequiredTrait.StructureOrUnion")) .count(), equalTo(0L)); } @@ -137,7 +137,7 @@ public void detectsInvalidRemovalOfRequired() { assertThat(result.isDiffBreaking(), is(true)); assertThat(result.getDiffEvents().stream() .filter(event -> event.getSeverity() == Severity.ERROR) - .filter(event -> event.getId().equals("ChangedNullability")) + .filter(event -> event.getId().equals("ChangedNullability.RemovedRequiredTrait")) .filter(event -> event.getShapeId().get().equals(a.getAllMembers().get("foo").getId())) .filter(event -> event.getMessage().contains("The @required trait was removed and not " + "replaced with the @default trait")) @@ -157,7 +157,7 @@ public void detectAdditionOfRequiredTrait() { assertThat(events.stream() .filter(event -> event.getSeverity() == Severity.ERROR) - .filter(event -> event.getId().equals("ChangedNullability")) + .filter(event -> event.getId().equals("ChangedNullability.AddedRequiredTrait")) .filter(event -> event.getMessage().contains("The @required trait was added to a member " + "that is not marked as @nullable")) .count(), equalTo(1L)); @@ -180,7 +180,7 @@ public void detectAdditionOfNullableTrait() { assertThat(events.stream() .filter(event -> event.getSeverity() == Severity.ERROR) - .filter(event -> event.getId().equals("ChangedNullability")) + .filter(event -> event.getId().equals("ChangedNullability.AddedNullableTrait")) .filter(event -> event.getMessage().contains("The @nullable trait was added to a " + "@required member")) .count(), equalTo(1L)); @@ -202,7 +202,7 @@ public void detectsAdditionOfInputTrait() { assertThat(events.stream() .filter(event -> event.getSeverity() == Severity.DANGER) - .filter(event -> event.getId().equals("ChangedNullability")) + .filter(event -> event.getId().equals("ChangedNullability.AddedInputTrait")) .filter(event -> event.getMessage().contains("The @input trait was added to")) .count(), equalTo(1L)); } @@ -226,7 +226,7 @@ public void detectsRemovalOfInputTrait() { assertThat(events.stream() .filter(event -> event.getSeverity() == Severity.ERROR) - .filter(event -> event.getId().equals("ChangedNullability")) + .filter(event -> event.getId().equals("ChangedNullability.RemovedInputTrait")) .filter(event -> event.getMessage().contains("The @input trait was removed from")) .count(), equalTo(1L)); } diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/TestHelper.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/TestHelper.java index 2f8681178c3..f268733494a 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/TestHelper.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/TestHelper.java @@ -24,7 +24,7 @@ public final class TestHelper { public static List findEvents(List events, String eventId) { return events.stream() - .filter(event -> event.getId().equals(eventId)) + .filter(event -> event.containsId(eventId)) .collect(Collectors.toList()); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEvent.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEvent.java index 9e890578147..e156d36ca03 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEvent.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEvent.java @@ -241,6 +241,31 @@ public String getEventId() { return getId(); } + /** + * Tests if the event ID hierarchically contains the given ID. + * + *

Event IDs that contain dots (.) are hierarchical. An event ID of + * {@code "Foo.Bar"} contains the ID {@code "Foo"} and {@code "Foo.Bar"}. + * However, an event ID of {@code "Foo"} does not contain the ID + * {@code "Foo.Bar"} as {@code "Foo.Bar"} is more specific than {@code "Foo"}. + * If an event ID exactly matches the given {@code id}, then it also contains + * the ID (for example, {@code "Foo.Bar."} contains {@code "Foo.Bar."}. + * + * @param id ID to test. + * @return Returns true if the event's event ID contains the given {@code id}. + */ + public boolean containsId(String id) { + int eventLength = eventId.length(); + int suppressionLength = id.length(); + if (suppressionLength == eventLength) { + return id.equals(eventId); + } else if (suppressionLength > eventLength) { + return false; + } else { + return eventId.startsWith(id) && eventId.charAt(id.length()) == '.'; + } + } + /** * Returns the identifier of the validation event. * diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppression.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppression.java index 0c0c6570196..db3af4835b4 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppression.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppression.java @@ -55,7 +55,7 @@ static MetadataSuppression fromNode(Node node) { @Override public boolean test(ValidationEvent event) { - return event.getId().equals(id) && matchesNamespace(event); + return event.containsId(id) && matchesNamespace(event); } @Override diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/TraitSuppression.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/TraitSuppression.java index ea6e5c1beb7..5e8de0572ef 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/TraitSuppression.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/TraitSuppression.java @@ -34,6 +34,16 @@ final class TraitSuppression implements Suppression { @Override public boolean test(ValidationEvent event) { - return event.getShapeId().filter(shape::equals).isPresent() && trait.getValues().contains(event.getId()); + if (!event.getShapeId().filter(shape::equals).isPresent()) { + return false; + } + + for (String value : trait.getValues()) { + if (event.containsId(value)) { + return true; + } + } + + return false; } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventTest.java index 34f1f61a0cb..564a0827a0c 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventTest.java @@ -21,8 +21,12 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import java.util.stream.Stream; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.node.ObjectNode; @@ -330,4 +334,35 @@ public void convertsToNode() { assertEquals(result.getMember("filename").get().asStringNode().get().getValue(), "file"); assertEquals(result.getMember("message").get().asStringNode().get().getValue(), "The message"); } + + @ParameterizedTest + @MethodSource("containsIdSupplier") + public void suppressesEventIds(boolean match, String eventId, String testId) { + ValidationEvent event = ValidationEvent.builder().id(eventId).severity(Severity.DANGER).message(".").build(); + + assertThat(eventId + " contains? " + match + " " + testId, event.containsId(testId), is(match)); + } + + public static Stream containsIdSupplier() { + return Stream.of( + Arguments.of(true, "BadThing", "BadThing"), + Arguments.of(true, "BadThing.Foo", "BadThing"), + Arguments.of(true, "BadThing.Foo", "BadThing.Foo"), + Arguments.of(true, "BadThing.Foo.Bar", "BadThing.Foo.Bar"), + + Arguments.of(false, "BadThing.Foo", "BadThing.Foo.Bar"), + Arguments.of(false, "BadThing.Foo", "BadThing.Foo.Bar.Baz"), + Arguments.of(false, "BadThing.Fooz", "BadThing.Foo"), + Arguments.of(false, "BadThing.Foo.Bar", "BadThing.Foo.Bar.Baz"), + + // Tests for strange, but acceptable ids and suppression IDs. Preventing these now is + // technically backward incompatible, so they're acceptable. + Arguments.of(true, "BadThing.", "BadThing."), + Arguments.of(true, "BadThing.", "BadThing"), + Arguments.of(false, "BadThing", "BadThing."), + Arguments.of(true, "BadThing.Foo.", "BadThing.Foo"), + Arguments.of(true, "BadThing.Foo.", "BadThing.Foo."), + Arguments.of(false, "BadThing.Foo.", "BadThing.Foo.Bar") + ); + } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppressionTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppressionTest.java new file mode 100644 index 00000000000..d619b4d06ec --- /dev/null +++ b/smithy-model/src/test/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppressionTest.java @@ -0,0 +1,65 @@ +package software.amazon.smithy.model.validation.suppressions; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import software.amazon.smithy.model.node.Node; +import software.amazon.smithy.model.node.ObjectNode; +import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.StringShape; +import software.amazon.smithy.model.validation.Severity; +import software.amazon.smithy.model.validation.ValidationEvent; + +public class MetadataSuppressionTest { + + @Test + public void doesNotMatchWhenUsingDifferentNamespace() { + ObjectNode node = Node.objectNodeBuilder() + .withMember("id", "Foo") + .withMember("namespace", "smithy.example.nested") + .build(); + Shape s = StringShape.builder().id("smithy.example#String").build(); + Suppression suppression = Suppression.fromMetadata(node); + ValidationEvent event = ValidationEvent.builder() + .id("Foo") + .shape(s) + .severity(Severity.DANGER) + .message("test") + .build(); + + assertThat(suppression.test(event), is(false)); + } + + @ParameterizedTest + @MethodSource("suppressions") + public void suppressesEventIds(boolean match, String eventId, String suppressionId) { + ObjectNode node = Node.objectNodeBuilder() + .withMember("id", suppressionId) + .withMember("namespace", "*") + .build(); + Suppression suppression = Suppression.fromMetadata(node); + ValidationEvent event = ValidationEvent.builder() + .id(eventId) + .severity(Severity.DANGER) + .message("test") + .build(); + + assertThat(eventId + " is " + match + " match for " + suppressionId, suppression.test(event), is(match)); + } + + // See tests for ValidationEvent#containsId for exhaustive test cases. + public static Stream suppressions() { + return Stream.of( + Arguments.of(true, "BadThing", "BadThing"), + Arguments.of(true, "BadThing.Foo", "BadThing"), + Arguments.of(false, "BadThing.Foo", "BadThing.Foo.Bar"), + Arguments.of(false, "BadThing.Foo", "BadThing.Foo.Bar.Baz"), + Arguments.of(false, "BadThing.Fooz", "BadThing.Foo") + ); + } +} diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/validation/suppressions/TraitSuppressionTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/validation/suppressions/TraitSuppressionTest.java new file mode 100644 index 00000000000..9b2bbfb00f2 --- /dev/null +++ b/smithy-model/src/test/java/software/amazon/smithy/model/validation/suppressions/TraitSuppressionTest.java @@ -0,0 +1,68 @@ +package software.amazon.smithy.model.validation.suppressions; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +import java.util.List; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.StringShape; +import software.amazon.smithy.model.traits.SuppressTrait; +import software.amazon.smithy.model.validation.Severity; +import software.amazon.smithy.model.validation.ValidationEvent; +import software.amazon.smithy.utils.ListUtils; + +public class TraitSuppressionTest { + + @Test + public void doesNotMatchWhenUsingDifferentShape() { + Shape s1 = StringShape.builder() + .id("smithy.example#String1") + .addTrait(SuppressTrait.builder().values(ListUtils.of("Foo")).build()) + .build(); + Shape s2 = StringShape.builder().id("smithy.example#String2").build(); + Suppression suppression = Suppression.fromSuppressTrait(s1); + ValidationEvent event = ValidationEvent.builder() + .id("Foo") + .shape(s2) + .severity(Severity.DANGER) + .message("test") + .build(); + + assertThat(suppression.test(event), is(false)); + } + + @ParameterizedTest + @MethodSource("suppressions") + public void suppressesEventIds(boolean match, String eventId, List suppressions) { + SuppressTrait trait = SuppressTrait.builder().values(suppressions).build(); + Shape s = StringShape.builder().id("smithy.example#String").addTrait(trait).build(); + Suppression suppression = Suppression.fromSuppressTrait(s); + ValidationEvent event = ValidationEvent.builder() + .id(eventId) + .shapeId(s) + .severity(Severity.DANGER) + .message("test") + .build(); + + assertThat(eventId + " is " + match + " match for " + suppressions, suppression.test(event), is(match)); + } + + // See tests for ValidationEvent#containsId for exhaustive test cases. + public static Stream suppressions() { + return Stream.of( + Arguments.of(true, "BadThing", ListUtils.of("BadThing")), + Arguments.of(true, "BadThing", ListUtils.of("BadThing", "NotBadThing")), + Arguments.of(true, "BadThing", ListUtils.of("NotBadThing", "BadThing")), + Arguments.of(true, "BadThing.Foo", ListUtils.of("BadThing")), + Arguments.of(false, "BadThing", ListUtils.of("NotBadThing")), + Arguments.of(false, "BadThing.Foo", ListUtils.of("BadThing.Foo.Bar")), + Arguments.of(false, "BadThing.Foo", ListUtils.of("BadThing.Foo.Bar.Baz")), + Arguments.of(false, "BadThing.Fooz", ListUtils.of("BadThing.Foo")) + ); + } +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-hierarchy.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-hierarchy.errors new file mode 100644 index 00000000000..6fe0a734f04 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-hierarchy.errors @@ -0,0 +1,7 @@ +[DANGER] smithy.example#MyList1: Lists are not allowed | BadShape.List +[SUPPRESSED] smithy.example#MyList2: Lists are not allowed | BadShape.List +[SUPPRESSED] smithy.example#MyList3: Lists are not allowed | BadShape.List +[SUPPRESSED] smithy.example#MyMap: Maps are not allowed | BadShape.Map +[SUPPRESSED] smithy.example#Foo$a: Required is not allowed | BadTrait.Required +[SUPPRESSED] smithy.example#Foo$b: Documentation is not allowed | BadTrait +[SUPPRESSED] smithy.example#Foo$c: Default is not allowed | BadTrait.Default diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-hierarchy.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-hierarchy.smithy new file mode 100644 index 00000000000..9b8cc19222a --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/suppression-hierarchy.smithy @@ -0,0 +1,95 @@ +$version: "2.0" + +metadata validators = [ + { + name: "EmitEachSelector", + id: "BadShape.List", + message: "Lists are not allowed", + namespace: "smithy.example", + configuration: { + selector: "list" + } + }, + { + name: "EmitEachSelector", + id: "BadShape.Map", + message: "Maps are not allowed", + severity: "WARNING", + namespace: "smithy.example", + configuration: { + selector: "map" + } + }, + { + name: "EmitEachSelector", + id: "BadTrait", + message: "Documentation is not allowed", + namespace: "smithy.example", + configuration: { + selector: "[trait|documentation]" + } + }, + { + name: "EmitEachSelector", + id: "BadTrait.Default", + message: "Default is not allowed", + namespace: "smithy.example", + configuration: { + selector: "[trait|default]" + } + }, + { + name: "EmitEachSelector", + id: "BadTrait.Required", + message: "Required is not allowed", + namespace: "smithy.example", + configuration: { + selector: "[trait|required]" + } + } +] + +metadata suppressions = [ + { + id: "BadShape.Map", // ignore BadShape.Map, but leave BadShape.List alone. + namespace: "*", + reason: "Allow maps", + }, + { + id: "BadTrait", + namespace: "*", // Ignore BadTrait and BadTrait's children. + } +] + +namespace smithy.example + +list MyList1 { + member: String +} + +@suppress(["BadShape"]) +list MyList2 { + member: String +} + +@suppress(["BadShape.List"]) +list MyList3 { + member: String +} + +map MyMap { + key: String + value: String +} + +structure Foo { + @required + a: String + + /// Docs + b: String + + c: String = "" + + d: String +}