search

supercargo / guice-persist-jooq

Guice-persist extension for using jOOQ based persistence layer
Apache License 2.0
22 stars 10 forks source link

End unit of work after rollback #7

Closed mapoulin closed 7 years ago

mapoulin commented 7 years ago

Hi,

We noticed that the connections were not released when the interceptor triggered a rollback.

More specifically: The auto-commit flag was never set back to true and the guard in the finally block prevented the release of the connection.

I changed the way the dependencies were injected to make it easier to test. I followed the method described here: https://github.com/google/guice/wiki/AOP#Injecting_Interceptors

I added some tests. The last one verifies that unitOfWork.end() is called when a rollback happens.

dominiquejb commented 7 years ago

+1 for the tests

supercargo commented 7 years ago

The tests are much appreciated and it is great to get some coverage around expected transactional behavior. As far as the auto-commit bug goes, the change makes sense. I'm wondering though if it would be clearer to wrap the whole bottom part of that method in try/finally guard around turning auto-commit back on.

mapoulin commented 7 years ago

I agree with you. If you are willing to deviate a bit from the structure created by the author of GuicePersist, I propose to end the unit of work in that same finally and have it at one place instead of two.

supercargo commented 7 years ago

Okay, I've merged this as-is to get the tests in place and to have a fixed baseline for the re-factor we are discussing. I'd rather have any further changes in a new PR. Thanks for contributing!

mapoulin commented 7 years ago

@supercargo Perfect. Would it be possible to have this fix released to central? We would update our dependency. Thanks!

supercargo commented 7 years ago

Sure, version 0.2.0 is now available in central