Handle when uploader email matches more than one account
It's possible for an email to be used by more than one account. When the
uploader email matches more than one account, ChangeEventListener was
throwing an IllegalArgumentException preventing the operation from
completing successfully.
In most of the case, this is not harmful since ChangeEvent listeners
are called at the end of the operation but in the following scenario, it
makes gerrit data inconsistent.
If user push a new patchset to an existing change but bypass review, the
operation will be aborted by the IllegalArgumentException resulting in
the change being merged in the repo but its status still open.
Change-Id: Icc48fe4548fc46169bec3561ca490a199107af3b
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ChangeEventListener.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ChangeEventListener.java
index 2281997..ae0e9a6 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ChangeEventListener.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ChangeEventListener.java
@@ -18,7 +18,6 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import com.google.gerrit.common.EventListener;
import com.google.gerrit.common.errors.NoSuchGroupException;
@@ -30,9 +29,9 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountByEmailCache;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.GroupMembers;
+import com.google.gerrit.server.data.AccountAttribute;
import com.google.gerrit.server.events.DraftPublishedEvent;
import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.events.PatchSetCreatedEvent;
@@ -66,7 +65,6 @@
.getLogger(ChangeEventListener.class);
private final AccountResolver accountResolver;
- private final AccountByEmailCache byEmailCache;
private final Provider<GroupsCollection> groupsCollection;
private final GroupMembers.Factory groupMembersFactory;
private final DefaultReviewers.Factory reviewersFactory;
@@ -84,7 +82,6 @@
@Inject
ChangeEventListener(
final AccountResolver accountResolver,
- final AccountByEmailCache byEmailCache,
final Provider<GroupsCollection> groupsCollection,
final GroupMembers.Factory groupMembersFactory,
final DefaultReviewers.Factory reviewersFactory,
@@ -98,7 +95,6 @@
final Provider<CurrentUser> user,
final ChangeQueryBuilder queryBuilder) {
this.accountResolver = accountResolver;
- this.byEmailCache = byEmailCache;
this.groupsCollection = groupsCollection;
this.groupMembersFactory = groupMembersFactory;
this.reviewersFactory = reviewersFactory;
@@ -119,17 +115,17 @@
PatchSetCreatedEvent e = (PatchSetCreatedEvent) event;
onEvent(new Project.NameKey(e.change.project),
Integer.parseInt(e.change.number),
- Integer.parseInt(e.patchSet.number), e.uploader.email);
+ Integer.parseInt(e.patchSet.number), e.uploader);
} else if (event instanceof DraftPublishedEvent) {
DraftPublishedEvent e = (DraftPublishedEvent) event;
onEvent(new Project.NameKey(e.change.project),
Integer.parseInt(e.change.number),
- Integer.parseInt(e.patchSet.number), e.uploader.email);
+ Integer.parseInt(e.patchSet.number), e.uploader);
}
}
private void onEvent(Project.NameKey projectName, int changeNumber,
- int patchSetNumber, String email) {
+ int patchSetNumber, AccountAttribute uploader) {
// TODO(davido): we have to cache per project configuration
ReviewersConfig config = configFactory.create(projectName);
@@ -162,9 +158,8 @@
return;
}
- final Runnable task =
- reviewersFactory.create(change,
- toAccounts(reviewers, projectName, email));
+ final Runnable task = reviewersFactory.create(change,
+ toAccounts(reviewers, projectName, uploader));
workQueue.getDefaultQueue().submit(new Runnable() {
@Override
@@ -248,7 +243,7 @@
}
private Set<Account> toAccounts(Set<String> in, Project.NameKey p,
- String uploaderEMail) {
+ AccountAttribute uploader) {
Set<Account> reviewers = Sets.newHashSetWithExpectedSize(in.size());
GroupMembers groupMembers = null;
for (String r : in) {
@@ -265,13 +260,30 @@
continue;
}
if (groupMembers == null) {
- groupMembers =
- groupMembersFactory.create(identifiedUserFactory.create(Iterables
- .getOnlyElement(byEmailCache.get(uploaderEMail))));
+ // email is not unique to one account, try to locate the account using
+ // "Full name <email>" to increase chance of finding only one.
+ String uploaderNameEmail = String.format("%s <%s>", uploader.name, uploader.email);
+ try {
+ Account uploaderAccount = accountResolver.findByNameOrEmail(uploaderNameEmail);
+ if (uploaderAccount != null) {
+ groupMembers = groupMembersFactory
+ .create(identifiedUserFactory.create(uploaderAccount.getId()));
+ }
+ } catch (OrmException e) {
+ log.warn(String.format(
+ "Failed to list accounts for group %s, cannot retrieve uploader account %s",
+ r, uploaderNameEmail), e);
+ }
}
try {
- reviewers.addAll(groupMembers.listAccounts(
- groupsCollection.get().parse(r).getGroupUUID(), p));
+ if (groupMembers != null) {
+ reviewers.addAll(groupMembers
+ .listAccounts(groupsCollection.get().parse(r).getGroupUUID(), p));
+ } else {
+ log.warn(String.format(
+ "Failed to list accounts for group %s; cannot retrieve uploader account for %s",
+ r, uploader.email));
+ }
} catch (UnprocessableEntityException | NoSuchGroupException e) {
log.warn(String.format(
"Reviewer %s is neither an account nor a group", r));
