Merge "Allow plugins to contribute a value to the change ETag computation"

Documentation/dev-plugins.txt

@@ -964,6 +964,11 @@ Output:
 }
 ----
 
+Implementors of the `ChangeAttributeFactory` interface should check whether
+they need to contribute to the link:#change-etag-computation[change ETag
+computation] to prevent callers using ETags from potentially seeing outdated
+plugin attributes.
+
 [[simple-configuration]]
 == Simple Configuration in `gerrit.config`
 
@@ -2713,6 +2718,62 @@ to change in order to be compliant. These requirements should be kept once they
 are met, but marked as `OK`. If the requirements were not displayed, reviewers
 would need to use their precious time to manually check that they were met.
 
+Implementors of the `SubmitRule` interface should check whether they need to
+contribute to the link:#change-etag-computation[change ETag computation] to
+prevent callers using ETags from potentially seeing outdated submittability
+information.
+
+[[change-etag-computation]]
+== Change ETag Computation
+
+By implementing the `com.google.gerrit.server.change.ChangeETagComputation`
+interface plugins can contribute a value to the change ETag computation.
+
+Plugins can affect the result of the get change / get change details REST
+endpoints by:
+
+* providing link:#query_attributes[plugin defined attributes] in
+  link:rest-api-changes.html#change-info[ChangeInfo]
+* implementing a link:#pre-submit-evaluator[pre-submit evaluator] which affects
+  the computation of `submittable` field in
+  link:rest-api-changes.html#change-info[ChangeInfo]
+
+If the plugin defined part of link:rest-api-changes.html#change-info[
+ChangeInfo] depends on plugin specific data, callers that use change ETags to
+avoid unneeded recomputations of ChangeInfos may see outdated plugin attributes
+and/or outdated submittable information, because a ChangeInfo is only reloaded
+if the change ETag changes.
+
+By implementating the `com.google.gerrit.server.change.ChangeETagComputation`
+interface plugins can contribute to the ETag computation and thus ensure that
+the change ETag changes when the plugin data was changed. This way it can be
+ensured that callers do not see outdated ChangeInfos.
+
+IMPORTANT: Change ETags are computed very frequently and the computation must
+be cheap. Take good care to not perform any expensive computations when
+implementing this.
+
+[source, java]
+----
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.hash.Hasher;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.change.ChangeETagComputation;
+
+public class MyPluginChangeETagComputation implements ChangeETagComputation {
+  public String getETag(Project.NameKey projectName, Change.Id changeId) {
+    Hasher hasher = Hashing.murmur3_128().newHasher();
+
+    // Add hashes for all plugin-specific data that affects change infos.
+    hasher.putString(sha1OfPluginSpecificChangeRef, UTF_8);
+
+    return hasher.hash().toString();
+  }
+}
+----
+
 [[quota-enforcer]]
 == Quota Enforcer
 

java/com/google/gerrit/server/change/ChangeETagComputation.java

@@ -0,0 +1,58 @@
+// Copyright (C) 2019 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.google.gerrit.server.change;
+
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
+
+/**
+ * Allows plugins to contribute a value to the change ETag computation.
+ *
+ * <p>Plugins can affect the result of the get change / get change details REST endpoints by:
+ *
+ * <ul>
+ *   <li>providing plugin defined attributes to {@link
+ *       com.google.gerrit.extensions.common.ChangeInfo#plugins} (see {@link
+ *       ChangeAttributeFactory})
+ *   <li>implementing a {@link com.google.gerrit.server.rules.SubmitRule} which affects the
+ *       computation of {@link com.google.gerrit.extensions.common.ChangeInfo#submittable}
+ * </ul>
+ *
+ * <p>If the plugin defined part of {@link com.google.gerrit.extensions.common.ChangeInfo} depends
+ * on plugin specific data, callers that use the change ETags to avoid unneeded recomputations of
+ * ChangeInfos may see outdated plugin attributes and/or outdated submittable information, because a
+ * ChangeInfo is only reloaded if the change ETag changes.
+ *
+ * <p>By implementating this interface plugins can contribute to the change ETag computation and
+ * thus ensure that the ETag changes when the plugin data was changed. This way it is ensured that
+ * callers do not see outdated ChangeInfos.
+ *
+ * @see ChangeResource#getETag()
+ */
+@ExtensionPoint
+public interface ChangeETagComputation {
+  /**
+   * Computes an ETag of plugin-specific data for the given change.
+   *
+   * <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.
+   *
+   * @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
+   */
+  String getETag(Project.NameKey projectName, Change.Id changeId);
+}

