From 4a6482133843b958d2e5bd0dba7da355de419db9 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 4 Aug 2017 21:03:28 +0200 Subject: [PATCH] Disentangle ListPlugins and PluginLsCommand ListPlugins has a lot of code that is only used for printing the output to stdout, which is only used by PluginLsCommand. Simplify ListPlugins to only return the map of plugins, and move the logic for displaying the output into PluginLsCommand. PluginLsCommand does not have access to the Plugin objects, but only to what is returned in PluginInfo instances. Extend that to include the filename, which was previously accessible to ListPlugins. This change removes support for the format option from the list plugins REST API endpoint. Change-Id: I0c352e587d5f0e8d524ae5b7322e1feec9434d58 --- Documentation/rest-api-plugins.txt | 10 +++ .../acceptance/api/plugin/PluginIT.java | 1 + .../gerrit/extensions/common/PluginInfo.java | 4 +- .../server/api/plugins/PluginsImpl.java | 2 +- .../gerrit/server/plugins/ListPlugins.java | 80 +++---------------- .../gerrit/sshd/commands/PluginLsCommand.java | 49 +++++++++++- 6 files changed, 73 insertions(+), 73 deletions(-) diff --git a/Documentation/rest-api-plugins.txt b/Documentation/rest-api-plugins.txt index 0f687bf99a..938d101d8e 100644 --- a/Documentation/rest-api-plugins.txt +++ b/Documentation/rest-api-plugins.txt @@ -47,6 +47,7 @@ by plugin ID. "delete-project": { "id": "delete-project", "index_url": "plugins/delete-project/", + "filename": "delete-project.jar", "version": "2.9-SNAPSHOT" } } @@ -73,11 +74,13 @@ List all plugins including those that are disabled. "delete-project": { "id": "delete-project", "index_url": "plugins/delete-project/", + "filename": "delete-project.jar", "version": "2.9-SNAPSHOT" }, "reviewers-by-blame": { "id": "reviewers-by-blame", "index_url": "plugins/reviewers-by-blame/", + "filename": "reviewers-by-blame.jar", "version": "2.9-SNAPSHOT", "disabled": true } @@ -105,6 +108,7 @@ Query the first plugin in the plugin list: "delete-project": { "id": "delete-project", "index_url": "plugins/delete-project/", + "filename": "delete-project.jar", "version": "2.9-SNAPSHOT" } } @@ -134,6 +138,7 @@ List all plugins that start with `delete`: "delete-project": { "id": "delete-project", "index_url": "plugins/delete-project/", + "filename": "delete-project.jar", "version": "2.9-SNAPSHOT" } } @@ -168,11 +173,13 @@ List all plugins that match regex `some.*plugin`: "some-plugin": { "id": "some-plugin", "index_url": "plugins/some-plugin/", + "filename": "some-plugin.jar", "version": "2.9-SNAPSHOT" }, "some-other-plugin": { "id": "some-other-plugin", "index_url": "plugins/some-other-plugin/", + "filename": "some-other-plugin.jar", "version": "2.9-SNAPSHOT" } } @@ -200,6 +207,7 @@ Query the second plugin in the plugin list: "reviewers-by-blame": { "id": "reviewers-by-blame", "index_url": "plugins/reviewers-by-blame/", + "filename": "reviewers-by-blame.jar", "version": "2.9-SNAPSHOT", "disabled": true } @@ -229,6 +237,7 @@ List all plugins that match substring `project`: "delete-project": { "id": "delete-project", "index_url": "plugins/delete-project/", + "filename": "delete-project.jar", "version": "2.9-SNAPSHOT" } } @@ -429,6 +438,7 @@ The `PluginInfo` entity describes a plugin. |`id` ||The ID of the plugin. |`version` ||The version of the plugin. |`index_url`|optional|URL of the plugin's default page. +|`filename` |optional|The plugin's filename. |`disabled` |not set if `false`|Whether the plugin is disabled. |======================= diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/plugin/PluginIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/plugin/PluginIT.java index 3e1b2cb6c7..0fa09afb27 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/plugin/PluginIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/plugin/PluginIT.java @@ -68,6 +68,7 @@ public class PluginIT extends AbstractDaemonTest { assertThat(info.id).isEqualTo(name); assertThat(info.version).isEqualTo(pluginVersion(plugin)); assertThat(info.indexUrl).isEqualTo(String.format("plugins/%s/", name)); + assertThat(info.filename).isEqualTo(plugin); assertThat(info.disabled).isNull(); } assertPlugins(list().get(), PLUGINS); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/PluginInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/PluginInfo.java index bcb957ed56..0df6235c9c 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/PluginInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/PluginInfo.java @@ -18,12 +18,14 @@ public class PluginInfo { public final String id; public final String version; public final String indexUrl; + public final String filename; public final Boolean disabled; - public PluginInfo(String id, String version, String indexUrl, Boolean disabled) { + public PluginInfo(String id, String version, String indexUrl, String filename, Boolean disabled) { this.id = id; this.version = version; this.indexUrl = indexUrl; + this.filename = filename; this.disabled = disabled; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/plugins/PluginsImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/plugins/PluginsImpl.java index 75fb350bff..a955abed82 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/plugins/PluginsImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/plugins/PluginsImpl.java @@ -66,7 +66,7 @@ public class PluginsImpl implements Plugins { list.setMatchPrefix(this.getPrefix()); list.setMatchSubstring(this.getSubstring()); list.setMatchRegex(this.getRegex()); - return list.apply(); + return list.apply(TopLevelResource.INSTANCE); } }; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/ListPlugins.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/ListPlugins.java index 0e514d63c3..97d728deed 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/ListPlugins.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/ListPlugins.java @@ -15,11 +15,8 @@ package com.google.gerrit.server.plugins; import static java.util.Comparator.comparing; -import static java.util.stream.Collectors.toList; -import com.google.common.base.Strings; import com.google.common.collect.Streams; -import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.extensions.annotations.RequiresCapability; import com.google.gerrit.extensions.common.PluginInfo; @@ -27,16 +24,12 @@ import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.extensions.restapi.Url; -import com.google.gerrit.server.OutputFormat; -import com.google.gson.reflect.TypeToken; import com.google.inject.Inject; -import java.io.PrintWriter; -import java.util.List; import java.util.Locale; -import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; import java.util.regex.Pattern; +import java.util.stream.Collectors; import java.util.stream.Stream; import org.kohsuke.args4j.Option; @@ -52,10 +45,6 @@ public class ListPlugins implements RestReadView { private String matchSubstring; private String matchRegex; - @Deprecated - @Option(name = "--format", usage = "(deprecated) output format") - private OutputFormat format = OutputFormat.TEXT; - @Option( name = "--all", aliases = {"-a"}, @@ -115,29 +104,8 @@ public class ListPlugins implements RestReadView { this.pluginLoader = pluginLoader; } - public OutputFormat getFormat() { - return format; - } - - public ListPlugins setFormat(OutputFormat fmt) { - this.format = fmt; - return this; - } - @Override - public Object apply(TopLevelResource resource) throws BadRequestException { - format = OutputFormat.JSON; - return display(null); - } - - public SortedMap apply() throws BadRequestException { - format = OutputFormat.JSON; - return display(null); - } - - public SortedMap display(@Nullable PrintWriter stdout) - throws BadRequestException { - SortedMap output = new TreeMap<>(); + public SortedMap apply(TopLevelResource resource) throws BadRequestException { Stream s = Streams.stream(pluginLoader.getPlugins(all)); if (matchPrefix != null) { checkMatchOptions(matchSubstring == null && matchRegex == null); @@ -158,38 +126,7 @@ public class ListPlugins implements RestReadView { if (limit > 0) { s = s.limit(limit); } - List plugins = s.collect(toList()); - - if (!format.isJson()) { - stdout.format("%-30s %-10s %-8s %s\n", "Name", "Version", "Status", "File"); - stdout.print( - "-------------------------------------------------------------------------------\n"); - } - - for (Plugin p : plugins) { - PluginInfo info = toPluginInfo(p); - if (format.isJson()) { - output.put(p.getName(), info); - } else { - stdout.format( - "%-30s %-10s %-8s %s\n", - p.getName(), - Strings.nullToEmpty(info.version), - p.isDisabled() ? "DISABLED" : "ENABLED", - p.getSrcFile().getFileName()); - } - } - - if (stdout == null) { - return output; - } else if (format.isJson()) { - format - .newGson() - .toJson(output, new TypeToken>() {}.getType(), stdout); - stdout.print('\n'); - } - stdout.flush(); - return null; + return new TreeMap<>(s.collect(Collectors.toMap(p -> p.getName(), p -> toPluginInfo(p)))); } private void checkMatchOptions(boolean cond) throws BadRequestException { @@ -202,13 +139,20 @@ public class ListPlugins implements RestReadView { String id; String version; String indexUrl; + String filename; Boolean disabled; id = Url.encode(p.getName()); version = p.getVersion(); disabled = p.isDisabled() ? true : null; - indexUrl = p.getSrcFile() != null ? String.format("plugins/%s/", p.getName()) : null; + if (p.getSrcFile() != null) { + indexUrl = String.format("plugins/%s/", p.getName()); + filename = p.getSrcFile().getFileName().toString(); + } else { + indexUrl = null; + filename = null; + } - return new PluginInfo(id, version, indexUrl, disabled); + return new PluginInfo(id, version, indexUrl, filename, disabled); } } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PluginLsCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PluginLsCommand.java index 78c952679d..0fdd105577 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PluginLsCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PluginLsCommand.java @@ -16,25 +16,68 @@ package com.google.gerrit.sshd.commands; import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE; +import com.google.common.base.Strings; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.extensions.annotations.RequiresCapability; +import com.google.gerrit.extensions.common.PluginInfo; +import com.google.gerrit.extensions.restapi.TopLevelResource; +import com.google.gerrit.server.OutputFormat; import com.google.gerrit.server.plugins.ListPlugins; import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.SshCommand; +import com.google.gson.reflect.TypeToken; import com.google.inject.Inject; +import java.util.Map; +import org.kohsuke.args4j.Option; @RequiresCapability(GlobalCapability.VIEW_PLUGINS) @CommandMetaData(name = "ls", description = "List the installed plugins", runsAt = MASTER_OR_SLAVE) final class PluginLsCommand extends SshCommand { - @Inject private ListPlugins impl; + @Inject private ListPlugins list; + + @Option( + name = "--all", + aliases = {"-a"}, + usage = "List all plugins, including disabled plugins" + ) + private boolean all; + + @Option(name = "--format", usage = "output format") + private OutputFormat format = OutputFormat.TEXT; @Override public void run() throws Exception { - impl.display(stdout); + list.setAll(all); + Map output = list.apply(TopLevelResource.INSTANCE); + + if (format.isJson()) { + format + .newGson() + .toJson(output, new TypeToken>() {}.getType(), stdout); + stdout.print('\n'); + } else { + stdout.format("%-30s %-10s %-8s %s\n", "Name", "Version", "Status", "File"); + stdout.print( + "-------------------------------------------------------------------------------\n"); + for (Map.Entry p : output.entrySet()) { + PluginInfo info = p.getValue(); + stdout.format( + "%-30s %-10s %-8s %s\n", + p.getKey(), + Strings.nullToEmpty(info.version), + status(info.disabled), + Strings.nullToEmpty(info.filename)); + } + } + stdout.flush(); + } + + private String status(Boolean disabled) { + return disabled != null && disabled.booleanValue() ? "DISABLED" : "ENABLED"; } @Override protected void parseCommandLine() throws UnloggedFailure { - parseCommandLine(impl); + parseCommandLine(this); } }