Derby
  1. Derby
  2. DERBY-4748

StringIndexOutOfBoundsException on syntax error (invalid COMMIT)

    Details

    • Urgency:
      Normal
    • Bug behavior facts:
      Regression

      Description

      Start the network server on port 1527 on localhost.

      Run ant on the following:

      <project default="all">
      <property environment="env" />
      <path id="derby.classpath.id">
      <fileset dir="${env.DERBY_LIB">
      <include name="derbynet.jar" />
      <include name="derbyclient.jar" />
      </fileset>
      </path>

      <target name="all">
      <sql driver="org.apache.derby.jdbc.ClientDriver"

      url="jdbc:derby://localhost:1527/derbyDB;create=true;user=derbyuser;passsword=derbypwd"
      userid="derbyuser"
      password="derbypwd"
      classpathref="derby.classpath.id"
      onerror="continue">
      create table mytable1 (mycol varchar(255));
      commit;
      </sql>
      </target>
      </project>

      On 10.5.3, I get
      [sql] Executing commands
      [sql] Failed to execute: commit
      [sql] java.sql.SQLSyntaxErrorException: Syntax error: Encountered
      "commit"
      at line 1, column 1.
      [sql] 1 of 2 SQL statements executed successfully

      but on 10.6.1, I get
      [sql] Executing commands

      BUILD FAILED
      java.lang.StringIndexOutOfBoundsException: String index out of range: 6
      at java.lang.String.charAt(String.java:686)
      at org.apache.derby.client.am.Statement.isolateAnyInitialIdentifier(Unknown Source)
      at org.apache.derby.client.am.Statement.getStatementToken(Unknown Source)
      at org.apache.derby.client.am.Statement.parseSqlAndSetSqlModes(Unknown Source)
      ....

      1. derby-4748-1a-sioobe.diff
        2 kB
        Kristian Waagan
      2. derby-4748-1b-sioobe.diff
        3 kB
        Kristian Waagan

        Activity

        Hide
        Kristian Waagan added a comment -

        Thanks for the report.

        This bug was introduced under DERBY-4338. I'll have a look.

        Show
        Kristian Waagan added a comment - Thanks for the report. This bug was introduced under DERBY-4338 . I'll have a look.
        Hide
        Kristian Waagan added a comment -

        Attaching patch 1a, which changes the logic in the loop.
        I think there are many ways to write this logic/loop, and almost as many ways to get it wrong

        Regression tests passed.
        Patch ready for review.

        Show
        Kristian Waagan added a comment - Attaching patch 1a, which changes the logic in the loop. I think there are many ways to write this logic/loop, and almost as many ways to get it wrong Regression tests passed. Patch ready for review.
        Hide
        Knut Anders Hatlen added a comment -

        The fix looks correct to me. +1 to commit.

        Minor nit: The helper method assertCompileError() could be used to simplify the test case.

        Show
        Knut Anders Hatlen added a comment - The fix looks correct to me. +1 to commit. Minor nit: The helper method assertCompileError() could be used to simplify the test case.
        Hide
        Knut Anders Hatlen added a comment -

        By the way, I think the isolateAnyInitialIdentifier() method is somewhat more complex than it needs to be in the first place. Replacing it with something like the (untested) loop below would probably solve the out-of-bounds exception and also make the method easier to read since it has fewer special cases to check for.

        int idx;
        for (idx = 0; idx < sql.length(); idx++) {
        char ch = sql.charAt(idx);
        if (!Character.isLetter(ch))

        { // first non-token char found break; }

        }
        // return initial token if one is found, or the entire string otherwise
        return (idx > 0) ? sql.substring(0, idx) : sql;

        Show
        Knut Anders Hatlen added a comment - By the way, I think the isolateAnyInitialIdentifier() method is somewhat more complex than it needs to be in the first place. Replacing it with something like the (untested) loop below would probably solve the out-of-bounds exception and also make the method easier to read since it has fewer special cases to check for. int idx; for (idx = 0; idx < sql.length(); idx++) { char ch = sql.charAt(idx); if (!Character.isLetter(ch)) { // first non-token char found break; } } // return initial token if one is found, or the entire string otherwise return (idx > 0) ? sql.substring(0, idx) : sql;
        Hide
        Kristian Waagan added a comment -

        Thanks, Knut.
        That code looks better.

        I uploaded a new revision of the patch (1b), which uses your code and also includes some more changes to the test.
        Regression tests passed, I expect to commit it shortly.

        Show
        Kristian Waagan added a comment - Thanks, Knut. That code looks better. I uploaded a new revision of the patch (1b), which uses your code and also includes some more changes to the test. Regression tests passed, I expect to commit it shortly.
        Hide
        Knut Anders Hatlen added a comment -

        +1

        Show
        Knut Anders Hatlen added a comment - +1
        Hide
        Kristian Waagan added a comment -

        Committed patch 1b to trunk with revision 980684.
        Will backport before closing.

        Show
        Kristian Waagan added a comment - Committed patch 1b to trunk with revision 980684. Will backport before closing.
        Hide
        Kristian Waagan added a comment -

        Now I understand why I'm not supposed to resolve the issue before I have back-ported the fix

        Show
        Kristian Waagan added a comment - Now I understand why I'm not supposed to resolve the issue before I have back-ported the fix
        Hide
        Kristian Waagan added a comment -

        Backported to 10.6 with revision 984074, 10.5 with revision 984079, and finally to 10.4 with revision 984080. suites.All and derbyall ran without failures, with the exception of store/aes.sql on 10.4.

        Stephen, the issue is ready for verification and closing.

        Show
        Kristian Waagan added a comment - Backported to 10.6 with revision 984074, 10.5 with revision 984079, and finally to 10.4 with revision 984080. suites.All and derbyall ran without failures, with the exception of store/aes.sql on 10.4. Stephen, the issue is ready for verification and closing.
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Stephen Felts
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development