Add validation events decorators (smithy-lang#1774)

* Add validation events decorators Adds a new interface `ValidationEventDecorator` that libraries can implement and plug into Smithy by making those discoverable using the java builtin [`ServiceLoader`](https://docs.oracle.com/javase/8/docs/api/java/util/ServiceLoader.html). All of the `ValidationEventDecorator` instances are loaded and each `ValidationEvent` is passed to each of the decorators. The decorators can make use of the `ValidationEvent#eventId()` method to quickly filter out the events that it knows how to decorate form the events it does not care about, and the `ValidationEvent#toBuilder()` to create a builder that can be used to mutate the instance. The current use case is to add hints to a know set of events to point the user into the right solution for it (e.g., you need to pull X or Y dependency for this unresolved shape). Since often these solutions might require specific details about the environment in which Smithy is being used, those decorators cannot be part of Smithy itself but now can be implemented by libraries and loaded automatically by Smithy in a similar way in which validators are currently loaded. For instance, a decorator that suggest the user to use or remove unused structures might look like this ```java @OverRide public ValidationEvent decorate(ValidationEvent ev) { if (ev.containsId("UnreferencedShape")) { if (ev.getMessage().contains("The structure ")) { return ev.toBuilder() .hint("Consider using this structure in your service or remove it from the model") .build(); } } return ev; } ``` * Load the decorators using its own factory In order to be able to decorate events that are returned before running the validation logic we need to pull the decoration logic up to the ModelAssambler itself. For this, a new factory was created to load the decorators instead of coupling this logic with the ValidatorFactory one and the assembler will take care of loading them and passing them down to the validation logic. * Fix an event duplication bug * Update smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java Co-authored-by: Michael Dowling <[email protected]> * Update smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java Co-authored-by: Michael Dowling <[email protected]> * Address pull request comments * Move back the responsibility of loading the decorators to the ValidatorFactory and pass it down to the ModelValidator from the ModelAssembler * Overwrite the events in place in the list instead of crating a new list and copying + decorating. * Create a new method to decorate a single event and use it as part of the stream that returns core error events. * Remove the posibility of manually adding decorators in the validator and assembler and just use the ones loaded by the `ValidatorFacotry`. We don't need this now and we can add it back if and when needed. * Address pull request comments * Do not catch exceptions thrown by loaded decorators. We will ship like this for now and add later if the need arises. * Add a new method to create the validation factory instead of using the existing one to avoid braking backwards compatibility. * Address pull request comments Remove the 'builtin' qualifier that doesn't make sense in this context --------- Co-authored-by: Michael Dowling <[email protected]>
Xtansia · Aug 11, 2023 · a8790f8 · a8790f8
1 parent 09d5e2b
commit a8790f8
Show file tree

Hide file tree

Showing 13 changed files with 418 additions and 22 deletions.
diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/ColorTheme.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/ColorTheme.java
@@ -47,5 +47,7 @@ public final class ColorTheme {
     public static final Style DIFF_TITLE = Style.of(Style.BG_BRIGHT_BLACK, Style.WHITE);
     public static final Style DIFF_EVENT_TITLE = Style.of(Style.BG_BRIGHT_BLUE, Style.BLACK);
 
+    public static final Style HINT_TITLE = Style.of(Style.BRIGHT_GREEN);
+
     private ColorTheme() {}
 }
diff --git a/...-cli/src/main/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatter.java b/...-cli/src/main/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatter.java
@@ -104,6 +104,13 @@ public String format(ValidationEvent event) {
 
             writeMessage(writer, event.getMessage());
             writer.append(ls);
+            event.getHint().ifPresent(hint -> {
+                String hintLabel = "HINT: ";
+                writer.append(ls);
+                colors.style(writer, hintLabel, ColorTheme.HINT_TITLE);
+                writer.append(StyleHelper.formatMessage(hint, LINE_LENGTH - hintLabel.length(), colors));
+                writer.append(ls);
+            });
 
             return writer.toString();
         }

diff --git a/.../src/test/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatterTest.java b/.../src/test/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatterTest.java
@@ -51,23 +51,60 @@ public void formatsEventsWithNoColors() {
     }
 
     @Test
-    public void formatsEventsWithColors() {
-        PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.FORCE_COLOR);
-        String formatted = formatTestEventWithSeverity(pretty, Severity.ERROR);
+    public void formatsEventsWithHintWithNoColors() {
+        PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.NO_COLOR);
+        String formatted = formatTestEventWithSeverity(pretty, Severity.ERROR, "Some hint");
 
         assertThat(formatted, equalTo(
                 "\n"
-                + "\u001B[31m── \u001B[0m\u001B[41;30m ERROR \u001B[0m\u001B[31m ───────────────────────────────────────────────────────────────── \u001B[0mFoo\n"
-                + "\u001B[90mShape: \u001B[0m\u001B[95msmithy.example#Foo\u001B[0m\n"
-                + "\u001B[90mFile:  \u001B[0m\u001B[90mbuild/resources/test/software/amazon/smithy/cli/commands/valid-model.smithy:5:1\u001B[0m\n"
+                + "──  ERROR  ───────────────────────────────────────────────────────────────── Foo\n"
+                + "Shape: smithy.example#Foo\n"
+                + "File:  build/resources/test/software/amazon/smithy/cli/commands/valid-model.smithy:5:1\n"
                 + "\n"
-                + "\u001B[90m4| \u001B[0m\n"
-                + "\u001B[90m5| \u001B[0mresource Foo {\n"
-                + "\u001B[90m |\u001B[0m \u001B[31m^\u001B[0m\n"
+                + "4| \n"
+                + "5| resource Foo {\n"
+                + " | ^\n"
                 + "\n"
-                + "Hello, \u001B[36mthere\u001B[0m\n"));
+                + "Hello, `there`\n\n"
+                + "HINT: Some hint\n")); // keeps ticks because formatting is disabled.
+    }
+
+    @Test
+    public void formatsEventsWithColors() {
+        PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.FORCE_COLOR);
+        String formatted = formatTestEventWithSeverity(pretty, Severity.ERROR);
+
+        assertThat(formatted, equalTo(
+            "\n"
+            + "\u001B[31m── \u001B[0m\u001B[41;30m ERROR \u001B[0m\u001B[31m ───────────────────────────────────────────────────────────────── \u001B[0mFoo\n"
+            + "\u001B[90mShape: \u001B[0m\u001B[95msmithy.example#Foo\u001B[0m\n"
+            + "\u001B[90mFile:  \u001B[0m\u001B[90mbuild/resources/test/software/amazon/smithy/cli/commands/valid-model.smithy:5:1\u001B[0m\n"
+            + "\n"
+            + "\u001B[90m4| \u001B[0m\n"
+            + "\u001B[90m5| \u001B[0mresource Foo {\n"
+            + "\u001B[90m |\u001B[0m \u001B[31m^\u001B[0m\n"
+            + "\n"
+            + "Hello, \u001B[36mthere\u001B[0m\n"));
     }
 
