Merge "Allow plugins to contribute a value to the change ETag computation"
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index 198e000..c8ea869 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -964,6 +964,11 @@
}
----
+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 @@
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
diff --git a/java/com/google/gerrit/server/change/ChangeETagComputation.java b/java/com/google/gerrit/server/change/ChangeETagComputation.java
new file mode 100644
index 0000000..5f03200
--- /dev/null
+++ b/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);
+}
diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index 19a4e5c..47986d9 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -38,6 +38,7 @@
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 @@
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 @@
PermissionBackend permissionBackend,
StarredChangesUtil starredChangesUtil,
ProjectCache projectCache,
+ PluginSetContext<ChangeETagComputation> changeETagComputation,
@Assisted ChangeNotes notes,
@Assisted CurrentUser user) {
this.accountCache = accountCache;
@@ -94,6 +97,7 @@
this.permissionBackend = permissionBackend;
this.starredChangesUtil = starredChangesUtil;
this.projectCache = projectCache;
+ this.changeETagComputation = changeETagComputation;
this.notes = notes;
this.user = user;
}
@@ -202,6 +206,13 @@
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/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index bbc5748..45b70b2 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -102,6 +102,7 @@
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 @@
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);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 1c1ef22..6d1c223 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -154,6 +154,7 @@
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 @@
@Inject private IndexConfig indexConfig;
@Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private DynamicSet<ChangeETagComputation> changeETagComputations;
@Inject
@Named("diff")
@@ -2153,6 +2155,34 @@
}
@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();
