Add support for iBatis 3 [SPR-5991] #10659
Comments
|
Gabriel Axel commented I implemented and attached a very simple factory bean that creates an SqlSession instance. Configuration example: <bean id="sqlSessionFactory" class="org.springframework.orm.SqlSessionFactoryBean" p:dataSource-ref="dataSource" p:configLocation="MapperConfig.xml"/> and in your DAO simply use an wired field:
|
|
Marino A. Jonsson commented The old SqlMapClient was thread safe, but the new SqlSession is not - so I'm thinking it might not be a good idea to reuse the same SqlSession instance :) |
|
AngerClown commented Here is a more thorough, but probably still incomplete implementation that also deals with transaction management. This is nowhere near production ready and it's barely tested, but it does work in the usual case. See the attached for all code and a simple HSQLDB test. Test code is in the com.example package; framework code in com.example.springibatis3. The SpringSqlSessionFactoryBuilder , based on the earlier attachment to this issue, creates DefaultSqlSessionFactory objects, using the standard Ibatis config file. It ignores any Environments set up in the config file, instead setting it null so that a ManagedTransactionFactory (no-op) will be used (see DefaultSqlSessionFactory). This builder class could certainly be enhanced so that the Ibatis config file is not needed, but that seems like overkill to me especially given the amount of code that would be needed to do this in a way that mimics Ibatis - see org.apache.ibatis.builder.xml.XMLConfigBuilder. There is a subclass of SqlSession, SpringSqlSession that carries references to the current DataSource and Connection but otherwise delegates to the usual DefaultSqlSession while wrapping the IbatisExceptions sql based exceptions in Spring's exception hierarchy. Transaction management is handled by Ibatis3TransactionManager, which subclasses DataSourceTransactionManager. At the appropriate places, it just makes sure the SqlSession is created and then cleans up after itself. Finally, there is an Ibatis3DaoSupport class that DAOs can subclass. Note that I made a design decision with this cod. Personally, I never like the SqlMapTemplate approach used with Ibatis 2. I know it's not much, if any, of a performance hit in practice, but the all the callbacks, along with opening and closing a SqlMapSession had a 'code smell' to me. In Ibatis3, the SqlSession is a more first class object, so I think it's even more important that new ones are only created as needed. For this reason, I didn't create a SqlSessionTemplate class. The assumption is that all access must be transactional. There is an execute() method in Ibatis3DaoSupport that will support a callback, but this was meant for one-off kinds of operations. I however, don't yet understand the underlying Spring tx code enough to figure out how or if this could be enforced, or how a DAO should know it's running transactionally or not. I will leave that up to the Spring designers. |
|
Putthibong Boonbong commented Hello, I implemented the full support of iBatis3 and updated most of javadoc. File attachment contains: Please revise source code and javadoc. It would be nice if you can release Spring 3.0 Final with iBatis3 support. Best regards, Putthibong Boonbong |
|
AngerClown commented The latest code submitted is much better than what I previously submitted. But, there are still a few issues that I think I have corrected. First, the easy stuff in SqlSessionFactoryBean: The major change was to add transaction synchronization support. This is in the SqlSessionSynchronization class (part of SqlSessionUtils). The idea here is to make sure that if synchronization is active (from DataSourceTransactionManager), the SqlSession cleans up after itself. In addition, only one SqlSession should be created per set of calls to the template (or transaction). SqlSessionUtils has get and close methods to support this. This is instead of creating a separate iBATIS transaction manager. In most cases, users will not need to deal with commit or rollback, but SqlSessionUtils has methods for this that make sure the underlying JDBC connection is dealt with. Here's how things should work: |
|
Matt Moriarity commented An important note that I think should be made is that when using this with Mapper interfaces without transactions, SqlSessions do not get closed properly. I was running into issues where I would get IO errors from the database because my connections weren't being closed. Adding Just note in case someone else runs into a similar issue. |
|
Matt Moriarity commented Ok nevermind, that doesn't fix anything. I'm still getting IO errors. I'm not sure if it's related to this or not. |
|
Matt Moriarity commented After some diagnosis, it seems that somewhere in the process of getting Connections, they are being closed before I actually use them. I'm using C3P0 for pooling and have it testing connections on checkout, which is succeeding. However, after that I get exceptions that my connection is already closed. Here is the full stack trace: |
|
AngerClown commented I ran into this issue too. The problem is that the SqlSession.getConnection() method returns a proxy if logging of java.sql is enabled. This causes the close call in DataSourceUtils to fail since connectionEquals always returns false. The solution was to add an unwrapConnection method in SqlSessionUtils. It's questionable if this is the right place - it works now, but a better method may be to have SqlSessionUtils.getSqlSession return a proxy SqlSession that handles this. Also, I don't know if this performs well since the unwrap method calls Proxy.isProxyClass then Proxy.getInvocationHandler (which calls isProxyClass again). It may be better if iBatis used a marker interface for logging Connections so we could just call instance of to see if we need to unwrap the Connection. This would require a bug fix in iBatis. |
|
Matt Moriarity commented I appreciate the effort AngerClown, but I tried your new code and i'm still running into the same issues. The messages saying close() has been called more than once went away but the root problem is still there. |
|
Putthibong Boonbong commented Could you please attach your code (implementation and test) that using c3p0? |
|
Matt Moriarity commented My code is available at http://github.com/mjm/triumph/ It's a GWT application using Spring to manage services. All my iBATIS operations happen through the new Mapper interfaces. |
|
Eduardo Macarron commented Hi, I think it would be great that Spring was able to inject iBATIS new Mapper Interfaces to bussiness beans. I have written a really simple class that provides that functionality (it works over Putthibong Boonbong's SqlSessionDaoSupport). It is just a FactoryBean that holds a JDK proxy that is able to redirect method calls to real iBATIS mappers. So you can simply define your beans using just interfaces, implementations are not needed anymore. <bean id="dao" class="org.springframework.orm.ibatis3.support.IBatisMapperFactoryBean"> where UserMapper is: public interface UserMapper { |
|
Eduardo Macarron commented After some tests with transactions I have also reached to the connection closed issue: DEBUG (SqlSessionUtils.java:55) - Fetching SqlSession from current transaction It fails when executing two transactions within the same Thread. It seemed that SqlSession was not properly cleaned in threadlocal. So I debugged step by step the transaction closing code to see what was happening. In SqlSessionUtils.afterCompletion() the code reads this: changed it to And it seems to work fine now. I hope you don't mind if I upload again the test code with the fix (please remove my two previous uploads) |
|
Eduardo Macarron commented It seems that holder reference count is always two. I suppose that SqlSession getSqlSession has a bug because it calls twice to hoder.requested() The problem seems to be related to Mappers. getMapper calls SqlSessionUtils.getSqlSession that adds one to reference count but mappers dont call to SqlSessionUtils.closeSqlSession that removes it. So transaction closes propertly the session but counter is not set to zero. |
|
Eduardo Macarron commented The only solution I can imagine is to wrap Mapper calls. The JDK proxy I submitted helps in that purpose: This code, fixes the problem with the holder counter and the code runs fine keeping the original code if (!holder.isOpen()) public final Object invoke(Object proxy, Method method, Object[] args) throws Throwable { } |
|
Eduardo Macarron commented Please have a look to the MapperFactoryBean.tar.gz I have uploaded. It seems to work fine with Mappers (with and without transaction context). I have made two changes to Anger's code, but please have a look at them because I may be wrong. In SQlSessionUtils getSqlSession, I have removed one "holder.requested();" And have added the same line (holder.requested();) at the end, when a new holder is created. |
|
AngerClown commented I think the call in SqlSessionSynchronization.afterCompletion() should still be if(!holder.isOpen()). The sqlSession should only be closed if there are no other references to it. Changing to just if(holder.isOpen()) would close the session every time even if it is still being used by another TX / thread, which is not correct. The duplicate call to holder.requested() is certainly a bug though. Not sure how I missed that. The idea of the MapperFactoryBean as a proxy could be useful, but I think it needs to be expanded to deal with transactions. The way Eduardo has it coded now explicitly closes the session. I have an updated class in the attached that calls SqlSessionTemplate.getMapper directly, but haven't tested it. Eduardo's exception handling in the MapperFactory also inspired me to update the exception handling in SqlSessionTemplate (see the wrapException method). I think the attached code also fixes Matt's issues. The problem was that the getMapper method on SqlSessionTemplate needs to return a mapper proxy that is TX aware. I have updated that method to return a different proxy if there is an existing transaction. |
|
Eduardo Macarron commented Sorry Anger, I just was trying to point out that the problem was related to that line (!holder.isOpen()) but of course the solution is not changing it to the opposite. I think I have been more clear in my following post. The code you submitted still fails with the connection close error, but I think its just because the duplicated holder.requested() is still there. I think mappers should not have any transactional special dealing because all transactional stuff is being handled inside SqlSessionUtils. And I am afraid this code is the root of the connection closed problem: because the bean that uses the mapper should call SqlSession.close() and it cant, and if it could it should not because it did not open the SqlSession. In fact this is exactly the problem with Matts code. He uses the mapper but does not close sessions so the Holder counter is always in wrong state so the SqlSession survives and so the closed connection inside it. Anyway Matt's should be almost the right use. I mean that mappers let us to provide an implementation that releases the application to have direct calls to ibatis api. Have a look at context.xml, the "daos" are declared this way: In the case of Matts code, he has a TagDaoIbatisImpl, TagDao and TagMapper. He should inject directly the TagMapper and get rid of the other objects. Regarding the new code, MapperFactoryBean could be even simpler because it does not need to create a proxy it can use directly the one created in SqlSessionTemplete but just for performance maybe it good be good to get rid from the creation of a new dynamic proxy when mapper is called from MapperFactoryBean, it would be better to hold the proxy on MapperFactoryBean (it should be one MapperFactoryBean for each mapper) and reuse it. I have done some little changes. It works with and without transactions in my simple test. It would be great if Matt tries it in his project.
Have a look at MapperFactoryBean_20091229.tar.gz |
|
Eduardo Macarron commented I have tried to know if there is any performance impact for creating that proxy in each call and.. if there is any it is almost zero. So I think your code is cleaner and is better to remove the proxy from MapperFactoryBean and have it inside SqlSessionTemplate. An also, we dont have that awful invokeMapper public method on SqlSessionTemplate. So let me upload a new code again. This one with a really trivial MapperFactory, holder.requested() fix and transaction handling inside SqlUtils. |
|
Ivan Hui commented Dear Eduardo, com.ying.yppc.orm.ibatis3.IbatisSystemException: SqlSession operation; nested exception is java.lang.IllegalArgumentException: Mapped Statements collection does not contain value for java.lang.Object.toString it works fine if i discard MapperFactoryBean,enclosed article is for your reference: thanks a lot more for your help. |
|
Eduardo Macarron commented Ivan Xu, I would say that you are trying to execute a toString over a mapper... "Mapped Statements collection does not contain value for java.lang.Object.toString" If this is que case, the IllegalArgumentException is right. |
|
Ivan Hui commented sorry to my stupid mistake,Spring version 2.5.6 cause the problem,I upgrade my Spring to version 3.0 and the issue fixed already,thanks everyone. |
|
Ivan Hui commented Dear Eduardo,I do nothing,you can try MapperFactoryBean_20091229-2.tar.gz with Spring 2.5.6,then you can get the issue. |
|
Eduardo Macarron commented Ivan Xu, it works for me with 2.5.6. All you have to do is replacing FactoryBean<?> with FactoryBean because in 2.5.6 FactoryBean is not generic. But the problem you posted is related to the use of toString over a Mapper. If I code this I get exactly the same error: |
|
Eduardo Macarron commented Have a look at your applicationContext.xml, you have an aop interceptor that calls com.ying.yppc.logging.Logging.after(), it seems there is the problem (maybe some bean is receving a mapper as an argument, and it is being logged automatically. |
|
Ivan Hui commented Dear Eduardo,yes it's all that,it works fine after I remove the interceptor,thank you very much. |
|
Matt Moriarity commented Eduardo, I'm currently modifying my project to use the MapperFactoryBeans to get my mappers. I will comment when I'm done to see if there was success inside of transactions. |
|
Matt Moriarity commented Things are working now. I assume this is due to the other fixes included in Eduardo's code, not just the use of MapperFactoryBean, seeing as it is quite basic in the most recent code. Regardless, I'm no longer having issues with closed connections. Thank you all. |
|
Eduardo Macarron commented Good to hear that! Anger what do you think about this?. getMapper method could be even more simpler and cleaner this way The problem is that MapperMethod.execute declares SQLException and I think this is an error in iBatis code. If the finally fix it, this code wont compile. |
|
Robert Novotny commented There is a slight problem when programmatically configuring if (!usesManagedTransactions(sessionFactory.getConfiguration())
&& DataSourceUtils.isConnectionTransactional(unwrapConnection(holder.getSqlSession()), dataSource)) {This occurs when programmatically configuring the I suppose that instead of NPE, it should throw the "Pre-bound JDBC Connection found!" exception. |
|
AngerClown commented It's been a while since I have looked at the code, but I think this will fix Robert's issue. Replace the the original if check with this: Looking at the code again, the holder will always be null at this point so that was a bug. What I think we really care about is if there is a current Connection active. (The nested if's are there to avoid Assert errors on nulls.) This should allow SqlSessions not managed by Spring to pass through as long as there is no other active Connection. There could be an active Spring transaction that's not using a Connection or a suspended Spring tx - should be output a warning message in those cases? |
|
AngerClown commented Oops, hit submit right as I noticed an error. The innermost if in my last comment should be |
|
AngerClown commented For anyone trying to use Mybatis instead of iBatis, note that the current code will not work properly. The code attached will work for Mybatis. It should also work for iBatis GA (not any of the betas) except that the MybatisSystemException and SqlSessionTemplate classes will need to reference IbatisException instead of PersistenceException (IbatisException is deprecated in Mybatis). The necessary code is commented out in the attched classes. Please test this code out and let me know if there are any issues. I have made some major changes to SqlSessionUtils but not any significant API changes. This should be a drop in replacement. Also note that the org.springframework.orm.ibatis3.test package is not needed for running the code and introduces some other dependencies. If you don't care about the internals or test cases, there's no need to include this. h5.Reasons for the changes: h5.What Changed: h5.Other Changes:
SqlSessionUtils
SqlSessionDaoSupport
SqlSessionTemplate
SqlSessionHolder
h5.Tests: |
|
Eduardo Macarron commented Hi Anger. I've run my simple tests and everything works fine. |
|
Putthibong Boonbong commented Revise from AngerClown's code on 06/Jun/10.
Furthermore, mybatis version 3.0.x remain using package name 'org.apache.ibatis' and Clinton Begin will rename package to others in version 4.0.x. Renaming spring package to 'org.springframework.orm.mybatis' should postpone until version 4.0.x was released. |
|
Eduardo Macarron commented Hi Putthibong I think we should keep the possibility to build mappers the way it was done before, just injecting a SqlSessionFactory in a MapperFactoryBean. This way: But now if a MapperFactoryBean needs a SqlSessionTemplate instead of a SqlSessionFactory the configuration will be bigger (create a SqlSessionFactory + create a SqlSessionTemplate + create Mappers) but I think we won´t get any advantage and would prefer the easier way. Maybe a MapperFactoryBean could get a SqlSessionFactory instead of a SqlSessionTemplate or maybe both. Just adding this method. I think this change is needed. And if we also want to be able to set a datasource then what we get is that we have SqlSessionDaoSupport code fully replicated on MapperFactoryBean and in that case maybe extending is not so bad. At least something that "makes" DAOs is going to have similar config option that a DAO. |
|
Putthibong Boonbong commented Sorry to remove checkDaoConfig, this method should declare here in SqlSessionDaoSupport to prevent subclass from this declaration. And what about this MapperFactoryBean? |
|
Eduardo Macarron commented Hi Putthibong. That version looks perfect to me :) |
|
Eduardo Macarron commented Just a note. Now that MapperFactoryBean does not depend on SqlSessionDaoSupport it should be moved to the parent package because it is not supposed to be used from application beans. |
|
arpana gupta commented Hi, Is there any final spring-orm-ibatis3 jar file available, that can be used directly? Thanks, |
|
Eduardo Macarron commented Hi Arpana. I am sure you wont have any problem in building a working jar by your own. Just take org.springframework.orm.ibatis3-201006100756.tar.gz + spring 3.0.3 + commons logging + mybatis-3.0.1 and compile it. |
|
Jonathan Komorek commented For what it is worth, I have added the following method to the MapperFactoryBean in my own project By referencing this as an init-method within my bean definition it removes the need to maintain a separate list of mappers in the ibatis-config.xml. I think this is pretty handy and thought I would share. |
|
Jeremy Pyman commented I attempted to build the jar file according to Eduardo's instructions and ran into some problems. The issue I was experiencing seems to be identical to the one reported at https://issues.apache.org/jira/browse/IBATIS-625 even though it is listed as resolved. I am using spring 3.0.3-RELEASE and MyBatis-3.0.1. I got around the problem by renaming the mapper interface from org.springframework.orm.ibatis3.TestDao to org.springframework.orm.ibatis3.TestDaoMapper and updating the mapperInterface value in application-context-test-MapperFactoryBean.xml |
|
Bing Lu commented warnings when compiling 610 source IbatisException is deprecated and the import of org.springframework.util.ClassUtils is never used in SqlSessionTemplate.java, just to let you know |
|
Eduardo Macarron commented Jonathan Komorek, good idea to let mappers register by their own. Maybe using just another property for it would be cleaner (easier to configure). Something like addToConfig should be optional and default to false. |
|
Jonathan Komorek commented Eduardo, I like the idea of the property. That's certainly a cleaner implementation. That being said, I think it might be better to default to true. I suspect that most developers will opt for this approach as opposed to the configuration file, which would mean that every Mapper bean would be required to define this property - a bit of unnecessary clutter. In addition, the ``` |
|
Eduardo Macarron commented Yep Jonathan, you are completely right. BTW does anybody know why this issue has been unscheduled? |
|
Juergen Hoeller commented Since iBATIS 3.x / MyBatis is a moving target, it looks like we won't support it in Spring Framework 3.x proper. It's likely that we'll rather cover it within our emerging Spring Data sister project - some time later this year. Juergen |
|
Eduardo Macarron commented I am sorry to hear that Juergen. I hope that Spring Data comes soon :) Let me upload some little changes I have on my workspace.
You can have a look at a sample config on /orm-ibatis3/src/test/java/sample/context.xml Just make a maven install to build it and run tests Maybe building a MyBatis namespace could made config less verbose. |
|
Eduardo Macarron commented Sorry my previous submission was wrong, I hit the submit button too fast :) |
|
Thomas Risberg commented The support for MyBatis 3 will be provided as part of the Spring Data project in the datastore-sql component. See DATASQL-1. |
|
Putthibong Boonbong commented mybatis-spring-1.0.0-RC1 has released, please try and report bug to mybatis issue tracking. Thank you. |
|
Chris Beams commented See resolution comments at DATAJDBC-2 for information on the MyBatis-Spring project and why integration need not occur in the core Spring Framework project nor the Spring Data family of projects. |
spring-projects-issues
added
status: declined
Gabriel Axel opened SPR-5991 and commented
iBatis 3 beta 1 introduced API changes which break the existing Spring support.
Affects: 3.0 M3
Attachments:
Issue Links:
50 votes, 55 watchers
The text was updated successfully, but these errors were encountered: