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. 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. Support multi-line SQL comments in the component extracted in the previous deliverable.
3. Make the start and end delimiters for block comments configurable.

---

Affects: 3.1.1

Issue Links:

• Support EOF as statement separator in SQL scripts [SPR-11687] #16310 Support EOF as statement separator in SQL scripts ("is depended on by")
• SQL Scripts with newline separators no longer work [SPR-11560] #16184 SQL Scripts with newline separators no longer work
• Support comments within SQL statements in JdbcTestUtils [SPR-9982] #14616 Support comments within SQL statements in JdbcTestUtils
• Support comments within SQL statements in ResourceDatabasePopulator [SPR-10075] #14708 Support comments within SQL statements in ResourceDatabasePopulator
• H2 aliases are not supported in SQL ScriptUtils [SPR-15896] #20450 H2 aliases are not supported in SQL ScriptUtils
• Support custom comment prefix in JdbcTestUtils.executeSqlScript() [SPR-11519] #16144 Support custom comment prefix in JdbcTestUtils.executeSqlScript()
• Introduce SQL script exception hierarchy [SPR-11564] #16188 Introduce SQL script exception hierarchy

Referenced from: commits 01b2f67, 7853e74, 9230b38, cc0ae3a, fbd2546, bb67cd4, 2bfd6dd, e5c1756

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