From 96915b72dda4f5c39aa5358150707182d4a35e06 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 22 Jul 2019 14:57:40 +0200 Subject: [PATCH] Allow plugins to contribute a value to the change ETag computation Plugins can affect the result of the get change / get change details REST endpoints by: * providing plugin defined attributes to the 'plugins' field in ChangeInfo * implementing a SubmitRule which affects the computation of 'submittable' field in ChangeInfo If the plugin defined part of 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 new ChangeETagComputation interface plugins can now contribute to the change ETag computation and thus ensure that the ETag changes when the plugin data was changed. This way it can be ensured that callers do not see outdated ChangeInfos. Signed-off-by: Edwin Kempin Change-Id: I358e3d4fb2145a164ec1387fc1b40778959ad41e --- Documentation/dev-plugins.txt | 61 +++++++++++++++++++ .../server/change/ChangeETagComputation.java | 58 ++++++++++++++++++ .../gerrit/server/change/ChangeResource.java | 11 ++++ .../server/config/GerritGlobalModule.java | 2 + .../acceptance/api/change/ChangeIT.java | 30 +++++++++ 5 files changed, 162 insertions(+) create mode 100644 java/com/google/gerrit/server/change/ChangeETagComputation.java diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index 198e000d57..c8ea869dc5 100644 --- a/Documentation/dev-plugins.txt +++ b/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 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 0000000000..5f032008ff --- /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. + * + *

Plugins can affect the result of the get change / get change details REST endpoints by: + * + *

+ * + *

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. + * + *

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. + * + *

Note: 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 19a4e5ce80..47986d96ae 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.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; 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, @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(); } diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index bbc5748b88..45b70b2c07 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.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); diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 70884d5e02..b91840440f 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.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 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();