Merge branch 'stable-2.15'
* stable-2.15:
Fix name of commons-io dependency
Fix name of prolog dependency
Format BUILD file with buildifier
Change-Id: I11275980a4fa628483f46e1b3ff4d05f52002961
diff --git a/BUILD b/BUILD
index 0371641..8c3dff9 100644
--- a/BUILD
+++ b/BUILD
@@ -1,29 +1,42 @@
load("//lib/prolog:prolog.bzl", "prolog_cafe_library")
load("//tools/bzl:junit.bzl", "junit_tests")
-load("//tools/bzl:plugin.bzl", "PLUGIN_DEPS", "PLUGIN_TEST_DEPS", "gerrit_plugin")
+load(
+ "//tools/bzl:plugin.bzl",
+ "PLUGIN_DEPS",
+ "PLUGIN_DEPS_NEVERLINK",
+ "PLUGIN_TEST_DEPS",
+ "gerrit_plugin",
+)
+
+MODULE = ["src/main/java/com/googlesource/gerrit/plugins/findowners/Module.java"]
java_library(
name = "find-owners-lib",
- srcs = glob(["src/main/java/**/*.java"]),
- deps = PLUGIN_DEPS + ["@prolog-runtime//jar"],
+ srcs = glob(
+ ["src/main/java/**/*.java"],
+ exclude = MODULE,
+ ),
+ deps = PLUGIN_DEPS_NEVERLINK + [
+ "@prolog-runtime//jar:neverlink",
+ ],
)
prolog_cafe_library(
name = "find-owners-prolog-rules",
srcs = glob(["src/main/prolog/*.pl"]),
- deps = [
+ deps = PLUGIN_DEPS_NEVERLINK + [
":find-owners-lib",
- "//gerrit-server:prolog-common",
],
)
gerrit_plugin(
name = "find-owners",
- srcs = glob(["src/main/java/**/Module.java"]),
+ srcs = MODULE,
manifest_entries = [
"Gerrit-PluginName: find-owners",
"Gerrit-ReloadMode: restart",
"Gerrit-Module: com.googlesource.gerrit.plugins.findowners.Module",
+ "Gerrit-BatchModule: com.googlesource.gerrit.plugins.findowners.PredicateModule",
"Implementation-Title: Find-Owners plugin",
"Implementation-URL: https://gerrit.googlesource.com/plugins/find-owners",
],
@@ -37,11 +50,11 @@
junit_tests(
name = "findowners_tests",
srcs = glob(["src/test/java/**/*.java"]),
- # resources = glob(['src/test/resources/**/*']),
tags = ["findowners"],
deps = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [
"@commons-io//jar",
":find-owners-lib",
":find-owners-prolog-rules",
+ ":find-owners__plugin",
],
)
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 d232382..361cecc 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -14,13 +14,15 @@
package com.googlesource.gerrit.plugins.findowners;
+import static java.util.stream.Collectors.toList;
+
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.Streams;
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.webui.UiAction;
-import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -32,6 +34,8 @@
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
@@ -44,6 +48,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
@@ -60,6 +65,7 @@
private GitRepositoryManager repoManager;
private Provider<CurrentUser> userProvider;
private SchemaFactory<ReviewDb> reviewDbProvider;
+ private ProjectCache projectCache;
static class Parameters {
Boolean debug; // REST API "debug" parameter, or null
@@ -75,13 +81,15 @@
ChangeData.Factory changeDataFactory,
AccountCache accountCache,
Emails emails,
- GitRepositoryManager repoManager) {
+ GitRepositoryManager repoManager,
+ ProjectCache projectCache) {
this.userProvider = userProvider;
this.reviewDbProvider = reviewDbProvider;
this.changeDataFactory = changeDataFactory;
this.accountCache = accountCache;
this.emails = emails;
this.repoManager = repoManager;
+ this.projectCache = projectCache;
Config.setVariables(pluginName, configFactory);
Cache.getInstance(); // Create a single Cache.
}
@@ -90,8 +98,8 @@
Action() {}
private String getUserName() {
- if (userProvider != null && userProvider.get().getUserName() != null) {
- return userProvider.get().getUserName();
+ if (userProvider != null && userProvider.get().getUserName().isPresent()) {
+ return userProvider.get().getUserName().get();
}
return "?";
}
@@ -136,17 +144,21 @@
/** Returns reviewer emails got from ChangeData. */
static List<String> getReviewers(ChangeData changeData, AccountCache accountCache) {
- List<String> result = new ArrayList<>();
try {
- for (Account.Id id : changeData.reviewers().all()) {
- Account account = accountCache.get(id).getAccount();
- result.add(account.getPreferredEmail());
- }
+ // Reviewers may have no preferred email, skip them if the preferred email is not set.
+ return changeData
+ .reviewers()
+ .all()
+ .stream()
+ .map(accountCache::get)
+ .flatMap(Streams::stream)
+ .map(a -> a.getAccount().getPreferredEmail())
+ .filter(Objects::nonNull)
+ .collect(toList());
} catch (OrmException e) {
log.error("Exception for " + Config.getChangeId(changeData), e);
- result = new ArrayList<>();
+ return new ArrayList<>();
}
- return result;
}
/** Returns the current patchset number or the given patchsetNum if it is valid. */
@@ -171,12 +183,16 @@
Repository repository, Parameters params, ChangeData changeData)
throws OrmException, BadRequestException, IOException {
int patchset = getValidPatchsetNum(changeData, params.patchset);
- OwnersDb db = Cache.getInstance().get(accountCache, emails, repository, changeData, patchset);
+ ProjectState projectState = projectCache.get(changeData.project());
+ OwnersDb db =
+ Cache.getInstance()
+ .get(projectState, accountCache, emails, repository, changeData, patchset);
Collection<String> changedFiles = changeData.currentFilePaths();
Map<String, Set<String>> file2Owners = db.findOwners(changedFiles);
Boolean addDebugMsg = (params.debug != null) ? params.debug : Config.getAddDebugMsg();
- RestResult obj = new RestResult(Config.getMinOwnerVoteLevel(changeData), addDebugMsg);
+ RestResult obj =
+ new RestResult(Config.getMinOwnerVoteLevel(projectState, changeData), addDebugMsg);
obj.change = changeData.getId().get();
obj.patchset = patchset;
obj.ownerRevision = db.revision;
@@ -219,7 +235,14 @@
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(accountCache, emails, repo, changeData);
+ OwnersDb db =
+ Cache.getInstance()
+ .get(
+ projectCache.get(resource.getProject()),
+ accountCache,
+ emails,
+ 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 aa43896..3f48166 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
@@ -17,9 +17,9 @@
import static java.util.concurrent.TimeUnit.SECONDS;
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.Emails;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import java.io.IOException;
@@ -88,48 +88,59 @@
}
/** Returns a cached or new OwnersDb, for the current patchset. */
- OwnersDb get(AccountCache accountCache, Emails emails, Repository repo, ChangeData changeData)
+ OwnersDb get(
+ ProjectState projectState,
+ AccountCache accountCache,
+ Emails emails,
+ Repository repo,
+ ChangeData changeData)
throws OrmException, IOException {
- return get(accountCache, emails, repo, changeData, changeData.currentPatchSet().getId().get());
+ return get(
+ projectState,
+ accountCache,
+ emails,
+ repo,
+ changeData,
+ changeData.currentPatchSet().getId().get());
}
/** Returns a cached or new OwnersDb, for the specified patchset. */
OwnersDb get(
+ ProjectState projectState,
AccountCache accountCache,
Emails emails,
Repository repository,
ChangeData changeData,
int patchset)
throws OrmException, IOException {
- 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(
+ projectState,
accountCache,
emails,
dbKey,
repository,
changeData,
- project,
branch,
changeData.currentFilePaths());
}
/** Returns a cached or new OwnersDb, for the specified branch and changed files. */
OwnersDb get(
+ ProjectState projectState,
AccountCache accountCache,
Emails emails,
String key,
Repository repository,
ChangeData changeData,
- Project.NameKey project,
String branch,
Collection<String> files) {
if (dbCache == null) { // Do not cache OwnersDb
log.trace("Create new OwnersDb, key=" + key);
return new OwnersDb(
- accountCache, emails, key, repository, changeData, project, branch, files);
+ projectState, accountCache, emails, key, repository, changeData, branch, files);
}
try {
log.trace("Get from cash " + dbCache + ", key=" + key + ", cache size=" + dbCache.size());
@@ -140,13 +151,13 @@
public OwnersDb call() {
log.trace("Create new OwnersDb, key=" + key);
return new OwnersDb(
- accountCache, emails, key, repository, changeData, project, branch, files);
+ projectState, accountCache, emails, key, repository, changeData, branch, files);
}
});
} catch (ExecutionException e) {
log.error("Cache.get has exception for " + Config.getChangeId(changeData), e);
return new OwnersDb(
- accountCache, emails, key, repository, changeData, project, branch, files);
+ projectState, accountCache, emails, key, repository, changeData, 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 ae163d5..54861a1 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -16,15 +16,17 @@
import com.google.gerrit.reviewdb.client.Change.Status;
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.Emails;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.StoredValues;
import com.google.gwtorm.server.OrmException;
import com.googlecode.prolog_cafe.lang.Prolog;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.lib.Repository;
import org.slf4j.Logger;
@@ -54,16 +56,24 @@
Map<String, Integer> map = new HashMap<>();
for (PatchSetApproval p : changeData.currentApprovals()) {
if (p.getValue() != 0) {
- map.put(
- accountCache.get(p.getAccountId()).getAccount().getPreferredEmail(),
- Integer.valueOf(p.getValue()));
+ // Reviewers may have no preferred email, skip them if the preferred email is not set.
+ Optional<String> preferredEmail =
+ accountCache.get(p.getAccountId()).map(a -> a.getAccount().getPreferredEmail());
+ if (preferredEmail.isPresent()) {
+ map.put(preferredEmail.get(), Integer.valueOf(p.getValue()));
+ }
}
}
// Give CL author a default minVoteLevel vote.
- String author =
- accountCache.get(changeData.change().getOwner()).getAccount().getPreferredEmail();
- if (!map.containsKey(author) || map.get(author) == 0) {
- map.put(author, minVoteLevel);
+ // The preferred email of the author may not be set. Pushing changes only requires an email in
+ // the external IDs, but the preferred email may still be null (also emails may have been
+ // deleted after creating the change). Skip the author if it doesn't have a preferred email.
+ Optional<String> author =
+ accountCache
+ .get(changeData.change().getOwner())
+ .map(a -> a.getAccount().getPreferredEmail());
+ if (author.isPresent() && (!map.containsKey(author.get()) || map.get(author.get()) == 0)) {
+ map.put(author.get(), minVoteLevel);
}
return map;
}
@@ -104,10 +114,12 @@
ChangeData changeData = null;
try {
changeData = StoredValues.CHANGE_DATA.get(engine);
+ ProjectState projectState = StoredValues.PROJECT_STATE.get(engine);
AccountCache accountCache = StoredValues.ACCOUNT_CACHE.get(engine);
Emails emails = StoredValues.EMAILS.get(engine);
Repository repository = StoredValues.REPOSITORY.get(engine);
- return new Checker(repository, changeData, minVoteLevel).findApproval(accountCache, emails);
+ return new Checker(repository, changeData, minVoteLevel)
+ .findApproval(projectState, accountCache, emails);
} catch (OrmException | IOException e) {
log.error("Exception for " + Config.getChangeId(changeData), e);
return 0; // owner approval may or may not be required.
@@ -130,18 +142,20 @@
return (status == Status.ABANDONED || status == Status.MERGED);
}
- int findApproval(AccountCache accountCache, Emails emails) throws OrmException, IOException {
+ int findApproval(ProjectState projectState, AccountCache accountCache, Emails emails)
+ throws OrmException, IOException {
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(accountCache, emails, repository, changeData);
+ OwnersDb db =
+ Cache.getInstance().get(projectState, accountCache, emails, repository, changeData);
if (db.getNumOwners() <= 0) {
return 0;
}
if (minVoteLevel <= 0) {
- minVoteLevel = Config.getMinOwnerVoteLevel(changeData);
+ minVoteLevel = Config.getMinOwnerVoteLevel(projectState, changeData);
}
log.trace("findApproval db key = " + db.key);
return findApproval(accountCache, db);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
index e450dfb..c09b3df 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
@@ -15,12 +15,10 @@
package com.googlesource.gerrit.plugins.findowners;
import com.google.common.annotations.VisibleForTesting;
-import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.config.PluginConfig;
import com.google.gerrit.server.config.PluginConfigFactory;
-import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gwtorm.server.OrmException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -91,29 +89,31 @@
return data == null ? "(unknown change)" : ("change c/" + data.getId().get());
}
- static String getOwnersFileName(Project.NameKey project, ChangeData c) {
- if (config != null && project != null) {
- try {
- String name =
- config
- .getFromProjectConfigWithInheritance(project, PLUGIN_NAME)
- .getString(OWNERS_FILE_NAME, OWNERS);
- if (name.trim().equals("")) {
- log.error(
- "Project "
- + project
- + " has wrong "
- + OWNERS_FILE_NAME
- + ": \""
- + name
- + "\" for "
- + getChangeId(c));
- return OWNERS;
- }
- return name;
- } catch (NoSuchProjectException e) {
- log.error("Cannot find project " + project + " for " + getChangeId(c), e);
+ static String getDefaultOwnersFileName() {
+ return OWNERS;
+ }
+
+ static String getOwnersFileName(ProjectState projectState, ChangeData c) {
+ if (projectState == null) {
+ log.error("Null projectState for change " + getChangeId(c));
+ } else if (config != null) {
+ String name =
+ config
+ .getFromProjectConfigWithInheritance(projectState, PLUGIN_NAME)
+ .getString(OWNERS_FILE_NAME, OWNERS);
+ if (name.trim().equals("")) {
+ log.error(
+ "Project "
+ + projectState.getProject()
+ + " has wrong "
+ + OWNERS_FILE_NAME
+ + ": \""
+ + name
+ + "\" for "
+ + getChangeId(c));
+ return OWNERS;
}
+ return name;
}
return OWNERS;
}
@@ -123,17 +123,16 @@
reportSyntaxError = value;
}
- static int getMinOwnerVoteLevel(ChangeData changeData) throws OrmException {
- Project.NameKey project = changeData.change().getProject();
- try {
- return (config == null || project == null)
- ? minOwnerVoteLevel
- : config
- .getFromProjectConfigWithInheritance(project, PLUGIN_NAME)
- .getInt(MIN_OWNER_VOTE_LEVEL, minOwnerVoteLevel);
- } catch (NoSuchProjectException e) {
- log.error("Cannot find project " + project + " for " + getChangeId(changeData), e);
+ static int getMinOwnerVoteLevel(ProjectState projectState, ChangeData c) {
+ if (projectState == null) {
+ log.error("Null projectState for change " + getChangeId(c));
return minOwnerVoteLevel;
+ } else if (config == null) {
+ return minOwnerVoteLevel;
+ } else {
+ return config
+ .getFromProjectConfigWithInheritance(projectState, PLUGIN_NAME)
+ .getInt(MIN_OWNER_VOTE_LEVEL, minOwnerVoteLevel);
}
}
}
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 fd60850..322a3b5 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
@@ -25,6 +25,7 @@
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
@@ -58,7 +59,8 @@
ChangeData.Factory dataFactory,
AccountCache accountCache,
Emails emails,
- GitRepositoryManager repoManager) {
+ GitRepositoryManager repoManager,
+ ProjectCache projectCache) {
this.action =
new Action(
pluginName,
@@ -68,7 +70,8 @@
dataFactory,
accountCache,
emails,
- repoManager);
+ repoManager,
+ projectCache);
}
@Override
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Module.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Module.java
index e57bd51..aab4097 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Module.java
@@ -17,34 +17,14 @@
import static com.google.gerrit.server.change.ChangeResource.CHANGE_KIND;
import static com.google.gerrit.server.change.RevisionResource.REVISION_KIND;
-import com.google.common.collect.ImmutableSet;
-import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.RestApiModule;
import com.google.gerrit.extensions.webui.JavaScriptPlugin;
import com.google.gerrit.extensions.webui.WebUiPlugin;
-import com.google.gerrit.rules.PredicateProvider;
-import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.inject.AbstractModule;
-import com.google.inject.Inject;
/** find-owners plugin module */
public class Module extends AbstractModule {
- /** Prolog Predicate Provider. */
- static class FindOwnersProvider implements PredicateProvider {
-
- @Inject
- public FindOwnersProvider(@PluginName String pluginName, PluginConfigFactory configFactory) {
- Config.setVariables(pluginName, configFactory);
- Cache.getInstance(); // Create a single Cache.
- }
-
- @Override
- public ImmutableSet<String> getPackages() {
- return ImmutableSet.of(Config.PROLOG_NAMESPACE);
- }
- }
-
@Override
protected void configure() {
install(OwnersValidator.module());
@@ -58,6 +38,7 @@
});
DynamicSet.bind(binder(), WebUiPlugin.class)
.toInstance(new JavaScriptPlugin(Config.PLUGIN_NAME + ".js"));
- DynamicSet.bind(binder(), PredicateProvider.class).to(FindOwnersProvider.class);
+
+ install(new PredicateModule());
}
}
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 2c595b2..bd84c13 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -18,9 +18,9 @@
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.Emails;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
@@ -63,19 +63,19 @@
OwnersDb() {}
OwnersDb(
+ ProjectState projectState,
AccountCache accountCache,
Emails emails,
String key,
Repository repository,
ChangeData changeData,
- Project.NameKey project,
String branch,
Collection<String> files) {
this.accountCache = accountCache;
this.emails = emails;
this.key = key;
preferredEmails.put("*", "*");
- String ownersFileName = Config.getOwnersFileName(project, changeData);
+ String ownersFileName = Config.getOwnersFileName(projectState, changeData);
// Some hacked CL could have a target branch that is not created yet.
ObjectId id = getBranchId(repository, branch, changeData);
revision = "";
@@ -99,7 +99,7 @@
}
}
try {
- revision = repository.getRef(branch).getObjectId().getName();
+ revision = repository.exactRef(branch).getObjectId().getName();
} catch (Exception e) {
log.error("Fail to get branch revision for " + Config.getChangeId(changeData), e);
}
@@ -152,7 +152,12 @@
if (ids == null || ids.size() != 1) {
errors.add(owner);
} else {
- email = accountCache.get(ids.iterator().next()).getAccount().getPreferredEmail();
+ // Accounts may have no preferred email.
+ email =
+ accountCache
+ .get(ids.iterator().next())
+ .map(a -> a.getAccount().getPreferredEmail())
+ .orElse(null);
}
}
} catch (Exception e) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
index 8d81574..866a2e7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
@@ -51,7 +51,6 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
-import java.util.concurrent.ExecutionException;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.eclipse.jgit.diff.RawText;
@@ -142,7 +141,7 @@
messages = performValidation(receiveEvent.commit, receiveEvent.revWalk, name, false);
}
}
- } catch (NoSuchProjectException | IOException | ExecutionException e) {
+ } catch (NoSuchProjectException | IOException e) {
throw new CommitValidationException("failed to check owners files", e);
}
if (hasError(messages)) {
@@ -153,8 +152,7 @@
@VisibleForTesting
List<CommitValidationMessage> performValidation(
- RevCommit c, RevWalk revWalk, String ownersFileName, boolean verbose)
- throws IOException, ExecutionException {
+ RevCommit c, RevWalk revWalk, String ownersFileName, boolean verbose) throws IOException {
// Collect all messages from all files.
List<CommitValidationMessage> messages = new LinkedList<>();
// Collect all email addresses from all files and check each address only once.
@@ -197,7 +195,7 @@
if (!wrongEmail) {
try {
Collection<Account.Id> ids = email2ids.get(owner);
- wrongEmail = (ids == null || ids.size() != 1);
+ wrongEmail = (ids == null || ids.isEmpty());
} catch (Exception e) {
wrongEmail = true;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/PredicateModule.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/PredicateModule.java
new file mode 100644
index 0000000..d0fb040
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/PredicateModule.java
@@ -0,0 +1,46 @@
+// Copyright (C) 2018 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.googlesource.gerrit.plugins.findowners;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.config.PluginConfigFactory;
+import com.google.gerrit.server.rules.PredicateProvider;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+
+/** provides the Prolog predicate, even in a batch mode */
+public class PredicateModule extends AbstractModule {
+ /** Prolog Predicate Provider. */
+ static class FindOwnersProvider implements PredicateProvider {
+
+ @Inject
+ public FindOwnersProvider(@PluginName String pluginName, PluginConfigFactory configFactory) {
+ Config.setVariables(pluginName, configFactory);
+ Cache.getInstance(); // Create a single Cache.
+ }
+
+ @Override
+ public ImmutableSet<String> getPackages() {
+ return ImmutableSet.of(Config.PROLOG_NAMESPACE);
+ }
+ }
+
+ @Override
+ protected void configure() {
+ DynamicSet.bind(binder(), PredicateProvider.class).to(FindOwnersProvider.class);
+ }
+}
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 296f408..15f01de 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
@@ -57,5 +57,5 @@
List<String> errors;
SortedMap<String, List<String>> path2owners;
SortedMap<String, List<String>> owner2paths;
- };
+ }
}
diff --git a/src/main/resources/static/find-owners.js b/src/main/resources/static/find-owners.js
index 0abdabe..24a8554 100644
--- a/src/main/resources/static/find-owners.js
+++ b/src/main/resources/static/find-owners.js
@@ -505,6 +505,19 @@
}
return true; // Okay to submit.
}
+ var actionKey = null;
+ function onShowChangePolyGerrit(change, revision) {
+ var changeActions = self.changeActions();
+ // Hide previous 'Find Owners' button under 'MORE'.
+ changeActions.setActionHidden('revision', 'find-owners~find-owners', true);
+ if (!!actionKey) {
+ changeActions.setActionHidden('revision', actionKey, false);
+ } else {
+ actionKey = changeActions.add('revision', 'Find Owners');
+ changeActions.addTapListener(actionKey,
+ () => popupFindOwnersPage(null, change, revision, false));
+ }
+ }
function onClick(e) {
if (pageDiv.style.visibility != 'hidden' && !useContextPopup) {
var x = event.clientX;
@@ -522,6 +535,10 @@
} else {
console.log('WARNING, no handler for the Find Owners button');
}
+ // When using PolyGerrit, move "Find Owners" button out of the 'MORE' list.
+ if (window.Polymer) {
+ self.on('showchange', onShowChangePolyGerrit);
+ }
// When the "Submit" button is clicked, call onSubmit.
self.on('submitchange', onSubmit);
// Clicks outside the pop up window should close the window.
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 160d5fa..323a861 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -15,6 +15,7 @@
package com.googlesource.gerrit.plugins.findowners;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -32,11 +33,13 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.inject.Inject;
import java.util.Collection;
+import java.util.Optional;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevObject;
@@ -208,7 +211,9 @@
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]);
+ Optional<Account> account = accountCache.get(id1).map(AccountState::getAccount);
+ assertThat(account).named("account %s", id1).isPresent();
+ assertThat(account.get().getPreferredEmail()).isEqualTo(emails1[i]);
}
// Wrong or non-existing email address.
String[] wrongEmails = {"nobody", "@g.com", "nobody@g.com", "*"};
@@ -259,9 +264,9 @@
PushOneCommit.Result cB = createChange("add tB.c", "tB.c", "Hello B!");
// Default owners file name is "OWNERS".
- assertThat(Config.getOwnersFileName(null, null)).isEqualTo("OWNERS");
- assertThat(Config.getOwnersFileName(pA, null)).isEqualTo("OWNERS");
- assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS");
+ assertThat(Config.getDefaultOwnersFileName()).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(projectCache.get(pA), null)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS");
String ownerX = oneOwnerList("x@x");
String ownerY = oneOwnerList("y@y");
@@ -273,8 +278,9 @@
setProjectConfig("ownersFileName", "OWNERS.alpha");
switchProject(pB);
setProjectConfig("ownersFileName", "OWNERS.beta");
- assertThat(Config.getOwnersFileName(pA, null)).isEqualTo("OWNERS.alpha");
- assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS.beta");
+
+ assertThat(Config.getOwnersFileName(projectCache.get(pA), null)).isEqualTo("OWNERS.alpha");
+ assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS.beta");
String ownerA = oneOwnerList("a@a");
String ownerB = oneOwnerList("b@b");
assertThat(getOwnersResponse(cA)).contains(ownerA + ", files:[ tA.c ]");
@@ -283,24 +289,24 @@
// Change back to OWNERS in Project_A
switchProject(pA);
setProjectConfig("ownersFileName", "OWNERS");
- assertThat(Config.getOwnersFileName(pA, null)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(projectCache.get(pA), null)).isEqualTo("OWNERS");
assertThat(getOwnersResponse(cA)).contains(ownerX + ", files:[ tA.c ]");
assertThat(getOwnersResponse(cB)).contains(ownerB + ", files:[ tB.c ]");
// Change back to OWNERS.alpha in Project_B, but there is no OWNERS.alpha
switchProject(pB);
setProjectConfig("ownersFileName", "OWNERS.alpha");
- assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS.alpha");
+ assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS.alpha");
assertThat(getOwnersResponse(cA)).contains(ownerX + ", files:[ tA.c ]");
assertThat(getOwnersResponse(cB)).contains("owners:[], files:[ tB.c ]");
// Do not accept empty string or all-white-spaces for ownersFileName.
setProjectConfig("ownersFileName", " ");
- assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS");
setProjectConfig("ownersFileName", " \t ");
- assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("OWNERS");
setProjectConfig("ownersFileName", "O");
- assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("O");
+ assertThat(Config.getOwnersFileName(projectCache.get(pB), null)).isEqualTo("O");
}
@Test
@@ -327,7 +333,15 @@
Action.Parameters param = new Action.Parameters();
Action action =
new Action(
- "find-owners", null, null, null, changeDataFactory, accountCache, emails, repoManager);
+ "find-owners",
+ null,
+ null,
+ null,
+ changeDataFactory,
+ accountCache,
+ emails,
+ repoManager,
+ projectCache);
Response<RestResult> response = action.apply(db, cr, param);
RestResult result = response.value();
verifyRestResult(result, 1, 1, changeInfo._number, false);
@@ -436,9 +450,11 @@
}
private int checkApproval(PushOneCommit.Result r) throws Exception {
- Repository repo = repoManager.openRepository(r.getChange().project());
+ Project.NameKey project = r.getChange().project();
+ Repository repo = repoManager.openRepository(project);
Cache cache = Cache.getInstance().init(0, 0);
- OwnersDb db = cache.get(accountCache, emails, repo, r.getChange(), 1);
+ OwnersDb db =
+ cache.get(projectCache.get(project), accountCache, emails, repo, r.getChange(), 1);
Checker c = new Checker(repo, r.getChange(), 1);
return c.findApproval(accountCache, db);
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
index 68e8b3d..b8d445a 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
@@ -59,7 +59,7 @@
Set<String> registered;
MockedEmails() {
- super(null, null);
+ super(null, null, null);
registered =
ImmutableSet.of(
"u1@g.com", "u2@g.com", "u2.m@g.com", "user1@google.com", "u1+review@g.com");
