Fix async change message that lists owned path on PostReview
Support for posting the owned paths asynchronously when a code owner
approval is applied was implemented by change I129e611af.
At the moment the asynchronous message posting is triggered via the
OnPostReview#getChangeMessageAddOn extension point. This extension point
is intended to let plugins extend the change message that is posted by
PostReview.
Triggering the asynchronous message posting from this extension point
has issues:
* When the asynchronous message posting is triggered PostReview is not
done yet, i.e. it didn't commit its updates the change meta ref yet.
This means now there is a race between PostReview and the extension to
update the change meta ref, which makes it likely that one of the
updates fails due to LockFailure.
* Since there is a race with updating the change meta ref, the order in
which the messages are posted is undefined, but we want the PostReview
message to be posted first.
* If the update that is applied by PostReview fails with LockFailure,
the whole operation, including the triggering of async message, is
retried. This can lead to a situation where the change message with
the owned path is created twice:
1. posting the async message succeeds, but the update from PostReview
fails due to LockFailure (e.g. due to the problem described above,
but LockFailures are also common due to other reasons)
2. PostReview is retried
3. posting the update from PostReview succeeds, but triggers the async
message again which also succeeds (posting the async message is
also retried on LockFailure, so it will likely succeed if the
PostReview update is applied first)
Fix this by not triggering the async posting of the owned paths from the
OnPostReview#getChangeMessageAddOn extension point, but from the
CommentAddedListener#onCommentAdded extension point which is only
invoked after the PostReview update has been applied.
Bug: Google b/276879211
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I4d32542457de2f50b25eb7bf57f5f504720131ab
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
index cba9409..55421e9 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
@@ -16,6 +16,7 @@
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.events.CommentAddedListener;
import com.google.gerrit.extensions.events.ReviewerAddedListener;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicSet;
@@ -61,6 +62,7 @@
DynamicSet.bind(binder(), ExceptionHook.class).to(CodeOwnersExceptionHook.class);
DynamicSet.bind(binder(), OnPostReview.class).to(OnCodeOwnerApproval.class);
+ DynamicSet.bind(binder(), CommentAddedListener.class).to(OnCodeOwnerApproval.class);
DynamicSet.bind(binder(), OnPostReview.class).to(OnCodeOwnerOverride.class);
DynamicSet.bind(binder(), ReviewerAddedListener.class).to(CodeOwnersOnAddReviewer.class);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index e7c03b1..3977521 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -15,12 +15,18 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.CHANGE_MODIFICATION;
import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.PLUGIN;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.common.ApprovalInfo;
+import com.google.gerrit.extensions.events.CommentAddedListener;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -28,6 +34,7 @@
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.ChangeMessagesUtil;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -42,6 +49,7 @@
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.nio.file.Path;
import java.time.Instant;
@@ -63,7 +71,7 @@
* </ul>
*/
@Singleton
-class OnCodeOwnerApproval implements OnPostReview {
+class OnCodeOwnerApproval implements OnPostReview, CommentAddedListener {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private static final String TAG_ADD_CODE_OWNER_APPROVAL =
@@ -74,7 +82,9 @@
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
private final CodeOwnerMetrics codeOwnerMetrics;
+ private final ChangeNotes.Factory changeNotesFactory;
private final ChangeMessagesUtil changeMessageUtil;
+ private final Provider<CurrentUser> userProvider;
private final RetryHelper retryHelper;
@Inject
@@ -84,14 +94,18 @@
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerApprovalCheck codeOwnerApprovalCheck,
CodeOwnerMetrics codeOwnerMetrics,
+ ChangeNotes.Factory changeNotesFactory,
ChangeMessagesUtil changeMessageUtil,
+ Provider<CurrentUser> userProvider,
RetryHelper retryHelper) {
this.workQueue = workQueue;
this.oneOffRequestContext = oneOffRequestContext;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerApprovalCheck = codeOwnerApprovalCheck;
this.codeOwnerMetrics = codeOwnerMetrics;
+ this.changeNotesFactory = changeNotesFactory;
this.changeMessageUtil = changeMessageUtil;
+ this.userProvider = userProvider;
this.retryHelper = retryHelper;
}
@@ -108,11 +122,20 @@
int maxPathsInChangeMessage = codeOwnersConfig.getMaxPathsInChangeMessages();
if (codeOwnersConfig.isDisabled(changeNotes.getChange().getDest().branch())
|| maxPathsInChangeMessage <= 0) {
+ logger.atFine().log("skip extending the change message since message posting is disabled");
+ return Optional.empty();
+ }
+ if (codeOwnersConfig.enableAsyncMessageOnCodeOwnerApproval()) {
+ // To avoid adding latency to PostReview post the change message asynchronously from
+ // #onCommentAdded(Event) after PostReview is done.
+ logger.atFine().log(
+ "skip extending the change message since async message posting is enabled");
return Optional.empty();
}
// code owner approvals are only computed for the current patch set
if (!changeNotes.getChange().currentPatchSetId().equals(patchSet.id())) {
+ logger.atFine().log("skip extending the change message on non-current patch set");
return Optional.empty();
}
@@ -125,32 +148,6 @@
return Optional.empty();
}
- if (codeOwnersConfig.enableAsyncMessageOnCodeOwnerApproval()) {
- // post change message asynchronously to avoid adding latency to PostReview
- logger.atFine().log("schedule asynchronous posting of the change message");
- @SuppressWarnings("unused")
- WorkQueue.Task<?> possiblyIgnoredError =
- (WorkQueue.Task<?>)
- workQueue
- .getDefaultQueue()
- .submit(
- () -> {
- try (ManualRequestContext ignored =
- oneOffRequestContext.openAs(user.getAccountId())) {
- postChangeMessage(
- when,
- user,
- changeNotes,
- patchSet,
- oldApprovals,
- approvals,
- requiredApproval,
- maxPathsInChangeMessage);
- }
- });
- return Optional.empty();
- }
-
logger.atFine().log("post change message synchronously");
try (Timer0.Context ctx = codeOwnerMetrics.extendChangeMessageOnPostReview.start()) {
return buildMessageForCodeOwnerApproval(
@@ -181,6 +178,75 @@
}
}
+ @Override
+ public void onCommentAdded(Event event) {
+ Project.NameKey projectName = Project.nameKey(event.getChange().project);
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(projectName);
+ int maxPathsInChangeMessage = codeOwnersConfig.getMaxPathsInChangeMessages();
+ if (codeOwnersConfig.isDisabled(event.getChange().branch) || maxPathsInChangeMessage <= 0) {
+ logger.atFine().log("skip posting the change message since message posting is disabled");
+ return;
+ }
+ if (!codeOwnersConfig.enableAsyncMessageOnCodeOwnerApproval()) {
+ // The change message has already been synchronously extended by #getChangeMessageAddOn(...).
+ logger.atFine().log(
+ "skip posting the change message since async message posting is disabled");
+ return;
+ }
+
+ // post change message asynchronously to avoid adding latency to PostReview
+ logger.atFine().log("schedule asynchronous posting of the change message");
+
+ CurrentUser user = userProvider.get();
+ if (!user.isIdentifiedUser()) {
+ // cannot compute owned paths for non-identified user
+ logger.atFine().log(
+ "skip posting the change message for non-identified user %s", user.getLoggableName());
+ return;
+ }
+
+ Change.Id changeId = Change.id(event.getChange()._number);
+ ChangeNotes changeNotes = changeNotesFactory.create(projectName, changeId);
+ PatchSet.Id patchSetId = PatchSet.id(changeId, event.getRevision()._number);
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
+
+ // code owner approvals are only computed for the current patch set
+ PatchSet currentPatchSet = changeNotes.getCurrentPatchSet();
+ if (!currentPatchSet.id().equals(patchSetId)) {
+ logger.atFine().log("skip posting the change message on non-current patch set");
+ return;
+ }
+
+ if (event.getOldApprovals().get(requiredApproval.labelType().getName()) == null) {
+ // If oldApprovals doesn't contain the label or if the labels value in it is null, the label
+ // was not changed.
+ // This means that the user only voted on unrelated labels.
+ return;
+ }
+
+ @SuppressWarnings("unused")
+ WorkQueue.Task<?> possiblyIgnoredError =
+ (WorkQueue.Task<?>)
+ workQueue
+ .getDefaultQueue()
+ .submit(
+ () -> {
+ try (ManualRequestContext ignored =
+ oneOffRequestContext.openAs(user.getAccountId())) {
+ postChangeMessage(
+ event.getWhen(),
+ user.asIdentifiedUser(),
+ changeNotes,
+ currentPatchSet,
+ mapApprovalInfosToVotingValues(event.getOldApprovals()),
+ mapApprovalInfosToVotingValues(event.getApprovals()),
+ requiredApproval,
+ maxPathsInChangeMessage);
+ }
+ });
+ }
+
private void postChangeMessage(
Instant when,
IdentifiedUser user,
@@ -398,6 +464,15 @@
return LabelVote.create(labelName, approvals.get(labelName));
}
+ private static ImmutableMap<String, Short> mapApprovalInfosToVotingValues(
+ Map<String, ApprovalInfo> approvals) {
+ return approvals.entrySet().stream()
+ .collect(
+ toImmutableMap(
+ Map.Entry::getKey,
+ e -> e.getValue().value != null ? e.getValue().value.shortValue() : null));
+ }
+
private class Op implements BatchUpdateOp {
private final IdentifiedUser user;
private final ChangeNotes changeNotes;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
index d31cbfb..3875eb9 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -1172,8 +1172,14 @@
String path = "foo/bar.baz";
String changeId = createChange("Test Change", path, "file content").getChangeId();
+ int numberOfChangeMessages = gApi.changes().id(changeId).get().messages.size();
+
recommend(changeId);
+ // expect that 2 changes messages are posted, one for applying the approval and one to inform
+ // about the owned paths
+ int expectedNumberOfChangeMessages = numberOfChangeMessages + 2;
+
assertAsyncChangeMessage(
changeId,
String.format(
@@ -1181,15 +1187,18 @@
+ "By voting Code-Review+1 the following files are now code-owner approved by"
+ " %s:\n"
+ "* %s\n",
- AccountTemplateUtil.getAccountTemplate(admin.id()), path));
+ AccountTemplateUtil.getAccountTemplate(admin.id()), path),
+ expectedNumberOfChangeMessages);
}
- private void assertAsyncChangeMessage(String changeId, String expectedChangeMessage)
+ private void assertAsyncChangeMessage(
+ String changeId, String expectedChangeMessage, int expectedNumberOfChangeMessages)
throws Exception {
assertAsync(
() -> {
Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
assertThat(Iterables.getLast(messages).message).isEqualTo(expectedChangeMessage);
+ assertThat(messages).hasSize(expectedNumberOfChangeMessages);
return null;
});
}