+    @Test
+    public void formatsEventsWithHintWithColors() {
+        PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.FORCE_COLOR);
+        String formatted = formatTestEventWithSeverity(pretty, Severity.ERROR, "Some hint");
+
+        assertThat(formatted, equalTo(
+            "\n"
+            + "\u001B[31m── \u001B[0m\u001B[41;30m ERROR \u001B[0m\u001B[31m ───────────────────────────────────────────────────────────────── \u001B[0mFoo\n"
+            + "\u001B[90mShape: \u001B[0m\u001B[95msmithy.example#Foo\u001B[0m\n"
+            + "\u001B[90mFile:  \u001B[0m\u001B[90mbuild/resources/test/software/amazon/smithy/cli/commands/valid-model.smithy:5:1\u001B[0m\n"
+            + "\n"
+            + "\u001B[90m4| \u001B[0m\n"
+            + "\u001B[90m5| \u001B[0mresource Foo {\n"
+            + "\u001B[90m |\u001B[0m \u001B[31m^\u001B[0m\n"
+            + "\n"
+            + "Hello, \u001B[36mthere\u001B[0m\n\n"
+            + "\u001B[92mHINT: \u001B[0mSome hint\n"));
+    }
     @Test
     public void doesNotIncludeSourceLocationNoneInOutput() {
         PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.NO_COLOR);
