Support multi-line SQL comments in ResourceDatabasePopulator and JdbcTestUtils [SPR-9531]

[spring-projects-issues]

Ian Wright opened SPR-9531 and commented

Overview

When executing SQL scripts using ResourceDatabasePopulator or JdbcTestUtils errors may occur if there are block comments in the file. In general multi-line comments using the /* ... */ syntax are not explicitly supported.

Notes

Proper support for standard SQL comments (i.e., any text beginning with two hyphens and extending to the end of the line) was introduced in #14616 and #14708. However, neither ResourceDatabasePopulator nor JdbcTestUtils contains explicit support for multi-line SQL comments.

Deliverables

1. [x] Extract common functionality related to script parsing from ResourceDatabasePopulator and JdbcTestUtils into a new public component or utility in the org.springframework.jdbc.datasource.init package in the spring-jdbc module.
2. [x] Support multi-line SQL comments in the component extracted in the previous deliverable.
3. [x] Make the start and end delimiters for block comments configurable.

---

Affects: 3.1.1

Issue Links:

• 16310 Support EOF as statement separator in SQL scripts ("is depended on by")

• 16184 SQL Scripts with newline separators no longer work

• 14616 Support comments within SQL statements in JdbcTestUtils

• 14708 Support comments within SQL statements in ResourceDatabasePopulator

• 20450 H2 aliases are not supported in SQL ScriptUtils

• 16144 Support custom comment prefix in JdbcTestUtils.executeSqlScript()

• 16188 Introduce SQL script exception hierarchy

Referenced from: commits https://github.com/spring-projects/spring-framework/commit/01b2f67f11207c5007164518a83fcfb546b36837, https://github.com/spring-projects/spring-framework/commit/7853e74e786efda8aea70a5a0e79385a0c5d5cc9, https://github.com/spring-projects/spring-framework/commit/9230b38aea41316d78dc2f1bb206ce7a92b9ca56, https://github.com/spring-projects/spring-framework/commit/cc0ae3a881dde5910ecc340fa20c2365dd7474fa, https://github.com/spring-projects/spring-framework/commit/fbd25467c47fcda1e4d500e1cc3ede3d8ca7f61e, https://github.com/spring-projects/spring-framework/commit/bb67cd4657ee288d9a0a80c8e81f24308a6b1c0a, https://github.com/spring-projects/spring-framework/commit/2bfd6ddcf4de600942a7564a43a338febd4267f2, https://github.com/spring-projects/spring-framework/commit/e5c17560db71996157f52baada68c8a294c95d10

2 votes, 10 watchers

[spring-projects-issues]

John McCarthy commented

+1 Ran into this as well. I had block comments with contractions in them, which were interpreted as delineating literals. As a result, the script wasn't correctly split, leading to multiple statements being combined and execution errors.

Looks like ResourceDatabasePopulator.splitSqlScript() is the method in particular that should be updated to handle comments.

For example, in a script run against mysql 5.5,

/* let's create 2 tables */
create table foo(id int);    
create table bar(id int);

This fails because the apostrophe in "let's" is considered as starting a literal, when it should be ignored because it's in a comment. As a result, both statements (and the comment) are sent to the database as a single statement, which fails to execute.

Changing the script to remove apostrophes and semicolons in comments is a workaround. For example,

/* let us create 2 tables */
create table foo(id int);    
create table bar(id int);  

will work. You are probably already aware of this, but I thought I would clarify to be sure. Thanks!

[spring-projects-issues]

Chris Beams commented

Ian, with regard to single line (--) comments, this issue is a duplicate of #14708—please upgrade to Spring Framework 3.2.0.RELEASE and give it a try there if you haven't already.

John, ResourceDatabasePopulator contains no explicit support for multi-line comments. This should be straightforward enough to investigate and potentially implement, thus I've labeled the issue as pull-request-encouraged to let folks know this issue is a good candidate for contribution.

At the very least, we should document that SQL scripts containing multi-line comments (containing un-escaped delimiter chars) are not supported.

[spring-projects-issues]

Sam Brannen commented

This issue has been updated to refer to both ResourceDatabasePopulator and JdbcTestUtils since any changes to ResourceDatabasePopulator would need to be applied similarly in JdbcTestUtils.

[spring-projects-issues]

John McCarthy commented

