Skip to content

Commit

Permalink
Use absolute paths to de-dupe model sources
Browse files Browse the repository at this point in the history
We previously used Paths to de-dupe model sources in the
ModelAssembler, but weren't using absolute paths to
normalize paths before de-duping. This caused issues when
smithy-build.json files defined imports or sources and
sources were provided to the Smithy CLI as positional
arguments. We now normalize files provided to the model
assembler using toAbsolutePath to ensure that relative and
absolute paths are de-duped.

Further, this commit adds the same treatment to SmithyBuild
sources. This isn't needed to de-dupe for the assembler, but
rather to de-dupe files when generating the sources plugin.
Without this, the sources plugin would fail due to conflicting
files that look different because one might be absolute and
another might be relative, but are the same.

Integration test cases were added to the Smithy CLI to test this
because it does not appear reliable to change the current working
directory.
  • Loading branch information
mtdowling committed Dec 5, 2022
1 parent 390e642 commit 17c1a2f
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,18 @@ public void build(Consumer<ProjectionResult> resultCallback, BiConsumer<String,
public SmithyBuild config(SmithyBuildConfig config) {
this.config = config;
for (String source : config.getSources()) {
sources.add(Paths.get(source));
addSource(Paths.get(source));
}
return this;
}

// Add a source path using absolute paths to better de-conflict source files. ModelAssembler also
// de-conflicts imports with absolute paths, but this ensures the same file doesn't appear twice in
// the build plugin output (though it does not use realpath to de-conflict based on symlinks).
private void addSource(Path path) {
sources.add(path.toAbsolutePath());
}

/**
* Sets the <strong>required</strong> configuration object used to
* build the model.
Expand Down Expand Up @@ -377,7 +384,9 @@ public SmithyBuild pluginClassLoader(ClassLoader pluginClassLoader) {
* @return Returns the builder.
*/
public SmithyBuild registerSources(Path... pathToSources) {
Collections.addAll(sources, pathToSources);
for (Path path : pathToSources) {
addSource(path);
}
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,14 @@ public void showsErrorMessageWithStacktraceWhenConfigIsMissing() {
assertThat(result.getOutput(), containsString("Caused by"));
});
}

@Test
public void successfullyDeDupesConfigAndCliArguments() {
// The config adds model as a source and so does the CLI. Without de-duping, this would fail due to the
// enum being defined twice with the same members. Without de-conflicting in SmithyBuild, the sources plugin
// would fail due to finding two files named main.smithy.
IntegUtils.run("simple-config-sources", ListUtils.of("build", "model"), result -> {
assertThat(result.getExitCode(), equalTo(0));
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ public void validatesModelFailure() {
IntegUtils.run("invalid-model", ListUtils.of("validate", "model"), result -> {
assertThat(result.getExitCode(), equalTo(1));
assertThat(result.getOutput(), containsString("ERROR: smithy.example#MyString (TraitTarget)"));
// Normalize windows paths.
assertThat(result.getOutput().replace("\\", "/"), containsString("@ model/invalid.smithy"));
assertThat(result.getOutput(), containsString("invalid.smithy"));
assertThat(result.getOutput(), containsString("@range(min: 10, max: 100) // not valid for strings!"));
assertThat(result.getOutput(), containsString("ERROR: 1"));
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,16 @@
$version: "2.0"
// Use a 1.0 model to use an enum trait without a warning.
$version: "1.0"
namespace smithy.example

// This is used for assertions around de-duping files because duplicate enum values would fail validation.
@enum([
{
value: "FOO",
name: "FOO"
},
{
value: "BAR",
name: "BAR"
}
])
string MyString
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,8 @@ public ModelAssembler addImport(Path importPath) {
throw new ModelImportException("Error loading the contents of " + importPath, e);
}
} else if (Files.isRegularFile(importPath)) {
inputStreamModels.put(importPath.toString(), () -> {
// Use an absolute path for better de-duping of the same file.
inputStreamModels.put(importPath.toAbsolutePath().toString(), () -> {
try {
return Files.newInputStream(importPath);
} catch (IOException e) {
Expand Down Expand Up @@ -309,8 +310,8 @@ public ModelAssembler addImport(URL url) {

if (key.startsWith("file:")) {
try {
// Paths.get ensures paths are normalized for Windows too.
key = Paths.get(url.toURI()).toString();
// Use an absolute Path to ensure paths are normalized for Windows too, and better de-duping.
key = Paths.get(url.toURI()).toAbsolutePath().toString();
} catch (URISyntaxException e) {
key = key.substring(5);
}
Expand Down

0 comments on commit 17c1a2f

Please sign in to comment.