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.
smithy-lang · Oct 27, 2022 · fc4594e · fc4594e
1 parent c079c9b
commit fc4594e
Show file tree

Hide file tree

Showing 12 changed files with 418 additions and 48 deletions.
diff --git 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 <suppression-definition>`.
     * - 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 <suppressions-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
@@ -18,14 +18,14 @@
 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;
 import software.amazon.smithy.model.Model;
 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<ValidationEvent> evaluate(Differences differences) {
         NullableIndex oldIndex = NullableIndex.of(differences.getOldModel());
         NullableIndex newIndex = NullableIndex.of(differences.getNewModel());
-
         List<ValidationEvent> events = new ArrayList<>();
 
-        Stream<ChangedShape<MemberShape>> 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,78 +78,92 @@ private Stream<ChangedShape<MemberShape>> changedInputMembers(Differences differ
                         .filter(Objects::nonNull));
     }
 
-    private ValidationEvent createError(
+    private void createErrors(
             Differences differences,
             ChangedShape<MemberShape> change,
-            boolean wasNullable
+            boolean wasNullable,
+            List<ValidationEvent> events
     ) {
         MemberShape oldMember = change.getOldShape();
         MemberShape newMember = change.getNewShape();
         String message = String.format("Member `%s` changed from %s to %s: ",
                                        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
@@ -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
@@ -24,7 +24,7 @@
 public final class TestHelper {
     public static List<ValidationEvent> findEvents(List<ValidationEvent> 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
@@ -241,6 +241,31 @@ public String getEventId() {
         return getId();
     }
 
+    /**
+     * Tests if the event ID hierarchically contains the given ID.
+     *
+     * <p>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/...c/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSuppression.java b/...c/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/.../src/main/java/software/amazon/smithy/model/validation/suppressions/TraitSuppression.java b/.../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;
     }
 }