@@ -116,12 +153,17 @@ private PrettyAnsiValidationFormatter createFormatter(ColorFormatter colors) {
     }
 
     private String formatTestEventWithSeverity(PrettyAnsiValidationFormatter pretty, Severity severity) {
+        return formatTestEventWithSeverity(pretty, severity, null);
+    }
+
+    private String formatTestEventWithSeverity(PrettyAnsiValidationFormatter pretty, Severity severity, String hint) {
         Model model = Model.assembler().addImport(getClass().getResource("valid-model.smithy")).assemble().unwrap();
         ValidationEvent event = ValidationEvent.builder()
                 .id("Foo")
                 .severity(severity)
                 .shape(model.expectShape(ShapeId.from("smithy.example#Foo")))
                 .message("Hello, `there`")
+                .hint(hint)
                 .build();
         return normalizeLinesAndFiles(pretty.format(event));
     }

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderTraitMap.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderTraitMap.java
@@ -38,6 +38,7 @@
 final class LoaderTraitMap {
 
     private static final Logger LOGGER = Logger.getLogger(LoaderTraitMap.class.getName());
+    private static final String UNRESOLVED_TRAIT_SUFFIX = ".UnresolvedTrait";
 
     private final TraitFactory traitFactory;
     private final Map<ShapeId, Map<ShapeId, Node>> traits = new HashMap<>();
@@ -114,7 +115,7 @@ private void validateTraitIsKnown(ShapeId target, ShapeId traitId, Trait trait,
         if (!shapeMap.isRootShapeDefined(traitId) && (trait == null || !trait.isSynthetic())) {
             Severity severity = allowUnknownTraits ? Severity.WARNING : Severity.ERROR;
             events.add(ValidationEvent.builder()
-                    .id(Validator.MODEL_ERROR)
+                    .id(Validator.MODEL_ERROR + UNRESOLVED_TRAIT_SUFFIX)
                     .severity(severity)
                     .sourceLocation(sourceLocation)
                     .shapeId(target)

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
@@ -50,6 +50,7 @@
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidatedResult;
 import software.amazon.smithy.model.validation.ValidationEvent;
+import software.amazon.smithy.model.validation.ValidationEventDecorator;
 import software.amazon.smithy.model.validation.Validator;
 import software.amazon.smithy.model.validation.ValidatorFactory;
 import software.amazon.smithy.utils.Pair;
@@ -509,6 +510,9 @@ public ValidatedResult<Model> assemble() {
         if (traitFactory == null) {
             traitFactory = LazyTraitFactoryHolder.INSTANCE;
         }
+        if (validatorFactory == null) {
+            validatorFactory = ModelValidator.defaultValidationFactory();
+        }
 
         Model prelude = disablePrelude ? null : Prelude.getPreludeModel();
         LoadOperationProcessor processor = new LoadOperationProcessor(
@@ -576,7 +580,8 @@ public ValidatedResult<Model> assemble() {
         }
 
         if (disableValidation) {
-            return new ValidatedResult<>(transformed, events);
+            List<ValidationEventDecorator> decorators = validatorFactory.loadDecorators();
+            return new ValidatedResult<>(transformed, ModelValidator.decorateEvents(decorators, events));
         }
 
         try {
@@ -594,9 +599,11 @@ private void addMetadataToProcessor(Map<String, Node> metadataMap, LoadOperation
     }
 
     private ValidatedResult<Model> returnOnlyErrors(Model model, List<ValidationEvent> events) {
+        List<ValidationEventDecorator> decorators = validatorFactory.loadDecorators();
         return new ValidatedResult<>(model, events.stream()
-                .filter(event -> event.getSeverity() == Severity.ERROR)
-                .collect(Collectors.toList()));
+                                                  .filter(event -> event.getSeverity() == Severity.ERROR)
+                                                  .map(event -> ModelValidator.decorateEvent(decorators, event))
+                                                  .collect(Collectors.toList()));
     }
 
     private ValidatedResult<Model> validate(Model model, List<ValidationEvent> events) {

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java
@@ -32,6 +32,7 @@
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidatedResult;
 import software.amazon.smithy.model.validation.ValidationEvent;
+import software.amazon.smithy.model.validation.ValidationEventDecorator;
 import software.amazon.smithy.model.validation.Validator;
 import software.amazon.smithy.model.validation.ValidatorFactory;
 import software.amazon.smithy.model.validation.suppressions.Suppression;
@@ -49,7 +50,6 @@
  * loaded from metadata.
  */
 final class ModelValidator {
-
     private static final String SUPPRESSIONS = "suppressions";
 
     // Lazy initialization holder class idiom to hold a default validator factory.
@@ -169,6 +169,7 @@ public Validator createValidator() {
         }
 
         List<Validator> staticValidators = resolveStaticValidators();
+        List<ValidationEventDecorator> staticDecorators = validatorFactory.loadDecorators();
 
         return model -> {
             List<ValidationEvent> coreEvents = new ArrayList<>();
@@ -186,6 +187,8 @@ public Validator createValidator() {
             // which will only obscure the root cause.
             coreEvents.addAll(suppressEvents(model, new TargetValidator().validate(model), modelSuppressions));
             coreEvents.addAll(suppressEvents(model, new ResourceCycleValidator().validate(model), modelSuppressions));
+            // Decorate all the events
+            coreEvents = decorateEvents(staticDecorators, coreEvents);
             // Emit any events that have already occurred.
             coreEvents.forEach(eventListener);
 
@@ -194,16 +197,18 @@ public Validator createValidator() {
             }
 
             List<ValidationEvent> result = modelValidators.parallelStream()
-                    .flatMap(validator -> validator.validate(model).stream())
+                    .map(validator -> validator.validate(model))
+                    .flatMap(Collection::stream)
                     .filter(ModelValidator::filterPrelude)
                     .map(event -> suppressEvent(model, event, modelSuppressions))
+                    .map(event -> decorateEvent(staticDecorators, event))
                     // Emit events as they occur during validation.
                     .peek(eventListener)
                     .collect(Collectors.toList());
 
             for (ValidationEvent event : includeEvents) {
                 if (ModelValidator.filterPrelude(event)) {
-                    result.add(suppressEvent(model, event, modelSuppressions));
+                    result.add(decorateEvent(staticDecorators, suppressEvent(model, event, modelSuppressions)));
                 }
             }
 
@@ -214,6 +219,32 @@ public Validator createValidator() {
         };
     }
 
+    static List<ValidationEvent> decorateEvents(
+        List<ValidationEventDecorator> decorators,
+        List<ValidationEvent> events
+    ) {
+        if (!decorators.isEmpty()) {
+            for (int idx = 0; idx < events.size(); idx++) {
+                events.set(idx, decorateEvent(decorators, events.get(idx)));
+            }
+        }
+        return events;
+    }
+
+    static ValidationEvent decorateEvent(List<ValidationEventDecorator> decorators, ValidationEvent event) {
+        ValidationEvent decoratedEvent = event;
+        for (ValidationEventDecorator decorator : decorators) {
+            if (decorator.canDecorate(event)) {
+                decoratedEvent = decorator.decorate(decoratedEvent);
+            }
+        }
+        return decoratedEvent;
+    }
+
+    static ValidatorFactory defaultValidationFactory() {
+        return LazyValidatorFactoryHolder.INSTANCE;
+    }
+
     private List<Validator> resolveStaticValidators() {
         List<Validator> resolvedValidators = new ArrayList<>(validatorFactory.loadBuiltinValidators());
         resolvedValidators.addAll(validators);

diff --git a/...l/src/main/java/software/amazon/smithy/model/validation/LineValidationEventFormatter.java b/...l/src/main/java/software/amazon/smithy/model/validation/LineValidationEventFormatter.java
@@ -29,6 +29,10 @@ public String format(ValidationEvent event) {
         if (reason != null) {
             message += " (" + reason + ")";
         }
+        String hint = event.getHint().orElse(null);
+        if (hint != null) {
+            message += " [" + hint + "]";
+        }
 
         return String.format(
                 "[%s] %s: %s | %s %s:%s:%s",

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
@@ -48,6 +48,7 @@ public final class ValidationEvent
     private final Severity severity;
     private final ShapeId shapeId;
     private final String suppressionReason;
+    private final String hint;
     private int hash;
 
     private ValidationEvent(Builder builder) {
@@ -61,6 +62,7 @@ private ValidationEvent(Builder builder) {
         this.eventId = SmithyBuilder.requiredState("id", builder.eventId);
         this.shapeId = builder.shapeId;
         this.suppressionReason = builder.suppressionReason;
+        this.hint = builder.hint;
     }
 
     public static Builder builder() {
@@ -141,6 +143,7 @@ public Builder toBuilder() {
         builder.eventId = eventId;
         builder.shapeId = shapeId;
         builder.suppressionReason = suppressionReason;
+        builder.hint = hint;
         return builder;
     }
 
@@ -158,14 +161,15 @@ public boolean equals(Object o) {
                 && severity.equals(other.severity)
                 && eventId.equals(other.eventId)
                 && getShapeId().equals(other.getShapeId())
-                && getSuppressionReason().equals(other.getSuppressionReason());
+                && getSuppressionReason().equals(other.getSuppressionReason())
+                && getHint().equals(other.getHint());
     }
 
     @Override
     public int hashCode() {
         int result = hash;
         if (result == 0) {
-            result = Objects.hash(eventId, shapeId, severity, sourceLocation, message, suppressionReason);
+            result = Objects.hash(eventId, shapeId, severity, sourceLocation, message, suppressionReason, hint);
             hash = result;
         }
         return result;
@@ -184,6 +188,7 @@ public Node toNode() {
                 .withOptionalMember("shapeId", getShapeId().map(Object::toString).map(Node::from))
                 .withMember("message", Node.from(getMessage()))
                 .withOptionalMember("suppressionReason", getSuppressionReason().map(Node::from))
+                .withOptionalMember("hint", getHint().map(Node::from))
                 .withMember("filename", Node.from(getSourceLocation().getFilename()))
                 .withMember("line", Node.from(getSourceLocation().getLine()))
                 .withMember("column", Node.from(getSourceLocation().getColumn()))
@@ -205,6 +210,7 @@ public static ValidationEvent fromNode(Node node) {
                 .expectMember("severity", Severity::fromNode, builder::severity)
                 .expectStringMember("message", builder::message)
                 .getStringMember("suppressionReason", builder::suppressionReason)
+                .getStringMember("hint", builder::hint)
                 .getMember("shapeId", ShapeId::fromNode, builder::shapeId);
         return builder.build();
     }
@@ -294,6 +300,15 @@ public Optional<String> getSuppressionReason() {
         return Optional.ofNullable(suppressionReason);
     }
 
+    /**
+     * Get an optional hint that adds more detail about how to fix a specific issue.
+     *
+     * @return Returns the hint if available.
+     */
+    public Optional<String> getHint() {
+        return Optional.ofNullable(hint);
+    }
+
     /**
      * Builds ValidationEvent values.
      */
@@ -305,6 +320,7 @@ public static final class Builder implements SmithyBuilder<ValidationEvent> {
         private String eventId;
         private ShapeId shapeId;
         private String suppressionReason;
+        private String hint;
 
         private Builder() {}
 
@@ -402,6 +418,17 @@ public Builder suppressionReason(String eventSuppressionReason) {
             return this;
         }
 
+        /**
+         * Sets an optional hint adding more detail about how to fix a specific issue.
+         *
+         * @param hint Hint to set
+         * @return Returns the builder.
+         */
+        public Builder hint(String hint) {
+            this.hint = hint;
+            return this;
+        }
+
         @Override
         public ValidationEvent build() {
             return new ValidationEvent(this);