Clean up the DWIMery for database.* configuration settings This code was a mess. We went through a lot of hoops to try and figure out what the administrator wanted from us given strange combinations of database.type and database.url. Instead use a very simple set of rules that pretty much requires database.type or database.url to be set to indicate how we should proceed. Change-Id: I44ef20592df288616907d967b46c87fecc9ed80c Signed-off-by: Shawn O. Pearce <sop@google.com>
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 931060b..451cabb 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt
@@ -531,7 +531,7 @@ ---- [database] - type = postgres + type = POSTGRESQL hostname = localhost database = reviewdb username = gerrit2 @@ -544,17 +544,21 @@ used to automatically create correct database.driver and database.url values to open the connection. + -* `Postgres` or `PostgreSQL` +* `POSTGRESQL` + Connect to a PostgreSQL database server. + * `H2` + -Connect to a local (or remote) H2 database. +Connect to a local embedded H2 database. + -* `MySQL` +* `MYSQL` + Connect to a MySQL database server. ++ +* `JDBC` ++ +Connect using a JDBC driver class name and URL. + If not specified, database.driver and database.url are used as-is, @@ -571,10 +575,10 @@ [[database.database]]database.database:: + -For PostgreSQL or MySQL, the name of the database on the server. +For POSTGRESQL or MYSQL, the name of the database on the server. + For H2, this is the path to the database, and if not absolute is -relative to `$site_path`. +relative to `'$site_path'`. [[database.username]]database.username:: + @@ -586,13 +590,15 @@ [[database.driver]]database.driver:: + -Name of the JDBC driver class to connect to the database. -Setting this usually isn't necessary, set database.type instead. +Name of the JDBC driver class to connect to the database with. +Setting this usually isn't necessary as it can be derived from +database.type or database.url for any supported database. [[database.url]]database.url:: + -JDBC style URL for the database. Setting this usually isn't -necessary, set database.type instead. +'jdbc:' URL for the database. Setting this variable usually +isn't necessary as it can be constructed from the all of the +above properties. [[gerrit]]Section gerrit
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitDatabase.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitDatabase.java index c3270ab..d501ea5 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitDatabase.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitDatabase.java
@@ -82,7 +82,6 @@ break; } - case POSTGRES: case POSTGRESQL: case MYSQL: { userPassAuth = true;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/DataSourceProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/DataSourceProvider.java index 91d6346..74f620b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/DataSourceProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/DataSourceProvider.java
@@ -14,8 +14,9 @@ package com.google.gerrit.server.schema; +import static com.google.gerrit.server.config.ConfigUtil.getEnum; + import com.google.gerrit.lifecycle.LifecycleListener; -import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; import com.google.gwtorm.jdbc.SimpleDataSource; @@ -71,132 +72,87 @@ } public static enum Type { - DEFAULT, JDBC, POSTGRES, POSTGRESQL, H2, MYSQL; + H2, POSTGRESQL, MYSQL, JDBC; } private DataSource open(final SitePaths site, final Config cfg, final Context context) { - Type type = ConfigUtil.getEnum(cfg, "database", null, "type", Type.DEFAULT); + Type type = getEnum(cfg, "database", null, "type", Type.values(), null); String driver = optional(cfg, "driver"); String url = optional(cfg, "url"); String username = optional(cfg, "username"); String password = optional(cfg, "password"); - String hostname = optional(cfg, "hostname"); - String port = optional(cfg, "port"); - if (hostname == null) { - hostname = "localhost"; - } - if (Type.DEFAULT == type && (driver == null || driver.isEmpty())) { - if (url != null && url.isEmpty()) { - - if (url.startsWith("jdbc:postgresql:")) { - type = Type.POSTGRES; - } else if (url.startsWith("postgresql:")) { - url = "jdbc:" + url; - type = Type.POSTGRES; - } else if (url.startsWith("postgres:")) { - url = "jdbc:postgresql:" + url.substring(url.indexOf(':') + 1); - type = Type.POSTGRES; - - } else if (url.startsWith("jdbc:h2:")) { + if (url == null || url.isEmpty()) { + if (type == null) { + if (url != null && !url.isEmpty()) { + type = Type.JDBC; + } else { type = Type.H2; - } else if (url.startsWith("h2:")) { - url = "jdbc:" + url; - type = Type.H2; - - } else if (url.startsWith("jdbc:mysql:")) { - type = Type.MYSQL; - } else if (url.startsWith("mysql:")) { - url = "jdbc:" + url; - type = Type.MYSQL; - } - - } else if (url == null || url.isEmpty()) { - type = Type.H2; - } - } - - switch (type) { - case POSTGRES: - case POSTGRESQL: { - final String pfx = "jdbc:postgresql://"; - driver = "org.postgresql.Driver"; - if (url == null) { - final StringBuilder b = new StringBuilder(); - b.append(pfx); - b.append(hostname); - if (port != null && !port.isEmpty()) { - b.append(":"); - b.append(port); - } - b.append("/"); - b.append(required(cfg, "database")); - url = b.toString(); - } - if (url == null || !url.startsWith(pfx)) { - throw new IllegalArgumentException("database.url must be " + pfx - + " and not " + url); - } - break; } - case H2: { - final String pfx = "jdbc:h2:"; - driver = "org.h2.Driver"; - if (url == null) { + switch (type) { + case H2: { String database = optional(cfg, "database"); if (database == null || database.isEmpty()) { database = "db/ReviewDB"; } - File db = site.resolve(database); try { db = db.getCanonicalFile(); } catch (IOException e) { db = db.getAbsoluteFile(); } - url = pfx + db.toURI().toString(); + url = "jdbc:h2:" + db.toURI().toString(); + break; } - if (url == null || !url.startsWith(pfx)) { - throw new IllegalArgumentException("database.url must be " + pfx - + " and not " + url); - } - break; - } - case MYSQL: { - final String pfx = "jdbc:mysql://"; - driver = "com.mysql.jdbc.Driver"; - if (url == null) { + case POSTGRESQL: { final StringBuilder b = new StringBuilder(); - b.append(pfx); - b.append(hostname); - if (port != null && !port.isEmpty()) { - b.append(":"); - b.append(port); - } + b.append("jdbc:postgresql://"); + b.append(hostname(optional(cfg, "hostname"))); + b.append(port(optional(cfg, "port"))); b.append("/"); b.append(required(cfg, "database")); url = b.toString(); + break; } - if (url == null || !url.startsWith(pfx)) { - throw new IllegalArgumentException("database.url must be " + pfx - + " and not " + url); - } - break; - } - case DEFAULT: - case JDBC: - default: - driver = required(cfg, "driver"); - url = required(cfg, "url"); - if (!url.startsWith("jdbc:")) { - throw new IllegalArgumentException("database.url must be jdbc: style"); + case MYSQL: { + final StringBuilder b = new StringBuilder(); + b.append("jdbc:mysql://"); + b.append(hostname(optional(cfg, "hostname"))); + b.append(port(optional(cfg, "port"))); + b.append("/"); + b.append(required(cfg, "database")); + url = b.toString(); + break; } - break; + + case JDBC: + driver = required(cfg, "driver"); + url = required(cfg, "url"); + break; + + default: + throw new IllegalArgumentException(type + " not supported"); + } + } + + if (driver == null || driver.isEmpty()) { + if (url.startsWith("jdbc:h2:")) { + driver = "org.h2.Driver"; + + } else if (url.startsWith("jdbc:postgresql:")) { + driver = "org.postgresql.Driver"; + + } else if (url.startsWith("jdbc:mysql:")) { + driver = "com.mysql.jdbc.Driver"; + + } else { + throw new IllegalArgumentException("database.driver must be set"); + } } boolean usePool; @@ -252,6 +208,23 @@ } } + private static String hostname(String hostname) { + if (hostname == null || hostname.isEmpty()) { + hostname = "localhost"; + + } else if (hostname.contains(":") && !hostname.startsWith("[")) { + hostname = "[" + hostname + "]"; + } + return hostname; + } + + private static String port(String port) { + if (port != null && !port.isEmpty()) { + return ":" + port; + } + return ""; + } + private static String optional(final Config config, final String name) { return config.getString("database", null, name); }