METAMODEL-1093: Close compiled ResultSets
authorDennis Du Krøger <dennis.dukroger@humaninference.com>
Mon, 13 Jun 2016 11:07:11 +0000 (13:07 +0200)
committerDennis Du Krøger <dennis.dukroger@humaninference.com>
Mon, 13 Jun 2016 11:09:03 +0000 (13:09 +0200)
Fixes #107

jdbc/pom.xml
jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcCompiledQueryLeaseFactory.java
jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java
jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataSet.java
jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcMetadataLoader.java
jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcUpdateCallback.java
jdbc/src/test/java/org/apache/metamodel/jdbc/integrationtests/MysqlTest.java

index 7f7579f..fb332f7 100644 (file)
@@ -80,7 +80,7 @@
                <dependency>
                        <groupId>mysql</groupId>
                        <artifactId>mysql-connector-java</artifactId>
-                       <version>5.1.18</version>
+                       <version>5.1.39</version>
                        <scope>test</scope>
                </dependency>
                <dependency>
index d973d13..931c224 100644 (file)
@@ -23,6 +23,7 @@ import java.sql.PreparedStatement;
 import java.sql.SQLException;
 
 import org.apache.commons.pool.PoolableObjectFactory;
+import org.apache.metamodel.util.FileHelper;
 
 /**
  * Factory for the object pool of {@link JdbcCompiledQueryLease}s.
@@ -52,9 +53,9 @@ final class JdbcCompiledQueryLeaseFactory implements PoolableObjectFactory<JdbcC
 
     @Override
     public void destroyObject(JdbcCompiledQueryLease lease) throws Exception {
-        final PreparedStatement statement = lease.getStatement();
+        FileHelper.safeClose(lease.getStatement());
         final Connection connection = lease.getConnection();
-        _dataContext.close(connection, null, statement);
+        _dataContext.close(connection);
     }
 
     @Override
index 84254f9..63e42e5 100644 (file)
@@ -216,7 +216,7 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
         } catch (SQLException e) {
             logger.debug("Unexpected exception during JdbcDataContext initialization", e);
         } finally {
-            closeIfNescesary(con);
+            closeIfNecessary(con);
         }
         _databaseProductName = databaseProductName;
         logger.debug("Database product name: {}", _databaseProductName);
@@ -296,7 +296,7 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
         } catch (SQLException e) {
             throw JdbcUtils.wrapException(e, "retrieve schema and catalog metadata");
         } finally {
-            close(null, rs, null);
+            close(null);
         }
         return result;
     }
@@ -507,12 +507,12 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
         } catch (SQLException e) {
             // only close in case of an error - the JdbcDataSet will close
             // otherwise
-            close(connection, null, statement);
+            close(connection);
             throw JdbcUtils.wrapException(e, "execute query");
         } catch (RuntimeException e) {
             // only close in case of an error - the JdbcDataSet will close
             // otherwise
-            close(connection, null, statement);
+            close(connection);
             throw e;
         }
 
@@ -533,18 +533,24 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
     }
 
     /**
-     * Quietly closes any of the parameterized JDBC objects
-     * 
-     * @param connection
-     * 
-     * @param rs
-     * @param st
+     * Quietly closes the connection
+     *  @param connection The connection to close (if it makes sense, @see closeIfNecessary)
+
      */
+    public void close(Connection connection) {
+        closeIfNecessary(connection);
+    }
+
+    /**
+     * @deprecated Manually close {@link ResultSet} and {@link Statement} instead.
+     */
+    @Deprecated
     public void close(Connection connection, ResultSet rs, Statement st) {
-        closeIfNescesary(connection);
+        close(connection);
         FileHelper.safeClose(rs, st);
     }
 
