EvidentSolutions / dalesbred

Dalesbred - a database access library for Java
https://dalesbred.org
MIT License
54 stars 15 forks source link

Support for Stream API #38

Open mme-private opened 7 years ago

mme-private commented 7 years ago

Is there any thought about adding stream support as return option? I understand that for stream to work, Statement and ResultSet must be open and there would be needed some special cleanup. Probably it would work only for existing transactions and not for implicitly created.

First cleanup could happen in Stream.onClose handler, but if Stream is not closed, then it could happen just before connection release in DefaultTransaction (statements and result sets would be registered there for cleanup).

So if someone would use stream like

SqlQuery query = query("select id, name from department where update_timestamp > ?", date);
try (Stream<Department> stream = db.streamAll(Department.class, query)) {
    stream....
}

then all resources would be released after stream is used up, otherwise resources will be released when connection is released.

In RowMapper interfaces would be something like code below, but executed not from executeQuery method which closes PreparedStatement

     default ResultSetProcessor<Stream<T>> stream() {

        return resultSet -> StreamSupport
                .stream(new Spliterators.AbstractSpliterator<T>(Long.MAX_VALUE, Spliterator.ORDERED) {
                    @Override
                    public boolean tryAdvance(Consumer<? super T> action) {
                        try {
                            if (!resultSet.next())
                                return false;
                            action.accept(mapRow(resultSet));
                            return true;
                        } catch (SQLException ex) {
                            throw new RuntimeException(ex);
                        }
                    }
                }, false).onClose(() -> {
                    try {
                        resultSet.close();
                    } catch (SQLException e) {
                        throw new DatabaseSQLException(e);
                    }
                });
    }
komu commented 7 years ago

Thanks for the suggestion.

I've thought about this along the same lines, so I'm open to the idea.

I have some concerns though: nobody reads documentation and I'm afraid people would just end up calling streamAll without bothering to check if they need to release the resources. I'd guess that most people don't even know that you can close Streams or at least think about such concerns upon seeing a stream returned. Perhaps I shouldn't worry about that: ResultSet would be closed when the transaction is finished and shame on the programmer for not reading the Javadoc and releasing resources earlier.

Other annoying thing is that this would not work with implicit transactions, unlike other methods in Database.

(One kind of solution to both of these problems would be to implement a method that would allow a callback Function<Stream,T>, similar to withTransaction. But I guess the API ergonomics would be so bad that the solution is worse than the problem of possibly leaking resources.)

So I have some reservations (which is why I haven't implemented this so far), but of course I want to provide useful functionality, if it's needed. So I will merge a reasonable PR and provide help if necessary. I might possibly even implement this myself, but don't hold your breath for that because I have quite a lot of other things to do as well. 😄