Avoiding internal calls causes interceptor methods invalid. #1239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

abel533 wants to merge 1 commit into mybatis:master

from abel533:interceptor

Open

Avoiding internal calls causes interceptor methods invalid. #1239

abel533 wants to merge 1 commit into mybatis:master from abel533:interceptor

Conversation

@abel533

Copy link

Contributor

@abel533 abel533 commented Apr 1, 2018 •

edited

Loading

There are a few internally invoked methods in the executor implementation that the interceptor cannot intercept.

Especially the following method:

<E> List<E> query(MappedStatement ms, Object parameterObject, 
 RowBounds rowBounds, ResultHandler resultHandler, 
 CacheKey key, BoundSql boundSql) throws SQLException

The parameters of this method are the most complete, and it is most appropriate to intercept this method.

Binding the intercepted object can be accomplished through the interface provided by Executor.

void setExecutorWrapper(Executor executor)

Call this method in the Configuration class.

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

@abel533


 Avoiding internal calls causes interceptor methods invalid.

618f762

@harawata

Copy link

Member

harawata commented Apr 5, 2018

Thank you, @abel533 .

I'll try to look into it.
It might take some time, though (so many things to do 🤯).

By the way, you wrote 'cannot intercept', but what happens actually?
If there is any error message or stack trace, please add that information.

Cc-ing @emacarron

@harawata

Copy link

Member

harawata commented Apr 19, 2018

Um...I'm sorry. I am not familiar with plugins and couldn't understand the problem.
Could you create an executable example including configurations and failing assertions, please?

@abel533

Copy link

Contributor Author

abel533 commented Apr 21, 2018

Don't be misled by specific plugins.

After all, how do I intercept the following method?

<E> List<E> query(MappedStatement ms, Object parameterObject, 
 RowBounds rowBounds, ResultHandler resultHandler, 
 CacheKey key, BoundSql boundSql) throws SQLException

@harawata harawata added the waiting for feedback label

Nov 6, 2018

@abel533

Copy link

Contributor Author

abel533 commented Nov 22, 2018 •

edited

Loading

@harawata Please resolve this PR before the 3.5.0 release.

If you accept this PR, the interceptor will be more convenient to handle.

@harawata

Copy link

Member

harawata commented Nov 22, 2018

Hi @abel533 ,

If you want me to proceed, please provide a small project including a simple example plugin.
Here are some project templates if it helps.

Preferably, I would look for an alternative approach as the change in this PR looks strange. i.e. there seems to be no legitimate reason to call wrapper's methods instead of its own.
In any case, I need the example to understand the problem first.

@abel533

Copy link

Contributor Author

abel533 commented Nov 23, 2018

@harawata I am not familiar with gradle, and I have the following error when creating a project:

Caused by: groovy.lang.MissingPropertyException: No such property: mybatis for class: SimpleTemplateScript4
 at SimpleTemplateScript4.run(SimpleTemplateScript4.groovy:1)
 at org.gradle.api.internal.file.copy.FilterChain3ドル.transform(FilterChain.java:121)
 at org.gradle.api.internal.file.copy.FilterChain3ドル.transform(FilterChain.java:110)
 at org.gradle.api.internal.ChainingTransformer.transform(ChainingTransformer.java:37)
 at org.gradle.api.internal.file.copy.FilterChain.transform(FilterChain.java:56)
 at org.gradle.api.internal.file.copy.FilterChain.transform(FilterChain.java:64)
 at org.gradle.api.internal.file.copy.DefaultFileCopyDetails.open(DefaultFileCopyDetails.java:93)
 at org.gradle.api.internal.file.AbstractFileTreeElement.copyTo(AbstractFileTreeElement.java:56)
 at org.gradle.api.internal.file.copy.DefaultFileCopyDetails.copyTo(DefaultFileCopyDetails.java:101)
 at org.gradle.api.internal.file.AbstractFileTreeElement.copyFile(AbstractFileTreeElement.java:93)
 at org.gradle.api.internal.file.AbstractFileTreeElement.copyTo(AbstractFileTreeElement.java:74)
 ... 156 more

I put the example directly here, and the comment says the reason.

import org.apache.ibatis.cache.CacheKey;
import org.apache.ibatis.executor.Executor;
import org.apache.ibatis.mapping.BoundSql;
import org.apache.ibatis.mapping.MappedStatement;
import org.apache.ibatis.plugin.*;
import org.apache.ibatis.session.ResultHandler;
import org.apache.ibatis.session.RowBounds;
import java.util.Properties;
@Intercepts(
 @Signature(
 type = Executor.class,
 method = "query",
 //Cannot intercept the query method of 6 parameters now
 //This PR can solve this problem
 args = {MappedStatement.class, Object.class, RowBounds.class, ResultHandler.class, CacheKey.class, BoundSql.class}
 //The current version can only intercept the following method
 //args = {MappedStatement.class, Object.class, RowBounds.class, ResultHandler.class}
 )
)
public class QueryInterceptor implements Interceptor {
 @Override
 public Object intercept(Invocation invocation) throws Throwable {
 Object[] args = invocation.getArgs();
 //It is often necessary to use BoundSql in the interceptor, which is very inconvenient now.
 BoundSql boundSql = (BoundSql) args[5];
 System.out.println("SQL: " + boundSql.getSql());
 return invocation.proceed();
 }
 @Override
 public Object plugin(Object target) {
 return Plugin.wrap(target, this);
 }
 @Override
 public void setProperties(Properties properties) {
 }
}

I think it is very important to be able to intercept the following interface below.

public interface Executor {
 //others
 <E> List<E> query(MappedStatement ms, Object parameter, RowBounds rowBounds, ResultHandler resultHandler) throws SQLException;
 //others
 
}

@kazuki43zoo

Copy link

Member

kazuki43zoo commented Nov 23, 2018

@abel533 @harawata

I am not familiar with gradle, and I have the following error when creating a project:

Sorry, I've fixed build error. Please try again.
Thanks.

abel533 added a commit to abel533/mybatis-issues that referenced this pull request

Nov 23, 2018

@abel533


 Demo for mybatis/mybatis-3#1239

fd9f082

@abel533

Copy link

Contributor Author

abel533 commented Nov 23, 2018 •

edited

Loading

@harawata I wrote an example. After merge this PR. This interceptor will work.
zip: gh-1239.zip
github: https://github.com/abel533/mybatis-issues/tree/master/gh-1239

@harawata

Copy link

Member

harawata commented Nov 24, 2018

Thank you, @abel533 !
I'll look into it.

@harawata

Copy link

Member

harawata commented Nov 28, 2018

I now understand what you want, but as you wrote, the six-arg-version of query method is an internally invoked method and is not intended to be intercepted, I think.

@abel533

Copy link

Contributor Author

abel533 commented Nov 28, 2018

If it is for internal calls, why design it on the interface. I think this is a design mistake.

Rowbounds is a very useful parameter, and it is necessary to intercept 6 parameters.

@harawata

Copy link

Member

harawata commented Nov 28, 2018

I think this is a design mistake.

I don't know, maybe (as I wrote, I am not familiar with plugin design).
As overwriting private fields might not work in a future version of Java, I myself am not so keen on such enhancement anyway, but someone else might.