Commons DbUtils
  1. Commons DbUtils
  2. DBUTILS-4

[dbutils] ResultSetIterator should rethrow SQLExceptions as RuntimException

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Nightly Builds
    • Fix Version/s: 1.1
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      SQLExceptions are not handled.
      See TODOs in org.apache.commons.dbutils.ResultSetIterator.java

      From discussion in COM-1333
      I do not think that swallowing the exception would be a good decission, the user
      (of DbUtils) should know something went wrong and we should not hide application
      bugs and make debugging more difficult.

      I would implement this myself if nobody objects.

        Activity

        Hide
        Stefan Fleiter added a comment -

        Created an attachment (id=13575)
        Patch for rethrowing SQLException as SQLRuntimeException in
        ResultSetIterator.java

        Three things which are controversial:

        1. A subpackage named exceptions for only one class
        Maybe there will be other exceptions, this solution
        is ready for future enhancements.
        2. Implementation of all exception constructors, but only one is used
        Completeness.
        3. Having no unittest for this
        I have no time for adding one at the moment and I do not think
        think it is really needed.

        Please tell me what you think of this.

        Show
        Stefan Fleiter added a comment - Created an attachment (id=13575) Patch for rethrowing SQLException as SQLRuntimeException in ResultSetIterator.java Three things which are controversial: 1. A subpackage named exceptions for only one class Maybe there will be other exceptions, this solution is ready for future enhancements. 2. Implementation of all exception constructors, but only one is used Completeness. 3. Having no unittest for this I have no time for adding one at the moment and I do not think think it is really needed. Please tell me what you think of this.
        Hide
        David Graham added a comment -

        Why not just wrap them in RuntimeException and add a note to the javadoc?
        Adding an exception class plus a package seems like overkill.

        Show
        David Graham added a comment - Why not just wrap them in RuntimeException and add a note to the javadoc? Adding an exception class plus a package seems like overkill.
        Hide
        Stefan Fleiter added a comment -

        The reason for this is that one may need to distinguish the Exception from other
        RuntimeExceptions thrown by ones try-Block.

        Now you can wrap your code with something like this (beware: not tested!):
        try {
        while (iter.hasNext())

        { // do something }

        } catch (SQLRuntimeException e)

        { SQLException org = (SQLException) e.getCause(); throw new SQLException(org.getMessage(), org.getSQLState(), org.getErrorCode()); }

        It's the same reason we do not only distinguish between Exception and
        RuntimeException but have subclasses like IllegalArgumentException.

        Where is the problem with having another class, what does it cost?

        Show
        Stefan Fleiter added a comment - The reason for this is that one may need to distinguish the Exception from other RuntimeExceptions thrown by ones try-Block. Now you can wrap your code with something like this (beware: not tested!): try { while (iter.hasNext()) { // do something } } catch (SQLRuntimeException e) { SQLException org = (SQLException) e.getCause(); throw new SQLException(org.getMessage(), org.getSQLState(), org.getErrorCode()); } It's the same reason we do not only distinguish between Exception and RuntimeException but have subclasses like IllegalArgumentException. Where is the problem with having another class, what does it cost?
        Hide
        Stefan Fleiter added a comment -

        Created an attachment (id=14272)
        Rethrow SQLException as plain RuntimeException

        This patch does what was asked for in comment 2.
        Could this be applied?

        Show
        Stefan Fleiter added a comment - Created an attachment (id=14272) Rethrow SQLException as plain RuntimeException This patch does what was asked for in comment 2. Could this be applied?
        Hide
        David Graham added a comment -

        This may break current users of the iterator whether we throw a RuntimeException
        or a subclass of it. I'm hesitant to break backwards compatibility but
        supressing the exceptions makes the iterator rather useless anyways.

        Show
        David Graham added a comment - This may break current users of the iterator whether we throw a RuntimeException or a subclass of it. I'm hesitant to break backwards compatibility but supressing the exceptions makes the iterator rather useless anyways.
        Hide
        Stefan Fleiter added a comment -

        IMHO alls current uses of ResultSetIterator are broken because one can not
        reliably know whether an exception has happend and data in a traversal of an
        resultset is incomplete or not.
        Better fix this now than never.

        Show
        Stefan Fleiter added a comment - IMHO alls current uses of ResultSetIterator are broken because one can not reliably know whether an exception has happend and data in a traversal of an resultset is incomplete or not. Better fix this now than never.
        Hide
        David Graham added a comment -

        I added a protected rethrow(SQLException) method that throws a RuntimeException.
        We can't wrap the SQLException because our minimum Java level is 1.3. You can
        subclass and override rethrow() to throw any exception you like. This should be
        in the next nightly build.

        Show
        David Graham added a comment - I added a protected rethrow(SQLException) method that throws a RuntimeException. We can't wrap the SQLException because our minimum Java level is 1.3. You can subclass and override rethrow() to throw any exception you like. This should be in the next nightly build.
        Hide
        Stefan Fleiter added a comment -

        Making the type of Exception overrideable by implementing a protected rethrow
        method is simple an ingenious at the same time.
        Thanks a lot.

        Show
        Stefan Fleiter added a comment - Making the type of Exception overrideable by implementing a protected rethrow method is simple an ingenious at the same time. Thanks a lot.

          People

          • Assignee:
            Unassigned
            Reporter:
            Stefan Fleiter
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development