Can’t loop over ResultSet in Java

I have a problem. I am using the following MySQL driver in my java project:

// SET THE MYSQL DRIVER
Class.forName("com.mysql.cj.jdbc.Driver");
SqlConn sqlConn = new SqlConn();

In the SqlConn class, I have the following function:

public ResultSet executeQuery(String query) {
    Statement stmt = null;
    ResultSet rs = null;
    try {
        stmt = conn.createStatement();

        if (stmt.execute(query)) {
            rs = stmt.getResultSet();
        }

        // Now do something with the ResultSet ....
    } catch (SQLException ex) {
        System.out.println("SQLException: " + ex.getMessage());
        System.out.println("SQLState: " + ex.getSQLState());
        System.out.println("VendorError: " + ex.getErrorCode());
    } catch (Exception e) {
        e.printStackTrace();
    } finally {
        if (rs != null) {
            try {
                rs.close();
            } catch (SQLException ignored) {
            }
        }

        if (stmt != null) {
            try {
                stmt.close();
            } catch (SQLException sqlEx) {
            } // ignore

            stmt = null;
        }
    }

    return rs;
}

The function is used like this:

ResultSet result = sqlConn.executeQuery("SELECT Market, Coin FROM Wallets GROUP BY Market, Coin ORDER BY Market, Coin;");

But then when I want to loop over it like this:

while (result.next()) {
    System.out.println(result.getString("Market"));
    System.out.println(result.getString("Coin"));
    System.out.println();
}

I get the following error:

Exception in thread "main" java.sql.SQLException: Operation not allowed after ResultSet closed
    at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:129)
    at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:97)
    at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:89)
    at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:63)
    at com.mysql.cj.jdbc.result.ResultSetImpl.checkClosed(ResultSetImpl.java:464)
    at com.mysql.cj.jdbc.result.ResultSetImpl.next(ResultSetImpl.java:1744)
    at com.company.drivers.SimulatorDriver.main(SimulatorDriver.java:82)

What am I doing wrong and how can I fix this?

Answer

In the code you pasted, you first create a connection which you then fail to close (Resource leak!), then you make a statement and a resultset. These, you do close, always (via a finally block), before you return. Thus, this method creates a ResultSet which it immediately closes and returns this now closed resultset.

All of this stuff is 20+ year old thinking. It makes total sense to try to paper over JDBC with some nice methods, but there is absolutely no need to invent this wheel yourself; use JOOQ or JDBI which did this for you.

Some notes on this code if you insist on using this:

  1. Use try-with-resources whenever managing resources, don’t use finally blocks.

  2. All 3 things need closing (Connection, Statement, and ResultSet), but note that close() propagates: If you close a resultset, you just close the resultset, but if you close a statement, you also close any resultsets it spawned, and if you close a connection, you also close any statements (And thus all resultsets) it spawned, in turn. That makes writing such a method effectively impossible (how do you manage how to close things)? So, investigate lambdas. You want lambdas for retry anyway.

  3. Statement is almost entirely useless. The moment you have parameterized queries (i.e. SELECT * FROM users WHERE username = ... username the user just entered into a web form here ... you can’t use any of this: You can’t make a string where you concatenate the username in, because what if the username, as typed on the form, is whatever' OR 1 == 1; DROP TABLE users CASCADE; EXEC 'format C: /y';? No, the only way to go is PreparedStatement which has support for parameterization that doesn’t immediately open you up to SQL injection attacks.

  4. Seriously, JOOQ or JDBI did all this, and far better.

  5. You don’t need Class.forName("com.mysql.cj.jdbc.Driver"); – haven’t needed it for 20 years.

  6. This model of ‘start with nothing, and out comes a resultset’ does not work. DBs have transactions; there is a greater context (a transaction) which usually encompasses multiple queries. You need to redesign this API to take that into account: Either pass in a connection object to the executeQuery method, or make your own object representing a connection, and it would have query methods. JOOQ and JDBI covered this, too. Assuming non-disastrously-dangerous isolationlevels, even a series of read-only queries needs transactions.

Leave a Reply

Your email address will not be published. Required fields are marked *