Skip to content
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

feat: allow DefaultSqlSessionFactory to provide a custom SqlSession #3128

Merged

Conversation

epochcoder
Copy link
Contributor

@epochcoder epochcoder commented Apr 3, 2024

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:

  • Duplicate (copy/paste) DefaultSqlSession and DefaultSqlSessionFactory, the drawbacks here are that you do not get upstream updates anymore and have to compare and update these files every release.
  • Use reflection, the drawbacks here are that this is quite dirty and suffers from the same problems mentioned above.

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
Copy link

coveralls commented Apr 3, 2024

Coverage Status

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

@epochcoder
Copy link
Contributor Author

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

@harawata
Copy link
Member

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
Copy link
Contributor Author

@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 epochcoder force-pushed the feat/custom-sql-session-using-defaults branch from c49bfc5 to dddd9f7 Compare May 24, 2024 08:51
Copy link
Member

@harawata harawata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update, @epochcoder !

Could you fix the two minor issues and force-push?
If you are busy, please let me know and I'll do it.

@epochcoder
Copy link
Contributor Author

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

@harawata harawata merged commit 94e185b into mybatis:master May 28, 2024
19 checks passed
@harawata harawata added the enhancement Improve a feature or add a new feature label May 28, 2024
@harawata harawata self-assigned this May 28, 2024
@harawata harawata added this to the 3.5.17 milestone May 28, 2024
@harawata
Copy link
Member

Thank you, @epochcoder !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve a feature or add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants