Invalid parsing of SQL Init script containing procedures

[manikmagar]

I have a very simple init sql script for MySQL that is provided to TC JDBC URL using TC_INITSCRIPT parameter -

drop table if exists books;

CREATE TABLE books (
  book_id int(11) NOT NULL AUTO_INCREMENT,
  title varchar(45) NOT NULL,
  description varchar(45) NOT NULL,
  price decimal(10,0) NOT NULL,
  total_quantity int(11) NOT NULL,
  available_quantity int(11) NOT NULL,
  create_timestamp timestamp NULL DEFAULT NULL,
  PRIMARY KEY (book_id)
) ENGINE=InnoDB DEFAULT CHARSET=latin1;

CREATE PROCEDURE calculate_library_value(out totalValue double)
BEGIN

select sum(price) into totalValue from books;

END;

When TestContainer starts, ScriptUtils class splits the statements by default ; delimiter. This results in last CREATE PROCEDURE statement to break after inline ; and the SQL Statement created by utils looks like below which is incorrect and hence fails

CREATE PROCEDURE calculate_library_value(out totalValue double)
BEGIN

select sum(price) into totalValue from books

Exception:

Caused by: org.testcontainers.jdbc.ext.ScriptUtils$ScriptStatementFailedException: Script execution failed (library_db.sql:3): CREATE PROCEDURE calculate_library_value(out totalValue double) BEGIN select sum(price) into totalValue from books

Possible solution:
When ; inside procedure are escaped \; then CREATE PROCEDURE block is read completely but again the execution of that SQL fails because ; is an invalid delimiter at runtime. Before executing the statement, should \; be replaced with ;?

Or I am doing it wrong?

[manikmagar]

@rnorth @bsideup Any suggestions for this one, please? I have something coming up to demo TestContainers usage and being able to initialize with procedures is needed. I sent a PR #571 with a possible solution. Thanks!

[rnorth]

Hmm, sorry for not responding sooner. Our ScriptUtils class is simply stolen/borrowed from Spring Data, and I think this issue is the same problem: https://jira.spring.io/browse/SPR-13389

I just need to get my head around this; I'm not clear if there is a suitable workaround given there.

I'm afraid you've also been unlucky with timing; we merged a change earlier that moves ScriptUtils from one package to another, so you're likely to have some merge conflicts on your PR. Sorry for this.

[rnorth]

OK, none of the options in the Spring ticket sound like they'll work for us.

I've coded up a quick experiment to make ScriptUtils aware of BEGIN...END compound statements, and to treat them atomically. This actually works without any script modification, but I'm mildly hesitant and want to think this through. I don't want there to be a risk of breaking existing functionality, and it is potentially a risk here.

[manikmagar]

Thank you @rnorth for looking into this. Timing seems to be really bad for me :). I see you are considering the occurrences of BEGIN and END, I am not really sure how that might work for the nested BEGIN .. END occurrences. But for the plain simple procedure, that might work. Is there a snapshot version available with the code you added? I tried with the solution in my PR (local snapshot) and that worked for me. If you think that could be a viable solution, I can rebase my PR.

[rnorth]

I'm not sure; doesn't \; make the script technically invalid SQL, so therefore unusable anywhere that isn't Testcontainers? Also it looked like there were some test failures on your PR on CI: https://travis-ci.org/testcontainers/testcontainers-java/jobs/336400408

By detecting BEGIN/END blocks I've attempted to keep us fully compatible with unmodified SQL. I think/hope the way I've done it also handles nested blocks well enough too; to be sure, I've updated the init_mysql.sql test script to include a few more conditions that I think we need to guard against. WDYT?

[rnorth]

BTW if you could perhaps try it out, there's a jitpack.io build available:

You'll need the jitpack repository added to your POM:

<repositories>
	<repository>
	    <id>jitpack.io</id>
	    <url>https://jitpack.io</url>
	</repository>
</repositories>

and replace any testcontainers dependencies you have with this (leaving artifactId the same as before, though):

<dependency>
    <groupId>com.github.testcontainers.testcontainers-java</groupId>
    <artifactId>MODULE_NAME_GOES_HERE</artifactId>
    <version>experimental-sql-compound-statement-awareness-SNAPSHOT</version>
</dependency>

[manikmagar]

I think you are right @rnorth. Using escape character would make script unusable elsewhere. I would prefer to use the production-ready, unmodified scripts during testing. So, using escape literal is not a good solution.

I tried the jitpack version as you mentioned and it seemed to work perfectly.

Here is my modified version of test procedure script -

DROP PROCEDURE IF EXISTS calculate_library_value;

CREATE PROCEDURE `calculate_library_value`(out totalValue decimal)
BEGIN

	-- Total Value of Library books
	select sum(price * total_quantity) into totalValue from books;
	
	/*
		Also get all the books.
	*/

	BEGIN 
		select * from books order by title;
	END;

END;

There I intentionally added a nested BEGIN ... END inside procedure body. The new code parsed it correctly as well as it was created successfully 👍. So Thank you very much for that!

Now for my usage, I am not sure when this fix will be released as new TestContainers-Java version. But I am sure, I am not the only one who would love to have this one as soon as possible released :).

If you think this will take a while to release, then I am thinking to extract out ScriptUtils of 1.6.0 (like this) in my code, add this code fix in it (current extract code has escape literal fix, but would remove it) and run init script after db container is started (something like this). I hope that would be ok with you and of course I would switch to the new version as soon as it is released and let TC handle that initialization.

Thank you for the help!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.