helidon-io / helidon

Java libraries for writing microservices
https://helidon.io
Apache License 2.0
3.51k stars 566 forks source link

Blocking DBClient Part 1 #6991

Closed romain-grecourt closed 1 year ago

romain-grecourt commented 1 year ago

Here is the initial proposal for the update of DBClient to a blocking style API.


DbExecute.

public interface DbExecute extends AutoCloseable {
    Stream<DbRow> query(String statement, Object... parameters);
    Optional<DbRow> get(String statement, Object... parameters);
    long insert(String statement, Object... parameters);
    long update(String statement, Object... parameters);
    long delete(String statementName, Object... parameters);
    long dml(String statement, Object... parameters);
}

AutoClosable

Note the use of AutoClosable.

In the blocking style, onComplete is achieved by the next statement.

List<String> names = exec.query("SELECT * FROM names").toList();
System.out.println("done");

onError is achieved by a try-catch block:

try {
    List<String> names = exec.query("SELECT * FROM names").toList();
    System.out.println("done");
} catch (DbClientException dce) {
    // ugh-oh
}

DbExecute should work like a session, where the object can be used multiple times to perform database operations. If DbExecute is designed as single-use, we should introduce a DbSession class that implements AutoClosable.

The try-with-resources block represents code that uses a session:

try (DbExecute exec = dbClient.execute()) {
  // use exec
}

Transaction

DbTransaction extends DbExecute and defines a contract where commit() MUST be called. When the auto close() is called, all non-committed and non rollbacked transactions are rollbacked.

public interface DbTransaction extends DbExecute {
    void commit();
    void rollback();
}
try (DbTransaction tx = dbClient.transaction()) {
    long count = tx.delete("DELETE FROM pokemons WHERE name = 'foo'");
    tx.commit();
    return count;
}

Stream.close

Use Stream close handler as an API to close the related ResultSet (for JDBC). However since Stream does not require try-with-resources, hook the closing of the stream as part of the enclosing AutoClosable (I.e. DbExecute).

This means, if the user uses a Stream produced by DbStatementQuery outside of a try-with-resources, then the stream is unusable.

The creation of the Stream<DbRow> can be done like this:

Stream<DbRow> stream = StreamSupport.stream(new AbstractSpliterator<DbRow>(Long.MAX_VALUE, Spliterator.ORDERED) {
    @Override
    public boolean tryAdvance(Consumer<? super DbRow> action) {
        try {
            if (!rs.next()) {
                return false;
            }
            DbRow dbRow = createDbRow(rs, metadata, dbMapperManager, mapperManager);
            action.accept(dbRow);
            return true;
        } catch (SQLException ex) {
            throw new DbClientException(ex.getMessage(), ex);
        }
    }
}, false).onClose(() -> {
    try {
        rs.close();
    } catch (SQLException e) {
        throw new DbClientException("Failed to close result-set", e);
    }
});

Boxed Long

Remove the result type parameter from DbStatement in order to avoid forcing a boxed Long as a return type. This means that the execute method has to be defined in all sub interfaces of DbStatement:

public interface DbStatementQuery extends DbStatement<DbStatementQuery> {
    Stream<DbRow> execute();
}
public interface DbStatementGet extends DbStatement<DbStatementGet> {
    Optional<DbRow> execute();
}
public interface DbStatementDml extends DbStatement<DbStatementDml> {
    long execute();
}

Interceptor

Update the interceptor mechanism to be blocking friendly similar to WebClientService:

public interface DbClientService {

    Object handle(Chain chain, DbClientServiceContext context);

    interface Chain {
        Object proceed(DbClientServiceContext context);
    }
}

MongoDB


Misc

trentjeff commented 1 year ago

What if the user really wants to use ExecutorService? For instance, if the user wants to have a timeout before canceling the query?

romain-grecourt commented 1 year ago

Unlike the reactive DBClient, the blocking DBCLient API does not need an executor service, it runs on whatever thread you are running on.

Tomas-Kraus commented 1 year ago

Timeout on statement execution makes sense in some corner cases. But I'd better examine JDBC API options and few other things before starting any threads. Also 1st step is to adopt existing reactive API and make 1:1 copy of blocking API/implementation. I'll try to preserve as much as possible from existing code patterns, but as Romain already mentioned, all reactive support has to be removed. Additional things may be added in follow up issues. But let's to the minimal required changes to fulfill this goal first.

Tomas-Kraus commented 1 year ago

@romain-grecourt Looks like the only thing that needs close() is Stream<DbRow>. DML/Get conenction/statement/resultset is closed before result (long or Optional) is returned from execute method. Those are single shot results so I can do that. Query result is Stream<DbRow>. So execute opens conenction/statement/resultset and passes them to onClose method of the stream. Stream reads ResultSet on the fly so everything must be opened during Stream processing phase.

I'll added javadoc comments to DbStatementQuery so hope users will at least read it.

Tomas-Kraus commented 1 year ago

Progress:

romain-grecourt commented 1 year ago

Update on closing.

Making DbExecute extend AutoCloseable forces everything to be done with a try-with-resources when in reality only query statements need closing. Single execution statements can be closed right away.

Using Stream.close() doesn't warn about the need to close like other AutoCloseable, thus it makes the API a bit unsafe if calling .close() is a requirement.

We can mitigate the issue by automatically closing on all terminal operations in the stream. This means the implementation has to use a stream delegate that intercepts all terminal operations in the stream.

We can also have a fail-safe where the Spliterator that powers the stream can ensure that the connection and result set are closed when the result set has nothing left.

This is a win-win, as we have less boilerplate and less requirement in order to properly use the API.

However, it is still possible to mis-use the stream API and consume items without using a terminal operation ; in this case the users must call .close(). This seems acceptable.

zimmi commented 1 year ago

Hi! Unsurprisingly, there is a lot of prior art when it comes to JDBC wrappers. I quite liked https://github.com/zsoltherpai/fluent-jdbc , which sports a similar API to what is worked on here. Maybe it can serve for inspiration? Thank you for working on this! :+1:

Edit: I'd also like to mention https://github.com/OpenGamma/ElSql which I've used in the past for externalizing SQL. Having those two kinds of libraries together provides enough abstraction layer for many apps. Maybe something like this can be considered as an extension to the current approach?

romain-grecourt commented 1 year ago

Edit: I'd also like to mention https://github.com/OpenGamma/ElSql which I've used in the past for externalizing SQL. Having those two kinds of libraries together provides enough abstraction layer for many apps. Maybe something like this can be considered as an extension to the current approach?

Currently queries can be defined externally using Helidon Config, we can consider an SPI to have an extension that integrates something like ElSql.

barchetta commented 1 year ago

This issue tracks the work done for 4.0.0-M1. For follow-up work see issue #7187