Skip to content

Commit

Permalink
Add --mode arbitrary|project to smithy diff
Browse files Browse the repository at this point in the history
This commit adds a --mode option to Smithy diff to support the
arbitrary mode (the current mode used to compare two models), and
the project mode used to compare the current project against another
project or model. When running in project mode, the imports and
sources of the current model are used to load the "new" model
(the current project), but they aren't used to load the "old" model.
If the --old argument points to a directory that contains a
smithy-build.json file, its imports and sources are used when loading
the old model, though its dependencies are ignored and it's loaded
using the resolved classpath of the new model. This ensures that the
models are comparable and won't cause comparison issues when trying
to compare things like traits across class loaders.
  • Loading branch information
mtdowling committed Apr 6, 2023
1 parent 813b2a5 commit 9b16244
Show file tree
Hide file tree
Showing 10 changed files with 239 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;

import java.io.FileWriter;
import java.io.IOException;
Expand Down Expand Up @@ -120,4 +121,45 @@ public void requiresOldAndNewForArbitraryMode() {
assertThat(result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("Missing required --old argument"));
}

@Test
public void doesNotAllowNewWithProjectMode() {
RunResult result = IntegUtils.run(Paths.get("."), ListUtils.of("diff",
"--mode", "project",
"--new", "x",
"--old", "y"));

assertThat(result.getExitCode(), is(1));
assertThat(result.getOutput(), containsString("--new cannot be used with this diff mode"));
}

@Test
public void projectModeUsesConfigOfOldModel() {
IntegUtils.withProject("diff-example-conflict-with-simple", outer -> {
IntegUtils.withProject("simple-config-sources", dir -> {
RunResult result = IntegUtils.run(dir, ListUtils.of(
"diff",
"--mode",
"project",
"--old",
outer.toString()));
assertThat(result.getOutput(), containsString("ChangedShapeType"));
assertThat(result.getExitCode(), equalTo(1));
});
});
}

