Merge "Merge branch 'stable-3.0'"
diff --git a/Documentation/cmd-plugin-remove.txt b/Documentation/cmd-plugin-remove.txt
index f5fe56b..012bf7b 100644
--- a/Documentation/cmd-plugin-remove.txt
+++ b/Documentation/cmd-plugin-remove.txt
@@ -20,6 +20,7 @@
* Caller must be a member of the privileged 'Administrators' group.
* link:config-gerrit.html#plugins.allowRemoteAdmin[plugins.allowRemoteAdmin]
must be enabled in `$site_path/etc/gerrit.config`.
+* Mandatory plugin cannot be disabled
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 67cd0f9..1668ffc 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -3481,6 +3481,11 @@
and SSH. If set to true Administrators can install new plugins
remotely, or disable existing plugins. Defaults to false.
+[[plugins.mandatory]]plugins.mandatory::
++
+List of mandatory plugins. If a plugin from this list does not load,
+Gerrit start will fail.
+
[[plugins.jsLoadTimeout]]plugins.jsLoadTimeout::
+
Set the timeout value for loading JavaScript plugins in Gerrit UI.
diff --git a/Documentation/rest-api-plugins.txt b/Documentation/rest-api-plugins.txt
index 938d101..c34fe77 100644
--- a/Documentation/rest-api-plugins.txt
+++ b/Documentation/rest-api-plugins.txt
@@ -387,6 +387,24 @@
}
----
+Disabling of a link:config-gerrit.html#plugins.mandatory[mandatory plugin]
+is rejected:
+
+.Request
+----
+ DELETE /plugins/replication HTTP/1.0
+----
+
+.Response
+----
+ HTTP/1.1 405 Method Not Allowed
+ Content-Disposition: attachment
+ Content-Type: application/json; charset=UTF-8
+
+ )]}'
+ Plugin replication is mandatory
+----
+
[[reload-plugin]]
=== Reload Plugin
--
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 87da183..197a6a3 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -207,7 +207,8 @@
beforeTest(description);
ProjectResetter.Config input = requireNonNull(resetProjects());
- try (ProjectResetter resetter = projectResetter.builder().build(input)) {
+ try (ProjectResetter resetter =
+ projectResetter != null ? projectResetter.builder().build(input) : null) {
AbstractDaemonTest.this.resetter = resetter;
base.evaluate();
} finally {
@@ -287,12 +288,16 @@
@Before
public void clearSender() {
- sender.clear();
+ if (sender != null) {
+ sender.clear();
+ }
}
@Before
public void startEventRecorder() {
- eventRecorder = eventRecorderFactory.create(admin);
+ if (eventRecorderFactory != null) {
+ eventRecorder = eventRecorderFactory.create(admin);
+ }
}
@Before
@@ -307,7 +312,9 @@
@After
public void closeEventRecorder() {
- eventRecorder.close();
+ if (eventRecorder != null) {
+ eventRecorder.close();
+ }
}
@AfterClass
diff --git a/java/com/google/gerrit/server/plugins/DisablePlugin.java b/java/com/google/gerrit/server/plugins/DisablePlugin.java
index 62eb993..877b348 100644
--- a/java/com/google/gerrit/server/plugins/DisablePlugin.java
+++ b/java/com/google/gerrit/server/plugins/DisablePlugin.java
@@ -17,6 +17,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.common.Input;
import com.google.gerrit.extensions.common.PluginInfo;
+import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.permissions.GlobalPermission;
@@ -30,11 +31,16 @@
private final PluginLoader loader;
private final PermissionBackend permissionBackend;
+ private final MandatoryPluginsCollection mandatoryPluginsCollection;
@Inject
- DisablePlugin(PluginLoader loader, PermissionBackend permissionBackend) {
+ DisablePlugin(
+ PluginLoader loader,
+ PermissionBackend permissionBackend,
+ MandatoryPluginsCollection mandatoryPluginsCollection) {
this.loader = loader;
this.permissionBackend = permissionBackend;
+ this.mandatoryPluginsCollection = mandatoryPluginsCollection;
}
@Override
@@ -46,6 +52,9 @@
}
loader.checkRemoteAdminEnabled();
String name = resource.getName();
+ if (mandatoryPluginsCollection.contains(name)) {
+ throw new MethodNotAllowedException("Plugin " + name + " is mandatory");
+ }
loader.disablePlugins(ImmutableSet.of(name));
return ListPlugins.toPluginInfo(loader.get(name));
}
diff --git a/java/com/google/gerrit/server/plugins/MandatoryPluginsCollection.java b/java/com/google/gerrit/server/plugins/MandatoryPluginsCollection.java
new file mode 100644
index 0000000..70a0fff
--- /dev/null
+++ b/java/com/google/gerrit/server/plugins/MandatoryPluginsCollection.java
@@ -0,0 +1,50 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License 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 com.google.gerrit.server.plugins;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.Arrays;
+import java.util.Set;
+import java.util.concurrent.CopyOnWriteArraySet;
+import org.eclipse.jgit.lib.Config;
+
+@Singleton
+public class MandatoryPluginsCollection {
+ private final CopyOnWriteArraySet<String> members;
+
+ @Inject
+ MandatoryPluginsCollection(@GerritServerConfig Config cfg) {
+ members = Sets.newCopyOnWriteArraySet();
+ members.addAll(Arrays.asList(cfg.getStringList("plugins", null, "mandatory")));
+ }
+
+ public boolean contains(String name) {
+ return members.contains(name);
+ }
+
+ public Set<String> asSet() {
+ return ImmutableSet.copyOf(members);
+ }
+
+ @VisibleForTesting
+ public void add(String name) {
+ members.add(name);
+ }
+}
diff --git a/java/com/google/gerrit/server/plugins/MissingMandatoryPluginsException.java b/java/com/google/gerrit/server/plugins/MissingMandatoryPluginsException.java
new file mode 100644
index 0000000..1c23550
--- /dev/null
+++ b/java/com/google/gerrit/server/plugins/MissingMandatoryPluginsException.java
@@ -0,0 +1,30 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License 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 com.google.gerrit.server.plugins;
+
+import java.util.Collection;
+
+/** Raised when one or more mandatory plugins are missing. */
+public class MissingMandatoryPluginsException extends RuntimeException {
+ private static final long serialVersionUID = 1L;
+
+ public MissingMandatoryPluginsException(Collection<String> pluginNames) {
+ super(getMessage(pluginNames));
+ }
+
+ private static String getMessage(Collection<String> pluginNames) {
+ return String.format("Cannot find or load the following mandatory plugins: %s", pluginNames);
+ }
+}
diff --git a/java/com/google/gerrit/server/plugins/PluginLoader.java b/java/com/google/gerrit/server/plugins/PluginLoader.java
index 9279f0fe..c4f4a1f 100644
--- a/java/com/google/gerrit/server/plugins/PluginLoader.java
+++ b/java/com/google/gerrit/server/plugins/PluginLoader.java
@@ -52,6 +52,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Comparator;
+import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
@@ -87,6 +88,7 @@
private final Provider<String> urlProvider;
private final PersistentCacheFactory persistentCacheFactory;
private final boolean remoteAdmin;
+ private final MandatoryPluginsCollection mandatoryPlugins;
private final UniversalServerPluginProvider serverPluginFactory;
private final GerritRuntime gerritRuntime;
@@ -101,6 +103,7 @@
@CanonicalWebUrl Provider<String> provider,
PersistentCacheFactory cacheFactory,
UniversalServerPluginProvider pluginFactory,
+ MandatoryPluginsCollection mpc,
GerritRuntime gerritRuntime) {
pluginsDir = sitePaths.plugins_dir;
dataDir = sitePaths.data_dir;
@@ -114,6 +117,7 @@
serverPluginFactory = pluginFactory;
remoteAdmin = cfg.getBoolean("plugins", null, "allowRemoteAdmin", false);
+ mandatoryPlugins = mpc;
this.gerritRuntime = gerritRuntime;
long checkFrequency =
@@ -226,6 +230,11 @@
continue;
}
+ if (mandatoryPlugins.contains(name)) {
+ logger.atWarning().log("Mandatory plugin %s cannot be disabled", name);
+ continue;
+ }
+
logger.atInfo().log("Disabling plugin %s", active.getName());
Path off =
active.getSrcFile().resolveSibling(active.getSrcFile().getFileName() + ".disabled");
@@ -381,50 +390,57 @@
}
public synchronized void rescan() {
+ Set<String> loadedPlugins = new HashSet<>();
SetMultimap<String, Path> pluginsFiles = prunePlugins(pluginsDir);
- if (pluginsFiles.isEmpty()) {
- return;
+
+ if (!pluginsFiles.isEmpty()) {
+ syncDisabledPlugins(pluginsFiles);
+
+ Map<String, Path> activePlugins = filterDisabled(pluginsFiles);
+ for (Map.Entry<String, Path> entry : jarsFirstSortedPluginsSet(activePlugins)) {
+ String name = entry.getKey();
+ Path path = entry.getValue();
+ String fileName = path.getFileName().toString();
+ if (!isUiPlugin(fileName) && !serverPluginFactory.handles(path)) {
+ logger.atWarning().log(
+ "No Plugin provider was found that handles this file format: %s", fileName);
+ continue;
+ }
+
+ FileSnapshot brokenTime = broken.get(name);
+ if (brokenTime != null && !brokenTime.isModified(path.toFile())) {
+ continue;
+ }
+
+ Plugin active = running.get(name);
+ if (active != null && !active.isModified(path)) {
+ loadedPlugins.add(name);
+ continue;
+ }
+
+ if (active != null) {
+ logger.atInfo().log("Reloading plugin %s", active.getName());
+ }
+
+ try {
+ Plugin loadedPlugin = runPlugin(name, path, active);
+ if (!loadedPlugin.isDisabled()) {
+ loadedPlugins.add(name);
+ logger.atInfo().log(
+ "%s plugin %s, version %s",
+ active == null ? "Loaded" : "Reloaded",
+ loadedPlugin.getName(),
+ loadedPlugin.getVersion());
+ }
+ } catch (PluginInstallException e) {
+ logger.atWarning().withCause(e.getCause()).log("Cannot load plugin %s", name);
+ }
+ }
}
- syncDisabledPlugins(pluginsFiles);
-
- Map<String, Path> activePlugins = filterDisabled(pluginsFiles);
- for (Map.Entry<String, Path> entry : jarsFirstSortedPluginsSet(activePlugins)) {
- String name = entry.getKey();
- Path path = entry.getValue();
- String fileName = path.getFileName().toString();
- if (!isUiPlugin(fileName) && !serverPluginFactory.handles(path)) {
- logger.atWarning().log(
- "No Plugin provider was found that handles this file format: %s", fileName);
- continue;
- }
-
- FileSnapshot brokenTime = broken.get(name);
- if (brokenTime != null && !brokenTime.isModified(path.toFile())) {
- continue;
- }
-
- Plugin active = running.get(name);
- if (active != null && !active.isModified(path)) {
- continue;
- }
-
- if (active != null) {
- logger.atInfo().log("Reloading plugin %s", active.getName());
- }
-
- try {
- Plugin loadedPlugin = runPlugin(name, path, active);
- if (!loadedPlugin.isDisabled()) {
- logger.atInfo().log(
- "%s plugin %s, version %s",
- active == null ? "Loaded" : "Reloaded",
- loadedPlugin.getName(),
- loadedPlugin.getVersion());
- }
- } catch (PluginInstallException e) {
- logger.atWarning().withCause(e.getCause()).log("Cannot load plugin %s", name);
- }
+ Set<String> missingMandatory = Sets.difference(mandatoryPlugins.asSet(), loadedPlugins);
+ if (!missingMandatory.isEmpty()) {
+ throw new MissingMandatoryPluginsException(missingMandatory);
}
cleanInBackground();
@@ -471,6 +487,12 @@
throws PluginInstallException {
FileSnapshot snapshot = FileSnapshot.save(plugin.toFile());
try {
+ boolean restartRequired = oldPlugin != null && !oldPlugin.canReload();
+ if (restartRequired && mandatoryPlugins.contains(name)) {
+ logger.atWarning().log("Restarting mandatory plugin %s not allowed", name);
+ return oldPlugin;
+ }
+
Plugin newPlugin = loadPlugin(name, plugin, snapshot);
if (newPlugin.getCleanupHandle() != null) {
cleanupHandles.put(newPlugin, newPlugin.getCleanupHandle());
diff --git a/java/com/google/gerrit/server/plugins/PluginModule.java b/java/com/google/gerrit/server/plugins/PluginModule.java
index 6bc37bd..71186e5 100644
--- a/java/com/google/gerrit/server/plugins/PluginModule.java
+++ b/java/com/google/gerrit/server/plugins/PluginModule.java
@@ -34,6 +34,7 @@
bind(PluginLoader.class);
bind(CopyConfigModule.class);
listener().to(PluginLoader.class);
+ bind(MandatoryPluginsCollection.class);
DynamicSet.setOf(binder(), ServerPluginProvider.class);
DynamicSet.bind(binder(), ServerPluginProvider.class).to(JarPluginProvider.class);
diff --git a/javatests/com/google/gerrit/acceptance/api/plugin/PluginIT.java b/javatests/com/google/gerrit/acceptance/api/plugin/PluginIT.java
index e8af0bb..dd12382 100644
--- a/javatests/com/google/gerrit/acceptance/api/plugin/PluginIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/plugin/PluginIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.api.plugin;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.toList;
@@ -35,6 +36,7 @@
import com.google.gerrit.extensions.restapi.RawInput;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.plugins.MandatoryPluginsCollection;
import com.google.inject.Inject;
import java.util.List;
import org.junit.Test;
@@ -53,6 +55,7 @@
"plugin-a.js", "plugin-b.html", "plugin-c.js", "plugin-d.html", "plugin_e.js");
@Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private MandatoryPluginsCollection mandatoryPluginsCollection;
@Test
@GerritConfig(name = "plugins.allowRemoteAdmin", value = "true")
@@ -99,7 +102,19 @@
assertBadRequest(list().regex(".*in-b").prefix("a"));
assertBadRequest(list().substring(".*in-b").prefix("a"));
- // Disable
+ // Disable mandatory
+ mandatoryPluginsCollection.add("plugin_e");
+ api = gApi.plugins().name("plugin_e");
+ try {
+ api.disable();
+ assert_().fail("Disabling mandatory plugin should have failed");
+ } catch (MethodNotAllowedException e) {
+ // expected
+ }
+ api = gApi.plugins().name("plugin_e");
+ assertThat(api.get().disabled).isNull();
+
+ // Disable non-mandatory
api = gApi.plugins().name("plugin-a");
api.disable();
api = gApi.plugins().name("plugin-a");
diff --git a/javatests/com/google/gerrit/acceptance/api/plugin/PluginLoaderIT.java b/javatests/com/google/gerrit/acceptance/api/plugin/PluginLoaderIT.java
new file mode 100644
index 0000000..7eb3680
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/api/plugin/PluginLoaderIT.java
@@ -0,0 +1,42 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License 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 com.google.gerrit.acceptance.api.plugin;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.GerritConfig;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.server.plugins.MissingMandatoryPluginsException;
+import org.junit.Test;
+import org.junit.runner.Description;
+
+@NoHttpd
+public class PluginLoaderIT extends AbstractDaemonTest {
+
+ Description testDescription;
+
+ @Override
+ protected void beforeTest(Description description) throws Exception {
+ this.testDescription = description;
+ }
+
+ @Override
+ protected void afterTest() throws Exception {}
+
+ @Test(expected = MissingMandatoryPluginsException.class)
+ @GerritConfig(name = "plugins.mandatory", value = "my-mandatory-plugin")
+ public void shouldFailToStartGerritWhenMandatoryPluginsAreMissing() throws Exception {
+ super.beforeTest(testDescription);
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index a8a19ac..aaa698a 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -2406,6 +2406,46 @@
assertThat(diffInfo).content().isEmpty();
}
+ // This behavior is likely a bug. A fix might not be easy as it might break syntax highlighting.
+ // TODO: Fix this issue or remove the broken parameter (at least in the documentation).
+ @Test
+ public void contextParameterIsIgnored() throws Exception {
+ addModifiedPatchSet(
+ changeId, FILE_NAME, content -> content.replace("Line 20\n", "Line twenty\n"));
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withBase(initialPatchSetId)
+ .withContext(5)
+ .get();
+ assertThat(diffInfo).content().element(0).commonLines().hasSize(19);
+ assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 20");
+ assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line twenty");
+ assertThat(diffInfo).content().element(2).commonLines().hasSize(81);
+ }
+
+ // This behavior is likely a bug. A fix might not be easy as it might break syntax highlighting.
+ // TODO: Fix this issue or remove the broken parameter (at least in the documentation).
+ @Test
+ public void contextParameterIsIgnoredForUnmodifiedFileWithComment() throws Exception {
+ addModifiedPatchSet(
+ changeId, FILE_NAME, content -> content.replace("Line 20\n", "Line twenty\n"));
+ String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+ CommentInput comment = createCommentInput(20, 0, 21, 0, "Should be 'Line 20'.");
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.comments = ImmutableMap.of(FILE_NAME, ImmutableList.of(comment));
+ gApi.changes().id(changeId).revision(previousPatchSetId).review(reviewInput);
+ addModifiedPatchSet(
+ changeId, FILE_NAME2, content -> content.replace("2nd line\n", "Second line\n"));
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withBase(previousPatchSetId)
+ .withContext(5)
+ .get();
+ assertThat(diffInfo).content().element(0).commonLines().hasSize(101);
+ }
+
@Test
public void requestingDiffForOldFileNameOfRenamedFileYieldsReasonableResult() throws Exception {
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index a2a6f09..632c094 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -271,6 +271,35 @@
}
@Test
+ public void postCommentsReplacingDrafts() throws Exception {
+ String file = "file";
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "first subject", file, "contents");
+ PushOneCommit.Result r = push.to("refs/for/master");
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+
+ DraftInput draft = newDraft(file, Side.REVISION, 0, "comment");
+ addDraft(changeId, revId, draft);
+ Map<String, List<CommentInfo>> drafts = getDraftComments(changeId, revId);
+ CommentInfo draftInfo = Iterables.getOnlyElement(drafts.get(draft.path));
+
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.drafts = DraftHandling.KEEP;
+ reviewInput.message = "foo";
+ CommentInput comment = newComment(file, Side.REVISION, 0, "comment", false);
+ // Replace the existing draft.
+ comment.id = draftInfo.id;
+ reviewInput.comments = new HashMap<>();
+ reviewInput.comments.put(comment.path, ImmutableList.of(comment));
+ revision(r).review(reviewInput);
+
+ // DraftHandling.KEEP is ignored on publishing a comment.
+ drafts = getDraftComments(changeId, revId);
+ assertThat(drafts).isEmpty();
+ }
+
+ @Test
public void listComments() throws Exception {
String file = "file";
PushOneCommit push =
