Coverage Status

Coverage decreased (-0.002%) to 87.241% when pulling c956fe0 on kvasnickakb:flush-result-handler into 260dba8 on mybatis:master.

No answer usually means the question is difficult to answer. :)
I could be wrong, but you seem to try to use batch executor for a purpose that is not designed to do.

Hi harawata,

it is not generally about batch execution, but about the way how to intercept flushStatements method of Executor (for our purposes in batch mode).
We don't change any logic or purpose of batch executor at all, just add possibility to create custom interceptor which allows user to intercept flushStatements method.

Our solution is in "symmetry" with simple strategy/mode of Executor.

So thats why I don't understand the reason you closed this pull request.
Could you please provide us solution how to intercept this method or suggest us anyone we can discuss the problem?

Thank you

Hello @kvasnickakb ,

Could you post the entire implementation of the plugin?

Hi @harawata,

Thank you for reopening.

I pushed to github simple example maven application where is the usage of FlushResultHandler intercepting (feature we would like to add to mybatis).
Here is the the link GitHub link to flush interceptor example application

In the application, there is OptimisticLockingFlushInterceptor class which intercepts newly added FlushResultHandler.handleResults method and call OptimisticLocking application logic (which check number of affected rows - result of DAO update method - and throw exception if result is 0 - which means version column has already been updated by another call).

There is simple junit test, which use some example of Customer POJO and CustomerDAO (mybatis mapper class) and check if optimistic lock exception is thrown from interceptor. For test purposes I used h2 database, but it is not important.

Please take a look of it :)
Thank you

Thank you for the project @kvasnickakb !

So, the fundamental issue here is that plugins cannot intercept flushStatements() when it's not invoked directly, is that correct?
Then I would look for a way to fix that issue rather than introducing a completely new API.

Hi @harawata, huge thanks you are still active solving this pull request and for your answer.

Yes, exactly - it's not invoked directly. But to solve the problem is not as easy as it may seem.
Trust me, I spent lot of time to solve this problem via existing mybatis mechanisms or by making small changes to existing mybatis code. But I was not successful...
Maybe I am wrong and simple solution exists. I've tried to asked in mybatis group forum but nobody answers.

In following text I use my example application, I sent earlier, to describe the flow...

In CustomerDAOTest, transaction is commited when testOptimisticLockingConcurrent() method ends because mybatis batch mode is set. All update methods have been called. Commit is controlled via spring transaction manager.
I can intercept the Exectution.commit method, because it is called explicitely (from outside) from transaction manager. This intercepting works, but commit method is void method and there is no chance to get any results of executed CustomerDAO.update method.
In the body of commit method is called flushStatements method which is defined in same Exectution interface and returns List of BatchResult (this is what I need for resolving Optimistic Lock exception).
But the problem is that this method is called internally from commit method (BaseExecutor:commit -> this.flushStatements();).
And such method call never goes through the proxy.

This is the feature of java proxies and there is no simple workaround how to "fix" this issue. There is some workaround by using "self-refernce" proxy instance (adding new instance of Executor to existing BaseExector and do the flushStatements call on this proxied instance).
But this solution is not too pretty and has much bigger impact to mybatis code than adding new FlushResultHandler interface and intercepts this one.

In my opinion, if you want to keep using java proxies in mybatis, there is no simple way how to intercept Execution.flushStatements in batch mode.

And that's the reason we added the FlushResultHandler. It is not "completely new API". This is new interceptable interface which is only extension of actual interceptable interfaces in mybatis. So I think we follow actual mybatis intercepting "pattern".

Please, if you have any idea how to solve it simplier or if you know anyone who could help us, let me know.

Thank you

In my opinion, if you want to keep using java proxies in mybatis, there is no simple way how to intercept Execution.flushStatements in batch mode.

