Uploaded image for project: 'Derby'
  1. Derby
  2. DERBY-4748

StringIndexOutOfBoundsException on syntax error (invalid COMMIT)

Details

    • Normal
    • 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)
      ....

      Attachments

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

        Activity

          Thanks for the report.

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

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

          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.

          kristwaa 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.

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

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

          knutanders 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.

          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;

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

          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.

          kristwaa 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.

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

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

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

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

          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.

          kristwaa 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.

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

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

          People

            kristwaa Kristian Waagan
            sdfelts Stephen Felts
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: