Derby
  1. Derby
  2. DERBY-5157

Incomplete quoting of SQL identifiers in AlterTableConstantAction

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.7.1.1
    • Fix Version/s: 10.8.1.2
    • Component/s: SQL
    • Labels:
      None
    • Issue & fix info:
      Repro attached

      Description

      AlterTableConstantAction generates SQL statements various places. Identifiers (schema names, table names, column names) are surrounded with double quotes in case they contain special characters. This is not enough if the identifiers contain double quotes, as can be seen with this example:

      ij> create table t(x int);
      0 rows inserted/updated/deleted
      ij> alter table t add column """" int default 42;
      ERROR 42X01: Syntax error: Encountered "\"" at line 1, column 22.

      I've found three places where AlterTableConstantAction generates SQL statements: updateNewColumnToDefault(), updateNewAutoincrementColumn(), getColumnMax(). All three places suffer from this problem.

      1. derby-5157-1a.diff
        4 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          I think the code in updateNewAutoincrementColumn() isn't currently in use because of DERBY-3888.

          The bug in updateNewColumnToDefault() is shown by the ALTER TABLE statement in the bug description.

          The problem in getColumnMax() can be exposed by executing these statements:

          ij> create table t ("""" int generated always as identity);
          0 rows inserted/updated/deleted
          ij> alter table t alter column """" set increment by 2;
          ERROR 42X01: Syntax error: Encountered "APP" at line 1, column 22.

          Show
          Knut Anders Hatlen added a comment - I think the code in updateNewAutoincrementColumn() isn't currently in use because of DERBY-3888 . The bug in updateNewColumnToDefault() is shown by the ALTER TABLE statement in the bug description. The problem in getColumnMax() can be exposed by executing these statements: ij> create table t ("""" int generated always as identity); 0 rows inserted/updated/deleted ij> alter table t alter column """" set increment by 2; ERROR 42X01: Syntax error: Encountered "APP" at line 1, column 22.
          Hide
          Knut Anders Hatlen added a comment -

          The attached patch fixes the problems by using the helper methods in IdUtil and StringUtil to do the quoting properly. It also adds test cases to verify the fixes (except the one in updateNewAutoincrementColumn() since that code isn't currently in use).

          Show
          Knut Anders Hatlen added a comment - The attached patch fixes the problems by using the helper methods in IdUtil and StringUtil to do the quoting properly. It also adds test cases to verify the fixes (except the one in updateNewAutoincrementColumn() since that code isn't currently in use).
          Hide
          Dag H. Wanvik added a comment -

          Patch looks good. I verified that the new test cases fail without the patch. Well, actually the first one did ("testDerby5157_addColumnWithDefaultValue"). The second one "testDerby5157_changeIncrement" subsequently gave a lock timeout in the "CREATE SCHEMA".

          With the patch the tests ran fine.

          +1

          Show
          Dag H. Wanvik added a comment - Patch looks good. I verified that the new test cases fail without the patch. Well, actually the first one did ("testDerby5157_addColumnWithDefaultValue"). The second one "testDerby5157_changeIncrement" subsequently gave a lock timeout in the "CREATE SCHEMA". With the patch the tests ran fine. +1
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for looking at the patch, Dag. I see a lock timeout in the second test case as well if I back out the fix. But if I only back out the fix in getColumnMax(), so that the first test case passes, the second one fails with the expected syntax error, so I think it does its job. I'm a bit puzzled by the timeout exception, though. The transaction should be rolled back by BaseJDBCTestCase.tearDown() and release the locks, but for some reason rollback() fails with "ERROR X0Y67: Cannot issue rollback in a nested connection when there is a pending operation in the parent connection." This isn't a problem introduced by the fix, so I'll go ahead committing the patch. I'll do some more digging and file a separate issue for the rollback problem if it looks like there is a bug.

          Show
          Knut Anders Hatlen added a comment - Thanks for looking at the patch, Dag. I see a lock timeout in the second test case as well if I back out the fix. But if I only back out the fix in getColumnMax(), so that the first test case passes, the second one fails with the expected syntax error, so I think it does its job. I'm a bit puzzled by the timeout exception, though. The transaction should be rolled back by BaseJDBCTestCase.tearDown() and release the locks, but for some reason rollback() fails with "ERROR X0Y67: Cannot issue rollback in a nested connection when there is a pending operation in the parent connection." This isn't a problem introduced by the fix, so I'll go ahead committing the patch. I'll do some more digging and file a separate issue for the rollback problem if it looks like there is a bug.
          Hide
          Knut Anders Hatlen added a comment -

          Logged DERBY-5161 for the rollback issue.

          Committed derby-5157-1a.diff to trunk with revision 1086526.

          Show
          Knut Anders Hatlen added a comment - Logged DERBY-5161 for the rollback issue. Committed derby-5157-1a.diff to trunk with revision 1086526.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development