Clarify and cleanup Config variables and functions.
* Clarify gerrit.config and project.config parameters.
* "alwaysShowButton" is obsolete, assumed true now.
* Include all parameters to REST API returned JSON object;
fix expected test output in ApiIT.java.
* Explain all parameters in rest-api.md.
* Minimize repeated calls to create PluginConfig:
* Change Action, Cache, Checker, OwnersDb, FindOwners to reuse
Config instead of creating it from PluginConfigFactory.
* Save one gerritConfig in Config and Cache projectConfig in Config.
* Add/simplify unit test interface functions.
* Coding style improvements:
* Use Duration.ofSeconds instead of TimeUnit.SECONDS.
Change-Id: If27b7ae0e0f59f434ec2d536e1b08af0857708e0
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 0d9f967..c97b9f8 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -58,7 +58,6 @@
private final ChangeData.Factory changeDataFactory;
private final GitRepositoryManager repoManager;
private final PermissionBackend permissionBackend;
- private final PluginConfigFactory configFactory;
private final Provider<CurrentUser> userProvider;
private final ProjectCache projectCache;
private final Config config;
@@ -86,7 +85,6 @@
this.emails = emails;
this.repoManager = repoManager;
this.projectCache = projectCache;
- this.configFactory = configFactory;
this.config = new Config(configFactory);
}
@@ -164,7 +162,7 @@
ProjectState projectState = projectCache.get(changeData.project());
Boolean useCache = params.nocache == null || !params.nocache;
OwnersDb db =
- Cache.getInstance(configFactory, repoManager)
+ Cache.getInstance(config, repoManager)
.get(
useCache,
permissionBackend,
@@ -172,22 +170,17 @@
accountCache,
emails,
repoManager,
- configFactory,
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(projectState, changeData), addDebugMsg);
- obj.change = changeData.getId().get();
+ RestResult obj = new RestResult(config, projectState, changeData, addDebugMsg);
obj.patchset = patchset;
obj.ownerRevision = db.revision;
if (addDebugMsg) {
obj.dbgmsgs.user = getUserName();
- obj.dbgmsgs.project = changeData.change().getProject().get();
- obj.dbgmsgs.branch = changeData.change().getDest().branch();
obj.dbgmsgs.errors = db.errors;
obj.dbgmsgs.path2owners = Util.makeSortedMap(db.path2Owners);
obj.dbgmsgs.owner2paths = Util.makeSortedMap(db.owner2Paths);
@@ -220,23 +213,6 @@
&& userProvider.get() instanceof IdentifiedUser
&& status != Status.ABANDONED
&& status != Status.MERGED;
- // If alwaysShowButton is true, skip expensive owner lookup.
- if (needFindOwners && !config.getAlwaysShowButton()) {
- needFindOwners = false; // Show button only if some owner is found.
- OwnersDb db =
- Cache.getInstance(configFactory, repoManager)
- .get(
- true, // use cached OwnersDb
- permissionBackend,
- projectCache.get(resource.getProject()),
- accountCache,
- emails,
- repoManager,
- configFactory,
- changeData);
- logger.atFiner().log("getDescription db key = %s", db.key);
- needFindOwners = db.getNumOwners() > 0;
- }
return new Description()
.setLabel("Find Owners")
.setTitle("Find owners to add to Reviewers list")
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 7d2cfa5..df766e4 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
@@ -14,18 +14,17 @@
package com.googlesource.gerrit.plugins.findowners;
-import static java.util.concurrent.TimeUnit.SECONDS;
import com.google.common.cache.CacheBuilder;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.Emails;
-import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
+import java.time.Duration;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
@@ -61,8 +60,11 @@
// dbCache key is generated by makeKey.
private com.google.common.cache.Cache<String, OwnersDb> dbCache;
- private Cache(int maxSeconds, int maxSize) {
- init(maxSeconds, maxSize);
+ private Config config; // global config shared by all OwnersDb in dbCache
+
+ private Cache(Config config) {
+ this.config = config;
+ init(config.getMaxCacheAge(), config.getMaxCacheSize());
}
long size() {
@@ -80,7 +82,7 @@
dbCache =
CacheBuilder.newBuilder()
.maximumSize(maxSize)
- .expireAfterWrite(maxSeconds, SECONDS)
+ .expireAfterWrite(Duration.ofSeconds(maxSeconds))
.build();
} else {
logger.atInfo().log("Cache disabled.");
@@ -97,7 +99,6 @@
AccountCache accountCache,
Emails emails,
GitRepositoryManager repoManager,
- PluginConfigFactory configFactory,
ChangeData changeData)
throws StorageException {
return get(
@@ -107,7 +108,6 @@
accountCache,
emails,
repoManager,
- configFactory,
changeData,
changeData.currentPatchSet().id().get());
}
@@ -120,7 +120,6 @@
AccountCache accountCache,
Emails emails,
GitRepositoryManager repoManager,
- PluginConfigFactory configFactory,
ChangeData changeData,
int patchset)
throws StorageException {
@@ -135,7 +134,6 @@
emails,
dbKey,
repoManager,
- configFactory,
changeData,
branch,
changeData.currentFilePaths());
@@ -150,7 +148,6 @@
Emails emails,
String key,
GitRepositoryManager repoManager,
- PluginConfigFactory configFactory,
ChangeData changeData,
String branch,
Collection<String> files) {
@@ -163,7 +160,7 @@
emails,
key,
repoManager,
- configFactory,
+ config,
changeData,
branch,
files);
@@ -184,7 +181,7 @@
emails,
key,
repoManager,
- configFactory,
+ config,
changeData,
branch,
files);
@@ -200,7 +197,7 @@
emails,
key,
repoManager,
- configFactory,
+ config,
changeData,
branch,
files);
@@ -211,14 +208,13 @@
return String.format("%d:%d:%H", change, patch, repoManager);
}
- public static Cache getInstance(
- PluginConfigFactory configFactory, GitRepositoryManager repoManager) {
+ public static Cache getInstance(Config config, GitRepositoryManager repoManager) {
Cache cache = cacheMap.get(repoManager);
if (cache == null) {
- Config config = new Config(configFactory);
- cache = new Cache(config.getMaxCacheAge(), config.getMaxCacheSize());
+ cache = new Cache(config);
cacheMap.put(repoManager, cache);
}
+ cache.config = config; // Always use newest config, although dbCache could have older data.
return cache;
}
}
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 190f1eb..0004bf2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -40,7 +40,6 @@
private static final String EXEMPT_MESSAGE2 = "Exempted-From-Owner-Approval:";
private final GitRepositoryManager repoManager;
- private final PluginConfigFactory configFactory;
private final Config config;
private final ProjectState projectState; // could be null when used by FindOwnersIT
private final ChangeData changeData;
@@ -53,7 +52,6 @@
ChangeData changeData,
int v) {
this.repoManager = repoManager;
- this.configFactory = configFactory;
this.projectState = projectState;
this.changeData = changeData;
this.config = new Config(configFactory);
@@ -154,7 +152,7 @@
// 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(configFactory, repoManager)
+ Cache.getInstance(config, repoManager)
.get(
true,
null, /* allow submit checker to read all OWNERS files */
@@ -162,7 +160,6 @@
accountCache,
emails,
repoManager,
- configFactory,
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 9d5ec98..6a35190 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
@@ -16,23 +16,30 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.flogger.FluentLogger;
+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 java.util.HashMap;
+import java.util.Map;
/** find-owners configuration parameters */
class Config {
- // Name of config parameters.
- static final String ADD_DEBUG_MSG = "addDebugMsg";
- static final String ALWAYS_SHOW_BUTTON = "alwaysShowButton"; // always show "Find Owners" button
+ // Name of config parameters that should be defined in gerrit.config:
+ static final String ADD_DEBUG_MSG = "addDebugMsg"; // include "dbgmsgs" in returned JSON object
static final String MAX_CACHE_AGE = "maxCacheAge"; // seconds to stay in cache
static final String MAX_CACHE_SIZE = "maxCacheSize"; // number of OwnersDb in cache
static final String MIN_OWNER_VOTE_LEVEL = "minOwnerVoteLevel"; // default +1
- static final String OWNERS = "OWNERS"; // Default file name
+ static final String REPORT_SYNTAX_ERROR = "reportSyntaxError"; // only for tests
+ // "alwaysShowButton" is obsolete, new UI design always shows the [Find Owners] button
+
+ // Name of config parameters that can be defined in project.config or gerrit.config:
static final String OWNERS_FILE_NAME = "ownersFileName"; // config key for file name
- static final String REJECT_ERROR_IN_OWNERS = "rejectErrorInOwners"; // config key for validator
- static final String REPORT_SYNTAX_ERROR = "reportSyntaxError";
+ static final String REJECT_ERROR_IN_OWNERS = "rejectErrorInOwners"; // enable upload validator
+
+ static final String OWNERS = "OWNERS"; // default OWNERS file name
// Name of plugin and namespace.
static final String PLUGIN_NAME = "find-owners";
@@ -40,31 +47,45 @@
private final PluginConfigFactory configFactory;
+ // Each call to API entry point creates one new Config and parses gerrit.config.
+ private final PluginConfig gerritConfig;
+
+ // Each Config has a cache of project.config, with projectName:changeId key.
+ private final Map<String, PluginConfig> projectConfigMap;
+
// Global/plugin config parameters.
private boolean addDebugMsg = false;
private int minOwnerVoteLevel = 1;
private int maxCacheAge = 0;
private int maxCacheSize = 1000;
private boolean reportSyntaxError = false;
- private boolean alwaysShowButton = false;
- private String ownersFileName = OWNERS;
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
Config(PluginConfigFactory configFactory) {
+ // Called by Action() and Checker.findApproval, through Prolog submit filter.
+ this(configFactory, null);
+ }
+
+ @VisibleForTesting
+ Config(PluginConfig rawConfig) {
+ this(null, rawConfig);
+ }
+
+ Config(PluginConfigFactory configFactory, PluginConfig config) {
this.configFactory = configFactory;
- if (configFactory == null) { // When called from integration tests.
+ this.projectConfigMap = new HashMap<>();
+ if (configFactory == null && config == null) { // When called from integration tests.
+ this.gerritConfig = config;
return;
}
- PluginConfig gc = configFactory.getFromGerritConfig(PLUGIN_NAME);
- // Get config variables from the plugin section of gerrit.config
- addDebugMsg = gc.getBoolean(ADD_DEBUG_MSG, false);
- reportSyntaxError = gc.getBoolean(REPORT_SYNTAX_ERROR, false);
- alwaysShowButton = gc.getBoolean(ALWAYS_SHOW_BUTTON, false);
- minOwnerVoteLevel = gc.getInt(MIN_OWNER_VOTE_LEVEL, 1);
- maxCacheAge = gc.getInt(MAX_CACHE_AGE, 0);
- maxCacheSize = gc.getInt(MAX_CACHE_SIZE, 1000);
- ownersFileName = gc.getString(OWNERS_FILE_NAME, OWNERS);
+ this.gerritConfig = config == null ? configFactory.getFromGerritConfig(PLUGIN_NAME) : config;
+ // Get config variables from the plugin section of gerrit.config.
+ addDebugMsg = gerritConfig.getBoolean(ADD_DEBUG_MSG, false);
+ minOwnerVoteLevel = gerritConfig.getInt(MIN_OWNER_VOTE_LEVEL, 1);
+ maxCacheAge = gerritConfig.getInt(MAX_CACHE_AGE, 0);
+ maxCacheSize = gerritConfig.getInt(MAX_CACHE_SIZE, 1000);
+ reportSyntaxError = gerritConfig.getBoolean(REPORT_SYNTAX_ERROR, false);
}
boolean getAddDebugMsg() {
@@ -79,33 +100,91 @@
return maxCacheSize;
}
+ boolean getGlobalBooleanValue(String key) {
+ return gerritConfig != null && gerritConfig.getBoolean(key, false);
+ }
+
+ boolean getRejectErrorInOwners() {
+ return getGlobalBooleanValue(REJECT_ERROR_IN_OWNERS);
+ }
+
+ boolean getRejectErrorInOwners(Project project) {
+ return getBooleanValue(project, REJECT_ERROR_IN_OWNERS);
+ }
+
+ boolean getRejectErrorInOwners(ProjectState projectState, ChangeData changeData) {
+ return getBooleanValue(projectState, changeData, REJECT_ERROR_IN_OWNERS);
+ }
+
+ boolean getBooleanValue(Project project, String key) {
+ return getBooleanValue(project, key, getGlobalBooleanValue(key));
+ }
+
+ boolean getBooleanValue(Project project, String key, boolean defaultValue) {
+ try {
+ return getProjectConfig(project).getBoolean(key, defaultValue);
+ } catch (NoSuchProjectException e) {
+ logger.atSevere().withCause(e).log(
+ "Exception in getBooleanValue for %s:%s", project.getName(), key);
+ return defaultValue;
+ }
+ }
+
+ boolean getBooleanValue(ProjectState projectState, ChangeData changeData, String key) {
+ return getBooleanValue(projectState, changeData, key, getGlobalBooleanValue(key));
+ }
+
+ boolean getBooleanValue(
+ ProjectState projectState, ChangeData changeData, String key, boolean defaultValue) {
+ return getProjectConfig(projectState, changeData).getBoolean(key, defaultValue);
+ }
+
boolean getReportSyntaxError() {
return reportSyntaxError;
}
- boolean getAlwaysShowButton() {
- return alwaysShowButton;
- }
-
static String getChangeId(ChangeData data) {
- return data == null ? "(unknown change)" : ("change c/" + data.getId().get());
+ return data == null ? "(unknown change)" : ("c/" + data.getId().get());
}
String getDefaultOwnersFileName() {
- return ownersFileName; // could be defined in gerrit.config
+ return gerritConfig == null ? OWNERS : gerritConfig.getString(OWNERS_FILE_NAME, OWNERS);
}
- private PluginConfig getPluginConfig(ProjectState state) {
- return configFactory.getFromProjectConfigWithInheritance(state, PLUGIN_NAME);
+ // This is per ProjectState and ChangeData.
+ private PluginConfig getProjectConfig(ProjectState state, ChangeData data) {
+ // A new Config object is created for every call to Action or Checker.
+ // So it is okay to reuse a PluginConfig per (ProjectState:ChangeData).
+ // ProjectState parameter must not be null.
+ // The data parameter could be used in the future to generate change
+ // dependent PluginConfig.
+ String key = state.getName() + ":" + getChangeId(data);
+ return projectConfigMap.computeIfAbsent(
+ key, (String k) -> configFactory.getFromProjectConfigWithInheritance(state, PLUGIN_NAME));
+ }
+
+ // Used by OwnersValidator and tests, not cached.
+ PluginConfig getProjectConfig(Project project) throws NoSuchProjectException {
+ return configFactory.getFromProjectConfigWithInheritance(project.getNameKey(), PLUGIN_NAME);
+ }
+
+ String getOwnersFileName() {
+ return getOwnersFileName(null, null);
+ }
+
+ String getOwnersFileName(ProjectState projectState) {
+ return getOwnersFileName(projectState, null);
}
String getOwnersFileName(ProjectState projectState, ChangeData c) {
String defaultName = getDefaultOwnersFileName();
if (projectState == null) {
- logger.atSevere().log("Null projectState for change %s", getChangeId(c));
+ if (c != null) {
+ logger.atSevere().log("Null projectState for change %s", getChangeId(c));
+ }
return defaultName;
}
- String name = getPluginConfig(projectState).getString(OWNERS_FILE_NAME, defaultName);
+ String name = getProjectConfig(projectState, c).getString(OWNERS_FILE_NAME, defaultName);
if (name.trim().isEmpty()) {
logger.atSevere().log(
"Project %s has wrong %s: \"%s\" for %s",
@@ -115,6 +194,22 @@
return name;
}
+ String getOwnersFileName(Project project) {
+ String defaultName = getDefaultOwnersFileName();
+ try {
+ String name = getProjectConfig(project).getString(OWNERS_FILE_NAME, defaultName);
+ if (name.trim().isEmpty()) {
+ logger.atSevere().log("Project %s has empty %s", project, OWNERS_FILE_NAME);
+ return defaultName;
+ }
+ return name;
+ } catch (NoSuchProjectException e) {
+ logger.atSevere().withCause(e).log(
+ "Exception in getOwnersFileName for %s", project.getName());
+ return defaultName;
+ }
+ }
+
@VisibleForTesting
void setReportSyntaxError(boolean value) {
reportSyntaxError = value;
@@ -125,6 +220,6 @@
logger.atSevere().log("Null projectState for change %s", getChangeId(c));
return minOwnerVoteLevel;
}
- return getPluginConfig(projectState).getInt(MIN_OWNER_VOTE_LEVEL, minOwnerVoteLevel);
+ return getProjectConfig(projectState, c).getInt(MIN_OWNER_VOTE_LEVEL, minOwnerVoteLevel);
}
}
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 db3bc5b..34e9c1e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -24,7 +24,6 @@
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.config.PluginConfigFactory;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -82,7 +81,7 @@
Emails emails,
String key,
GitRepositoryManager repoManager,
- PluginConfigFactory configFactory,
+ Config config,
ChangeData changeData,
String branch,
Collection<String> files) {
@@ -91,7 +90,7 @@
this.repoManager = repoManager;
this.emails = emails;
this.key = key;
- this.config = new Config(configFactory);
+ this.config = config;
try {
InetAddress inetAddress = InetAddress.getLocalHost();
logs.add("HostName:" + inetAddress.getHostName());
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 7669d55..aa69aa9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
@@ -15,14 +15,12 @@
package com.googlesource.gerrit.plugins.findowners;
import static com.google.common.base.Strings.isNullOrEmpty;
-import static com.googlesource.gerrit.plugins.findowners.Config.OWNERS;
-import static com.googlesource.gerrit.plugins.findowners.Config.OWNERS_FILE_NAME;
import static com.googlesource.gerrit.plugins.findowners.Config.REJECT_ERROR_IN_OWNERS;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Multimap;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.annotations.Exports;
-import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.api.projects.ProjectConfigEntryType;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.reviewdb.client.Account;
@@ -36,7 +34,6 @@
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
-import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import java.io.BufferedReader;
@@ -64,6 +61,8 @@
/** Check syntax of changed OWNERS files. */
public class OwnersValidator implements CommitValidationListener {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private interface TreeWalkVisitor {
void onVisit(TreeWalk tw);
}
@@ -87,67 +86,67 @@
};
}
- private final String pluginName;
- private final PluginConfigFactory cfgFactory;
+ private final Config config;
private final GitRepositoryManager repoManager;
private final Emails emails;
@Inject
- OwnersValidator(
- @PluginName String pluginName,
- PluginConfigFactory cfgFactory,
- GitRepositoryManager repoManager,
- Emails emails) {
- this.pluginName = pluginName;
- this.cfgFactory = cfgFactory;
+ OwnersValidator(PluginConfigFactory cfgFactory, GitRepositoryManager repoManager, Emails emails) {
+ this(new Config(cfgFactory), repoManager, emails);
+ }
+
+ @VisibleForTesting
+ OwnersValidator(PluginConfig config, GitRepositoryManager repoManager, Emails emails) {
+ this(new Config(config), repoManager, emails);
+ }
+
+ private OwnersValidator(Config config, GitRepositoryManager repoManager, Emails emails) {
+ this.config = config;
this.repoManager = repoManager;
this.emails = emails;
}
- public static String getOwnersFileName(PluginConfig cfg) {
- return getOwnersFileName(cfg, OWNERS);
- }
-
- public static String getOwnersFileName(PluginConfig cfg, String defaultName) {
- return cfg.getString(OWNERS_FILE_NAME, defaultName);
- }
-
- public String getOwnersFileName(Project.NameKey project) {
- String name = getOwnersFileName(cfgFactory.getFromGerritConfig(pluginName, true));
- try {
- return getOwnersFileName(
- cfgFactory.getFromProjectConfigWithInheritance(project, pluginName), name);
- } catch (NoSuchProjectException e) {
- return name;
- }
+ @VisibleForTesting
+ String getOwnersFileName() {
+ return config.getOwnersFileName();
}
@VisibleForTesting
- static boolean isActive(PluginConfig cfg) {
- return cfg.getBoolean(REJECT_ERROR_IN_OWNERS, false);
+ public String getOwnersFileName(Project project) {
+ return config.getOwnersFileName(project);
+ }
+
+ @VisibleForTesting
+ boolean isActive(Project project) {
+ return config.getRejectErrorInOwners(project);
+ }
+
+ @VisibleForTesting
+ boolean isActive() {
+ return config.getRejectErrorInOwners();
}
@Override
public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent event)
throws CommitValidationException {
+ if (!isActive(event.project)) {
+ return new ArrayList<>();
+ }
Checker checker = new Checker(event, false);
try {
- Project.NameKey project = event.project.getNameKey();
- PluginConfig cfg = cfgFactory.getFromProjectConfigWithInheritance(project, pluginName);
- if (isActive(cfg)) {
- checker.check(getOwnersFileName(project));
+ checker.check(getOwnersFileName(event.project));
+ if (checker.hasError()) {
+ checker.addError(
+ "See OWNERS file syntax document at "
+ + "https://gerrit.googlesource.com/plugins/find-owners/+/"
+ + "master/src/main/resources/Documentation/syntax.md");
+ throw new CommitValidationException("found invalid owners file", checker.messages);
}
- } catch (NoSuchProjectException | IOException e) {
- throw new CommitValidationException("failed to check owners files", e);
+ return checker.messages;
+ } catch (IOException e) {
+ logger.atSevere().withCause(e).log("Failed in onCommitReceived");
+ return new ArrayList<>();
}
- if (checker.hasError()) {
- checker.addError(
- "See OWNERS file syntax document at "
- + "https://gerrit.googlesource.com/plugins/find-owners/+/"
- + "master/src/main/resources/Documentation/syntax.md");
- throw new CommitValidationException("found invalid owners file", checker.messages);
- }
- return checker.messages;
}
class Checker {
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 378f2de..dc108c7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
@@ -14,6 +14,8 @@
package com.googlesource.gerrit.plugins.findowners;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.query.change.ChangeData;
import com.google.gson.annotations.SerializedName;
import java.util.ArrayList;
import java.util.List;
@@ -22,11 +24,23 @@
/** REST API return data Java class. */
public class RestResult {
+ @SerializedName("addDebugMsg")
+ boolean addDebugMsg;
+
+ @SerializedName("maxCacheAge")
+ int maxCacheAge;
+
+ @SerializedName("maxCacheSize")
+ int maxCacheSize;
+
@SerializedName("minOwnerVoteLevel")
int minOwnerVoteLevel;
- @SerializedName("addDebugMsg")
- boolean addDebugMsg;
+ @SerializedName("ownersFileName")
+ String ownersFileName;
+
+ @SerializedName("rejectErrorInOwners")
+ boolean rejectErrorInOwners;
int change;
int patchset;
@@ -40,13 +54,20 @@
List<OwnerInfo> owners = new ArrayList<>();
List<String> files = new ArrayList<>();
- RestResult(int voteLevel, boolean addDebugMsg) {
- minOwnerVoteLevel = voteLevel;
+ RestResult(Config config, ProjectState projectState, ChangeData changeData, boolean addDebugMsg) {
this.addDebugMsg = addDebugMsg;
+ maxCacheAge = config.getMaxCacheAge();
+ maxCacheSize = config.getMaxCacheSize();
+ minOwnerVoteLevel = config.getMinOwnerVoteLevel(projectState, changeData);
+ ownersFileName = config.getOwnersFileName(projectState, changeData);
+ rejectErrorInOwners = config.getRejectErrorInOwners(projectState, changeData);
+ change = changeData.getId().get();
if (addDebugMsg) {
dbgmsgs = new DebugMessages();
dbgmsgs.path2owners = new TreeMap<>();
dbgmsgs.owner2paths = new TreeMap<>();
+ dbgmsgs.project = changeData.change().getProject().get();
+ dbgmsgs.branch = changeData.change().getDest().branch();
}
}
diff --git a/src/main/resources/Documentation/rest-api.md b/src/main/resources/Documentation/rest-api.md
index 435b12b..41829e5 100644
--- a/src/main/resources/Documentation/rest-api.md
+++ b/src/main/resources/Documentation/rest-api.md
@@ -16,7 +16,7 @@
* **patchset**: is the patchset number of the change to look for changed files.
By default the current (latest) patchset of given change is used.
-* **debug**: can be set to true or false to override the configuration variable
+* **debug**: can be set to true/1 or false/0 to override the configuration variable
**addDebugMsg**.
* **nocache**: can be set to true to collect owerns info without using the cached OwnersDb.
@@ -31,13 +31,28 @@
This API returns a JSON object with the following attributes:
-* **minOwnerVoteLevel**: is 1 by default, but could be set to 2.
+* **addDebugMsg**: is false by default. It can be set to true in a
+ development/test gerrit.config file, or with the `debug=1`
+ REST URL parameter. When it is true, extra **dbgmsgs** attributes
+ are included in this JSON object.
+
+* **maxCacheAge**: has default value 0; can be defined in gerrit.config.
+ It is the number of seconds OWNERS info that will stay in a cache.
+
+* **maxCacheSize**: has default value 100; can be defined in gerrit.config.
+ It is the number of most recently accessed CLs OWNERS info that will stay in a cache.
+
+* **minOwnerVoteLevel**: is 1 by default; can be set to 2 in gerrit.config.
It is the minimal Code-Review vote value all changed files must get
from at least one owner to make a change submittable.
-* **addDebugMsg**: is false by default. In a development/test configuration,
- this attribute could be true, to tell a client to display extra debug
- information.
+* **ownersFileName**: is "OWNERS" by default; can be redefined in project.config
+ or gerrit.config. It is the name of OWNERS file.
+
+* **rejectErrorInOwners**: is false by default; can be redefined in
+ project.config or gerrit.config.
+ When enabled, the CL uploader will check and reject OWNERS file with
+ wrong syntax or unknown email address.
* **change**: is the change number.
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/ApiIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/ApiIT.java
index 514d146..ecd1401 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/ApiIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/ApiIT.java
@@ -46,8 +46,9 @@
ChangeInfo info2 = newChangeInfo("test2 GetOwners");
assertThat(info2._number).isEqualTo(info1._number + 1);
String expected =
- ")]}' { minOwnerVoteLevel:1, addDebugMsg:false, change:"
- + info1._number
+ ")]}' { addDebugMsg:false, maxCacheAge:0, maxCacheSize:1000,"
+ + " minOwnerVoteLevel:1, ownersFileName:OWNERS, rejectErrorInOwners:false,"
+ + (" change:" + info1._number)
+ ", patchset:1, file2owners:{}, reviewers:[], owners:[], files:[] }";
Cache cache = getCache().init(0, 10); // reset, no Cache
assertThat(cache.size()).isEqualTo(0L);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java
index f0dcd43..d414755 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java
@@ -176,7 +176,6 @@
accountCache,
emails,
repoManager,
- pluginConfig,
r.getChange(),
1);
Checker c = new Checker(repoManager, pluginConfig, null, r.getChange(), 1);
@@ -241,10 +240,13 @@
}
protected String projectOwnersFileName(Project.NameKey name) {
+ // This function is called repeatedly in ConfigIT without recreating config.
+ // So, here we recreate config, to get the latest owners file name.
+ setConfig();
return config.getOwnersFileName(projectCache.get(name), null);
}
protected Cache getCache() {
- return Cache.getInstance(pluginConfig, repoManager);
+ return Cache.getInstance(config, repoManager);
}
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java
index 0030953..0eb7df1 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java
@@ -126,18 +126,22 @@
private static final PluginConfig ENABLED_CONFIG = createEnabledConfig(); // use OWNERS
private static final PluginConfig DISABLED_CONFIG = createDisabledConfig();
+ private OwnersValidator newOwnersValidator(PluginConfig cfg) {
+ return new OwnersValidator(cfg, repoManager, new MockedEmails());
+ }
+
@Test
public void chekIsActiveAndFileName() throws Exception {
// This check should be enabled in project.config, default is not active.
- assertThat(OwnersValidator.isActive(EMPTY_CONFIG)).isFalse();
- assertThat(OwnersValidator.isActive(ENABLED_CONFIG)).isTrue();
- assertThat(OwnersValidator.isActive(ANDROID_CONFIG)).isTrue();
- assertThat(OwnersValidator.isActive(DISABLED_CONFIG)).isFalse();
+ assertThat(newOwnersValidator(EMPTY_CONFIG).isActive()).isFalse();
+ assertThat(newOwnersValidator(ENABLED_CONFIG).isActive()).isTrue();
+ assertThat(newOwnersValidator(ANDROID_CONFIG).isActive()).isTrue();
+ assertThat(newOwnersValidator(DISABLED_CONFIG).isActive()).isFalse();
// Default file name is "OWNERS".
- assertThat(OwnersValidator.getOwnersFileName(EMPTY_CONFIG)).isEqualTo(OWNERS);
- assertThat(OwnersValidator.getOwnersFileName(ENABLED_CONFIG)).isEqualTo(OWNERS);
- assertThat(OwnersValidator.getOwnersFileName(DISABLED_CONFIG)).isEqualTo(OWNERS);
- assertThat(OwnersValidator.getOwnersFileName(ANDROID_CONFIG)).isEqualTo(OWNERS_ANDROID);
+ assertThat(newOwnersValidator(EMPTY_CONFIG).getOwnersFileName()).isEqualTo(OWNERS);
+ assertThat(newOwnersValidator(ENABLED_CONFIG).getOwnersFileName()).isEqualTo(OWNERS);
+ assertThat(newOwnersValidator(DISABLED_CONFIG).getOwnersFileName()).isEqualTo(OWNERS);
+ assertThat(newOwnersValidator(ANDROID_CONFIG).getOwnersFileName()).isEqualTo(OWNERS_ANDROID);
}
private static final ImmutableMap<String, String> FILES_WITHOUT_OWNERS =
@@ -392,10 +396,9 @@
private List<String> validate(CommitReceivedEvent event, boolean verbose, PluginConfig cfg)
throws Exception {
- OwnersValidator validator =
- new OwnersValidator("find-owners", pluginConfig, repoManager, new MockedEmails());
+ OwnersValidator validator = newOwnersValidator(cfg);
OwnersValidator.Checker checker = validator.new Checker(event, verbose);
- checker.check(OwnersValidator.getOwnersFileName(cfg));
+ checker.check(validator.getOwnersFileName());
return transformMessages(checker.messages);
}