Thanks Chris and Sam. I would like to contribute and will look into this. I'm not familiar with JdbcTestUtils but I see the similarities. I don't understand the dependencies in spring-test. Would it be alright if I created a sql script utility class in spring-jdbc that JdbcTestUtils could delegate to?

[spring-projects-issues]

Sam Brannen commented

John McCarthy, that's a very good question.

Historically speaking, the code in JdbcTestUtils existed first (i.e., since Spring 2.5.4), and that code was later duplicated in ResourceDatabasePopulator in Spring 3.0. Since then, the two have evolved independently, to an extent. However, for the sake of eliminating duplication in the framework it would be nice to extract the common functionality into a utility or component in spring-jdbc that both JdbcTestUtils and ResourceDatabasePopulator can delegate to.

Note that there are perhaps some minor challenges related to char vs. String for various method arguments in internal methods.

So have at it! ;)

Cheers,

Sam

[spring-projects-issues]

Chris Baldwin commented

Seeing as there has not been any activity on this issue for over a year I thought I would jump in and take a look. My first step was to create a test case that would demonstrate the issue described. However, I have hit a wall as the default embedded DB used for testing, the Hypersonic DB, ignores comments in the SQL statements. A quick Google search seems to confirm that the other two supported embedded DBs, H2 and Derby, also behave this way. So even though I can create a script that results in multi-line comments being used in the DB statement this does not cause an error or keep the SQL from successful execution.

So I am not entirely sure how to precede from here. I could work on a solution to this and use debugging to confirm that the SQL used in creating the DB statement does not contain the comments. I guess the test case can still be used to confirm that the stripping of the comments does not corrupt the rest of the valid SQL.

[spring-projects-issues]

Ian Wright commented

If it helps I saw this when using MySQL as the database

[spring-projects-issues]

Chris Baldwin commented

Thanks Ian,

That confirms that this could still be an issue in actual deployments but I am not sure if MySQL can be rolled into the test framework.

[spring-projects-issues]

Sam Brannen commented

Chris Baldwin,

I guess the test case can still be used to confirm that the stripping of the comments does not corrupt the rest of the valid SQL.

Yes, a simple unit test verifying that the multi-line comments are in fact stripped from the script should suffice.

For example, we already do something similar here: org.springframework.test.jdbc.JdbcTestUtilsTests.readAndSplitScriptContainingComments().

Writing unit tests against ResourceDatabasePopulator is more challenging (i.e., impossible without using reflection) since its readScript(EncodedResource) method is private.

As I mentioned in a previous comment, a secondary (though still very important) goal of this issue is to extract the common functionality into a utility or component in spring-jdbc that both JdbcTestUtils and ResourceDatabasePopulator can delegate to. This new component should logically be easy to unit test as well.

Regards,

Sam

[spring-projects-issues]

Sam Brannen commented

Please note that this issue is scheduled to be addressed in the upcoming Spring Framework 4.0.3 release.

Regards,

Sam

[spring-projects-issues]

Ian Wright commented

??but I am not sure if MySQL can be rolled into the test framework.??

Surely a test framework for generic database code should support, when available, at least MySQL (MariaDB), PostgreSQL, SQLServer, Oracle, DB2?

[spring-projects-issues]

Sam Brannen commented

Ian Wright, I think Chris Baldwin merely meant that it is not easy to write a small, self-contained integration test that executes against MySQL, since Spring currently only supports HSQL DB, H2, and Derby as embedded databases.

ResourceDatabasePopulator and JdbcTestUtils are in fact general purpose components and should therefore support such mainstream databases as you've mentioned. The challenge lies in testing against such databases, since Spring's test suite cannot be executed against external systems.

Regards,

Sam

[spring-projects-issues]

Chris Baldwin commented

Sam Brannen, that is exactly when I meant by my earlier comment. I certainly did not think exploring a new testing framework was within the scope of this issue.

[spring-projects-issues]

Chris Baldwin commented

Sébastien Deleuze, unless you have already made progress on this issue I am happy to continue working this issue using the following strategy:

• extend the existing org.springframework.jdbc.support.JdbcUtils class to include the common functionality
• create test cases using the existing ones from org.springframework.test.jdbc.JdbcTestUtilsTests as a pattern
  • this should allow us to write tests that do not rely on the embedded DBs to provide errors as I have mentioned that this does not actually work in the case of comments
