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

SQL compound statement awareness in ScriptUtils #579

Merged
merged 4 commits into from
Dec 24, 2018

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Feb 8, 2018

This came out of #570. In brief, for DB init script handling, ScriptUtils is not capable of handling scripts that contain compound statements in stored procedures (BEGIN...END sections). It will simply split the script on semicolons, mid-procedure, thus breaking the script.

An example would be:

CREATE PROCEDURE count_foo()
    BEGIN
      SELECT *
        FROM bar;   -- <-- ScriptUtils will split here, which is wrong
      SELECT 1
        FROM dual;
    END; -- <-- statement should end here

This prevents users from having stored procedures in their init scripts, which feels like an unpleasant limitation.

The Spring developers behind ScriptUtils chose not to go down the path of parsing the SQL, and I can see why they would do this. For arbitrary scripts and many SQL dialects, this could be quite open ended.

However, I wonder if we can still improve on this just enough to be useful with a small change, so have created this WIP PR. I'm not intending for this to be the solution to all possible problems, but hope it unlocks us in some ways.

As-is, this:

  • won't split statements in between BEGIN/END tokens
  • can deal with nested blocks
  • seems to be OK with BEGIN/END appearing in string literals or comments

I don't believe this could create any regressions in its current form, and I believe BEGIN/END support is all we need to improve things greatly.

@rnorth rnorth changed the title WIP: SQL compound statement awareness in ScriptUtils SQL compound statement awareness in ScriptUtils Feb 24, 2018
@rnorth rnorth force-pushed the experimental-sql-compound-statement-awareness branch 2 times, most recently from ad24ba1 to 424141b Compare February 24, 2018 21:21
@rnorth rnorth requested a review from bsideup February 24, 2018 21:33
@jeacott1
Copy link

I don't think this fixes the problem, see my comment here:
#608 (comment)

@rnorth
Copy link
Member Author

rnorth commented Dec 16, 2018

@bsideup @kiview I'd like to resurrect this PR, as it is a quick win to enable better SQL init scripts for JDBC containers. Issues such as #946 and #570 will continue to catch people out, which is silly when we have a solution that works well enough for typical usage.

Please could you review?

For reference init_mysql.sql shows the kinds of syntax that this PR enables, and is included in this PR for testing.

@bsideup
Copy link
Member

bsideup commented Dec 17, 2018

@rnorth will do!

Prevents compound statements from being naively split by ScriptUtils
@rnorth rnorth force-pushed the experimental-sql-compound-statement-awareness branch from 424141b to f56b399 Compare December 17, 2018 11:52
@rnorth rnorth requested a review from kiview as a code owner December 17, 2018 11:52
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had some style remarks, actual logic seems fine for the specified case.

@rnorth rnorth changed the title SQL compound statement awareness in ScriptUtils WIP: SQL compound statement awareness in ScriptUtils Dec 17, 2018
@testcontainers testcontainers deleted a comment Dec 18, 2018
@testcontainers testcontainers deleted a comment Dec 18, 2018
@testcontainers testcontainers deleted a comment Dec 18, 2018
@testcontainers testcontainers deleted a comment Dec 18, 2018
@rnorth rnorth changed the title WIP: SQL compound statement awareness in ScriptUtils SQL compound statement awareness in ScriptUtils Dec 18, 2018
@rnorth rnorth force-pushed the experimental-sql-compound-statement-awareness branch from 6b5133b to 6045dd3 Compare December 18, 2018 09:37
@testcontainers testcontainers deleted a comment Dec 18, 2018
@testcontainers testcontainers deleted a comment Dec 18, 2018
@testcontainers testcontainers deleted a comment Dec 18, 2018
@testcontainers testcontainers deleted a comment Dec 18, 2018
…ScriptUtils.java

Co-Authored-By: rnorth <rich.north@gmail.com>
@rnorth rnorth added this to the next milestone Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@testcontainers testcontainers deleted a comment Dec 23, 2018
@rnorth rnorth merged commit 1d03cba into master Dec 24, 2018
@delete-merged-branch delete-merged-branch bot deleted the experimental-sql-compound-statement-awareness branch December 24, 2018 11:07
@rnorth
Copy link
Member Author

rnorth commented Dec 28, 2018

Released in 1.10.4!

@rnorth rnorth removed this from the next milestone Dec 28, 2018
@bsideup bsideup added this to the 1.10.4 milestone Dec 28, 2018
@manikmagar
Copy link
Contributor

manikmagar commented Dec 28, 2018

Thanks @rnorth, we ran into this issue with postgres a few weeks back. We will try it with latest version!

@johnhenaot
Copy link

johnhenaot commented Nov 20, 2019

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 $$;

Instead of using the BEGIN - END block to ignore ;, I think the solution is to ignore ; whenever is inside $$-$$

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

Successfully merging this pull request may close these issues.

None yet

6 participants