From 064eaebd54719e0ae13a30802cc0dff3125ac797 Mon Sep 17 00:00:00 2001
From: Edwin Kempin <ekempin@google.com>
Date: Thu, 1 Aug 2019 09:54:37 +0200
Subject: [PATCH] Document and test that plugins can throw RuntimeExceptions
 from ETag computation

If an error is encountered during the ETag computation in a plugin, the
plugin can indicate this by throwing any RuntimeException. In this case
no value will be included in the change ETag computation. This means if
the error is transient, the ETag will differ when the computation
succeeds on a follow-up run.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I08a91bfae5d37fe881ccee491f73ef5b35b4c180
---
 .../server/change/ChangeETagComputation.java  |  5 +++++
 .../acceptance/api/change/ChangeIT.java       | 19 +++++++++++++++++++
 .../acceptance/rest/change/ActionsIT.java     | 19 +++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/java/com/google/gerrit/server/change/ChangeETagComputation.java b/java/com/google/gerrit/server/change/ChangeETagComputation.java
index 5f032008ff..463989b58d 100644
--- a/java/com/google/gerrit/server/change/ChangeETagComputation.java
+++ b/java/com/google/gerrit/server/change/ChangeETagComputation.java
@@ -50,6 +50,11 @@ public interface ChangeETagComputation {
    * <p><strong>Note:</strong> Change ETags are computed very frequently and the computation must be
    * cheap. Take good care to not perform any expensive computations when implementing this.
    *
+   * <p>If an error is encountered during the ETag computation the plugin can indicate this by
+   * throwing any RuntimeException. In this case no value will be included in the change ETag
+   * computation. This means if the error is transient, the ETag will differ when the computation
+   * succeeds on a follow-up run.
+   *
    * @param projectName the name of the project that contains the change
    * @param changeId ID of the change for which the ETag should be computed
    * @return the ETag
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 7fc1134aea..be4162e9a1 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -87,6 +87,7 @@ import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.common.data.LabelFunction;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.annotations.Exports;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.AddReviewerResult;
@@ -2182,6 +2183,24 @@ public class ChangeIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void throwingExceptionFromETagComputationDoesNotBreakGerrit() throws Exception {
+    PushOneCommit.Result r = createChange();
+    String oldETag = parseResource(r).getETag();
+
+    RegistrationHandle registrationHandle =
+        changeETagComputations.add(
+            "gerrit",
+            (p, id) -> {
+              throw new StorageException("exception during test");
+            });
+    try {
+      assertThat(parseResource(r).getETag()).isEqualTo(oldETag);
+    } finally {
+      registrationHandle.remove();
+    }
+  }
+
   @Test
   public void emailNotificationForFileLevelComment() throws Exception {
     String changeId = createChange().getChangeId();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
index 94ad94f7d4..ae212b6bab 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -25,6 +25,7 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestProjectInput;
 import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.api.changes.ActionVisitor;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.client.ListChangesOption;
@@ -235,6 +236,24 @@ public class ActionsIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void throwingExceptionFromETagComputationDoesNotBreakGerrit() throws Exception {
+    String change = createChange().getChangeId();
+    String oldETag = getETag(change);
+
+    RegistrationHandle registrationHandle =
+        changeETagComputations.add(
+            "gerrit",
+            (p, id) -> {
+              throw new StorageException("exception during test");
+            });
+    try {
+      assertThat(getETag(change)).isEqualTo(oldETag);
+    } finally {
+      registrationHandle.remove();
+    }
+  }
+
   @Test
   public void revisionActionsTwoChangesInTopic_conflicting() throws Exception {
     String changeId = createChangeWithTopic().getChangeId();