Commons Dbcp
  1. Commons Dbcp
  2. DBCP-310

SQLNestedException & use of initCause() with SQLException

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3, 1.4
    • Labels:
      None

      Description

      SQLNestedException in its current state is a hangover from supporting JDK 1.3 when there was no "initCause()" method. This implementation can now be greatly simplified with DBCP now having a minimum of JDK 1.4

      Also SQLNestedException is deprecated and the following code has been used in a number of places

           throw (SQLException )new SQLException(message).initCause(e)
      

      DBCP is inconsistent though - sometimes using the above and sometimes using SQLNestedException. IMO SQLNestedException should be un-deprecated and used consistently everywhere - its now a simple implementation and I think the code is cleaner using it rather than the above.

      1. DBCP-310-v2.patch
        20 kB
        Niall Pemberton
      2. SQLNestedException.patch
        13 kB
        Niall Pemberton

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          7h 44m 1 Niall Pemberton 29/Nov/09 00:13
          Resolved Resolved Closed Closed
          78d 1h 35m 1 Phil Steitz 15/Feb/10 01:49
          Phil Steitz made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Niall Pemberton made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Niall Pemberton added a comment -

          OK I applied the changes to SQLNestedException:

          http://svn.apache.org/viewvc?view=revision&revision=885179

          Show
          Niall Pemberton added a comment - OK I applied the changes to SQLNestedException: http://svn.apache.org/viewvc?view=revision&revision=885179
          Hide
          Phil Steitz added a comment -

          I guess I am OK with the backwards compatible change to SQLNestedException, but want to keep the deprecation and not increase its use as we should remove it in 2.0. Given that, its probably not worth the effort.

          Regarding your comments on API contract and breakage, I agree in principle, but I can imagine scenarios where your second bullet could lead to badness - exceptions propagating up the stack when they could have been handled. Your third bullet is valid, but not for a .x release that we want to be a drop-in replacement. So basically, I am still in agreement with your former self on this

          Show
          Phil Steitz added a comment - I guess I am OK with the backwards compatible change to SQLNestedException, but want to keep the deprecation and not increase its use as we should remove it in 2.0. Given that, its probably not worth the effort. Regarding your comments on API contract and breakage, I agree in principle, but I can imagine scenarios where your second bullet could lead to badness - exceptions propagating up the stack when they could have been handled. Your third bullet is valid, but not for a .x release that we want to be a drop-in replacement. So basically, I am still in agreement with your former self on this
          Hide
          Niall Pemberton added a comment -

          Doh! Didn't re-read that thread - and I even agreed at the time - how stupid does that look!

          Anyway, having said that I guess I'm going to lose the argument, but here goes anyway:

          • The contract of the API doesn't specify that SQLNestedException is thrown, just SQLException - so no-one should be relying on that implementation being thrown
          • If they did anticipate it being thrown, then they should have done it as a specific } catch(SQLNestedException) { block so that if another implementation is thrown their code doesn't break
          • Since DBCP 1.3 is now a minimum of JDK 1.4 which has a getCause() method - then anyone wanting to unwrap can do so without casting to SQLNestedException

          I guess its not a big deal, except for the guy who has a remote client side GUI that knows nothing about DBCP.

          I'll give up now, but I think we should apply the changes to SQLNestedException - since that simplifies it greatly, is completely backwards compatible and removes the hacks required for JDK 1.3

          Show
          Niall Pemberton added a comment - Doh! Didn't re-read that thread - and I even agreed at the time - how stupid does that look! Anyway, having said that I guess I'm going to lose the argument, but here goes anyway: The contract of the API doesn't specify that SQLNestedException is thrown, just SQLException - so no-one should be relying on that implementation being thrown If they did anticipate it being thrown, then they should have done it as a specific } catch(SQLNestedException) { block so that if another implementation is thrown their code doesn't break Since DBCP 1.3 is now a minimum of JDK 1.4 which has a getCause() method - then anyone wanting to unwrap can do so without casting to SQLNestedException I guess its not a big deal, except for the guy who has a remote client side GUI that knows nothing about DBCP. I'll give up now, but I think we should apply the changes to SQLNestedException - since that simplifies it greatly, is completely backwards compatible and removes the hacks required for JDK 1.3
          Hide
          Phil Steitz added a comment -

          Per comments on the thread referenced in DBCP-143, I am concerned that this could break clients that try to unpack SQLNestedException.

          Show
          Phil Steitz added a comment - Per comments on the thread referenced in DBCP-143 , I am concerned that this could break clients that try to unpack SQLNestedException.
          Niall Pemberton made changes -
          Link This issue is related to DBCP-143 [ DBCP-143 ]
          Niall Pemberton made changes -
          Attachment DBCP-310-v2.patch [ 12426333 ]
          Niall Pemberton made changes -
          Comment [ OK, then we should consistently use the ,initCause(e) method and remove all references that throw SQLNestedException.

          Attaching a new patch ]
          Niall Pemberton made changes -
          Attachment DBCP-310-v2.patch [ 12426333 ]
          Niall Pemberton made changes -
          Attachment DBCP-310-v2.patch [ 12426332 ]
          Hide
          Niall Pemberton added a comment -

          OK, then we should consistently use the ,initCause(e) method and remove all references that throw SQLNestedException.

          Attaching a new patch

          Show
          Niall Pemberton added a comment - OK, then we should consistently use the ,initCause(e) method and remove all references that throw SQLNestedException. Attaching a new patch
          Hide
          Mark Thomas added a comment -

          I'd rather see the back of this completely as per DBCP-143. That means leaving it deprecated for now and removing it in a 2.0 release which is now more likely given that there is likely to be a pool 2.0.

          Show
          Mark Thomas added a comment - I'd rather see the back of this completely as per DBCP-143 . That means leaving it deprecated for now and removing it in a 2.0 release which is now more likely given that there is likely to be a pool 2.0.
          Niall Pemberton made changes -
          Fix Version/s 1.3 [ 12311977 ]
          Fix Version/s 1.4 [ 12312615 ]
          Priority Major [ 3 ] Minor [ 4 ]
          Description SQLNestedException in its current state is a hangover from supporting JDK 1.3 when there was no "initCause()" method. This implementation can now be greatly simplified with DBCP now having a minimum of JDK 1.4

          Also SQLNestedException is deprecated and the following code has been used in a number of places

          {code}
               (SQLException )new SQLException(message).initCause(e)
          {code}

          DBCP is inconsistent though - sometimes using the above and sometimes using SQLNestedException. IMO SQLNestedException should be un-deprecated and used consistently everywhere - its now a simple implementation and I think the code is cleaner using it rather than the above.
          SQLNestedException in its current state is a hangover from supporting JDK 1.3 when there was no "initCause()" method. This implementation can now be greatly simplified with DBCP now having a minimum of JDK 1.4

          Also SQLNestedException is deprecated and the following code has been used in a number of places

          {code}
               throw (SQLException )new SQLException(message).initCause(e)
          {code}

          DBCP is inconsistent though - sometimes using the above and sometimes using SQLNestedException. IMO SQLNestedException should be un-deprecated and used consistently everywhere - its now a simple implementation and I think the code is cleaner using it rather than the above.
          Niall Pemberton made changes -
          Field Original Value New Value
          Attachment SQLNestedException.patch [ 12426325 ]
          Niall Pemberton created issue -

            People

            • Assignee:
              Unassigned
              Reporter:
              Niall Pemberton
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development