Merge branch 'stable-2.15' into stable-2.16
* stable-2.15:
Removed redundant PLUGIN_DEPS from BUILD
Format with GJF 1.7
Add integration test for owners-autoassign
Prevent infinite event processing with NoteDb
Look for gerrit:/username: schemes for username
Get rid of references to ReviewDb
Change-Id: Ia7769a54f6ecae8ce90ecd9f4cd9d231413f8e33
diff --git a/owners-autoassign/BUILD b/owners-autoassign/BUILD
index 13aabbe..463afcb 100644
--- a/owners-autoassign/BUILD
+++ b/owners-autoassign/BUILD
@@ -1,4 +1,5 @@
-load("//tools/bzl:plugin.bzl", "gerrit_plugin")
+load("//tools/bzl:junit.bzl", "junit_tests")
+load("//tools/bzl:plugin.bzl", "PLUGIN_DEPS", "PLUGIN_DEPS_NEVERLINK", "PLUGIN_TEST_DEPS", "gerrit_plugin")
gerrit_plugin(
name = "owners-autoassign",
@@ -18,3 +19,24 @@
"//owners-common",
],
)
+
+java_library(
+ name = "owners-autoassign_deps",
+ srcs = glob([
+ "src/main/java/**/*.java",
+ ]),
+ visibility = ["//visibility:public"],
+ deps = PLUGIN_DEPS_NEVERLINK + [
+ "//owners-common",
+ ],
+)
+
+junit_tests(
+ name = "owners_autoassign_tests",
+ testonly = 1,
+ srcs = glob(["src/test/java/**/*.java"]),
+ deps = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [
+ "//owners-common",
+ ":owners-autoassign_deps",
+ ],
+)
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 e123534..3eb5754 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
@@ -20,19 +20,22 @@
import com.google.common.collect.Sets;
import com.google.gerrit.extensions.annotations.Listen;
+import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.Changes;
+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.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
-import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import java.io.IOException;
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
@@ -44,9 +47,7 @@
public class GitRefListener implements GitReferenceUpdatedListener {
private static final Logger logger = LoggerFactory.getLogger(GitRefListener.class);
- private static final String CHANGES_REF = "refs/changes/";
-
- private final Provider<ReviewDb> db;
+ private final GerritApi api;
private final PatchListCache patchListCache;
@@ -58,12 +59,12 @@
@Inject
public GitRefListener(
- Provider<ReviewDb> db,
+ GerritApi api,
PatchListCache patchListCache,
GitRepositoryManager repositoryManager,
Accounts accounts,
ReviewerManager reviewerManager) {
- this.db = db;
+ this.api = api;
this.patchListCache = patchListCache;
this.repositoryManager = repositoryManager;
this.accounts = accounts;
@@ -77,7 +78,10 @@
try {
repository = repositoryManager.openRepository(Project.NameKey.parse(projectName));
try {
- processEvent(repository, event);
+ String refName = event.getRefName();
+ if (refName.startsWith(RefNames.REFS_CHANGES) && !RefNames.isNoteDbMetaRef(refName)) {
+ processEvent(repository, event);
+ }
} finally {
repository.close();
}
@@ -86,38 +90,36 @@
}
}
- private void processEvent(Repository repository, Event event) {
- if (event.getRefName().startsWith(CHANGES_REF)) {
- Change.Id id = Change.Id.fromRef(event.getRefName());
- ReviewDb reviewDb = db.get();
- // 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 {
- Change change = reviewDb.changes().get(id);
- if (change == null) {
- return;
- }
- PatchList patchList = getPatchList(event, change);
- if (patchList != null) {
- PathOwners owners =
- new PathOwners(accounts, repository, change.getDest().get(), patchList);
- Set<Account.Id> allReviewers = Sets.newHashSet();
- allReviewers.addAll(owners.get().values());
- for (Matcher matcher : owners.getMatchers().values()) {
- allReviewers.addAll(matcher.getOwners());
- }
- logger.debug("Autoassigned reviewers are: {}", allReviewers.toString());
- reviewerManager.addReviewers(change, allReviewers);
- }
- } catch (OrmException e) {
- logger.warn("Could not open change: {}", id, e);
- } catch (ReviewerManagerException e) {
- logger.warn("Could not add reviewers for change: {}", id, e);
+ public void processEvent(Repository repository, Event event) {
+ Change.Id cId = Change.Id.fromRef(event.getRefName());
+ 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.id);
+ ChangeInfo change = cApi.get();
+ if (change == null) {
+ return;
}
+ PatchList patchList = getPatchList(event, change);
+ if (patchList != null) {
+ PathOwners owners = new PathOwners(accounts, repository, change.branch, patchList);
+ Set<Account.Id> allReviewers = Sets.newHashSet();
+ allReviewers.addAll(owners.get().values());
+ for (Matcher matcher : owners.getMatchers().values()) {
+ allReviewers.addAll(matcher.getOwners());
+ }
+ logger.debug("Autoassigned reviewers are: {}", allReviewers.toString());
+ reviewerManager.addReviewers(cApi, allReviewers);
+ }
+ } catch (RestApiException e) {
+ logger.warn("Could not open change: {}", cId, e);
+ } catch (ReviewerManagerException e) {
+ logger.warn("Could not add reviewers for change: {}", cId, e);
}
}
- private PatchList getPatchList(Event event, Change change) {
+ private PatchList getPatchList(Event event, ChangeInfo change) {
ObjectId newId = null;
if (event.getNewObjectId() != null) {
newId = ObjectId.fromString(event.getNewObjectId());
@@ -125,7 +127,7 @@
PatchListKey plKey = PatchListKey.againstCommit(null, newId, IGNORE_NONE);
try {
- return patchListCache.get(plKey, change.getProject());
+ return patchListCache.get(plKey, new Project.NameKey(change.project));
} catch (PatchListNotAvailableException e) {
logger.warn("Could not load patch list: {}", plKey, e);
}
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
index f62b8c7..efb9910 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
@@ -18,10 +18,11 @@
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
+import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gwtorm.server.OrmException;
@@ -36,28 +37,32 @@
public class ReviewerManager {
private static final Logger log = LoggerFactory.getLogger(ReviewerManager.class);
- private final GerritApi gApi;
private final OneOffRequestContext requestContext;
+ private final GerritApi gApi;
@Inject
- public ReviewerManager(GerritApi gApi, OneOffRequestContext requestContext) {
- this.gApi = gApi;
+ public ReviewerManager(OneOffRequestContext requestContext, GerritApi gApi) {
this.requestContext = requestContext;
+ this.gApi = gApi;
}
- public void addReviewers(Change change, Collection<Account.Id> reviewers)
+ public void addReviewers(ChangeApi cApi, Collection<Account.Id> reviewers)
throws ReviewerManagerException {
- try (ManualRequestContext ctx = requestContext.openAs(change.getOwner())) {
- // TODO(davido): Switch back to using changes API again,
- // when it supports batch mode for adding reviewers
- ReviewInput in = new ReviewInput();
- in.reviewers = new ArrayList<>(reviewers.size());
- for (Account.Id account : reviewers) {
- AddReviewerInput addReviewerInput = new AddReviewerInput();
- addReviewerInput.reviewer = account.toString();
- in.reviewers.add(addReviewerInput);
+ try {
+ ChangeInfo changeInfo = cApi.get();
+ try (ManualRequestContext ctx =
+ requestContext.openAs(new Account.Id(changeInfo.owner._accountId))) {
+ // TODO(davido): Switch back to using changes API again,
+ // when it supports batch mode for adding reviewers
+ ReviewInput in = new ReviewInput();
+ in.reviewers = new ArrayList<>(reviewers.size());
+ for (Account.Id account : reviewers) {
+ AddReviewerInput addReviewerInput = new AddReviewerInput();
+ addReviewerInput.reviewer = account.toString();
+ in.reviewers.add(addReviewerInput);
+ }
+ gApi.changes().id(changeInfo.id).current().review(in);
}
- gApi.changes().id(change.getId().get()).current().review(in);
} catch (RestApiException | OrmException e) {
log.error("Couldn't add reviewers to the change", e);
throw new ReviewerManagerException(e);
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
new file mode 100644
index 0000000..aa6db4f
--- /dev/null
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
@@ -0,0 +1,75 @@
+// 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.vmware.gerrit.owners.common;
+
+import static org.junit.Assert.assertEquals;
+
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.inject.AbstractModule;
+import org.eclipse.jgit.transport.ReceiveCommand.Type;
+import org.junit.Test;
+
+@TestPlugin(
+ name = "owners-autoassign",
+ sysModule = "com.vmware.gerrit.owners.common.GitRefListenerIT$TestModule")
+public class GitRefListenerIT extends LightweightPluginDaemonTest {
+
+ public static class TestModule extends AbstractModule {
+ @Override
+ protected void configure() {
+ bind(GitReferenceUpdatedListener.class).to(GitRefListenerTest.class);
+ }
+ }
+
+ @Test
+ public void shouldNotProcessNoteDbOnlyRefs() {
+ GitRefListenerTest gitRefListener = getPluginInstance(GitRefListenerTest.class);
+
+ String aRefChange = RefNames.REFS_CHANGES + "01/01" + RefNames.META_SUFFIX;
+ String anOldObjectId = "anOldRef";
+ String aNewObjectId = "aNewRef";
+
+ ReferenceUpdatedEventTest refUpdatedEvent =
+ new ReferenceUpdatedEventTest(
+ project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE);
+
+ gitRefListener.onGitReferenceUpdated(refUpdatedEvent);
+ assertEquals(0, gitRefListener.getProcessedEvents());
+ }
+
+ @Test
+ public void shouldProcessRefChanges() {
+ GitRefListenerTest gitRefListener = getPluginInstance(GitRefListenerTest.class);
+
+ String aRefChange = RefNames.REFS_CHANGES + "01/01/01";
+ String anOldObjectId = "anOldRef";
+ String aNewObjectId = "aNewRef";
+
+ ReferenceUpdatedEventTest refUpdatedEvent =
+ new ReferenceUpdatedEventTest(
+ project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE);
+
+ gitRefListener.onGitReferenceUpdated(refUpdatedEvent);
+ assertEquals(1, gitRefListener.getProcessedEvents());
+ }
+
+ private <T> T getPluginInstance(Class<T> clazz) {
+ return plugin.getSysInjector().getInstance(clazz);
+ }
+}
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
new file mode 100644
index 0000000..dbeedd4
--- /dev/null
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
@@ -0,0 +1,35 @@
+package com.vmware.gerrit.owners.common;
+
+import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.patch.PatchListCache;
+import com.google.inject.Inject;
+import com.googlesource.gerrit.owners.common.Accounts;
+import com.googlesource.gerrit.owners.common.GitRefListener;
+import com.googlesource.gerrit.owners.common.ReviewerManager;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Ignore;
+
+@Ignore
+public class GitRefListenerTest extends GitRefListener {
+ int processedEvents = 0;
+
+ @Inject
+ public GitRefListenerTest(
+ GerritApi api,
+ PatchListCache patchListCache,
+ GitRepositoryManager repositoryManager,
+ Accounts accounts,
+ ReviewerManager reviewerManager) {
+ super(api, patchListCache, repositoryManager, accounts, reviewerManager);
+ }
+
+ @Override
+ public void processEvent(Repository repository, Event event) {
+ processedEvents++;
+ }
+
+ int getProcessedEvents() {
+ return processedEvents;
+ }
+}
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java
new file mode 100644
index 0000000..601ec8d
--- /dev/null
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/ReferenceUpdatedEventTest.java
@@ -0,0 +1,76 @@
+package com.vmware.gerrit.owners.common;
+
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
+import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.reviewdb.client.Project;
+import org.eclipse.jgit.transport.ReceiveCommand;
+import org.junit.Ignore;
+
+@Ignore
+public class ReferenceUpdatedEventTest implements GitReferenceUpdatedListener.Event {
+
+ private final String projectName;
+ private final String ref;
+ private final String oldObjectId;
+ private final String newObjectId;
+ private final ReceiveCommand.Type type;
+
+ public ReferenceUpdatedEventTest(
+ Project.NameKey project,
+ String ref,
+ String oldObjectId,
+ String newObjectId,
+ ReceiveCommand.Type type) {
+ this.projectName = project.get();
+ this.ref = ref;
+ this.oldObjectId = oldObjectId;
+ this.newObjectId = newObjectId;
+ this.type = type;
+ }
+
+ @Override
+ public String getProjectName() {
+ return projectName;
+ }
+
+ @Override
+ public String getRefName() {
+ return ref;
+ }
+
+ @Override
+ public String getOldObjectId() {
+ return oldObjectId;
+ }
+
+ @Override
+ public String getNewObjectId() {
+ return newObjectId;
+ }
+
+ @Override
+ public boolean isCreate() {
+ return type == ReceiveCommand.Type.CREATE;
+ }
+
+ @Override
+ public boolean isDelete() {
+ return type == ReceiveCommand.Type.DELETE;
+ }
+
+ @Override
+ public boolean isNonFastForward() {
+ return type == ReceiveCommand.Type.UPDATE_NONFASTFORWARD;
+ }
+
+ @Override
+ public AccountInfo getUpdater() {
+ return null;
+ }
+
+ @Override
+ public NotifyHandling getNotify() {
+ return NotifyHandling.NONE;
+ }
+}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/AccountsImpl.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/AccountsImpl.java
index 8a12871..106a91a 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/AccountsImpl.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/AccountsImpl.java
@@ -16,10 +16,13 @@
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GERRIT;
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_MAILTO;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME;
+import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Account.Id;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountState;
@@ -48,6 +51,7 @@
private final AccountCache byId;
private final GroupCache groupCache;
private final GroupMembers groupMembers;
+ private final IdentifiedUser adminUser;
private final OneOffRequestContext oneOffRequestContext;
@Inject
@@ -56,11 +60,13 @@
AccountCache byId,
GroupCache groupCache,
GroupMembers groupMembers,
- OneOffRequestContext oneOffRequestContext) {
+ OneOffRequestContext oneOffRequestContext,
+ IdentifiedUser.GenericFactory userFactory) {
this.resolver = resolver;
this.byId = byId;
this.groupCache = groupCache;
this.groupMembers = groupMembers;
+ this.adminUser = userFactory.create(new Account.Id(1000000));
this.oneOffRequestContext = oneOffRequestContext;
}
@@ -84,11 +90,12 @@
return Collections.emptySet();
}
- try {
+ try (ManualRequestContext ctx = oneOffRequestContext.openAs(adminUser.getAccountId())) {
+
return groupMembers.listAccounts(group.get().getGroupUUID(), null).stream()
.map(Account::getId)
.collect(Collectors.toSet());
- } catch (NoSuchProjectException | IOException e) {
+ } catch (NoSuchProjectException | OrmException | IOException e) {
log.error("Unable to list accounts in group " + group, e);
return Collections.emptySet();
}
@@ -153,7 +160,9 @@
}
private boolean isUsernameMatch(ExternalId externalId, String username) {
- return keySchemeRest(SCHEME_GERRIT, externalId.key())
+ return OptionalUtils.combine(
+ keySchemeRest(SCHEME_GERRIT, externalId.key()),
+ keySchemeRest(SCHEME_USERNAME, externalId.key()))
.filter(name -> name.equals(username))
.isPresent();
}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
index e6e1e91..a5df462 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
@@ -20,7 +20,6 @@
import com.google.common.base.Charsets;
import com.google.gerrit.reviewdb.client.Patch;
-import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListEntry;
import java.io.IOException;
@@ -34,7 +33,6 @@
@Ignore
public abstract class Config {
- protected ReviewDb db;
protected Repository repository;
protected PatchList patchList;
protected ConfigurationParser parser;