• refactor both ResourseDatabasePopulator and JdbcTestUtils to use the new common JdbcUtils class

I have also noticed that many databases are also supporting Java style (//) line comments as well as the SQL (--) and C (/ ... /) style. Do we want to include the Java style as well?

[spring-projects-issues]

Sam Brannen commented

Regarding special support for Java style single line comments, this isn't actually necessary for ResourceDatabasePopulator since you can override the default (i.e., "--") via setCommentPrefix(String).

For JdbcTestUtils, it's only half necessary: JdbcTestUtils.readScript(LineNumberReader, String) already allows you to supply a custom commentPrefix. However, there is room for improvement, since there is currently no variant of executeSqlScript() that allows you to provide the commentPrefix. So this is actually a missing feature in the current implementation, and I'm surprised I hadn't noticed this before. We will address this shortcoming in #16144.

In any case, I would not make any attempt to provide special support for "//" single line comments in conjunction with this issue.

Regards,

Sam

[spring-projects-issues]

Sébastien Deleuze commented

Chris Baldwin I have not began to work on this issue yet, so feel free to contribute. If you need any help, don't hesitate to send a comment.

[spring-projects-issues]

Chris Baldwin commented

Sébastien Deleuze I have committed changes on my SPR-9531 branch that consolidate the common code and add the multi-line comment handling. Could you give it a quick look and if there is nothing obviously wrong then I can create the pull request, Thanks.

[spring-projects-issues]

Sam Brannen commented

Chris Baldwin, the changes in your branch look pretty good!

A few points:

• The remaining duplication in executeSqlScript() in both classes still needs to be moved to JdbcUtils.
• It is not permissible to delete public methods between releases due to backward compatibility concerns. Thus, the existing public static methods in JdbcTestUtils must remain intact and must now delegate to the moved code.
• Please rename the block comment constants to DEFAULT_BLOCK_COMMENT_START_DELIMITER and DEFAULT_BLOCK_COMMENT_END_DELIMITER.
• Analogous to commentPrefix, the start and end delimiters for block comments should be configurable in ResourceDatabasePopulator - for example, blockCommentStartDelimiter and blockCommentEndDelimiter.
• Don't forget to add @author tags for yourself in the classes you've modified.

Please make these changes, and we'll take another look.

Thanks,

Sam

[spring-projects-issues]

Sébastien Deleuze commented

Hi Chris Baldwin, thanks for this great contribution. I had a look to your commit at the same time than Sam Brannen, my feedbacks are mostly the same you will find them directly in your commit.

It could be also great if you could cleanup the commit history with interactive rebase and fix commit message typos.

Thanks

[spring-projects-issues]

Chris Baldwin commented

Sam Brannen,Sébastien Deleuze I have addressed the comments you provided in your reviews and the changes are ready for you to have another look.

[spring-projects-issues]

Sam Brannen commented

Hi Chris,

Thanks for making the changes.

Here are our latest findings:

1. There is a package cycle between org.springframework.jdbc.datasource.init and org.springframework.jdbc.support. To resolve this, we need to move the script parsing and execution code from JdbcUtils to a new component in the init package so that it lives alongside the CannotReadScriptException and ScriptStatementFailedException exceptions. This new component could potentially be named ScriptUtils.
2. The setBlockCommentDelimiters() method should be split into true JavaBean style properties: setBlockCommentStartDelimiter() and setBlockCommentEndDelimiter().
3. Anytime something is deprecated, we not only annotate the element with @Deprecated, but we also supply comments via the @deprecated Javadoc tag mentioning why the element is deprecated, since when, and what the recommended alternative is. For example: @deprecated as of Spring 4.0.3, in favor of using XXX (where XXX is a fully qualified Javadoc link to the new class or method).
4. StatementScriptStatementExecutor should either be a static class or an anonymous inner class within the populate() method, preferably the latter.

As for the necessity of the ScriptStatementExecutor callback interface, I am not yet decided. We can leave it intact for now, but I may later refactor JdbcTestUtlils so that this callback becomes superfluous.

Regards,

Sam

[spring-projects-issues]

Chris Baldwin commented

Hi Sam,

In reply to your first comment how about I change the executeSqlScript() method to use org.springframework.dao.DataAccessResourceFailureException instead of the other two exceptions? This was how the JdbcTestUtils version of the method worked. I ended up pulling the majority of the code from the ResourceDatabasePopulator version. This eliminates the need for any significant moving of code while removing the dependency on the org.springframework.jdbc.datasource.init package.

A little background on why I introduced the ScriptStatementExecutor interface. The two versions of the executeSqlScript() method were functionally identical except for what they would pass script SQL statements to for execution. The ResourceDatabasePopulator version just used a naked java.sql.Connection to create a java.sql.Statement while the JdbcTestUtils version used a JdbcTemplate object. To generalize the common method I introduced the interface so that callers could just create a simple wrapper object for which ever DB object type.

Thanks Chris

[spring-projects-issues]

Sam Brannen commented

In JdbcUtils, you have the following code:

// if there's no block comment close, we must be at the end
// of the script, so stop here.
break;

The absence of a closing delimiter does not imply that we are at the end of the file, but rather that a required delimiter is missing.

Thus, if there is no closing block comment delimiter, we should throw an exception, not silently ignore it.

[spring-projects-issues]

Sam Brannen commented

Hi Chris,

In reply to your first comment how about I change the executeSqlScript() method to use org.springframework.dao.DataAccessResourceFailureException instead of the other two exceptions?

That would unfortunately be inappropriate. CannotReadScriptException and ScriptStatementFailedException have become part of the API for ResourceDatabasePopulator. We therefore cannot simply throw them away. The fact that JdbcTestUtils currently does not throw these exceptions is not important. Backwards compatibility for production code (i.e., direct or indirect clients of ResourceDatabasePopulator) always trumps compatibility for test code.

Furthermore, the changes we are making are very specific to database initialization and execution of scripts, and the init package is already dedicated solely to these topics. As such, any related code should reside in the init package.

We need to finish up this work in time to review it and fine tune it for the upcoming 4.0.3 release. So feel free to make your final changes and submit a pull request. If you do not submit a pull request within the next few days, I will likely take it upon myself to complete this task based on our discussions here.

Cheers,

Sam

[spring-projects-issues]

Chris Baldwin commented

Pull request submitted.

[spring-projects-issues]

Sam Brannen commented

Resolved as described in the comments for...

GitHub commit e5c1756:

Support multi-line comments in SQL scripts

Prior to this commit neither ResourceDatabasePopulator nor JdbcTestUtils properly supported multi-line comments (e.g., / ... /). Secondarily there has developed a significant amount of code duplication in these two classes that has led to maintenance issues over the years.

This commit addresses these issues as follows:

• Common code has been extracted from ResourceDatabasePopulator and JdbcTestUtils and moved to a new ScriptUtils class in the spring-jdbc module.

• Relevant test cases have been migrated from JdbcTestUtilsTests to ScriptUtilsTests.

• ScriptUtils.splitSqlScript() has been modified to ignore multi-line comments in scripts during processing.

• ResourceDatabasePopulator supports configuration of the start and end delimiters for multi-line (block) comments.

• A new test case was added to ScriptUtilsTests for the new multi-line comment support.

and GitHub commit 2bfd6dd:

Refactor SQL script support

This commit continues the work in the previous commit as follows:

• Introduced an exception hierarchy for exceptions related to SQL scripts, with ScriptException as the base.

• CannotReadScriptException and ScriptStatementFailedException now extend ScriptException.

• Introduced ScriptParseException, used by ScriptUtils.splitSqlScript().

• DatabasePopulatorUtils.execute() now explicitly throws a DataAccessException.

• Polished Javadoc in ResourceDatabasePopulator.

• Overhauled Javadoc in ScriptUtils and documented all constants.

• Added missing @author tags for original authors in ScriptUtils and ScriptUtilsTests.

• ScriptUtils.splitSqlScript() now asserts preconditions.

• Deleted superfluous methods in ScriptUtils and changed method visibility to private or package private as appropriate.

• Deleted the ScriptStatementExecutor introduced in the previous commit; ScriptUtils.executeSqlScript() now accepts a JDBC Connection; JdbcTestUtils, AbstractTransactionalJUnit4SpringContextTests, and AbstractTransactionalTestNGSpringContextTests now use DatabasePopulatorUtils to execute a ResourceDatabasePopulator instead of executing a script directly via ScriptUtils.

• Introduced JdbcTestUtilsIntegrationTests.