FindOwners: Avoid reading project state in PluginConfigFactory
All of the "Project Not Found" exceptions happen when
Checker#findApproval is called from the prolog predicate.
We can get project state from the prolog engine rather than
read it in PluginConfigFactory. This could avoid PNF there.
For other places, we have to read from ProjectCache, which I
think will work.
Also I notice some classes in this plugin could be annotated by
@Singleton, e.g. GetOwners.
Change-Id: I6caf2572c366efa77af5fadacbd68e0052a2fe4a
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..7e80fd9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -32,6 +32,7 @@
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.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
@@ -60,6 +61,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 +77,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.
}
@@ -171,7 +175,15 @@
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);
+ OwnersDb db =
+ Cache.getInstance()
+ .get(
+ projectCache.get(changeData.project()),
+ accountCache,
+ emails,
+ repository,
+ changeData,
+ patchset);
Collection<String> changedFiles = changeData.currentFilePaths();
Map<String, Set<String>> file2Owners = db.findOwners(changedFiles);
@@ -219,7 +231,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 cb206b8..03225df 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.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;
@@ -104,10 +105,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,13 +133,15 @@
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;
}
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..e69275d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
@@ -19,6 +19,7 @@
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;
@@ -91,29 +92,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;
}
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/OwnersDb.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
index 2c595b2..9bb11a1 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 = "";
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..6874245 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -35,6 +35,7 @@
import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.PluginConfigFactory;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.inject.Inject;
import java.util.Collection;
import org.eclipse.jgit.lib.ObjectLoader;
@@ -51,6 +52,7 @@
@Inject private Emails emails;
@Inject private PluginConfigFactory configFactory;
+ @Inject private ProjectCache projectCache;
@Test
public void getOwnersTest() throws Exception {
@@ -259,9 +261,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 +275,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 +286,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 +330,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 +447,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);
}
