Use preferred email addresses.
OWNERS file could contain any owner email address,
which should be converted to a registered account's
preferred email address and compared with
code reviewer email addresses.
Add DebugMessages.errors to report bad owner email addresses
and other server side errors.
Change-Id: I88c6aef15295ea9bdbf5301834afe688a6306289
diff --git a/BUILD b/BUILD
index a3006a8..eb304bb 100644
--- a/BUILD
+++ b/BUILD
@@ -1,6 +1,6 @@
load("//lib/prolog:prolog.bzl", "prolog_cafe_library")
load("//tools/bzl:junit.bzl", "junit_tests")
-load("//tools/bzl:plugin.bzl", "gerrit_plugin", "PLUGIN_DEPS")
+load("//tools/bzl:plugin.bzl", "gerrit_plugin", "PLUGIN_DEPS", "PLUGIN_TEST_DEPS")
java_library(
name = 'find-owners-lib',
@@ -39,9 +39,8 @@
srcs = glob(['src/test/java/**/*.java']),
# resources = glob(['src/test/resources/**/*']),
tags = ['findowners'],
- deps = PLUGIN_DEPS + [
+ deps = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [
':find-owners-lib',
':find-owners-prolog-rules',
- '//gerrit-acceptance-framework:lib',
],
)
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
index 13a1cc9..0eba65b 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -27,6 +27,7 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.config.PluginConfigFactory;
@@ -54,6 +55,7 @@
private static final Logger log = LoggerFactory.getLogger(Action.class);
private AccountCache accountCache;
+ private Accounts accounts;
private ChangeData.Factory changeDataFactory;
private GitRepositoryManager repoManager;
private Provider<CurrentUser> userProvider;
@@ -72,11 +74,13 @@
SchemaFactory<ReviewDb> reviewDbProvider,
ChangeData.Factory changeDataFactory,
AccountCache accountCache,
+ Accounts accounts,
GitRepositoryManager repoManager) {
this.userProvider = userProvider;
this.reviewDbProvider = reviewDbProvider;
this.changeDataFactory = changeDataFactory;
this.accountCache = accountCache;
+ this.accounts = accounts;
this.repoManager = repoManager;
Config.setVariables(pluginName, configFactory);
Cache.getInstance(); // Create a single Cache.
@@ -167,7 +171,7 @@
Repository repository, Parameters params, ChangeData changeData)
throws OrmException, BadRequestException {
int patchset = getValidPatchsetNum(changeData, params.patchset);
- OwnersDb db = Cache.getInstance().get(repository, changeData, patchset);
+ OwnersDb db = Cache.getInstance().get(accountCache, accounts, repository, changeData, patchset);
Collection<String> changedFiles = changeData.currentFilePaths();
Map<String, Set<String>> file2Owners = db.findOwners(changedFiles);
@@ -180,6 +184,7 @@
obj.dbgmsgs.user = getUserName();
obj.dbgmsgs.project = changeData.change().getProject().get();
obj.dbgmsgs.branch = changeData.change().getDest().get();
+ obj.dbgmsgs.errors = db.errors;
obj.dbgmsgs.path2owners = Util.makeSortedMap(db.path2Owners);
obj.dbgmsgs.owner2paths = Util.makeSortedMap(db.owner2Paths);
}
@@ -211,7 +216,7 @@
if (needFindOwners && !Config.getAlwaysShowButton()) {
needFindOwners = false; // Show button only if some owner is found.
try (Repository repo = repoManager.openRepository(change.getProject())) {
- OwnersDb db = Cache.getInstance().get(repo, changeData);
+ OwnersDb db = Cache.getInstance().get(accountCache, accounts, repo, changeData);
log.trace("getDescription db key = " + db.key);
needFindOwners = db.getNumOwners() > 0;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
index 48f86e6..10fdebf 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
@@ -18,6 +18,8 @@
import com.google.common.cache.CacheBuilder;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import java.util.Collection;
@@ -85,21 +87,32 @@
}
/** Returns a cached or new OwnersDb, for the current patchset. */
- OwnersDb get(Repository repo, ChangeData changeData) throws OrmException {
- return get(repo, changeData, changeData.currentPatchSet().getId().get());
+ OwnersDb get(AccountCache accountCache, Accounts accounts, Repository repo, ChangeData changeData)
+ throws OrmException {
+ return get(
+ accountCache, accounts, repo, changeData, changeData.currentPatchSet().getId().get());
}
/** Returns a cached or new OwnersDb, for the specified patchset. */
- OwnersDb get(Repository repository, ChangeData changeData, int patchset) throws OrmException {
+ OwnersDb get(
+ AccountCache accountCache,
+ Accounts accounts,
+ Repository repository,
+ ChangeData changeData,
+ int patchset)
+ throws OrmException {
Project.NameKey project = changeData.change().getProject();
String branch = changeData.change().getDest().get();
String dbKey = Cache.makeKey(changeData.getId().get(), patchset, branch);
// TODO: get changed files of the given patchset?
- return get(dbKey, repository, project, branch, changeData.currentFilePaths());
+ return get(
+ accountCache, accounts, dbKey, repository, project, branch, changeData.currentFilePaths());
}
/** Returns a cached or new OwnersDb, for the specified branch and changed files. */
OwnersDb get(
+ AccountCache accountCache,
+ Accounts accounts,
String key,
Repository repository,
Project.NameKey project,
@@ -107,7 +120,7 @@
Collection<String> files) {
if (dbCache == null) { // Do not cache OwnersDb
log.trace("Create new OwnersDb, key=" + key);
- return new OwnersDb(key, repository, project, branch, files);
+ return new OwnersDb(accountCache, accounts, key, repository, project, branch, files);
}
try {
log.trace("Get from cash " + dbCache + ", key=" + key + ", cache size=" + dbCache.size());
@@ -117,12 +130,12 @@
@Override
public OwnersDb call() {
log.trace("Create new OwnersDb, key=" + key);
- return new OwnersDb(key, repository, project, branch, files);
+ return new OwnersDb(accountCache, accounts, key, repository, project, branch, files);
}
});
} catch (ExecutionException e) {
log.error("Cache.get has exception: " + e);
- return new OwnersDb(key, repository, project, branch, files);
+ return new OwnersDb(accountCache, accounts, key, repository, project, branch, files);
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
index 57e725c..ff75081 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -18,6 +18,7 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.rules.StoredValues;
import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.googlecode.prolog_cafe.lang.Prolog;
@@ -98,9 +99,10 @@
public static int findApproval(Prolog engine, int minVoteLevel) {
try {
AccountCache accountCache = StoredValues.ACCOUNT_CACHE.get(engine);
+ Accounts accounts = StoredValues.ACCOUNTS.get(engine);
ChangeData changeData = StoredValues.CHANGE_DATA.get(engine);
Repository repository = StoredValues.REPOSITORY.get(engine);
- return new Checker(repository, changeData, minVoteLevel).findApproval(accountCache);
+ return new Checker(repository, changeData, minVoteLevel).findApproval(accountCache, accounts);
} catch (OrmException e) {
log.error("Exception", e);
return 0; // owner approval may or may not be required.
@@ -123,13 +125,13 @@
return (status == Status.ABANDONED || status == Status.MERGED);
}
- int findApproval(AccountCache accountCache) throws OrmException {
+ int findApproval(AccountCache accountCache, Accounts accounts) throws OrmException {
if (isExemptFromOwnerApproval(changeData)) {
return 0;
}
// One update to a Gerrit change can call submit_rule or submit_filter
// many times. So this function should use cached values.
- OwnersDb db = Cache.getInstance().get(repository, changeData);
+ OwnersDb db = Cache.getInstance().get(accountCache, accounts, repository, changeData);
if (db.getNumOwners() <= 0) {
return 0;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
index d4e45ef..e918be7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
@@ -21,6 +21,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -56,6 +57,7 @@
SchemaFactory<ReviewDb> reviewDbProvider,
ChangeData.Factory dataFactory,
AccountCache accountCache,
+ Accounts accounts,
GitRepositoryManager repoManager) {
this.action =
new Action(
@@ -65,6 +67,7 @@
reviewDbProvider,
dataFactory,
accountCache,
+ accounts,
repoManager);
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
index b2ce1f9..4f0f6f6 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -16,7 +16,11 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.collect.Multimap;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Accounts;
import java.io.IOException;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
@@ -26,6 +30,7 @@
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Set;
import org.eclipse.jgit.lib.ObjectReader;
@@ -40,6 +45,8 @@
class OwnersDb {
private static final Logger log = LoggerFactory.getLogger(OwnersDb.class);
+ private AccountCache accountCache;
+ private Accounts accounts;
private int numOwners = -1; // # of owners of all given files.
String key = ""; // key to find this OwnersDb in a cache.
@@ -49,16 +56,23 @@
Map<String, Set<String>> path2Owners = new HashMap<>(); // dir or file glob to owner emails
Set<String> readDirs = new HashSet<>(); // directories in which we have checked OWNERS
Set<String> stopLooking = new HashSet<>(); // directories where OWNERS has "set noparent"
+ Map<String, String> preferredEmails = new HashMap<>(); // owner email to preferred email
+ List<String> errors = new ArrayList<>(); // error messages
OwnersDb() {}
OwnersDb(
+ AccountCache accountCache,
+ Accounts accounts,
String key,
Repository repository,
Project.NameKey project,
String branch,
Collection<String> files) {
+ this.accountCache = accountCache;
+ this.accounts = accounts;
this.key = key;
+ preferredEmails.put("*", "*");
String ownersFileName = Config.getOwnersFileName(project);
for (String fileName : files) {
// Find OWNERS in fileName's directory and parent directories.
@@ -110,14 +124,50 @@
}
}
+ void addPreferredEmails(Set<String> ownerEmails) {
+ List<String> owners = new ArrayList<String>(ownerEmails);
+ owners.removeIf(o -> preferredEmails.get(o) != null);
+ if (owners.size() > 0) {
+ String[] emails = new String[owners.size()];
+ owners.toArray(emails);
+ Multimap<String, Account.Id> email2ids = null;
+ try {
+ email2ids = accounts.byEmails(emails);
+ } catch (Exception e) {
+ log.error("accounts.byEmails failed with exception: ", e);
+ }
+ for (String owner : emails) {
+ String email = owner;
+ try {
+ if (email2ids == null) {
+ errors.add(owner);
+ } else {
+ Collection<Account.Id> ids = email2ids.get(owner);
+ if (ids == null || ids.size() != 1) {
+ errors.add(owner);
+ } else {
+ email = accountCache.get(ids.iterator().next()).getAccount().getPreferredEmail();
+ }
+ }
+ } catch (Exception e) {
+ log.error("Fail to find preferred email of " + owner, e);
+ errors.add(owner);
+ }
+ preferredEmails.put(owner, email);
+ }
+ }
+ }
+
void addFile(String dirPath, String filePath, String[] lines) {
Parser.Result result = Parser.parseFile(dirPath, filePath, lines);
if (result.stopLooking) {
stopLooking.add(dirPath);
}
+ addPreferredEmails(result.owner2paths.keySet());
for (String owner : result.owner2paths.keySet()) {
+ String email = preferredEmails.get(owner);
for (String path : result.owner2paths.get(owner)) {
- addOwnerPathPair(owner, path);
+ addOwnerPathPair(email, path);
}
}
if (Config.getReportSyntaxError()) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
index f91ecbe..296f408 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
@@ -54,6 +54,7 @@
String user;
String project;
String branch;
+ List<String> errors;
SortedMap<String, List<String>> path2owners;
SortedMap<String, List<String>> owner2paths;
};
diff --git a/src/main/resources/Documentation/rest-api.md b/src/main/resources/Documentation/rest-api.md
index 7676059..10b1565 100644
--- a/src/main/resources/Documentation/rest-api.md
+++ b/src/main/resources/Documentation/rest-api.md
@@ -54,6 +54,8 @@
* **branch**: the change's destination branch name.
+ * **errors**: error messages from OWNERS files.
+
* **path2owners**:
a map from directory path or file glob to an array of owner emails.
Note that `*` is a special owner email address.
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
index 5e73d66..571c0c5 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -18,10 +18,12 @@
import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.collect.Multimap;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.extensions.api.accounts.EmailInput;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -34,6 +36,7 @@
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.inject.Inject;
+import java.util.Collection;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevTree;
@@ -45,8 +48,8 @@
@TestPlugin(name = "find-owners", sysModule = "com.googlesource.gerrit.plugins.findowners.Module")
public class FindOwnersIT extends LightweightPluginDaemonTest {
- @Inject private PluginConfigFactory configFactory;
@Inject private Accounts accounts;
+ @Inject private PluginConfigFactory configFactory;
@Test
public void getOwnersTest() throws Exception {
@@ -172,13 +175,46 @@
@Test
public void accountTest() throws Exception {
- String[] myTestEmails = {"abc@g.com", "abc+xyz@g.com", "xyz@g.com"};
- for (String email : myTestEmails) {
- Account.Id id = accountCreator.create("User" + email, email, "FullName" + email).getId();
- // Action.getReviewers uses accountCache to get email address.
- assertThat(accountCache.get(id).getAccount().getPreferredEmail()).isEqualTo(email);
- // Checker.getVotes uses AccountCache to get email address.
- assertThat(accounts.get(db, id).getPreferredEmail()).isEqualTo(email);
+ String[] users = {"user1", "user2", "user3"};
+ String[] emails1 = {"abc@g.com", "abc+xyz@g.com", "xyz-team+review@g.com"};
+ String[] emails2 = {"abc@goog.com", "abc+xyz2@g.com", "xyz-team@goog.com"};
+ // Create accounts with given user name, first and second email addresses.
+ for (int i = 0; i < users.length; i++) {
+ Account.Id id = accountCreator.create(users[i], emails1[i], "FullName " + users[i]).getId();
+ EmailInput input = new EmailInput();
+ input.email = emails2[i];
+ input.noConfirmation = true;
+ gApi.accounts().id(users[i]).addEmail(input);
+ }
+ // Find accounts with given first and second email addresses.
+ // OwnersDb uses either accounts.byEmail or byEmails to get preferred email addresses.
+ Multimap<String, Account.Id> map1 = accounts.byEmails(emails1);
+ Multimap<String, Account.Id> map2 = accounts.byEmails(emails2);
+ for (int i = 0; i < users.length; i++) {
+ Collection<Account.Id> ids1 = accounts.byEmail(emails1[i]);
+ Collection<Account.Id> ids2 = accounts.byEmail(emails2[i]);
+ Collection<Account.Id> ids3 = map1.get(emails1[i]);
+ Collection<Account.Id> ids4 = map2.get(emails2[i]);
+ assertThat(ids1.size()).isEqualTo(1);
+ assertThat(ids2.size()).isEqualTo(1);
+ assertThat(ids3.size()).isEqualTo(1);
+ assertThat(ids4.size()).isEqualTo(1);
+ Account.Id id1 = ids1.iterator().next();
+ Account.Id id2 = ids2.iterator().next();
+ Account.Id id3 = ids3.iterator().next();
+ Account.Id id4 = ids4.iterator().next();
+ assertThat(id1).isEqualTo(id2); // Both emails should find the same account.
+ assertThat(id1).isEqualTo(id3);
+ assertThat(id1).isEqualTo(id4);
+ // Action.getReviewers and Checker.getVotes use accountCache to get email address.
+ assertThat(accountCache.get(id1).getAccount().getPreferredEmail()).isEqualTo(emails1[i]);
+ }
+ // Wrong or non-existing email address.
+ String[] wrongEmails = {"nobody", "@g.com", "nobody@g.com", "*"};
+ Multimap<String, Account.Id> email2ids = accounts.byEmails(wrongEmails);
+ for (String email : wrongEmails) {
+ assertThat(accounts.byEmail(email).size()).isEqualTo(0);
+ assertThat(email2ids.get(email).size()).isEqualTo(0);
}
}
@@ -275,7 +311,15 @@
ChangeResource cr = parseChangeResource(changeInfo.changeId);
Action.Parameters param = new Action.Parameters();
Action action =
- new Action("find-owners", null, null, null, changeDataFactory, accountCache, repoManager);
+ new Action(
+ "find-owners",
+ null,
+ null,
+ null,
+ changeDataFactory,
+ accountCache,
+ accounts,
+ repoManager);
Response<RestResult> response = action.apply(db, cr, param);
RestResult result = response.value();
verifyRestResult(result, 1, 1, changeInfo._number, false);
