Move replication config parsing out of DestinationsCollection
DestinationsCollection class is breaking single responsibility principle
because is mixing ReplicationDestination and ConfigParser functionality.
This split allows better code decoupling and is a prerequisite for
replacing ReplicationFileBasedConfig with ReplicationConfig interface.
Feature: Issue 12450
Change-Id: I33e3350596884e24bc32eacc4b51b73048e3f18c
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 a1084a8..a518e81 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java
@@ -30,7 +30,7 @@
private final Path pluginDataDir;
private final EventBus eventBus;
private final Provider<ObservableQueue> queueObserverProvider;
- private final ReplicationConfigValidator configValidator;
+ private final ConfigParser configParser;
private ReplicationFileBasedConfig loadedConfig;
private String loadedConfigVersion;
@@ -38,7 +38,7 @@
@Inject
public AutoReloadRunnable(
- ReplicationConfigValidator configValidator,
+ ConfigParser configParser,
ReplicationFileBasedConfig config,
SitePaths site,
@PluginData Path pluginDataDir,
@@ -51,7 +51,7 @@
this.pluginDataDir = pluginDataDir;
this.eventBus = eventBus;
this.queueObserverProvider = queueObserverProvider;
- this.configValidator = configValidator;
+ this.configParser = configParser;
}
@Override
@@ -73,7 +73,7 @@
try {
ReplicationFileBasedConfig newConfig = new ReplicationFileBasedConfig(site, pluginDataDir);
final List<RemoteConfiguration> newValidDestinations =
- configValidator.validateConfig(newConfig);
+ configParser.parseRemotes(newConfig.getConfig());
loadedConfig = newConfig;
loadedConfigVersion = newConfig.getVersion();
lastFailedConfigVersion = "";
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ConfigParser.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ConfigParser.java
new file mode 100644
index 0000000..bb9097e
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ConfigParser.java
@@ -0,0 +1,102 @@
+// Copyright (C) 2020 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 com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
+import com.google.common.flogger.FluentLogger;
+import java.net.URISyntaxException;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.transport.RefSpec;
+import org.eclipse.jgit.transport.RemoteConfig;
+import org.eclipse.jgit.transport.URIish;
+
+public class ConfigParser {
+
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ /**
+ * parse the new replication config
+ *
+ * @param config new configuration to parse
+ * @return List of parsed {@link RemoteConfiguration}
+ * @throws ConfigInvalidException if the new configuration is not valid.
+ */
+ public List<RemoteConfiguration> parseRemotes(Config config) throws ConfigInvalidException {
+
+ if (config.getSections().isEmpty()) {
+ logger.atWarning().log("Replication config does not exist or it's empty; not replicating");
+ return Collections.emptyList();
+ }
+
+ boolean defaultForceUpdate = config.getBoolean("gerrit", "defaultForceUpdate", false);
+
+ ImmutableList.Builder<RemoteConfiguration> confs = ImmutableList.builder();
+ for (RemoteConfig c : allRemotes(config)) {
+ if (c.getURIs().isEmpty()) {
+ continue;
+ }
+
+ // If destination for push is not set assume equal to source.
+ for (RefSpec ref : c.getPushRefSpecs()) {
+ if (ref.getDestination() == null) {
+ ref.setDestination(ref.getSource());
+ }
+ }
+
+ if (c.getPushRefSpecs().isEmpty()) {
+ c.addPushRefSpec(
+ new RefSpec()
+ .setSourceDestination("refs/*", "refs/*")
+ .setForceUpdate(defaultForceUpdate));
+ }
+
+ DestinationConfiguration destinationConfiguration = new DestinationConfiguration(c, config);
+
+ if (!destinationConfiguration.isSingleProjectMatch()) {
+ for (URIish u : c.getURIs()) {
+ if (u.getPath() == null || !u.getPath().contains("${name}")) {
+ throw new ConfigInvalidException(
+ String.format(
+ "remote.%s.url \"%s\" lacks ${name} placeholder in %s",
+ c.getName(), u, config));
+ }
+ }
+ }
+
+ confs.add(destinationConfiguration);
+ }
+
+ return confs.build();
+ }
+
+ private static List<RemoteConfig> allRemotes(Config cfg) throws ConfigInvalidException {
+ Set<String> names = cfg.getSubsections("remote");
+ List<RemoteConfig> result = Lists.newArrayListWithCapacity(names.size());
+ for (String name : names) {
+ try {
+ result.add(new RemoteConfig(cfg, name));
+ } catch (URISyntaxException e) {
+ throw new ConfigInvalidException(
+ String.format("remote %s has invalid URL in %s", name, cfg), e);
+ }
+ }
+ return result;
+ }
+}
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 2f210e0..472d29d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
@@ -25,7 +25,6 @@
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMultimap;
-import com.google.common.collect.Lists;
import com.google.common.collect.Multimap;
import com.google.common.collect.SetMultimap;
import com.google.common.eventbus.EventBus;
@@ -38,22 +37,16 @@
import com.google.inject.Singleton;
import com.googlesource.gerrit.plugins.replication.Destination.Factory;
import com.googlesource.gerrit.plugins.replication.ReplicationConfig.FilterType;
-import java.io.IOException;
import java.net.URISyntaxException;
-import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
-import java.util.Set;
import java.util.function.Predicate;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.storage.file.FileBasedConfig;
-import org.eclipse.jgit.transport.RefSpec;
-import org.eclipse.jgit.transport.RemoteConfig;
import org.eclipse.jgit.transport.URIish;
@Singleton
-public class DestinationsCollection implements ReplicationDestinations, ReplicationConfigValidator {
+public class DestinationsCollection implements ReplicationDestinations {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final Factory destinationFactory;
@@ -74,11 +67,14 @@
Destination.Factory destinationFactory,
Provider<ReplicationQueue> replicationQueue,
ReplicationFileBasedConfig replicationConfig,
+ ConfigParser configParser,
EventBus eventBus)
throws ConfigInvalidException {
this.destinationFactory = destinationFactory;
this.replicationQueue = replicationQueue;
- this.destinations = allDestinations(destinationFactory, validateConfig(replicationConfig));
+ this.destinations =
+ allDestinations(
+ destinationFactory, configParser.parseRemotes(replicationConfig.getConfig()));
eventBus.register(this);
}
@@ -247,79 +243,6 @@
}
}
- @Override
- public List<RemoteConfiguration> validateConfig(ReplicationFileBasedConfig replicationConfig)
- throws ConfigInvalidException {
- if (!replicationConfig.getConfig().getFile().exists()) {
- logger.atWarning().log(
- "Config file %s does not exist; not replicating",
- replicationConfig.getConfig().getFile());
- return Collections.emptyList();
- }
- if (replicationConfig.getConfig().getFile().length() == 0) {
- logger.atInfo().log(
- "Config file %s is empty; not replicating", replicationConfig.getConfig().getFile());
- return Collections.emptyList();
- }
-
- try {
- replicationConfig.getConfig().load();
- } catch (ConfigInvalidException e) {
- throw new ConfigInvalidException(
- String.format(
- "Config file %s is invalid: %s",
- replicationConfig.getConfig().getFile(), e.getMessage()),
- e);
- } catch (IOException e) {
- throw new ConfigInvalidException(
- String.format(
- "Cannot read %s: %s", replicationConfig.getConfig().getFile(), e.getMessage()),
- e);
- }
-
- boolean defaultForceUpdate =
- replicationConfig.getConfig().getBoolean("gerrit", "defaultForceUpdate", false);
-
- ImmutableList.Builder<RemoteConfiguration> confs = ImmutableList.builder();
- for (RemoteConfig c : allRemotes(replicationConfig.getConfig())) {
- if (c.getURIs().isEmpty()) {
- continue;
- }
-
- // If destination for push is not set assume equal to source.
- for (RefSpec ref : c.getPushRefSpecs()) {
- if (ref.getDestination() == null) {
- ref.setDestination(ref.getSource());
- }
- }
-
- if (c.getPushRefSpecs().isEmpty()) {
- c.addPushRefSpec(
- new RefSpec()
- .setSourceDestination("refs/*", "refs/*")
- .setForceUpdate(defaultForceUpdate));
- }
-
- DestinationConfiguration destinationConfiguration =
- new DestinationConfiguration(c, replicationConfig.getConfig());
-
- if (!destinationConfiguration.isSingleProjectMatch()) {
- for (URIish u : c.getURIs()) {
- if (u.getPath() == null || !u.getPath().contains("${name}")) {
- throw new ConfigInvalidException(
- String.format(
- "remote.%s.url \"%s\" lacks ${name} placeholder in %s",
- c.getName(), u, replicationConfig.getConfig().getFile()));
- }
- }
- }
-
- confs.add(destinationConfiguration);
- }
-
- return confs.build();
- }
-
private List<Destination> allDestinations(
Destination.Factory destinationFactory, List<RemoteConfiguration> remoteConfigurations) {
@@ -331,18 +254,4 @@
}
return dest.build();
}
-
- private static List<RemoteConfig> allRemotes(FileBasedConfig cfg) throws ConfigInvalidException {
- Set<String> names = cfg.getSubsections("remote");
- List<RemoteConfig> result = Lists.newArrayListWithCapacity(names.size());
- for (String name : names) {
- try {
- result.add(new RemoteConfig(cfg, name));
- } catch (URISyntaxException e) {
- throw new ConfigInvalidException(
- String.format("remote %s has invalid URL in %s", name, cfg.getFile()), e);
- }
- }
- return result;
- }
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigValidator.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigValidator.java
deleted file mode 100644
index 7883114..0000000
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigValidator.java
+++ /dev/null
@@ -1,31 +0,0 @@
-// 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 RemoteConfiguration}
- * @throws ConfigInvalidException if the new configuration is not valid.
- */
- List<RemoteConfiguration> validateConfig(ReplicationFileBasedConfig newConfig)
- throws ConfigInvalidException;
-}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfig.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfig.java
index b963ab8..088a8e7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfig.java
@@ -23,6 +23,7 @@
import java.io.IOException;
import java.nio.file.Path;
import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.storage.file.FileBasedConfig;
import org.eclipse.jgit.util.FS;
@@ -104,7 +105,7 @@
return cfgPath;
}
- public FileBasedConfig getConfig() {
+ public Config getConfig() {
return config;
}
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 979c8e3..d4f3d1c 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java
@@ -77,7 +77,7 @@
bind(EventBus.class).in(Scopes.SINGLETON);
bind(ReplicationDestinations.class).to(DestinationsCollection.class);
- bind(ReplicationConfigValidator.class).to(DestinationsCollection.class);
+ bind(ConfigParser.class).in(Scopes.SINGLETON);
if (getReplicationConfig().getBoolean("gerrit", "autoReload", false)) {
bind(ReplicationConfig.class).to(AutoReloadConfigDecorator.class);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java
index 3932fb3..70804cb 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java
@@ -49,6 +49,7 @@
protected WorkQueue workQueueMock;
protected EventBus eventBus = new EventBus();
protected FakeExecutorService executorService = new FakeExecutorService();
+ protected ConfigParser configParser;
static class FakeDestination extends Destination {
public final DestinationConfiguration config;
@@ -71,6 +72,7 @@
sitePaths = new SitePaths(sitePath);
pluginDataPath = createTempPath("data");
destinationFactoryMock = mock(Destination.Factory.class);
+ configParser = new ConfigParser();
}
@Before
@@ -128,6 +130,7 @@
destinationFactoryMock,
Providers.of(replicationQueueMock),
replicationFileBasedConfig,
+ configParser,
eventBus);
}
}
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 d85f622..372540f 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java
@@ -21,7 +21,6 @@
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;
@@ -98,10 +97,10 @@
assertThat(destinationsCollections.getAll(FilterType.ALL)).isEqualTo(destinations);
}
- private AutoReloadConfigDecorator newAutoReloadConfig() throws ConfigInvalidException {
+ private AutoReloadConfigDecorator newAutoReloadConfig() {
AutoReloadRunnable autoReloadRunnable =
new AutoReloadRunnable(
- newDestinationsCollections(replicationFileBasedConfig),
+ configParser,
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
index b1aa7c8..a37726a 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnableTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnableTest.java
@@ -28,6 +28,7 @@
import java.util.Collections;
import java.util.List;
import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
import org.junit.Before;
import org.junit.Test;
@@ -54,24 +55,24 @@
}
@Test
- public void configurationIsReloadedWhenValidationSucceeds() {
- ReplicationConfigValidator validator = new TestValidConfigurationListener();
+ public void configurationIsReloadedWhenParsingSucceeds() {
+ ConfigParser parser = new TestValidConfigurationListener();
- attemptAutoReload(validator);
+ attemptAutoReload(parser);
assertThat(onReloadSubscriber.reloaded).isTrue();
}
@Test
- public void configurationIsNotReloadedWhenValidationFails() {
- ReplicationConfigValidator validator = new TestInvalidConfigurationListener();
+ public void configurationIsNotReloadedWhenParsingFails() {
+ ConfigParser parser = new TestInvalidConfigurationListener();
- attemptAutoReload(validator);
+ attemptAutoReload(parser);
assertThat(onReloadSubscriber.reloaded).isFalse();
}
- private void attemptAutoReload(ReplicationConfigValidator validator) {
+ private void attemptAutoReload(ConfigParser validator) {
final AutoReloadRunnable autoReloadRunnable =
new AutoReloadRunnable(
validator,
@@ -103,17 +104,17 @@
}
}
- private static class TestValidConfigurationListener implements ReplicationConfigValidator {
+ private static class TestValidConfigurationListener extends ConfigParser {
@Override
- public List<RemoteConfiguration> validateConfig(ReplicationFileBasedConfig newConfig) {
+ public List<RemoteConfiguration> parseRemotes(Config newConfig) {
return Collections.emptyList();
}
}
- private static class TestInvalidConfigurationListener implements ReplicationConfigValidator {
+ private static class TestInvalidConfigurationListener extends ConfigParser {
@Override
- public List<RemoteConfiguration> validateConfig(
- ReplicationFileBasedConfig configurationChangeEvent) throws ConfigInvalidException {
+ public List<RemoteConfiguration> parseRemotes(Config configurationChangeEvent)
+ throws ConfigInvalidException {
throw new ConfigInvalidException("expected test failure");
}
}
