Returning a ResultSet

Question

I am trying to create a method from where I can query my database and retrieve a whole table.

Currently, it works just fine if I use the data inside the method. However, I want the method to return the results.

I'm getting a java.sql.SQLException: Operation not allowed after ResultSet closed on the current code.

How can I achieve this?

public ResultSet select() {

    con = null;
    st = null;
    rs = null;

    try {
        con = DriverManager.getConnection(url, user, password);
        st = con.createStatement();

        rs = st.executeQuery("SELECT * FROM biler");
        /*
        if (rs.next()) {
            System.out.println(rs.getString("model"));
        }*/

    } catch (SQLException ex) {
        Logger lgr = Logger.getLogger(MySQL.class.getName());
        lgr.log(Level.SEVERE, ex.getMessage(), ex);

    } finally {
        try {
            if (rs != null) {
                rs.close();
            }
            if (st != null) {
                st.close();
            }
            if (con != null) {
                con.close();
            }

        } catch (SQLException ex) {
            Logger lgr = Logger.getLogger(MySQL.class.getName());
            lgr.log(Level.WARNING, ex.getMessage(), ex);
        }
    }

    return rs;
}

Why couldn't you interpret the obvious error message - "*java.sql.SQLException: Operation not allowed after ResultSet closed*"? — Lion, Feb 13 '13 at 12:27

BalusC · Accepted Answer · 2015-01-13T14:19:59.623

You should never pass a ResultSet around through public methods. This is prone to resource leaking because you're forced to keep the statement and the connection open. Closing them would implicitly close the result set. But keeping them open would cause them to dangle around and cause the DB to run out of resources when there are too many of them open.

Map it to a collection of Javabeans like so and return it instead:

public List<Biler> list() throws SQLException {
    Connection connection = null;
    PreparedStatement statement = null;
    ResultSet resultSet = null;
    List<Biler> bilers = new ArrayList<Biler>();

    try {
        connection = database.getConnection();
        statement = connection.prepareStatement("SELECT id, name, value FROM Biler");
        resultSet = statement.executeQuery();

        while (resultSet.next()) {
            Biler biler = new Biler();
            biler.setId(resultSet.getLong("id"));
            biler.setName(resultSet.getString("name"));
            biler.setValue(resultSet.getInt("value"));
            bilers.add(biler);
        }
    } finally {
        if (resultSet != null) try { resultSet.close(); } catch (SQLException ignore) {}
        if (statement != null) try { statement.close(); } catch (SQLException ignore) {}
        if (connection != null) try { connection.close(); } catch (SQLException ignore) {}
    }

    return bilers;
}

Or, if you're on Java 7 already, just make use of try-with-resources statement which will auto-close those resources:

public List<Biler> list() throws SQLException {
    List<Biler> bilers = new ArrayList<Biler>();

    try (
        Connection connection = database.getConnection();
        PreparedStatement statement = connection.prepareStatement("SELECT id, name, value FROM Biler");
        ResultSet resultSet = statement.executeQuery();
    ) {
        while (resultSet.next()) {
            Biler biler = new Biler();
            biler.setId(resultSet.getLong("id"));
            biler.setName(resultSet.getString("name"));
            biler.setValue(resultSet.getInt("value"));
            bilers.add(biler);
        }
    }

    return bilers;
}

By the way, you should not be declaring the Connection, Statement and ResultSet as instance variables at all (major threadsafety problem!), nor be swallowing the SQLException at that point at all (the caller will have no clue that a problem occurred), nor be closing the resources in the same try (if e.g. result set close throws an exception, then statement and connection are still open). All those issues are fixed in the above code snippets.

Thank you for taking your time to write an in-depth answer. Using this code, I would have to make a method for each and every table. Is that really the best way to do it? — Patrick Reck, Feb 13 '13 at 12:53
Yes, when you stick to low level JDBC. You can however refactor the repeated boilerplate code to a pretty high degree like Hibernate did a decade ago. No, in my humble opinion JPA is the best way. It's then a matter of a `return em.createQuery("SELECT b FROM Biler b", Biler.class).getResultList();` oneliner. — BalusC, Feb 13 '13 at 12:56
Can you please explain why you said "major threadsafety problem" for instance variable "conn"? If it is not static, why is the concurrency problem? I cannot see, because the conn is available only within the instance, not the class — user1156544, Oct 27 '17 at 13:40

score 16 · Answer 2 · answered Feb 13 '13 at 12:59

