From b615c84ad8d3250f851aa9c80938c08422b48975 Mon Sep 17 00:00:00 2001
From: Edwin Kempin <ekempin@google.com>
Date: Wed, 31 Jul 2019 16:43:44 +0200
Subject: Consider change ETag computations from plugins when computing ETag
 for actions

Change I358e3d4fb added an extension point that allows plugins to
contribute to the ETag computation of changes. One case where plugins
need to contribute to the ETag computation is if they implement a
SubmitRule which is based on plugin data, because only then it is
guaranteed that callers always see current submittable information in
ChangeInfo.

In the case where a plugin implements a SubmitRule it is also important
to include the change ETag computations from plugins when the ETag for
revision actions is computed because the revision actions decide whether
the 'Submit' button should be shown. If change ETag computations from
plugins are not considered when the ETag for the revision actions is
computed it can happen that the submittable information in ChangeInfo
disagrees with the presence/absense of the 'Submit' button.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Id83533e7b67cc82737feb6615c8ec9277b676d1b
---
 .../gerrit/server/change/ChangeResource.java       | 15 ++++++-----
 .../gerrit/acceptance/rest/change/ActionsIT.java   | 30 ++++++++++++++++++++++
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index bb0040de15..d8d82c628d 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -197,6 +197,14 @@ public class ChangeResource implements RestResource, HasETag {
     for (ProjectState p : projectStateTree) {
       hashObjectId(h, p.getConfig().getRevision(), buf);
     }
+
+    changeETagComputation.runEach(
+        c -> {
+          String pluginETag = c.getETag(notes.getProjectName(), notes.getChangeId());
+          if (pluginETag != null) {
+            h.putString(pluginETag, UTF_8);
+          }
+        });
   }
 
   @Override
@@ -206,13 +214,6 @@ public class ChangeResource implements RestResource, HasETag {
       h.putString(starredChangesUtil.getObjectId(user.getAccountId(), getId()).name(), UTF_8);
     }
     prepareETag(h, user);
-    changeETagComputation.runEach(
-        c -> {
-          String pluginETag = c.getETag(notes.getProjectName(), notes.getChangeId());
-          if (pluginETag != null) {
-            h.putString(pluginETag, UTF_8);
-          }
-        });
     return h.hash().toString();
   }
 
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
index 5fb42da750..94ad94f7d4 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -36,6 +36,7 @@ import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.registration.RegistrationHandle;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.change.ChangeETagComputation;
 import com.google.gerrit.server.change.RevisionJson;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.testing.ConfigSuite;
@@ -58,6 +59,7 @@ public class ActionsIT extends AbstractDaemonTest {
   @Inject private DynamicSet<ActionVisitor> actionVisitors;
   @Inject private RequestScopeOperations requestScopeOperations;
   @Inject private RevisionJson.Factory revisionJsonFactory;
+  @Inject private DynamicSet<ChangeETagComputation> changeETagComputations;
 
   private RegistrationHandle visitorHandle;
 
@@ -205,6 +207,34 @@ public class ActionsIT extends AbstractDaemonTest {
     assertThat(etag2).isEqualTo(etag1);
   }
 
+  @Test
+  public void pluginCanContributeToETagComputation() throws Exception {
+    String change = createChange().getChangeId();
+    String oldETag = getETag(change);
+
+    RegistrationHandle registrationHandle = changeETagComputations.add("gerrit", (p, id) -> "foo");
+    try {
+      assertThat(getETag(change)).isNotEqualTo(oldETag);
+    } finally {
+      registrationHandle.remove();
+    }
+
+    assertThat(getETag(change)).isEqualTo(oldETag);
+  }
+
+  @Test
+  public void returningNullFromETagComputationDoesNotBreakGerrit() throws Exception {
+    String change = createChange().getChangeId();
+    String oldETag = getETag(change);
+
+    RegistrationHandle registrationHandle = changeETagComputations.add("gerrit", (p, id) -> null);
+    try {
+      assertThat(getETag(change)).isEqualTo(oldETag);
+    } finally {
+      registrationHandle.remove();
+    }
+  }
+
   @Test
   public void revisionActionsTwoChangesInTopic_conflicting() throws Exception {
     String changeId = createChangeWithTopic().getChangeId();
-- 
cgit v1.2.3