@Test
public void projectModeCanDiffAgainstSingleFile() {
// Diff against itself (the only model file of the project), so there should be no differences.
IntegUtils.withProject("simple-config-sources", dir -> {
RunResult result = IntegUtils.run(dir, ListUtils.of(
"diff",
"--mode",
"project",
"--old",
dir.resolve("model").resolve("main.smithy").toString()));
assertThat(result.getExitCode(), equalTo(0));
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
$version: "2.0"
namespace smithy.example

// See the DiffCommandTest#projectModeUsesConfigOfOldModel integration test.
//
// This is to cause a diff event when compared against simple-config-sources, ensuring that the config file of this
// project is used and not the config file of the "new" model. If the new model's config was also loaded, the
// integration test would create a shape conflict error after loading.
integer MyString
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"version": "1.0",
"sources": ["model"]
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ final class ConfigOptions implements ArgumentReceiver {
private static final Logger LOGGER = Logger.getLogger(ConfigOptions.class.getName());
private final List<String> config = new ArrayList<>();
private boolean noConfig = false;
private Path root;

@Override
public void registerHelp(HelpPrinter printer) {
Expand Down Expand Up @@ -65,12 +66,18 @@ public boolean testOption(String name) {
}
}

void root(Path root) {
this.root = root;
}

List<String> config() {
List<String> config = this.config;

// Don't find the default config if --no-config is passed.
if (config.isEmpty() && !noConfig) {
Path defaultConfig = Paths.get("smithy-build.json").toAbsolutePath();
Path defaultConfig = root != null
? root.resolve("smithy-build.json").toAbsolutePath()
: Paths.get("smithy-build.json").toAbsolutePath();
if (Files.exists(defaultConfig)) {
LOGGER.fine("Detected smithy-build.json at " + defaultConfig);
config = Collections.singletonList(defaultConfig.toString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,16 @@

package software.amazon.smithy.cli.commands;

import java.nio.file.Paths;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.function.Consumer;
import java.util.logging.Logger;
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.ColorFormatter;
import software.amazon.smithy.cli.ColorTheme;
import software.amazon.smithy.cli.Command;
import software.amazon.smithy.cli.HelpPrinter;
Expand All @@ -35,7 +37,6 @@

final class DiffCommand implements Command {

private static final Logger LOGGER = Logger.getLogger(DiffCommand.class.getName());
private final String parentCommandName;
private final DependencyResolver.Factory dependencyResolverFactory;

Expand Down Expand Up @@ -63,12 +64,46 @@ public int execute(Arguments arguments, Env env) {
arguments.getReceiver(BuildOptions.class).noPositionalArguments(true);

CommandAction action = HelpActionWrapper.fromCommand(
this, parentCommandName, new ClasspathAction(dependencyResolverFactory, this::runWithClassLoader));
this,
parentCommandName,
this::getDocumentation,
new ClasspathAction(dependencyResolverFactory, this::runWithClassLoader)
);

return action.apply(arguments, env);
}

private String getDocumentation(ColorFormatter colors) {
String ls = System.lineSeparator();
String content =
"The `diff` command supports different modes through the `--mode` option:"
+ ls
+ ls
+ "`--mode arbitrary`:"
+ ls
+ "Compares two arbitrary models. This mode requires that `--old` and `--new` are specified. "
+ "When run within a project directory that contains a `smithy-build.json` config, any dependencies "
+ "defined in the config file are used when loading both the old and new models; however, `imports` "
+ "and `sources` defined in the config file are not used. This is the default mode when no `--mode` "
+ "is specified."
+ ls
+ ls
+ "`--mode project`:"
+ ls
+ "Compares the current state of a project against another project. `--old` is required and points "
+ "to the location of another Smithy model or the root directory of another project. `--new` is not "
+ "allowed in this mode because the new model is the project in the current working directory. The "
+ "old model does not use any `sources` or `imports` defined by the current project, though it is "
+ "loaded using any dependencies defined by the current project. If the `--old` argument points to "
+ "a directory that contains a `smithy-build.json` file, any `imports` or `sources` defined in that "
+ "config file will be used when loading the old model, though the dependencies of the old model "
+ "are ignored.";

return StyleHelper.markdownLiterals(content, colors);
}

private static final class Options implements ArgumentReceiver {
private DiffMode diffMode = DiffMode.ARBITRARY;
private String oldModel;
private String newModel;

Expand All @@ -79,12 +114,21 @@ public boolean testOption(String name) {

@Override
public Consumer<String> testParameter(String name) {
if (name.equals("--old")) {
return m -> oldModel = m;
} else if (name.equals("--new")) {
return n -> newModel = n;
} else {
return null;
switch (name) {
case "--old":
return m -> oldModel = m;
case "--new":
return n -> newModel = n;
case "--mode":
return m -> {
try {
diffMode = DiffMode.valueOf(m.toUpperCase(Locale.ENGLISH));
} catch (IllegalArgumentException e) {
throw new CliError("Invalid --diff mode provided: " + m);
}
};
default:
return null;
}
}

Expand All @@ -94,6 +138,8 @@ public void registerHelp(HelpPrinter printer) {
"Path to an old Smithy model file or directory that contains model files.");
printer.param("--new", null, "NEW_MODEL",
"Path to the new Smithy model file or directory that contains model files.");
printer.param("--mode", null, "DIFF_MODE",
"The diff mode to use: 'arbitrary' (default mode), 'project'.");
}
}

Expand All @@ -104,64 +150,102 @@ int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env) {
throw new CliError("Unexpected arguments: " + arguments.getPositional());
}

// TODO: Add checks here for `mode` to change the DiffMode. Defaults to arbitrary.
return DiffMode.ARBITRARY.diff(config, arguments, options, env);
return options.diffMode.diff(config, arguments, options, env);
}

private enum DiffMode {
ARBITRARY {
@Override
int diff(SmithyBuildConfig config, Arguments arguments, Options options, Env env) {
String oldModelOpt = options.oldModel;
if (oldModelOpt == null) {
if (options.oldModel == null) {
throw new CliError("Missing required --old argument");
}

String newModelOpt = options.newModel;
if (newModelOpt == null) {
if (options.newModel == null) {
throw new CliError("Missing required --new argument");
}

LOGGER.fine(() -> String.format("Setting old models to: %s; new models to: %s",
oldModelOpt, newModelOpt));

ModelBuilder modelBuilder = new ModelBuilder()
.config(config)
.arguments(arguments)
.env(env)
.validationPrinter(env.stderr())
// Don't use imports or sources from the model config file.
.disableConfigModels(true)
// Only report issues that fail the build.
.validationMode(Validator.Mode.DISABLE)
.severity(Severity.DANGER);
ModelBuilder modelBuilder = createModelBuilder(config, arguments, env);
// Don't use imports or sources from the model config file.
modelBuilder.disableConfigModels(true);

// Use the ModelBuilder template to build the old model.
Model oldModel = modelBuilder
.models(Collections.singletonList(oldModelOpt))
.models(Collections.singletonList(options.oldModel))
.titleLabel("OLD", ColorTheme.DIFF_EVENT_TITLE)
.build();

// Use the same ModelBuilder template to build the new model.
Model newModel = modelBuilder
.models(Collections.singletonList(newModelOpt))
.titleLabel("NEW", ColorTheme.DIFF_EVENT_TITLE)
.build();
// Use the same ModelBuilder template to build the new model, being careful to use the original config.
Model newModel = createNewModel(modelBuilder, Collections.singletonList(options.newModel), config);
runDiff(modelBuilder, env, oldModel, newModel);
return 0;
}
},

PROJECT {
@Override
int diff(SmithyBuildConfig config, Arguments arguments, Options options, Env env) {
if (options.oldModel == null) {
throw new CliError("Missing required --old argument");
}

if (options.newModel != null) {
throw new CliError("--new cannot be used with this diff mode");
}

ModelBuilder modelBuilder = createModelBuilder(config, arguments, env);

// Use the ModelBuilder template to build the old model.
// The old model is built as if we're rooted in the given directory, allowing ConfigOptions to look
// for a smithy-build.json file, which can then use imports and sources.
ConfigOptions oldConfig = new ConfigOptions();
oldConfig.root(Paths.get(options.oldModel));

// Diff the models and report on the events, failing if necessary.
// We *do* use dependencies in smithy-build.json (if present) to find custom diff evaluators.
ClassLoader classLoader = env.classLoader();
List<ValidationEvent> events = ModelDiff.compare(classLoader, oldModel, newModel);
modelBuilder
.titleLabel("DIFF", ColorTheme.DIFF_TITLE)
.validatedResult(new ValidatedResult<>(newModel, events))
.severity(null) // reset so it takes on standard option settings.
Model oldModel = modelBuilder
.models(Collections.emptyList())
.config(oldConfig.createSmithyBuildConfig())
.titleLabel("OLD", ColorTheme.DIFF_EVENT_TITLE)
.build();

// Use the same ModelBuilder template to build the new model, being careful to use the original config.
Model newModel = createNewModel(modelBuilder, Collections.emptyList(), config);
runDiff(modelBuilder, env, oldModel, newModel);
return 0;
}
};

// Create a ModelBuilder template to load the old, then new, then diff both.
protected ModelBuilder createModelBuilder(SmithyBuildConfig config, Arguments arguments, Env env) {
return new ModelBuilder()
.config(config)
.arguments(arguments)
.env(env)
.validationPrinter(env.stderr())
// Only report issues that fail the build.
.validationMode(Validator.Mode.QUIET_CORE_ONLY)
.severity(Severity.DANGER);
}

// Creating a new model is the same for each diff mode.
protected Model createNewModel(ModelBuilder builder, List<String> models, SmithyBuildConfig config) {
return builder
.models(models)
.titleLabel("NEW", ColorTheme.DIFF_EVENT_TITLE)
.config(config)
.build();
}

// Running the diff is the same for each diff mode.
protected void runDiff(ModelBuilder builder, Env env, Model oldModel, Model newModel) {
ClassLoader classLoader = env.classLoader();
List<ValidationEvent> events = ModelDiff.compare(classLoader, oldModel, newModel);
builder
.titleLabel("DIFF", ColorTheme.DIFF_TITLE)
.validatedResult(new ValidatedResult<>(newModel, events))
.severity(null) // reset so it takes on standard option settings.
.build();
}

abstract int diff(SmithyBuildConfig config, Arguments arguments, Options options, Env env);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public Model build() {
if (validatedResult == null) {
ModelAssembler assembler = createModelAssembler(classLoader);

if (validationMode == Validator.Mode.DISABLE) {
if (validationMode == Validator.Mode.QUIET_CORE_ONLY) {
assembler.disableValidation();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import java.io.UncheckedIOException;
import java.nio.file.Paths;
import java.util.Collection;
import java.util.regex.Pattern;
import software.amazon.smithy.cli.ColorBuffer;
import software.amazon.smithy.cli.ColorFormatter;
import software.amazon.smithy.cli.ColorTheme;
Expand All @@ -28,12 +27,10 @@
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.model.validation.ValidationEventFormatter;
import software.amazon.smithy.utils.SmithyBuilder;
import software.amazon.smithy.utils.StringUtils;

final class PrettyAnsiValidationFormatter implements ValidationEventFormatter {

private static final int LINE_LENGTH = 80;
private static final Pattern TICK_PATTERN = Pattern.compile("`(.*?)`");
private final SourceContextLoader sourceContextLoader;
private final ColorFormatter colors;
private final String rootPath = Paths.get("").normalize().toAbsolutePath().toString();
Expand Down Expand Up @@ -140,13 +137,7 @@ private void printTitle(ColorBuffer writer, ValidationEvent event, Style border,

// Converts Markdown style ticks to use color highlights if colors are enabled.
private void writeMessage(ColorBuffer writer, String message) {
String content = StringUtils.wrap(message, LINE_LENGTH, System.lineSeparator(), false);

if (colors.isColorEnabled()) {
content = TICK_PATTERN.matcher(content).replaceAll(colors.style("$1", ColorTheme.LITERAL));
}

writer.append(content);
writer.append(StyleHelper.formatMessage(message, LINE_LENGTH, colors));
}

private String getHumanReadableFilename(String filename) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ private int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, En
.env(env)
.models(arguments.getPositional())
.validationPrinter(env.stderr())
.validationMode(Validator.Mode.DISABLE)
.validationMode(Validator.Mode.QUIET_CORE_ONLY)
.severity(Severity.DANGER)
.build();

Expand Down
Loading

0 comments on commit 9b16244

Please sign in to comment.