Feature/decouple query builder from query execution

[pavlo-liapota]

Goal

The goal of this PR is to allow execution of a custom SQL (for example, built by JOOQ library) with Reactive Wizard.

Usage

Potential usage may look like something like this:

public <T> Flux<T> selectFlux(JooqSelectStatement jooqStatement, Class<T> cls) {
    String sql = jooqStatement.getSql();
    List<Object> args = jooqStatement.getArgs();
    var deserializer = new DbResultSetDeserializerImpl<>(cls);
    return reactiveStatementFactory.createFlux(
        createMetrics("query", sql),
        sql,
        () -> new SelectSqlStatement<>(sql, createParamSetters(args), deserializer)
    );
}

private Metrics createMetrics(String methodType, String sql) {
    var metricsName = String.format("DAO_type:%s_method:%s", methodType, sql);
    return Metrics.get(metricsName);
}

private List<ParamSetter> createParamSetters(List<Object> args) {
    return args.stream()
        .map(arg -> {
            if (arg == null) {
                return (ParamSetter) PreparedStatementParameters::addNull;
            } else {
                return ParamSetterFactory.forType(arg.getClass(), () -> getListElementType(arg));
            }
        })
        .toList();
}

Changes

To simplify the review process, PR is divided to several commits:

1. Remove resultConverter from ReactiveStatementFactory
ReactiveStatementFactory always creates Mono if dao method return type is Mono and Flux if return type is Flux. So it looks like there is no need for a converter.
The check that return type is Mono or Flux was moved to ReactiveStatementFactory.

2. Remove paramSerializer from DbProxy
I did not find any usages of paramSerializer in DbProxy so I have removed it.

3. Move dao method related code from ReactiveStatementFactory to DaoMethodHandler
As example code above shows, the idea is that ReactiveStatementFactory can be used as a tool to create Mono or Flux that executes a Statement.
Everything related to a dao method was moved out from ReactiveStatementFactory and everything required for an execution of a statement was moved in from DbProxy class.
Previously ReactiveStatementFactory was representing pre-calculations done for a dao method. Now this logic has been moved to DaoMethodHandler class.

4. Rename DbResultSetDeserializer to DbResultSetDeserializerImpl
I have created interface named DbResultSetDeserializer in the next step, so implementation class was renamed.

5. Add DbResultSetDeserializer interface and generic types
DbResultSetDeserializer interface is added to allow to swap the implementation if needed.
Generic types were provided in several places to remove compiler warnings.

6. Add Statement implementation that is independent from ParameterizedQuery
New implementations of Statement were added. They work the same, but they are independent from ParameterizedQuery. They accept already built sql and parameter setters based on PreparedStatementParameters class.
StatementDebug.log(preparedStatement); is executed in all cases now.

7. Make StatementFactory create new Statement implementation
ParameterizedQuery was updated to be used with new Statements: it does not create prepared statement and add parameters anymore, it builds sql and creates parameters setters instead.
Old StatementFactorys were removed. New StatementFactorys create new Statement implementations instead.

Questions

1. When sql is built by jooq, everything is done at runtime. It is not possible to pre-calculate anything. But it should not be a problem, right?

2. When we have doa method, the metric name is the method name. But with jooq, there is no identifier, we can only use sql, but it can be too long. Can you suggest anything better?

