From 8a4d315f98469ed63c04c63381e008604c82f618 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Fri, 14 May 2021 15:39:35 -0700 Subject: [PATCH] Make CLI output improvements This commit closes #797 The CLI will now log validation events as they occur rather than waiting until all events are encountered before writing them. This makes the CLI a bit more responsive when validating hundreds of thousands of shapes. Other improvements were added to the CLI output as well, including the --severity parameter to set the minimum severity to display in output. This can significantly cut down on noise when model files emit things < DANGER. --- .../software/amazon/smithy/cli/SmithyCli.java | 1 + .../smithy/cli/commands/BuildCommand.java | 3 + .../smithy/cli/commands/CommandUtils.java | 37 +++++++++ .../smithy/cli/commands/ValidateCommand.java | 3 + .../amazon/smithy/cli/commands/Validator.java | 20 +---- .../cli/commands/ValidateCommandTest.java | 82 +++++++++++++++++++ .../cli/commands/validation-events.smithy | 50 +++++++++++ .../smithy/model/loader/ModelAssembler.java | 33 +++++++- .../smithy/model/loader/ModelValidator.java | 17 +++- .../model/loader/ModelAssemblerTest.java | 19 ++++- 10 files changed, 241 insertions(+), 24 deletions(-) create mode 100644 smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/validation-events.smithy diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/SmithyCli.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/SmithyCli.java index c180ecc5f15..5b8e1304875 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/SmithyCli.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/SmithyCli.java @@ -29,6 +29,7 @@ public final class SmithyCli { public static final String DISCOVER = "--discover"; public static final String DISCOVER_CLASSPATH = "--discover-classpath"; public static final String ALLOW_UNKNOWN_TRAITS = "--allow-unknown-traits"; + public static final String SEVERITY = "--severity"; private ClassLoader classLoader = getClass().getClassLoader(); diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildCommand.java index a61b9edcc80..5ee04526d79 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildCommand.java @@ -68,6 +68,9 @@ public Parser getParser() { .option(SmithyCli.DISCOVER, "-d", "Enables model discovery, merging in models found inside of jars") .parameter(SmithyCli.DISCOVER_CLASSPATH, "Enables model discovery using a custom classpath for models") .option(SmithyCli.ALLOW_UNKNOWN_TRAITS, "Ignores unknown traits when building models") + .parameter(SmithyCli.SEVERITY, "Sets a minimum validation event severity to display. " + + "Defaults to NOTE. Can be set to SUPPRESSED, NOTE, WARNING, " + + "DANGER, ERROR.") .positional("", "Path to Smithy models or directories") .build(); } diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/CommandUtils.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/CommandUtils.java index 3545e664844..6271828f296 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/CommandUtils.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/CommandUtils.java @@ -21,14 +21,20 @@ import java.nio.file.Paths; import java.security.AccessController; import java.security.PrivilegedAction; +import java.util.Arrays; import java.util.List; import java.util.Set; +import java.util.function.Consumer; import java.util.logging.Logger; import software.amazon.smithy.cli.Arguments; +import software.amazon.smithy.cli.Cli; import software.amazon.smithy.cli.CliError; +import software.amazon.smithy.cli.Colors; import software.amazon.smithy.cli.SmithyCli; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.loader.ModelAssembler; +import software.amazon.smithy.model.validation.ContextualValidationEventFormatter; +import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.model.validation.ValidatedResult; final class CommandUtils { @@ -40,6 +46,32 @@ private CommandUtils() {} static Model buildModel(Arguments arguments, ClassLoader classLoader, Set features) { List models = arguments.positionalArguments(); ModelAssembler assembler = CommandUtils.createModelAssembler(classLoader); + + ContextualValidationEventFormatter formatter = new ContextualValidationEventFormatter(); + boolean stdout = features.contains(Validator.Feature.STDOUT); + boolean quiet = features.contains(Validator.Feature.QUIET); + Consumer writer = stdout ? Cli.getStdout() : Cli.getStderr(); + + // --severity defaults to NOTE. + Severity minSeverity = arguments.has(SmithyCli.SEVERITY) + ? parseSeverity(arguments.parameter(SmithyCli.SEVERITY)) + : Severity.NOTE; + + assembler.validationEventListener(event -> { + // Only log events that are >= --severity. + if (event.getSeverity().ordinal() >= minSeverity.ordinal()) { + if (event.getSeverity() == Severity.WARNING && !quiet) { + // Only log warnings when not quiet + Colors.YELLOW.write(writer, formatter.format(event) + System.lineSeparator()); + } else if (event.getSeverity() == Severity.DANGER || event.getSeverity() == Severity.ERROR) { + // Always output error and danger events, even when quiet. + Colors.RED.write(writer, formatter.format(event) + System.lineSeparator()); + } else if (!quiet) { + writer.accept(formatter.format(event) + System.lineSeparator()); + } + } + }); + CommandUtils.handleModelDiscovery(arguments, assembler, classLoader); CommandUtils.handleUnknownTraitsOption(arguments, assembler); models.forEach(assembler::addImport); @@ -48,6 +80,11 @@ static Model buildModel(Arguments arguments, ClassLoader classLoader, Set new RuntimeException("Expected Validator to throw")); } + static Severity parseSeverity(String str) { + return Severity.fromString(str).orElseThrow(() -> new IllegalArgumentException( + "Invalid severity: " + str + ". Expected one of: " + Arrays.toString(Severity.values()))); + } + static ModelAssembler createModelAssembler(ClassLoader classLoader) { return Model.assembler(classLoader).putProperty(ModelAssembler.DISABLE_JAR_CACHE, true); } diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidateCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidateCommand.java index d6f5bd39919..cbf8955f234 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidateCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidateCommand.java @@ -42,6 +42,9 @@ public Parser getParser() { .option(SmithyCli.ALLOW_UNKNOWN_TRAITS, "Ignores unknown traits when validating models") .option(SmithyCli.DISCOVER, "-d", "Enables model discovery, merging in models found inside of jars") .parameter(SmithyCli.DISCOVER_CLASSPATH, "Enables model discovery using a custom classpath for models") + .parameter(SmithyCli.SEVERITY, "Sets a minimum validation event severity to display. " + + "Defaults to NOTE. Can be set to SUPPRESSED, NOTE, WARNING, " + + "DANGER, ERROR.") .positional("", "Path to Smithy models or directories") .build(); } diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Validator.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Validator.java index 94428f973fa..b80860f151b 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Validator.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Validator.java @@ -21,9 +21,7 @@ import java.util.function.Consumer; import software.amazon.smithy.cli.Cli; import software.amazon.smithy.cli.CliError; -import software.amazon.smithy.cli.Colors; import software.amazon.smithy.model.Model; -import software.amazon.smithy.model.validation.ContextualValidationEventFormatter; import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.model.validation.ValidatedResult; @@ -46,26 +44,10 @@ enum Feature { } static void validate(ValidatedResult result, Set features) { - ContextualValidationEventFormatter formatter = new ContextualValidationEventFormatter(); - - boolean stdout = features.contains(Feature.STDOUT); boolean quiet = features.contains(Feature.QUIET); + boolean stdout = features.contains(Feature.STDOUT); Consumer writer = stdout ? Cli.getStdout() : Cli.getStderr(); - result.getValidationEvents().stream() - .filter(event -> event.getSeverity() != Severity.SUPPRESSED) - .sorted() - .forEach(event -> { - if (event.getSeverity() == Severity.WARNING) { - Colors.YELLOW.write(writer, formatter.format(event)); - } else if (event.getSeverity() == Severity.DANGER || event.getSeverity() == Severity.ERROR) { - Colors.RED.write(writer, formatter.format(event)); - } else { - writer.accept(event.toString()); - } - writer.accept(""); - }); - long errors = result.getValidationEvents(Severity.ERROR).size(); long dangers = result.getValidationEvents(Severity.DANGER).size(); diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/ValidateCommandTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/ValidateCommandTest.java index 9b967762ea6..2d1dce0e809 100644 --- a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/ValidateCommandTest.java +++ b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/ValidateCommandTest.java @@ -17,15 +17,18 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; import java.io.ByteArrayOutputStream; import java.io.PrintStream; import java.net.URISyntaxException; +import java.nio.file.Path; import java.nio.file.Paths; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import software.amazon.smithy.cli.CliError; import software.amazon.smithy.cli.SmithyCli; +import software.amazon.smithy.model.validation.Severity; public class ValidateCommandTest { @Test @@ -72,4 +75,83 @@ public void allowsUnknownTrait() throws URISyntaxException { String model = Paths.get(getClass().getResource("unknown-trait.smithy").toURI()).toString(); SmithyCli.create().run("validate", "--allow-unknown-traits", model); } + + @Test + public void canSetSeverityToSuppressed() throws Exception { + String result = runValidationEventsTest(Severity.SUPPRESSED); + + assertThat(result, containsString("EmitSuppressed")); + assertThat(result, containsString("EmitNotes")); + assertThat(result, containsString("EmitWarnings")); + assertThat(result, containsString("EmitDangers")); + assertThat(result, containsString("TraitTarget")); + } + + @Test + public void canSetSeverityToNote() throws Exception { + String result = runValidationEventsTest(Severity.NOTE); + + assertThat(result, not(containsString("EmitSuppressed"))); + assertThat(result, containsString("EmitNotes")); + assertThat(result, containsString("EmitWarnings")); + assertThat(result, containsString("EmitDangers")); + assertThat(result, containsString("TraitTarget")); + } + + @Test + public void canSetSeverityToWarning() throws Exception { + String result = runValidationEventsTest(Severity.WARNING); + + assertThat(result, not(containsString("EmitSuppressed"))); + assertThat(result, not(containsString("EmitNotes"))); + assertThat(result, containsString("EmitWarnings")); + assertThat(result, containsString("EmitDangers")); + assertThat(result, containsString("TraitTarget")); + } + + @Test + public void canSetSeverityToDanger() throws Exception { + String result = runValidationEventsTest(Severity.DANGER); + + assertThat(result, not(containsString("EmitSuppressed"))); + assertThat(result, not(containsString("EmitNotes"))); + assertThat(result, not(containsString("EmitWarnings"))); + assertThat(result, containsString("EmitDangers")); + assertThat(result, containsString("TraitTarget")); + } + + @Test + public void canSetSeverityToError() throws Exception { + String result = runValidationEventsTest(Severity.ERROR); + + assertThat(result, not(containsString("EmitSuppressed"))); + assertThat(result, not(containsString("EmitNotes"))); + assertThat(result, not(containsString("EmitWarnings"))); + assertThat(result, not(containsString("EmitDangers"))); + assertThat(result, containsString("TraitTarget")); + } + + private String runValidationEventsTest(Severity severity) throws Exception { + PrintStream err = System.err; + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + PrintStream printStream = new PrintStream(outputStream); + System.setErr(printStream); + + Path validationEventsModel = Paths.get(getClass().getResource("validation-events.smithy").toURI()); + try { + SmithyCli.create().run("validate", "--severity", severity.toString(), validationEventsModel.toString()); + } catch (RuntimeException e) { + // ignore the error since everything we need was captured via stderr. + } + + System.setErr(err); + return outputStream.toString("UTF-8"); + } + + @Test + public void validatesSeverity() { + Assertions.assertThrows( + IllegalArgumentException.class, + () -> SmithyCli.create().run("validate", "--severity", "FOO")); + } } diff --git a/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/validation-events.smithy b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/validation-events.smithy new file mode 100644 index 00000000000..d7349eb5ebc --- /dev/null +++ b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/validation-events.smithy @@ -0,0 +1,50 @@ +$version: "1.0" + +metadata validators = [ + { + name: "EmitEachSelector", + id: "EmitSuppressed", + severity: "NOTE", + configuration: { + selector: "[id = smithy.example#Suppressed]" + } + }, + { + name: "EmitEachSelector", + id: "EmitNotes", + severity: "NOTE", + configuration: { + selector: "[id = smithy.example#Note]" + } + }, + { + name: "EmitEachSelector", + id: "EmitWarnings", + severity: "WARNING", + configuration: { + selector: "[id = smithy.example#Warning]" + } + }, + { + name: "EmitEachSelector", + id: "EmitDangers", + severity: "DANGER", + configuration: { + selector: "[id = smithy.example#Danger]" + } + } +] + +namespace smithy.example + +@suppress(["EmitSuppressed"]) +string Suppressed + +string Note + +string Warning + +string Danger + +@required // this trait is invalid and causes an error. +string 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 10155c1c047..0c3f526710e 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 @@ -33,6 +33,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.Consumer; import java.util.function.Supplier; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -85,6 +86,10 @@ public final class ModelAssembler { private static final Logger LOGGER = Logger.getLogger(ModelAssembler.class.getName()); + private static final Consumer DEFAULT_EVENT_LISTENER = ValidationEvent -> { + // Ignore events by default. + }; + private TraitFactory traitFactory; private ValidatorFactory validatorFactory; private boolean disableValidation; @@ -97,6 +102,7 @@ public final class ModelAssembler { private final Map metadata = new HashMap<>(); private final Map properties = new HashMap<>(); private boolean disablePrelude; + private Consumer validationEventListener = DEFAULT_EVENT_LISTENER; // Lazy initialization holder class idiom to hold a default validator factory. private static final class LazyValidatorFactoryHolder { @@ -128,6 +134,7 @@ public ModelAssembler copy() { assembler.disablePrelude = disablePrelude; assembler.properties.putAll(properties); assembler.disableValidation = disableValidation; + assembler.validationEventListener = validationEventListener; return assembler; } @@ -146,6 +153,7 @@ public ModelAssembler copy() { *
  • Shape registered via {@link #addModel}
    • *
  • Metadata registered via {@link #putMetadata}
    • *
  • Validation is re-enabled if it was disabled.
    • + *
  • Validation event listener via {@link #validationEventListener(Consumer)}
    • * * *

    The state of {@link #disablePrelude} is reset such that the prelude @@ -163,6 +171,7 @@ public ModelAssembler reset() { documentNodes.clear(); disablePrelude = false; disableValidation = false; + validationEventListener = null; return this; } @@ -475,6 +484,24 @@ public ModelAssembler disableValidation() { return this; } + /** + * Sets a listener that is invoked each time a ValidationEvent is encountered + * while loading and validating the model. + * + *

    The consumer could be invoked simultaneously by multiple threads. It's + * up to the consumer to perform any necessary synchronization. Providing + * an event listener is useful for things like CLIs so that events can + * be streamed to stdout as soon as they are encountered, rather than + * waiting until the entire model is parsed and validated. + * + * @param eventListener Listener invoked for each ValidationEvent. + * @return Returns the assembler. + */ + public ModelAssembler validationEventListener(Consumer eventListener) { + validationEventListener = eventListener == null ? DEFAULT_EVENT_LISTENER : eventListener; + return this; + } + /** * Assembles the model and returns the validated result. * @@ -599,6 +626,7 @@ private List createModelFiles() { private ValidatedResult validate(Model model, TraitContainer traits, List events) { validateTraits(model.getShapeIds(), traits, events); + events.forEach(validationEventListener); // If ERROR validation events occur while loading, then performing more // granular semantic validation will only obscure the root cause of errors. @@ -611,7 +639,10 @@ private ValidatedResult validate(Model model, TraitContainer traits, List } // Validate the model based on the explicit validators and model metadata. - List mergedEvents = ModelValidator.validate(model, validatorFactory, assembleValidators()); + // Note the ModelValidator handles emitting events to the validationEventListener. + List mergedEvents = ModelValidator + .validate(model, validatorFactory, assembleValidators(), validationEventListener); + mergedEvents.addAll(events); return new ValidatedResult<>(model, mergedEvents); } 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 index 41ec26b6afe..ecfac469a6e 100644 --- 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 @@ -23,6 +23,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.function.Consumer; import java.util.stream.Collectors; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.SourceLocation; @@ -72,16 +73,19 @@ final class ModelValidator { private final ValidatorFactory validatorFactory; private final Model model; private final Map> namespaceSuppressions = new HashMap<>(); + private final Consumer eventListener; private ModelValidator( Model model, ValidatorFactory validatorFactory, - List validators + List validators, + Consumer eventListener ) { this.model = model; this.validatorFactory = validatorFactory; this.validators = new ArrayList<>(validators); this.validators.removeIf(v -> CORE_VALIDATORS.contains(v.getClass())); + this.eventListener = eventListener; } /** @@ -91,14 +95,16 @@ private ModelValidator( * @param model Model to validate. * @param validatorFactory Factory used to find ValidatorService providers. * @param validators Additional validators to use. + * @param eventListener Consumer invoked each time a validation event is encountered. * @return Returns the encountered validation events. */ static List validate( Model model, ValidatorFactory validatorFactory, - List validators + List validators, + Consumer eventListener ) { - return new ModelValidator(model, validatorFactory, validators).doValidate(); + return new ModelValidator(model, validatorFactory, validators, eventListener).doValidate(); } private List doValidate() { @@ -111,6 +117,9 @@ private List doValidate() { // which will only obscure the root cause. events.addAll(new TargetValidator().validate(model)); events.addAll(new ResourceCycleValidator().validate(model)); + // Emit any events that have already occurred. + events.forEach(eventListener); + if (LoaderUtils.containsErrorEvents(events)) { return events; } @@ -120,6 +129,8 @@ private List doValidate() { .flatMap(validator -> validator.validate(model).stream()) .map(this::suppressEvent) .filter(ModelValidator::filterPrelude) + // Emit events as they occur during validation. + .peek(eventListener) .collect(Collectors.toList()); // Add in events encountered while building up validators and suppressions. 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 375cb1b71f8..aa9fe6ed3e1 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 @@ -37,6 +37,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; @@ -53,7 +54,6 @@ import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.node.ObjectNode; import software.amazon.smithy.model.node.StringNode; -import software.amazon.smithy.model.shapes.ModelSerializer; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.ShapeType; @@ -642,4 +642,21 @@ public void dedupesExactSameTraitsAndMetadataFromSameLocation() { assertThat(model, equalTo(model2)); } + + @Test + public void canListenToEvents() { + List toEmit = new ArrayList<>(); + toEmit.add(ValidationEvent.builder().id("a").severity(Severity.WARNING).message("").build()); + toEmit.add(ValidationEvent.builder().id("b").severity(Severity.WARNING).message("").build()); + toEmit.add(ValidationEvent.builder().id("c").severity(Severity.WARNING).message("").build()); + List collectedEvents = Collections.synchronizedList(new ArrayList<>()); + + Model.assembler() + .addValidator(model -> toEmit) + .validationEventListener(collectedEvents::add) + .assemble() + .unwrap(); + + assertThat(collectedEvents, equalTo(toEmit)); + } }