Skip to content

Commit

Permalink
Fail by default when a build plugin can't be found
Browse files Browse the repository at this point in the history
smithy-build previously just logged an info when a build plugin couldn't
be found on the classpath. This was a bad user experience for actually
using Smithy. The requirement to be able to ignore a missing build
plugin is really an edge case for build tools that want to be able to
build just models and ignore other plugins, so those tools should opt-in
to that behavior instead. Just logging was way too subtle for diagnosing
issues with config files and dependencies.
  • Loading branch information
mtdowling committed Sep 15, 2021
1 parent 8c7094f commit c492603
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 7 deletions.
5 changes: 5 additions & 0 deletions docs/source/1.0/guides/building-models/build-config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ The configuration file accepts the following properties:
- Defines the plugins to apply to the model when building every
projection. Plugins are a mapping of plugin names to an arbitrary
plugin configuration object.
* - ignoreMissingPlugins
- ``bool``
- If a plugin can't be found, Smithy will by default fail the build. This
setting can be set to ``true`` to allow the build to progress even if
a plugin can't be found on the classpath.

The following is an example ``smithy-build.json`` configuration:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -362,9 +362,14 @@ private void applyPlugin(
SmithyBuildPlugin resolved = pluginFactory.apply(pluginName).orElse(null);

if (resolved == null) {
LOGGER.info(() -> String.format(
"Unable to find a plugin for `%s` in the `%s` projection",
pluginName, projectionName));
String message = "Unable to find a plugin named `" + pluginName + "` in the `" + projectionName + "` "
+ "projection. Is this the correct spelling? Are you missing a dependency? Is your "
+ "classpath configured correctly?";
if (config.isIgnoreMissingPlugins()) {
LOGGER.severe(message);
} else {
throw new SmithyBuildException(message);
}
} else if (resolved.requiresValidModel() && modelResult.isBroken()) {
LOGGER.fine(() -> String.format(
"Skipping `%s` plugin for `%s` projection because the model is broken",
Expand Down Expand Up @@ -411,7 +416,9 @@ private List<Pair<ObjectNode, ProjectionTransformer>> createTransformers(

ProjectionTransformer transformer = transformFactory.apply(name)
.orElseThrow(() -> new UnknownTransformException(String.format(
"Unable to find a transform for `%s` in the `%s` projection.", name, projectionName)));
"Unable to find a transform named `%s` in the `%s` projection. Is this the correct "
+ "spelling? Are you missing a dependency? Is your classpath configured correctly?",
name, projectionName)));
resolved.add(Pair.of(transformConfig.getArgs(), transformer));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public final class SmithyBuildConfig implements ToSmithyBuilder<SmithyBuildConfi
private final String outputDirectory;
private final Map<String, ProjectionConfig> projections;
private final Map<String, ObjectNode> plugins;
private final boolean ignoreMissingPlugins;

private SmithyBuildConfig(Builder builder) {
SmithyBuilder.requiredState("version", builder.version);
Expand All @@ -53,6 +54,7 @@ private SmithyBuildConfig(Builder builder) {
imports = ListUtils.copyOf(builder.imports);
projections = MapUtils.copyOf(builder.projections);
plugins = new HashMap<>(builder.plugins);
ignoreMissingPlugins = builder.ignoreMissingPlugins;
for (String builtin : BUILTIN_PLUGINS) {
plugins.put(builtin, Node.objectNode());
}
Expand Down Expand Up @@ -113,7 +115,8 @@ public Builder toBuilder() {
.outputDirectory(outputDirectory)
.imports(imports)
.projections(projections)
.plugins(plugins);
.plugins(plugins)
.ignoreMissingPlugins(ignoreMissingPlugins);
}

/**
Expand Down Expand Up @@ -163,6 +166,17 @@ public Map<String, ObjectNode> getPlugins() {
return Collections.unmodifiableMap(plugins);
}

/**
* If a plugin can't be found, Smithy will by default fail the build.
* This setting can be set to true to allow the build to progress even
* if there is a missing plugin.
*
* @return Returns true if missing build plugins are allowed.
*/
public boolean isIgnoreMissingPlugins() {
return ignoreMissingPlugins;
}

/**
* Builder used to create a {@link SmithyBuildConfig}.
*/
Expand All @@ -172,6 +186,7 @@ public static final class Builder implements SmithyBuilder<SmithyBuildConfig> {
private final Map<String, ObjectNode> plugins = new LinkedHashMap<>();
private String version;
private String outputDirectory;
private boolean ignoreMissingPlugins;

Builder() {}

Expand Down Expand Up @@ -213,6 +228,12 @@ public Builder merge(SmithyBuildConfig config) {
imports.addAll(config.getImports());
projections.putAll(config.getProjections());
plugins.putAll(config.getPlugins());

// If either one wants to ignore missing plugins, then ignore them.
if (config.isIgnoreMissingPlugins()) {
ignoreMissingPlugins(config.ignoreMissingPlugins);
}

return this;
}

Expand Down Expand Up @@ -262,5 +283,16 @@ public Builder plugins(Map<String, ObjectNode> plugins) {
this.plugins.putAll(plugins);
return this;
}

/**
* Logs instead of failing when a plugin can't be found by name.
*
* @param ignoreMissingPlugins Set to true to ignore missing plugins on the classpath.
* @return Returns the builder.
*/
public Builder ignoreMissingPlugins(boolean ignoreMissingPlugins) {
this.ignoreMissingPlugins = ignoreMissingPlugins;
return this;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,9 @@ public void doesNotCopyErroneousModelsToBuildOutput() throws Exception {
}

@Test
public void ignoresUnknownPlugins() throws Exception {
public void canIgnoreUnknownPlugins() throws Exception {
SmithyBuildConfig config = SmithyBuildConfig.builder()
.load(Paths.get(getClass().getResource("unknown-plugin.json").toURI()))
.load(Paths.get(getClass().getResource("unknown-plugin-ignored.json").toURI()))
.outputDirectory(outputDirectory.toString())
.build();
Model model = Model.assembler()
Expand All @@ -200,6 +200,25 @@ public void ignoresUnknownPlugins() throws Exception {
builder.build();
}

@Test
public void failsByDefaultForUnknownPlugins() throws Exception {
SmithyBuildConfig config = SmithyBuildConfig.builder()
.load(Paths.get(getClass().getResource("unknown-plugin.json").toURI()))
.outputDirectory(outputDirectory.toString())
.build();
Model model = Model.assembler()
.addImport(Paths.get(getClass().getResource("resource-model.json").toURI()))
.assemble()
.unwrap();

SmithyBuildException e = Assertions.assertThrows(SmithyBuildException.class, () -> {
SmithyBuild builder = new SmithyBuild(config).model(model);
builder.build();
});

assertThat(e.getMessage(), containsString("Unable to find a plugin named `unknown1`"));
}

@Test
public void cannotSetFiltersOrMappersOnSourceProjection() {
Throwable thrown = Assertions.assertThrows(SmithyBuildException.class, () -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,26 @@ private String getResourcePath(String name) {
throw new RuntimeException(e);
}
}

@Test
public void convertingToBuilderRetainsIgnoreMissingPlugins() {
SmithyBuildConfig a = SmithyBuildConfig.builder()
.version("1")
.ignoreMissingPlugins(true)
.build();

assertThat(a.toBuilder().build().isIgnoreMissingPlugins(), equalTo(true));
}

@Test
public void mergingTakesIgnoreMissingPluginsFromEither() {
SmithyBuildConfig a = SmithyBuildConfig.builder()
.version("1")
.ignoreMissingPlugins(true)
.build();
SmithyBuildConfig b = SmithyBuildConfig.builder().version("1").build();

assertThat(a.toBuilder().merge(b).build().isIgnoreMissingPlugins(), equalTo(true));
assertThat(b.toBuilder().merge(a).build().isIgnoreMissingPlugins(), equalTo(true));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"version": "1.0",
"ignoreMissingPlugins": true,
"plugins": {
"unknown1": {}
},
"projections": {
"a": {
"plugins": {
"unknown2": {}
}
},
"b": {
"plugins": {
"unknown3": {}
}
}
}
}

0 comments on commit c492603

Please sign in to comment.