3. If statement parameter is a list, then it is possible to check its generic type and use proper parameter setter (see ParamQueryPart#getListElementType). But with jooq we don't know the generic type of the list at runtime. Is it fine to check that all list elements are of a specific type instead?

4. If dao method returns Mono, but select query returns multiple values, we throw an exception. But if update statement returning generated values returns multiple values, then we just returns the first one. Is it intended? I did not change this logic.

The PR is quite big. Let me know if you want to have a meeting to discuss it.

[sonarqubecloud]

Quality Gate passed

Issues
34 New issues

Measures
0 Security Hotspots
91.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[JonasHallFnx]

When I designed reactive-wizard, the static sql queries was an important part for some different reasons:

• Security; it is not possible to make a sql-injection attack on a system that can only run static sql. It should not be possible for developers to make any mistake.
• Performance; when the query is dynamic, the developer could easily make a mistake that results in a query with e.g. thousands of conditions (by mistake, by making the query structure depend on some data). The database could probably make less optimizations if queries are different in structure for every execution.
• Testability; when the query is a 1-1 representation of a method in an interface, mocking it in tests is really simple.

I also think that you could build functionality like in this PR on top of the ConnectionProvider and therefore have a module that is separate from reactive-wizard but still giving you the same features. Either in an internal or public repo.

I vote for not including this in reactive-wizard based on my arguments above.

[mkfortnox]

2. When we have doa method, the metric name is the method name. But with jooq, there is no identifier, we can only use sql, but it can be too long. Can you suggest anything better?

You really want to have the caller provide the label even if if the number of unique statement texts generated by jooq will have low cardinality. All meaningful application context is lost once it has been translated to text. Crash on missing metric label instead of attempting to solve it.

Edit. Since we'll want to emit trace spans and will need the same type of label/tag there. Would using the tracing context be a suitable way to propagate this information down from the method that corresponds to the *Dao methods?

[mkfortnox]

Since the end goal here is to enable a usable and safe DSL over string hacking like #680 it may be counterproductive to use string representations of queries as the interface here. The only place where you really need to be able to execute both jooq and rw dao statements back to back on the same connection is during a write transaction. Could the DaoTransactions interface be a better extension point than the statement level?

[pavlo-liapota]

Thanks for the comments Jonas!

I also think that you could build functionality like in this PR on top of the ConnectionProvider and therefore have a module that is separate from reactive-wizard but still giving you the same features. Either in an internal or public repo.

Few weeks ago I have created all this functionality outside of the RW, just to see if this is possible at all.
Here are the changes: https://git.fnox.se/projects/FNF/repos/finance-ledger/commits/89ec511f0d35d24b097fcebbbb868f6a2eaac8fc
Everything in jooq package except of QueryRunner class is code copied from RW-dao and slightly modified to support my use case.
QueryRunner class is an adapter between JOOQ query builder and RW.
And JooqXxxDao classes are examples how it will be used.

But as I said I have copied a lot of the code from RW-dao. Which means that when RW-dao gets an update (like migration from Observable to Flux and Mono), I would need to update my code too.
So refactoring RW-dao to support my usecase looked like a good idea. It allows to avoid having the duplicated code and the need to maintain it myself.
If I duplicate the RW-dao code and put it for example into RW-kotlin repo, then most likely it will not keep up with the changes in RW-dao and there would be a chance to miss a security update or a bug fix.

I understand the reasons why you want to support only static sql in RW, but will my changes actually change how RW is used by developers? Only implementation details are refactored. Will developers even notice? If developer wants to build sql in other way and continue to use RW to execute it, this is possible even right now (as my changes linked above show). This PR just makes it easier to build on top of RW.

The goal is to build sql with JOOQ query builder. It has predefined building methods and type safety that reduce a risk of writing an invalid sql.
JOOQ will build the same sql every time in most of the cases. Only exception is filtering. Currently our sqls have a lot of ? is null or column = ? which we can avoid using JOOQ.
And as you can see from my example, 1-1 representation of a sql to a method will stay. So it can be mocked the same way.

If I was not able to convince you, would you agree to keep first 3 commits? It will remove the need to duplicate big part of RW-dao code.

[splitfeed]

Just wanted to update everyone that there are discussions ongoing regarding this, so it's not standing still but not yet ready to be approved or merged :)

[mkfortnox]

The hypothesis here at the moment is that we'd probably want to merge the first 3-5 commits that are clean ups of the existing code. That would make it significantly easier to review the contentious changes and revisit the design of them in a follow-up PR.

[pavlo-liapota]

There are a lot of changes in this PR. So to make the review process easier PR #706 was opened instead, which is 4 times smaller. We may discuss other changes later.