opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.14' into stable-2.15

Browse Source 
* stable-2.14:
  ConfigInfoImpl: Factor out initialization of MaxObjectSizeLimitInfo to a method
  Rename instances of TransferConfig to transferConfig
  Clarify inherited_value in documentation of MaxObjectSizeLimitInfo
  Reword docmentation of project specific object size limit for clarity
  PutConfig: Reload project config before returning response
  SiteProgram: Extract local variable
  SiteProgram: Extract constant
  SiteProgram: Inline variable to avoid hiding field
  DataSourceProvider: Rename local variable to avoid hiding field
  DataSourceProvider: Remove unneeded finals
  DataSourceProvider: Replace inner class with lambda
  DataSourceProvider: Extract constant
  Migrate `tools/bazel.rc` to `.bazelrc`
  ProjectIT: Add tests for maxObjectSize configuration
  EventRecorder: Add assertNoRefUpdatedEvents helper method
  ProjectIT: Rename 'config' method to 'submitType'

Change-Id: If803aa7390a2644c3ccd9baa04e1b874c3de5d40
This commit is contained in:
David Pursehouse
2018-08-07 22:13:18 +01:00
parent be7d28d392 ad87670f59
commit 02efb34cc8
10 changed files with 175 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

0
tools/bazel.rc → .bazelrc
View File

9
Documentation/config-project-config.txt
View File

@@ -140,9 +140,12 @@ objects which are too large to Gerrit. This setting can also be set in
`gerrit.config` globally (link:config-gerrit.html#receive.maxObjectSizeLimit[
receive.maxObjectSizeLimit]).
+
The project specific setting in `project.config` is only honored when it
further reduces the global limit. The setting is not inherited from the
parent project; it must be explicitly set per project.
The project specific setting in `project.config` may not set a value higher
than the global limit (if configured). In other words, it is only honored when
it further reduces the global limit.
+
The setting is not inherited from the parent project; it must be explicitly
set per project.
+
Default is zero.
+

3
Documentation/rest-api-projects.txt
View File

@@ -3062,7 +3062,8 @@ formatted string. +
Not set if there is no limit for the object size configured on project
level.
|`inherited_value` |optional|
The max object size limit that is inherited as a formatted string. +
The max object size limit that is inherited from the global config as a
formatted string. +
Not set if there is no global limit for the object size.
|===============================

4
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/EventRecorder.java
View File