If you don't know what you want of the ResultSet on retrieving time I suggest mapping the complete thing into a map like this:

    List<Map<String, Object>> resultList = new ArrayList<Map<String, Object>>();
    Map<String, Object> row = null;

    ResultSetMetaData metaData = rs.getMetaData();
    Integer columnCount = metaData.getColumnCount();

    while (rs.next()) {
        row = new HashMap<String, Object>();
        for (int i = 1; i <= columnCount; i++) {
            row.put(metaData.getColumnName(i), rs.getObject(i));
        }
        resultList.add(row);
    }

So basically you have the same thing as the ResultSet then (without the ResultSetMetaData).

Does this approach take more memory as we create a separate hashMap than just having the resultSet and not closing it? — ramu, Nov 13 '15 at 22:47
You can close the statement and connection after the while loop, then you will have just the resultlist object, which means it will not a problem. — mcvkr, Oct 05 '18 at 13:59

Joachim Sauer · Answer 3 · 2013-02-13T12:58:33.887

Well, you do call rs.close() in your finally-block.

That's basically a good idea, as you should close all your resources (connections, statements, result sets, ...).

But you must close them after you use them.

There are at least three possible solutions:

don't close the resultset (and connection, ...) and require the caller to call a separate "close" method.

This basically means that now the caller needs to remember to call close and doesn't really make things easier.

let the caller pass in a class that gets passed the resultset and call that within your method

This works, but can become slightly verbose, as you'll need a subclass of some interface (possibly as an anonymous inner class) for each block of code you want to execute on the resultset.

The interface looked like this:

public interface ResultSetConsumer<T> {
  public T consume(ResultSet rs);
}

and your select method looked like this:

public <T> List<T> select(String query, ResultSetConsumer<T> consumer) {
  Connection con = null;
  Statement st = null;
  ResultSet rs = null;

    try {
      con = DriverManager.getConnection(url, user, password);
      st = con.createStatement();

      rs = st.executeQuery(query);
      List<T> result = new ArrayList<T>();
      while (rs.next()) {
          result.add(consumer.consume(rs));
      }
    } catch (SQLException ex) {
      // logging
    } finally {
      try {
        if (rs != null) {
            rs.close();
        }
        if (st != null) {
            st.close();
        }
        if (con != null) {
            con.close();
        }
      } catch (SQLException ex) {
        Logger lgr = Logger.getLogger(MySQL.class.getName());
        lgr.log(Level.WARNING, ex.getMessage(), ex);
      }
    }
  return rs;
}

do all the work inside the select method and return some List as a result.

This is probably the most widely used one: iterate over the resultset and convert the data into custom data in your own DTOs and return those.

I would've used the third option, but I can't quite figure how to make a list containing all the information regardless of the chosen table — Patrick Reck, Feb 13 '13 at 12:42
@PatrickReck: why do you want to do it *regardless of the chosen table*? Different tables contain different data types. — Joachim Sauer, Feb 13 '13 at 12:44
Exactly. What I want is a single method from where I can pass an SQL call and return the data retrieved from the call. If I were to select the whole Car table (id, model), I would want those two returned. If it was a Customer table (id, name, adress, phone) I would want all of this data returned *using the same method* — Patrick Reck, Feb 13 '13 at 12:48
@PatrickReck: I'd strongly discourage this, but you *could* return a `List>` if you wanted (the map, obviously, contains a map from field name to value for each result). — Joachim Sauer, Feb 13 '13 at 12:54
@PatrickReck: Also, see my updated post for sample code for option #2, maybe that will help. — Joachim Sauer, Feb 13 '13 at 12:59

score 6 · Answer 4 · answered Jun 21 '14 at 18:20

As everyone before me said its a bad idea to pass the result set. If you are using Connection pool library like c3p0 then you can safely user CachedRowSet and its implementation CachedRowSetImpl. Using this you can close the connection. It will only use connection when required. Here is snippet from the java doc:

A CachedRowSet object is a disconnected rowset, which means that it makes use of a connection to its data source only briefly. It connects to its data source while it is reading data to populate itself with rows and again while it is propagating changes back to its underlying data source. The rest of the time, a CachedRowSet object is disconnected, including while its data is being modified. Being disconnected makes a RowSet object much leaner and therefore much easier to pass to another component. For example, a disconnected RowSet object can be serialized and passed over the wire to a thin client such as a personal digital assistant (PDA).

Here is the code snippet for querying and returning ResultSet:

