implement database delegate abstraction for ScriptUtils #551
Conversation
| @@ -14,16 +14,14 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| package org.testcontainers.jdbc.ext; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I know at least one non-public project where it is used (I checked ;) ), so personally I think it is a problem :D
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
probably personal
Your assumption is incorrect. These are some big companies actually ;)
Why do you think ScriptUtils is an internal API btw?
There was a problem hiding this comment.
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.
| /** | ||
| * Testcontainers container | ||
| */ | ||
| protected CONTAINER container; |
There was a problem hiding this comment.
It does used in children - JdbcDatabaseDelegate and CassandraDatabaseDelegate.
There was a problem hiding this comment.
children should store it themselves
| LOGGER.debug("Could not close JDBC Statement", ex); | ||
| } | ||
|
|
||
| try (DatabaseDelegate closeableDelegate = databaseDelegate) { |
There was a problem hiding this comment.
executeDatabaseScript(databaseDelegate, ...);
executeDatabaseScript(databaseDelegate, ...)
will not work because you close the delegate in this method. It should not perform such side effects
There was a problem hiding this comment.
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.
| * Get or create new connection to the database | ||
| */ | ||
| protected CONNECTION getConnection() { | ||
| if (connection == null) { |
There was a problem hiding this comment.
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.
keep public API of ScriptUtils
|
All comments are fixed, as usual. Any response? |
|
Sorry, it's been a hectic week. I'm aiming to do some PR re-review/merging tonight, and this is on my list. |
|
I've rebased, squashed and added a changelog (keeping the commit author as yourself though, @AnkBurov). Will merge now from my branch. |
|
Merged into master. Thanks @AnkBurov ! |
|
Thanks @rnorth ! |
|
|
||
| private JdbcDatabaseContainer container; | ||
|
|
||
| public JdbcDatabaseDelegate(JdbcDatabaseContainer container) { |
There was a problem hiding this comment.
There's a design issue here. JdbcDatabaseContainer is a generic type since 4cb5a1d. But here, it's using the raw type. It should take JdbcDatabaseContainer<?> instead.
The current raw type causes Scala, with its more sound type system, to choke.
Currently ScriptUtils class is tied up with relational databases. This PR introduces database delegate concept allowing for ScriptUtils to work with any database (Cassandra with CQL as an example). It allows Cassandra container (different PR) to use ScriptUtils without any code duplication.
Blocker for PR #525