@@ -137,6 +137,10 @@ public class EventRecorder {
return events;
}
public void assertNoRefUpdatedEvents(String project, String branch) throws Exception {
getRefUpdatedEvents(project, branch, 0);
}
public void assertRefUpdatedEvents(String project, String branch, String... expected)
throws Exception {
ImmutableList<RefUpdatedEvent> events =

114
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/ProjectIT.java
View File

@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.projects.BranchInput;
@@ -31,6 +32,7 @@ import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Test;
@@ -44,9 +46,9 @@ public class ProjectIT extends AbstractDaemonTest {
assertThat(name).isEqualTo(gApi.projects().create(name).get().name);
RevCommit head = getRemoteHead(name, RefNames.REFS_CONFIG);
eventRecorder.assertRefUpdatedEvents(name, RefNames.REFS_CONFIG, null, head);
eventRecorder.assertRefUpdatedEvents(name, "refs/heads/master", new String[] {});
eventRecorder.assertRefUpdatedEvents(name, RefNames.REFS_CONFIG, null, head);
eventRecorder.assertNoRefUpdatedEvents(name, "refs/heads/master");
}
@Test
@@ -55,9 +57,9 @@ public class ProjectIT extends AbstractDaemonTest {
assertThat(name).isEqualTo(gApi.projects().create(name + ".git").get().name);
RevCommit head = getRemoteHead(name, RefNames.REFS_CONFIG);
eventRecorder.assertRefUpdatedEvents(name, RefNames.REFS_CONFIG, null, head);
eventRecorder.assertRefUpdatedEvents(name, "refs/heads/master", new String[] {});
eventRecorder.assertRefUpdatedEvents(name, RefNames.REFS_CONFIG, null, head);
eventRecorder.assertNoRefUpdatedEvents(name, "refs/heads/master");
}
@Test
@@ -138,13 +140,13 @@ public class ProjectIT extends AbstractDaemonTest {
public void configChangeCausesRefUpdate() throws Exception {
RevCommit initialHead = getRemoteHead(project, RefNames.REFS_CONFIG);
ConfigInfo info = gApi.projects().name(project.get()).config();
ConfigInfo info = getConfig();
assertThat(info.submitType).isEqualTo(SubmitType.MERGE_IF_NECESSARY);
ConfigInput input = new ConfigInput();
input.submitType = SubmitType.CHERRY_PICK;
info = gApi.projects().name(project.get()).config(input);
info = setConfig(input);
assertThat(info.submitType).isEqualTo(SubmitType.CHERRY_PICK);
info = gApi.projects().name(project.get()).config();
info = getConfig();
assertThat(info.submitType).isEqualTo(SubmitType.CHERRY_PICK);
RevCommit updatedHead = getRemoteHead(project, RefNames.REFS_CONFIG);
@@ -208,6 +210,88 @@ public class ProjectIT extends AbstractDaemonTest {
gApi.projects().name(project.get()).config(input);
}
@Test
public void maxObjectSizeIsNotSetByDefault() throws Exception {
ConfigInfo info = getConfig();
assertThat(info.maxObjectSizeLimit.value).isNull();
assertThat(info.maxObjectSizeLimit.configuredValue).isNull();
assertThat(info.maxObjectSizeLimit.inheritedValue).isNull();
}
@Test
public void maxObjectSizeCanBeSetAndCleared() throws Exception {
// Set a value
ConfigInput input = new ConfigInput();
input.maxObjectSizeLimit = "100k";
ConfigInfo info = setConfig(input);
assertThat(info.maxObjectSizeLimit.value).isEqualTo("100k");
assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k");
assertThat(info.maxObjectSizeLimit.inheritedValue).isNull();
// Clear the value
input.maxObjectSizeLimit = "0";
info = setConfig(input);
assertThat(info.maxObjectSizeLimit.value).isNull();
assertThat(info.maxObjectSizeLimit.configuredValue).isNull();
assertThat(info.maxObjectSizeLimit.inheritedValue).isNull();
}
@Test
public void maxObjectSizeIsNotInheritedFromParentProject() throws Exception {
Project.NameKey child = createProject(name("child"), project);
ConfigInput input = new ConfigInput();
input.maxObjectSizeLimit = "100k";
ConfigInfo info = setConfig(input);
assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k");
assertThat(info.maxObjectSizeLimit.inheritedValue).isNull();
info = getConfig(child);
assertThat(info.maxObjectSizeLimit.value).isNull();
assertThat(info.maxObjectSizeLimit.configuredValue).isNull();
assertThat(info.maxObjectSizeLimit.inheritedValue).isNull();
}
@Test
@GerritConfig(name = "receive.maxObjectSizeLimit", value = "200k")
public void maxObjectSizeIsInheritedFromGlobalConfig() throws Exception {
ConfigInfo info = getConfig();
assertThat(info.maxObjectSizeLimit.value).isEqualTo("200k");
assertThat(info.maxObjectSizeLimit.configuredValue).isNull();
assertThat(info.maxObjectSizeLimit.inheritedValue).isEqualTo("200k");
}
@Test
@GerritConfig(name = "receive.maxObjectSizeLimit", value = "200k")
public void maxObjectSizeOverridesGlobalConfigWhenLower() throws Exception {
ConfigInput input = new ConfigInput();
input.maxObjectSizeLimit = "100k";
ConfigInfo info = setConfig(input);
assertThat(info.maxObjectSizeLimit.value).isEqualTo("100k");
assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k");
assertThat(info.maxObjectSizeLimit.inheritedValue).isEqualTo("200k");
}
@Test
@GerritConfig(name = "receive.maxObjectSizeLimit", value = "200k")
public void maxObjectSizeDoesNotOverrideGlobalConfigWhenHigher() throws Exception {
ConfigInput input = new ConfigInput();
input.maxObjectSizeLimit = "300k";
ConfigInfo info = setConfig(input);
assertThat(info.maxObjectSizeLimit.value).isEqualTo("200k");
assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("300k");
assertThat(info.maxObjectSizeLimit.inheritedValue).isEqualTo("200k");
}
@Test
public void invalidMaxObjectSizeIsRejected() throws Exception {
ConfigInput input = new ConfigInput();
input.maxObjectSizeLimit = "100 foo";
exception.expect(ResourceConflictException.class);
exception.expectMessage("100 foo");
setConfig(input);
}
private ConfigInput createTestConfigInput() {
ConfigInput input = new ConfigInput();
input.description = "some description";
@@ -224,4 +308,20 @@ public class ProjectIT extends AbstractDaemonTest {
input.state = ProjectState.HIDDEN;
return input;
}
private ConfigInfo setConfig(Project.NameKey name, ConfigInput input) throws Exception {
return gApi.projects().name(name.get()).config(input);
}
private ConfigInfo setConfig(ConfigInput input) throws Exception {
return setConfig(project, input);
}
private ConfigInfo getConfig(Project.NameKey name) throws Exception {
return gApi.projects().name(name.get()).config();
}
private ConfigInfo getConfig() throws Exception {
return getConfig(project);
}
}

16
gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java
View File

@@ -63,6 +63,8 @@ import org.eclipse.jgit.lib.Config;
import org.kohsuke.args4j.Option;
public abstract class SiteProgram extends AbstractProgram {
private static final String CONNECTION_ERROR = "Cannot connect to SQL database";
@Option(
name = "--site-path",
aliases = {"-d"},
@@ -105,14 +107,13 @@ public abstract class SiteProgram extends AbstractProgram {
/** @return provides database connectivity and site path. */
protected Injector createDbInjector(boolean enableMetrics, DataSourceProvider.Context context) {
Path sitePath = getSitePath();
List<Module> modules = new ArrayList<>();
Module sitePathModule =
new AbstractModule() {
@Override
protected void configure() {
bind(Path.class).annotatedWith(SitePath.class).toInstance(sitePath);
bind(Path.class).annotatedWith(SitePath.class).toInstance(getSitePath());
bind(String.class)
.annotatedWith(SecureStoreClassName.class)
.toProvider(Providers.of(getConfiguredSecureStoreClass()));
@@ -190,16 +191,16 @@ public abstract class SiteProgram extends AbstractProgram {
Throwable why = first.getCause();
if (why instanceof SQLException) {
throw die("Cannot connect to SQL database", why);
throw die(CONNECTION_ERROR, why);
}
if (why instanceof OrmException
&& why.getCause() != null
&& "Unable to determine driver URL".equals(why.getMessage())) {
why = why.getCause();
if (isCannotCreatePoolException(why)) {
throw die("Cannot connect to SQL database", why.getCause());
throw die(CONNECTION_ERROR, why.getCause());
}
throw die("Cannot connect to SQL database", why);
throw die(CONNECTION_ERROR, why);
}
StringBuilder buf = new StringBuilder();
@@ -246,8 +247,9 @@ public abstract class SiteProgram extends AbstractProgram {
for (Binding<DataSourceType> binding : dsTypeBindings) {
Annotation annotation = binding.getKey().getAnnotation();
if (annotation instanceof Named) {
if (((Named) annotation).value().toLowerCase().contains(dbProductName)) {
return ((Named) annotation).value();
Named named = (Named) annotation;
if (named.value().toLowerCase().contains(dbProductName)) {
return named.value();
}
}
}

24
gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java
View File

@@ -40,7 +40,7 @@ public class ConfigInfoImpl extends ConfigInfo {
public ConfigInfoImpl(
boolean serverEnableSignedPush,
ProjectControl control,
TransferConfig config,
TransferConfig transferConfig,
DynamicMap<ProjectConfigEntry> pluginConfigEntries,
PluginConfigFactory cfgFactory,
AllProjectsName allProjects,
@@ -114,14 +114,7 @@ public class ConfigInfoImpl extends ConfigInfo {
this.privateByDefault = privateByDefault;
this.workInProgressByDefault = workInProgressByDefault;
MaxObjectSizeLimitInfo maxObjectSizeLimit = new MaxObjectSizeLimitInfo();
maxObjectSizeLimit.value =
config.getEffectiveMaxObjectSizeLimit(projectState) == config.getMaxObjectSizeLimit()
? config.getFormattedMaxObjectSizeLimit()
: p.getMaxObjectSizeLimit();
maxObjectSizeLimit.configuredValue = p.getMaxObjectSizeLimit();
maxObjectSizeLimit.inheritedValue = config.getFormattedMaxObjectSizeLimit();
this.maxObjectSizeLimit = maxObjectSizeLimit;
this.maxObjectSizeLimit = getMaxObjectSizeLimit(projectState, transferConfig, p);
this.submitType = p.getSubmitType();
this.state =
@@ -146,6 +139,19 @@ public class ConfigInfoImpl extends ConfigInfo {
this.extensionPanelNames = projectState.getConfig().getExtensionPanelSections();
}
private MaxObjectSizeLimitInfo getMaxObjectSizeLimit(
ProjectState projectState, TransferConfig transferConfig, Project p) {
MaxObjectSizeLimitInfo info = new MaxObjectSizeLimitInfo();
info.value =
transferConfig.getEffectiveMaxObjectSizeLimit(projectState)
== transferConfig.getMaxObjectSizeLimit()
? transferConfig.getFormattedMaxObjectSizeLimit()
: p.getMaxObjectSizeLimit();
info.configuredValue = p.getMaxObjectSizeLimit();
info.inheritedValue = transferConfig.getFormattedMaxObjectSizeLimit();
return info;
}
private Map<String, Map<String, ConfigParameterInfo>> getPluginConfig(
ProjectState project,
DynamicMap<ProjectConfigEntry> pluginConfigEntries,

8
gerrit-server/src/main/java/com/google/gerrit/server/project/GetConfig.java
View File

@@ -30,7 +30,7 @@ import com.google.inject.Singleton;
@Singleton
public class GetConfig implements RestReadView<ProjectResource> {
private final boolean serverEnableSignedPush;
private final TransferConfig config;
private final TransferConfig transferConfig;
private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
private final PluginConfigFactory cfgFactory;
private final AllProjectsName allProjects;
@@ -40,14 +40,14 @@ public class GetConfig implements RestReadView<ProjectResource> {
@Inject
public GetConfig(
@EnableSignedPush boolean serverEnableSignedPush,
TransferConfig config,
TransferConfig transferConfig,
DynamicMap<ProjectConfigEntry> pluginConfigEntries,
PluginConfigFactory cfgFactory,
AllProjectsName allProjects,
UiActions uiActions,
DynamicMap<RestView<ProjectResource>> views) {
this.serverEnableSignedPush = serverEnableSignedPush;
this.config = config;
this.transferConfig = transferConfig;
this.pluginConfigEntries = pluginConfigEntries;
this.allProjects = allProjects;
this.cfgFactory = cfgFactory;
@@ -60,7 +60,7 @@ public class GetConfig implements RestReadView<ProjectResource> {
return new ConfigInfoImpl(
serverEnableSignedPush,
resource.getControl(),
config,
transferConfig,
pluginConfigEntries,
cfgFactory,
allProjects,

10
gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java
View File

@@ -61,7 +61,7 @@ public class PutConfig implements RestModifyView<ProjectResource, ConfigInput> {
private final Provider<MetaDataUpdate.User> metaDataUpdateFactory;
private final ProjectCache projectCache;
private final ProjectState.Factory projectStateFactory;
private final TransferConfig config;
private final TransferConfig transferConfig;
private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
private final PluginConfigFactory cfgFactory;
private final AllProjectsName allProjects;
@@ -75,7 +75,7 @@ public class PutConfig implements RestModifyView<ProjectResource, ConfigInput> {
Provider<MetaDataUpdate.User> metaDataUpdateFactory,
ProjectCache projectCache,
ProjectState.Factory projectStateFactory,
TransferConfig config,
TransferConfig transferConfig,
DynamicMap<ProjectConfigEntry> pluginConfigEntries,
PluginConfigFactory cfgFactory,
AllProjectsName allProjects,
@@ -86,7 +86,7 @@ public class PutConfig implements RestModifyView<ProjectResource, ConfigInput> {
this.metaDataUpdateFactory = metaDataUpdateFactory;
this.projectCache = projectCache;
this.projectStateFactory = projectStateFactory;
this.config = config;
this.transferConfig = transferConfig;
this.pluginConfigEntries = pluginConfigEntries;
this.cfgFactory = cfgFactory;
this.allProjects = allProjects;
@@ -193,11 +193,11 @@ public class PutConfig implements RestModifyView<ProjectResource, ConfigInput> {
throw new ResourceConflictException("Cannot update " + projectName, e);
}
ProjectState state = projectStateFactory.create(projectConfig);
ProjectState state = projectStateFactory.create(ProjectConfig.read(md));
return new ConfigInfoImpl(
serverEnableSignedPush,
state.controlFor(user.get()),
config,
transferConfig,
pluginConfigEntries,
cfgFactory,
allProjects,

44
gerrit-server/src/main/java/com/google/gerrit/server/schema/DataSourceProvider.java
View File

@@ -44,6 +44,8 @@ import org.eclipse.jgit.lib.Config;
/** Provides access to the DataSource. */
@Singleton
public class DataSourceProvider implements Provider<DataSource>, LifecycleListener {
private static final String DATABASE_KEY = "database";
private final Config cfg;
private final MetricMaker metrics;
private final Context ctx;
@@ -93,7 +95,7 @@ public class DataSourceProvider implements Provider<DataSource>, LifecycleListen
}
private DataSource open(Config cfg, Context context, DataSourceType dst) {
ConfigSection dbs = new ConfigSection(cfg, "database");
ConfigSection dbs = new ConfigSection(cfg, DATABASE_KEY);
String driver = dbs.optional("driver");
if (Strings.isNullOrEmpty(driver)) {
driver = dst.getDriver();
@@ -112,41 +114,41 @@ public class DataSourceProvider implements Provider<DataSource>, LifecycleListen
if (context == Context.SINGLE_USER) {
usePool = false;
} else {
usePool = cfg.getBoolean("database", "connectionpool", dst.usePool());
usePool = cfg.getBoolean(DATABASE_KEY, "connectionpool", dst.usePool());
}
if (usePool) {
final BasicDataSource ds = new BasicDataSource();
ds.setDriverClassName(driver);
ds.setUrl(url);
final BasicDataSource lds = new BasicDataSource();
lds.setDriverClassName(driver);
lds.setUrl(url);
if (username != null && !username.isEmpty()) {
ds.setUsername(username);
lds.setUsername(username);
}
if (password != null && !password.isEmpty()) {
ds.setPassword(password);
lds.setPassword(password);
}
int poolLimit = threadSettingsConfig.getDatabasePoolLimit();
ds.setMaxActive(poolLimit);
ds.setMinIdle(cfg.getInt("database", "poolminidle", 4));
ds.setMaxIdle(cfg.getInt("database", "poolmaxidle", Math.min(poolLimit, 16)));
ds.setMaxWait(
lds.setMaxActive(poolLimit);
lds.setMinIdle(cfg.getInt(DATABASE_KEY, "poolminidle", 4));
lds.setMaxIdle(cfg.getInt(DATABASE_KEY, "poolmaxidle", Math.min(poolLimit, 16)));
lds.setMaxWait(
ConfigUtil.getTimeUnit(
cfg,
"database",
DATABASE_KEY,
null,
"poolmaxwait",
MILLISECONDS.convert(30, SECONDS),
MILLISECONDS));
ds.setInitialSize(ds.getMinIdle());
lds.setInitialSize(lds.getMinIdle());
long evictIdleTimeMs = 1000L * 60;
ds.setMinEvictableIdleTimeMillis(evictIdleTimeMs);
ds.setTimeBetweenEvictionRunsMillis(evictIdleTimeMs / 2);
ds.setTestOnBorrow(true);
ds.setTestOnReturn(true);
ds.setValidationQuery(dst.getValidationQuery());
ds.setValidationQueryTimeout(5);
exportPoolMetrics(ds);
return intercept(interceptor, ds);
lds.setMinEvictableIdleTimeMillis(evictIdleTimeMs);
lds.setTimeBetweenEvictionRunsMillis(evictIdleTimeMs / 2);
lds.setTestOnBorrow(true);
lds.setTestOnReturn(true);
lds.setValidationQuery(dst.getValidationQuery());
lds.setValidationQueryTimeout(5);
exportPoolMetrics(lds);
return intercept(interceptor, lds);
}
// Don't use the connection pool.
//