public ResultSet getContent(String queryStr) {
    Connection conn = null;
    Statement stmt = null;
    ResultSet resultSet = null;
    CachedRowSetImpl crs = null;
    try {
        Connection conn = dataSource.getConnection();
        stmt = conn.createStatement();
        resultSet = stmt.executeQuery(queryStr);

        crs = new CachedRowSetImpl();
        crs.populate(resultSet);
    } catch (SQLException e) {
        throw new IllegalStateException("Unable to execute query: " + queryStr, e);
    }finally {
        try {
            if (resultSet != null) {
                resultSet.close();
            }
            if (stmt != null) {
                stmt.close();
            }
            if (conn != null) {
                conn.close();
            }
        } catch (SQLException e) {
            LOGGER.error("Ignored", e);
        }
    }

    return crs;
}

Here is the snippet for creating data source using c3p0:

 ComboPooledDataSource cpds = new ComboPooledDataSource();
            try {
                cpds.setDriverClass("<driver class>"); //loads the jdbc driver
            } catch (PropertyVetoException e) {
                e.printStackTrace();
                return;
            }
            cpds.setJdbcUrl("jdbc:<url>");
            cpds.setMinPoolSize(5);
            cpds.setAcquireIncrement(5);
            cpds.setMaxPoolSize(20);

 javax.sql.DataSource dataSource = cpds;

Here is the correct way to create `CachedRowSet`: https://stackoverflow.com/questions/2228462/are-there-any-good-cachedrowset-implementations-other-than-the-proprietary-sun-o — Vadzim, Jul 23 '18 at 21:28
1 possible dilemma; static code analysis tools like SonarQube just triggering on anything that implements "AutoCloseable", would likely flag the "CachedRowSet" creation (factory or direct) as being outside a "try-with-resources"/"try-finally" (kinda silly in this use-case). — galaxis, Jun 06 '22 at 04:14

score 4 · Answer 5 · edited Jul 23 '18 at 21:22

You can use the CachedRowSet object that is just for what you want:

public CachedRowSetImpl select(String url, String user, String password) {

    CachedRowSetImpl crs = null;

    try (Connection con = DriverManager.getConnection(url, user, password);
         Statement st = con.createStatement();
         ResultSet rs = st.executeQuery("SELECT * FROM biler");) {

        crs = new CachedRowSetImpl();
        crs.populate(rs);

    } catch (SQLException ex) {
        Logger lgr = Logger.getLogger(MySQL.class.getName());
        lgr.log(Level.SEVERE, ex.getMessage(), ex);


    } catch (SQLException ex) {
        Logger lgr = Logger.getLogger(MySQL.class.getName());
        lgr.log(Level.WARNING, ex.getMessage(), ex);
    }

    return crs;
}

You can read the documentation here: https://docs.oracle.com/javase/7/docs/api/javax/sql/rowset/CachedRowSet.html

Here is the correct way to create `CachedRowSet`: https://stackoverflow.com/questions/2228462/are-there-any-good-cachedrowset-implementations-other-than-the-proprietary-sun-o — Vadzim, Jul 23 '18 at 21:28

Brian Agnew · Answer 6 · 2013-02-13T12:43:42.617

You're closing the ResultSet and consequently you can't use it anymore.

In order to return the contents of the table, you'll have to iterate through the ResultSet and build a per-row representation (in a List, perhaps?). Presumably each row represents some entity, and I would create such an entity for each row.

while (rs.next()) {
   list.add(new Entity(rs));
}
return list;

The alternative is to provide some callback object, and your ResultSet iteration would call on that object for each ResultSet row. That way you don't need to build an object representing the whole table (which may be a problem if it's sizable)

   while (rs.next()) {
      client.processResultSet(rs);
   }

I would be reluctant to let clients close the result set/statement/connection. These need to be managed carefully to avoid resource leaks, and you're much better off handling this in one place (preferably close to where you open them!).

Note: You can use Apache Commons DbUtils.closeQuietly() to simply and reliably close the connect/statement/resultset tuple (handling nulls and exceptions properly)

cavok · Answer 7 · 2020-02-24T13:05:56.483

0

Assuming you can afford storing the entire result in memory, you may simply return some table-like structure. Using Tablesaw for instance, simply do

Table t = Table.read().db(rows);

with rows a standard java.sql.ResultSet. For details see here. Tablesaw becomes especially useful if you intend to slice-and-dice your data further as it gives you Pandas-like functionality.

edited Feb 24 '20 at 13:05

answered Feb 24 '20 at 11:27

cavok

31
2

score 0 · Answer 8 · answered Feb 24 '20 at 11:33

It is bad practice to return result set ,secondly you are already closing it so after closing it you can not use it anymore. I would suggest using Java 7 with multiple resource in try block will helpful you as suggested above. If you want entire table result ,you should return its output rather than resultSet.

Returning a ResultSet

8 Answers8

Linked

Related