pass connection as parameter to AbstractCursorItemReader.cleanupOnClose() method [BATCH-1899]

[spring-projects-issues]

Martin Bosak opened BATCH-1899 and commented

I created a child class of JdbcCursorItemReader in which I needed to, in the openCursor() method create & seed a temporary table (the 'normal' retrievals return items from the temporary table). I found that, when I wanted to drop the table before the connection was closed, I didn't have access to the Connection object.

I ended up as a hack, in the openCursor() method of storing it in a thread local variable & accessed in the cleanup method (and removed from the threadlocal var).

---

Affects: 2.1.8

Referenced from: pull request #735

[spring-projects-issues]

Mahmoud Ben Hassine commented

I could agree on the suggested change to pass the connection to cleanupOnClose. However, I don't agree on a couple of things:

in the openCursor() method create & seed a temporary table

The openCursor is not meant for business code, I would create this temporary table and feed it upfront

I found that, when I wanted to drop the table before the connection was closed, I didn't have access to the Connection object.

You have access to the datasource so you can acquire another connection and cleanup resources as needed. That said, I can understand that it's easier and more efficient to have a handle to the connection and reuse it for the cleanup rather than acquiring a new connection from the datasource.

Michael Minella Any abjection on adding the connection as a parameter to cleanupOnClose? I don't see any side effect even if the connection is closed in that method, the code after cleanupOnClose in doClose is safe against null connections. This change would make the API consistent with its counterpart: the AbstractCursorItemReader#openCursor(Connection connection) used in doOpen.

[spring-projects-issues]

Mahmoud Ben Hassine commented

BTW, looking at the two implementations of AbstractCursorItemReader which are JdbcCursorItemReader and StoredProcedureItemReader, they both override the cleanupOnClose method as follows:

/**
 * Close the cursor and database connection.
 */
@Override
protected void cleanupOnClose() throws Exception {
	JdbcUtils.closeStatement(this.preparedStatement);
}

The Javadoc shows that it is supposed to close both the cursor AND the database connection (but it is not the case for the database connection, it is done in the parent class, but as I said, there is no side effect if the connection is closed here). So if we add a handle to the database connection in cleanOnClose, the implementation would be more coherent, something like:

/**
 * Close the cursor and database connection.
 */
@Override
protected void cleanupOnClose(Connection con) throws Exception {
	JdbcUtils.closeConnection(con);
	JdbcUtils.closeStatement(this.preparedStatement);
}

I made the changes in the BATCH-1899 to see how it would look like. I can open a PR if those changes make sense.