Merge pull request #2559 from hgschmie/trailing-semicolon-switch

Make trailing semicolons in scripts configurable

RELEASE_NOTES.md

@@ -1,5 +1,8 @@
 # Unreleased
 
+- finally give up on trying to guess SQL script parsing and add a switch to control whether to strip trailing semicolons or not. Another attempt to
+  fix SQL script parsing is (reported by @IrinaTerlizhenko, @2554).
+
 # 3.42.0
 
 - Add Spring Jdbi repositories with `@EnableJdbiRepositories`, thanks @xfredk (#2528, #2544)

core/src/main/java/org/jdbi/v3/core/internal/SqlScriptParser.java

@@ -88,12 +88,18 @@ public interface TokenHandler {
     }
 
     public static final class ScriptTokenHandler implements TokenHandler {
+        private final boolean requireSemicolon;
 
         private final List<String> statements = new ArrayList<>();
+
         private Token lastToken = null;
 
         private int blockLevel = 0;
 
+        public ScriptTokenHandler(boolean requireSemicolon) {
+            this.requireSemicolon = requireSemicolon;
+        }
+
         @Override
         public void handle(Token t, StringBuilder sb) {
             switch (t.getType()) {
@@ -111,7 +117,8 @@ public void handle(Token t, StringBuilder sb) {
                     break;
                 case SEMICOLON:
                     if (blockLevel == 0) {
-                        if (lastToken != null && lastToken.getType() == BLOCK_END) {
+                        if (requireSemicolon
+                            && (lastToken != null && lastToken.getType() == BLOCK_END)) {
                             sb.append(t.getText());
                         }
                         addStatement(sb.toString());

core/src/main/java/org/jdbi/v3/core/statement/Script.java

@@ -24,8 +24,11 @@
  * Represents a number of SQL statements delimited by semicolon which will be executed in order in a batch statement.
  */
 public class Script extends SqlStatement<Script> {
+    private final boolean requireSemicolon;
+
     public Script(Handle handle, CharSequence sql) {
         super(handle, sql);
+        this.requireSemicolon = handle.getConfig(SqlStatements.class).isScriptStatementsNeedSemicolon();
     }
 
     /**
@@ -35,6 +38,7 @@ public Script(Handle handle, CharSequence sql) {
      */
     public Script(Handle handle, String sql) {
         super(handle, sql);
+        this.requireSemicolon = handle.getConfig(SqlStatements.class).isScriptStatementsNeedSemicolon();
     }
 
     /**
@@ -64,11 +68,12 @@ public void executeAsSeparateStatements() {
      * @return the split statements
      */
     public List<String> getStatements() {
-        return splitToStatements(getConfig(SqlStatements.class).getTemplateEngine().render(getSql(), getContext()));
+        var templateEngine = getConfig(SqlStatements.class).getTemplateEngine();
+        return splitToStatements(templateEngine.render(getSql(), getContext()));
     }
 
     private List<String> splitToStatements(String script) {
-        ScriptTokenHandler scriptTokenHandler = new ScriptTokenHandler();
+        ScriptTokenHandler scriptTokenHandler = new ScriptTokenHandler(this.requireSemicolon);
         String lastStatement = new SqlScriptParser(scriptTokenHandler)
             .parse(CharStreams.fromString(script));
 

core/src/main/java/org/jdbi/v3/core/statement/SqlStatements.java

@@ -55,6 +55,7 @@ public final class SqlStatements implements JdbiConfig<SqlStatements> {
     private boolean allowUnusedBindings;
     private boolean attachAllStatementsForCleanup;
     private boolean attachCallbackStatementsForCleanup = true;
+    private boolean scriptStatementsNeedSemicolon = true;
     private final Collection<StatementCustomizer> customizers;
 
     private final Collection<StatementContextListener> contextListeners;
@@ -79,6 +80,7 @@ private SqlStatements(SqlStatements that) {
         this.allowUnusedBindings = that.allowUnusedBindings;
         this.attachAllStatementsForCleanup = that.attachAllStatementsForCleanup;
         this.attachCallbackStatementsForCleanup = that.attachCallbackStatementsForCleanup;
+        this.scriptStatementsNeedSemicolon = that.scriptStatementsNeedSemicolon;
         this.customizers = new CopyOnWriteArrayList<>(that.customizers);
         this.contextListeners = new CopyOnWriteArraySet<>(that.contextListeners);
         this.templateCache = that.templateCache;
@@ -323,6 +325,36 @@ public boolean isAttachCallbackStatementsForCleanup() {
         return attachCallbackStatementsForCleanup;
     }
 
+    /**
+     * If true, script statements parsed by a {@link Script} object will have a trailing semicolon. Some
+     * databases (e.g. Oracle) require that trailing semicolon while others (e.g. MySQL) do not and may consider
+     * it a syntax error. If executing a script against the database results in syntax errors that point at semicolons,
+     * change the value of this setting.
+     * <br>
+     * The default setting is {@code true} for historical reasons.
+     *
+     * @param scriptStatementsNeedSemicolon If true, parsed statements will have a trailing semicolon.
+     *
+     * @since 3.43.0
+     */
+    public void setScriptStatementsNeedSemicolon(boolean scriptStatementsNeedSemicolon) {
+        this.scriptStatementsNeedSemicolon = scriptStatementsNeedSemicolon;
+    }
+
+    /**
+     * If true, script statements parsed by a {@link Script} object will have a trailing semicolon. Some
+     * databases (e.g. Oracle) require that trailing semicolon while others (e.g. MySQL) do not and may consider
+     * it a syntax error. If executing a script against the database results in syntax errors that point at semicolons,
+     * change the value of this setting.
+     *
+     * @return True if parsed statements will have a trailing semicolon.
+     *
+     * @since 3.43.0
+     */
+    public boolean isScriptStatementsNeedSemicolon() {
+        return scriptStatementsNeedSemicolon;
+    }
+
     /**
      * Sets whether statements created within the {@link Jdbi#withHandle}, {@link Jdbi#useHandle}, {@link Jdbi#inTransaction} and {@link Jdbi#useTransaction}
      * callback methods will automatically attached to the {@link Handle} object and therefore cleaned up when the callback ends. The default is true.

core/src/test/java/org/jdbi/v3/core/statement/TestScript.java

@@ -13,17 +13,14 @@
  */
 package org.jdbi.v3.core.statement;
 
-import java.sql.SQLException;
 import java.util.List;
 import java.util.Map;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import de.softwareforge.testing.postgres.junit5.EmbeddedPgExtension;
 import de.softwareforge.testing.postgres.junit5.MultiDatabaseBuilder;
-import org.assertj.core.api.Condition;
 import org.jdbi.v3.core.Handle;
-import org.jdbi.v3.core.HandleAccess;
 import org.jdbi.v3.core.junit5.H2DatabaseExtension;
 import org.jdbi.v3.core.junit5.PgDatabaseExtension;
 import org.jdbi.v3.core.locator.ClasspathSqlLocator;
@@ -112,34 +109,37 @@ public void testPostgresJsonExtractTextOperator() {
         }
     }
 
-    /**
-     * <p>
-     *   Test for correct handling of semicolons in sql containing begin/end blocks.
-     * </p>
-     * <p>
-     *   Class {@link Script} splits sql scripts into lists of statements by semicolon ({@code ;}) and then batch-executes them.<br>
-     *   Statements may contain {@code BEGIN...END} blocks containing subordinated statements (also ending in semicolons).<br>
-     *   Only semicolons on the highest level (i.e. outside any block) actually signal the end of an sql statement.
-     * </p>
-     * @throws SQLException on failure to create the database handle
-     */
     @Test
-    public void testOracleScriptWithBeginEndBlock() throws SQLException {
-        String sql = getClasspathSqlLocator().getResource("script/oracle-with-begin-end-blocks.sql");
-        try (Script script = new Script(HandleAccess.createHandle(), sql)) {
+    public void testOracleScript() {
+        Handle h = h2Extension.getSharedHandle();
+        String sql = getClasspathSqlLocator().getResource("script/oracle-issue-2021.sql");
+
+        try (Script script = new Script(h, sql)) {
+
+            List<String> statements = script.getStatements();
+
+            assertThat(statements).hasSize(3);
+
+            for (String statement : statements) {
+                assertThat(statement).doesNotEndWithIgnoringCase("end");
+            }
+        }
+    }
+
+    @Test
+    public void testMySQLScript() {
+        Handle h = h2Extension.getSharedHandle();
+        h.getConfig(SqlStatements.class).setScriptStatementsNeedSemicolon(false);
+        String sql = getClasspathSqlLocator().getResource("script/oracle-issue-2021.sql");
 
+        try (Script script = new Script(h, sql)) {
             List<String> statements = script.getStatements();
 
             assertThat(statements).hasSize(3);
 
-            // local variable of CharSequence not String because
-            // CharSequence.chars() since Java 1.8 <=> String.chars() since Java 9
-            CharSequence lastStmt = statements.get(2);
-            assertThat(lastStmt)
-                    .startsWith("CREATE OR REPLACE TRIGGER EXAMPLE_TRIGGER")
-                    .endsWith("END;")
-                    .hasLineCount(15)
-                    .has(new Condition<>(s -> 7 == s.chars().filter(ch -> ch == ';').count(), "count semicolons"));
+            for (String statement : statements) {
+                assertThat(statement).doesNotEndWithIgnoringCase("end;");
+            }
         }
     }
 

docs/src/adoc/index.adoc

@@ -584,7 +584,8 @@ Any database operation within Jdbi uses a statement. Multiple types of statement
 * link:{jdbidocs}/core/statement/Update.html[Update^] - SQL statements that return no value such as `INSERT`, `UPDATE`, `DELETE` or a DDL operation. See link:{jdbidocs}/core/Handle.html#createUpdate(java.lang.CharSequence)[createUpdate^]
 * link:{jdbidocs}/core/statement/Batch.html[Batch^] - a set of operations that are executed as a batch. See link:{jdbidocs}/core/Handle.html#createBatch()[createBatch^]
 * link:{jdbidocs}/core/statement/Call.html[Call^] - execute a stored procedure. See link:{jdbidocs}/core/Handle.html#createCall(java.lang.CharSequence)[createCall^]
-* link:{jdbidocs}/core/statement/Script.html[Script^] - a SQL script containing multiple statements separated by `;` that is executed as a batch. See link:{jdbidocs}/core/Handle.html#createScript(java.lang.CharSequence)[createScript^]
+* link:{jdbidocs}/core/statement/Script.html[Script^] - a SQL script containing multiple statements separated by `;` that is executed as a batch. See also link:{jdbidocs}/core/Handle.html#createScript(java.lang.CharSequence)[createScript^]
+* link:{jdbidocs}/core/statement/Metadata.html[Metadata^] - SQL Metadata access.
 
 
 ==== Queries
@@ -883,6 +884,22 @@ can be executed in a single *Batch* or individually.
 include::{exampledir}/StatementsTest.java[tags=script]
 ----
 
+[NOTE]
+Script parsing uses a generic SQL parser and creates separate statements. Depending on the database and its driver, there are small differences in how the script needs to be parsed. The most notorious issue is whether the parsed statements need to retain a trailing semicolon or not. Some databases (e.g. Oracle) require the trailing semicolon while others (e.g. MySQL) actually report a syntax error if it is present. As we can not please everyone equally, there is the link:{jdbidocs}/core/statement/SqlStatements.html#setScriptStatementsNeedSemicolon(boolean)[setScriptStatementsNeedSemicolon()^] switch, which controls this behavior. By default, Jdbi *retains the semicolon if present*.
+
+
+Turning off the trailing semicolons for databases that have problems with it:
+[source,java,indent=0]
+----
+jdbi.withHandle(h -> {
+    // turn off trailing semicolons for script statements
+    h.getConfig(SqlStatements.class).setScriptStatementsNeedSemicolon(false);
+    return h.createScript(ClasspathSqlLocator.removingComments()
+            .getResource("scripts/mysql-script.sql"))
+        .execute();
+    });
+----
+
 
 ==== Metadata
 
@@ -3400,7 +3417,7 @@ Custom strategies can be set by implementing link:{jdkdocs}/java/util/function/U
 ! link:{jdbidocs}/core/array/SqlArrayArgumentStrategy.html#OBJECT_ARRAY[OBJECT_ARRAY^] ! call link:{jdkdocs}/java/sql/PreparedStatement.html#setObject-int-java.lang.Object-[PreparedStatement#setObject^] and assume that the driver can handle this.
 !===
 
-.7+| link:{jdbidocs}/core/statement/SqlStatements.html[SqlStatements^]     | attachAllStatementsForCleanup
+.8+| link:{jdbidocs}/core/statement/SqlStatements.html[SqlStatements^]     | attachAllStatementsForCleanup
     | boolean | `false`
     | Jdbi supports automatic resource management by attaching statements to their handle so that closing the handle will free up all its resources.
 If this setting is `true`, then statements are attached by default.
@@ -3415,6 +3432,11 @@ If this setting is `true`, then statements are attached by default.
 <| Sets the query timeout value in seconds. This value is used to call link:{jdkdocs}/java/sql/Statement.html#setQueryTimeout-int-[Statement#setQueryTimeout^]. Enforcement of the timeout depends on the JDBC driver.
 
 
+| scriptStatementsNeedSemicolon
+^| boolean ^| `true`
+<| Controls whether the statements parsed in a link:{jdbidocs}/core/statements/Script.html[Script^] object have trailing semicolons or not. This fixes some issues with specific JDBC drivers (e.g. MySQL when using `rewriteBatchedStatements=true`).
+
+
 <| sqlParser
 ^| link:{jdbidocs}/core/statement/SqlParser.html[SqlParser^] ^| link:{jdbidocs}/core/statement/ColonPrefixSqlParser.html[ColonPrefixSqlParser^]
 <a| The parser used to find the placeholders to bind arguments to.

mysql/src/test/java/org/jdbi/v3/mysql/TestScript.java

@@ -0,0 +1,76 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jdbi.v3.mysql;
+
+import java.util.List;
+
+import org.jdbi.v3.core.Handle;
+import org.jdbi.v3.core.locator.ClasspathSqlLocator;
+import org.jdbi.v3.core.statement.Script;
+import org.jdbi.v3.core.statement.SqlStatements;
+import org.jdbi.v3.testing.junit5.JdbiExtension;
+import org.jdbi.v3.testing.junit5.tc.JdbiTestcontainersExtension;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.testcontainers.containers.JdbcDatabaseContainer;
+import org.testcontainers.containers.MySQLContainer;
+import org.testcontainers.junit.jupiter.Container;
+import org.testcontainers.junit.jupiter.Testcontainers;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+@Tag("slow")
+@Testcontainers
+public class TestScript {
+    @Container
+    static JdbcDatabaseContainer<?> dbContainer = new MySQLContainer<>("mysql").withUrlParam("rewriteBatchedStatements", "true");
+
+    @RegisterExtension
+    JdbiExtension extension = JdbiTestcontainersExtension.instance(dbContainer);
+
+    @BeforeEach
+    public void setUp() {
+        extension.getSharedHandle().getConfig(SqlStatements.class).setScriptStatementsNeedSemicolon(false);
+    }
+
+    @Test
+    void testIssue2554Script() {
+        Handle h = extension.getSharedHandle();
+        String sql = ClasspathSqlLocator.removingComments()
+            .getResource("scripts/test-issue-2554.sql");
+        try (Script script = new Script(h, sql)) {
+            List<String> statements = script.getStatements();
+            assertThat(statements).hasSize(5);
+
+            for (String statement : statements) {
+                assertThat(statement).doesNotEndWith(";");
+            }
+        }
+    }
+
+    @Test
+    void testIssue2554() {
+        Handle h = extension.getSharedHandle();
+        String sql = ClasspathSqlLocator.removingComments()
+            .getResource("scripts/test-issue-2554.sql");
+        try (Script script = new Script(h, sql)) {
+            int[] outcome = script.execute();
+            assertThat(outcome)
+                .hasSize(5)
+                .containsExactly(0, -1, -1, -1, -1);
+        }
+    }
+}

mysql/src/test/resources/scripts/test-issue-2554.sql

@@ -0,0 +1,22 @@
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+CREATE PROCEDURE QWE()
+BEGIN
+END;
+
+SELECT 1 FROM DUAL;
+SELECT 1 FROM DUAL;
+SELECT 1 FROM DUAL;
+SELECT 1 FROM DUAL;