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

Invalid parsing of SQL Init script containing procedures #570

Closed
manikmagar opened this issue Feb 1, 2018 · 36 comments
Closed

Invalid parsing of SQL Init script containing procedures #570

manikmagar opened this issue Feb 1, 2018 · 36 comments

Comments

@manikmagar
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Member

rnorth commented Feb 3, 2018

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 added a commit that referenced this issue Feb 3, 2018
@rnorth
Copy link
Member

rnorth commented Feb 3, 2018

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
Copy link
Contributor Author

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
Copy link
Member

rnorth commented Feb 4, 2018

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
Copy link
Member

rnorth commented Feb 4, 2018

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
Copy link
Contributor Author

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!

rnorth added a commit that referenced this issue Feb 24, 2018
Prevents compound statements from being naively split by ScriptUtils
rnorth added a commit that referenced this issue Feb 24, 2018
Prevents compound statements from being naively split by ScriptUtils
@stale
Copy link

stale bot commented Oct 28, 2018

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.

@stale stale bot added the stale label Oct 28, 2018
@stale
Copy link

stale bot commented Nov 11, 2018

This issue has been automatically closed due to inactivity. We apologise if this is still an active problem for you, and would ask you to re-open the issue if this is the case.

@stale stale bot closed this as completed Nov 11, 2018
@rnorth
Copy link
Member

rnorth commented Nov 11, 2018

Reopening- this is still an issue.

@rnorth rnorth reopened this Nov 11, 2018
@stale stale bot removed the stale label Nov 11, 2018
rnorth added a commit that referenced this issue Dec 17, 2018
Prevents compound statements from being naively split by ScriptUtils
@rnorth rnorth closed this as completed in 1d03cba Dec 24, 2018
@rnorth
Copy link
Member

rnorth commented Dec 28, 2018

Released in 1.10.4!

rnorth pushed a commit that referenced this issue Mar 10, 2019
Bumps [neo4j-java-driver](https://github.com/neo4j/neo4j-java-driver) from 1.7.2 to 1.7.3.
<details>
<summary>Commits</summary>

- [`742d280`](neo4j/neo4j-java-driver@742d280) Merge pull request [#567](https://github-redirect.dependabot.com/neo4j/neo4j-java-driver/issues/567) from ali-ince/1.7-pass-access-mode
- [`49ef4db`](neo4j/neo4j-java-driver@49ef4db) Fixing wrong file header
- [`f66e194`](neo4j/neo4j-java-driver@f66e194) Merge pull request [#570](https://github-redirect.dependabot.com/neo4j/neo4j-java-driver/issues/570) from zhenlineo/1.7-flaky-test
- [`5b58956`](neo4j/neo4j-java-driver@5b58956) Fixing the flaky test that is caused by certificate file changes.
- [`f9a6fca`](neo4j/neo4j-java-driver@f9a6fca) Merge pull request [#569](https://github-redirect.dependabot.com/neo4j/neo4j-java-driver/issues/569) from michael-simons/fix-java-doc-trust-strategy
- [`bc68f0f`](neo4j/neo4j-java-driver@bc68f0f) Merge pull request [#566](https://github-redirect.dependabot.com/neo4j/neo4j-java-driver/issues/566) from zhenlineo/1.7-hostname-for-sni
- [`41949fc`](neo4j/neo4j-java-driver@41949fc) Fix JavaDoc of withTrustStrategy.
- [`227c0fc`](neo4j/neo4j-java-driver@227c0fc) Fix test failure
- [`11345c1`](neo4j/neo4j-java-driver@11345c1) Pass access mode as part of statement metadata
- [`e9e5a93`](neo4j/neo4j-java-driver@e9e5a93) Fix some test failures due to changes to routing table procedure on read-repl...
- Additional commits viewable in [compare view](neo4j/neo4j-java-driver@1.7.2...1.7.3)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=org.neo4j.driver:neo4j-java-driver&package-manager=gradle&previous-version=1.7.2&new-version=1.7.3)](https://dependabot.com/compatibility-score.html?dependency-name=org.neo4j.driver:neo4j-java-driver&package-manager=gradle&previous-version=1.7.2&new-version=1.7.3)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
If all status checks pass Dependabot will automatically merge this pull request.

[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
@Ordiel
Copy link

Ordiel commented Apr 24, 2019

Was this really solved... can someone verify it, I am using 1.11.2 and still getting this issue when trying to create procedures...

@kiview
Copy link
Member

kiview commented Apr 24, 2019

@Ordiel Can you please provide a non-working example script?

@Ordiel
Copy link

Ordiel commented May 2, 2019

Will try to roll back to that non-working version (I am using Liquibase anyway, so I thought "why not just get the connection and let Liquibase do the init?)

@jaimecasero
Copy link

Not working here. Running version 1.11.3 mysql testcontainers.

@batbayar-su
Copy link

Not working on 1.11.3 postgresql as well :(

@rnorth
Copy link
Member

rnorth commented Aug 8, 2019

1.12.0, released 15 days ago, includes a number of further fixes for SQL script splitting. This may resolve your problem - please try it.

If it does not, then please provide examples of scripts that fail - otherwise there is nothing we can do.

For what it's worth, @Ordiel's approach of using Liquibase (or Flyway, or any other DB migration tool) is sensible - these tools will always have stronger, more specialised SQL loading capabilities than we can afford to maintain.

@cdfeasy
Copy link

cdfeasy commented Aug 14, 2019

1.12.0 Not working with
CREATE FUNCTION procedure(val bigint, error_message_p text) RETURNS void
LANGUAGE plpgsql
AS
$BODY$
declare
r text;

BEGIN
...
code example
testcontainer-procedure-error.zip

@cdfeasy
Copy link

cdfeasy commented Aug 15, 2019

@rnorth
"IF foo is null THEN bar = '1' ; END IF;" in procedure body also not working
Similar with END LOOP; END CASE;

@cdfeasy
Copy link

cdfeasy commented Aug 16, 2019

@rnorth for my tests all work without init script splitting (fix in ScriptUtils)
code example
testcontainer_fixed_test.zip

@stephen-maina
Copy link

A workaround I found was creating my own implementation of PostgreSQLContainer and including a constructor that accepts Future<String>, then I could use builder.from("postgres:9.5-alpine").env("POSTGRES_USER", "app_user").expose(5433) .env("POSTGRES_DB", "test").env("POSTGRES_PASSWORD", "test").copy("init.sql", "/docker-entrypoint-initdb.d/") and include the init script. Hope this helps anyone else who comes by this post.

@bsideup
Copy link
Member

bsideup commented Aug 30, 2019

@stephen-maina

PostgreSQLContainer postgres = new PostgreSQLContainer<>();
postgres.setImage(future);

works too :)

@stephen-maina
Copy link

@bsideup Thanks for that, hehe, now my code will be leaner.

@johnhenaot
Copy link

Hi guys, I'm having some issues when using IF statement, it seems that it is assuming the word END in the "END IF" as the END of the BEGIN statement.

CREATE OR REPLACE PROCEDURE updatealladvertisers() LANGUAGE plpgsql AS $$ BEGIN IF (SELECT count(1) FROM information_schema.tables WHERE table_schema = 'public' AND table_name='all_advertisers') = 0 THEN CREATE TABLE IF NOT EXISTS public.all_advertisers ( partnerid BIGINT, partnername VARCHAR(255), partner VARCHAR(255), gsm_id BIGINT ); END IF; END $$;

@vipulnewaskar7
Copy link

Hi,
I am gettting similar issue for MS-SQL.
Can't we just "ask" database to run an init.sql file, instead of we(testcontainers) parsing sql file and execute??
So that, it will be end-user's responsibility to provide correct sql script.

@longstone
Copy link

longstone commented Aug 11, 2020

We're facing this issue with testcontainers-java and Postgres 11.4.
Is there a workaround?

@AleksandrRadchenko
Copy link

AleksandrRadchenko commented Aug 21, 2020

For those, who struggles with this issue and MS-SQL. As @vipulnewaskar7 suggested, I've tried to run init script from within container: .execInContainer("/opt/mssql-tools/bin/sqlcmd", "-S localhost -U SA -P 'A_Str0ng_Required_Password' -i /home/initMssql.sql"). This somehow causes TCP Provider: Error code 0x2AF9 error, while running the same command using native docker exec -i ... works fine. So the workaround is to run .sh script via .execInContainer API with sqlcmd inside:

#!/usr/bin/env sh
/opt/mssql-tools/bin/sqlcmd -S localhost -U SA -P $SA_PASSWORD -i /home/initMssql.sql

Finally, java-side config for singleton container looks like

    protected static final JdbcDatabaseContainer<?> MSSQL_CONTAINER;
    static {
        String localResourcesPath = "sql/";
        String dbInitScriptFileName = "initMssql.sh";
        String sqlInitScriptFileName = "initMssql.sql";
        MountableFile dbInitScript = MountableFile.forClasspathResource(localResourcesPath + dbInitScriptFileName);
        MountableFile sqlInitScript = MountableFile.forClasspathResource(localResourcesPath + sqlInitScriptFileName);
        String containerResourcesPath = "/home/";
        MSSQL_CONTAINER = new MSSQLServerContainer<>().withCopyFileToContainer(dbInitScript, containerResourcesPath)
                                                      .withCopyFileToContainer(sqlInitScript, containerResourcesPath);
        MSSQL_CONTAINER.start();
        try {
            MSSQL_CONTAINER.execInContainer("chmod", "+x", containerResourcesPath + dbInitScriptFileName);
            MSSQL_CONTAINER.execInContainer(containerResourcesPath + dbInitScriptFileName);
        } catch (IOException | InterruptedException e) {
            e.printStackTrace();
        }
    }

@gregturn
Copy link

I'm trying to run a postgres script and keep hitting this issue with Testcontainers 1.15.2...

@Bean
public DataSource dataSource() {

	if (POSTGRESQL_CONTAINER == null) {

		PostgreSQLContainer<?> container = new PostgreSQLContainer<>(parse("postgres").withTag("9.6.12")) //
				.withUsername("postgres") //
				.withInitScript("scripts/postgres-stored-procedures.sql");
		container.start();

		POSTGRESQL_CONTAINER = container;
	}

	PGSimpleDataSource dataSource = new PGSimpleDataSource();
	dataSource.setUrl(POSTGRESQL_CONTAINER.getJdbcUrl());
	dataSource.setUser(POSTGRESQL_CONTAINER.getUsername());
	dataSource.setPassword(POSTGRESQL_CONTAINER.getPassword());

	return dataSource;
}

This fails when I try to run a postgres script found at https://github.com/GabrielBB/spring-data-jpa-procedure-tests/blob/master/src/main/resources/postgres.sql.

Why, again, is the issue closed?

@gregturn
Copy link

The only thing I could get to work was this:

	@BeforeEach
	@Transactional
	void setUp() {

		Resource initScript = new ClassPathResource("scripts/postgres-stored-procedures.sql");
		JdbcTemplate jdbcTemplate = new JdbcTemplate(dataSource);

		try (Reader reader = new InputStreamReader(initScript.getInputStream(), StandardCharsets.UTF_8)) {
			jdbcTemplate.execute(FileCopyUtils.copyToString(reader));
		} catch (IOException e) {
			throw new RuntimeException(e);
		}
	}

This lets me grab an existing DataSource, open a JDBC connection, and avoid any intermediate parsers. Also lets my sidestep anything inside the Docker container.

@virtual-machinist
Copy link

With 1.16.0 having either the same or similar issue. When I do

  private static PostgreSQLContainer<?> postgresqlContainer = new PostgreSQLContainer<>("postgres:11")
      .withDatabaseName("somedb")
      .withUsername("postgres")
      .withPassword("somepassword")
      .withUrlParam("currentSchema", "someschema")
      .withInitScript("init-somedb.sql");

I get

Caused by: org.postgresql.util.PSQLException: Unterminated dollar quote started at position ... in SQL CREATE FUNCTION ...  Expected terminating $$
    at org.postgresql.core.Parser.checkParsePosition(Parser.java:1328)
    ...

But when I do

  private static PostgreSQLContainer<?> postgresqlContainer = new PostgreSQLContainer<>("postgres:11")
      .withCopyFileToContainer(MountableFile.forClasspathResource("init-somedb.sql"), "/docker-entrypoint-initdb.d/init.sql")
      .withDatabaseName("somedb")
      .withUsername("postgres")
      .withPassword("somepassword")
      .withUrlParam("currentSchema", "someschema");

Everything works as expected. The same file also works correctly with docker-compose.

@kiview
Copy link
Member

kiview commented Aug 19, 2021

The first example will use Testcontainers' ScriptUtils for executing the script, while the second example lets the database itself execute it.

There is always a chance for edge cases not being supported by ScriptUtils, can you provide a reproducer?

@virtual-machinist
Copy link

Yes, I understand that they work differently, but I'd argue it shouldn't matter to the end-user how is the database initialized, as long as it's done. 😉

withInitScript() fails with the following file:

CREATE FUNCTION some_function(i DOUBLE PRECISION) RETURNS void
    LANGUAGE plpgsql
    AS
$$
BEGIN
    EXECUTE pg_sleep(i);
END;
$$;

@kiview
Copy link
Member

kiview commented Aug 19, 2021

I can understand the argument from a user perspective, but from a maintainer perspective, Testcontainers simply can't support all SQL dialects to 100%. I think relying on the script execution by the mechanisms provided by the image (having a file in /docker-entrypoint-initdb.d/init.sql in this case) is a good approach.

Note that removing the semicolon after END makes the script work for me.

@virtual-machinist
Copy link

My example was purely synthetic obviously, and I have no control over the DDLs in the real init file, so I can't really use withInitScript() if it has such limitations.
I also understand it isn't easy to support every SQL dialect. However it would be a good idea to hint at possible problems and/or add a workaround recipe to the docs at least for PostgreSQL TC module. I imagine I won't be the only person having this issue. 😉

@kiview
Copy link
Member

kiview commented Aug 19, 2021

Since this feature of the PostgreSQL image is documented on its Docker Hub Page, I don't see the need to replicate this in the Testcontainers docs. However, I agree that it might make sense to add an info block to the Testcontainers docs that mentions possible limitations of using the provided init-script functions.

@antonovdmitriy
Copy link

this issue is still actual for oracle at least

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests