Derby
  1. Derby
  2. DERBY-953

Add miscellaneous Statement methods introduced by JDBC 4

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.2.1.6
    • Component/s: JDBC
    • Labels:
      None

      Description

      As described in the JDBC 4 spec, sections 13.1 and 3.1.

      This adds support for new Statement methods added by JDBC4 and not addressed by other JIRAs: isClosed() and getResultSetHoldability().

      1. DERBY-953-3a.diff
        6 kB
        Kristian Waagan
      2. DERBY-953-2a.diff
        2 kB
        Kristian Waagan
      3. DERBY-953-1a.diff
        2 kB
        Kristian Waagan
      4. DERBY-953-1a.stat
        0.1 kB
        Kristian Waagan

        Issue Links

        There are no Sub-Tasks for this issue.

          Activity

          Hide
          Kristian Waagan added a comment -

          Uploaded patch 'DERBY-953-1a.diff' for implementing Statement.isClosed() on both client and embedded side.
          There seems to be a bug in Derby, where Statements are not closed when the parent connection is. The problem is not seem when testing the client side, but it might still be the case that the problem do occur "under the hood" on the server/embedded side. Created issue DERBY-1095 for the bug.

          No tests are uploaded yet, as they are dependent on some JUnit changes. Will be handled as a separate Jira issue.
          Derbyall has not been run for this patch, as it is only created two new methods that return variables.
          All tests will be run as part of the testing issue. The new tests will basically test the existing implementation.

          I leave it up to a committer if the patch is delayed until the testing is in place or not, as the patch is rather simple.

          As a side note, it is possible to implement isClosed() to return the intended values despite the bug described. However, in my opinion, this will only mask the bug, thus it is better to implement it as it is done in the current patch and wait for the bug to be fixed.
          If we want to have correct results despite the bug, we only need to have isClosed() check the status of the parent connection before checking it's own state, but the Statement would actually not be closed even though isClosed() says it is...

          BTW: getResultSetHoldability() is already implemented (JDBC3?).

          Show
          Kristian Waagan added a comment - Uploaded patch ' DERBY-953 -1a.diff' for implementing Statement.isClosed() on both client and embedded side. There seems to be a bug in Derby, where Statements are not closed when the parent connection is. The problem is not seem when testing the client side, but it might still be the case that the problem do occur "under the hood" on the server/embedded side. Created issue DERBY-1095 for the bug. No tests are uploaded yet, as they are dependent on some JUnit changes. Will be handled as a separate Jira issue. Derbyall has not been run for this patch, as it is only created two new methods that return variables. All tests will be run as part of the testing issue. The new tests will basically test the existing implementation. I leave it up to a committer if the patch is delayed until the testing is in place or not, as the patch is rather simple. As a side note, it is possible to implement isClosed() to return the intended values despite the bug described. However, in my opinion, this will only mask the bug, thus it is better to implement it as it is done in the current patch and wait for the bug to be fixed. If we want to have correct results despite the bug, we only need to have isClosed() check the status of the parent connection before checking it's own state, but the Statement would actually not be closed even though isClosed() says it is... BTW: getResultSetHoldability() is already implemented (JDBC3?).
          Hide
          Kristian Waagan added a comment -

          Statement.isClosed() will not return correct values on the embedded side when parent connection is closed until the blocking issue DERBY-1095 is resolved.

          Show
          Kristian Waagan added a comment - Statement.isClosed() will not return correct values on the embedded side when parent connection is closed until the blocking issue DERBY-1095 is resolved.
          Hide
          Kristian Waagan added a comment -

          'DERBY-953-2a.diff' implements EmbedStatement.isClosed() in different way. If the statement is marked as active, it goes to the parent connection to verify this. Tests have been run locally, but they are not yet submitted for commit. See DERBY-1097 for testing code.
          Derbyall has not been run, the patch only adds new code that is not used anywhere yet.
          Implementation is in line with the comment for the initial submission, and with the comments on DERBY-1095.
          Nothing have been changed for the client side since the previous patch. See 'DERBY-953-1a.stat' for svn status (unchanged).

          Please review, and when acceptable, commit.

          Show
          Kristian Waagan added a comment - ' DERBY-953 -2a.diff' implements EmbedStatement.isClosed() in different way. If the statement is marked as active, it goes to the parent connection to verify this. Tests have been run locally, but they are not yet submitted for commit. See DERBY-1097 for testing code. Derbyall has not been run, the patch only adds new code that is not used anywhere yet. Implementation is in line with the comment for the initial submission, and with the comments on DERBY-1095 . Nothing have been changed for the client side since the previous patch. See ' DERBY-953 -1a.stat' for svn status (unchanged). Please review, and when acceptable, commit.
          Hide
          David Van Couvering added a comment -

          I don't like the approach of using an exception to determine if the statement is closed. I see your motivation – you want to reuse the code that sets active to false. I think the better way to do this is to refactor out the code inside checkExecStatus() that sets the active field, thusly:

          protected final boolean checkActive() throws SQLException

          { if (!getConnection().isClosed()) return true; active = false; return false; }

          protected final void checkExecStatus() throws SQLException {
          if ( ! checkActive() )

          { throw Util.noCurrentConnection(); }

          }

          public boolean isClosed() throws SQLException

          { if ( active ) checkActive(); return !active; }
          Show
          David Van Couvering added a comment - I don't like the approach of using an exception to determine if the statement is closed. I see your motivation – you want to reuse the code that sets active to false. I think the better way to do this is to refactor out the code inside checkExecStatus() that sets the active field, thusly: protected final boolean checkActive() throws SQLException { if (!getConnection().isClosed()) return true; active = false; return false; } protected final void checkExecStatus() throws SQLException { if ( ! checkActive() ) { throw Util.noCurrentConnection(); } } public boolean isClosed() throws SQLException { if ( active ) checkActive(); return !active; }
          Hide
          Kristian Waagan added a comment -

          The code snippet posted in the previous comment still has the same problem as the original code, which was the reason why I returned true in the catch block.
          A NoCurrentConnection exception can be thrown in getConnection(). active would then still not be set to false, and isClosed would throw this exception. I do not like that isClosed can throw an exception in this case, and in this situation I would dare say a NoCurrentConnection is the same as the statement being closed and we could simply return true.

          So I don't quite see how the new proposal would solve the issue. It would also introduce yet another method for checking the state, taking the number up to three; checkStatus, checkExecStatus and checkActive.

          If you still want this to happen, give me a little more pushback, I'm not yet convinced I want to do this.
          I do however see that I could have checked that the exception thrown actually is a NoCurrentConnection exception, and then re-throw the exception if it is not.
          Would that ease your concerns?

          Show
          Kristian Waagan added a comment - The code snippet posted in the previous comment still has the same problem as the original code, which was the reason why I returned true in the catch block. A NoCurrentConnection exception can be thrown in getConnection(). active would then still not be set to false, and isClosed would throw this exception. I do not like that isClosed can throw an exception in this case, and in this situation I would dare say a NoCurrentConnection is the same as the statement being closed and we could simply return true. So I don't quite see how the new proposal would solve the issue. It would also introduce yet another method for checking the state, taking the number up to three; checkStatus, checkExecStatus and checkActive. If you still want this to happen, give me a little more pushback, I'm not yet convinced I want to do this. I do however see that I could have checked that the exception thrown actually is a NoCurrentConnection exception, and then re-throw the exception if it is not. Would that ease your concerns?
          Hide
          David Van Couvering added a comment -

          Sorry, I missed that about getConnection(). But my point still stands, we shouldn't use exceptions for making decisions mainline execution.

          We can refactor getConnection() using the same approach:

          /**

          • Try to get the connection. Returns null if it's not valid
            */
            protected final Connection getConnectionInternal() {
            java.sql.Connection appConn = getEmbedConnection().getApplicationConnection();
            if (appConn != applicationConnection) { appCon = null; return appConn; }

          /**

          • Check the status without throwing an exception. Returns true if the statement
          • is active and has a valid, open connection, false otherwise
            */
            protected final boolean checkExecNoException() { Connection conn = getConnectionInternal(); if ( conn == null || conn.isClosed() ) active = false; return active; }

          protected final void checkExecStatus() throws SQLException

          { checkStatus(); if ( ! checkExecStatusNoException() ) throw Util.noCurrentConnection() }

          public final java.sql.Connection getConnection() throws SQLException

          { checkStatus(); java.sql.Connection appConn = getConnectionInternal(); if ( appConn == null ) throw Util.noCurrentConnection(); }

          public final boolean isClosed() throws SQLException

          { return ( ! active || checkExecStatusNoException() ); }
          Show
          David Van Couvering added a comment - Sorry, I missed that about getConnection(). But my point still stands, we shouldn't use exceptions for making decisions mainline execution. We can refactor getConnection() using the same approach: /** Try to get the connection. Returns null if it's not valid */ protected final Connection getConnectionInternal() { java.sql.Connection appConn = getEmbedConnection().getApplicationConnection(); if (appConn != applicationConnection) { appCon = null; return appConn; } /** Check the status without throwing an exception. Returns true if the statement is active and has a valid, open connection, false otherwise */ protected final boolean checkExecNoException() { Connection conn = getConnectionInternal(); if ( conn == null || conn.isClosed() ) active = false; return active; } protected final void checkExecStatus() throws SQLException { checkStatus(); if ( ! checkExecStatusNoException() ) throw Util.noCurrentConnection() } public final java.sql.Connection getConnection() throws SQLException { checkStatus(); java.sql.Connection appConn = getConnectionInternal(); if ( appConn == null ) throw Util.noCurrentConnection(); } public final boolean isClosed() throws SQLException { return ( ! active || checkExecStatusNoException() ); }
          Hide
          Kristian Waagan added a comment -

          'DERBY-953-3a.diff' is a patch implementing pretty much what David suggested. I made some corrections, and I also had to add a try-catch block to the 'checkExecStatusNoException'-method, because we are using the Connection-interface there, not the EmbedConnection implementation. It is not quite clear to me if we can get another Connection-implementation there, but I have assumed so. If the connection is always an EmbedConnection, we could cast.

          Other comments:
          1) The patch has some white-space changes. The file contains a mix of tabs and spaces, and I chose to use spaces for the patch.
          2) The patch also has some Javadoc fixes.
          3) My StatementTest passes (embedded and DerbyNetClient, JCC excluded due to missing JDBC4 support).
          4) I ran derbyall, but made some minor changes afterwards. The first one passed, the second run is ongoing. I will report if errors occur.

          Patch can be reviewed and committed.

          Show
          Kristian Waagan added a comment - ' DERBY-953 -3a.diff' is a patch implementing pretty much what David suggested. I made some corrections, and I also had to add a try-catch block to the 'checkExecStatusNoException'-method, because we are using the Connection-interface there, not the EmbedConnection implementation. It is not quite clear to me if we can get another Connection-implementation there, but I have assumed so. If the connection is always an EmbedConnection, we could cast. Other comments: 1) The patch has some white-space changes. The file contains a mix of tabs and spaces, and I chose to use spaces for the patch. 2) The patch also has some Javadoc fixes. 3) My StatementTest passes (embedded and DerbyNetClient, JCC excluded due to missing JDBC4 support). 4) I ran derbyall, but made some minor changes afterwards. The first one passed, the second run is ongoing. I will report if errors occur. Patch can be reviewed and committed.
          Hide
          Daniel John Debrunner added a comment -

          I was expecting the patch to be very similar to the one for ResultSet.isClosed(), but it seems to have gained in complexity for little value.

          Not sure I understand David's comment about "using exception for mainline decisions", I don't see that happening in the simpler version
          of the patch (ie. one similar to the changes made for ResultSet.isClosed()). An execption is only used when the Statement is closed, that's
          not the mainline execution, it's the exception case.

          Sorry, I wasn't able to comment earlier, I've been busy,.

          Show
          Daniel John Debrunner added a comment - I was expecting the patch to be very similar to the one for ResultSet.isClosed(), but it seems to have gained in complexity for little value. Not sure I understand David's comment about "using exception for mainline decisions", I don't see that happening in the simpler version of the patch (ie. one similar to the changes made for ResultSet.isClosed()). An execption is only used when the Statement is closed, that's not the mainline execution, it's the exception case. Sorry, I wasn't able to comment earlier, I've been busy,.
          Hide
          David Van Couvering added a comment -

          I could be stubborn about my point of view, but I think that's not worthwhile. Dan has a good point, and Kristian has also pointed out to me that there are exceptions all the way down so there's no way to avoid catching an exception.

          So I must humbly apologize to Kristian and say I think I have to agree with Dan that the first patch is simpler and is more in line with ResultSet.isClosed().

          I would like it to check for a specific SQL State (e.g. SQLState.NO_CURRENT_CONNECTION) rather than swallow any old exception. I'll make that change and commit.

          David

          Show
          David Van Couvering added a comment - I could be stubborn about my point of view, but I think that's not worthwhile. Dan has a good point, and Kristian has also pointed out to me that there are exceptions all the way down so there's no way to avoid catching an exception. So I must humbly apologize to Kristian and say I think I have to agree with Dan that the first patch is simpler and is more in line with ResultSet.isClosed(). I would like it to check for a specific SQL State (e.g. SQLState.NO_CURRENT_CONNECTION) rather than swallow any old exception. I'll make that change and commit. David
          Hide
          Daniel John Debrunner added a comment -

          Why do you need to check for a specific state, the field active tells you everything you need to know?

          A ResultSet can still be open and the check methods throw no current connection, this is the case when the ResultSet is open
          in a global transaction but the transaction is detached from the connection.

          Statement may fall along the same lines.

          Show
          Daniel John Debrunner added a comment - Why do you need to check for a specific state, the field active tells you everything you need to know? A ResultSet can still be open and the check methods throw no current connection, this is the case when the ResultSet is open in a global transaction but the transaction is detached from the connection. Statement may fall along the same lines.
          Hide
          Daniel John Debrunner added a comment -

          I think this is the correct fix, a slightly modified version of the second patch, removing the assumption that if
          checkExecStatus throws an exception that the statement is closed. This them matches the ResultSet.isClosed() approach.

          /**
          + * Tell whether this statment has been closed or not.
          + *
          + * @return <code>true</code> is closed, <code>false</code> otherwise.
          + * @exception SQLException if a database access error occurs.
          + */
          + public boolean isClosed()
          + throws SQLException {
          + // If active, verify state by consulting parent connection.
          + if (active) {
          + try

          { + checkExecStatus(); + }

          catch (SQLException sqle)

          { + }

          + }
          + return !active;
          + }

          Show
          Daniel John Debrunner added a comment - I think this is the correct fix, a slightly modified version of the second patch, removing the assumption that if checkExecStatus throws an exception that the statement is closed. This them matches the ResultSet.isClosed() approach. /** + * Tell whether this statment has been closed or not. + * + * @return <code>true</code> is closed, <code>false</code> otherwise. + * @exception SQLException if a database access error occurs. + */ + public boolean isClosed() + throws SQLException { + // If active, verify state by consulting parent connection. + if (active) { + try { + checkExecStatus(); + } catch (SQLException sqle) { + } + } + return !active; + }
          Hide
          David Van Couvering added a comment -

          Thanks for your tips on this, Dan. Your version looks just right. I'll wait to hear from Kristian, but if he's OK, I can make the change you suggest and commit.

          Show
          David Van Couvering added a comment - Thanks for your tips on this, Dan. Your version looks just right. I'll wait to hear from Kristian, but if he's OK, I can make the change you suggest and commit.
          Hide
          David Van Couvering added a comment -

          One question on this: your example swallows any SqlException. I think this assumes that the only possible exception is going to be NO_CURRENT_CONNECTION. Is that a fair assumption? I can't say I fully understand the code. Shouldn't we be throwing other exceptions besides NO_CURRENT_CONNECTION?

          Show
          David Van Couvering added a comment - One question on this: your example swallows any SqlException. I think this assumes that the only possible exception is going to be NO_CURRENT_CONNECTION. Is that a fair assumption? I can't say I fully understand the code. Shouldn't we be throwing other exceptions besides NO_CURRENT_CONNECTION?
          Hide
          Daniel John Debrunner added a comment -

          No, that's not the assumption I made. The assumption is that after a call to checkExecStatus() the active field will correctly represent the closed/open state of the Statement. I can add comments to three methods that are involved in this code stating what exceptions they throw if you think that will help.

          Show
          Daniel John Debrunner added a comment - No, that's not the assumption I made. The assumption is that after a call to checkExecStatus() the active field will correctly represent the closed/open state of the Statement. I can add comments to three methods that are involved in this code stating what exceptions they throw if you think that will help.
          Hide
          David Van Couvering added a comment -

          I guess I just feel uncomfortable with swallowing all exceptions. Can you explain it to me like I were a novice why that's OK in this case? Why wouldn't it be better to check for the specific exception?

          Show
          David Van Couvering added a comment - I guess I just feel uncomfortable with swallowing all exceptions. Can you explain it to me like I were a novice why that's OK in this case? Why wouldn't it be better to check for the specific exception?
          Hide
          Daniel John Debrunner added a comment -

          Not really sure what explanations you are looking for David, I just clarified the javadoc comments for EmbedStatement.checkStatus and checkExecStatus based upon a few minutes of code inspection. Modifying the javadoc firms up the contract this method is exposing and thus the code in isClosed() is allowed to make assumptions
          based upon that contract.

          The checkExecStatus method only throws execeptions in two cases, one the statement is closed and two the statement is part of a non-active global transaction.
          In either of those two cases and the no exception case after the execution the active field correctly represents the open state of the Statement.

          One exception thrown is the NO_CURRENT_CONNECTION sql state, means either :
          the Statement is open but in a suspended transaction (isClosed needs to return false)
          the Statement has been closed implicitly due to its connection being closed and this is the first
          call against the Statement that has noticed the connection has been closed. (isClosed needs to return true)

          The other is ALREADY_CLOSED, means either :
          the Statement has been explictly closed (isClosed needs to return true)
          the Statement has been closed implicitly and a previous checkExecStatus threw a NO_CURRENT_CONNECTION (isClosed needs to return true)

          Thus only catching NO_CURRENT_CONNECTION would be wrong as ALREADY_CLOSED is a valid exception that could occur
          here and require isClosed() to return true. Thus we are left with needing to catch the two exceptions that this method can throw, which seems,
          to me, to be the same as catching everything.

          Show
          Daniel John Debrunner added a comment - Not really sure what explanations you are looking for David, I just clarified the javadoc comments for EmbedStatement.checkStatus and checkExecStatus based upon a few minutes of code inspection. Modifying the javadoc firms up the contract this method is exposing and thus the code in isClosed() is allowed to make assumptions based upon that contract. The checkExecStatus method only throws execeptions in two cases, one the statement is closed and two the statement is part of a non-active global transaction. In either of those two cases and the no exception case after the execution the active field correctly represents the open state of the Statement. One exception thrown is the NO_CURRENT_CONNECTION sql state, means either : the Statement is open but in a suspended transaction (isClosed needs to return false) the Statement has been closed implicitly due to its connection being closed and this is the first call against the Statement that has noticed the connection has been closed. (isClosed needs to return true) The other is ALREADY_CLOSED, means either : the Statement has been explictly closed (isClosed needs to return true) the Statement has been closed implicitly and a previous checkExecStatus threw a NO_CURRENT_CONNECTION (isClosed needs to return true) Thus only catching NO_CURRENT_CONNECTION would be wrong as ALREADY_CLOSED is a valid exception that could occur here and require isClosed() to return true. Thus we are left with needing to catch the two exceptions that this method can throw, which seems, to me, to be the same as catching everything.
          Hide
          David Van Couvering added a comment -

          Thanks, Dan, this was what I'm looking for. I'm working on committing this patch.

          Show
          David Van Couvering added a comment - Thanks, Dan, this was what I'm looking for. I'm working on committing this patch.
          Hide
          David Van Couvering added a comment -

          Resolved, revision 388234. With StatementTest.junit added to the jdbc40 test suite, the suite passes. I did not run derbyall because this is a JDBC4-specific method, and only new, JDBC4-specific code was added; no existing code was modified.

          Show
          David Van Couvering added a comment - Resolved, revision 388234. With StatementTest.junit added to the jdbc40 test suite, the suite passes. I did not run derbyall because this is a JDBC4-specific method, and only new, JDBC4-specific code was added; no existing code was modified.
          Hide
          Kristian Waagan added a comment -

          Task completed.

          Show
          Kristian Waagan added a comment - Task completed.
          Hide
          Rick Hillegas added a comment -

          Ported DERBY-935 / DERBY-1565 (433814) to 10.2 branch at subversion revision 436974.

          Show
          Rick Hillegas added a comment - Ported DERBY-935 / DERBY-1565 (433814) to 10.2 branch at subversion revision 436974.

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Rick Hillegas
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development