I pretty much agree with you on this.
I haven't given much thought about it (or at all), but I was actually thinking about using something other than Java proxy (Javassist would be ideal as it's already in the dependencies).
It still may not be easy and there must be some level of backward compatibility, but there was a similar report (#1239) and it is worth exploring a way to intercept indirect invocations (a.k.a. self-invocations), I thought.

Thank you for similar report, it was helpful. I think this also could be a way how to achieve our goal.
I remember when I tried to use Exectutor wrapper to implement flushStatements intercepting but I didn't notice public void setExecutorWrapper(Executor wrapper) method.

Question is for what purposes is Exectutor wrapper designed? I found the only one usage in mybatis inCachingExecutor. So maybe it was designed for caching purposes.

Anyway if we use it, we could try to set this wrapper in Configuration.newExecutor method. To be more specific same way like it has been done in #1239:

executor = (Executor) interceptorChain.pluginAll(executor);
executor.setExecutorWrapper(executor); // added

and in BaseExecutor.commit method call flushStatemets on wrapper instance (then it is ready for intercepting because it is called on proxy intance of Executor)

public void commit(boolean required) throws SQLException {
 if (this.closed) {
 throw new ExecutorException("Cannot commit, transaction is already closed");
 } else {
 this.clearLocalCache();
 this.wrapper.flushStatements(); // changed
 if (required) {
 this.transaction.commit();
 }
 }
 }

I would have to try it out but I think this should work. Would be this solution better (or more acceptable) than solution in PR?
Maybe, there are more possible inner called method candidates in BaseExecutor which could be called on this.wrapper instance to be ready for intercepting.

Thank you

@kvasnickakb ,

I don't remember the details about that PR. :D
Let me take another look.

Hi @kvasnickakb ,

Sorry about the delay.
The new plan looks better than the other solutions.
Are you willing to work on it?

Hi @harawata,

I have tried to implement our new plan by applying changes from the PR#1239 ...

Let me remind you the context.
Change in PR#1239 is about set wrapper attribute of BaseExecutor class. (instead of using this keyword is wrapper set up to plugin proxy).
This wrapper attribute is actually used only in CachingExecutor where is stored reference to this CachingExecutor instance.
To achieve my purposes (intercept flushStatements method), the flushStatements method is in PR#1239 called in BaseExecutor.commit as this.wrapper.flushStatements which enables to intercept flushStatements method in any of user application.
To keep some consistence and uniformity of implementation, on wrapper instance is called every public method referenced internally from BaseExecutor or any of its children (BatchExecutor, ...).

I applied changes from (from PR#1239) to my test application.

You can see it in this example in feature branch feature/PR1239 of my test application on github.
The only change is switching interceptor back to mybatis Executor.flushStatements:
@Intercepts({@Signature(type = Executor.class, method = "flushStatements", args = {})})

After run junit test CutomerDAOTest everything works well. So it could be possibly used as solution of our optimistic locking problem.

BUT if we use this second plan (changes from PR#1239) there could be some problem with "compatibility". I think this change is not minor change.
If anyone currently uses, in custom mybatis application, interceptor to intercept any of the methods, newly called on BaseExecutor.wrapper (here is te list: clearLocalCache, createCacheKey, flushStatements, rollback, query).
flushStatements method could be NOW intercepted for example in mybatis simple mode or flush() methods could be called explicitly from custom DAO logic where it works right.
After merging changes from PR#1239, this application starts to bahave in different way.
Because custom interceptor after merging PR#1239 starts to intercept more flushStatements calls then before and it could theoretically cause problems for someone.
And there could be similar situations for the other methods in the list.

Implementation from our (this) PR request is not so "invasive" as implementation from PR#1239.
It does not break/change any of user application logic after you merge this pull request and user updates on newer mybatis version (PR#1239 does).
It is new feature which could be used without impact to compatibility.

Resume is: we could use implementation from PR#1239, but there is some compatibility risk.

So please, let me know, what you think about it.

Thank you

Daniel

@kvasnickakb ,

I should have been clearer, but I was not going to use #1239 as-is.
I was thinking about much smaller change which focuses on flushStatements (for now, at least).
Please take a look at this branch:
https://github.com/mybatis/mybatis-3/compare/master...harawata:flushstatements-interceptor?expand=1

Regarding backward compatibility, it is true that existing interceptors will be invoked more frequently and it should definitely be listed in the backward incompatible changes in the release note, however, for plugins which intercept flushStatements, the impact looks relatively small (in the extra invocations, invocation.proceed() just returns an empty list).
Wouldn't you agree?

I can see the advantage of FlushResultHandler, but its functionality overlaps with Interceptor and providing both seems a little bit redundant and confusing, IMHO.

@emacarron
Any thoughts on this?

@harawata ,
thank you for the commit in your feature branch.

Ok, I agree with calling only flushStatements method on wrapper.
Compatibility impact will be "smaller" and we achieve funcionality we desire :)

So whats next?
We definitely need to notice this change in release notes.
We can close this PR and start another one from your feature branch, right?

Thank you for help!

Hello, @kvasnickakb ,

I needed some time to re-think about this.

In your original request, you wrote that you don't want to call flushStatement() explicitly, but in reality, if you perform batch operation, you should call flushStatement (or a method annotated with @Flush) explicitly to control the batch size because it affects performance.
And if you do, your plugin will be invoked as you expect.

So, I think that the current implementation is working as designed.
I apologize that I got off track from the beginning... 🙏

Hi @harawata,

Thank you for the answer. I have to disagree with you.

Batch mode is designed to keep sql statements during the transaction and flush them when transaction is commited (or when some select statement is called earlier).
Why should I call flush method explicitly? If I do this, I'll lost the feature of mybatis batch mode (to automatically flush all statements when transaction is commited).
It does not make any sence for me. Everything works perfect in mybatis batch mode as it is designed for now.
If I call flush method explicitly I will break batch behavior of transaction and that's what I wouldn't like to do.

The only lack here is that mybatis does not allow to intercept flushStatements method, in such case, because it is called internally (not on proxy) from commit method.
I think it should be right to give mybatis users possibility to intercept each call of flushStatements method (not only the ones from outside/explicit).

Or do you mean, our change could affect mybatis performance because each flushStatements method, called inside commit method, is newly called via proxy?
I don't think it does. Or we can do some performance tests to check it out.

Thank you.

Hi @kvasnickakb ,

If I call flush method explicitly I will break batch behavior of transaction

Flushing statements should have no effect on transaction regardless of whether it is managed by Spring or not.
Maybe I am missing something. Could you elaborate if that is the case?
I'll try to explain myself.

Here is an example batch operation code.
As explained, flushing statements every X rows usually gives you a better performance compared to flushing all rows at the end.
It also is necessary when you need the number of affected rows.
Here is another example using Spring managed transaction.

So, from what I see, all you have to do to trigger the plugin is to flush statements explicitly which has no disadvantage (well, except more typing) and is even necessary in some cases.
And it does not look too awful, considering that the solutions on the table require a possibly backward incompatible change [1] or a new API which overlaps with the existing plugin API.

We can keep this issue open for other devs to comment if you prefer.

[1] You might be aware, but my patch affects other methods than flushStatements() as well.

Hi @harawata,

to explicit flush:

Yes, you can call flush explicitly to possibly get better performance, but this differs per use case and some services does not have hundreds of db calls. So there is no need to optimize performance every time.
In our use case there are only units of sql statements and it's really not necessary to call flush explicitly for better performace.
For us this is not advantage. More likely there is disadvantage in more typing and mainly each developer in our team/company needs to have knowledge that each time he wants to use optimistic locking in his business code, he must explicitly call flush after each update call.
And this knowledge could be removed if we have possibility for intercepting flushStatements method in batch mode. Everything could work automatically.

Please, imagine we have two mybatis user applications. In first app is mybatis set to "batch" mode and in the second one to "simple" mode.
Every application has to contain different code to implement same feature of "optimistic locking" now.
IMHO mybatis should not force user applications to adapt its implementation according to mybatis mode. It should be the same in each app.

to backward compatibility:

"You might be aware, but my patch affects other methods than flushStatements() as well."

Ok, I can see that in your solution is BaseExecutor.wrapper newly as proxy instance but not as this instance as earlier. This could cause another incompatibility because each call on wrapper is newly called via proxy.
I went quickly through the code and there are some classes where wrapper instance occurs as executor attribute (all children of BaseStatementHandler, DefaultResultSetHandler, ResultLoader, ...).
I agree with you there is quite difficult to control the compatibility impact but not impossible. Or is there anything else what I miss?

I also agree that our solution adds new API and it is not as so clear as your solution.

But I still stand by what I've written earlier - mybatis should provide functionality for intercepting each public method of Executor, no matter if it is called from internal Executor code.
So I really wouldn't like to close this PR.

our suggestions:

a) Solve compatibility issue by adding configurable property to control new feature. For example some property allow_internal_proxy_call=true/false and default value would be false. This could solve compatibility issue.

b) I can go through the code and build some table of classes/methods which are affected by change of your commit (ExecutorBase.wrapper as proxy) and we can discuss incompatibility in more details?
We could build some compatibility list of affected classes/methods. I don't think that the real impact of this change could be too big.

Would it be acceptable for you? To continue this way?

Thank you

Okay, I'll keep it open for other devs to comment.
As they look busy lately, it might take some time.

that feature would help me: the possibility to intercept the flushStatements() in the batch mode (BatchResult(s)), makes perfectly sense in my event tracing library, that emits traces about infra calls for an application (regardless the batch / simple mode). My library traces events for org.hibernate calls (hibernate event-listeners/org.hibernate.Interceptor), but with mybatis it works only in the simple executor mode (I need the BatchResults). And my library cannot call flush() instead of an application.

thank you so much for your support

@kvasnickakb ,

I should have been clearer, but I was not going to use #1239 as-is. I was thinking about much smaller change which focuses on flushStatements (for now, at least). Please take a look at this branch: https://github.com/mybatis/mybatis-3/compare/master...harawata:flushstatements-interceptor?expand=1

Regarding backward compatibility, it is true that existing interceptors will be invoked more frequently and it should definitely be listed in the backward incompatible changes in the release note, however, for plugins which intercept flushStatements, the impact looks relatively small (in the extra invocations, invocation.proceed() just returns an empty list). Wouldn't you agree?

I can see the advantage of FlushResultHandler, but its functionality overlaps with Interceptor and providing both seems a little bit redundant and confusing, IMHO.

@emacarron Any thoughts on this?

+1

We need this support,we need to intercept this method in a logging scenario.

@kvasnickakb ,

I took another look at this PR and #1239 , but still don't feel too great about either solution.

The common solution for intercepting indirect/non-public/in-class method invocation is to use AspectJ, it seems.
I sent a PR that demonstrates the basic usage: kvasnickakb/flush-interceptor#1

There are several ways to use AspectJ and my PR uses LTW (load-time weaving).
You may need to specify javaagent VM option when running the app [1].

[1] I didn't dig deeper, but there seems to be some kind of workaround. See https://stackoverflow.com/q/22580352/1261766

Hi @harawata,
thank you for the comment.

I think that using AspectJ to intercept flushStatements method is more workaround than conceptual solution. Why should anyone add JVM bytecode weaver to his project when Mybatis internally offers its intercepting mechanism? In my opinion is flushStatements method good candidate to be added to other mybatis intercepted methods.

I think our solution has not too big impact and possible problems can be covered by junits. Or could be added it to major version mybatis 4? :)

MyBatis' internal interceptor mechanism uses dynamic proxy which has limitations, so I think it's perfectly reasonable to use AspectJ if you want/need to do something beyond the capability of dynamic proxy.

In any case, I will keep the usage/need in mind for future development.