mybatis / mybatis-3

MyBatis SQL mapper framework for Java
http://mybatis.github.io/mybatis-3/
Apache License 2.0
19.72k stars 12.82k forks source link

feat: allow `DefaultSqlSessionFactory` to provide a custom `SqlSession` #3128

Closed epochcoder closed 4 months ago

epochcoder commented 6 months ago

Hi Maintainers!

While extending SqlSessionFactoryBuilder allows users to fully customise their implementations of SqlSessionFactory and SqlSession, the drawback is that you cannot use the safe defaults MyBatis provides anymore, which leaves two options:

I added a minimal test case which shows a structure that would allow the use of the defaults while providing important extension points.

Below is parked for future discussion:

Additionally, allow subclasses of DefaultSqlSession to call selectList(...), which allows providing a system wide result handler if user code did not specify one.

I understand that this is definitely not a common use case, but believe the changes required are so minimal that it would provide a good tradeoff.

PS: note that the result handler is not the only reason here, we do quite some custom work (in our codebase) in the factory and session respectively.

coveralls commented 6 months ago

Coverage Status

coverage: 87.17% (+0.001%) from 87.169% when pulling 67c6babee6d38522191b4b2bf6b051d4f150046c on epochcoder:feat/custom-sql-session-using-defaults into 043270b83a01162d50a05ae1656ee57f57336a88 on mybatis:master.

epochcoder commented 4 months ago

Hi @harawata, do you have some feedback on this one?

harawata commented 4 months ago

DefaultSqlSessionFactory looks OK.

I'm not sure about extending DefaultSqlSession, though. This particular change looks harmless and may be sufficient for your needs, but it could trigger future trickier requests.

If you revert the DefaultSqlSession, I will merge the DefaultSqlSessionFactory change. If you cannot give up the DefaultSqlSession change, I would like other devs' opinion.

epochcoder commented 4 months ago

@harawata I'm fine with reverting the change to DefaultSqlSession for now, we can discuss it at a later point. Ill update the PR with the changes

epochcoder commented 4 months ago

Please feel free to do it @harawata , if not, I'll do this by end of day GMT+2 tommorow 😊

harawata commented 4 months ago

Thank you, @epochcoder !