Merge "Introduce configuration validation listener"
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java
index c5009d3..8ac0307 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java
@@ -21,6 +21,7 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.nio.file.Path;
+import java.util.List;
public class AutoReloadRunnable implements Runnable {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -29,6 +30,7 @@
private final Path pluginDataDir;
private final EventBus eventBus;
private final Provider<ReplicationQueue> replicationQueue;
+ private final ReplicationConfigValidator configValidator;
private ReplicationFileBasedConfig loadedConfig;
private String loadedConfigVersion;
@@ -36,6 +38,7 @@
@Inject
public AutoReloadRunnable(
+ ReplicationConfigValidator configValidator,
ReplicationFileBasedConfig config,
SitePaths site,
@PluginData Path pluginDataDir,
@@ -48,6 +51,7 @@
this.pluginDataDir = pluginDataDir;
this.eventBus = eventBus;
this.replicationQueue = replicationQueue;
+ this.configValidator = configValidator;
}
@Override
@@ -67,10 +71,13 @@
synchronized void reload() {
String pendingConfigVersion = loadedConfig.getVersion();
try {
- loadedConfig = new ReplicationFileBasedConfig(site, pluginDataDir);
- loadedConfigVersion = loadedConfig.getVersion();
+ ReplicationFileBasedConfig newConfig = new ReplicationFileBasedConfig(site, pluginDataDir);
+ final List<DestinationConfiguration> newValidDestinations =
+ configValidator.validateConfig(newConfig);
+ loadedConfig = newConfig;
+ loadedConfigVersion = newConfig.getVersion();
lastFailedConfigVersion = "";
- eventBus.post(loadedConfig);
+ eventBus.post(newValidDestinations);
} catch (Exception e) {
logger.atSevere().withCause(e).log(
"Cannot reload replication configuration: keeping existing settings");
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
index 996cb2a..47664d7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
@@ -607,21 +607,7 @@
}
boolean isSingleProjectMatch() {
- List<String> projects = config.getProjects();
- boolean ret = (projects.size() == 1);
- if (ret) {
- String projectMatch = projects.get(0);
- if (ReplicationFilter.getPatternType(projectMatch)
- != ReplicationFilter.PatternType.EXACT_MATCH) {
- // projectMatch is either regular expression, or wild-card.
- //
- // Even though they might refer to a single project now, they need not
- // after new projects have been created. Hence, we do not treat them as
- // matching a single project.
- ret = false;
- }
- }
- return ret;
+ return config.isSingleProjectMatch();
}
boolean wouldPushRef(String ref) {
@@ -666,7 +652,7 @@
} else if (!remoteNameStyle.equals("slash")) {
repLog.debug("Unknown remoteNameStyle: {}, falling back to slash", remoteNameStyle);
}
- String replacedPath = replaceName(uri.getPath(), name, isSingleProjectMatch());
+ String replacedPath = replaceName(uri.getPath(), name, config.isSingleProjectMatch());
if (replacedPath != null) {
uri = uri.setPath(replacedPath);
r.add(uri);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationConfiguration.java b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationConfiguration.java
index 815b1b7..053739c 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationConfiguration.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationConfiguration.java
@@ -17,6 +17,7 @@
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.server.config.ConfigUtil;
+import java.util.List;
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.transport.RemoteConfig;
@@ -155,6 +156,24 @@
return cfg.getInt("remote", rc.getName(), name, defValue);
}
+ boolean isSingleProjectMatch() {
+ List<String> projects = getProjects();
+ boolean ret = (projects.size() == 1);
+ if (ret) {
+ String projectMatch = projects.get(0);
+ if (ReplicationFilter.getPatternType(projectMatch)
+ != ReplicationFilter.PatternType.EXACT_MATCH) {
+ // projectMatch is either regular expression, or wild-card.
+ //
+ // Even though they might refer to a single project now, they need not
+ // after new projects have been created. Hence, we do not treat them as
+ // matching a single project.
+ ret = false;
+ }
+ }
+ return ret;
+ }
+
public int getSlowLatencyThreshold() {
return slowLatencyThreshold;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
index a1f0095..88df5f6 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
@@ -52,12 +52,11 @@
import org.eclipse.jgit.transport.URIish;
@Singleton
-public class DestinationsCollection implements ReplicationDestinations {
+public class DestinationsCollection implements ReplicationDestinations, ReplicationConfigValidator {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final Factory destinationFactory;
private final Provider<ReplicationQueue> replicationQueue;
- private volatile ReplicationFileBasedConfig replicationConfig;
private volatile List<Destination> destinations;
private boolean shuttingDown;
@@ -78,8 +77,7 @@
throws ConfigInvalidException {
this.destinationFactory = destinationFactory;
this.replicationQueue = replicationQueue;
- this.replicationConfig = replicationConfig;
- this.destinations = allDestinations(destinationFactory);
+ this.destinations = allDestinations(destinationFactory, validateConfig(replicationConfig));
eventBus.register(this);
}
@@ -233,8 +231,7 @@
}
@Subscribe
- public synchronized void onReload(ReplicationFileBasedConfig newConfig)
- throws ConfigInvalidException {
+ public synchronized void onReload(List<DestinationConfiguration> destinationConfigurations) {
if (shuttingDown) {
logger.atWarning().log("Shutting down: configuration reload ignored");
return;
@@ -242,15 +239,15 @@
try {
replicationQueue.get().stop();
- replicationConfig = newConfig;
- destinations = allDestinations(destinationFactory);
+ destinations = allDestinations(destinationFactory, destinationConfigurations);
logger.atInfo().log("Configuration reloaded: %d destinations", getAll(FilterType.ALL).size());
} finally {
replicationQueue.get().start();
}
}
- private List<Destination> allDestinations(Destination.Factory destinationFactory)
+ @Override
+ public List<DestinationConfiguration> validateConfig(ReplicationFileBasedConfig replicationConfig)
throws ConfigInvalidException {
if (!replicationConfig.getConfig().getFile().exists()) {
logger.atWarning().log(
@@ -282,7 +279,7 @@
boolean defaultForceUpdate =
replicationConfig.getConfig().getBoolean("gerrit", "defaultForceUpdate", false);
- ImmutableList.Builder<Destination> dest = ImmutableList.builder();
+ ImmutableList.Builder<DestinationConfiguration> confs = ImmutableList.builder();
for (RemoteConfig c : allRemotes(replicationConfig.getConfig())) {
if (c.getURIs().isEmpty()) {
continue;
@@ -302,10 +299,10 @@
.setForceUpdate(defaultForceUpdate));
}
- Destination destination =
- destinationFactory.create(new DestinationConfiguration(c, replicationConfig.getConfig()));
+ DestinationConfiguration destinationConfiguration =
+ new DestinationConfiguration(c, replicationConfig.getConfig());
- if (!destination.isSingleProjectMatch()) {
+ if (!destinationConfiguration.isSingleProjectMatch()) {
for (URIish u : c.getURIs()) {
if (u.getPath() == null || !u.getPath().contains("${name}")) {
throw new ConfigInvalidException(
@@ -316,7 +313,19 @@
}
}
- dest.add(destination);
+ confs.add(destinationConfiguration);
+ }
+
+ return confs.build();
+ }
+
+ private List<Destination> allDestinations(
+ Destination.Factory destinationFactory,
+ List<DestinationConfiguration> destinationConfigurations) {
+
+ ImmutableList.Builder<Destination> dest = ImmutableList.builder();
+ for (DestinationConfiguration c : destinationConfigurations) {
+ dest.add(destinationFactory.create(c));
}
return dest.build();
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigValidator.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigValidator.java
new file mode 100644
index 0000000..2a2ced1
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigValidator.java
@@ -0,0 +1,31 @@
+// 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.googlesource.gerrit.plugins.replication;
+
+import java.util.List;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+
+public interface ReplicationConfigValidator {
+
+ /**
+ * validate the new replication.config
+ *
+ * @param newConfig new configuration detected
+ * @return List of validated {@link DestinationConfiguration}
+ * @throws ConfigInvalidException if the new configuration is not valid.
+ */
+ List<DestinationConfiguration> validateConfig(ReplicationFileBasedConfig newConfig)
+ throws ConfigInvalidException;
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java
index be6e7f6..da2b1b5 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java
@@ -76,6 +76,7 @@
bind(EventBus.class).in(Scopes.SINGLETON);
bind(ReplicationDestinations.class).to(DestinationsCollection.class);
+ bind(ReplicationConfigValidator.class).to(DestinationsCollection.class);
if (getReplicationConfig().getBoolean("gerrit", "autoReload", false)) {
bind(ReplicationConfig.class).to(AutoReloadConfigDecorator.class);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java
index 4b16314..d85f622 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java
@@ -21,6 +21,7 @@
import java.io.IOException;
import java.util.List;
import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.storage.file.FileBasedConfig;
import org.junit.Before;
import org.junit.Test;
@@ -97,9 +98,10 @@
assertThat(destinationsCollections.getAll(FilterType.ALL)).isEqualTo(destinations);
}
- private AutoReloadConfigDecorator newAutoReloadConfig() {
+ private AutoReloadConfigDecorator newAutoReloadConfig() throws ConfigInvalidException {
AutoReloadRunnable autoReloadRunnable =
new AutoReloadRunnable(
+ newDestinationsCollections(replicationFileBasedConfig),
replicationFileBasedConfig,
sitePaths,
pluginDataPath,
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnableTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnableTest.java
new file mode 100644
index 0000000..c302271
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnableTest.java
@@ -0,0 +1,120 @@
+// 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.googlesource.gerrit.plugins.replication;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.common.eventbus.EventBus;
+import com.google.common.eventbus.Subscribe;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.inject.util.Providers;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Collections;
+import java.util.List;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.junit.Before;
+import org.junit.Test;
+
+public class AutoReloadRunnableTest {
+
+ private SitePaths sitePaths;
+ private EventBus eventBus;
+ private ReloadTrackerSubscriber onReloadSubscriber;
+ private String pluginName;
+ private ReplicationQueue replicationQueueMock;
+
+ @Before
+ public void setUp() throws IOException {
+ Path tmp = Files.createTempFile(pluginName, "_site");
+ Files.deleteIfExists(tmp);
+ sitePaths = new SitePaths(tmp);
+ pluginName = "replication";
+ eventBus = new EventBus();
+ onReloadSubscriber = new ReloadTrackerSubscriber();
+ eventBus.register(onReloadSubscriber);
+
+ replicationQueueMock = mock(ReplicationQueue.class);
+ when(replicationQueueMock.isRunning()).thenReturn(Boolean.TRUE);
+ }
+
+ @Test
+ public void configurationIsReloadedWhenValidationSucceeds() {
+ ReplicationConfigValidator validator = new TestValidConfigurationListener();
+
+ attemptAutoReload(validator);
+
+ assertThat(onReloadSubscriber.reloaded).isTrue();
+ }
+
+ @Test
+ public void configurationIsNotReloadedWhenValidationFails() {
+ ReplicationConfigValidator validator = new TestInvalidConfigurationListener();
+
+ attemptAutoReload(validator);
+
+ assertThat(onReloadSubscriber.reloaded).isFalse();
+ }
+
+ private void attemptAutoReload(ReplicationConfigValidator validator) {
+ final AutoReloadRunnable autoReloadRunnable =
+ new AutoReloadRunnable(
+ validator,
+ newVersionConfig(),
+ sitePaths,
+ sitePaths.data_dir,
+ eventBus,
+ Providers.of(replicationQueueMock));
+
+ autoReloadRunnable.run();
+ }
+
+ private ReplicationFileBasedConfig newVersionConfig() {
+ return new ReplicationFileBasedConfig(sitePaths, sitePaths.data_dir) {
+ @Override
+ public String getVersion() {
+ return String.format("%s", System.nanoTime());
+ }
+ };
+ }
+
+ private static class ReloadTrackerSubscriber {
+ public boolean reloaded = false;
+
+ @Subscribe
+ public void onReload(
+ @SuppressWarnings("unused") List<DestinationConfiguration> destinationConfigurations) {
+ reloaded = true;
+ }
+ }
+
+ private static class TestValidConfigurationListener implements ReplicationConfigValidator {
+ @Override
+ public List<DestinationConfiguration> validateConfig(ReplicationFileBasedConfig newConfig) {
+ return Collections.emptyList();
+ }
+ }
+
+ private static class TestInvalidConfigurationListener implements ReplicationConfigValidator {
+ @Override
+ public List<DestinationConfiguration> validateConfig(
+ ReplicationFileBasedConfig configurationChangeEvent) throws ConfigInvalidException {
+ throw new ConfigInvalidException("expected test failure");
+ }
+ }
+}
