Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-352

Calling next() on closed AvaticaResultSet gives NPE

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0.0-incubating
    • Component/s: None
    • Labels:

      Description

      If you call close() on an AvaticaResultSet then call next(), it gives a NullPointerException (due to the cursor field being null). It should give a SQLException (per the Javadoc of the ResultSet.next method).

      1. CALCITE-352-1.patch
        2 kB
        Sivaramakrishnan Narayanan

        Activity

        Hide
        snarayanan Sivaramakrishnan Narayanan added a comment -

        Taking a shot at this.

        Show
        snarayanan Sivaramakrishnan Narayanan added a comment - Taking a shot at this.
        Hide
        snarayanan Sivaramakrishnan Narayanan added a comment -

        Attaching patch that checks for closed cursor and throws SQLException with appropriate error code.

        I'm not sure how to test this, though.

        Show
        snarayanan Sivaramakrishnan Narayanan added a comment - Attaching patch that checks for closed cursor and throws SQLException with appropriate error code. I'm not sure how to test this, though.
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        Attaching patch that checks

        1) Pull requests for https://github.com/apache/incubator-calcite would be easier to review (github allows conversations on the line-by-line basis) and merge (commit author is added automatically). Nevertheless, patch files are welcome as well.

        2) In such trivial cases it makes sense to have just a single patch, not two distinct changes.

        I'm not sure how to test this, though

        3) Historically, jdbc-related tests are located in org.apache.calcite.test.JdbcTest. Personally, I think this class should be refactored to smaller units, however it is another issue.
        Can you please add a test method that runs sql, closes the resultset and tries to invoke resultset.next?
        The expected behaviour would be to catch SQLException with the appropriate code.

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - Attaching patch that checks 1) Pull requests for https://github.com/apache/incubator-calcite would be easier to review (github allows conversations on the line-by-line basis) and merge (commit author is added automatically). Nevertheless, patch files are welcome as well. 2) In such trivial cases it makes sense to have just a single patch, not two distinct changes. I'm not sure how to test this, though 3) Historically, jdbc-related tests are located in org.apache.calcite.test.JdbcTest . Personally, I think this class should be refactored to smaller units, however it is another issue. Can you please add a test method that runs sql, closes the resultset and tries to invoke resultset.next? The expected behaviour would be to catch SQLException with the appropriate code.
        Hide
        snarayanan Sivaramakrishnan Narayanan added a comment -

        Thanks for your comments. Will add tests and start a PR on github as suggested.

        Show
        snarayanan Sivaramakrishnan Narayanan added a comment - Thanks for your comments. Will add tests and start a PR on github as suggested.
        Show
        snarayanan Sivaramakrishnan Narayanan added a comment - Opened PR https://github.com/apache/incubator-calcite/pull/22
        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - - edited Thanks. Merged in https://git-wip-us.apache.org/repos/asf?p=incubator-calcite.git;a=commitdiff;h=59657ec75981fb1416d127c413a8dd7ae3571a1d
        Hide
        julianhyde Julian Hyde added a comment -

        Closing now that 1.0.0-incubating has been released.

        Show
        julianhyde Julian Hyde added a comment - Closing now that 1.0.0-incubating has been released.

          People

          • Assignee:
            vladimirsitnikov Vladimir Sitnikov
            Reporter:
            julianhyde Julian Hyde
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development