java/com/google/gerrit/server/change/ChangeResource.java

@@ -38,6 +38,7 @@ import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.plugincontext.PluginSetContext;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.inject.Inject;
@@ -75,6 +76,7 @@ public class ChangeResource implements RestResource, HasETag {
   private final PermissionBackend permissionBackend;
   private final StarredChangesUtil starredChangesUtil;
   private final ProjectCache projectCache;
+  private final PluginSetContext<ChangeETagComputation> changeETagComputation;
   private final ChangeNotes notes;
   private final CurrentUser user;
 
@@ -86,6 +88,7 @@ public class ChangeResource implements RestResource, HasETag {
       PermissionBackend permissionBackend,
       StarredChangesUtil starredChangesUtil,
       ProjectCache projectCache,
+      PluginSetContext<ChangeETagComputation> changeETagComputation,
       @Assisted ChangeNotes notes,
       @Assisted CurrentUser user) {
     this.accountCache = accountCache;
@@ -94,6 +97,7 @@ public class ChangeResource implements RestResource, HasETag {
     this.permissionBackend = permissionBackend;
     this.starredChangesUtil = starredChangesUtil;
     this.projectCache = projectCache;
+    this.changeETagComputation = changeETagComputation;
     this.notes = notes;
     this.user = user;
   }
@@ -202,6 +206,13 @@ 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();
   }
 

java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -102,6 +102,7 @@ import com.google.gerrit.server.cache.CacheRemovalListener;
 import com.google.gerrit.server.change.AbandonOp;
 import com.google.gerrit.server.change.AccountPatchReviewStore;
 import com.google.gerrit.server.change.ChangeAttributeFactory;
+import com.google.gerrit.server.change.ChangeETagComputation;
 import com.google.gerrit.server.change.ChangeFinder;
 import com.google.gerrit.server.change.ChangeJson;
 import com.google.gerrit.server.change.ChangeKindCacheImpl;
@@ -388,6 +389,7 @@ public class GerritGlobalModule extends FactoryModule {
     DynamicSet.setOf(binder(), PerformanceLogger.class);
     DynamicSet.setOf(binder(), RequestListener.class);
     DynamicSet.bind(binder(), RequestListener.class).to(TraceRequestListener.class);
+    DynamicSet.setOf(binder(), ChangeETagComputation.class);
 
     DynamicMap.mapOf(binder(), MailFilter.class);
     bind(MailFilter.class).annotatedWith(Exports.named("ListMailFilter")).to(ListMailFilter.class);

javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -154,6 +154,7 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.StarredChangesUtil;
+import com.google.gerrit.server.change.ChangeETagComputation;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.git.ChangeMessageModifier;
 import com.google.gerrit.server.group.SystemGroupBackend;
@@ -217,6 +218,7 @@ public class ChangeIT extends AbstractDaemonTest {
   @Inject private IndexConfig indexConfig;
   @Inject private ProjectOperations projectOperations;
   @Inject private RequestScopeOperations requestScopeOperations;
+  @Inject private DynamicSet<ChangeETagComputation> changeETagComputations;
 
   @Inject
   @Named("diff")
@@ -2152,6 +2154,34 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(rsrc.getETag()).isNotEqualTo(oldETag);
   }
 
+  @Test
+  public void pluginCanContributeToETagComputation() throws Exception {
+    PushOneCommit.Result r = createChange();
+    String oldETag = parseResource(r).getETag();
+
+    RegistrationHandle registrationHandle = changeETagComputations.add("gerrit", (p, id) -> "foo");
+    try {
+      assertThat(parseResource(r).getETag()).isNotEqualTo(oldETag);
+    } finally {
+      registrationHandle.remove();
+    }
+
+    assertThat(parseResource(r).getETag()).isEqualTo(oldETag);
+  }
+
+  @Test
+  public void returningNullFromETagComputationDoesNotBreakGerrit() throws Exception {
+    PushOneCommit.Result r = createChange();
+    String oldETag = parseResource(r).getETag();
+
+    RegistrationHandle registrationHandle = changeETagComputations.add("gerrit", (p, id) -> null);
+    try {
+      assertThat(parseResource(r).getETag()).isEqualTo(oldETag);
+    } finally {
+      registrationHandle.remove();
+    }
+  }
+
   @Test
   public void emailNotificationForFileLevelComment() throws Exception {
     String changeId = createChange().getChangeId();