Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement database delegate abstraction for ScriptUtils #551

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions modules/database-commons/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

<parent>
<groupId>org.testcontainers</groupId>
<artifactId>testcontainers-parent</artifactId>
<version>0-SNAPSHOT</version>
<relativePath>../../pom.xml</relativePath>
</parent>

<artifactId>database-commons</artifactId>
<name>TestContainers :: Database-Commons</name>

<dependencies>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>testcontainers</artifactId>
<version>${project.version}</version>
</dependency>
</dependencies>

</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package org.testcontainers.delegate;

import java.util.Collection;

/**
* @param <CONTAINER> testcontainers container
* @param <CONNECTION> connection to the database
* @author Eugeny Karpov
*/
public abstract class AbstractDatabaseDelegate<CONTAINER, CONNECTION> implements DatabaseDelegate {

/**
* Testcontainers container
*/
protected CONTAINER container;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does used in children - JdbcDatabaseDelegate and CassandraDatabaseDelegate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children should store it themselves

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


/**
* Database connection
*/
private CONNECTION connection;

public AbstractDatabaseDelegate(CONTAINER container) {
this.container = container;
}

/**
* Get or create new connection to the database
*/
protected CONNECTION getConnection() {
if (connection == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not thread safe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment where database delegates are invoked is single threaded, so from my perspective there is no reason to implement here thread safety. Delegate is used to apply a script. No need to even have multiple threads here.

connection = createNewConnection();
}
return connection;
}

@Override
public void execute(Collection<String> statements, String scriptPath, boolean continueOnError, boolean ignoreFailedDrops) {
int lineNumber = 0;
for (String statement : statements) {
lineNumber++;
execute(statement, scriptPath, lineNumber, continueOnError, ignoreFailedDrops);
}
}

@Override
public void close() {
if (connection != null) {
closeConnectionQuietly(connection);
}
}

/**
* Quietly close the connection
*/
protected abstract void closeConnectionQuietly(CONNECTION connection);

/**
* Template method for creating new connections to the database
*/
protected abstract CONNECTION createNewConnection();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package org.testcontainers.delegate;

import java.util.Collection;

/**
* Database delegate
*
* Gives an abstraction from concrete database
*
* @author Eugeny Karpov
*/
public interface DatabaseDelegate extends AutoCloseable {

/**
* Execute statement by the implementation of the delegate
*/
void execute(String statement, String scriptPath, int lineNumber, boolean continueOnError, boolean ignoreFailedDrops);

/**
* Execute collection of statements
*/
void execute(Collection<String> statements, String scriptPath, boolean continueOnError, boolean ignoreFailedDrops);

/**
* Close connection to the database
*
* Overridden to suppress throwing Exception
*/
@Override
void close();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package org.testcontainers.exception;

/**
* Inability to create connection to the database
*
* @author Eugeny Karpov
*/
public class ConnectionCreationException extends RuntimeException {

public ConnectionCreationException(String message, Throwable cause) {
super(message, cause);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,14 @@
* limitations under the License.
*/

package org.testcontainers.jdbc.ext;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this breaks public API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScriptUtils is mainly for internal usage (as it's stated in javadoc). No public testcontainers projects are using this class (I checked :) ). So personally I don't think it's a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know at least one non-public project where it is used (I checked ;) ), so personally I think it is a problem :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, some non-public, probably personal, project uses internal API of another project and therefore internal API of that project must be sealed in stone. The tail wagging the dog.

Okay, I've kept public API of the Sql-based ScriptUtils - wrapper over new ScriptUtils.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably personal

Your assumption is incorrect. These are some big companies actually ;)

Why do you think ScriptUtils is an internal API btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think ScriptUtils is an internal API btw?

It is said in the javadoc:

  • Generic utility methods for working with SQL scripts. Mainly for internal use
  • within the framework.

package org.testcontainers.ext;

import org.apache.commons.lang.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.delegate.DatabaseDelegate;

import javax.script.ScriptException;
import java.sql.Connection;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.LinkedList;
import java.util.List;

Expand Down Expand Up @@ -212,17 +210,16 @@ public static boolean containsSqlScriptDelimiters(String script, String delim) {
return false;
}

public static void executeSqlScript(Connection connection, String scriptPath, String script) throws ScriptException {
executeSqlScript(connection, scriptPath, script, false, false, DEFAULT_COMMENT_PREFIX, DEFAULT_STATEMENT_SEPARATOR, DEFAULT_BLOCK_COMMENT_START_DELIMITER, DEFAULT_BLOCK_COMMENT_END_DELIMITER);
public static void executeDatabaseScript(DatabaseDelegate databaseDelegate, String scriptPath, String script) throws ScriptException {
executeDatabaseScript(databaseDelegate, scriptPath, script, false, false, DEFAULT_COMMENT_PREFIX, DEFAULT_STATEMENT_SEPARATOR, DEFAULT_BLOCK_COMMENT_START_DELIMITER, DEFAULT_BLOCK_COMMENT_END_DELIMITER);
}

/**
* Execute the given SQL script.
* Execute the given database script.
* <p>Statement separators and comments will be removed before executing
* individual statements within the supplied script.
* <p><b>Do not use this method to execute DDL if you expect rollback.</b>
* @param connection the JDBC connection to use to execute the script; already
* configured and ready to use
* @param databaseDelegate database delegate for script execution
* @param scriptPath the resource (potentially associated with a specific encoding)
* to load the SQL script from
* @param script the raw script content
Expand All @@ -240,13 +237,13 @@ public static void executeSqlScript(Connection connection, String scriptPath, St
* @param blockCommentEndDelimiter the <em>end</em> block comment delimiter; never
* {@code null} or empty @throws ScriptException if an error occurred while executing the SQL script
*/
public static void executeSqlScript(Connection connection, String scriptPath, String script, boolean continueOnError,
public static void executeDatabaseScript(DatabaseDelegate databaseDelegate, String scriptPath, String script, boolean continueOnError,
boolean ignoreFailedDrops, String commentPrefix, String separator, String blockCommentStartDelimiter,
String blockCommentEndDelimiter) throws ScriptException {

try {
if (LOGGER.isInfoEnabled()) {
LOGGER.info("Executing SQL script from " + scriptPath);
LOGGER.info("Executing database script from " + scriptPath);
}

long startTime = System.currentTimeMillis();
Expand All @@ -261,44 +258,14 @@ public static void executeSqlScript(Connection connection, String scriptPath, St

splitSqlScript(scriptPath, script, separator, commentPrefix, blockCommentStartDelimiter,
blockCommentEndDelimiter, statements);
int lineNumber = 0;
Statement stmt = connection.createStatement();
try {
for (String statement : statements) {
lineNumber++;
try {
stmt.execute(statement);
int rowsAffected = stmt.getUpdateCount();
if (LOGGER.isDebugEnabled()) {
LOGGER.debug(rowsAffected + " returned as updateCount for SQL: " + statement);
}
}
catch (SQLException ex) {
boolean dropStatement = statement.trim().toLowerCase().startsWith("drop");
if (continueOnError || (dropStatement && ignoreFailedDrops)) {
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Failed to execute SQL script statement at line " + lineNumber
+ " of resource " + scriptPath + ": " + statement, ex);
}
}
else {
throw new ScriptStatementFailedException(statement, lineNumber, scriptPath, ex);
}
}
}
}
finally {
try {
stmt.close();
}
catch (Throwable ex) {
LOGGER.debug("Could not close JDBC Statement", ex);
}

try (DatabaseDelegate closeableDelegate = databaseDelegate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executeDatabaseScript(databaseDelegate, ...);
executeDatabaseScript(databaseDelegate, ...)

will not work because you close the delegate in this method. It should not perform such side effects

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, with current code it won't work either - because java.sql.Connection will be closed after first line.
Okay, I created new boolean property in AbstractDatabaseDelegate (to avoid null based logic). Now first statement execution creates new connection. All subsequent statements will use existing connection. Closing the connection closes the connection and sets isConnectionStarted propery to false, if connection was started in the first place. All subsequent database delegate invokations will use the same logic.

It should not perform such side effects

I disagree with you. Connection to the database should be closed after script invocation. As it is in the current diff or current code in master.

closeableDelegate.execute(statements, scriptPath, continueOnError, ignoreFailedDrops);
}

long elapsedTime = System.currentTimeMillis() - startTime;
if (LOGGER.isInfoEnabled()) {
LOGGER.info("Executed SQL script from " + scriptPath + " in " + elapsedTime + " ms.");
LOGGER.info("Executed database script from " + scriptPath + " in " + elapsedTime + " ms.");
}
}
catch (Exception ex) {
Expand All @@ -317,8 +284,8 @@ public ScriptParseException(String format, String scriptPath) {
}
}

private static class ScriptStatementFailedException extends RuntimeException {
public ScriptStatementFailedException(String statement, int lineNumber, String scriptPath, SQLException ex) {
public static class ScriptStatementFailedException extends RuntimeException {
public ScriptStatementFailedException(String statement, int lineNumber, String scriptPath, Exception ex) {
super(String.format("Script execution failed (%s:%d): %s", scriptPath, lineNumber, statement), ex);
}
}
Expand Down
6 changes: 6 additions & 0 deletions modules/jdbc/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
<artifactId>testcontainers</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>database-commons</artifactId>
<version>${project.version}</version>
</dependency>
</dependencies>

</project>
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
import org.slf4j.LoggerFactory;
import org.testcontainers.containers.JdbcDatabaseContainer;
import org.testcontainers.containers.JdbcDatabaseContainerProvider;
import org.testcontainers.jdbc.ext.ScriptUtils;
import org.testcontainers.delegate.DatabaseDelegate;
import org.testcontainers.ext.ScriptUtils;

import javax.script.ScriptException;
import java.io.IOException;
Expand Down Expand Up @@ -153,7 +154,8 @@ public synchronized Connection connect(String url, final Properties info) throws
an init script or function has been specified, use it
*/
if (!initializedContainers.contains(container.getContainerId())) {
runInitScriptIfRequired(url, connection);
DatabaseDelegate databaseDelegate = new JdbcDatabaseDelegate(container);
runInitScriptIfRequired(url, databaseDelegate);
runInitFunctionIfRequired(url, connection);
initializedContainers.add(container.getContainerId());
}
Expand Down Expand Up @@ -214,10 +216,10 @@ private Connection wrapConnection(final Connection connection, final JdbcDatabas
* Run an init script from the classpath.
*
* @param url the JDBC URL to check for init script declarations.
* @param connection JDBC connection to apply init scripts to.
* @param databaseDelegate database delegate to apply init scripts to the database
* @throws SQLException on script or DB error
*/
private void runInitScriptIfRequired(String url, Connection connection) throws SQLException {
private void runInitScriptIfRequired(String url, DatabaseDelegate databaseDelegate) throws SQLException {
Matcher matcher = INITSCRIPT_MATCHING_PATTERN.matcher(url);
if (matcher.matches()) {
String initScriptPath = matcher.group(2);
Expand All @@ -230,7 +232,7 @@ private void runInitScriptIfRequired(String url, Connection connection) throws S
}

String sql = IOUtils.toString(resource, StandardCharsets.UTF_8);
ScriptUtils.executeSqlScript(connection, initScriptPath, sql);
ScriptUtils.executeDatabaseScript(databaseDelegate, initScriptPath, sql);
} catch (IOException e) {
LOGGER.warn("Could not load classpath init script: {}", initScriptPath);
throw new SQLException("Could not load classpath init script: " + initScriptPath, e);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package org.testcontainers.jdbc;

import lombok.extern.slf4j.Slf4j;
import org.testcontainers.containers.JdbcDatabaseContainer;
import org.testcontainers.delegate.AbstractDatabaseDelegate;
import org.testcontainers.exception.ConnectionCreationException;
import org.testcontainers.ext.ScriptUtils;

import java.sql.SQLException;
import java.sql.Statement;

/**
* JDBC database delegate
*
* @author Eugeny Karpov
*/
@Slf4j
public class JdbcDatabaseDelegate extends AbstractDatabaseDelegate<JdbcDatabaseContainer, Statement> {

public JdbcDatabaseDelegate(JdbcDatabaseContainer jdbcDatabaseContainer) {
super(jdbcDatabaseContainer);
}

@Override
protected Statement createNewConnection() {
try {
return container.createConnection("").createStatement();
} catch (SQLException e) {
log.error("Could not obtain JDBC connection");
throw new ConnectionCreationException("Could not obtain JDBC connection", e);
}
}


@Override
public void execute(String statement, String scriptPath, int lineNumber, boolean continueOnError, boolean ignoreFailedDrops) {
try {
boolean rowsAffected = getConnection().execute(statement);
log.debug("{} returned as updateCount for SQL: {}", rowsAffected, statement);
} catch (SQLException ex) {
boolean dropStatement = statement.trim().toLowerCase().startsWith("drop");
if (continueOnError || (dropStatement && ignoreFailedDrops)) {
log.debug("Failed to execute SQL script statement at line {} of resource {}: {}", lineNumber, scriptPath, statement, ex);
} else {
throw new ScriptUtils.ScriptStatementFailedException(statement, lineNumber, scriptPath, ex);
}
}
}

@Override
protected void closeConnectionQuietly(Statement statement) {
try {
statement.close();
} catch (Exception e) {
log.error("Could not close JDBC connection", e);
}
}
}
1 change: 1 addition & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@
<module>modules/selenium</module>
<module>modules/nginx</module>
<module>modules/jdbc-test</module>
<module>modules/database-commons</module>
</modules>

<profiles>
Expand Down