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

fix bug in splitSqlScript #7646

Merged
merged 8 commits into from
Oct 30, 2023
Merged

Conversation

inponomarev
Copy link
Contributor

@inponomarev inponomarev commented Oct 10, 2023

Fix #3316
Fix #4441

Problem

Current version of ScriptUtils.splitSqlScript tries to take into account BEGIN...END blocks. However, it treats any END as an end of the block, even if it's END IF or END LOOP, so that the following one-statement script

BEGIN
     rec_loop: LOOP
         FETCH blah;
         IF something_wrong THEN LEAVE rec_loop; END IF;
         do_something_else;
     END LOOP;
END;

Is incorrectly split into following "statements":

BEGIN
    rec_loop: LOOP
        FETCH blah;
        IF something_wrong THEN LEAVE rec_loop; END IF
-----------------------------------------------------------------
       do_something_else
-----------------------------------------------------------------
   END LOOP
-----------------------------------------------------------------
END

This is also broken for the case when a special statement separator is used (like GO in MSSQL tradition, / in Oracle tradition or @ in DB2).

The following statement is not split over @ sign as when it comes to the separator, the algorithm counted more ENDs than BEGINs and and since the balance is not zero, it is not splitting the code:

BEGIN
    rec_loop: LOOP
        FETCH blah;
        IF something_wrong THEN LEAVE rec_loop; END IF;
        do_something_else;
    END LOOP;
END;
@
CALL something();
@

Solution

Apparently, the "true end of block" must be END immediately followed by a separator (with probably whitespace and/or comments preceding the separator).

The current implementation of splitSqlScript is poorly written and it's difficult to extend "syntax rules". So I have completely rewritten it in a style of a standard parser:

1 "Lexical analysis" is perfromed in ScriptScanner class. This class retrieves next id, next comment, next string literal etc.
2 "Syntactic rules" are implemented in ScriptSplitter whis is just a recursive procedural parser.

@kiview
Copy link
Member

kiview commented Oct 11, 2023

Hey @inponomarev, thanks for your PR, but unfortunately we will have to close it.

ScriptUtils has been mostly taken from the Spring-JDBC project as a convenient feature and we agree that it can't support all use cases and all SQL syntax variants. We also agree with your assessment regarding the difficulty to extend and maintain it. But that's precisely the reason why we don't want to invest further in this feature or refactoring it extensively.

Instead, we recommend to use the mechanisms provided by the vendor specific images, which mostly means using withCopyFileToContainer() and a specific path for init scripts. I am pretty sure it should also work for the use case you have in mind.

In the future, we'd suggest to reach out to us first via Slack or GH discussions, to avoid unnecessary work on features that won't get merged.

@kiview kiview closed this Oct 11, 2023
@kiview kiview reopened this Oct 13, 2023
@kiview
Copy link
Member

kiview commented Oct 13, 2023

Re-opened after private discussion in Slack.

@eddumelendez eddumelendez added this to the next milestone Oct 13, 2023
kiview
kiview previously approved these changes Oct 23, 2023
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.

Can you PTAL @eddumelendez?

…ScriptScanner.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez eddumelendez merged commit 9cfa166 into testcontainers:main Oct 30, 2023
87 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @inponomarev !

inponomarev added a commit to inponomarev/testcontainers-java that referenced this pull request Nov 16, 2023
eddumelendez added a commit that referenced this pull request Nov 17, 2023
…L identifiers (as introduced by #7646) (#7818)

Co-authored-by: Eddú Meléndez Gonzales <eddu.melendez@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants