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>

Documentation/config-gerrit.txt

@@ -531,7 +531,7 @@ records about user accounts and change reviews.
 
 ----
 [database]
-  type = postgres
+  type = POSTGRESQL
   hostname = localhost
   database = reviewdb
   username = gerrit2
@@ -544,17 +544,21 @@ Type of database server to connect to.  If set this value will be
 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 @@ of the server named by database.type.
 
 [[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 @@ Password to authenticate to the database server with.
 
 [[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

gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitDatabase.java

@@ -82,7 +82,6 @@ class InitDatabase implements InitStep {
         break;
       }
 
-      case POSTGRES:
       case POSTGRESQL:
       case MYSQL: {
         userPassAuth = true;

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 final class DataSourceProvider implements Provider<DataSource>,
   }
 
   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 @@ public final class DataSourceProvider implements Provider<DataSource>,
     }
   }
 
+  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);
   }