Derby
  1. Derby
  2. DERBY-4338

Network client raises error "executeQuery method can not be used for update" when sql is preceded by /* */ comments

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.4.1.3, 10.4.2.0, 10.5.1.1, 10.5.2.0, 10.5.3.0
    • Fix Version/s: 10.4.2.1, 10.5.3.1, 10.6.1.0
    • Component/s: Network Client
    • Labels:
      None
    • Urgency:
      Normal
    • Issue & fix info:
      Newcomer
    • Bug behavior facts:
      Embedded/Client difference

      Description

      Network derby client does not properly detect a sql select statement preceded by /* */ comments. As a result the sql appears to be detected as an update statement, and results in the following error:

      org.apache.derby.client.am.SqlException: executeQuery method can not be used for update.
      at org.apache.derby.client.am.Statement.checkForAppropriateSqlMode(Unknown Source)
      at org.apache.derby.client.am.PreparedStatement.flowExecute(Unknown Source)
      at org.apache.derby.client.am.PreparedStatement.executeQueryX(Unknown Source)

      The problem appears to be in Statment.parseSqlAndSetSqlModes(), which only appears to check for "--" style comments.

      1. derby-4338-a.diff
        10 kB
        Dag H. Wanvik
      2. derby-4338-a.stat
        0.1 kB
        Dag H. Wanvik
      3. derby-4338-b.diff
        10 kB
        Dag H. Wanvik
      4. derby-4338-b.stat
        0.1 kB
        Dag H. Wanvik
      5. derby-4338-c.diff
        12 kB
        Dag H. Wanvik
      6. derby-4338-c.stat
        0.1 kB
        Dag H. Wanvik
      7. derby-4338-d.diff
        13 kB
        Dag H. Wanvik
      8. derby-4338-d.stat
        0.1 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Will Gomes created issue -
          Dag H. Wanvik made changes -
          Field Original Value New Value
          Issue & fix info [Newcomer]
          Component/s Network Client [ 11690 ]
          Dag H. Wanvik made changes -
          Urgency Normal
          Hide
          Art Allisany added a comment -

          This error prevents hibernate comments from being prepended to the sql query which is a very useful feature to correlate jpa queries with sql queries.

          in jpa persistence.xml

          <property name="hibernate.use_sql_comments" value="true"/>

          Hibernate: /* SELECT e FROM CommonEntity e JOIN e.addresses a WHERE a.countryCode = 'us' */ select commonenti0_.id as id0_, commonenti0_.billingAddress as billingA3_0_, commonenti0_.shippingAddress as shipping4_0_, commonenti0_.purchaser_id as purchaser6_0_, commonenti0_.businessId as businessId0_, commonenti0_.DTYPE as DTYPE0_ from CommonEntity commonenti0_ inner join CommonEntity_Address addresses1_ on commonenti0_.id=addresses1_.CommonEntity_id inner join Address address2_ on addresses1_.addresses_id=address2_.id where address2_.countryCode='us'

          WARNING: SQL Error: 20000, SQLState: XJ207
          SEVERE: executeQuery method can not be used for update.

          If I remove the hibernate.use_sql_comments the select query succeeds, so I have confirmed Will's bug on Apache Derby Network Server - 10.5.1.1.

          Show
          Art Allisany added a comment - This error prevents hibernate comments from being prepended to the sql query which is a very useful feature to correlate jpa queries with sql queries. in jpa persistence.xml <property name="hibernate.use_sql_comments" value="true"/> Hibernate: /* SELECT e FROM CommonEntity e JOIN e.addresses a WHERE a.countryCode = 'us' */ select commonenti0_.id as id0_, commonenti0_.billingAddress as billingA3_0_, commonenti0_.shippingAddress as shipping4_0_, commonenti0_.purchaser_id as purchaser6_0_, commonenti0_.businessId as businessId0_, commonenti0_.DTYPE as DTYPE0_ from CommonEntity commonenti0_ inner join CommonEntity_Address addresses1_ on commonenti0_.id=addresses1_.CommonEntity_id inner join Address address2_ on addresses1_.addresses_id=address2_.id where address2_.countryCode='us' WARNING: SQL Error: 20000, SQLState: XJ207 SEVERE: executeQuery method can not be used for update. If I remove the hibernate.use_sql_comments the select query succeeds, so I have confirmed Will's bug on Apache Derby Network Server - 10.5.1.1.
          Dag H. Wanvik made changes -
          Assignee Dag H. Wanvik [ dagw ]
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading a patch for this issue, and added test cases to CommentTest,
          please review. Running regressions.

          While rewriting the handling of this in the client driver, I noticed
          that the client driver seems to filter away the "newline" characters:
          '\n', 'r' and '\f', at least they are gone by the time the query
          string is seen by the compiler. The latter ('\f') is rejected by the
          Derby compiler, which makes the client driver slightly more lenient
          than the embedded driver in this regard. Probably not important, but I
          thought I'd mention it.

          Show
          Dag H. Wanvik added a comment - - edited Uploading a patch for this issue, and added test cases to CommentTest, please review. Running regressions. While rewriting the handling of this in the client driver, I noticed that the client driver seems to filter away the "newline" characters: '\n', 'r' and '\f', at least they are gone by the time the query string is seen by the compiler. The latter ('\f') is rejected by the Derby compiler, which makes the client driver slightly more lenient than the embedded driver in this regard. Probably not important, but I thought I'd mention it.
          Dag H. Wanvik made changes -
          Attachment derby-4338-a.stat [ 12417562 ]
          Attachment derby-4338-a.diff [ 12417563 ]
          Dag H. Wanvik made changes -
          Issue & fix info [Newcomer] [Newcomer, Patch Available]
          Dag H. Wanvik made changes -
          Affects Version/s 10.5.3.0 [ 12314117 ]
          Affects Version/s 10.5.2.0 [ 12314116 ]
          Affects Version/s 10.5.1.1 [ 12313771 ]
          Hide
          Knut Anders Hatlen added a comment -

          This looks like an improvement to me. Two small comments:

          1) Should getNonCommentToken() also skip '{' when in the OUTSIDE state? I was thinking of JDBC escaped call statements, like "

          { call TOURS.BOOK_TOUR(?, ?) }

          ", where the first token should be "CALL" and not "{".

          2) CommentTest.suite() could be simplified to "return TestConfiguration.defaultSuite(CommentTest.class);"

          Show
          Knut Anders Hatlen added a comment - This looks like an improvement to me. Two small comments: 1) Should getNonCommentToken() also skip '{' when in the OUTSIDE state? I was thinking of JDBC escaped call statements, like " { call TOURS.BOOK_TOUR(?, ?) } ", where the first token should be "CALL" and not "{". 2) CommentTest.suite() could be simplified to "return TestConfiguration.defaultSuite(CommentTest.class);"
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at this Knut!

          Actually the escapes have been removed at this point in processing, cf. calls to
          escape(sql) just before the call to parseSqlAndSetSqlModes in Statement.flowExecute(); ca line 3.
          (although not for batches, not sure if batches can contain escapes?)

          I'll insert a comment to that fact, and simplify the test.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at this Knut! Actually the escapes have been removed at this point in processing, cf. calls to escape(sql) just before the call to parseSqlAndSetSqlModes in Statement.flowExecute(); ca line 3. (although not for batches, not sure if batches can contain escapes?) I'll insert a comment to that fact, and simplify the test.
          Hide
          Dag H. Wanvik added a comment -

          Uploading version "b" of this patch, which, in addition to test simplification and the added comment, wrapped some SanityManager methods in
          tests for SanityManager.DEBUG, missing in version "a".

          If no further comments I intend to commit this version. Re-running regressions.

          Show
          Dag H. Wanvik added a comment - Uploading version "b" of this patch, which, in addition to test simplification and the added comment, wrapped some SanityManager methods in tests for SanityManager.DEBUG, missing in version "a". If no further comments I intend to commit this version. Re-running regressions.
          Dag H. Wanvik made changes -
          Attachment derby-4338-b.stat [ 12417664 ]
          Attachment derby-4338-b.diff [ 12417665 ]
          Dag H. Wanvik made changes -
          Fix Version/s 10.6.0.0 [ 12313727 ]
          Affects Version/s 10.4.1.3 [ 12313111 ]
          Hide
          Knut Anders Hatlen added a comment -

          I only tested it from ij, but when I made getNonCommentToken() print the returned token, this is what I saw:

          ij(CONNECTION1)>

          { call syscs_util.syscs_checkpoint_database() };
          tokenFound: call
          Statement executed.
          ij(CONNECTION1)> /* asdfads */ { call syscs_util.syscs_checkpoint_database() }

          ;
          tokenFound: {
          0 rows inserted/updated/deleted

          Note that if the escaped call is prefixed with a comment, the braces are not stripped away, and getNonCommentToken() returns the opening brace. Not sure if this is a problem, and I don't think your patch is making the situation worse in any way (the difference in the return status - "Statement executed" vs "0 rows inserted/..." - is also seen without your patch), so don't let this issue hold the patch.

          Show
          Knut Anders Hatlen added a comment - I only tested it from ij, but when I made getNonCommentToken() print the returned token, this is what I saw: ij(CONNECTION1)> { call syscs_util.syscs_checkpoint_database() }; tokenFound: call Statement executed. ij(CONNECTION1)> /* asdfads */ { call syscs_util.syscs_checkpoint_database() } ; tokenFound: { 0 rows inserted/updated/deleted Note that if the escaped call is prefixed with a comment, the braces are not stripped away, and getNonCommentToken() returns the opening brace. Not sure if this is a problem, and I don't think your patch is making the situation worse in any way (the difference in the return status - "Statement executed" vs "0 rows inserted/..." - is also seen without your patch), so don't let this issue hold the patch.
          Hide
          Knut Anders Hatlen added a comment -

          One more data point. If I add a case for '{' in getNonCommentToken(), the statements above consistently return "Statement executed". This is different from the return status with the embedded driver (because of DERBY-211), but I guess it would be good to be internally consistent within the client driver, even if it's not consistent with the embedded driver.

          Show
          Knut Anders Hatlen added a comment - One more data point. If I add a case for '{' in getNonCommentToken(), the statements above consistently return "Statement executed". This is different from the return status with the embedded driver (because of DERBY-211 ), but I guess it would be good to be internally consistent within the client driver, even if it's not consistent with the embedded driver.
          Hide
          Knut Anders Hatlen added a comment -

          From getNonCommentToken():

          + case '=': //
          + case '?': //

          It looks like you intended, but forgot, to write comments for those two cases.

          For extra credit, you could also add a javadoc comment describing getNonCommentToken(). Though, sadly, describing an internal method in a comment will break the established pattern in that class...

          Show
          Knut Anders Hatlen added a comment - From getNonCommentToken(): + case '=': // + case '?': // It looks like you intended, but forgot, to write comments for those two cases. For extra credit, you could also add a javadoc comment describing getNonCommentToken(). Though, sadly, describing an internal method in a comment will break the established pattern in that class...
          Hide
          Dag H. Wanvik added a comment -

          > /* asdfads */

          { call syscs_util.syscs_checkpoint_database() }

          ;

          Yes, I realize that I could have chosen to remove the left brace here easily, but the old code didn't handle that case after an end-of-line comment so I was hesitant to change behavior. But thinking again
          I agree it's better to be consistent. I don't think it should cause much a problem: the only change would be to correctly classify an escaped call as such after a end-of-line comment.

          As for the comment stubs, I moved the comments up to the delim variable. I'll remove the stubs
          and provide a method comment as well.

          Show
          Dag H. Wanvik added a comment - > /* asdfads */ { call syscs_util.syscs_checkpoint_database() } ; Yes, I realize that I could have chosen to remove the left brace here easily, but the old code didn't handle that case after an end-of-line comment so I was hesitant to change behavior. But thinking again I agree it's better to be consistent. I don't think it should cause much a problem: the only change would be to correctly classify an escaped call as such after a end-of-line comment. As for the comment stubs, I moved the comments up to the delim variable. I'll remove the stubs and provide a method comment as well.
          Hide
          Dag H. Wanvik added a comment -

          Digging more, here is a more serious classification error done in the client, due to its reliance on StringTokenizer:

          ij> select* from sys.systables;
          firstToken=select*
          :
          ERROR X0Y79: Statement.executeUpdate() cannot be called with a statement that returns a ResultSet.

          Similarly:
          select'a' from sys.systables;
          select"TABLEID" from sys.systables;

          All the above are legal queries missed in the client statement classification logic..

          Show
          Dag H. Wanvik added a comment - Digging more, here is a more serious classification error done in the client, due to its reliance on StringTokenizer: ij> select* from sys.systables; firstToken=select* : ERROR X0Y79: Statement.executeUpdate() cannot be called with a statement that returns a ResultSet. Similarly: select'a' from sys.systables; select"TABLEID" from sys.systables; All the above are legal queries missed in the client statement classification logic..
          Hide
          Dag H. Wanvik added a comment -

          Uploading version "c", which adds a new testcase,
          testWrongKeywordLexing_derby4338, and a fix for that problem, cf. the
          new method isolateAnyInitialIdentifier in Statement.java used instead
          of the Java tokenizer which wasn't really suitable in this case. A
          regexp could have been used though, but in this simple case, that
          seemed overkill.

          I also added a testcase to show how the client before this fix fails to
          scan past the escape starting after an end-of-line comment. See the
          "call" examples in CommentTest.testInitialComment_derby4338.

          Investigating this lead to my filing DERBY-4362, btw.

          Show
          Dag H. Wanvik added a comment - Uploading version "c", which adds a new testcase, testWrongKeywordLexing_derby4338, and a fix for that problem, cf. the new method isolateAnyInitialIdentifier in Statement.java used instead of the Java tokenizer which wasn't really suitable in this case. A regexp could have been used though, but in this simple case, that seemed overkill. I also added a testcase to show how the client before this fix fails to scan past the escape starting after an end-of-line comment. See the "call" examples in CommentTest.testInitialComment_derby4338. Investigating this lead to my filing DERBY-4362 , btw.
          Dag H. Wanvik made changes -
          Attachment derby-4338-c.diff [ 12417800 ]
          Attachment derby-4338-c.stat [ 12417801 ]
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for catching those additional cases and incorporating them into the test. The patch looks good to me.

          Some small nits:

          You may be able to simplify isolateAnyInitialIdentifier() by using the JDK's regex library. I think the code below is equivalent to the code in the patch.

          private final static Pattern INITIAL_IDENTIFIER = Pattern.compile("(
          p

          {Alpha}

          +).*");
          private String isolateAnyInitialIdentifier (String sql) {
          Matcher m = INITIAL_IDENTIFIER.matcher(sql);
          if (m.matches())

          { return m.group(1); }

          return sql;
          }

          In the tests there are some calls to check the return value from getUpdateCount() on this form:

          assertTrue(ps.getUpdateCount() == 0);

          It's probably better to use assertEquals(0, ps.getUpdateCount()) to get more information in case of a failure.

          There are a couple of occurrences of getConnection().createStatement() and getConnection().prepareStatement(). The getConnection() part should be dropped so that we use the methods in BaseJDBCTestCase that automatically close the statements.

          Perhaps the calls to executeQuery() in testInitialComment_derby4338() and testWrongKeywordLexing_derby4338() should be wrapped in JDBC.assertDrainResults() so that we don't leave any results sets open?

          A comment in testInitialComment_derby4338() stops in the middle of a sentence: // Change to 0 when DERBY-4362 is fixed. Before both

          Show
          Knut Anders Hatlen added a comment - Thanks for catching those additional cases and incorporating them into the test. The patch looks good to me. Some small nits: You may be able to simplify isolateAnyInitialIdentifier() by using the JDK's regex library. I think the code below is equivalent to the code in the patch. private final static Pattern INITIAL_IDENTIFIER = Pattern.compile("( p {Alpha} +).*"); private String isolateAnyInitialIdentifier (String sql) { Matcher m = INITIAL_IDENTIFIER.matcher(sql); if (m.matches()) { return m.group(1); } return sql; } In the tests there are some calls to check the return value from getUpdateCount() on this form: assertTrue(ps.getUpdateCount() == 0); It's probably better to use assertEquals(0, ps.getUpdateCount()) to get more information in case of a failure. There are a couple of occurrences of getConnection().createStatement() and getConnection().prepareStatement(). The getConnection() part should be dropped so that we use the methods in BaseJDBCTestCase that automatically close the statements. Perhaps the calls to executeQuery() in testInitialComment_derby4338() and testWrongKeywordLexing_derby4338() should be wrapped in JDBC.assertDrainResults() so that we don't leave any results sets open? A comment in testInitialComment_derby4338() stops in the middle of a sentence: // Change to 0 when DERBY-4362 is fixed. Before both
          Hide
          Knut Anders Hatlen added a comment -

          Sorry, I missed your comment that you had already considered a regex, so just disregard that.

          Show
          Knut Anders Hatlen added a comment - Sorry, I missed your comment that you had already considered a regex, so just disregard that.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at my revised patch, Knut. I will fix up the test as you suggested.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at my revised patch, Knut. I will fix up the test as you suggested.
          Hide
          Dag H. Wanvik added a comment -

          Attaching version "d", which incorporates Knut's suggestions regarding the test.

          Show
          Dag H. Wanvik added a comment - Attaching version "d", which incorporates Knut's suggestions regarding the test.
          Dag H. Wanvik made changes -
          Attachment derby-4338-d.diff [ 12417890 ]
          Attachment derby-4338-d.stat [ 12417891 ]
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Dag! +1 to commit.

          Show
          Knut Anders Hatlen added a comment - Thanks, Dag! +1 to commit.
          Hide
          Dag H. Wanvik added a comment -

          Committed derby-4338-d on trunk as svn 809643.

          Show
          Dag H. Wanvik added a comment - Committed derby-4338-d on trunk as svn 809643.
          Dag H. Wanvik made changes -
          Issue & fix info [Patch Available, Newcomer] [Newcomer]
          Dag H. Wanvik made changes -
          Attachment derby-151-b.diff [ 12418182 ]
          Attachment derby-151-b.stat [ 12418183 ]
          Dag H. Wanvik made changes -
          Comment [ Uploading version "b" of this patch.

          - Changed severity to database; updated ErrorCodeTest to reflect this.
            Rationale: It seems right - I don't know that it's safe to continue
            after this kind of error, and I compared to other IO level errors in
            raw store and mostly seem to have database severity.

          - Made the new Derby151Test not fail even if expected error is not
            seen. I did this to allow for possible different behavior on other
            VMs, JUnit verbose mode would instead print "Not able to test fix
            for DERBY-151: No interrupt seen".

          - I added the same handling to read (even if not seen yet) as for
            write. Regressions passed modulo ErrorCodeTest, which i missed
            first time around.

          - Fixed some broken Javadocs.

          Ready for review.
          ]
          Dag H. Wanvik made changes -
          Attachment derby-151-b.diff [ 12418182 ]
          Dag H. Wanvik made changes -
          Attachment derby-151-b.stat [ 12418183 ]
          Hide
          Dag H. Wanvik added a comment -

          Back-ported to 10.4 as svn 809764.
          Back-ported to 10.5 as svn 809763.

          Resolving.

          Show
          Dag H. Wanvik added a comment - Back-ported to 10.4 as svn 809764. Back-ported to 10.5 as svn 809763. Resolving.
          Dag H. Wanvik made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 10.4.3.0 [ 12313654 ]
          Fix Version/s 10.5.4.0 [ 12314154 ]
          Resolution Fixed [ 1 ]
          Dag H. Wanvik made changes -
          Fix Version/s 10.5.3.1 [ 12314182 ]
          Knut Anders Hatlen made changes -
          Link This issue is duplicated by DERBY-3626 [ DERBY-3626 ]
          Dag H. Wanvik made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Dag H. Wanvik made changes -
          Fix Version/s 10.5.4.0 [ 12314154 ]
          Dag H. Wanvik made changes -
          Fix Version/s 10.4.2.1 [ 12313401 ]
          Fix Version/s 10.4.3.0 [ 12313654 ]
          Knut Anders Hatlen made changes -
          Link This issue is related to DERBY-5011 [ DERBY-5011 ]
          Gavin made changes -
          Workflow jira [ 12472419 ] Default workflow, editable Closed status [ 12801686 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          26d 8h 11m 1 Dag H. Wanvik 31/Aug/09 22:21
          Resolved Resolved Closed Closed
          252d 1h 29m 1 Dag H. Wanvik 10/May/10 23:51

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Will Gomes
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development