Use ReplicationConfig interface instead of ReplicationFileBasedConfigv3.1.4

ReplicationFileBasedConfing is an implementation of the
ReplicationConfig interface. Using ReplicationFileBasedConfig directly
is causing unnecessary code coupling and makes use of different
ReplicationConfig implementation difficult.

Feature: Issue 12450
Change-Id: Icda484ce6bd4a9246c530b8705910331c12d6c8f

12 files changed, 99 insertions, 72 deletions

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java
index 4c07f40..fe5dbad 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java
@@ -25,13 +25,14 @@ import java.nio.file.Path;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.lib.Config;
 
 @Singleton
 public class AutoReloadConfigDecorator implements ReplicationConfig, LifecycleListener {
   private static final long RELOAD_DELAY = 120;
   private static final long RELOAD_INTERVAL = 60;
 
-  private volatile ReplicationFileBasedConfig currentConfig;
+  private volatile ReplicationConfig currentConfig;
 
   private final ScheduledExecutorService autoReloadExecutor;
   private ScheduledFuture<?> autoReloadRunnable;
@@ -41,7 +42,7 @@ public class AutoReloadConfigDecorator implements ReplicationConfig, LifecycleLi
   public AutoReloadConfigDecorator(
       @PluginName String pluginName,
       WorkQueue workQueue,
-      ReplicationFileBasedConfig replicationConfig,
+      @MainReplicationConfig ReplicationConfig replicationConfig,
       AutoReloadRunnable reloadRunner,
       EventBus eventBus) {
     this.currentConfig = replicationConfig;
@@ -99,7 +100,7 @@ public class AutoReloadConfigDecorator implements ReplicationConfig, LifecycleLi
   }
 
   @Subscribe
-  public void onReload(ReplicationFileBasedConfig newConfig) {
+  public void onReload(ReplicationConfig newConfig) {
     currentConfig = newConfig;
   }
 
@@ -112,4 +113,9 @@ public class AutoReloadConfigDecorator implements ReplicationConfig, LifecycleLi
   public synchronized int getSshCommandTimeout() {
     return currentConfig.getSshCommandTimeout();
   }
+
+  @Override
+  public Config getConfig() {
+    return currentConfig.getConfig();
+  }
 }
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 a518e81..71f7c67 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java
@@ -16,39 +16,31 @@ package com.googlesource.gerrit.plugins.replication;
 
 import com.google.common.eventbus.EventBus;
 import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.extensions.annotations.PluginData;
-import com.google.gerrit.server.config.SitePaths;
 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();
 
-  private final SitePaths site;
-  private final Path pluginDataDir;
   private final EventBus eventBus;
   private final Provider<ObservableQueue> queueObserverProvider;
   private final ConfigParser configParser;
-
-  private ReplicationFileBasedConfig loadedConfig;
+  private ReplicationConfig loadedConfig;
+  private Provider<ReplicationConfig> replicationConfigProvider;
   private String loadedConfigVersion;
   private String lastFailedConfigVersion;
 
   @Inject
   public AutoReloadRunnable(
       ConfigParser configParser,
-      ReplicationFileBasedConfig config,
-      SitePaths site,
-      @PluginData Path pluginDataDir,
+      @MainReplicationConfig Provider<ReplicationConfig> replicationConfigProvider,
       EventBus eventBus,
       Provider<ObservableQueue> queueObserverProvider) {
-    this.loadedConfig = config;
-    this.loadedConfigVersion = config.getVersion();
+    this.replicationConfigProvider = replicationConfigProvider;
+    this.loadedConfig = replicationConfigProvider.get();
+    this.loadedConfigVersion = loadedConfig.getVersion();
     this.lastFailedConfigVersion = "";
-    this.site = site;
-    this.pluginDataDir = pluginDataDir;
     this.eventBus = eventBus;
     this.queueObserverProvider = queueObserverProvider;
     this.configParser = configParser;
@@ -71,7 +63,7 @@ public class AutoReloadRunnable implements Runnable {
   synchronized void reload() {
     String pendingConfigVersion = loadedConfig.getVersion();
     try {
-      ReplicationFileBasedConfig newConfig = new ReplicationFileBasedConfig(site, pluginDataDir);
+      ReplicationConfig newConfig = replicationConfigProvider.get();
       final List<RemoteConfiguration> newValidDestinations =
           configParser.parseRemotes(newConfig.getConfig());
       loadedConfig = newConfig;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java
index 98f364d..5bae0af 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java
@@ -31,11 +31,10 @@ public class AutoReloadSecureCredentialsFactoryDecorator implements CredentialsF
   private final AtomicReference<SecureCredentialsFactory> secureCredentialsFactory;
   private volatile long secureCredentialsFactoryLoadTs;
   private final SitePaths site;
-  private ReplicationFileBasedConfig config;
+  private ReplicationConfig config;
 
   @Inject
-  public AutoReloadSecureCredentialsFactoryDecorator(
-      SitePaths site, ReplicationFileBasedConfig config)
+  public AutoReloadSecureCredentialsFactoryDecorator(SitePaths site, ReplicationConfig config)
       throws ConfigInvalidException, IOException {
     this.site = site;
     this.config = config;
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 472d29d..71c38d0 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
@@ -66,7 +66,7 @@ public class DestinationsCollection implements ReplicationDestinations {
   public DestinationsCollection(
       Destination.Factory destinationFactory,
       Provider<ReplicationQueue> replicationQueue,
-      ReplicationFileBasedConfig replicationConfig,
+      ReplicationConfig replicationConfig,
       ConfigParser configParser,
       EventBus eventBus)
       throws ConfigInvalidException {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/MainReplicationConfig.java b/src/main/java/com/googlesource/gerrit/plugins/replication/MainReplicationConfig.java
new file mode 100644
index 0000000..e8d95ec
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/MainReplicationConfig.java
@@ -0,0 +1,23 @@
+// 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.inject.BindingAnnotation;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+@BindingAnnotation
+@Retention(RetentionPolicy.RUNTIME)
+public @interface MainReplicationConfig {}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfig.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfig.java
index b981bc8..d3d64db 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfig.java
@@ -15,6 +15,7 @@
 package com.googlesource.gerrit.plugins.replication;
 
 import java.nio.file.Path;
+import org.eclipse.jgit.lib.Config;
 
 /** Configuration of all the replication end points. */
 public interface ReplicationConfig {
@@ -70,4 +71,6 @@ public interface ReplicationConfig {
    * @return current logical version number.
    */
   String getVersion();
+
+  Config getConfig();
 }
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 088a8e7..06e69b3 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfig.java
@@ -19,7 +19,6 @@ import com.google.common.base.Strings;
 import com.google.gerrit.extensions.annotations.PluginData;
 import com.google.gerrit.server.config.SitePaths;
 import com.google.inject.Inject;
-import com.google.inject.Singleton;
 import java.io.IOException;
 import java.nio.file.Path;
 import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -27,7 +26,6 @@ import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.eclipse.jgit.util.FS;
 
-@Singleton
 public class ReplicationFileBasedConfig implements ReplicationConfig {
   private static final int DEFAULT_SSH_CONNECTION_TIMEOUT_MS = 2 * 60 * 1000; // 2 minutes
 
@@ -105,6 +103,7 @@ public class ReplicationFileBasedConfig implements ReplicationConfig {
     return cfgPath;
   }
 
+  @Override
   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 d4f3d1c..456b17f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java
@@ -80,12 +80,15 @@ class ReplicationModule extends AbstractModule {
     bind(ConfigParser.class).in(Scopes.SINGLETON);
 
     if (getReplicationConfig().getBoolean("gerrit", "autoReload", false)) {
-      bind(ReplicationConfig.class).to(AutoReloadConfigDecorator.class);
+      bind(ReplicationConfig.class)
+          .annotatedWith(MainReplicationConfig.class)
+          .to(ReplicationFileBasedConfig.class);
+      bind(ReplicationConfig.class).to(AutoReloadConfigDecorator.class).in(Scopes.SINGLETON);
       bind(LifecycleListener.class)
           .annotatedWith(UniqueAnnotations.create())
           .to(AutoReloadConfigDecorator.class);
     } else {
-      bind(ReplicationConfig.class).to(ReplicationFileBasedConfig.class);
+      bind(ReplicationConfig.class).to(ReplicationFileBasedConfig.class).in(Scopes.SINGLETON);
     }
 
     DynamicSet.setOf(binder(), ReplicationStateListener.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 70804cb..14ac595 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java
@@ -124,13 +124,17 @@ public abstract class AbstractConfigTest {
     assertThatIsDestination(matchingDestinations.get(0), remoteName, remoteUrls);
   }
 
-  protected DestinationsCollection newDestinationsCollections(
-      ReplicationFileBasedConfig replicationFileBasedConfig) throws ConfigInvalidException {
+  protected DestinationsCollection newDestinationsCollections(ReplicationConfig replicationConfig)
+      throws ConfigInvalidException {
     return new DestinationsCollection(
         destinationFactoryMock,
         Providers.of(replicationQueueMock),
-        replicationFileBasedConfig,
+        replicationConfig,
         configParser,
         eventBus);
   }
+
+  protected ReplicationConfig newReplicationFileBasedConfig() {
+    return new ReplicationFileBasedConfig(sitePaths, pluginDataPath);
+  }
 }
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 54a4d1c..b6a3c4d 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java
@@ -16,6 +16,7 @@ package com.googlesource.gerrit.plugins.replication;
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.inject.Provider;
 import com.google.inject.util.Providers;
 import com.googlesource.gerrit.plugins.replication.ReplicationConfig.FilterType;
 import java.io.IOException;
@@ -25,7 +26,7 @@ import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.junit.Test;
 
 public class AutoReloadConfigDecoratorTest extends AbstractConfigTest {
-  ReplicationFileBasedConfig replicationFileBasedConfig;
+  ReplicationConfig replicationConfig;
 
   public AutoReloadConfigDecoratorTest() throws IOException {
     super();
@@ -33,19 +34,18 @@ public class AutoReloadConfigDecoratorTest extends AbstractConfigTest {
 
   @Test
   public void shouldAutoReloadReplicationConfig() throws Exception {
-    FileBasedConfig replicationConfig = newReplicationConfig();
-    replicationConfig.setBoolean("gerrit", null, "autoReload", true);
+    FileBasedConfig fileConfig = newReplicationConfig();
+    fileConfig.setBoolean("gerrit", null, "autoReload", true);
     String remoteName1 = "foo";
     String remoteUrl1 = "ssh://git@git.foo.com/${name}";
-    replicationConfig.setString("remote", remoteName1, "url", remoteUrl1);
-    replicationConfig.save();
+    fileConfig.setString("remote", remoteName1, "url", remoteUrl1);
+    fileConfig.save();
 
-    replicationFileBasedConfig = newReplicationFileBasedConfig();
+    replicationConfig = newReplicationFileBasedConfig();
 
     newAutoReloadConfig().start();
 
-    DestinationsCollection destinationsCollections =
-        newDestinationsCollections(replicationFileBasedConfig);
+    DestinationsCollection destinationsCollections = newDestinationsCollections(replicationConfig);
     destinationsCollections.startup(workQueueMock);
     List<Destination> destinations = destinationsCollections.getAll(FilterType.ALL);
     assertThat(destinations).hasSize(1);
@@ -55,8 +55,8 @@ public class AutoReloadConfigDecoratorTest extends AbstractConfigTest {
 
     String remoteName2 = "bar";
     String remoteUrl2 = "ssh://git@git.bar.com/${name}";
-    replicationConfig.setString("remote", remoteName2, "url", remoteUrl2);
-    replicationConfig.save();
+    fileConfig.setString("remote", remoteName2, "url", remoteUrl2);
+    fileConfig.save();
     executorService.refreshCommand.run();
 
     destinations = destinationsCollections.getAll(FilterType.ALL);
@@ -69,15 +69,14 @@ public class AutoReloadConfigDecoratorTest extends AbstractConfigTest {
   public void shouldNotAutoReloadReplicationConfigIfDisabled() throws Exception {
     String remoteName1 = "foo";
     String remoteUrl1 = "ssh://git@git.foo.com/${name}";
-    FileBasedConfig replicationConfig = newReplicationConfig();
-    replicationConfig.setBoolean("gerrit", null, "autoReload", false);
-    replicationConfig.setString("remote", remoteName1, "url", remoteUrl1);
-    replicationConfig.save();
+    FileBasedConfig fileConfig = newReplicationConfig();
+    fileConfig.setBoolean("gerrit", null, "autoReload", false);
+    fileConfig.setString("remote", remoteName1, "url", remoteUrl1);
+    fileConfig.save();
 
-    replicationFileBasedConfig = newReplicationFileBasedConfig();
+    replicationConfig = newReplicationFileBasedConfig();
 
-    DestinationsCollection destinationsCollections =
-        newDestinationsCollections(replicationFileBasedConfig);
+    DestinationsCollection destinationsCollections = newDestinationsCollections(replicationConfig);
     destinationsCollections.startup(workQueueMock);
     List<Destination> destinations = destinationsCollections.getAll(FilterType.ALL);
     assertThat(destinations).hasSize(1);
@@ -85,8 +84,8 @@ public class AutoReloadConfigDecoratorTest extends AbstractConfigTest {
 
     TimeUnit.SECONDS.sleep(1); // Allow the filesystem to change the update TS
 
-    replicationConfig.setString("remote", "bar", "url", "ssh://git@git.bar.com/${name}");
-    replicationConfig.save();
+    fileConfig.setString("remote", "bar", "url", "ssh://git@git.bar.com/${name}");
+    fileConfig.save();
     executorService.refreshCommand.run();
 
     assertThat(destinationsCollections.getAll(FilterType.ALL)).isEqualTo(destinations);
@@ -96,16 +95,20 @@ public class AutoReloadConfigDecoratorTest extends AbstractConfigTest {
     AutoReloadRunnable autoReloadRunnable =
         new AutoReloadRunnable(
             configParser,
-            replicationFileBasedConfig,
-            sitePaths,
-            pluginDataPath,
+            new Provider<ReplicationConfig>() {
+
+              @Override
+              public ReplicationConfig get() {
+                return newReplicationFileBasedConfig();
+              }
+            },
             eventBus,
             Providers.of(replicationQueueMock));
     return new AutoReloadConfigDecorator(
-        "replication", workQueueMock, replicationFileBasedConfig, autoReloadRunnable, eventBus);
-  }
-
-  private ReplicationFileBasedConfig newReplicationFileBasedConfig() {
-    return new ReplicationFileBasedConfig(sitePaths, pluginDataPath);
+        "replication",
+        workQueueMock,
+        newReplicationFileBasedConfig(),
+        autoReloadRunnable,
+        eventBus);
   }
 }
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 a37726a..93e8886 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnableTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnableTest.java
@@ -21,6 +21,7 @@ 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.Provider;
 import com.google.inject.util.Providers;
 import java.io.IOException;
 import java.nio.file.Files;
@@ -75,21 +76,21 @@ public class AutoReloadRunnableTest {
   private void attemptAutoReload(ConfigParser validator) {
     final AutoReloadRunnable autoReloadRunnable =
         new AutoReloadRunnable(
-            validator,
-            newVersionConfig(),
-            sitePaths,
-            sitePaths.data_dir,
-            eventBus,
-            Providers.of(replicationQueueMock));
+            validator, newVersionConfigProvider(), eventBus, Providers.of(replicationQueueMock));
 
     autoReloadRunnable.run();
   }
 
-  private ReplicationFileBasedConfig newVersionConfig() {
-    return new ReplicationFileBasedConfig(sitePaths, sitePaths.data_dir) {
+  private Provider<ReplicationConfig> newVersionConfigProvider() {
+    return new Provider<ReplicationConfig>() {
       @Override
-      public String getVersion() {
-        return String.format("%s", System.nanoTime());
+      public ReplicationConfig get() {
+        return new ReplicationFileBasedConfig(sitePaths, sitePaths.data_dir) {
+          @Override
+          public String getVersion() {
+            return String.format("%s", System.nanoTime());
+          }
+        };
       }
     };
   }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfigTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfigTest.java
index f2b027c..5943757 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfigTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationFileBasedConfigTest.java
@@ -65,10 +65,4 @@ public class ReplicationFileBasedConfigTest extends AbstractConfigTest {
     assertThatIsDestination(destinations.get(0), remoteName1, remoteUrl1);
     assertThatIsDestination(destinations.get(1), remoteName2, remoteUrl2);
   }
-
-  private ReplicationFileBasedConfig newReplicationFileBasedConfig() {
-    ReplicationFileBasedConfig replicationConfig =
-        new ReplicationFileBasedConfig(sitePaths, pluginDataPath);
-    return replicationConfig;
-  }
 }