From f1ef09068823bc81c0884f0b9106131984952172 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Thu, 30 Mar 2023 20:17:56 -0700 Subject: [PATCH] Refactor CLI to remove --severity from some commands Some commands don't need the severity option and should instead only show validation events if the command fails. This includes the ast and select commands. To make this work, I refactored several CLI classes: * Add a shared ModeBuilder abstraction to cleanup the old utility code. This class now uses a Validator.Mode to indicate how validation is reported, decoupling arguments from the shared validation abstraction. * Move --discover and --discover-classpath to DiscoveryOptions * Move --severity to SeverityOption so it's more granular * Add the ability to check for receivers on Arguments * Add the ability to remove receivers from Arguments * Add the ability to get positional arguments more than once * Can now add/remove argument receivers in ClasspathCommand subclasses --- .../amazon/smithy/cli/AstCommandTest.java | 53 ++++++++ .../model-with-syntax-error/model/main.smithy | 6 + .../model-with-syntax-error/smithy-build.json | 4 + .../software/amazon/smithy/cli/Arguments.java | 73 +++++++---- .../smithy/cli/commands/AstCommand.java | 20 ++- .../smithy/cli/commands/BuildCommand.java | 17 ++- .../smithy/cli/commands/BuildOptions.java | 73 +---------- .../smithy/cli/commands/ClasspathCommand.java | 15 +-- .../smithy/cli/commands/CleanCommand.java | 6 +- .../smithy/cli/commands/DiffCommand.java | 7 +- .../smithy/cli/commands/DiscoveryOptions.java | 69 ++++++++++ .../{CommandUtils.java => ModelBuilder.java} | 122 ++++++++++++++---- .../smithy/cli/commands/SelectCommand.java | 27 ++-- .../smithy/cli/commands/SeverityOption.java | 76 +++++++++++ .../smithy/cli/commands/SimpleCommand.java | 15 +-- .../smithy/cli/commands/SmithyCommand.java | 2 +- .../cli/commands/Upgrade1to2Command.java | 19 ++- .../smithy/cli/commands/ValidateCommand.java | 11 +- .../smithy/cli/commands/ValidationFlag.java | 28 ---- .../amazon/smithy/cli/commands/Validator.java | 9 ++ .../smithy/cli/commands/WarmupCommand.java | 4 +- .../amazon/smithy/cli/ArgumentsTest.java | 12 +- 22 files changed, 454 insertions(+), 214 deletions(-) create mode 100644 smithy-cli/src/it/java/software/amazon/smithy/cli/AstCommandTest.java create mode 100644 smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/model/main.smithy create mode 100644 smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/smithy-build.json create mode 100644 smithy-cli/src/main/java/software/amazon/smithy/cli/commands/DiscoveryOptions.java rename smithy-cli/src/main/java/software/amazon/smithy/cli/commands/{CommandUtils.java => ModelBuilder.java} (68%) create mode 100644 smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SeverityOption.java delete mode 100644 smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidationFlag.java diff --git a/smithy-cli/src/it/java/software/amazon/smithy/cli/AstCommandTest.java b/smithy-cli/src/it/java/software/amazon/smithy/cli/AstCommandTest.java new file mode 100644 index 00000000000..8f273ef297b --- /dev/null +++ b/smithy-cli/src/it/java/software/amazon/smithy/cli/AstCommandTest.java @@ -0,0 +1,53 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.smithy.cli; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; + +import org.junit.jupiter.api.Test; +import software.amazon.smithy.utils.ListUtils; + +public class AstCommandTest { + @Test + public void validatesModelSuccess() { + IntegUtils.run("model-with-warning", ListUtils.of("ast"), result -> { + assertThat(result.getExitCode(), equalTo(0)); + assertThat(result.getOutput(), containsString("\"smithy\"")); + assertThat(result.getOutput(), not(containsString("WARNING"))); + }); + } + + @Test + public void convertsSyntacticallyCorrectModels() { + IntegUtils.run("invalid-model", ListUtils.of("ast"), result -> { + assertThat(result.getExitCode(), equalTo(0)); + assertThat(result.getOutput(), containsString("\"smithy\"")); + assertThat(result.getOutput(), not(containsString("WARNING"))); + }); + } + + @Test + public void showsErrorsForSyntacticallyIncorrectModels() { + IntegUtils.run("model-with-syntax-error", ListUtils.of("ast"), result -> { + assertThat(result.getExitCode(), equalTo(1)); + assertThat(result.getOutput(), containsString("ERROR")); + assertThat(result.getOutput(), containsString("bar // <- invalid syntax")); + }); + } +} diff --git a/smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/model/main.smithy b/smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/model/main.smithy new file mode 100644 index 00000000000..fc88412ce8d --- /dev/null +++ b/smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/model/main.smithy @@ -0,0 +1,6 @@ +$version: "2.0" +namespace smithy.example + +structure Foo { + bar // <- invalid syntax +} diff --git a/smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/smithy-build.json b/smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/smithy-build.json new file mode 100644 index 00000000000..db4a958216c --- /dev/null +++ b/smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/smithy-build.json @@ -0,0 +1,4 @@ +{ + "version": "1.0", + "sources": ["model"] +} diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/Arguments.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/Arguments.java index d4cf2ddb7f6..6cececd91ba 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/Arguments.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/Arguments.java @@ -27,9 +27,9 @@ * Command line arguments list to evaluate. * *

Arguments are parsed on demand. To finalize parsing arguments and - * force validation of remaining arguments, call {@link #finishParsing()}. + * force validation of remaining arguments, call {@link #getPositional()}. * Note that because arguments are parsed on demand and whole set of - * arguments isn't known before {@link #finishParsing()} is called, you + * arguments isn't known before {@link #getPositional()} is called, you * may need to delay configuring parts of the CLI or commands by adding * subscribers via {@link #onComplete(BiConsumer)}. These subscribers are * invoked when all arguments have been parsed. @@ -39,6 +39,7 @@ public final class Arguments { private final String[] args; private final Map, ArgumentReceiver> receivers = new LinkedHashMap<>(); private final List>> subscribers = new ArrayList<>(); + private List positional; private boolean inPositional = false; private int position = 0; @@ -55,6 +56,28 @@ public void addReceiver(T receiver) { receivers.put(receiver.getClass(), receiver); } + /** + * Removes an argument receiver by class. + * + * @param receiverClass Class of receiver to remove. + * @return Returns the removed receiver if found, or null. + * @param Kind of receiver to remove. + */ + @SuppressWarnings("unchecked") + public T removeReceiver(Class receiverClass) { + return (T) receivers.remove(receiverClass); + } + + /** + * Check if this class contains a receiver. + * + * @param receiverClass Class of receiver to detect. + * @return Returns true if found. + */ + public boolean hasReceiver(Class receiverClass) { + return receivers.containsKey(receiverClass); + } + /** * Get a receiver by class. * @@ -97,7 +120,7 @@ public boolean hasNext() { } /** - * Peeks at the next value in the arguments list without shifting it. + * Peeks at the next value in the argument list without shifting it. * *

Note that arguments consumed by a {@link ArgumentReceiver} are never * peeked. @@ -134,7 +157,7 @@ public String peek() { } /** - * Shifts off the next value in the arguments list or returns null. + * Shifts off the next value in the argument list or returns null. * * @return Returns the next value or null. */ @@ -166,41 +189,41 @@ public String shiftFor(String parameter) { } /** - * Expects that all remaining arguments are positional, and returns them. + * Gets the positional arguments. + * + *

Expects that all remaining arguments are positional, and returns them. * *

If an argument is "--", then it's skipped and remaining arguments are * considered positional. If any argument is encountered that isn't valid * for a registered Receiver, then an error is raised. Otherwise, all remaining * arguments are returned in a list. * - *

Subscribers for different receivers are called when this method is called. + *

Subscribers for different receivers are called when this method is first called. * * @return Returns remaining arguments. * @throws CliError if the next argument starts with "--" but isn't "--". */ - public List finishParsing() { - List positional = new ArrayList<>(); - - while (hasNext()) { - String next = shift(); - if (next != null) { - if (!inPositional && next.startsWith("-")) { - throw new CliError("Unexpected CLI argument: " + next); - } else { - inPositional = true; - positional.add(next); + public List getPositional() { + if (positional == null) { + positional = new ArrayList<>(); + + while (hasNext()) { + String next = shift(); + if (next != null) { + if (!inPositional && next.startsWith("-")) { + throw new CliError("Unexpected CLI argument: " + next); + } else { + inPositional = true; + positional.add(next); + } } } - } - invokeSubscribers(positional); + for (BiConsumer> subscriber : subscribers) { + subscriber.accept(this, positional); + } + } return positional; } - - private void invokeSubscribers(List positional) { - for (BiConsumer> subscriber : subscribers) { - subscriber.accept(this, positional); - } - } } diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/AstCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/AstCommand.java index 6f3551b36a0..bbfc31acb76 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/AstCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/AstCommand.java @@ -22,6 +22,7 @@ import software.amazon.smithy.model.Model; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.shapes.ModelSerializer; +import software.amazon.smithy.model.validation.Severity; final class AstCommand extends ClasspathCommand { @@ -39,9 +40,26 @@ public String getSummary() { return "Reads Smithy models in and writes out a single JSON AST model."; } + @Override + protected void configureArgumentReceivers(Arguments arguments) { + super.configureArgumentReceivers(arguments); + + // The AST command isn't meant for validation. Events are only shown when they fail the command. + arguments.removeReceiver(SeverityOption.class); + } + @Override int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, List models) { - Model model = CommandUtils.buildModel(arguments, models, env, env.stderr(), ValidationFlag.QUIET, config); + Model model = new ModelBuilder() + .config(config) + .arguments(arguments) + .env(env) + .models(models) + .validationPrinter(env.stderr()) + .validationMode(Validator.Mode.QUIET) + .severity(Severity.DANGER) + .build(); + ModelSerializer serializer = ModelSerializer.builder().build(); env.stdout().println(Node.prettyPrintJson(serializer.serialize(model))); return 0; 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 bba9516b6de..b2854dd5c5e 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 @@ -89,8 +89,9 @@ public void registerHelp(HelpPrinter printer) { } @Override - protected void addAdditionalArgumentReceivers(List receivers) { - receivers.add(new Options()); + protected void configureArgumentReceivers(Arguments arguments) { + super.configureArgumentReceivers(arguments); + arguments.addReceiver(new Options()); } @Override @@ -99,11 +100,13 @@ int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, L BuildOptions buildOptions = arguments.getReceiver(BuildOptions.class); StandardOptions standardOptions = arguments.getReceiver(StandardOptions.class); ClassLoader classLoader = env.classLoader(); - - // Build the model and fail if there are errors. Prints errors to stdout. - // Configure whether the build is quiet or not based on the --quiet option. - ValidationFlag flag = ValidationFlag.from(standardOptions); - Model model = CommandUtils.buildModel(arguments, models, env, env.stderr(), flag, config); + Model model = new ModelBuilder() + .config(config) + .arguments(arguments) + .env(env) + .models(models) + .validationPrinter(env.stderr()) + .build(); Supplier modelAssemblerSupplier = () -> { ModelAssembler assembler = Model.assembler(classLoader); diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildOptions.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildOptions.java index dc2a3ff98e7..6b12236185f 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildOptions.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildOptions.java @@ -17,37 +17,22 @@ import java.util.function.Consumer; import software.amazon.smithy.cli.ArgumentReceiver; -import software.amazon.smithy.cli.CliError; import software.amazon.smithy.cli.HelpPrinter; -import software.amazon.smithy.cli.StandardOptions; -import software.amazon.smithy.model.validation.Severity; /** * Arguments available to commands that load and build models. */ final class BuildOptions implements ArgumentReceiver { - static final String SEVERITY = "--severity"; static final String ALLOW_UNKNOWN_TRAITS = "--allow-unknown-traits"; static final String MODELS = ""; - private Severity severity; - private String discoverClasspath; private boolean allowUnknownTraits; - private boolean discover; private String output; @Override public void registerHelp(HelpPrinter printer) { - printer.param(SEVERITY, null, "SEVERITY", "Set the minimum reported validation severity (one of NOTE, " - + "WARNING [default setting], DANGER, ERROR)."); printer.option(ALLOW_UNKNOWN_TRAITS, null, "Ignore unknown traits when validating models"); - /* - Hide these for now until we figure out a plan forward for these. - printer.option(DISCOVER, "-d", "Enable model discovery, merging in models found inside of jars"); - printer.param(DISCOVER_CLASSPATH, null, "CLASSPATH", - "Enable model discovery using a custom classpath for models"); - */ printer.param("--output", null, "OUTPUT_PATH", "Where to write Smithy artifacts, caches, and other files (defaults to './build/smithy')."); printer.positional(MODELS, "Model files and directories to load"); @@ -55,70 +40,26 @@ public void registerHelp(HelpPrinter printer) { @Override public boolean testOption(String name) { - switch (name) { - case ALLOW_UNKNOWN_TRAITS: - allowUnknownTraits = true; - return true; - case "--discover": - case "-d": - discover = true; - return true; - default: - return false; + if (ALLOW_UNKNOWN_TRAITS.equals(name)) { + allowUnknownTraits = true; + return true; } + return false; } @Override public Consumer testParameter(String name) { - switch (name) { - case "--output": - return value -> output = value; - case "--discover-classpath": - return value -> discoverClasspath = value; - case SEVERITY: - return value -> { - severity(Severity.fromString(value).orElseThrow(() -> { - return new CliError("Invalid severity level: " + value); - })); - }; - default: - return null; + if ("--output".equals(name)) { + return value -> output = value; } - } - - String discoverClasspath() { - return discoverClasspath; + return null; } boolean allowUnknownTraits() { return allowUnknownTraits; } - boolean discover() { - return discover; - } - String output() { return output; } - - void severity(Severity severity) { - this.severity = severity; - } - - /** - * Get the severity level, taking into account standard options that affect the default. - * - * @param options Standard options to query if no severity is explicitly set. - * @return Returns the resolved severity option. - */ - Severity severity(StandardOptions options) { - if (severity != null) { - return severity; - } else if (options.quiet()) { - return Severity.DANGER; - } else { - return Severity.WARNING; - } - } } diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ClasspathCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ClasspathCommand.java index e1fa3d5de1b..5d44c72c1a7 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ClasspathCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ClasspathCommand.java @@ -28,7 +28,6 @@ import software.amazon.smithy.build.model.MavenConfig; import software.amazon.smithy.build.model.MavenRepository; import software.amazon.smithy.build.model.SmithyBuildConfig; -import software.amazon.smithy.cli.ArgumentReceiver; import software.amazon.smithy.cli.Arguments; import software.amazon.smithy.cli.CliError; import software.amazon.smithy.cli.EnvironmentVariable; @@ -53,12 +52,11 @@ abstract class ClasspathCommand extends SimpleCommand { } @Override - protected final List createArgumentReceivers() { - List receivers = new ArrayList<>(); - receivers.add(new ConfigOptions()); - receivers.add(new BuildOptions()); - addAdditionalArgumentReceivers(receivers); - return receivers; + protected void configureArgumentReceivers(Arguments arguments) { + arguments.addReceiver(new ConfigOptions()); + arguments.addReceiver(new DiscoveryOptions()); + arguments.addReceiver(new SeverityOption()); + arguments.addReceiver(new BuildOptions()); } @Override @@ -80,9 +78,6 @@ private static final class ThreadResult { int returnCode; } - protected void addAdditionalArgumentReceivers(List receivers) { - } - abstract int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, List positional); private void runTaskWithClasspath( diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/CleanCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/CleanCommand.java index 2bd6c1584f0..e364365cf9c 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/CleanCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/CleanCommand.java @@ -21,10 +21,8 @@ import java.util.logging.Logger; import software.amazon.smithy.build.SmithyBuild; import software.amazon.smithy.build.model.SmithyBuildConfig; -import software.amazon.smithy.cli.ArgumentReceiver; import software.amazon.smithy.cli.Arguments; import software.amazon.smithy.utils.IoUtils; -import software.amazon.smithy.utils.ListUtils; final class CleanCommand extends SimpleCommand { @@ -35,8 +33,8 @@ final class CleanCommand extends SimpleCommand { } @Override - protected List createArgumentReceivers() { - return ListUtils.of(new ConfigOptions()); + protected void configureArgumentReceivers(Arguments arguments) { + arguments.addReceiver(new ConfigOptions()); } @Override diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/DiffCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/DiffCommand.java index 0f6a0a63628..3aace9c84a6 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/DiffCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/DiffCommand.java @@ -87,8 +87,9 @@ public void registerHelp(HelpPrinter printer) { } @Override - protected void addAdditionalArgumentReceivers(List receivers) { - receivers.add(new Options()); + protected void configureArgumentReceivers(Arguments arguments) { + super.configureArgumentReceivers(arguments); + arguments.addReceiver(new Options()); } @Override @@ -101,7 +102,7 @@ int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, L List newModels = options.newModels; LOGGER.fine(() -> String.format("Setting old models to: %s; new models to: %s", oldModels, newModels)); - ModelAssembler assembler = CommandUtils.createModelAssembler(classLoader); + ModelAssembler assembler = ModelBuilder.createModelAssembler(classLoader); Model oldModel = loadModel("old", assembler, oldModels); assembler.reset(); Model newModel = loadModel("new", assembler, newModels); diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/DiscoveryOptions.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/DiscoveryOptions.java new file mode 100644 index 00000000000..8d122775be0 --- /dev/null +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/DiscoveryOptions.java @@ -0,0 +1,69 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.smithy.cli.commands; + +import java.util.function.Consumer; +import software.amazon.smithy.cli.ArgumentReceiver; +import software.amazon.smithy.cli.HelpPrinter; + +/** + * Arguments available to commands that use model discovery. + * + *

TODO: It would be great to just always do model discovery and remove these hidden options. + */ +final class DiscoveryOptions implements ArgumentReceiver { + + private String discoverClasspath; + private boolean discover; + + @Override + public void registerHelp(HelpPrinter printer) { + /* + Hide these for now until we figure out a plan forward for these. + printer.option("--discover", "-d", "Enable model discovery, merging in models found inside of jars"); + printer.param("--discover-classpath", null, "CLASSPATH", + "Enable model discovery using a custom classpath for models"); + */ + } + + @Override + public boolean testOption(String name) { + switch (name) { + case "--discover": + case "-d": + discover = true; + return true; + default: + return false; + } + } + + @Override + public Consumer testParameter(String name) { + if (name.equals("--discover-classpath")) { + return value -> discoverClasspath = value; + } + return null; + } + + String discoverClasspath() { + return discoverClasspath; + } + + boolean discover() { + return discover; + } +} 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/ModelBuilder.java similarity index 68% rename from smithy-cli/src/main/java/software/amazon/smithy/cli/commands/CommandUtils.java rename to smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ModelBuilder.java index 877c59ab379..ea0a722a598 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/ModelBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"). * You may not use this file except in compliance with the License. @@ -39,32 +39,84 @@ 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.utils.SmithyBuilder; -final class CommandUtils { +/** + * Loads, builds, and report validation issues with models. + */ +final class ModelBuilder { - private static final Logger LOGGER = Logger.getLogger(CommandUtils.class.getName()); + private static final Logger LOGGER = Logger.getLogger(ModelBuilder.class.getName()); private static final String CLEAR_LINE_ESCAPE = "\033[2K\r"; private static final int DEFAULT_CODE_LINES = 6; - private CommandUtils() {} + private Validator.Mode validationMode; + private CliPrinter validationPrinter; + private Arguments arguments; + private List models; + private Command.Env env; + private SmithyBuildConfig config; + private Severity severity; + + public ModelBuilder arguments(Arguments arguments) { + this.arguments = arguments; + return this; + } + + public ModelBuilder models(List models) { + this.models = models; + return this; + } + + public ModelBuilder env(Command.Env env) { + this.env = env; + return this; + } + + public ModelBuilder validationPrinter(CliPrinter validationPrinter) { + this.validationPrinter = validationPrinter; + return this; + } + + public ModelBuilder validationMode(Validator.Mode validationMode) { + this.validationMode = validationMode; + return this; + } + + public ModelBuilder config(SmithyBuildConfig config) { + this.config = config; + return this; + } + + public ModelBuilder severity(Severity severity) { + this.severity = severity; + return this; + } + + public Model build() { + SmithyBuilder.requiredState("arguments", arguments); + SmithyBuilder.requiredState("models", models); + SmithyBuilder.requiredState("env", env); + SmithyBuilder.requiredState("config", config); - static Model buildModel( - Arguments arguments, - List models, - Command.Env env, - CliPrinter printer, - ValidationFlag validationFlag, - SmithyBuildConfig config - ) { - ClassLoader classLoader = env.classLoader(); - ModelAssembler assembler = CommandUtils.createModelAssembler(classLoader); - ColorFormatter colors = env.colors(); StandardOptions standardOptions = arguments.getReceiver(StandardOptions.class); BuildOptions buildOptions = arguments.getReceiver(BuildOptions.class); - Severity minSeverity = buildOptions.severity(standardOptions); + DiscoveryOptions discoveryOptions = arguments.getReceiver(DiscoveryOptions.class); + Severity minSeverity = resolveMinSeverity(standardOptions); + ClassLoader classLoader = env.classLoader(); + ModelAssembler assembler = createModelAssembler(classLoader); + ColorFormatter colors = env.colors(); CliPrinter stderr = env.stderr(); - if (validationFlag == ValidationFlag.DISABLE) { + if (validationPrinter == null) { + validationPrinter = env.stderr(); + } + + if (validationMode == null) { + validationMode = Validator.Mode.from(standardOptions); + } + + if (validationMode == Validator.Mode.DISABLE) { assembler.disableValidation(); } @@ -72,8 +124,8 @@ static Model buildModel( AtomicInteger issueCount = new AtomicInteger(); assembler.validationEventListener(createStatusUpdater(standardOptions, colors, stderr, issueCount)); - CommandUtils.handleModelDiscovery(buildOptions, assembler, classLoader, config); - CommandUtils.handleUnknownTraitsOption(buildOptions, assembler); + handleModelDiscovery(discoveryOptions, assembler, classLoader, config); + handleUnknownTraitsOption(buildOptions, assembler); config.getSources().forEach(assembler::addImport); models.forEach(assembler::addImport); config.getImports().forEach(assembler::addImport); @@ -94,12 +146,17 @@ static Model buildModel( // Only log events that are >= --severity. Note that setting --quiet inherently // configures events to need to be >= DANGER. if (event.getSeverity().ordinal() >= minSeverity.ordinal()) { - printer.println(formatter.format(event)); + validationPrinter.println(formatter.format(event)); } } // Note: disabling validation will still show a summary of failures if the model can't be loaded. - Validator.validate(validationFlag != ValidationFlag.ENABLE, colors, stderr, result); + Validator.validate(validationMode != Validator.Mode.ENABLE, colors, stderr, result); + + // Flush outputs to ensure there is no interleaving with subsequent command output. + env.stderr().flush(); + env.stdout().flush(); + return result.getResult().orElseThrow(() -> new RuntimeException("Expected Validator to throw")); } @@ -135,10 +192,6 @@ static void clearStatusUpdateIfPresent(AtomicInteger issueCount, CliPrinter stde } } - static ModelAssembler createModelAssembler(ClassLoader classLoader) { - return Model.assembler(classLoader).putProperty(ModelAssembler.DISABLE_JAR_CACHE, true); - } - private static void handleUnknownTraitsOption(BuildOptions options, ModelAssembler assembler) { if (options.allowUnknownTraits()) { LOGGER.fine("Ignoring unknown traits"); @@ -147,7 +200,7 @@ private static void handleUnknownTraitsOption(BuildOptions options, ModelAssembl } private static void handleModelDiscovery( - BuildOptions options, + DiscoveryOptions options, ModelAssembler assembler, ClassLoader baseLoader, SmithyBuildConfig config @@ -159,7 +212,7 @@ private static void handleModelDiscovery( } } - private static boolean shouldDiscoverDependencies(BuildOptions options, SmithyBuildConfig config) { + private static boolean shouldDiscoverDependencies(DiscoveryOptions options, SmithyBuildConfig config) { if (options.discover()) { return true; } else { @@ -186,4 +239,19 @@ private static void discoverModelsWithClasspath(String rawClasspath, ModelAssemb URLClassLoader urlClassLoader = new URLClassLoader(urls); assembler.discoverModels(urlClassLoader); } + + // Determine a default severity if one wasn't given, by inspecting if there is a --severity option. + private Severity resolveMinSeverity(StandardOptions standardOptions) { + if (severity != null) { + return severity; + } else if (arguments.hasReceiver(SeverityOption.class)) { + return arguments.getReceiver(SeverityOption.class).severity(standardOptions); + } else { + return Severity.WARNING; + } + } + + 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/SelectCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java index 6c60987d21d..b60c7efb80f 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java @@ -134,24 +134,23 @@ public Selector selector() { } @Override - protected void addAdditionalArgumentReceivers(List receivers) { - receivers.add(new Options()); + protected void configureArgumentReceivers(Arguments arguments) { + super.configureArgumentReceivers(arguments); + arguments.addReceiver(new Options()); + arguments.removeReceiver(SeverityOption.class); // only show build-failing errors when selecting. } @Override int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, List models) { - // Don't write the summary, but do write danger/errors to STDERR. - ValidationFlag flag = ValidationFlag.DISABLE; - - // Force the severity to DANGER or ERROR to only see events if they'll fail the command. - BuildOptions buildOptions = arguments.getReceiver(BuildOptions.class); - buildOptions.severity(Severity.DANGER); - - Model model = CommandUtils.buildModel(arguments, models, env, env.stderr(), flag, config); - - // Flush outputs to ensure there is no interleaving with selection output. - env.stderr().flush(); - env.stdout().flush(); + Model model = new ModelBuilder() + .config(config) + .arguments(arguments) + .env(env) + .models(models) + .validationPrinter(env.stderr()) + .validationMode(Validator.Mode.DISABLE) + .severity(Severity.DANGER) + .build(); Options options = arguments.getReceiver(Options.class); Selector selector = options.selector(); diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SeverityOption.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SeverityOption.java new file mode 100644 index 00000000000..febca698a33 --- /dev/null +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SeverityOption.java @@ -0,0 +1,76 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.smithy.cli.commands; + +import java.util.function.Consumer; +import software.amazon.smithy.cli.ArgumentReceiver; +import software.amazon.smithy.cli.CliError; +import software.amazon.smithy.cli.HelpPrinter; +import software.amazon.smithy.cli.StandardOptions; +import software.amazon.smithy.model.validation.Severity; + +/** + * Add the --severity option. + */ +final class SeverityOption implements ArgumentReceiver { + + static final String SEVERITY = "--severity"; + + private Severity severity; + + @Override + public void registerHelp(HelpPrinter printer) { + printer.param(SEVERITY, null, "SEVERITY", "Set the minimum reported validation severity (one of NOTE, " + + "WARNING [default setting], DANGER, ERROR)."); + } + + @Override + public Consumer testParameter(String name) { + if (SEVERITY.equals(name)) { + return value -> { + severity(Severity.fromString(value).orElseThrow(() -> { + return new CliError("Invalid severity level: " + value); + })); + }; + } + return null; + } + + /** + * Set the severity. + * + * @param severity Severity to set. + */ + void severity(Severity severity) { + this.severity = severity; + } + + /** + * Get the severity level, taking into account standard options that affect the default. + * + * @param options Standard options to query if no severity is explicitly set. + * @return Returns the resolved severity option. + */ + Severity severity(StandardOptions options) { + if (severity != null) { + return severity; + } else if (options.quiet()) { + return Severity.DANGER; + } else { + return Severity.WARNING; + } + } +} diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SimpleCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SimpleCommand.java index 2ed37a90c57..74b11ebafa5 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SimpleCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SimpleCommand.java @@ -17,7 +17,6 @@ import java.util.List; import java.util.logging.Logger; -import software.amazon.smithy.cli.ArgumentReceiver; import software.amazon.smithy.cli.Arguments; import software.amazon.smithy.cli.CliError; import software.amazon.smithy.cli.CliPrinter; @@ -44,12 +43,8 @@ abstract class SimpleCommand implements Command { @Override public final int execute(Arguments arguments, Env env) { - List receivers = createArgumentReceivers(); - for (ArgumentReceiver receiver : receivers) { - arguments.addReceiver(receiver); - } - - List positionalArguments = arguments.finishParsing(); + configureArgumentReceivers(arguments); + List positionalArguments = arguments.getPositional(); StandardOptions options = arguments.getReceiver(StandardOptions.class); @@ -78,11 +73,11 @@ public void printHelp(Arguments arguments, ColorFormatter colors, CliPrinter pri } /** - * Creates argument receivers for the command. + * Configure argument receivers for the command. * - * @return Returns the parsed positional arguments. + * @param arguments Arguments to add/remove argument receivers from. */ - protected abstract List createArgumentReceivers(); + protected abstract void configureArgumentReceivers(Arguments arguments); /** * Run the non-help command after all arguments have been parsed. diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SmithyCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SmithyCommand.java index 594e98a3ce5..43b315fcf97 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SmithyCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SmithyCommand.java @@ -93,7 +93,7 @@ public int execute(Arguments arguments, Env env) { // If no command was given, then finish parsing to check if -h, --help, or --version was given. if (command == null) { - arguments.finishParsing(); + arguments.getPositional(); StandardOptions standardOptions = arguments.getReceiver(StandardOptions.class); diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java index 603fd16ec27..28dceefa0fc 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java @@ -38,10 +38,8 @@ import software.amazon.smithy.build.ProjectionResult; import software.amazon.smithy.build.SmithyBuild; import software.amazon.smithy.build.model.SmithyBuildConfig; -import software.amazon.smithy.cli.ArgumentReceiver; import software.amazon.smithy.cli.Arguments; import software.amazon.smithy.cli.CliError; -import software.amazon.smithy.cli.StandardOptions; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.loader.ModelAssembler; @@ -58,6 +56,7 @@ import software.amazon.smithy.model.traits.EnumTrait; import software.amazon.smithy.model.traits.Trait; import software.amazon.smithy.model.transform.ModelTransformer; +import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.utils.IoUtils; import software.amazon.smithy.utils.SimpleParser; import software.amazon.smithy.utils.StringUtils; @@ -73,8 +72,10 @@ final class Upgrade1to2Command extends SimpleCommand { } @Override - protected List createArgumentReceivers() { - return Arrays.asList(new ConfigOptions(), new BuildOptions()); + protected void configureArgumentReceivers(Arguments arguments) { + arguments.addReceiver(new ConfigOptions()); + arguments.addReceiver(new BuildOptions()); + arguments.addReceiver(new DiscoveryOptions()); } @Override @@ -105,8 +106,14 @@ protected int run(Arguments arguments, Env env, List models) { configBuilder.outputDirectory(tempDir.toString()); SmithyBuildConfig temporaryConfig = configBuilder.build(); - ValidationFlag flag = ValidationFlag.from(arguments.getReceiver(StandardOptions.class)); - Model initialModel = CommandUtils.buildModel(arguments, models, env, env.stderr(), flag, smithyBuildConfig); + Model initialModel = new ModelBuilder() + .config(smithyBuildConfig) + .arguments(arguments) + .env(env) + .models(models) + .validationPrinter(env.stderr()) + .severity(Severity.DANGER) + .build(); SmithyBuild smithyBuild = SmithyBuild.create(classLoader) .config(temporaryConfig) 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 e0b89316b8c..70a25682fb4 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 @@ -19,7 +19,6 @@ import java.util.logging.Logger; import software.amazon.smithy.build.model.SmithyBuildConfig; import software.amazon.smithy.cli.Arguments; -import software.amazon.smithy.cli.StandardOptions; import software.amazon.smithy.cli.dependencies.DependencyResolver; final class ValidateCommand extends ClasspathCommand { @@ -42,9 +41,13 @@ public String getSummary() { @Override int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, List models) { - StandardOptions standardOptions = arguments.getReceiver(StandardOptions.class); - ValidationFlag flag = ValidationFlag.from(standardOptions); - CommandUtils.buildModel(arguments, models, env, env.stdout(), flag, config); + new ModelBuilder() + .config(config) + .arguments(arguments) + .env(env) + .models(models) + .validationPrinter(env.stdout()) + .build(); LOGGER.info("Smithy validation complete"); return 0; } diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidationFlag.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidationFlag.java deleted file mode 100644 index 9107cd2fe7b..00000000000 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidationFlag.java +++ /dev/null @@ -1,28 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"). - * You may not use this file except in compliance with the License. - * A copy of the License is located at - * - * http://aws.amazon.com/apache2.0 - * - * or in the "license" file accompanying this file. This file is distributed - * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either - * express or implied. See the License for the specific language governing - * permissions and limitations under the License. - */ - -package software.amazon.smithy.cli.commands; - -import software.amazon.smithy.cli.StandardOptions; - -enum ValidationFlag { - QUIET, DISABLE, ENABLE; - - static ValidationFlag from(StandardOptions standardOptions) { - return standardOptions.quiet() - ? ValidationFlag.QUIET - : ValidationFlag.ENABLE; - } -} 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 24705f00652..99f5906d4dc 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 @@ -19,6 +19,7 @@ import software.amazon.smithy.cli.CliError; import software.amazon.smithy.cli.CliPrinter; import software.amazon.smithy.cli.ColorFormatter; +import software.amazon.smithy.cli.StandardOptions; import software.amazon.smithy.cli.Style; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.validation.Severity; @@ -29,6 +30,14 @@ */ final class Validator { + enum Mode { + QUIET, DISABLE, ENABLE; + + static Mode from(StandardOptions standardOptions) { + return standardOptions.quiet() ? Mode.QUIET : Mode.ENABLE; + } + } + private Validator() {} static void validate(boolean quiet, ColorFormatter colors, CliPrinter printer, ValidatedResult result) { diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/WarmupCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/WarmupCommand.java index 148feade162..49ebef0eb1e 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/WarmupCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/WarmupCommand.java @@ -70,8 +70,8 @@ public boolean isHidden() { } @Override - protected List createArgumentReceivers() { - return Collections.singletonList(new Config()); + protected void configureArgumentReceivers(Arguments arguments) { + arguments.addReceiver(new Config()); } @Override diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/ArgumentsTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/ArgumentsTest.java index 1af13b8f061..23e0b82637e 100644 --- a/smithy-cli/src/test/java/software/amazon/smithy/cli/ArgumentsTest.java +++ b/smithy-cli/src/test/java/software/amazon/smithy/cli/ArgumentsTest.java @@ -53,14 +53,14 @@ public void throwsWhenExpectedValueNotPresent() { public void allArgumentsMustHaveReceiver() { Arguments arguments = new Arguments(new String[]{"--a", "--b", "--c"}); - Assertions.assertThrows(CliError.class, arguments::finishParsing); + Assertions.assertThrows(CliError.class, arguments::getPositional); } @Test public void evenSingleHyphenArgumentsMustHaveReceiver() { Arguments arguments = new Arguments(new String[]{"-h"}); - Assertions.assertThrows(CliError.class, arguments::finishParsing); + Assertions.assertThrows(CliError.class, arguments::getPositional); } @Test @@ -90,7 +90,7 @@ public Consumer testParameter(String name) { } }); - assertThat(arguments.finishParsing(), contains("foo", "bar")); + assertThat(arguments.getPositional(), contains("foo", "bar")); assertThat(received.keySet(), contains("--a", "--b", "--c")); assertThat(received.values(), contains("1", null, "2")); } @@ -104,7 +104,7 @@ public void emitsOnCompleteEvents() { received.addAll(positional); }); - assertThat(arguments.finishParsing(), contains("foo", "bar")); + assertThat(arguments.getPositional(), contains("foo", "bar")); assertThat(received, contains("foo", "bar")); } @@ -115,7 +115,7 @@ public void considersDoubleHyphenBeginningOfPositionalArgs() { assertThat(arguments.hasNext(), is(true)); // Inherently skips "--" because it's handled by Arguments. assertThat(arguments.peek(), equalTo("--bar")); - assertThat(arguments.finishParsing(), contains("--bar")); + assertThat(arguments.getPositional(), contains("--bar")); } @Test @@ -124,7 +124,7 @@ public void canGetReceiversByClass() { Arguments arguments = new Arguments(new String[]{"--help"}); arguments.addReceiver(options); - arguments.finishParsing(); + arguments.getPositional(); assertThat(arguments.getReceiver(StandardOptions.class), sameInstance(options)); assertThat(arguments.getReceiver(StandardOptions.class).help(), is(true)); }