+
     /**
      * Convenience method to get the available catalogNames using this
      * connection.
@@ -576,7 +582,7 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
         } catch (SQLException e) {
             logger.error("Error retrieving catalog metadata", e);
         } finally {
-            close(connection, rs, null);
+            close(connection);
             logger.debug("Retrieved {} catalogs", catalogs.size());
         }
         return catalogs.toArray(new String[catalogs.size()]);
@@ -600,7 +606,7 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
      * Gets an appropriate connection object to use - either a dedicated
      * connection or a new connection from the datasource object.
      * 
-     * Hint: Use the {@link #close(Connection, ResultSet, Statement)} method to
+     * Hint: Use the {@link #close(Connection)} method to
      * close the connection (and any ResultSet or Statements involved).
      */
     public Connection getConnection() {
@@ -614,7 +620,7 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
         }
     }
 
-    private void closeIfNescesary(Connection con) {
+    private void closeIfNecessary(Connection con) {
         if (con != null) {
             if (_dataSource != null) {
                 // closing connections after individual usage is only nescesary
@@ -687,7 +693,7 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
             } catch (SQLException e) {
                 throw JdbcUtils.wrapException(e, "determine default schema name");
             } finally {
-                closeIfNescesary(connection);
+                closeIfNecessary(connection);
             }
 
             // Fourth strategy: Find default schema name by vendor-specific
@@ -798,7 +804,7 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
         } catch (SQLException e) {
             throw JdbcUtils.wrapException(e, "get schema names");
         } finally {
-            closeIfNescesary(connection);
+            closeIfNecessary(connection);
         }
     }
 
@@ -809,7 +815,7 @@ public class JdbcDataContext extends AbstractDataContext implements UpdateableDa
         try {
             _metadataLoader.loadTables(schema, connection);
         } finally {
-            close(connection, null, null);
+            close(connection);
         }
         return schema;
     }
index a1b76f7..4142ab7 100644 (file)
@@ -197,8 +197,12 @@ final class JdbcDataSet extends AbstractDataSet {
         if (_closed) {
             return;
         }
+
+        FileHelper.safeClose(_resultSet);
+
         if (_jdbcDataContext != null) {
-            _jdbcDataContext.close(_connection, _resultSet, _statement);
+            FileHelper.safeClose(_statement);
+            _jdbcDataContext.close(_connection);
         }
         if (_compiledQuery != null) {
             _compiledQuery.returnLease(_lease);
index 5d5697e..837fb18 100644 (file)
@@ -22,7 +22,6 @@ import java.sql.Connection;
 import java.sql.DatabaseMetaData;
 import java.sql.ResultSet;
 import java.sql.SQLException;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.Set;
 import java.util.StringTokenizer;
@@ -75,7 +74,7 @@ final class JdbcMetadataLoader implements MetadataLoader {
         try {
             loadTables(schema, connection);
         } finally {
-            _dataContext.close(connection, null, null);
+            _dataContext.close(connection);
         }
     }
 
@@ -92,20 +91,27 @@ final class JdbcMetadataLoader implements MetadataLoader {
         }
     }
 
+    private String getJdbcSchemaName(Schema schema) {
+        if(_usesCatalogsAsSchemas) {
+            return null;
+        } else {
+            return schema.getName();
+        }
+    }
+
+    private String getCatalogName(Schema schema) {
+        if(_usesCatalogsAsSchemas) {
+            return schema.getName();
+        } else {
+            return _dataContext.getCatalogName();
+        }
+    }
+
     private void loadTables(JdbcSchema schema, DatabaseMetaData metaData, String[] types) {
-        String catalogName = _dataContext.getCatalogName();
+        try (ResultSet rs = metaData.getTables(getCatalogName(schema), getJdbcSchemaName(schema), null, types)) {
+            logger.debug("Querying for table types {}, in catalog: {}, schema: {}", types,
+                    _dataContext.getCatalogName(), schema.getName());
 
-        ResultSet rs = null;
-        try {
-            if (logger.isDebugEnabled()) {
-                logger.debug("Querying for table types " + Arrays.toString(types) + " in catalog: " + catalogName
-                        + ", schema: " + schema.getName());
-            }
-            if (_usesCatalogsAsSchemas) {
-                rs = metaData.getTables(schema.getName(), null, null, types);
-            } else {
-                rs = metaData.getTables(catalogName, schema.getName(), null, types);
-            }
             schema.clearTables();
             int tableNumber = -1;
             while (rs.next()) {
@@ -122,10 +128,6 @@ final class JdbcMetadataLoader implements MetadataLoader {
                             + ",tableName=" + tableName);
                 }
 
-                if (tableSchema == null) {
-                    tableSchema = tableCatalog;
-                }
-
                 JdbcTable table = new JdbcTable(tableName, tableType, schema, this);
                 table.setRemarks(tableRemarks);
                 table.setQuote(_identifierQuoteString);
@@ -142,8 +144,6 @@ final class JdbcMetadataLoader implements MetadataLoader {
 
         } catch (SQLException e) {
             throw JdbcUtils.wrapException(e, "retrieve table metadata for " + schema.getName());
-        } finally {
-            _dataContext.close(null, rs, null);
         }
     }
     
@@ -158,7 +158,7 @@ final class JdbcMetadataLoader implements MetadataLoader {
         try {
             loadIndexes(jdbcTable, connection);
         } finally {
-            _dataContext.close(connection, null, null);
+            _dataContext.close(connection);
         }
     }
 
@@ -194,7 +194,7 @@ final class JdbcMetadataLoader implements MetadataLoader {
         try {
             loadPrimaryKeys(jdbcTable, connection);
         } finally {
-            _dataContext.close(connection, null, null);
+            _dataContext.close(connection);
         }
     }
 
@@ -220,14 +220,7 @@ final class JdbcMetadataLoader implements MetadataLoader {
 
     private void loadPrimaryKeys(JdbcTable table, DatabaseMetaData metaData) throws MetaModelException {
         Schema schema = table.getSchema();
-        ResultSet rs = null;
-
-        try {
-            if (_usesCatalogsAsSchemas) {
-                rs = metaData.getPrimaryKeys(schema.getName(), null, table.getName());
-            } else {
-                rs = metaData.getPrimaryKeys(_dataContext.getCatalogName(), schema.getName(), table.getName());
-            }
+        try (ResultSet rs = metaData.getPrimaryKeys(getCatalogName(schema), getJdbcSchemaName(schema), table.getName());){
             while (rs.next()) {
                 String columnName = rs.getString(4);
                 if (columnName != null) {
@@ -241,23 +234,15 @@ final class JdbcMetadataLoader implements MetadataLoader {
             }
         } catch (SQLException e) {
             throw JdbcUtils.wrapException(e, "retrieve primary keys for " + table.getName());
-        } finally {
-            _dataContext.close(null, rs, null);
         }
     }
 
     private void loadIndexes(Table table, DatabaseMetaData metaData) throws MetaModelException {
         Schema schema = table.getSchema();
-        ResultSet rs = null;
+
         // Ticket #170: IndexInfo is nice-to-have, not need-to-have, so
         // we will do a nice failover on SQLExceptions
-        try {
-            if (_usesCatalogsAsSchemas) {
-                rs = metaData.getIndexInfo(schema.getName(), null, table.getName(), false, true);
-            } else {
-                rs = metaData.getIndexInfo(_dataContext.getCatalogName(), schema.getName(), table.getName(), false,
-                        true);
-            }
+        try (ResultSet rs = metaData.getIndexInfo(getCatalogName(schema), getJdbcSchemaName(schema), table.getName(), false, true)) {
             while (rs.next()) {
                 String columnName = rs.getString(9);
                 if (columnName != null) {
@@ -271,8 +256,6 @@ final class JdbcMetadataLoader implements MetadataLoader {
             }
         } catch (SQLException e) {
             throw JdbcUtils.wrapException(e, "retrieve index information for " + table.getName());
-        } finally {
-            _dataContext.close(null, rs, null);
         }
     }
     
@@ -287,7 +270,7 @@ final class JdbcMetadataLoader implements MetadataLoader {
         try {
             loadColumns(jdbcTable, connection);
         } finally {
-            _dataContext.close(connection, null, null);
+            _dataContext.close(connection);
         }
     }
 
@@ -325,17 +308,13 @@ final class JdbcMetadataLoader implements MetadataLoader {
     private void loadColumns(JdbcTable table, DatabaseMetaData metaData) {
         final boolean convertLobs = isLobConversionEnabled();
         final Schema schema = table.getSchema();
-        ResultSet rs = null;
-        try {
+
+        try (ResultSet rs = metaData.getColumns(getCatalogName(schema), getJdbcSchemaName(schema), table.getName(), null)) {
             if (logger.isDebugEnabled()) {
                 logger.debug("Querying for columns in table: " + table.getName());
             }
             int columnNumber = -1;
-            if (_usesCatalogsAsSchemas) {
-                rs = metaData.getColumns(schema.getName(), null, table.getName(), null);
-            } else {
-                rs = metaData.getColumns(_dataContext.getCatalogName(), schema.getName(), table.getName(), null);
-            }
+
             while (rs.next()) {
                 columnNumber++;
                 final String columnName = rs.getString(4);
@@ -386,14 +365,11 @@ final class JdbcMetadataLoader implements MetadataLoader {
                 logger.info("No column metadata records returned for table '{}' in schema '{}'", table.getName(),
                         schema.getName());
             } else {
-                logger.debug("Returned {} column metadata records for table '{}' in schema '{}'", new Object[] {
-                        columnsReturned, table.getName(), schema.getName() });
+                logger.debug("Returned {} column metadata records for table '{}' in schema '{}'", columnsReturned,
+                        table.getName(), schema.getName());
             }
-
         } catch (SQLException e) {
             throw JdbcUtils.wrapException(e, "retrieve table metadata for " + table.getName());
-        } finally {
-            _dataContext.close(null, rs, null);
         }
     }
     
@@ -407,7 +383,7 @@ final class JdbcMetadataLoader implements MetadataLoader {
         try {
             loadRelations(jdbcSchema, connection);
         } finally {
-            _dataContext.close(connection, null, null);
+            _dataContext.close(connection);
         }
     }
 
@@ -436,18 +412,10 @@ final class JdbcMetadataLoader implements MetadataLoader {
 
     private void loadRelations(Table table, DatabaseMetaData metaData) {
         Schema schema = table.getSchema();
-        ResultSet rs = null;
-        try {
-            if (_usesCatalogsAsSchemas) {
-                rs = metaData.getImportedKeys(schema.getName(), null, table.getName());
-            } else {
-                rs = metaData.getImportedKeys(_dataContext.getCatalogName(), schema.getName(), table.getName());
-            }
+        try (ResultSet rs = metaData.getImportedKeys(getCatalogName(schema), getJdbcSchemaName(schema), table.getName())) {
             loadRelations(rs, schema);
         } catch (SQLException e) {
             throw JdbcUtils.wrapException(e, "retrieve imported keys for " + table.getName());
-        } finally {
-            _dataContext.close(null, rs, null);
         }
     }
 
@@ -481,7 +449,7 @@ final class JdbcMetadataLoader implements MetadataLoader {
             if (pkColumn == null || fkColumn == null) {
                 logger.error(
                         "Could not find relation columns: pkTableName={},pkColumnName={},fkTableName={},fkColumnName={}",
-                        new Object[] { pkTableName, pkColumnName, fkTableName, fkColumnName });
+                        pkTableName, pkColumnName, fkTableName, fkColumnName);
                 logger.error("pkColumn={}", pkColumn);
                 logger.error("fkColumn={}", fkColumn);
             } else {
index 5e60826..a9db2fd 100644 (file)
@@ -92,7 +92,7 @@ abstract class JdbcUpdateCallback extends AbstractUpdateCallback implements Upda
                         }
                     }
                 } finally {
-                    getDataContext().close(_connection, null, null);
+                    getDataContext().close(_connection);
                 }
             }
         }
index ba273e3..c5485ca 100644 (file)
@@ -22,6 +22,7 @@ import java.sql.Connection;
 import java.sql.DatabaseMetaData;
 import java.sql.SQLException;
 import java.sql.Statement;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
@@ -33,14 +34,17 @@ import org.apache.metamodel.UpdateCallback;
 import org.apache.metamodel.UpdateScript;
 import org.apache.metamodel.data.DataSet;
 import org.apache.metamodel.data.DataSetTableModel;
+import org.apache.metamodel.data.Row;
 import org.apache.metamodel.jdbc.JdbcDataContext;
 import org.apache.metamodel.jdbc.JdbcTestTemplates;
 import org.apache.metamodel.jdbc.QuerySplitter;
 import org.apache.metamodel.jdbc.dialects.MysqlQueryRewriter;
+import org.apache.metamodel.query.CompiledQuery;
 import org.apache.metamodel.query.FilterItem;
 import org.apache.metamodel.query.FromItem;
 import org.apache.metamodel.query.OperatorType;
 import org.apache.metamodel.query.Query;
+import org.apache.metamodel.query.QueryParameter;
 import org.apache.metamodel.schema.Column;
 import org.apache.metamodel.schema.ColumnType;
 import org.apache.metamodel.schema.Schema;
@@ -50,8 +54,8 @@ import org.apache.metamodel.schema.TableType;
 /**
  * Test case that tests mysql interaction. The test requires the "sakila" sample
  * database that can be found at dev.mysql.com.
- * 
- * @see http://dev.mysql.com/doc/sakila/en/sakila-installation.html
+ *
+ * @see <a href="http://dev.mysql.com/doc/sakila/en/sakila-installation.html">Sakila installation</a>
  */
 public class MysqlTest extends AbstractJdbIntegrationTest {
 
@@ -75,12 +79,12 @@ public class MysqlTest extends AbstractJdbIntegrationTest {
 
         JdbcTestTemplates.compositeKeyCreation(getDataContext(), "metamodel_test_composite_keys");
     }
-    
+
     public void testTimestampValueInsertSelect() throws Exception {
         if (!isConfigured()) {
             return;
         }
-        
+
         final Connection connection = getConnection();
         JdbcTestTemplates.timestampValueInsertSelect(connection, TimeUnit.MICROSECONDS, "TIMESTAMP(6)");
     }
@@ -180,11 +184,10 @@ public class MysqlTest extends AbstractJdbIntegrationTest {
         }
 
         DataContext dc = new JdbcDataContext(getConnection(), TableType.DEFAULT_TABLE_TYPES, "sakila");
-        Schema[] schemas = dc.getSchemas();
-        assertEquals("[Schema[name=mysql], Schema[name=performance_schema], Schema[name=portal], "
-                + "Schema[name=sakila], Schema[name=world]]", Arrays.toString(schemas));
+        final Schema sakila = dc.getSchemaByName("sakila");
+        assertNotNull(sakila);
 
-        Table table = dc.getSchemaByName("sakila").getTableByName("film");
+        Table table = sakila.getTableByName("film");
         Query q = new Query().from(table).select(table.getColumns());
         DataSet data = dc.executeQuery(q);
         TableModel tableModel = new DataSetTableModel(data);
@@ -199,9 +202,9 @@ public class MysqlTest extends AbstractJdbIntegrationTest {
 
         JdbcDataContext dataContext = new JdbcDataContext(getConnection());
         assertTrue(dataContext.getQueryRewriter() instanceof MysqlQueryRewriter);
-        String[] catalogNames = dataContext.getCatalogNames();
-        assertEquals("[information_schema, mysql, performance_schema, portal, sakila, world]",
-                Arrays.toString(catalogNames));
+        assertNotNull(dataContext.getSchemaByName("mysql"));
+        assertNotNull(dataContext.getSchemaByName("performance_schema"));
+        assertNotNull(dataContext.getSchemaByName("sakila"));
     }
 
     public void testGetDefaultSchema() throws Exception {
@@ -296,8 +299,6 @@ public class MysqlTest extends AbstractJdbIntegrationTest {
         }
 
         DataContext dc = new JdbcDataContext(getConnection());
-        Schema[] schemas = dc.getSchemas();
-        assertEquals(5, schemas.length);
         Schema schema = dc.getDefaultSchema();
 
         assertEquals("[Table[name=actor,type=TABLE,remarks=], " + "Table[name=address,type=TABLE,remarks=], "
@@ -335,9 +336,6 @@ public class MysqlTest extends AbstractJdbIntegrationTest {
                 "[Relationship[primaryTable=language,primaryColumns=[language_id],foreignTable=film,foreignColumns=[language_id]], Relationship[primaryTable=language,primaryColumns=[language_id],foreignTable=film,foreignColumns=[original_language_id]], Relationship[primaryTable=film,primaryColumns=[film_id],foreignTable=film_actor,foreignColumns=[film_id]], Relationship[primaryTable=film,primaryColumns=[film_id],foreignTable=film_category,foreignColumns=[film_id]], Relationship[primaryTable=film,primaryColumns=[film_id],foreignTable=inventory,foreignColumns=[film_id]]]",
                 Arrays.toString(filmTable.getRelationships()));
 
-        dc = new JdbcDataContext(getConnection(), TableType.DEFAULT_TABLE_TYPES, "sakila");
-        schemas = dc.getSchemas();
-        assertEquals(6, schemas.length);
         assertEquals("[Table[name=actor,type=TABLE,remarks=], " + "Table[name=address,type=TABLE,remarks=], "
                 + "Table[name=category,type=TABLE,remarks=], " + "Table[name=city,type=TABLE,remarks=], "
                 + "Table[name=country,type=TABLE,remarks=], " + "Table[name=customer,type=TABLE,remarks=], "
@@ -415,6 +413,37 @@ public class MysqlTest extends AbstractJdbIntegrationTest {
         assertFalse(ds.next());
     }
 
+    public void testCompiledQueries() throws Exception {
+        if (!isConfigured()) {
+            return;
+        }
+
+        DataContext dc = new JdbcDataContext(getConnection(), TableType.DEFAULT_TABLE_TYPES, "sakila");
+
+        final Query cityQuery = dc.query().from("city").select("city_id", "city").toQuery()
+                .where(dc.getColumnByQualifiedLabel("city.city_id"), OperatorType.EQUALS_TO, new QueryParameter());
+        final CompiledQuery userCompiledQuery = dc.compileQuery(cityQuery);
+
+        for (int i = 1; i <= 100; i++) {
+            System.out.println("Running test " + i);
+            List<Integer> cityIds = new ArrayList<>(1000);
+            try (final DataSet addresses = dc.query().from("address").select("city_id").execute()) {
+                while (addresses.next()) {
+                    final Row addressRow = addresses.getRow();
+                    cityIds.add((int) addressRow.getValue(0));
+                }
+            }
+
+            for(int value : cityIds) {
+                try (final DataSet users = dc.executeQuery(userCompiledQuery, value)) {
+                    while (users.next()) {
+                        assertEquals(value, users.getRow().getValue(0));
+                    }
+                }
+            }
+        }
+    }
+
     public void testWhiteSpaceColumns() throws Exception {
         if (!isConfigured()) {
             return;