DAO module clean up and refactoring

[pavlo-liapota]

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
There are no usages of paramSerializer field in DbProxy. I have checked internal RW too. So it looks like it is safe to remove it.

3. Move dao method related code from ReactiveStatementFactory to DaoMethodHandler
Currently DbProxy has two "roles":

• proxies any dao method
• has fields related to query execution (connectionScheduler, databaseConfig)

And ReactiveStatementFactory has two "roles":

• builds Mono or Flux for query executions
• handles dao method execution and stores precalculated information for a specific dao method

My idea is to distribute "roles" in the following way:

• DbProxy: proxies any dao method
• ReactiveStatementFactory: executes the query
• DaoMethodHandler: handles dao method execution and stores precalculated information for a specific dao method

So this refactoring has moved the following code:

• precalculated info for dao method and dao method handling from ReactiveStatementFactory to a new class DaoMethodHandler
• everything needed for query execution from DbProxy to ReactiveStatementFactory

Note: previously if query execution was slow then the query sql was logged, now the metric name related to the dao method will be logged instead.
We did this change so that we don't need to pass sql text as a parameter.
And it will be easier for a developer to find this query in the code.

So previously log message may look like this:

Slow query: select * from table where name=:name
time: 6000

Now it may look like this:

Slow query: DAO_type:query_method:com.fortnox.reactivewizard.dao.MyDao.doSomethingMono_1 time: 6000

[ghost]

And just to confirm that we're on the same page here this PR does not add support for execution of arbitrary SQL text unless you implement the full Statement interface etc, right?

[sj-robert]

Suggestion: reword from "Slow query:" to "Slow execution"; most often it's not the database that's slow, it's the thread being starved due to garbage collection.

[pavlo-liapota]

And just to confirm that we're on the same page here this PR does not add support for execution of arbitrary SQL text unless you implement the full Statement interface etc, right?

Yes 👍

[niklasga]

I would really like to see a much higher grade of code coverage here. Regardless of coverage of the old code, it was battle tested and known to work. If we are to make these changes and feel confident that it works, good coverage of tests is key.

Most of the coverage from ReactiveStatementFactoryTestDaoMethodHandlerTest comes from a single test that focuses only on releasing schedule workers, so I doubt much of the coverage is actually real coverage.

Edit: I did not notice that ReactiveStatementFactoryTest was renamed to DaoMethodHandlerTest. This just means that DaoMethodHandlerTest is poorly covered and ReactiveStatementFactory doesn't have a single unit test.

[softqwewasd]

Naming here is slightly ambiguous since we are not actually running the handler, we are creating a handler?

[softqwewasd]

Have you checked that these changes work with the internal RW? Is there a branch in the internal RW that we can view?

[niklasga]

I cannot say much about the functionality of the dao module, but I have a few concerns quality wise. Hopefully it should not be too painful to get some decent code coverage and add javadocs :)

[niklasga]

Please use actual types instead of var, especially when it's not easily inferred from the code.

[niklasga]

Add javadocs to explain the purpose of the class and at least the public methods on it.

You had some good explanations of classes and concepts in your PR description, those would be nice to transfer to well placed comments for future reference :)

[niklasga]

Will this not result in creating new RuntimeExceptions for each error, as opposed as the old code reusing the same one?

[pavlo-liapota]

Will this not result in creating new RuntimeExceptions for each error, as opposed as the old code reusing the same one?

In the old code, each time method ReactiveStatementFactory#create is called, the new instance of exception is created.
The only difference is that now exception is created only if error happens while previously it was created even if error didn't happened.

The reusability of the exception instance could only happen in the old code if lambda provided to method onErrorResume is called several times. I suppose this may happen if we add a retry. Is there any benefit in reusing the exception instance in this case?

[niklasga]

The old create method actually had javadocs that you removed, would you please add them to your new create-methods?

[softqwewasd]

Have you checked if any other dependents other than internal RW are impacted by these changes?

[pavlo-liapota]

Have you checked that these changes work with the internal RW? Is there a branch in the internal RW that we can view?

Yes. I have created a branch feature/NOJIRA-update-RW-dependency.

[pavlo-liapota]

Have you checked if any other dependents other than internal RW are impacted by these changes?

No, but I don't see an issue if others are dependent too. They would need to update the constructor usages as was done in internal RW.

[pavlo-liapota]

I would really like to see a much higher grade of code coverage here. Regardless of coverage of the old code, it was battle tested and known to work. If we are to make these changes and feel confident that it works, good coverage of tests is key.

Most of the coverage from ReactiveStatementFactoryTestDaoMethodHandlerTest comes from a single test that focuses only on releasing schedule workers, so I doubt much of the coverage is actually real coverage.

Edit: I did not notice that ReactiveStatementFactoryTest was renamed to DaoMethodHandlerTest. This just means that DaoMethodHandlerTest is poorly covered and ReactiveStatementFactory doesn't have a single unit test.

I have renamed DaoMethodHandlerTest back to ReactiveStatementFactoryTest and have created a separate unit test for DaoMethodHandler.
ReactiveStatementFactory is tested through DbProxyTest.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

[pavlo-liapota]

Hopefully it should not be too painful to get some decent code coverage and add javadocs :)

I have added javadocs and more tests.
To increase code coverage more, I would need to set DebugUtil.IS_DEBUG to true. Do you know how to do it in the unit test?

[jepp3]

I looked through the pr and i just have some general thoughts that I do not think you need to resolve now to get the pr merged.

• The class DaoMethodHandler was a bit hard to understand. I would have placed the variables in a record and extracted the create method to a method in the Proxy class. ( if the caching is needed for other things than the reactivestatementfactory. Like this:

record DaoMethodContext(Method method,
                               DbStatementFactory statementFactory,
                               PagingOutput pagingOutput,
                               Metrics metrics) { }

Then the proxy class would look something like this:

 @Override
    public Object invoke(Object proxy, Method method, Object[] methodArgs) throws Throwable {
        DaoMethodContext methodContext = methodContexts.get(method);
        if (methodContext == null || DebugUtil.IS_DEBUG) {
            if (DebugUtil.IS_DEBUG) {
                // Need to get the actual interface method in order to get updated annotations
                method = ReflectionUtil.getRedefinedMethod(method);
            }

            methodContext = new DaoMethodContext(
                method,
                dbStatementFactoryFactory.createStatementFactory(method),
                new PagingOutput(method),
                createMetrics(method)
            );
            methodContexts.put(method, methodContext);
        }

        return getExecutableStatement(methodArgs, methodContexts.get(method));
    }

    public static Publisher<Object> getExecutableStatement( Object[] args, DaoMethodContext daoMethodContext) {
        this.reactiveStatementFactory.createFlux() // the code from the create method.
    }

• It could be good to rename the args parameter to methodArgs to be more clear, where it is used.
• the ifs and else are a bit verbose in the daoMethodHandler class. you could maybe make it a bit more clear by writing it like this :

      Class<?> returnType = daoMethodContext.method().getReturnType();
        
        if(returnType.isAssignableFrom()) {
            
        }
        if(returnType.isAssignableFrom()) {
            
        }
        throw 

But overall it looks nice, hope it works well for you :)

[pavlo-liapota]

@splitfeed Can you review again?