[dbutils] ResultSetIterator should rethrow SQLExceptions as RuntimException

• Key: DBUTILS-4
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Stefan Fleiter
• Affects Version/s: Nightly Builds
• Fix Version/s: 1.1

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.

Comments

[Stefan Fleiter]

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.

[David Graham]

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

[Stefan Fleiter]

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?

[Stefan Fleiter]

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

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

[David Graham]

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.

[Stefan Fleiter]

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.

[David Graham]

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.

[Stefan Fleiter]

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