{"thread":{"id":"53985","subject":"[PATCH v1] TasksStorage: Remove delete() only used by tests","startedAt":"2020-08-05T07:16:50Z","lastAt":"2020-08-05T07:16:50Z","messageCount":1,"participants":["Adithya Chakilam"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"402901","messageId":"20200805071610.12249-1-achakila@codeaurora.org","threadId":"53985","inReplyTo":null,"subject":"[PATCH v1] TasksStorage: Remove delete() only used by tests","fromName":"Adithya Chakilam","fromEmail":"achakila@codeaurora.org","sentAt":"2020-08-05T07:16:10Z","receivedAt":"2020-08-05T07:16:50Z","isPatch":true,"sender":{"key":"achakila@codeaurora.org","avatar":null},"body":"From: Nasser Grainawi <nasser@codeaurora.org>\n\nIn general we shouldn't have methods that provide behavior only for\ntests. Callers of delete() in pre-3.1 code should move to start() and\nfinish() in 3.1. To facilitate that, update start(), reset(), and\nfinish() methods to not require a PushOne object. These methods can\ninstead use a new interface that PushOne already can implement without\nfunctional changes.\n\nChange-Id: Id82d2ec3125075832134c8dbe56b342e5a874bbc\n---\n .../gerrit/plugins/replication/PushOne.java   |  8 ++-\n .../replication/ReplicationTasksStorage.java  | 52 +++++++-------\n .../plugins/replication/ReplicationIT.java    | 15 +++-\n .../ReplicationTasksStorageTest.java          | 68 ++++++++++++-------\n .../plugins/replication/TestUriUpdates.java   | 52 ++++++++++++++\n 5 files changed, 139 insertions(+), 56 deletions(-)\n create mode 100644 src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java\n\ndiff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java\nindex 634f44d..3124940 100644\n--- a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java\n+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java\n@@ -84,7 +84,7 @@ import org.slf4j.MDC;\n  * <p>Instance members are protected by the lock within PushQueue. Callers must take that lock to\n  * ensure they are working with a current view of the object.\n  */\n-class PushOne implements ProjectRunnable, CanceledWhileRunning {\n+class PushOne implements ProjectRunnable, CanceledWhileRunning, ReplicationTasksStorage.UriUpdates {\n   private final ReplicationStateListener stateLog;\n   static final String ALL_REFS = \"..all..\";\n   static final String ID_MDC_KEY = \"pushOneId\";\n@@ -230,7 +230,8 @@ class PushOne implements ProjectRunnable, CanceledWhileRunning {\n     return canceled || canceledWhileRunning.get();\n   }\n \n-  URIish getURI() {\n+  @Override\n+  public URIish getURI() {\n     return uri;\n   }\n \n@@ -244,7 +245,8 @@ class PushOne implements ProjectRunnable, CanceledWhileRunning {\n     }\n   }\n \n-  Set<String> getRefs() {\n+  @Override\n+  public Set<String> getRefs() {\n     return pushAllRefs ? Sets.newHashSet(ALL_REFS) : delta;\n   }\n \ndiff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java\nindex 370d48b..8a5aba2 100644\n--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java\n+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java\n@@ -19,6 +19,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;\n import com.google.common.annotations.VisibleForTesting;\n import com.google.common.flogger.FluentLogger;\n import com.google.common.hash.Hashing;\n+import com.google.gerrit.entities.Project;\n import com.google.gson.Gson;\n import com.google.inject.Inject;\n import com.google.inject.ProvisionException;\n@@ -32,6 +33,7 @@ import java.nio.file.Path;\n import java.nio.file.StandardCopyOption;\n import java.util.ArrayList;\n import java.util.List;\n+import java.util.Set;\n import org.eclipse.jgit.lib.ObjectId;\n import org.eclipse.jgit.transport.URIish;\n \n@@ -68,8 +70,12 @@ public class ReplicationTasksStorage {\n     public final String uri;\n     public final String remote;\n \n-    public ReplicateRefUpdate(PushOne push, String ref) {\n-      this(push.getProjectNameKey().get(), ref, push.getURI(), push.getRemoteName());\n+    public ReplicateRefUpdate(UriUpdates uriUpdates, String ref) {\n+      this(\n+          uriUpdates.getProjectNameKey().get(),\n+          ref,\n+          uriUpdates.getURI(),\n+          uriUpdates.getRemoteName());\n     }\n \n     public ReplicateRefUpdate(String project, String ref, URIish uri, String remote) {\n@@ -85,6 +91,16 @@ public class ReplicationTasksStorage {\n     }\n   }\n \n+  public interface UriUpdates {\n+    Project.NameKey getProjectNameKey();\n+\n+    URIish getURI();\n+\n+    String getRemoteName();\n+\n+    Set<String> getRefs();\n+  }\n+\n   private static final Gson GSON = new Gson();\n \n   private final Path refUpdates;\n@@ -114,20 +130,15 @@ public class ReplicationTasksStorage {\n     this.disableDeleteForTesting = deleteDisabled;\n   }\n \n-  @VisibleForTesting\n-  public void delete(ReplicateRefUpdate r) {\n-    new Task(r).delete();\n-  }\n-\n-  public synchronized void start(PushOne push) {\n-    for (String ref : push.getRefs()) {\n-      new Task(new ReplicateRefUpdate(push, ref)).start();\n+  public synchronized void start(UriUpdates uriUpdates) {\n+    for (String ref : uriUpdates.getRefs()) {\n+      new Task(new ReplicateRefUpdate(uriUpdates, ref)).start();\n     }\n   }\n \n-  public synchronized void reset(PushOne push) {\n-    for (String ref : push.getRefs()) {\n-      new Task(new ReplicateRefUpdate(push, ref)).reset();\n+  public synchronized void reset(UriUpdates uriUpdates) {\n+    for (String ref : uriUpdates.getRefs()) {\n+      new Task(new ReplicateRefUpdate(uriUpdates, ref)).reset();\n     }\n   }\n \n@@ -137,9 +148,9 @@ public class ReplicationTasksStorage {\n     }\n   }\n \n-  public synchronized void finish(PushOne push) {\n-    for (String ref : push.getRefs()) {\n-      new Task(new ReplicateRefUpdate(push, ref)).finish();\n+  public synchronized void finish(UriUpdates uriUpdates) {\n+    for (String ref : uriUpdates.getRefs()) {\n+      new Task(new ReplicateRefUpdate(uriUpdates, ref)).finish();\n     }\n   }\n \n@@ -261,15 +272,6 @@ public class ReplicationTasksStorage {\n       }\n     }\n \n-    public void delete() {\n-      try {\n-        Files.deleteIfExists(waiting);\n-        Files.deleteIfExists(running);\n-      } catch (IOException e) {\n-        logger.atSevere().withCause(e).log(\"Error while deleting task %s\", taskKey);\n-      }\n-    }\n-\n     private void rename(Path from, Path to) {\n       try {\n         logger.atFine().log(\"RENAME %s to %s %s\", from, to, updateLog());\ndiff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java\nindex 31cd75d..bb0f283 100644\n--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java\n+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java\n@@ -36,7 +36,9 @@ import com.google.gerrit.server.config.SitePaths;\n import com.google.inject.Inject;\n import com.google.inject.Key;\n import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;\n+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.UriUpdates;\n import java.io.IOException;\n+import java.net.URISyntaxException;\n import java.nio.file.DirectoryStream;\n import java.nio.file.Files;\n import java.nio.file.Path;\n@@ -449,7 +451,7 @@ public class ReplicationIT extends LightweightPluginDaemonTest {\n   }\n \n   @Test\n-  public void shouldFirePendingOnlyToStoredUri() throws Exception {\n+  public void shouldFirePendingOnlyToRemainingUris() throws Exception {\n     String suffix1 = \"replica1\";\n     String suffix2 = \"replica2\";\n     Project.NameKey target1 = createTestProject(project + suffix1);\n@@ -463,7 +465,16 @@ public class ReplicationIT extends LightweightPluginDaemonTest {\n     String changeRef = createChange().getPatchSet().refName();\n \n     tasksStorage.disableDeleteForTesting(false);\n-    changeReplicationTasksForRemote(changeRef, remote1).forEach(tasksStorage::delete);\n+    changeReplicationTasksForRemote(changeRef, remote1)\n+        .forEach(\n+            (update) -> {\n+              try {\n+                UriUpdates uriUpdates = TestUriUpdates.create(update);\n+                tasksStorage.start(uriUpdates);\n+                tasksStorage.finish(uriUpdates);\n+              } catch (URISyntaxException e) {\n+              }\n+            });\n     tasksStorage.disableDeleteForTesting(true);\n \n     setReplicationDestination(remote1, suffix1, ALL_PROJECTS);\ndiff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java\nindex 38d5421..c102e14 100644\n--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java\n+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java\n@@ -22,6 +22,7 @@ import static org.junit.Assert.assertTrue;\n import com.google.common.jimfs.Configuration;\n import com.google.common.jimfs.Jimfs;\n import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;\n+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.UriUpdates;\n import java.net.URISyntaxException;\n import java.nio.file.FileSystem;\n import java.nio.file.Path;\n@@ -42,12 +43,14 @@ public class ReplicationTasksStorageTest {\n   protected ReplicationTasksStorage storage;\n   protected FileSystem fileSystem;\n   protected Path storageSite;\n+  protected UriUpdates uriUpdates;\n \n   @Before\n   public void setUp() throws Exception {\n     fileSystem = Jimfs.newFileSystem(Configuration.unix());\n     storageSite = fileSystem.getPath(\"replication_site\");\n     storage = new ReplicationTasksStorage(storageSite);\n+    uriUpdates = TestUriUpdates.create(REF_UPDATE);\n   }\n \n   @After\n@@ -67,9 +70,10 @@ public class ReplicationTasksStorageTest {\n   }\n \n   @Test\n-  public void canDeletePersistedUpdate() throws Exception {\n+  public void canFinishPersistedUpdate() throws Exception {\n     storage.create(REF_UPDATE);\n-    storage.delete(REF_UPDATE);\n+    storage.start(uriUpdates);\n+    storage.finish(uriUpdates);\n     assertThat(storage.list()).isEmpty();\n   }\n \n@@ -84,7 +88,8 @@ public class ReplicationTasksStorageTest {\n     assertContainsExactly(storage, REF_UPDATE);\n     assertContainsExactly(persistedView, REF_UPDATE);\n \n-    storage.delete(REF_UPDATE);\n+    storage.start(uriUpdates);\n+    storage.finish(uriUpdates);\n     assertThat(storage.list()).isEmpty();\n     assertThat(persistedView.list()).isEmpty();\n   }\n@@ -113,20 +118,23 @@ public class ReplicationTasksStorageTest {\n   }\n \n   @Test\n-  public void canDeleteDifferentUris() throws Exception {\n+  public void canFinishDifferentUris() throws Exception {\n     ReplicateRefUpdate updateB =\n         new ReplicateRefUpdate(\n             PROJECT,\n             REF,\n             getUrish(\"ssh://example.com/\" + PROJECT + \".git\"), // uses ssh not http\n             REMOTE);\n+    UriUpdates uriUpdatesB = TestUriUpdates.create(updateB);\n     storage.create(REF_UPDATE);\n     storage.create(updateB);\n+    storage.start(uriUpdates);\n+    storage.start(uriUpdatesB);\n \n-    storage.delete(REF_UPDATE);\n+    storage.finish(uriUpdates);\n     assertContainsExactly(storage, updateB);\n \n-    storage.delete(updateB);\n+    storage.finish(uriUpdatesB);\n     assertThat(storage.list()).isEmpty();\n   }\n \n@@ -158,47 +166,55 @@ public class ReplicationTasksStorageTest {\n   }\n \n   @Test\n-  public void canDeleteMulipleRefsForSameUri() throws Exception {\n-    ReplicateRefUpdate refA = new ReplicateRefUpdate(PROJECT, \"refA\", URISH, REMOTE);\n-    ReplicateRefUpdate refB = new ReplicateRefUpdate(PROJECT, \"refB\", URISH, REMOTE);\n-    storage.create(refA);\n-    storage.create(refB);\n-\n-    storage.delete(refA);\n-    assertContainsExactly(storage, refB);\n-\n-    storage.delete(refB);\n+  public void canFinishMulipleRefsForSameUri() throws Exception {\n+    ReplicateRefUpdate refUpdateA = new ReplicateRefUpdate(PROJECT, \"refA\", URISH, REMOTE);\n+    ReplicateRefUpdate refUpdateB = new ReplicateRefUpdate(PROJECT, \"refB\", URISH, REMOTE);\n+    UriUpdates uriUpdatesA = TestUriUpdates.create(refUpdateA);\n+    UriUpdates uriUpdatesB = TestUriUpdates.create(refUpdateB);\n+    storage.create(refUpdateA);\n+    storage.create(refUpdateB);\n+    storage.start(uriUpdatesA);\n+    storage.start(uriUpdatesB);\n+\n+    storage.finish(uriUpdatesA);\n+    assertContainsExactly(storage, refUpdateB);\n+\n+    storage.finish(uriUpdatesB);\n     assertThat(storage.list()).isEmpty();\n   }\n \n   @Test(expected = Test.None.class /* no exception expected */)\n-  public void illegalDeleteNonPersistedIsGraceful() throws Exception {\n-    storage.delete(REF_UPDATE);\n+  public void illegalFinishNonPersistedIsGraceful() throws Exception {\n+    storage.finish(uriUpdates);\n   }\n \n   @Test(expected = Test.None.class /* no exception expected */)\n-  public void illegalDoubleDeleteIsGraceful() throws Exception {\n+  public void illegalDoubleFinishIsGraceful() throws Exception {\n     storage.create(REF_UPDATE);\n-    storage.delete(REF_UPDATE);\n+    storage.start(uriUpdates);\n+    storage.finish(uriUpdates);\n \n-    storage.delete(REF_UPDATE);\n+    storage.finish(uriUpdates);\n   }\n \n   @Test(expected = Test.None.class /* no exception expected */)\n-  public void illegalDoubleDeleteDifferentUriIsGraceful() throws Exception {\n+  public void illegalDoubleFinishDifferentUriIsGraceful() throws Exception {\n     ReplicateRefUpdate updateB =\n         new ReplicateRefUpdate(\n             PROJECT,\n             REF,\n             getUrish(\"ssh://example.com/\" + PROJECT + \".git\"), // uses ssh not http\n             REMOTE);\n+    UriUpdates uriUpdatesB = TestUriUpdates.create(updateB);\n     storage.create(REF_UPDATE);\n     storage.create(updateB);\n-    storage.delete(REF_UPDATE);\n-    storage.delete(updateB);\n+    storage.start(uriUpdates);\n+    storage.start(uriUpdatesB);\n+    storage.finish(uriUpdates);\n+    storage.finish(uriUpdatesB);\n \n-    storage.delete(REF_UPDATE);\n-    storage.delete(updateB);\n+    storage.finish(uriUpdates);\n+    storage.finish(uriUpdatesB);\n     assertThat(storage.list()).isEmpty();\n   }\n \ndiff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java b/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java\nnew file mode 100644\nindex 0000000..b9e9701\n--- /dev/null\n+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java\n@@ -0,0 +1,52 @@\n+// Copyright (C) 2020 The Android Open Source Project\n+//\n+// Licensed under the Apache License, Version 2.0 (the \"License\");\n+// you may not use this file except in compliance with the License.\n+// You may obtain a copy of the License at\n+//\n+// http://www.apache.org/licenses/LICENSE-2.0\n+//\n+// Unless required by applicable law or agreed to in writing, software\n+// distributed under the License is distributed on an \"AS IS\" BASIS,\n+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\n+// See the License for the specific language governing permissions and\n+// limitations under the License.\n+\n+package com.googlesource.gerrit.plugins.replication;\n+\n+import com.google.auto.value.AutoValue;\n+import com.google.gerrit.entities.Project;\n+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;\n+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.UriUpdates;\n+import java.net.URISyntaxException;\n+import java.util.Collections;\n+import java.util.Set;\n+import org.eclipse.jgit.transport.URIish;\n+\n+@AutoValue\n+public abstract class TestUriUpdates implements UriUpdates {\n+  public static TestUriUpdates create(ReplicateRefUpdate update) throws URISyntaxException {\n+    return create(\n+        Project.nameKey(update.project),\n+        new URIish(update.uri),\n+        update.remote,\n+        Collections.singleton(update.ref));\n+  }\n+\n+  public static TestUriUpdates create(\n+      Project.NameKey project, URIish uri, String remote, Set<String> refs) {\n+    return new AutoValue_TestUriUpdates(project, uri, remote, refs);\n+  }\n+\n+  @Override\n+  public abstract Project.NameKey getProjectNameKey();\n+\n+  @Override\n+  public abstract URIish getURI();\n+\n+  @Override\n+  public abstract String getRemoteName();\n+\n+  @Override\n+  public abstract Set<String> getRefs();\n+}\n-- \n2.24.3 (Apple Git-128)\n\n"}]}