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-d.stat
        0.1 kB
        Dag H. Wanvik
      2. derby-4338-d.diff
        13 kB
        Dag H. Wanvik
      3. derby-4338-c.stat
        0.1 kB
        Dag H. Wanvik
      4. derby-4338-c.diff
        12 kB
        Dag H. Wanvik
      5. derby-4338-b.stat
        0.1 kB
        Dag H. Wanvik
      6. derby-4338-b.diff
        10 kB
        Dag H. Wanvik
      7. derby-4338-a.stat
        0.1 kB
        Dag H. Wanvik
      8. derby-4338-a.diff
        10 kB
        Dag H. Wanvik

        Issue Links

          Activity

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

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

          > /* 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
          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
          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 -

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

            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