Add Optional Support to JdbcTemplate

[marschall]

Extend JdbcTemplate to support Optional for cases where queries
return one or no row.

• Add a queryForOptional method for every queryForObject method in
  JdbcOperations.
• Implement the new methods in JdbcTemplate.
• Add tests for the new methods in JdbcTemplate.

Issue: SPR-12662

I have signed and agree to the terms of the SpringSource Individual
Contributor License Agreement.

[coryfoo]

Any reason why this PR has been open for almost 2 years without any comments? What is the hold up for getting this functionality into the next (or any) release?

[snicoll]

@coryfoo We were not allowed to use Java8 types in public interfaces since we are compatible with Java 6, 7 and 8. Only the current master can use Java8 directly. Thanks for the ping.

[golonzovsky]

Hey, since we are on spring5/java8 now, could we look into this again?

[marschall]

@golonzovsky the discussion in the JIRA turned up that this PR adds yet another set of methods to JdbcTemplate. So I went ahead and started experimenting with a builder based approach in marschall/jdbctemplate-ng. It is very rough around the edges and has no documentation so its probably best to start with the tests. I would appreciated feedback.

[rstoyanchev]

Team Decision: JdbcTemplate and other similar in the Spring Framework were not originally designed with java.util.Optional in mind. Those have already accumulated one too many overloaded methods, and adding even more variants exacerbates the issue. For that the use of Optional is not core to what the template does and only syntactic sugar. The same can be achieved with an independent interface/class that delegates to a JdbcTemplate internally.

[marschall]

@rstoyanchev I would understand this if there weren't Stream methods added.

[rstoyanchev]

The queryForStream methods were introduced for a concrete capability, the streaming of large results sets, and requires a more involved integration in the JdbcTemplate. Those methods return a "hot" Stream that lazily traverses the ResultSet , essentially interacting differently with the JDBC driver. This is orthogonal to the actual API used and the feature was originally based on Iterable in a separate library, see #18474.

As an additional thought, Stream can be turned to Optional with findFirst. If you also want to ensure a single result, there is DataAccessUtils where we could add methods that work on a Stream.

[rstoyanchev]

The last point is now scheduled in #27728.

[marschall]

I struggle with these arguments:

1. Iterating over the result set was already possible with RowCallbackHandler.
2. Most methods offered by Stream like #map, #filter, #skip, #limit should be avoided and instead be implemented in SQL. This promotes the adaptation of APIs that result in bad performance.
3. The stream methods do not operate with callbacks and can therefore not offer the resource management as the other methods. Instead resource management has to be done by user code by means of calling #close. Failing to do so will introduce resource leaks, one of the very problems JdbcTemplate was supposed to address.

[rstoyanchev]

I never said it was impossible, but only that the main reason wasn't the Stream API itself. Rather it is a feature request focused on something that otherwise requires a more involved integration.

Once available, a feature will be used for a wider range of cases than what it was introduced for, and some may be well suited for Stream operators. If you want to treat it as nothing more than Iterable you can do so easily, yet you have a range of choices for Collection types, reducing, Optional, etc. Your concern about SQL vs in-memory handling is more general, and bad choices can be made irrespective of that.

I don't really follow what you mean on 3) as the queryForStream method does close the connection.

[marschall]

Rather it is a feature request focused on something that otherwise requires a more involved integration.

I really struggle to see the value over existing RowCallbackHandler methods. The RowCallbackHandler methods also have the advantage that no leak is introduced if the call to Stream#close() if forgotten.

Your concern about SQL vs in-memory handling is more general, and bad choices can be made irrespective of that.

Sure, but with stream most of the choices are bad ones. You have to try really hard to find ones that aren't a bad idea. Reductions that don't result in scalar values come to find but even these are error prone and cumbersome due to the need to call #close on the stream. I went through the methods and the following seem all like a bad idea and shouldn't be called:

• #sequential
• #parallel, JDBC is single threaded
• #spliterator, a result set isn't splitable
• #unordered
• #filter
• #map, including #mapTo*, if can be done in SQL
• #distinct
• #sorted
• #peek
• #limit
• #skip
• #takeWhile
• #dropWhile
• #forEachOrdered
• #reduce, if resulting in a scalar value that could be done with an aggregation function
• #min
• #max
• #count
• #anyMatch
• #allMatch
• #noneMatch
• #findFirst, if the result set has more than one element
• #findAny, if the result set has more than one element

Methods that are likely ok

• #iterator
• #onClose
• #flatMap, including #flatMapTo*
• #forEach
• #toArray
• #toList
• #reduce, if not resulting in a scalar values
• #collect

I don't really follow what you mean on 3) as the queryForStream method does close the connection.

#queryForStream does not close the connection or statement. Stream#close() will close the connection and statement. The following will leak the statement and the connection

jdbcTemplate.queryForStream("SELECT 1", (rs, i) -> rs.getInt(1)).findAny();

to close the statement and the connection you have to use

try (Stream<Integer> stream = jdbcTemplate.queryForStream("SELECT 1", (rs, i) -> rs.getInt(1))) {
  stream.findAny();
}

In my opinion this is quite error prone. I set up an example with datasource-proxy to illustrate the issue https://github.com/marschall/jdbctemplate-queryforstream/blob/master/src/main/java/com/github/marschall/jdbctemplate/queryforstream/Main.java

[rstoyanchev]

Okay, you don't like how #18474 was implemented. Do you have a suggestion for an improvement?

[marschall]

@rstoyanchev sure, I see several options, some of them more focusing on smaller changes, other on bigger ones:

• One option would be to switch from returning a Stream to instead have the caller provide a StreamCallback. JdbcTemplate could then close the Stream which causes the resources to be released. Similar to ResultSetExtractor or RowCallbackHandler. This would be consistent with other methods on JdbcTemplate that close / release resources.
• It is an unfortunate design decision of the Stream API that it is not clear to the caller which streams need to be closed and which ones not. Unfortunately JSR-305 never happened so APIs can not tell tools like IDEs which streams need to be closed. We could write our own Stream implementation that closes on terminal operations but that would not be idiomatic Stream API and I would therefore not recommend this.
• We could try to build a custom stream implementation that tries to do a better job at optimization but that would quickly approach Speedment territory.
• We could try to write more comments for the #queryForStream methods that go into more details like which methods are fine and which ones you probably should not be calling. This comes close to admitting that Stream is the wrong API / abstraction. In addition such comments often go unread by the people who most need them.
• We could try to write our own Stream implementation that throws exceptions on some methods like #count() and #parallel() however this has two issues. First we need to decide on which methods we want to throw and which ones not. Second it again comes close to admitting that Stream is the wrong API / abstraction.

Going back to the original requirements of #18474 all I see is the requirement to "write a table with millions of rows to an output stream". And the author claiming

Those data cannot be reasonably fetched with the current JdbcTemplate

without providing any more details. I don't see why this can not be done with either a ResultSetExtractor or RowCallbackHandler. I had a look at the referenced springjdbc-iterable project but the provided examples and explanations don't help much either. I should note the project predates Java 8 / lambdas. Honestly I don't see why providing a lambda as a RowCallbackHandler to a #query method is such a problem while at the same time proposing passing a lambda to a Stream#forEach as a solution.

Without any more details I don't see any requirements that can not be solved just as well with the existing methods.