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. Text File
            CALCITE-352-1.patch
            2 kB
            Sivaramakrishnan Narayanan

            Activity

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            snarayanan Sivaramakrishnan Narayanan added a comment -

            Taking a shot at this.

            Show
            snarayanan Sivaramakrishnan Narayanan added a comment - Taking a shot at this.
            Hide
            Permalink
            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
            Permalink
            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
            Permalink
            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.
            Hide
            Permalink
            snarayanan Sivaramakrishnan Narayanan added a comment -

            Opened PR https://github.com/apache/incubator-calcite/pull/22

            Show
            snarayanan Sivaramakrishnan Narayanan added a comment - Opened PR https://github.com/apache/incubator-calcite/pull/22
            Hide
            Permalink
            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

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