Add Closeable/AutoCloseable on Database & interface (#2990)

* Add AutoCloseable on Database interface * Adapt close() implementation into AbstractJdbcDatabase * Handle the eventuality of no connection. * Use `try with resources` into unit test Co-authored-by: Bertrand Moreau <bertrand.moreau-prestataire@ca-cib.com> Co-authored-by: Nathan Voxland <nathan@voxland.net>
liquibase · Jul 20, 2022 · c72e682 · c72e682
1 parent 3d3f827
commit c72e682
Show file tree

Hide file tree

Showing 16 changed files with 215 additions and 191 deletions.
diff --git a/liquibase-core/src/main/java/liquibase/database/AbstractJdbcDatabase.java b/liquibase-core/src/main/java/liquibase/database/AbstractJdbcDatabase.java
@@ -1,6 +1,24 @@
 package liquibase.database;
 
+import static liquibase.util.StringUtil.join;
+import java.io.IOException;
+import java.io.Writer;
+import java.math.BigInteger;
+import java.sql.SQLException;
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.regex.Pattern;
 import liquibase.CatalogAndSchema;
+import liquibase.GlobalConfiguration;
 import liquibase.Scope;
 import liquibase.change.Change;
 import liquibase.change.core.DropTableChange;
@@ -9,7 +27,6 @@
 import liquibase.changelog.DatabaseChangeLog;
 import liquibase.changelog.RanChangeSet;
 import liquibase.changelog.StandardChangeLogHistoryService;
-import liquibase.GlobalConfiguration;
 import liquibase.configuration.ConfiguredValue;
 import liquibase.database.core.OracleDatabase;
 import liquibase.database.core.PostgresDatabase;
@@ -60,25 +77,6 @@
 import liquibase.util.StreamUtil;
 import liquibase.util.StringUtil;
 
-import java.io.IOException;
-import java.io.Writer;
-import java.math.BigInteger;
-import java.sql.SQLException;
-import java.text.ParseException;
-import java.text.SimpleDateFormat;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Date;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Locale;
-import java.util.Map;
-import java.util.Set;
-import java.util.regex.Pattern;
-
-import static liquibase.util.StringUtil.join;
-
 
 /**
  * AbstractJdbcDatabase is extended by all supported databases as a facade to the underlying database.
@@ -1224,20 +1222,15 @@ public int hashCode() {
 
     @Override
     public void close() throws DatabaseException {
-        Scope.getCurrentScope().getSingleton(ExecutorService.class).clearExecutor("jdbc", this);
-        DatabaseConnection connection = getConnection();
-        if (connection != null) {
-            if (previousAutoCommit != null) {
-                try {
-                    connection.setAutoCommit(previousAutoCommit);
-                } catch (DatabaseException e) {
-                    Scope.getCurrentScope().getLog(getClass()).warning("Failed to restore the auto commit to " + previousAutoCommit);
-
-                    throw e;
-                }
-            }
-            connection.close();
-        }
+      Scope.getCurrentScope().getSingleton(ExecutorService.class).clearExecutor("jdbc", this);
+      try (final DatabaseConnection connection = getConnection();) {
+        if (connection != null && previousAutoCommit != null) {
+          connection.setAutoCommit(previousAutoCommit);
+        }
+      } catch (final DatabaseException e) {
+        Scope.getCurrentScope().getLog(getClass()).warning("Failed to restore the auto commit to " + previousAutoCommit);
+        throw e;
+      }
     }
 
     @Override

diff --git a/liquibase-core/src/main/java/liquibase/database/Database.java b/liquibase-core/src/main/java/liquibase/database/Database.java
@@ -1,25 +1,28 @@
 package liquibase.database;
 
+import java.io.IOException;
+import java.io.Writer;
+import java.math.BigInteger;
+import java.util.Collection;
+import java.util.Date;
+import java.util.List;
+import java.util.Locale;
 import liquibase.CatalogAndSchema;
 import liquibase.change.Change;
 import liquibase.changelog.ChangeSet;
 import liquibase.changelog.DatabaseChangeLog;
 import liquibase.changelog.RanChangeSet;
-import liquibase.exception.*;
+import liquibase.exception.DatabaseException;
+import liquibase.exception.DatabaseHistoryException;
+import liquibase.exception.DateParseException;
+import liquibase.exception.LiquibaseException;
+import liquibase.exception.ValidationErrors;
 import liquibase.servicelocator.PrioritizedService;
 import liquibase.sql.visitor.SqlVisitor;
 import liquibase.statement.DatabaseFunction;
 import liquibase.statement.SqlStatement;
 import liquibase.structure.DatabaseObject;
 
-import java.io.IOException;
-import java.io.Writer;
-import java.math.BigInteger;
-import java.util.Collection;
-import java.util.Date;
-import java.util.List;
-import java.util.Locale;
-
 /**
  * Interface that every DBMS supported by this software must implement. Most methods belong into ont of these
  * categories:
@@ -29,7 +32,7 @@
  * <li>creating strings for use in SQL statements, e.g. literals for dates, time, numerals, etc.</li>
  * </ul>
  */
-public interface Database extends PrioritizedService {
+public interface Database extends PrioritizedService, AutoCloseable {
 
     String databaseChangeLogTableName = "DatabaseChangeLog".toUpperCase(Locale.US);
     String databaseChangeLogLockTableName = "DatabaseChangeLogLock".toUpperCase(Locale.US);
@@ -290,6 +293,7 @@ public interface Database extends PrioritizedService {
 
     String escapeStringForDatabase(String string);
 
+    @Override
     void close() throws DatabaseException;
 
     boolean supportsRestrictForeignKeys();

diff --git a/liquibase-core/src/main/java/liquibase/database/DatabaseConnection.java b/liquibase-core/src/main/java/liquibase/database/DatabaseConnection.java
@@ -1,18 +1,17 @@
 package liquibase.database;
 
-import liquibase.exception.DatabaseException;
-import liquibase.servicelocator.PrioritizedService;
-
 import java.sql.Driver;
 import java.util.Properties;
+import liquibase.exception.DatabaseException;
+import liquibase.servicelocator.PrioritizedService;
 
 /**
  * A liquibase abstraction over the normal Connection that is available in
  * java.sql. This interface allows wrappers and aspects over the basic 
  * connection.
  * 
  */
-public interface DatabaseConnection extends PrioritizedService {
+public interface DatabaseConnection extends PrioritizedService, AutoCloseable {
 
     void open(String url, Driver driverObject, Properties driverProperties)
             throws DatabaseException;
@@ -29,6 +28,7 @@ default boolean supports(String url) {
         return true;
     }
 
+    @Override
     void close() throws DatabaseException;
 
     void commit() throws DatabaseException;

diff --git a/liquibase-core/src/main/java/liquibase/database/OfflineConnection.java b/liquibase-core/src/main/java/liquibase/database/OfflineConnection.java
@@ -11,7 +11,6 @@
 import liquibase.parser.SnapshotParserFactory;
 import liquibase.resource.ResourceAccessor;
 import liquibase.servicelocator.LiquibaseService;
-import liquibase.servicelocator.PrioritizedService;
 import liquibase.snapshot.DatabaseSnapshot;
 import liquibase.snapshot.EmptyDatabaseSnapshot;
 import liquibase.snapshot.InvalidExampleException;

diff --git a/liquibase-core/src/main/java/liquibase/database/core/DerbyDatabase.java b/liquibase-core/src/main/java/liquibase/database/core/DerbyDatabase.java
@@ -59,7 +59,7 @@ public String getDefaultDriver(String url) {
                 } catch (ClassNotFoundException classNotFoundException) {
                     // Return class for newer versions anyway
                     return derbyNewDriverClassName;
-                }
+                } 
             }
         } else if (url.startsWith("jdbc:derby") || url.startsWith("java:derby")) {
             //Use EmbeddedDriver if using a derby URL but without the `://` in it
@@ -153,12 +153,15 @@ public String getViewDefinition(CatalogAndSchema schema, String name) throws Dat
 
     @Override
     public void close() throws DatabaseException {
+      // FIXME Seems not to be a good way to handle the possibility of getting `getConnection() == null`
+      if (getConnection() != null) {
         String url = getConnection().getURL();
         String driverName = getDefaultDriver(url);
         super.close();
-        if (getShutdownEmbeddedDerby() && (driverName != null) && driverName.toLowerCase().contains("embedded")) {
+        if (shutdownEmbeddedDerby && (driverName != null) && driverName.toLowerCase().contains("embedded")) {
             shutdownDerby(url, driverName);
         }
+      }
     }
 
     protected void shutdownDerby(String url, String driverName) throws DatabaseException {
@@ -194,7 +197,6 @@ protected void shutdownDerby(String url, String driverName) throws DatabaseExcep
     /**
      * Determine Apache Derby driver major/minor version.
      */
-    @SuppressWarnings({"static-access", "unchecked"})
     protected void determineDriverVersion() {
         try {
 // Locate the Derby sysinfo class and query its version info

diff --git a/liquibase-core/src/test/java/liquibase/database/AbstractJdbcDatabaseTest.java b/liquibase-core/src/test/java/liquibase/database/AbstractJdbcDatabaseTest.java
@@ -1,22 +1,21 @@
 package liquibase.database;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import java.util.ArrayList;
+import java.util.List;
+import org.junit.Test;
 import liquibase.Scope;
 import liquibase.change.core.CreateTableChange;
+import liquibase.exception.DatabaseException;
 import liquibase.executor.ExecutorService;
 import liquibase.sdk.executor.MockExecutor;
 import liquibase.sql.visitor.AppendSqlVisitor;
 import liquibase.sql.visitor.SqlVisitor;
 import liquibase.statement.SqlStatement;
 import liquibase.statement.core.DropTableStatement;
 import liquibase.structure.core.Table;
-import org.junit.Test;
-
-import java.util.ArrayList;
-import java.util.List;
-
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.assertFalse;
 
 /**
  * Base test class for database-specific tests
@@ -88,14 +87,14 @@ public void defaultsWorkWithoutAConnection() {
 //    }
 
     @Test
-    public void escapeTableName_noSchema() {
-        Database database = getDatabase();
+    public void escapeTableName_noSchema() throws DatabaseException {
+        final Database database = getDatabase();
         assertEquals("tableName", database.escapeTableName(null, null, "tableName"));
     }
 
     @Test
-    public void escapeTableName_withSchema() {
-        Database database = getDatabase();
+    public void escapeTableName_withSchema() throws DatabaseException {
+        final Database database = getDatabase();
         if (database.supportsCatalogInObjectName(Table.class)) {
             assertEquals("catalogName.schemaName.tableName", database.escapeTableName("catalogName", "schemaName", "tableName"));
         } else {
@@ -283,7 +282,7 @@ public void test_escapeObjectName() {
         assertTrue(tableName.matches("[\\[\\\"`]?My Table  [\\]\\\"`]?"));
 
         tableName = database.escapeObjectName("MyTable", Table.class);
-        assertTrue(tableName.equals("MyTable"));
+        assertEquals("MyTable", tableName);
 
         tableName = database.escapeObjectName("My Table", Table.class);
         assertTrue(tableName.matches("[\\[\\\"`]?My Table[\\]\\\"`]?"));

diff --git a/liquibase-core/src/test/java/liquibase/database/ConnectionServiceFactoryTest.java b/liquibase-core/src/test/java/liquibase/database/ConnectionServiceFactoryTest.java
@@ -1,13 +1,9 @@
 package liquibase.database;
 
-import liquibase.database.jvm.JdbcConnection;
+import static org.assertj.core.api.Assertions.assertThat;
 import org.junit.Before;
 import org.junit.Test;
-
-import java.sql.Driver;
-
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.mock;
+import liquibase.database.jvm.JdbcConnection;
 
 public class ConnectionServiceFactoryTest {
 

diff --git a/liquibase-core/src/test/java/liquibase/database/core/AbstractDb2DatabaseTest.java b/liquibase-core/src/test/java/liquibase/database/core/AbstractDb2DatabaseTest.java
@@ -1,15 +1,19 @@
 package liquibase.database.core;
 
 import junit.framework.TestCase;
-import liquibase.database.Database;
+import liquibase.exception.DatabaseException;
 
 public class AbstractDb2DatabaseTest extends TestCase {
-    public void testGetDateLiteral() {
-        AbstractDb2Database database = new DB2Database();
 
-        assertEquals("DATE('2018-12-31')", database.getDateLiteral("2018-12-31"));
-        assertEquals("TIME('23:58:59')", database.getDateLiteral("23:58:59"));
-        assertEquals("TIMESTAMP('2018-12-31 23:58:59')", database.getDateLiteral("2018-12-31 23:58:59"));
-        assertEquals("UNSUPPORTED:foo", database.getDateLiteral("foo"));
+    public void testGetDateLiteral() throws DatabaseException {
+        try (AbstractDb2Database database = new DB2Database()) {
+            assertEquals("DATE('2018-12-31')", database.getDateLiteral("2018-12-31"));
+            assertEquals("TIME('23:58:59')", database.getDateLiteral("23:58:59"));
+            assertEquals("TIMESTAMP('2018-12-31 23:58:59')", database.getDateLiteral("2018-12-31 23:58:59"));
+            assertEquals("UNSUPPORTED:foo", database.getDateLiteral("foo"));
+        } catch (final DatabaseException e) {
+            throw e;
+        }
     }
+
 }
diff --git a/liquibase-core/src/test/java/liquibase/database/core/DB2DatabaseTest.java b/liquibase-core/src/test/java/liquibase/database/core/DB2DatabaseTest.java
@@ -2,15 +2,18 @@
 
 import junit.framework.TestCase;
 import liquibase.database.Database;
+import liquibase.exception.DatabaseException;
 
 public class DB2DatabaseTest extends TestCase {
-    public void testGetDefaultDriver() {
-        Database database = new DB2Database();
 
-        assertEquals("com.ibm.db2.jcc.DB2Driver", database.getDefaultDriver("jdbc:db2://localhost:50000/liquibas"));
+    public void testGetDefaultDriver() throws DatabaseException {
+        try (Database database = new DB2Database()) {
+            assertEquals("com.ibm.db2.jcc.DB2Driver", database.getDefaultDriver("jdbc:db2://localhost:50000/liquibas"));
 
-        assertNull(database.getDefaultDriver("jdbc:oracle://localhost;databaseName=liquibase"));
+            assertNull(database.getDefaultDriver("jdbc:oracle://localhost;databaseName=liquibase"));
+        } catch (final DatabaseException e) {
+            throw e;
+        }
     }
 
-
 }
diff --git a/liquibase-core/src/test/java/liquibase/database/core/DB2zDatabaseTest.java b/liquibase-core/src/test/java/liquibase/database/core/DB2zDatabaseTest.java
@@ -2,15 +2,18 @@
 
 import junit.framework.TestCase;
 import liquibase.database.Database;
+import liquibase.exception.DatabaseException;
 
 public class DB2zDatabaseTest extends TestCase {
-    public void testGetDefaultDriver() {
-        Database database = new Db2zDatabase();
 
-        assertEquals("com.ibm.db2.jcc.DB2Driver", database.getDefaultDriver("jdbc:db2://localhost:50000/liquibas"));
+    public void testGetDefaultDriver() throws DatabaseException {
+        try (Database database = new Db2zDatabase()) {
+            assertEquals("com.ibm.db2.jcc.DB2Driver", database.getDefaultDriver("jdbc:db2://localhost:50000/liquibas"));
 
-        assertNull(database.getDefaultDriver("jdbc:oracle://localhost;databaseName=liquibase"));
+            assertNull(database.getDefaultDriver("jdbc:oracle://localhost;databaseName=liquibase"));
+        } catch (final DatabaseException e) {
+            throw e;
+        }
     }
 
-
 }
diff --git a/liquibase-core/src/test/java/liquibase/database/core/DerbyDatabaseTest.java b/liquibase-core/src/test/java/liquibase/database/core/DerbyDatabaseTest.java
@@ -1,7 +1,7 @@
 package liquibase.database.core;
 
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.RETURNS_SMART_NULLS;
-import static org.mockito.Mockito.anyString;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
@@ -19,12 +19,15 @@
 import liquibase.exception.DatabaseException;
 
 public class DerbyDatabaseTest extends TestCase {
-    public void testGetDefaultDriver() {
-        Database database = new DerbyDatabase();
 
-        assertEquals("org.apache.derby.jdbc.EmbeddedDriver", database.getDefaultDriver("java:derby:liquibase;create=true"));
+    public void testGetDefaultDriver() throws DatabaseException {
+        try (Database database = new DerbyDatabase()) {
+            assertEquals("org.apache.derby.jdbc.EmbeddedDriver", database.getDefaultDriver("java:derby:liquibase;create=true"));
 
-        assertNull(database.getDefaultDriver("jdbc:oracle://localhost;databaseName=liquibase"));
+            assertNull(database.getDefaultDriver("jdbc:oracle://localhost;databaseName=liquibase"));
+        } catch (final DatabaseException e) {
+            throw e;
+        }
     }
 
     public void testGetDateLiteral() {