In reviewing code in our application, I see a tension between what makes a convenient design for methods accessing database data versus coding to ensure that the JDBC resource objects are closed properly.
An example problem:
// Application.java: Singleton class for objects which want to execute SQL queries
/**
* Execute a query and return the Result. Provide a common point of
* connection/statement handling
*
* @param sql
* SQL to execute
* @return a ResultSet
*/
public ResultSet executeQuery(String sql) throws Exception {
DBConnectionObj connectionObj = dbPoolMgr.getConnection();
try {
if (LOG.isDebugEnabled()) {
LOG.debug("StateServer.executeQuery(): executing query=" + sql);
}
return connectionObj.executeQuery(sql);
} catch (Exception e) {
LOG.error("executeQuery(): error executing SQL=" + sql);
throw e;
} finally {
dbPoolMgr.putConnection(connectionObj);
}
}
// DBConnectionObj.java: JDBC Connection wrapper used in a connection pool
public ResultSet executeQuery(String sql, Connection connection) {
try {
if (Log4jUtils.isDebugLevel(LOG)) {
LOG.debug("executeQuery(): " + sql);
}
return connection.createStatement().executeQuery(sql);
} catch (SQLException e) {
UnsuccessfulSQLExecutionResult result = new UnsuccessfulSQLExecutionResult(e, sql);
LOG.error("executeQuery(): exception executing query. SQL info=" + result.getContents());
e.printStackTrace();
return null;
}
}
With above helper functions, other application classes can just pass the specific SQL and get back a reference to the ResultSet:
ResultSet rs = Application.getInstance().executeQuery(getSql());
while (rs.next()) {
// process data
}
rs.close();
The problem with the DBConnectionObj.executeQuery() method, is that it creates a temporary Statement
to execute the query, and this temporary object is never closed. But also, because the method returns the ResultSet
to the caller, it can't close the Statement
until the caller is ready to close the ResultSet
. (The code doesn't need to close the Connection
object, because it stays open for the lifetime of the application, and is part of a pool of open database connections.)
One solution would be to make sure every caller to the executeQuery()
method, always does this:
rs.close();
rs.getStatement().close();
So the question is this: for usefulness, having a method which takes a SQL string for input and returns a ResultSet
feels like a good design. However, it imposes requirement that callers make sure to close both the ResultSet
and the Statement, and it can't take advantage of the safer Java 7 try-close feature:
try(ResultSet rs = Application.getInstance().executeQuery(getSql())) {
while (rs.next()) {
// process data
}
} // rs is automatically closed, but not the Statement object
Any comments or suggestions on a better design are appreciated.
if (LOG.isDebugEnabled()) {
andif (Log4jUtils.isDebugLevel(LOG)) {
looks like they checked the same thing, but they are not the same, is there a difference ? Do you really need to check ifdebug
is set ? (I may be wrong, I'm just curious) \$\endgroup\$Level.DEBUG.isGreaterOrEqual(logger.getEffectiveLevel())
to check, so I made a wrapper method so we didn't have to litter our code with that every time. I'm not sure if I somehow overlooked theisDebugEnabled()
method, or if it was added after we wrote the earlier code. \$\endgroup\$