Trigger owners-autoassign processing for change readyForReview
The owners-autoassign plugin has historically ignored
all refs/changes/*/meta ref-updates, because they are related
to modifications on the change meta-data.
There is one moment, though, where the evaluation make a lot
of sense, and it is a meta-data only change, triggering only
the ref-update of the refs/changes/*/meta: a WIP change is
set ready for review.
Very often, the user that is working on a WIP change does not
like to have lots of reviewers and therefore tends to remove
them from the reviewers' list. Also, WIP changes do not generate
any attention-set action because the change isn't supposed to
be ready for review.
When the change is later set ready for review *without any
further updates*, the owners-autoassign must process the change
and, if there aren't already the expected owners added,
should add all the owners retrieved from the processing
of the OWNERS files as reviewers.
The detection of the diff cannot rely though on the new SHA1
of the event but rather get the latest patch-set SHA1 and
compute the diff against its change-base.
Bug: Issue 14679
Change-Id: I6b2b6f9b947615f96e331709aa257d54b9c52f07
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
index 81559f7..6f53d04 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
@@ -18,10 +18,12 @@
import static com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace.IGNORE_NONE;
+import com.google.common.base.Predicates;
import com.google.common.collect.Sets;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.annotations.Listen;
@@ -32,8 +34,10 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListKey;
@@ -46,6 +50,7 @@
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -64,6 +69,8 @@
private Provider<CurrentUser> currentUserProvider;
+ private ChangeNotes.Factory notesFactory;
+
@Inject
public GitRefListener(
GerritApi api,
@@ -72,7 +79,8 @@
Accounts accounts,
ReviewerManager reviewerManager,
OneOffRequestContext oneOffReqCtx,
- Provider<CurrentUser> currentUserProvider) {
+ Provider<CurrentUser> currentUserProvider,
+ ChangeNotes.Factory notesFactory) {
this.api = api;
this.patchListCache = patchListCache;
this.repositoryManager = repositoryManager;
@@ -80,6 +88,7 @@
this.reviewerManager = reviewerManager;
this.oneOffReqCtx = oneOffReqCtx;
this.currentUserProvider = currentUserProvider;
+ this.notesFactory = notesFactory;
}
@Override
@@ -120,11 +129,15 @@
String projectName = event.getProjectName();
Repository repository;
try {
- repository = repositoryManager.openRepository(Project.NameKey.parse(projectName));
+ NameKey projectNameKey = Project.NameKey.parse(projectName);
+ repository = repositoryManager.openRepository(projectNameKey);
try {
String refName = event.getRefName();
- if (refName.startsWith(RefNames.REFS_CHANGES) && !RefNames.isNoteDbMetaRef(refName)) {
- processEvent(repository, event);
+ Change.Id changeId = Change.Id.fromRef(refName);
+ if (changeId != null
+ && (!RefNames.isNoteDbMetaRef(refName)
+ || isChangeSetReadyForReview(projectNameKey, changeId, event.getNewObjectId()))) {
+ processEvent(repository, event, changeId);
}
} finally {
repository.close();
@@ -134,15 +147,25 @@
}
}
- public void processEvent(Repository repository, Event event) {
- Change.Id cId = Change.Id.fromRef(event.getRefName());
+ private boolean isChangeSetReadyForReview(
+ NameKey project, Change.Id changeId, String metaObjectId) {
+ ChangeNotes changeNotes = notesFactory.createChecked(project, changeId);
+ return !changeNotes.getChange().isWorkInProgress()
+ && changeNotes.getChangeMessages().stream()
+ .filter(message -> message.getKey().uuid().equals(metaObjectId))
+ .map(message -> message.getTag())
+ .filter(Predicates.notNull())
+ .anyMatch(tag -> tag.contains(ChangeMessagesUtil.TAG_SET_READY));
+ }
+
+ public void processEvent(Repository repository, Event event, Change.Id cId) {
Changes changes = api.changes();
// The provider injected by Gerrit is shared with other workers on the
// same local thread and thus cannot be closed in this event listener.
try {
ChangeApi cApi = changes.id(cId.get());
ChangeInfo change = cApi.get();
- PatchList patchList = getPatchList(event, change);
+ PatchList patchList = getPatchList(repository, event, change);
if (patchList != null) {
PathOwners owners = new PathOwners(accounts, repository, change.branch, patchList);
Set<Account.Id> allReviewers = Sets.newHashSet();
@@ -160,17 +183,23 @@
}
}
- private PatchList getPatchList(Event event, ChangeInfo change) {
+ private PatchList getPatchList(Repository repository, Event event, ChangeInfo change) {
ObjectId newId = null;
- if (event.getNewObjectId() != null) {
- newId = ObjectId.fromString(event.getNewObjectId());
- }
-
- PatchListKey plKey = PatchListKey.againstCommit(null, newId, IGNORE_NONE);
+ PatchListKey plKey;
try {
+ if (RefNames.isNoteDbMetaRef(event.getRefName())) {
+ newId = ObjectId.fromString(change.currentRevision);
+ RevCommit revCommit = repository.parseCommit(newId);
+ plKey = PatchListKey.againstBase(newId, revCommit.getParentCount());
+ } else {
+ if (event.getNewObjectId() != null) {
+ newId = ObjectId.fromString(event.getNewObjectId());
+ }
+ plKey = PatchListKey.againstCommit(null, newId, IGNORE_NONE);
+ }
return patchListCache.get(plKey, Project.nameKey(change.project));
- } catch (PatchListNotAvailableException e) {
- logger.warn("Could not load patch list: {}", plKey, e);
+ } catch (PatchListNotAvailableException | IOException e) {
+ logger.warn("Could not load patch list for change {}", change.id, e);
}
return null;
}
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
index 5c9f1a7..1e0f8ec 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
@@ -21,11 +21,14 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.registration.Extension;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
+import java.util.stream.StreamSupport;
import org.eclipse.jgit.transport.ReceiveCommand.Type;
import org.junit.Test;
@@ -34,62 +37,97 @@
sysModule = "com.vmware.gerrit.owners.common.GitRefListenerIT$TestModule")
public class GitRefListenerIT extends LightweightPluginDaemonTest {
- @Inject GitRefListenerTest gitRefListener;
+ @Inject DynamicSet<GitReferenceUpdatedListener> allRefUpdateListeners;
@Inject ThreadLocalRequestContext requestContext;
- String aRefChange = RefNames.REFS_CHANGES + "01/01/01";
String anOldObjectId = "anOldRef";
String aNewObjectId = "aNewRef";
public static class TestModule extends AbstractModule {
@Override
protected void configure() {
- bind(GitReferenceUpdatedListener.class).to(GitRefListenerTest.class);
+ DynamicSet.bind(binder(), GitReferenceUpdatedListener.class).to(GitRefListenerTest.class);
}
}
@Test
- public void shouldNotProcessNoteDbOnlyRefs() {
+ public void shouldNotProcessNoteDbOnlyRefs() throws Exception {
+ String changeRefPrefix = createChange().getChange().getId().toRefPrefix();
+ int baselineProcessedEvents = gitRefListener().getProcessedEvents();
+
ReferenceUpdatedEventTest refUpdatedEvent =
new ReferenceUpdatedEventTest(
project,
- RefNames.REFS_CHANGES + "01/01" + RefNames.META_SUFFIX,
+ changeRefPrefix + RefNames.META_SUFFIX.substring(1),
anOldObjectId,
aNewObjectId,
Type.CREATE,
admin.id());
- gitRefListener.onGitReferenceUpdated(refUpdatedEvent);
- assertEquals(0, gitRefListener.getProcessedEvents());
+ gitRefListener().onGitReferenceUpdated(refUpdatedEvent);
+ assertEquals(baselineProcessedEvents, gitRefListener().getProcessedEvents());
}
@Test
- public void shouldProcessRefChanges() {
- gitRefListener.onGitReferenceUpdated(newRefUpdateEvent());
- assertEquals(1, gitRefListener.getProcessedEvents());
+ public void shoulProcessSetReadyForReviewOnNoteDb() throws Exception {
+ int wipChangeNum = createChange().getChange().getId().get();
+ gApi.changes().id(wipChangeNum).setWorkInProgress();
+
+ int baselineProcessedEvents = gitRefListener().getProcessedEvents();
+
+ gApi.changes().id(wipChangeNum).setReadyForReview();
+ assertEquals(1, gitRefListener().getProcessedEvents() - baselineProcessedEvents);
+ }
+
+ @Test
+ public void shoulNotProcessEmptyMetaRefUpdatesAfterSetReadyForReviewOnNoteDb() throws Exception {
+ int wipChangeNum = createChange().getChange().getId().get();
+ gApi.changes().id(wipChangeNum).setWorkInProgress();
+ gApi.changes().id(wipChangeNum).setReadyForReview();
+ int baselineProcessedEvents = gitRefListener().getProcessedEvents();
+
+ gApi.changes().id(wipChangeNum).addReviewer("user");
+ assertEquals(baselineProcessedEvents, gitRefListener().getProcessedEvents());
+ }
+
+ @Test
+ public void shouldProcessRefChanges() throws Exception {
+ gitRefListener().onGitReferenceUpdated(newRefUpdateEvent());
+ assertEquals(1, gitRefListener().getProcessedEvents());
}
@Test
public void shouldRetrieveChangeFromAnonymousContext() throws Exception {
try (ManualRequestContext ctx = new ManualRequestContext(new AnonymousUser(), requestContext)) {
- gitRefListener.onGitReferenceUpdated(newRefUpdateEvent());
- assertEquals(1, gitRefListener.getProcessedEvents());
+ gitRefListener().onGitReferenceUpdated(newRefUpdateEvent());
+ assertEquals(1, gitRefListener().getProcessedEvents());
}
}
@Test
public void shouldRetrieveChangeFromAnonymousContextWithoutAccountId() throws Exception {
+ String refPrefix = createChange().getChange().getId().toRefPrefix();
ReferenceUpdatedEventTest refUpdateWithoutAccountId =
new ReferenceUpdatedEventTest(
- project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE, null);
+ project, refPrefix, anOldObjectId, aNewObjectId, Type.CREATE, null);
try (ManualRequestContext ctx = new ManualRequestContext(new AnonymousUser(), requestContext)) {
- gitRefListener.onGitReferenceUpdated(refUpdateWithoutAccountId);
- assertEquals(1, gitRefListener.getProcessedEvents());
+ gitRefListener().onGitReferenceUpdated(refUpdateWithoutAccountId);
+ assertEquals(1, gitRefListener().getProcessedEvents());
}
}
- private ReferenceUpdatedEventTest newRefUpdateEvent() {
+ private GitRefListenerTest gitRefListener() {
+ return (GitRefListenerTest)
+ StreamSupport.stream(allRefUpdateListeners.entries().spliterator(), false)
+ .map(Extension::get)
+ .filter(listener -> GitRefListenerTest.class.isAssignableFrom(listener.getClass()))
+ .findFirst()
+ .get();
+ }
+
+ private ReferenceUpdatedEventTest newRefUpdateEvent() throws Exception {
+ String refPrefix = createChange().getChange().getId().toRefPrefix();
return new ReferenceUpdatedEventTest(
- project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE, admin.id());
+ project, refPrefix, anOldObjectId, aNewObjectId, Type.CREATE, admin.id());
}
}
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
index e3d4be4..75a9189 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
@@ -15,13 +15,17 @@
package com.vmware.gerrit.owners.common;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener.Event;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import com.google.inject.Singleton;
import com.googlesource.gerrit.owners.common.Accounts;
import com.googlesource.gerrit.owners.common.GitRefListener;
import com.googlesource.gerrit.owners.common.ReviewerManager;
@@ -29,6 +33,7 @@
import org.junit.Ignore;
@Ignore
+@Singleton
public class GitRefListenerTest extends GitRefListener {
int processedEvents = 0;
@@ -40,7 +45,8 @@
Accounts accounts,
ReviewerManager reviewerManager,
OneOffRequestContext oneOffReqCtx,
- Provider<CurrentUser> currentUserProvider) {
+ Provider<CurrentUser> currentUserProvider,
+ ChangeNotes.Factory notesFactory) {
super(
api,
patchListCache,
@@ -48,11 +54,12 @@
accounts,
reviewerManager,
oneOffReqCtx,
- currentUserProvider);
+ currentUserProvider,
+ notesFactory);
}
@Override
- public void processEvent(Repository repository, Event event) {
+ public void processEvent(Repository repository, Event event, Change.Id cId) {
processedEvents++;
}
