Introduce configuration validation listener
Upon replication.config change, validate the new configuration.
The validation of the configuration is done separately from the loading
of it, which allows decoupling of responsibilities.
Because of this, the reloading of the configuration can no longer throw
a ConfigInvalid exception (as configuration has been already validated)
Feature: Issue 11126
Change-Id: I54212eadadfc1bd655495449c93917bb2a89a752
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");
+ }
+ }
+}
