Incremental work in testing DatabaseUpdater for real and fixing the Database mess.

* Allow in-memory databases in H2Connector (set path to 'mem:<db-name>').
* Remove some redundant copy-pasted readFileAsString() methods (more pending).
* Remove unused executeSQL() method from Database and DatabaseConnector.
* Get started on better testing in DatabaseUpdaterTest.
* Improve DatabaseUpdater code.
* Improve documentation and @nullable / @nonnull annotations.
* Code quality.

Author: MarcFraile

src/main/java/cloudgene/mapred/database/util/AbstractDatabaseConnector.java

@@ -6,23 +6,18 @@
 
 public abstract class AbstractDatabaseConnector implements DatabaseConnector {
 
-	protected static final Logger log = LoggerFactory.getLogger(DatabaseUpdater.class);
+	protected static final Logger log = LoggerFactory.getLogger(AbstractDatabaseConnector.class);
 
 	private int maxActive = 10;
-
-	private int maxWait = 10000;
-
+	private int maxWait = 10_000;
 	private boolean defaultAutoCommit = true;
-
 	private boolean testWhileIdle = true;
-
-	private int minEvictableIdleTimeMillis = 1800000;
-
-	private int timeBetweenEvictionRunsMillis = 1800000;
+	private int minEvictableIdleTimeMillis = 1_800_000;
+	private int timeBetweenEvictionRunsMillis = 1_800_000;
 
 	protected BasicDataSource createDataSource() {
-
 		BasicDataSource dataSource = new BasicDataSource();
+
 		dataSource.setMaxActive(maxActive);
 		dataSource.setMaxWait(maxWait);
 		dataSource.setMaxIdle(maxActive);

src/main/java/cloudgene/mapred/database/util/Database.java

@@ -1,28 +1,29 @@
 package cloudgene.mapred.database.util;
 
-import java.io.IOException;
-import java.io.InputStream;
-import java.net.URISyntaxException;
 import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.List;
 
+import io.micronaut.core.annotation.NonNull;
+import io.micronaut.core.annotation.Nullable;
 import org.apache.commons.dbcp.BasicDataSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+// TODO(Marc): This class is a wrapper around DatabaseConnector and the only thing it
+//             adds is listener support, but we don't use these listeners anywhere.
+//             Remove.
 public class Database {
 
 	private static final Logger log = LoggerFactory.getLogger(Database.class);
 
-	private DatabaseConnector connector;
-
+	private @Nullable DatabaseConnector connector;
 	private final List<DatabaseListener> listeners = new ArrayList<>();
 
 	public Database() {
 	}
 
-	public void connect(DatabaseConnector connector) throws SQLException {
+	public void connect(@NonNull DatabaseConnector connector) throws SQLException {
 		this.connector = connector;
 		try {
 			connector.connect();
@@ -62,6 +63,7 @@ public boolean isConnected() {
 		}
 	}
 
+	// TODO(Marc): Connector is nullable, so this can throw a NullPointerException.
 	public BasicDataSource getDataSource() {
 		return connector.getDataSource();
 	}
@@ -82,10 +84,7 @@ private void fireChangeEvent(int event) {
 		}
 	}
 
-	public void executeSQL(InputStream is) throws SQLException, IOException, URISyntaxException {
-		connector.executeSQL(is);
-	}
-
+	@Nullable
 	public DatabaseConnector getConnector() {
 		return connector;
 	}

src/main/java/cloudgene/mapred/database/util/DatabaseConnector.java

@@ -1,24 +1,18 @@
 package cloudgene.mapred.database.util;
 
-import java.io.IOException;
-import java.io.InputStream;
-import java.net.URISyntaxException;
 import java.sql.SQLException;
 
 import org.apache.commons.dbcp.BasicDataSource;
 
 public interface DatabaseConnector {
 
-	public void connect() throws SQLException;
+	void connect() throws SQLException;
 
-	public void disconnect() throws SQLException;
+	void disconnect() throws SQLException;
 
-	public BasicDataSource getDataSource();
+	BasicDataSource getDataSource();
 
-	public void executeSQL(InputStream is)
-			throws SQLException, IOException, URISyntaxException;
-
-	public String getSchema();
+	String getSchema();
 
 	boolean tableExists(String table) throws SQLException;
 }

src/main/java/cloudgene/mapred/database/util/DatabaseConnectorFactory.java

@@ -4,10 +4,25 @@
 
 import cloudgene.mapred.database.util.h2.H2Connector;
 import cloudgene.mapred.database.util.mysql.MySqlConnector;
+import io.micronaut.core.annotation.NonNull;
+import io.micronaut.core.annotation.Nullable;
 
-public class DatabaseConnectorFactory {
+/**
+ * Provides {@link #createConnector(Map)} to configure and return a
+ * {@link DatabaseConnector}.
+ */
+public final class DatabaseConnectorFactory {
 
-	public static DatabaseConnector createConnector(Map<String, String> settings) {
+	private DatabaseConnectorFactory() {
+	}
+
+	/**
+	 * Reads {@code settings} to configure and return either an {@link H2Connector}
+	 * or a {@link MySqlConnector}. The {@code} driver field is used to determine
+	 * the output type (should be {@code h2} or {@code mysql}).
+	 */
+	@Nullable
+	public static DatabaseConnector createConnector(@NonNull Map<String, String> settings) {
 		String driver = settings.get("driver");
 
 		if (driver == null) {

src/main/java/cloudgene/mapred/database/util/DatabaseUpdater.java

@@ -1,44 +1,53 @@
 package cloudgene.mapred.database.util;
 
-import genepi.io.FileUtil;
-
 import java.io.BufferedReader;
-import java.io.DataInputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.net.URISyntaxException;
+import java.net.URL;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.util.HashMap;
 import java.util.Map;
 
+import io.micronaut.core.annotation.NonNull;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 public class DatabaseUpdater {
 
 	protected static final Logger log = LoggerFactory.getLogger(DatabaseUpdater.class);
 
-	private final DatabaseConnector connector;
-	private final Database database;
-	private final String oldVersion;
-	private final String currentVersion;
-	private final String filename;
-	private final InputStream updateFileAsStream;
+	private final @NonNull Database database;
+	private final @NonNull File versionFile;
+	private final @NonNull URL updatesFile;
+	private final @NonNull String currentVersion;
+
+	private final @NonNull DatabaseConnector connector;
+	private final @NonNull Map<String, IUpdateListener> listeners;
+
+	private final @NonNull String oldVersion;
 	private final boolean needUpdate;
-	private final Map<String, IUpdateListener> listeners;
 
-	public DatabaseUpdater(Database database, String filename, InputStream updateFileAsStream, String currentVersion) {
-		this.filename = filename;
+	public DatabaseUpdater(
+			@NonNull Database database,
+			@NonNull File versionFile,
+			@NonNull URL updatesFile,
+			@NonNull String currentVersion) {
+
 		this.database = database;
-		this.connector = database.getConnector();
-		this.updateFileAsStream = updateFileAsStream;
+		this.versionFile = versionFile;
+		this.updatesFile = updatesFile;
 		this.currentVersion = currentVersion;
+
+		// TODO(Marc): database.getConnector() is nullable, but here we assume connector
+		// is not null!
+		this.connector = database.getConnector();
 		this.listeners = new HashMap<>();
 
 		if (isVersionTableAvailable()) {
@@ -47,30 +56,38 @@ public DatabaseUpdater(Database database, String filename, InputStream updateFil
 
 			// Should not happen, since an entry is created when metadata table exists.
 			if (oldVersion == null) {
-				oldVersion = readVersion(filename);
+				oldVersion = readVersion();
 				log.info("Read current version from DB was not successful, read it from file: {}", oldVersion);
 			}
 
 			this.oldVersion = oldVersion;
 		} else {
 			// check also file for backwards compatibility
-			this.oldVersion = readVersion(filename);
+			this.oldVersion = readVersion();
 			log.info("Read current version from file: {}", oldVersion);
 		}
 
 		log.info("Current app version: {}", currentVersion);
 		needUpdate = (compareVersion(currentVersion, oldVersion) > 0);
 	}
 
-	public void addUpdate(String version, IUpdateListener listener) {
+	/**
+	 * Assigns {@code listener} as the one and only update listener for
+	 * {@code version}.
+	 * Replaces any existing listeners for the same version.
+	 */
+	public void addListener(String version, IUpdateListener listener) {
 		listeners.put(version, listener);
 	}
 
+	/**
+	 * If the database needs updating, updates it. Otherwise, inserts the current
+	 * version to the version table.
+	 */
 	public boolean updateDB() {
-
 		if (needUpdate()) {
 			log.info("Database needs update...");
-			if (!update()) {
+			if (!update()) { // TODO: update() ALWAYS returns true...
 				log.error("Updating database failed.");
 				try {
 					database.disconnect();
@@ -98,12 +115,13 @@ public boolean updateDB() {
 		return true;
 	}
 
-	public boolean update() {
+	// TODO(Marc): Only called from updateDB(). Consider merging.
+	private boolean update() {
 		if (needUpdate) {
 			log.info("Updating database from {} to {}...", oldVersion, currentVersion);
 
 			try {
-				readAndPrepareSqlClasspath(updateFileAsStream, oldVersion, currentVersion);
+				readAndPrepareSqlClasspath(oldVersion, currentVersion);
 			} catch (IOException | URISyntaxException | SQLException e) {
 				// TODO Auto-generated catch block
 				e.printStackTrace();
@@ -129,20 +147,27 @@ public boolean needUpdate() {
 		return needUpdate;
 	}
 
-	public void writeVersion(String newVersion) {
+	/**
+	 * Creates the database version table if not already present, and inserts
+	 * {@code version} as the latest version. If a version file exists, it is
+	 * deleted. Exceptions are ignored.
+	 *
+	 * @param version Newest application version (semver format expected).
+	 */
+	private void writeVersion(String version) {
 		try {
 			if (!isVersionTableAvailable()) {
 				createVersionTable();
 			}
 
 			Connection connection = connector.getDataSource().getConnection();
 			PreparedStatement ps = connection.prepareStatement("INSERT INTO database_versions (version) VALUES (?)");
-			ps.setString(1, newVersion);
+			ps.setString(1, version);
 			ps.executeUpdate();
-			log.info("Version in DB updated to: {}", newVersion);
+			log.info("Version in DB updated to: {}", version);
 
-			if (new File(filename).exists()) {
-				FileUtil.deleteFile(filename);
+			if (versionFile.exists()) {
+				versionFile.delete();
 				log.info("Deleted version.txt on file system.");
 			}
 
@@ -153,10 +178,13 @@ public void writeVersion(String newVersion) {
 		}
 	}
 
-	public String readVersion(String versionFile) {
-		File file = new File(versionFile);
-
-		if (file.exists()) {
+	/**
+	 * If {@code versionFile} exists, returns its contents (expects semver version).
+	 * Defaults to {@code 0.0.0}.
+	 */
+	@NonNull
+	private String readVersion() {
+		if (versionFile.exists()) {
 			try {
 				return readFileAsString(versionFile);
 			} catch (Exception e) {
@@ -167,7 +195,7 @@ public String readVersion(String versionFile) {
 		}
 	}
 
-	public String readVersionDB() {
+	private String readVersionDB() {
 		String sql = "SELECT version FROM database_versions "
 				+ "WHERE updated_on = (SELECT MAX(updated_on) FROM database_versions) "
 				+ "ORDER BY updated_on, id DESC";
@@ -193,27 +221,33 @@ public String readVersionDB() {
 		return version;
 	}
 
-	public static String readFileAsString(String filename) throws java.io.IOException, URISyntaxException {
-		InputStream is = new FileInputStream(filename);
+	// TODO(Marc): Why is this here???
+	private static String readFileAsString(File file) throws IOException {
+		InputStream is = new FileInputStream(file);
+		InputStreamReader sr = new InputStreamReader(is);
+		BufferedReader br = new BufferedReader(sr);
 
-		DataInputStream in = new DataInputStream(is);
-		BufferedReader br = new BufferedReader(new InputStreamReader(in));
 		String strLine;
 		StringBuilder builder = new StringBuilder();
 
 		while ((strLine = br.readLine()) != null) {
 			builder.append(strLine);
 		}
 
-		in.close();
+		br.close();
+		sr.close();
+		is.close();
+
 		return builder.toString();
 	}
 
-	public String readAndPrepareSqlClasspath(InputStream filestream, String minVersion, String maxVersion)
-			throws java.io.IOException, URISyntaxException, SQLException {
+	private String readAndPrepareSqlClasspath(String minVersion, String maxVersion)
+			throws IOException, URISyntaxException, SQLException {
+
+		InputStream is = updatesFile.openStream();
+		InputStreamReader sr = new InputStreamReader(is);
+		BufferedReader br = new BufferedReader(sr);
 
-		DataInputStream in = new DataInputStream(filestream);
-		BufferedReader br = new BufferedReader(new InputStreamReader(in));
 		String strLine;
 		StringBuilder builder = new StringBuilder();
 		boolean reading = false;
@@ -250,7 +284,10 @@ public String readAndPrepareSqlClasspath(InputStream filestream, String minVersi
 		// last block
 		executeSQLFile(builder.toString(), version);
 
-		in.close();
+		br.close();
+		sr.close();
+		is.close();
+
 		return builder.toString();
 	}