[PATCH v1] TasksStorage: Remove delete() only used by tests

[PATCH v1] TasksStorage: Remove delete() only used by tests

[Adithya Chakilam]

From: Nasser Grainawi <nasser@codeaurora.org>

In general we shouldn't have methods that provide behavior only for
tests. Callers of delete() in pre-3.1 code should move to start() and
finish() in 3.1. To facilitate that, update start(), reset(), and
finish() methods to not require a PushOne object. These methods can
instead use a new interface that PushOne already can implement without
functional changes.

Change-Id: Id82d2ec3125075832134c8dbe56b342e5a874bbc
---
 .../gerrit/plugins/replication/PushOne.java   |  8 ++-
 .../replication/ReplicationTasksStorage.java  | 52 +++++++-------
 .../plugins/replication/ReplicationIT.java    | 15 +++-
 .../ReplicationTasksStorageTest.java          | 68 ++++++++++++-------
 .../plugins/replication/TestUriUpdates.java   | 52 ++++++++++++++
 5 files changed, 139 insertions(+), 56 deletions(-)
 create mode 100644 src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
index 634f44d..3124940 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
@@ -84,7 +84,7 @@ import org.slf4j.MDC;
  * <p>Instance members are protected by the lock within PushQueue. Callers must take that lock to
  * ensure they are working with a current view of the object.
  */
