From 81d820c0dc86c325c8569ebf0a73d051bb19df16 Mon Sep 17 00:00:00 2001 From: Ray Lee Date: Wed, 15 May 2024 23:59:50 -0400 Subject: [PATCH] DRYD-1422: Improve running of database upgrade scripts. - All upgrade scripts for a version of CSpace and the update of the database version number are now correctly running in one transaction. - DB upgrade scripts are now correctly undeployed. - DB upgrade scripts for versions of CSpace newer than the current version are now ignored. - Custom version number comparison code has been removed in favor of org.apache.maven.artifact.versioning.ComparableVersion. --- build.xml | 1 + services/common/pom.xml | 9 ++ .../services/common/ServiceMain.java | 125 ++++++------------ .../services/common/storage/JDBCTools.java | 60 +++++++-- 4 files changed, 99 insertions(+), 96 deletions(-) diff --git a/build.xml b/build.xml index c931af85b..20bbbc119 100644 --- a/build.xml +++ b/build.xml @@ -346,6 +346,7 @@ + diff --git a/services/common/pom.xml b/services/common/pom.xml index b72827120..c21843936 100644 --- a/services/common/pom.xml +++ b/services/common/pom.xml @@ -347,6 +347,10 @@ commons-fileupload 1.5 + + commons-io + commons-io + ch.elca.el4j.modules module-xml_merge-common @@ -412,6 +416,11 @@ freemarker 2.3.32 + + org.apache.maven + maven-artifact + 3.9.6 + diff --git a/services/common/src/main/java/org/collectionspace/services/common/ServiceMain.java b/services/common/src/main/java/org/collectionspace/services/common/ServiceMain.java index 807e4005d..fd0803b46 100644 --- a/services/common/src/main/java/org/collectionspace/services/common/ServiceMain.java +++ b/services/common/src/main/java/org/collectionspace/services/common/ServiceMain.java @@ -4,6 +4,7 @@ package org.collectionspace.services.common; import java.io.File; +import java.io.FileFilter; import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.FileReader; @@ -23,6 +24,7 @@ import javax.servlet.ServletContext; import javax.sql.DataSource; import org.apache.commons.io.FileUtils; +import org.apache.maven.artifact.versioning.ComparableVersion; import org.apache.tomcat.dbcp.dbcp2.BasicDataSource; import org.collectionspace.authentication.AuthN; @@ -468,7 +470,10 @@ public class ServiceMain { } private String applyRepositoryUpgradeScripts(Connection conn, String dataSourceName, String repositoryName, String fromVersion, String stage) throws Exception { - Map> upgradeScriptFiles = getRepositoryUpgradeScripts(dataSourceName, repositoryName, fromVersion, stage); + String currentVersion = getClass().getPackage().getImplementationVersion(); + String toVersion = currentVersion.replaceFirst("-SNAPSHOT$", ""); + + Map> upgradeScriptFiles = getRepositoryUpgradeScripts(dataSourceName, repositoryName, fromVersion, toVersion, stage); Set versions = upgradeScriptFiles.keySet(); String upgradedToVersion = null; @@ -479,13 +484,10 @@ public class ServiceMain { List scriptFiles = upgradeScriptFiles.get(version); - for (File file : scriptFiles) { - if (file.getName().endsWith(".sql")) { - logger.info(String.format("Running %s", file.getName())); + // getRepositoryUpgradeScripts ensures that version is a safe string. + String versionUpdateStatement = "UPDATE cspace.meta SET version = '" + version + "';"; - JDBCTools.runScript(conn, file); - } - } + JDBCTools.runScripts(conn, scriptFiles, versionUpdateStatement); upgradedToVersion = version; } @@ -511,29 +513,22 @@ public class ServiceMain { try { conn = JDBCTools.getConnection(dataSourceName, repositoryName, cspaceInstanceId); - conn.setAutoCommit(false); + JDBCTools.createCspaceMetaTableIfNotExists(conn); String version = JDBCTools.getRepositoryDatabaseVersion(conn); logger.info(String.format("%s repository current version is %s", repositoryName, version)); - String upgradedToVersion = applyRepositoryUpgradeScripts(conn, dataSourceName, repositoryName, version, stage); + String fromVersion = (version == null) ? "0" : version; + String upgradedToVersion = applyRepositoryUpgradeScripts(conn, dataSourceName, repositoryName, fromVersion, stage); if (upgradedToVersion != null) { logger.info(String.format("%s repository upgraded to version %s", repositoryName, upgradedToVersion)); - - JDBCTools.setRepositoryDatabaseVersion(conn, upgradedToVersion); } - - conn.commit(); } catch (Exception e) { logger.error(String.format("Could not upgrade %s repository", repositoryName)); logger.error(e.toString()); - - if (conn != null) { - conn.rollback(); - } } finally { if (conn != null) { @@ -568,7 +563,7 @@ public class ServiceMain { } } - public static Map> getRepositoryUpgradeScripts(String dataSourceName, String repositoryName, String fromVersion, String stage) throws Exception { + public static Map> getRepositoryUpgradeScripts(String dataSourceName, String repositoryName, String fromVersion, String toVersion, String stage) throws Exception { Map> upgradeScriptFiles = new LinkedHashMap<>(); Path upgradesPath = Paths.get( @@ -586,14 +581,19 @@ public class ServiceMain { File[] upgradesDirectoryFiles = upgradesDirectory.listFiles(); List versionDirectories = new ArrayList<>(); - VersionComparator versionComparator = new VersionComparator(); + ComparableVersion comparableFromVersion = new ComparableVersion(fromVersion); + ComparableVersion comparableToVersion = new ComparableVersion(toVersion); for (File file : upgradesDirectoryFiles) { + String filename = file.getName(); + ComparableVersion comparableFilenameVersion = new ComparableVersion(filename); + if ( file.isDirectory() && file.canRead() - && file.getName().matches("^\\d+\\.\\d+(\\.\\d+)?$") - && versionComparator.compare(fromVersion, file.getName()) < 0 + && filename.matches("^\\d+\\.\\d+(\\.\\d+)?$") + && comparableFromVersion.compareTo(comparableFilenameVersion) < 0 + && comparableToVersion.compareTo(comparableFilenameVersion) >= 0 ) { versionDirectories.add(file); } @@ -606,23 +606,21 @@ public class ServiceMain { File versionStageDirectory = versionStagePath.toFile(); if (versionStageDirectory.isDirectory()) { - File[] versionStageFiles = versionStageDirectory.listFiles(); - - Arrays.sort(versionStageFiles); - - List scriptFiles = new ArrayList<>(); - - for (File file : versionStageFiles) { - if ( - file.isFile() - && file.canRead() - ) { - scriptFiles.add(file); + File[] versionScriptFiles = versionStageDirectory.listFiles(new FileFilter() { + @Override + public boolean accept(File pathname) { + return ( + pathname.isFile() + && pathname.getName().endsWith(".sql") + && pathname.canRead() + ); } - } + }); + + Arrays.sort(versionScriptFiles); - if (scriptFiles.size() > 0) { - upgradeScriptFiles.put(versionDir.getName(), scriptFiles); + if (versionScriptFiles.length > 0) { + upgradeScriptFiles.put(versionDir.getName(), Arrays.asList(versionScriptFiles)); } } } @@ -630,60 +628,13 @@ public class ServiceMain { return upgradeScriptFiles; } - /** - * From https://dzone.com/articles/semantically-ordering-versioned-file-names-in-java - */ - public static class VersionComparator implements Comparator { - private static final Pattern NUMBERS = Pattern.compile("(?<=\\D)(?=\\d)|(?<=\\d)(?=\\D)"); - - @Override - public final int compare(String o1, String o2) { - // Optional "NULLS LAST" semantics: - if (o1 == null || o2 == null) { - return o1 == null ? o2 == null ? 0 : -1 : 1; - } - - // Splitting both input strings by the above patterns - String[] split1 = NUMBERS.split(o1); - String[] split2 = NUMBERS.split(o2); - int length = Math.min(split1.length, split2.length); - - // Looping over the individual segments - for (int i = 0; i < length; i++) { - char c1 = split1[i].charAt(0); - char c2 = split2[i].charAt(0); - int cmp = 0; - - // If both segments start with a digit, sort them - // numerically using BigInteger to stay safe - if (c1 >= '0' && c1 <= '9' && c2 >= 0 && c2 <= '9') - cmp = new BigInteger(split1[i]).compareTo( - new BigInteger(split2[i])); - - // If we haven't sorted numerically before, or if - // numeric sorting yielded equality (e.g 007 and 7) - // then sort lexicographically - if (cmp == 0) - cmp = split1[i].compareTo(split2[i]); - - // Abort once some prefix has unequal ordering - if (cmp != 0) - return cmp; - } - - // If we reach this, then both strings have equally - // ordered prefixes, but maybe one string is longer than - // the other (i.e. has more segments) - return split1.length - split2.length; - } - } - public static class VersionFileNameComparator implements Comparator { - private final VersionComparator versionComparator = new VersionComparator(); - @Override public final int compare(File o1, File o2) { - return versionComparator.compare(o1.getName(), o2.getName()); + ComparableVersion v1 = new ComparableVersion(o1.getName()); + ComparableVersion v2 = new ComparableVersion(o2.getName()); + + return v1.compareTo(v2); } } diff --git a/services/common/src/main/java/org/collectionspace/services/common/storage/JDBCTools.java b/services/common/src/main/java/org/collectionspace/services/common/storage/JDBCTools.java index 0c3e0adce..68b5a5441 100644 --- a/services/common/src/main/java/org/collectionspace/services/common/storage/JDBCTools.java +++ b/services/common/src/main/java/org/collectionspace/services/common/storage/JDBCTools.java @@ -30,8 +30,15 @@ import javax.sql.DataSource; import java.sql.DatabaseMetaData; import java.io.BufferedReader; import java.io.File; +import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.FileReader; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.io.SequenceInputStream; +import java.io.UnsupportedEncodingException; +import java.nio.charset.Charset; import java.sql.Connection; import java.sql.DriverManager; import java.sql.PreparedStatement; @@ -40,6 +47,7 @@ import java.sql.ResultSetMetaData; import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -47,6 +55,7 @@ import javax.sql.rowset.CachedRowSet; import javax.sql.rowset.RowSetFactory; import javax.sql.rowset.RowSetProvider; +import org.apache.commons.io.IOUtils; import org.apache.ibatis.jdbc.ScriptRunner; import org.apache.tomcat.dbcp.dbcp2.BasicDataSource; @@ -781,18 +790,18 @@ public class JDBCTools { return exists; } - public static void createCspaceMetaTable(Connection conn) throws SQLException { + public static void createCspaceMetaTableIfNotExists(Connection conn) throws SQLException { Statement stmt = conn.createStatement(); - stmt.executeUpdate("CREATE SCHEMA cspace"); - stmt.executeUpdate("CREATE TABLE cspace.meta (version varchar(32))"); - stmt.executeUpdate("INSERT INTO cspace.meta (version) values (null)"); + stmt.executeUpdate("CREATE SCHEMA IF NOT EXISTS cspace"); + stmt.executeUpdate("CREATE TABLE IF NOT EXISTS cspace.meta (version varchar(32))"); + stmt.executeUpdate("INSERT INTO cspace.meta (version) SELECT NULL WHERE NOT EXISTS (SELECT * FROM cspace.meta)"); stmt.close(); } public static String getRepositoryDatabaseVersion(Connection conn) throws SQLException { - String version = "0"; + String version = null; if (cspaceMetaTableExists(conn)) { Statement stmt = conn.createStatement(); @@ -810,9 +819,7 @@ public class JDBCTools { } public static void setRepositoryDatabaseVersion(Connection conn, String version) throws SQLException { - if (!cspaceMetaTableExists(conn)) { - createCspaceMetaTable(conn); - } + createCspaceMetaTableIfNotExists(conn); PreparedStatement stmt = conn.prepareStatement("UPDATE cspace.meta SET version = ?"); @@ -822,13 +829,48 @@ public class JDBCTools { stmt.close(); } + /** + * Run multiple SQL scripts followed by a given SQL statement, in a single transaction. + * + * @param conn The database connection. + * @param scriptFiles The SQL script files to run. + * @param afterStatement An SQL statement to execute after the statements in the script files have been executed. Can be null. + * @throws FileNotFoundException + * @throws UnsupportedEncodingException + */ + public static void runScripts(Connection conn, List scriptFiles, String afterStatement) throws FileNotFoundException, UnsupportedEncodingException { + List streams = new ArrayList<>(); + + for (File scriptFile : scriptFiles) { + streams.add(new FileInputStream(scriptFile)); + } + + if (afterStatement != null) { + streams.add(IOUtils.toInputStream(";" + afterStatement, Charset.forName("UTF-8"))); + } + + InputStream concatenatedStream = new SequenceInputStream(Collections.enumeration(streams)); + + runScript(conn, new BufferedReader(new InputStreamReader(concatenatedStream, Charset.forName("UTF-8")))); + } + + /** + * Run an SQL script. The statements in the given file are executed in a single transaction. + * @param conn The database connection. + * @param scriptFile The SQL script file to run. + * @throws FileNotFoundException + */ public static void runScript(Connection conn, File scriptFile) throws FileNotFoundException { + runScript(conn, new BufferedReader(new FileReader(scriptFile))); + } + + private static void runScript(Connection conn, Reader reader) throws FileNotFoundException { ScriptRunner scriptRunner = new ScriptRunner(conn); scriptRunner.setAutoCommit(false); scriptRunner.setStopOnError(true); scriptRunner.setSendFullScript(true); - scriptRunner.runScript(new BufferedReader(new FileReader(scriptFile))); + scriptRunner.runScript(reader); } } -- 2.47.3