Derby
  1. Derby
  2. DERBY-1204

CREATE TRIGGER with an INSERT action statement with multiple rows and a referenced column throws a StringIndexOutOfBoundsException

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.1.2.1, 10.2.1.6
    • Fix Version/s: 10.2.2.0, 10.3.1.4
    • Component/s: SQL
    • Labels:
      None

      Description

      These triggers succeed

      create trigger tgood after insert on x
      for each statement mode db2sql insert into x values (666), (999), (333);

      create trigger tgood after insert on x
      referencing new as n
      for each row mode db2sql insert into x values (n.x);

      create trigger tgood after insert on x
      referencing new as n
      for each row mode db2sql insert into x values (333), (999), (333)

      This one fails

      create trigger tgood after insert on x
      referencing new as n
      for each row mode db2sql insert into x values (n.x), (999), (333);

      Test cases will be added to triggerGeneral under this bug number.

      java.lang.StringIndexOutOfBoundsException: String index out of range: -3
      at java.lang.String.substring(String.java:1444)
      at org.apache.derby.impl.sql.compile.CreateTriggerNode.bindReferencesClause(CreateTriggerNode.java:421)
      at org.apache.derby.impl.sql.compile.CreateTriggerNode.bind(CreateTriggerNode.java:258)
      at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:344)
      at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:118)
      at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:713)
      at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:560)
      at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:507)
      at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:313)
      at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:433)
      at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:310)
      at org.apache.derby.impl.tools.ij.Main.go(Main.java:203)
      at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:169)
      at org.apache.derby.impl.tools.ij.Main14.main(Main14.java:55)
      at org.apache.derby.tools.ij.main(ij.java:60)

      1. derby1204-10.2-diff01.txt
        9 kB
        Yip Ng
      2. derby1204-10.2-stat01.txt
        0.3 kB
        Yip Ng
      3. derby1204-trunk-diff01.txt
        7 kB
        Yip Ng
      4. derby1204-trunk-diff02.txt
        9 kB
        Yip Ng
      5. derby1204-trunk-stat01.txt
        0.3 kB
        Yip Ng
      6. derby1204-trunk-stat02.txt
        0.3 kB
        Yip Ng
      7. XMLFile1.angosso1.xml
        21 kB
        Mbiama Assogo Roger
      8. XMLFile1.angosso1.xml
        21 kB
        Mbiama Assogo Roger

        Activity

        Hide
        Yip Ng added a comment -

        The following statement fails with the exception:

        create trigger tgood after insert on x
        referencing new as n
        for each row mode db2sql insert into x values (n.x), (999), (333);

        because the bindReferencesClause() method in CreateTriggerNode did not filter those column references that are not relevent to the conversion processing for OLD/NEW transition variables. The above trigger action will have the following parse tree: (roughly)

        InsertNode

        UnionNode
        / \
        UnionNode RowRSNode
        / \ |
        RowRSNode RowRSNode NumericConstantNode

           

        ColRef NumericConstantNode 333

         

        n.i 999

        Note that the CollectNodesVisitor collects ALL the ColumnReferences in the trigger action. In the above case, it returned 3 column references nodes. One in the top level Union node, one in top level Union Node's
        left child which it is also an UnionNode and the last one is found in the RowResultSetNode.

        The UnionNodes will have a copy of the CRs from its left child and those CRs will not have its beginOffset set which indicates they are not relevant for the conversion processing here, so the corrective action here is to skip these entries.

        Show
        Yip Ng added a comment - The following statement fails with the exception: create trigger tgood after insert on x referencing new as n for each row mode db2sql insert into x values (n.x), (999), (333); because the bindReferencesClause() method in CreateTriggerNode did not filter those column references that are not relevent to the conversion processing for OLD/NEW transition variables. The above trigger action will have the following parse tree: (roughly) InsertNode UnionNode / \ UnionNode RowRSNode / \ | RowRSNode RowRSNode NumericConstantNode     ColRef NumericConstantNode 333   n.i 999 Note that the CollectNodesVisitor collects ALL the ColumnReferences in the trigger action. In the above case, it returned 3 column references nodes. One in the top level Union node, one in top level Union Node's left child which it is also an UnionNode and the last one is found in the RowResultSetNode. The UnionNodes will have a copy of the CRs from its left child and those CRs will not have its beginOffset set which indicates they are not relevant for the conversion processing here, so the corrective action here is to skip these entries.
        Hide
        Yip Ng added a comment -

        Attaching patch for derby1204-trunk-diff01.txt. See the above comment for explanation of the fix. derbyall and junit suite passes. Please review.

        Show
        Yip Ng added a comment - Attaching patch for derby1204-trunk-diff01.txt. See the above comment for explanation of the fix. derbyall and junit suite passes. Please review.
        Hide
        Bryan Pendleton added a comment -

        Hi Yip, thank you for the patch! I will have a look at your changes this weekend.

        Show
        Bryan Pendleton added a comment - Hi Yip, thank you for the patch! I will have a look at your changes this weekend.
        Hide
        Bryan Pendleton added a comment -

        The patch looks quite good to me. I have two small comments, and a request:

        1) The original reporter already added a test case for the bug to triggerGeneral.sql.
        I think the new test cases are quite good and should be retained, but I think we
        should also enable the existing test case. It might also be reasonable to put
        the test cases together in the file, either by moving the existing test case to the
        end where the new cases are, or by adding the new cases in the middle, next to
        the existing test case.

        2) There appears to be an unnecessary whitespace-only diff change in CreateTriggerNode.

        The request: I think that DERBY-1644 is rather similar to this, in that both cases involve
        incorrect handling of the intermediate nodes in the UnionNode tree that is constructed
        for a INSERT statement with a multi-row VALUES clause. Although the two proposed
        fixes are completely independent, I was hoping that you could have a look at DERBY-1644,
        and perhaps see what you think of my proposed change. I'm a bit concerned that my
        proposed change turned out to be more complicated than was necessary, and perhaps
        you will see a simpler way to fix the problem if you have a look at that issue.

        Show
        Bryan Pendleton added a comment - The patch looks quite good to me. I have two small comments, and a request: 1) The original reporter already added a test case for the bug to triggerGeneral.sql. I think the new test cases are quite good and should be retained, but I think we should also enable the existing test case. It might also be reasonable to put the test cases together in the file, either by moving the existing test case to the end where the new cases are, or by adding the new cases in the middle, next to the existing test case. 2) There appears to be an unnecessary whitespace-only diff change in CreateTriggerNode. The request: I think that DERBY-1644 is rather similar to this, in that both cases involve incorrect handling of the intermediate nodes in the UnionNode tree that is constructed for a INSERT statement with a multi-row VALUES clause. Although the two proposed fixes are completely independent, I was hoping that you could have a look at DERBY-1644 , and perhaps see what you think of my proposed change. I'm a bit concerned that my proposed change turned out to be more complicated than was necessary, and perhaps you will see a simpler way to fix the problem if you have a look at that issue.
        Hide
        Yip Ng added a comment -

        Hi Bryan. Thanks for reviewing this patch. Yes, I'll be happy to review the proposed changes for DERBY-1644; however, I am currently on vacation and I won't be able to give my comments till this weekend (hope that is ok) since my internet access here is limited.

        I'll also address the two comments you made and resubmit a follow-up patch when I return. =)

        Show
        Yip Ng added a comment - Hi Bryan. Thanks for reviewing this patch. Yes, I'll be happy to review the proposed changes for DERBY-1644 ; however, I am currently on vacation and I won't be able to give my comments till this weekend (hope that is ok) since my internet access here is limited. I'll also address the two comments you made and resubmit a follow-up patch when I return. =)
        Hide
        Yip Ng added a comment -

        Attaching derby1204-trunk-diff02.txt to address Bryan's 2 comments:

        1) Re-enable existing test case in triggerGeneral.sql and move it to my new test cases at the end of the script.

        2) Remove unnecessary whitespace diffs.

        Show
        Yip Ng added a comment - Attaching derby1204-trunk-diff02.txt to address Bryan's 2 comments: 1) Re-enable existing test case in triggerGeneral.sql and move it to my new test cases at the end of the script. 2) Remove unnecessary whitespace diffs.
        Hide
        Bryan Pendleton added a comment -

        Committed revision 2 of the patch to subversion as revision 482358.

        I intend to merge this change to the 10.2 branch as well.

        Show
        Bryan Pendleton added a comment - Committed revision 2 of the patch to subversion as revision 482358. I intend to merge this change to the 10.2 branch as well.
        Hide
        Bryan Pendleton added a comment -

        Unfortunately, the merge to 10.2 is not trivial, I think because there have been intervening
        changes to triggerGeneral.sql and its out file.

        Yip, can you have a look at the merge and see if you can construct a clean patch for 10.2?
        If you can do so and can attach a 10.2 diff to this issue, I can have a look at it.

        Show
        Bryan Pendleton added a comment - Unfortunately, the merge to 10.2 is not trivial, I think because there have been intervening changes to triggerGeneral.sql and its out file. Yip, can you have a look at the merge and see if you can construct a clean patch for 10.2? If you can do so and can attach a 10.2 diff to this issue, I can have a look at it.
        Hide
        Yip Ng added a comment -

        Attaching derby1204-10.2-diff01.txt for 10.2. Please review.

        Show
        Yip Ng added a comment - Attaching derby1204-10.2-diff01.txt for 10.2. Please review.
        Hide
        Bryan Pendleton added a comment -

        Thanks for providing the 10.2 patch. My build and test runs were clean, and I
        committed the change to the 10.2 branch as revision 482484.

        Show
        Bryan Pendleton added a comment - Thanks for providing the 10.2 patch. My build and test runs were clean, and I committed the change to the 10.2 branch as revision 482484.
        Hide
        Andrew McIntyre added a comment -

        This issue has been resolved for over a year with no further movement. Closing.

        Show
        Andrew McIntyre added a comment - This issue has been resolved for over a year with no further movement. Closing.

          People

          • Assignee:
            Yip Ng
            Reporter:
            Daniel John Debrunner
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development