Index: jackrabbit-core/src/test/java/org/apache/jackrabbit/core/util/db/CheckSchemaOperationTest.java =================================================================== --- jackrabbit-core/src/test/java/org/apache/jackrabbit/core/util/db/CheckSchemaOperationTest.java (revision 0) +++ jackrabbit-core/src/test/java/org/apache/jackrabbit/core/util/db/CheckSchemaOperationTest.java (revision 0) @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.core.util.db; + +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.verify; + +import java.io.ByteArrayInputStream; +import java.sql.SQLException; + +import junit.framework.TestCase; + +public class CheckSchemaOperationTest extends TestCase { + + private ConnectionHelper mock; + + private final SQLException ex1 = new SQLException("ex1"); + + private final SQLException ex2 = new SQLException("ex2"); + + @Override + public void setUp() { + mock = createMock(ConnectionHelper.class); + } + + public void testSkipSchemaCreation() throws Exception { + // Arrange + expect(mock.tableExists("table name")).andReturn(true); + replay(mock); + // Act + new CheckSchemaOperation(mock, new ByteArrayInputStream("test dll".getBytes()), "table name").run(); + // Assert + verify(mock); + } + + public void testSchemaCreation() throws Exception { + // Arrange + expect(mock.tableExists("table name")).andReturn(false); + mock.startBatch(); + mock.exec("test ddl"); + mock.exec("test ddl2"); + mock.endBatch(true); + replay(mock); + // Act + new CheckSchemaOperation(mock, new ByteArrayInputStream("# comment\n \ntest ddl\ntest ddl2" + .getBytes()), "table name").run(); + // Assert + verify(mock); + } + + public void testVariableReplacement() throws Exception { + // Arrange + expect(mock.tableExists("table name")).andReturn(false); + mock.startBatch(); + mock.exec("test ddl"); + mock.endBatch(true); + replay(mock); + // Act + new CheckSchemaOperation(mock, new ByteArrayInputStream("test ${var}".getBytes()), "table name") + .addVariableReplacement("${var}", "ddl").run(); + // Assert + verify(mock); + } + + public void testExceptionDuringExec() throws Exception { + // Arrange + expect(mock.tableExists("table name")).andReturn(false); + mock.startBatch(); + mock.exec("test ddl"); + expectLastCall().andThrow(ex1); + mock.endBatch(false); + replay(mock); + // Act + try { + new CheckSchemaOperation(mock, new ByteArrayInputStream("test ddl".getBytes()), "table name") + .run(); + } catch (SQLException e) { + assertEquals(ex1, e); + } + // Assert + verify(mock); + } + + public void testExceptionDuringCommit() throws Exception { + // Arrange + expect(mock.tableExists("table name")).andReturn(false); + mock.startBatch(); + mock.exec("test ddl"); + mock.endBatch(true); + expectLastCall().andThrow(ex1); + replay(mock); + // Act + try { + new CheckSchemaOperation(mock, new ByteArrayInputStream("test ddl".getBytes()), "table name") + .run(); + } catch (SQLException e) { + assertEquals(ex1, e); + } + // Assert + verify(mock); + } + + public void testExceptionDuringExecAndCommit() throws Exception { + // Arrange + expect(mock.tableExists("table name")).andReturn(false); + mock.startBatch(); + mock.exec("test ddl"); + expectLastCall().andThrow(ex1); + mock.endBatch(false); + expectLastCall().andThrow(ex2); + replay(mock); + // Act + try { + new CheckSchemaOperation(mock, new ByteArrayInputStream("test ddl".getBytes()), "table name") + .run(); + } catch (SQLException e) { + assertEquals(ex1, e); + } + // Assert + verify(mock); + } +} Property changes on: jackrabbit-core/src/test/java/org/apache/jackrabbit/core/util/db/CheckSchemaOperationTest.java ___________________________________________________________________ Added: svn:eol-style + native Index: jackrabbit-core/src/test/java/org/apache/jackrabbit/core/util/db/TestAll.java =================================================================== --- jackrabbit-core/src/test/java/org/apache/jackrabbit/core/util/db/TestAll.java (revision 908870) +++ jackrabbit-core/src/test/java/org/apache/jackrabbit/core/util/db/TestAll.java (working copy) @@ -33,6 +33,7 @@ public static Test suite() { TestSuite suite = new TestSuite("Database utility tests"); suite.addTestSuite(ConnectionFactoryTest.class); + suite.addTestSuite(CheckSchemaOperationTest.class); return suite; } } Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/pool/BundleDbPersistenceManager.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/pool/BundleDbPersistenceManager.java (revision 908870) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/pool/BundleDbPersistenceManager.java (working copy) @@ -55,6 +55,7 @@ import org.apache.jackrabbit.core.util.db.CheckSchemaOperation; import org.apache.jackrabbit.core.util.db.ConnectionFactory; import org.apache.jackrabbit.core.util.db.ConnectionHelper; +import org.apache.jackrabbit.core.util.db.ConnectionHelperImpl; import org.apache.jackrabbit.core.util.db.DatabaseAware; import org.apache.jackrabbit.core.util.db.DbUtility; import org.apache.jackrabbit.core.util.db.StreamWrapper; @@ -569,7 +570,7 @@ * @throws Exception on error */ protected ConnectionHelper createConnectionHelper(DataSource dataSrc) throws Exception { - return new ConnectionHelper(dataSrc, blockOnConnectionLoss); + return new ConnectionHelperImpl(dataSrc, blockOnConnectionLoss); } /** Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/fs/db/DatabaseFileSystem.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/fs/db/DatabaseFileSystem.java (revision 908870) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/fs/db/DatabaseFileSystem.java (working copy) @@ -24,6 +24,7 @@ import org.apache.jackrabbit.core.persistence.PMContext; import org.apache.jackrabbit.core.util.db.CheckSchemaOperation; import org.apache.jackrabbit.core.util.db.ConnectionHelper; +import org.apache.jackrabbit.core.util.db.ConnectionHelperImpl; import org.apache.jackrabbit.core.util.db.DbUtility; import org.apache.jackrabbit.core.util.db.StreamWrapper; import org.apache.jackrabbit.util.TransientFileFactory; @@ -227,7 +228,7 @@ * @throws Exception on error */ protected ConnectionHelper createConnectionHelper(DataSource dataSrc) throws Exception { - return new ConnectionHelper(dataSrc, false); + return new ConnectionHelperImpl(dataSrc, false); } /** Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/db/DbDataStore.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/db/DbDataStore.java (revision 908870) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/db/DbDataStore.java (working copy) @@ -24,6 +24,7 @@ import org.apache.jackrabbit.core.util.db.CheckSchemaOperation; import org.apache.jackrabbit.core.util.db.ConnectionFactory; import org.apache.jackrabbit.core.util.db.ConnectionHelper; +import org.apache.jackrabbit.core.util.db.ConnectionHelperImpl; import org.apache.jackrabbit.core.util.db.DatabaseAware; import org.apache.jackrabbit.core.util.db.DbUtility; import org.apache.jackrabbit.core.util.db.StreamWrapper; @@ -597,7 +598,7 @@ * @throws Exception on error */ protected ConnectionHelper createConnectionHelper(DataSource dataSrc) throws Exception { - return new ConnectionHelper(dataSrc, false); + return new ConnectionHelperImpl(dataSrc, false); } /** Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/journal/DatabaseJournal.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/journal/DatabaseJournal.java (revision 908870) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/journal/DatabaseJournal.java (working copy) @@ -20,6 +20,7 @@ import org.apache.jackrabbit.core.util.db.CheckSchemaOperation; import org.apache.jackrabbit.core.util.db.ConnectionFactory; import org.apache.jackrabbit.core.util.db.ConnectionHelper; +import org.apache.jackrabbit.core.util.db.ConnectionHelperImpl; import org.apache.jackrabbit.core.util.db.DatabaseAware; import org.apache.jackrabbit.core.util.db.DbUtility; import org.apache.jackrabbit.core.util.db.StreamWrapper; @@ -297,7 +298,7 @@ * @throws Exception on error */ protected ConnectionHelper createConnectionHelper(DataSource dataSrc) throws Exception { - return new ConnectionHelper(dataSrc, false); + return new ConnectionHelperImpl(dataSrc, false); } /** Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/CheckSchemaOperation.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/CheckSchemaOperation.java (revision 908870) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/CheckSchemaOperation.java (working copy) @@ -21,11 +21,15 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.sql.SQLException; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.apache.commons.io.IOUtils; import org.apache.jackrabbit.util.Text; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * An operation which synchronously checks the DB schema in the {@link #run()} method. The @@ -33,6 +37,8 @@ */ public class CheckSchemaOperation { + private static Logger log = LoggerFactory.getLogger(CheckSchemaOperation.class); + public static final String SCHEMA_OBJECT_PREFIX_VARIABLE = "${schemaObjectPrefix}"; public static final String TABLE_SPACE_VARIABLE = "${tableSpace}"; @@ -70,34 +76,87 @@ } /** - * Checks if the required schema objects exist and creates them if they don't exist yet. + * Checks if the required schema objects exist and creates them if they don't exist yet. The creation of + * database objects is wrapped in a JDBC transaction to prevent partially created schemas (JCR-936). Note + * that this does not work on all databases and partially created schemas are very annoying. Therefore DDL + * statements to execute and the executed DDL statements are logged on INFO level. * - * @throws SQLException if an error occurs - * @throws IOException if an error occurs + * @throws IOException if the DDL could not be read + * @throws SQLException if an error occurs during execution of the DDL statements */ public void run() throws SQLException, IOException { if (!conHelper.tableExists(table)) { - BufferedReader reader = new BufferedReader(new InputStreamReader(ddl)); + List sqlList = readDDLStatements(new BufferedReader(new InputStreamReader(ddl))); try { - String sql = reader.readLine(); - while (sql != null) { - // Skip comments and empty lines - if (!sql.startsWith("#") && sql.length() > 0) { - // replace prefix variable - sql = replace(sql); - // execute sql stmt - conHelper.exec(sql); - } - // read next sql stmt - sql = reader.readLine(); + createSchema(sqlList); + } catch (SQLException e) { + log.error("Failed to create schema. Please make sure that " + + "either all or none of the required database objects exist."); + throw e; + } + } + } + + /** + * Constructs a list of DDL statements from the input stream given on construction and + * closes the stream. + * + * @param reader the reader that wraps the DDL input stream + * @return a list of DDL statements to execute + * @throws IOException if the statements could not be read + */ + private List readDDLStatements(BufferedReader reader) throws IOException { + try { + List sqlList = new ArrayList(); + String sql = reader.readLine(); + while (sql != null) { + // Skip comments and empty lines + if (!sql.startsWith("#") && sql.trim().length() > 0) { + sql = replace(sql); + sqlList.add(sql); } - } finally { - IOUtils.closeQuietly(ddl); + sql = reader.readLine(); } + return sqlList; + } finally { + IOUtils.closeQuietly(reader); } } /** + * Executes the schema creation DDL statements. + * + * @param sqlList the list of DDL statements to execute + * @throws SQLException on error + */ + private void createSchema(List sqlList) throws SQLException { + for (String sql : sqlList) { + log.info("Preparing execution of : " + sql); + } + conHelper.startBatch(); + boolean success = false; + try { + for (String sql : sqlList) { + conHelper.exec(sql); + log.info("Executed : " + sql); + } + success = true; + } finally { + try { + conHelper.endBatch(success); + } catch (SQLException e) { + if (success) { + // transaction commit failed + throw e; + } else { + // Don't mask an exception from the try block above + log.warn("Failed to properly rollback the schema-creation transaction", e); + } + } + } + } + + /** * Applies the variable replacement to the given string. * * @param sql the string in which to replace variables Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelper.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelper.java (revision 908870) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelper.java (working copy) @@ -16,207 +16,48 @@ */ package org.apache.jackrabbit.core.util.db; -import java.sql.Connection; -import java.sql.DatabaseMetaData; -import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; -import java.sql.Statement; -import javax.sql.DataSource; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - /** - * This class provides convenience methods to execute SQL statements. They can be either executed in isolation + * Instances provide convenience methods to execute SQL statements. They can be either executed in isolation * or within the context of a JDBC transaction; the so-called batch mode (use the {@link #startBatch()} * and {@link #endBatch(boolean)} methods for this). * *

* - * This class contains logic to retry execution of SQL statements. If this helper is not in batch mode - * and if a statement fails due to an {@code SQLException}, then it is retried. If the {@code block} argument - * of the constructor call was {@code false} then it is retried only once. Otherwise the statement is retried - * until either it succeeds or the thread is interrupted. This clearly assumes that the only cause of {@code - * SQLExceptions} is faulty {@code Connections} which are restored eventually.
Note: - * This retry logic only applies to the following methods: - *

    - *
  • {@link #exec(String, Object...)}
    • - *
  • {@link #update(String, Object[])}
    • - *
  • {@link #exec(String, Object[], boolean, int)}
    • - *
- * - *

- * - * This class is not thread-safe and if it is to be used by multiple threads then the clients must make sure - * that access to this class is properly synchronized. - * - *

- * - * Implementation note: The {@code Connection} that is retrieved from the {@code DataSource} - * in {@link #getConnection()} may be broken. This is so because if an internal {@code DataSource} is used, - * then this is a commons-dbcp {@code DataSource} with a testWhileIdle validation strategy (see - * the {@code ConnectionFactory} class). Furthermore, if it is a {@code DataSource} obtained through JNDI then we - * can make no assumptions about the validation strategy. This means that our retry logic must either assume that - * the SQL it tries to execute can do so without errors (i.e., the statement is valid), or it must implement its - * own validation strategy to apply. Currently, the former is in place. + * Instances are not required to be thread-safe and if it is to be used by multiple threads then the clients + * must make sure that access to this class is properly synchronized. */ -public class ConnectionHelper { +public interface ConnectionHelper { - private static Logger log = LoggerFactory.getLogger(ConnectionHelper.class); - - private static final int RETRIES = 1; - - private static final int SLEEP_BETWEEN_RETRIES_MS = 100; - - private final boolean blockOnConnectionLoss; - - private final boolean checkTablesWithUserName; - - protected final DataSource dataSource; - - private boolean inBatchMode = false; - - private Connection batchConnection = null; - /** - * @param dataSrc the {@link DataSource} on which this instance acts - * @param block whether the helper should transparently block on DB connection loss (otherwise it retries - * once and if that fails throws exception) - */ - public ConnectionHelper(DataSource dataSrc, boolean block) { - dataSource = dataSrc; - checkTablesWithUserName = false; - blockOnConnectionLoss = block; - } - - /** - * @param dataSrc the {@link DataSource} on which this instance acts - * @param checkWithUserName whether the username is to be used for the {@link #tableExists(String)} method - * @param block whether the helper should transparently block on DB connection loss (otherwise it throws exceptions) - */ - protected ConnectionHelper(DataSource dataSrc, boolean checkWithUserName, boolean block) { - dataSource = dataSrc; - checkTablesWithUserName = checkWithUserName; - blockOnConnectionLoss = block; - } - - /** * A utility method that makes sure that identifier does only consist of characters that are * allowed in names on the target database. Illegal characters will be escaped as necessary. * - * This method is not affected by the - * * @param identifier the identifier to convert to a db specific identifier * @return the db-normalized form of the given identifier * @throws SQLException if an error occurs */ - public final String prepareDbIdentifier(String identifier) throws SQLException { - if (identifier == null) { - return null; - } - String legalChars = "ABCDEFGHIJKLMNOPQRSTUVWXZY0123456789_"; - legalChars += getExtraNameCharacters(); - String id = identifier.toUpperCase(); - StringBuffer escaped = new StringBuffer(); - for (int i = 0; i < id.length(); i++) { - char c = id.charAt(i); - if (legalChars.indexOf(c) == -1) { - replaceCharacter(escaped, c); - } else { - escaped.append(c); - } - } - return escaped.toString(); - } + String prepareDbIdentifier(String identifier) throws SQLException; /** - * Called from {@link #prepareDbIdentifier(String)}. Default implementation replaces the illegal - * characters with their hexadecimal encoding. - * - * @param escaped the escaped db identifier - * @param c the character to replace - */ - protected void replaceCharacter(StringBuffer escaped, char c) { - escaped.append("_x"); - String hex = Integer.toHexString(c); - escaped.append("0000".toCharArray(), 0, 4 - hex.length()); - escaped.append(hex); - escaped.append("_"); - } - - /** - * The default implementation returns the {@code extraNameCharacters} provided by the databases metadata. - * - * @return the additional characters for identifiers supported by the db - * @throws SQLException on error - */ - private String getExtraNameCharacters() throws SQLException { - Connection con = dataSource.getConnection(); - try { - DatabaseMetaData metaData = con.getMetaData(); - return metaData.getExtraNameCharacters(); - } finally { - DbUtility.close(con, null, null); - } - } - - /** * Checks whether the given table exists in the database. * * @param tableName the name of the table * @return whether the given table exists * @throws SQLException on error */ - public final boolean tableExists(String tableName) throws SQLException { - Connection con = dataSource.getConnection(); - ResultSet rs = null; - boolean schemaExists = false; - String name = tableName; - try { - DatabaseMetaData metaData = con.getMetaData(); - if (metaData.storesLowerCaseIdentifiers()) { - name = tableName.toLowerCase(); - } else if (metaData.storesUpperCaseIdentifiers()) { - name = tableName.toUpperCase(); - } - String userName = null; - if (checkTablesWithUserName) { - userName = metaData.getUserName(); - } - rs = metaData.getTables(null, userName, name, null); - schemaExists = rs.next(); - } finally { - DbUtility.close(con, null, rs); - } - return schemaExists; - } + boolean tableExists(String tableName) throws SQLException; /** - * Starts the batch mode. If an {@link SQLException} is thrown, then the batch mode is not started.

- * Important: clients that call this method must make sure that + * Starts the batch mode. If an {@link SQLException} is thrown, then the batch mode is not started. + *

Important: clients that call this method must make sure that * {@link #endBatch(boolean)} is called eventually. * * @throws SQLException on error */ - public final void startBatch() throws SQLException { - if (inBatchMode) { - throw new IllegalStateException("already in batch mode"); - } - try { - batchConnection = getConnection(); - batchConnection.setAutoCommit(false); - inBatchMode = true; - } catch (SQLException e) { - // Strive for failure atomicity - if (batchConnection != null) { - DbUtility.close(batchConnection, null, null); - } - batchConnection = null; - throw e; - } - } + void startBatch() throws SQLException; /** * This method always ends the batch mode. @@ -225,62 +66,17 @@ * @throws SQLException if the commit or rollback of the underlying JDBC Connection threw an {@code * SQLException} */ - public final void endBatch(boolean commit) throws SQLException { - if (!inBatchMode) { - throw new IllegalStateException("not in batch mode"); - } - try { - if (commit) { - batchConnection.commit(); - } else { - batchConnection.rollback(); - } - } finally { - DbUtility.close(batchConnection, null, null); - batchConnection = null; - inBatchMode = false; - } - } + void endBatch(boolean commit) throws SQLException; /** - * Executes a general SQL statement and immediately closes all resources. + * Executes a general SQL statement. * - * Note: We use a Statement if there are no parameters to avoid a problem on - * the Oracle 10g JDBC driver w.r.t. :NEW and :OLD keywords that triggers ORA-17041. - * * @param sql an SQL statement string * @param params the parameters for the SQL statement * @throws SQLException on error */ - public final void exec(final String sql, final Object... params) throws SQLException { - new RetryManager() { + void exec(final String sql, final Object... params) throws SQLException; - @Override - protected Void call() throws SQLException { - reallyExec(sql, params); - return null; - } - - }.doTry(); - } - - private void reallyExec(String sql, Object... params) throws SQLException { - Connection con = null; - Statement stmt = null; - try { - con = getConnection(); - if (params == null || params.length == 0) { - stmt = con.createStatement(); - stmt.execute(sql); - } else { - stmt = con.prepareStatement(sql); - execute((PreparedStatement) stmt, params); - } - } finally { - closeResources(con, stmt, null); - } - } - /** * Executes an update or delete statement and returns the update count. * @@ -289,29 +85,8 @@ * @return the update count * @throws SQLException on error */ - public final int update(final String sql, final Object[] params) throws SQLException { - return new RetryManager() { + int update(final String sql, final Object[] params) throws SQLException; - @Override - protected Integer call() throws SQLException { - return reallyUpdate(sql, params); - } - - }.doTry(); - } - - private int reallyUpdate(String sql, Object[] params) throws SQLException { - Connection con = null; - PreparedStatement stmt = null; - try { - con = getConnection(); - stmt = con.prepareStatement(sql); - return execute(stmt, params).getUpdateCount(); - } finally { - closeResources(con, stmt, null); - } - } - /** * Executes a general SQL statement and returns the {@link ResultSet} of the executed statement. The * returned {@link ResultSet} should be closed by clients. @@ -323,151 +98,6 @@ * @return a {@link ResultSet} * @throws SQLException on error */ - public final ResultSet exec(final String sql, final Object[] params, final boolean returnGeneratedKeys, - final int maxRows) throws SQLException { - return new RetryManager() { - - @Override - protected ResultSet call() throws SQLException { - return reallyExec(sql, params, returnGeneratedKeys, maxRows); - } - - }.doTry(); - } - - private ResultSet reallyExec(String sql, Object[] params, boolean returnGeneratedKeys, int maxRows) - throws SQLException { - Connection con = null; - PreparedStatement stmt = null; - ResultSet rs = null; - try { - con = getConnection(); - if (returnGeneratedKeys) { - stmt = con.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS); - } else { - stmt = con.prepareStatement(sql); - } - stmt.setMaxRows(maxRows); - execute(stmt, params); - if (returnGeneratedKeys) { - rs = stmt.getGeneratedKeys(); - } else { - rs = stmt.getResultSet(); - } - // Don't wrap null - if (rs == null) { - return null; - } - if (inBatchMode) { - return ResultSetWrapper.newInstance(null, stmt, rs); - } else { - return ResultSetWrapper.newInstance(con, stmt, rs); - } - } catch (SQLException e) { - closeResources(con, stmt, rs); - throw e; - } - } - - /** - * Gets a connection based on the {@code batchMode} state of this helper. The connection should be closed - * by a call to {@link #closeResources(Connection, Statement, ResultSet)} which also takes the {@code - * batchMode} state into account. - * - * @return a {@code Connection} to use, based on the batch mode state - * @throws SQLException on error - */ - protected final Connection getConnection() throws SQLException { - if (inBatchMode) { - return batchConnection; - } else { - Connection con = dataSource.getConnection(); - // JCR-1013: Setter may fail unnecessarily on a managed connection - if (!con.getAutoCommit()) { - con.setAutoCommit(true); - } - return con; - } - } - - /** - * Closes the given resources given the {@code batchMode} state. - * - * @param con the {@code Connection} obtained through the {@link #getConnection()} method - * @param stmt a {@code Statement} - * @param rs a {@code ResultSet} - */ - protected final void closeResources(Connection con, Statement stmt, ResultSet rs) { - if (inBatchMode) { - DbUtility.close(null, stmt, rs); - } else { - DbUtility.close(con, stmt, rs); - } - } - - /** - * This method is used by all methods of this class that execute SQL statements. This default - * implementation sets all parameters and unwraps {@link StreamWrapper} instances. Subclasses may override - * this method to do something special with the parameters. E.g., the {@link Oracle10R1ConnectionHelper} - * overrides it in order to add special blob handling. - * - * @param stmt the {@link PreparedStatement} to execute - * @param params the parameters - * @return the executed statement - * @throws SQLException on error - */ - protected PreparedStatement execute(PreparedStatement stmt, Object[] params) throws SQLException { - for (int i = 0; params != null && i < params.length; i++) { - Object p = params[i]; - // FIXME: what about already consumed input streams when in a retry? - if (p instanceof StreamWrapper) { - StreamWrapper wrapper = (StreamWrapper) p; - stmt.setBinaryStream(i + 1, wrapper.getStream(), (int) wrapper.getSize()); - } else { - stmt.setObject(i + 1, p); - } - } - stmt.execute(); - return stmt; - } - - /** - * This class encapsulates the logic to retry a method invocation if it threw an SQLException. - * - * @param the return type of the method which is retried if it failed - */ - public abstract class RetryManager { - - public final T doTry() throws SQLException { - if (inBatchMode) { - return call(); - } else { - boolean sleepInterrupted = false; - int failures = 0; - SQLException lastException = null; - while (!sleepInterrupted && (blockOnConnectionLoss || failures <= RETRIES)) { - try { - return call(); - } catch (SQLException e) { - lastException = e; - } - log.error("Failed to execute SQL (stacktrace on DEBUG log level)", lastException); - log.debug("Failed to execute SQL", lastException); - failures++; - if (blockOnConnectionLoss || failures <= RETRIES) { // if we're going to try again - try { - Thread.sleep(SLEEP_BETWEEN_RETRIES_MS); - } catch (InterruptedException e1) { - Thread.currentThread().interrupt(); - sleepInterrupted = true; - log.error("Interrupted: canceling retry"); - } - } - } - throw lastException; - } - } - - protected abstract T call() throws SQLException; - } + ResultSet exec(final String sql, final Object[] params, final boolean returnGeneratedKeys, + final int maxRows) throws SQLException; } Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelperImpl.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelperImpl.java (revision 889236) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/ConnectionHelperImpl.java (working copy) @@ -29,17 +29,16 @@ import org.slf4j.LoggerFactory; /** - * This class provides convenience methods to execute SQL statements. They can be either executed in isolation - * or within the context of a JDBC transaction; the so-called batch mode (use the {@link #startBatch()} - * and {@link #endBatch(boolean)} methods for this). + * This class is not thread-safe and if it is to be used by multiple threads then the clients must make sure + * that access to this class is properly synchronized. * *

* - * This class contains logic to retry execution of SQL statements. If this helper is not in batch mode + * This type contains logic to retry execution of SQL statements. If this helper is not in batch mode * and if a statement fails due to an {@code SQLException}, then it is retried. If the {@code block} argument * of the constructor call was {@code false} then it is retried only once. Otherwise the statement is retried * until either it succeeds or the thread is interrupted. This clearly assumes that the only cause of {@code - * SQLExceptions} is faulty {@code Connections} which are restored eventually.
Note: + * SQLExceptions} is faulty {@code Connections} which are restored eventually. Note: * This retry logic only applies to the following methods: *

    *
  • {@link #exec(String, Object...)}
    • @@ -49,11 +48,6 @@ * *

    * - * This class is not thread-safe and if it is to be used by multiple threads then the clients must make sure - * that access to this class is properly synchronized. - * - *

    - * * Implementation note: The {@code Connection} that is retrieved from the {@code DataSource} * in {@link #getConnection()} may be broken. This is so because if an internal {@code DataSource} is used, * then this is a commons-dbcp {@code DataSource} with a testWhileIdle validation strategy (see @@ -62,9 +56,9 @@ * the SQL it tries to execute can do so without errors (i.e., the statement is valid), or it must implement its * own validation strategy to apply. Currently, the former is in place. */ -public class ConnectionHelper { +public class ConnectionHelperImpl implements ConnectionHelper { - private static Logger log = LoggerFactory.getLogger(ConnectionHelper.class); + private static Logger log = LoggerFactory.getLogger(ConnectionHelperImpl.class); private static final int RETRIES = 1; @@ -85,7 +79,7 @@ * @param block whether the helper should transparently block on DB connection loss (otherwise it retries * once and if that fails throws exception) */ - public ConnectionHelper(DataSource dataSrc, boolean block) { + public ConnectionHelperImpl(DataSource dataSrc, boolean block) { dataSource = dataSrc; checkTablesWithUserName = false; blockOnConnectionLoss = block; @@ -96,21 +90,14 @@ * @param checkWithUserName whether the username is to be used for the {@link #tableExists(String)} method * @param block whether the helper should transparently block on DB connection loss (otherwise it throws exceptions) */ - protected ConnectionHelper(DataSource dataSrc, boolean checkWithUserName, boolean block) { + protected ConnectionHelperImpl(DataSource dataSrc, boolean checkWithUserName, boolean block) { dataSource = dataSrc; checkTablesWithUserName = checkWithUserName; blockOnConnectionLoss = block; } - + /** - * A utility method that makes sure that identifier does only consist of characters that are - * allowed in names on the target database. Illegal characters will be escaped as necessary. - * - * This method is not affected by the - * - * @param identifier the identifier to convert to a db specific identifier - * @return the db-normalized form of the given identifier - * @throws SQLException if an error occurs + * {@inheritDoc} */ public final String prepareDbIdentifier(String identifier) throws SQLException { if (identifier == null) { @@ -163,11 +150,7 @@ } /** - * Checks whether the given table exists in the database. - * - * @param tableName the name of the table - * @return whether the given table exists - * @throws SQLException on error + * {@inheritDoc} */ public final boolean tableExists(String tableName) throws SQLException { Connection con = dataSource.getConnection(); @@ -194,11 +177,7 @@ } /** - * Starts the batch mode. If an {@link SQLException} is thrown, then the batch mode is not started.

    - * Important: clients that call this method must make sure that - * {@link #endBatch(boolean)} is called eventually. - * - * @throws SQLException on error + * {@inheritDoc} */ public final void startBatch() throws SQLException { if (inBatchMode) { @@ -219,11 +198,7 @@ } /** - * This method always ends the batch mode. - * - * @param commit whether the changes in the batch should be committed or rolled back - * @throws SQLException if the commit or rollback of the underlying JDBC Connection threw an {@code - * SQLException} + * {@inheritDoc} */ public final void endBatch(boolean commit) throws SQLException { if (!inBatchMode) { @@ -243,14 +218,7 @@ } /** - * Executes a general SQL statement and immediately closes all resources. - * - * Note: We use a Statement if there are no parameters to avoid a problem on - * the Oracle 10g JDBC driver w.r.t. :NEW and :OLD keywords that triggers ORA-17041. - * - * @param sql an SQL statement string - * @param params the parameters for the SQL statement - * @throws SQLException on error + * {@inheritDoc} */ public final void exec(final String sql, final Object... params) throws SQLException { new RetryManager() { @@ -260,10 +228,14 @@ reallyExec(sql, params); return null; } - + }.doTry(); } - + + /** + * Note: We use a Statement if there are no parameters to avoid a problem on the Oracle 10g JDBC driver + * w.r.t. :NEW and :OLD keywords that triggers ORA-17041. + */ private void reallyExec(String sql, Object... params) throws SQLException { Connection con = null; Statement stmt = null; @@ -282,12 +254,7 @@ } /** - * Executes an update or delete statement and returns the update count. - * - * @param sql an SQL statement string - * @param params the parameters for the SQL statement - * @return the update count - * @throws SQLException on error + * {@inheritDoc} */ public final int update(final String sql, final Object[] params) throws SQLException { return new RetryManager() { @@ -296,7 +263,7 @@ protected Integer call() throws SQLException { return reallyUpdate(sql, params); } - + }.doTry(); } @@ -313,15 +280,7 @@ } /** - * Executes a general SQL statement and returns the {@link ResultSet} of the executed statement. The - * returned {@link ResultSet} should be closed by clients. - * - * @param sql an SQL statement string - * @param params the parameters for the SQL statement - * @param returnGeneratedKeys whether generated keys should be returned - * @param maxRows the maximum number of rows in a potential {@link ResultSet} (0 means no limit) - * @return a {@link ResultSet} - * @throws SQLException on error + * {@inheritDoc} */ public final ResultSet exec(final String sql, final Object[] params, final boolean returnGeneratedKeys, final int maxRows) throws SQLException { @@ -331,10 +290,10 @@ protected ResultSet call() throws SQLException { return reallyExec(sql, params, returnGeneratedKeys, maxRows); } - + }.doTry(); } - + private ResultSet reallyExec(String sql, Object[] params, boolean returnGeneratedKeys, int maxRows) throws SQLException { Connection con = null; @@ -352,10 +311,11 @@ if (returnGeneratedKeys) { rs = stmt.getGeneratedKeys(); } else { - rs = stmt.getResultSet(); + rs = stmt.getResultSet(); // may return null } // Don't wrap null if (rs == null) { + closeResources(con, stmt, rs); return null; } if (inBatchMode) { @@ -430,7 +390,7 @@ stmt.execute(); return stmt; } - + /** * This class encapsulates the logic to retry a method invocation if it threw an SQLException. * @@ -467,7 +427,7 @@ throw lastException; } } - + protected abstract T call() throws SQLException; } } Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/DerbyConnectionHelper.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/DerbyConnectionHelper.java (revision 908870) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/DerbyConnectionHelper.java (working copy) @@ -28,7 +28,7 @@ /** * */ -public final class DerbyConnectionHelper extends ConnectionHelper { +public final class DerbyConnectionHelper extends ConnectionHelperImpl { /** name of the embedded driver */ public static final String DERBY_EMBEDDED_DRIVER = "org.apache.derby.jdbc.EmbeddedDriver"; Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/OracleConnectionHelper.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/OracleConnectionHelper.java (revision 908870) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/db/OracleConnectionHelper.java (working copy) @@ -28,7 +28,7 @@ /** * The connection helper for Oracle databases of version 10.2 and later. */ -public class OracleConnectionHelper extends ConnectionHelper { +public class OracleConnectionHelper extends ConnectionHelperImpl { /** * the default logger Index: jackrabbit-core/pom.xml =================================================================== --- jackrabbit-core/pom.xml (revision 908870) +++ jackrabbit-core/pom.xml (working copy) @@ -248,6 +248,11 @@ geronimo-jta_1.0.1B_spec test + + org.easymock + easymock + test +