-class PushOne implements ProjectRunnable, CanceledWhileRunning {
+class PushOne implements ProjectRunnable, CanceledWhileRunning, ReplicationTasksStorage.UriUpdates {
   private final ReplicationStateListener stateLog;
   static final String ALL_REFS = "..all..";
   static final String ID_MDC_KEY = "pushOneId";
@@ -230,7 +230,8 @@ class PushOne implements ProjectRunnable, CanceledWhileRunning {
     return canceled || canceledWhileRunning.get();
   }
 
-  URIish getURI() {
+  @Override
+  public URIish getURI() {
     return uri;
   }
 
@@ -244,7 +245,8 @@ class PushOne implements ProjectRunnable, CanceledWhileRunning {
     }
   }
 
-  Set<String> getRefs() {
+  @Override
+  public Set<String> getRefs() {
     return pushAllRefs ? Sets.newHashSet(ALL_REFS) : delta;
   }
 
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
index 370d48b..8a5aba2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
@@ -19,6 +19,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.hash.Hashing;
+import com.google.gerrit.entities.Project;
 import com.google.gson.Gson;
 import com.google.inject.Inject;
 import com.google.inject.ProvisionException;
@@ -32,6 +33,7 @@ import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.transport.URIish;
 
@@ -68,8 +70,12 @@ public class ReplicationTasksStorage {
     public final String uri;
     public final String remote;
 
-    public ReplicateRefUpdate(PushOne push, String ref) {
-      this(push.getProjectNameKey().get(), ref, push.getURI(), push.getRemoteName());
+    public ReplicateRefUpdate(UriUpdates uriUpdates, String ref) {
+      this(
+          uriUpdates.getProjectNameKey().get(),
+          ref,
+          uriUpdates.getURI(),
+          uriUpdates.getRemoteName());
     }
 
     public ReplicateRefUpdate(String project, String ref, URIish uri, String remote) {
@@ -85,6 +91,16 @@ public class ReplicationTasksStorage {
     }
   }
 
+  public interface UriUpdates {
+    Project.NameKey getProjectNameKey();
+
+    URIish getURI();
+
+    String getRemoteName();
+
+    Set<String> getRefs();
+  }
+
   private static final Gson GSON = new Gson();
 
   private final Path refUpdates;
@@ -114,20 +130,15 @@ public class ReplicationTasksStorage {
     this.disableDeleteForTesting = deleteDisabled;
   }
 
-  @VisibleForTesting
-  public void delete(ReplicateRefUpdate r) {
-    new Task(r).delete();
-  }
-
-  public synchronized void start(PushOne push) {
-    for (String ref : push.getRefs()) {
-      new Task(new ReplicateRefUpdate(push, ref)).start();
+  public synchronized void start(UriUpdates uriUpdates) {
+    for (String ref : uriUpdates.getRefs()) {
+      new Task(new ReplicateRefUpdate(uriUpdates, ref)).start();
     }
   }
 
-  public synchronized void reset(PushOne push) {
-    for (String ref : push.getRefs()) {
-      new Task(new ReplicateRefUpdate(push, ref)).reset();
+  public synchronized void reset(UriUpdates uriUpdates) {
+    for (String ref : uriUpdates.getRefs()) {
+      new Task(new ReplicateRefUpdate(uriUpdates, ref)).reset();
     }
   }
 
@@ -137,9 +148,9 @@ public class ReplicationTasksStorage {
     }
   }
 
-  public synchronized void finish(PushOne push) {
-    for (String ref : push.getRefs()) {
-      new Task(new ReplicateRefUpdate(push, ref)).finish();
+  public synchronized void finish(UriUpdates uriUpdates) {
+    for (String ref : uriUpdates.getRefs()) {
+      new Task(new ReplicateRefUpdate(uriUpdates, ref)).finish();
     }
   }
 
@@ -261,15 +272,6 @@ public class ReplicationTasksStorage {
       }
     }
 
-    public void delete() {
-      try {
-        Files.deleteIfExists(waiting);
-        Files.deleteIfExists(running);
-      } catch (IOException e) {
-        logger.atSevere().withCause(e).log("Error while deleting task %s", taskKey);
-      }
-    }
-
     private void rename(Path from, Path to) {
       try {
         logger.atFine().log("RENAME %s to %s %s", from, to, updateLog());
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
index 31cd75d..bb0f283 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
@@ -36,7 +36,9 @@ import com.google.gerrit.server.config.SitePaths;
 import com.google.inject.Inject;
 import com.google.inject.Key;
 import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;
+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.UriUpdates;
 import java.io.IOException;
+import java.net.URISyntaxException;
 import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -449,7 +451,7 @@ public class ReplicationIT extends LightweightPluginDaemonTest {
   }
 
   @Test
-  public void shouldFirePendingOnlyToStoredUri() throws Exception {
+  public void shouldFirePendingOnlyToRemainingUris() throws Exception {
     String suffix1 = "replica1";
     String suffix2 = "replica2";
     Project.NameKey target1 = createTestProject(project + suffix1);
@@ -463,7 +465,16 @@ public class ReplicationIT extends LightweightPluginDaemonTest {
     String changeRef = createChange().getPatchSet().refName();
 
     tasksStorage.disableDeleteForTesting(false);
-    changeReplicationTasksForRemote(changeRef, remote1).forEach(tasksStorage::delete);
+    changeReplicationTasksForRemote(changeRef, remote1)
+        .forEach(
+            (update) -> {
+              try {
+                UriUpdates uriUpdates = TestUriUpdates.create(update);
+                tasksStorage.start(uriUpdates);
+                tasksStorage.finish(uriUpdates);
+              } catch (URISyntaxException e) {
+              }
+            });
     tasksStorage.disableDeleteForTesting(true);
 
     setReplicationDestination(remote1, suffix1, ALL_PROJECTS);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
index 38d5421..c102e14 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
@@ -22,6 +22,7 @@ import static org.junit.Assert.assertTrue;
 import com.google.common.jimfs.Configuration;
 import com.google.common.jimfs.Jimfs;
 import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;
+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.UriUpdates;
 import java.net.URISyntaxException;
 import java.nio.file.FileSystem;
 import java.nio.file.Path;
@@ -42,12 +43,14 @@ public class ReplicationTasksStorageTest {
   protected ReplicationTasksStorage storage;
   protected FileSystem fileSystem;
   protected Path storageSite;
+  protected UriUpdates uriUpdates;
 
   @Before
   public void setUp() throws Exception {
     fileSystem = Jimfs.newFileSystem(Configuration.unix());
     storageSite = fileSystem.getPath("replication_site");
     storage = new ReplicationTasksStorage(storageSite);
+    uriUpdates = TestUriUpdates.create(REF_UPDATE);
   }
 
   @After
@@ -67,9 +70,10 @@ public class ReplicationTasksStorageTest {
   }
 
   @Test
-  public void canDeletePersistedUpdate() throws Exception {
+  public void canFinishPersistedUpdate() throws Exception {
     storage.create(REF_UPDATE);
-    storage.delete(REF_UPDATE);
+    storage.start(uriUpdates);
+    storage.finish(uriUpdates);
     assertThat(storage.list()).isEmpty();
   }
 
@@ -84,7 +88,8 @@ public class ReplicationTasksStorageTest {
     assertContainsExactly(storage, REF_UPDATE);
     assertContainsExactly(persistedView, REF_UPDATE);
 
-    storage.delete(REF_UPDATE);
+    storage.start(uriUpdates);
+    storage.finish(uriUpdates);
     assertThat(storage.list()).isEmpty();
     assertThat(persistedView.list()).isEmpty();
   }
@@ -113,20 +118,23 @@ public class ReplicationTasksStorageTest {
   }
 
   @Test
-  public void canDeleteDifferentUris() throws Exception {
+  public void canFinishDifferentUris() throws Exception {
     ReplicateRefUpdate updateB =
         new ReplicateRefUpdate(
             PROJECT,
             REF,
             getUrish("ssh://example.com/" + PROJECT + ".git"), // uses ssh not http
             REMOTE);
+    UriUpdates uriUpdatesB = TestUriUpdates.create(updateB);
     storage.create(REF_UPDATE);
     storage.create(updateB);
+    storage.start(uriUpdates);
+    storage.start(uriUpdatesB);
 
-    storage.delete(REF_UPDATE);
+    storage.finish(uriUpdates);
     assertContainsExactly(storage, updateB);
 
-    storage.delete(updateB);
+    storage.finish(uriUpdatesB);
     assertThat(storage.list()).isEmpty();
   }
 
@@ -158,47 +166,55 @@ public class ReplicationTasksStorageTest {
   }
 
   @Test
-  public void canDeleteMulipleRefsForSameUri() throws Exception {
-    ReplicateRefUpdate refA = new ReplicateRefUpdate(PROJECT, "refA", URISH, REMOTE);
-    ReplicateRefUpdate refB = new ReplicateRefUpdate(PROJECT, "refB", URISH, REMOTE);
-    storage.create(refA);
-    storage.create(refB);
-
-    storage.delete(refA);
-    assertContainsExactly(storage, refB);
-
-    storage.delete(refB);
+  public void canFinishMulipleRefsForSameUri() throws Exception {
+    ReplicateRefUpdate refUpdateA = new ReplicateRefUpdate(PROJECT, "refA", URISH, REMOTE);
+    ReplicateRefUpdate refUpdateB = new ReplicateRefUpdate(PROJECT, "refB", URISH, REMOTE);
+    UriUpdates uriUpdatesA = TestUriUpdates.create(refUpdateA);
+    UriUpdates uriUpdatesB = TestUriUpdates.create(refUpdateB);
+    storage.create(refUpdateA);
+    storage.create(refUpdateB);
+    storage.start(uriUpdatesA);
+    storage.start(uriUpdatesB);
+
+    storage.finish(uriUpdatesA);
+    assertContainsExactly(storage, refUpdateB);
+
+    storage.finish(uriUpdatesB);
     assertThat(storage.list()).isEmpty();
   }
 
   @Test(expected = Test.None.class /* no exception expected */)
-  public void illegalDeleteNonPersistedIsGraceful() throws Exception {
-    storage.delete(REF_UPDATE);
+  public void illegalFinishNonPersistedIsGraceful() throws Exception {
+    storage.finish(uriUpdates);
   }
 
   @Test(expected = Test.None.class /* no exception expected */)
-  public void illegalDoubleDeleteIsGraceful() throws Exception {
+  public void illegalDoubleFinishIsGraceful() throws Exception {
     storage.create(REF_UPDATE);
-    storage.delete(REF_UPDATE);
+    storage.start(uriUpdates);
+    storage.finish(uriUpdates);
 
-    storage.delete(REF_UPDATE);
+    storage.finish(uriUpdates);
   }
 
   @Test(expected = Test.None.class /* no exception expected */)
-  public void illegalDoubleDeleteDifferentUriIsGraceful() throws Exception {
+  public void illegalDoubleFinishDifferentUriIsGraceful() throws Exception {
     ReplicateRefUpdate updateB =
         new ReplicateRefUpdate(
             PROJECT,
             REF,
             getUrish("ssh://example.com/" + PROJECT + ".git"), // uses ssh not http
             REMOTE);
+    UriUpdates uriUpdatesB = TestUriUpdates.create(updateB);
     storage.create(REF_UPDATE);
     storage.create(updateB);
-    storage.delete(REF_UPDATE);
-    storage.delete(updateB);
+    storage.start(uriUpdates);
+    storage.start(uriUpdatesB);
+    storage.finish(uriUpdates);
+    storage.finish(uriUpdatesB);
 
-    storage.delete(REF_UPDATE);
-    storage.delete(updateB);
+    storage.finish(uriUpdates);
+    storage.finish(uriUpdatesB);
     assertThat(storage.list()).isEmpty();
   }
 
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java b/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java
new file mode 100644
index 0000000..b9e9701
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java
@@ -0,0 +1,52 @@
+// 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.auto.value.AutoValue;
+import com.google.gerrit.entities.Project;
+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;
+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.UriUpdates;
+import java.net.URISyntaxException;
+import java.util.Collections;
+import java.util.Set;
+import org.eclipse.jgit.transport.URIish;
+
+@AutoValue
+public abstract class TestUriUpdates implements UriUpdates {
+  public static TestUriUpdates create(ReplicateRefUpdate update) throws URISyntaxException {
+    return create(
+        Project.nameKey(update.project),
+        new URIish(update.uri),
+        update.remote,
+        Collections.singleton(update.ref));
+  }
+
+  public static TestUriUpdates create(
+      Project.NameKey project, URIish uri, String remote, Set<String> refs) {
+    return new AutoValue_TestUriUpdates(project, uri, remote, refs);
+  }
+
+  @Override
+  public abstract Project.NameKey getProjectNameKey();
+
+  @Override
+  public abstract URIish getURI();
+
+  @Override
+  public abstract String getRemoteName();
+
+  @Override
+  public abstract Set<String> getRefs();
+}
-- 
2.24.3 (Apple Git-128)