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

Make Session.transaction inline #57

Merged
merged 1 commit into from Aug 18, 2022
Merged

Make Session.transaction inline #57

merged 1 commit into from Aug 18, 2022

Conversation

augustl
Copy link
Contributor

@augustl augustl commented Aug 17, 2022

By marking it as an inline function, it becomes possible to use transaction from a coroutine context, without having to any additional sit-ups to make it work.

For example, this code does not work now, but works fine with this patch applied:

withTimeout(1000) {
    async {
        sessionOf(dataSource).use { dbSess ->
            dbSess.transaction { txSess ->
                delay(1000)
                // Without inline, fails with: 
                //     Suspension functions can be called only within coroutine body
                // Works fine when `transaction` is inline
            }
        }
    }
}

As far as I can tell, marking transaction as inline has no negative side effects for the current uses, and should be fully backwards compatible :)

@seratch
Copy link
Owner

seratch commented Aug 17, 2022

This should be fine but can you add some unit tests that demonstrate the use case for preventing future regressions?

@augustl
Copy link
Contributor Author

augustl commented Aug 17, 2022

@seratch good point! I just updated the PR with a test verifying it instead of a small blog post explaining it in the commit message :)

By marking it as an inline function, it becomes possible to use coroutine scoped functions inside the `transaction` handler.
@ievgen-kapinos
Copy link
Contributor

Hi @augustl
On my project I've ended up with the same idea. Implemented it locally and it works well!
Thank you for contributing this to the public.

@augustl
Copy link
Contributor Author

augustl commented Aug 17, 2022

@ievgen-kapinos I've done that too, actually! :) However, I'm writing a book for Apress about Kotlin, and it's using Kotliquery. So a library native solution to the problem is definitely preferred!

@seratch
Copy link
Owner

seratch commented Aug 18, 2022

Wow, it's so exciting to hear that you are writing a Kotlin book and using this library in the code examples! Cannot wait to read the book!

Copy link
Owner

@seratch seratch left a comment

Choose a reason for hiding this comment

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

LGTM

@seratch seratch merged commit 9cca285 into seratch:master Aug 18, 2022
@seratch
Copy link
Owner

seratch commented Aug 18, 2022

I just released v1.9.0 to the Sonatype OSS repos. Those jar files will be available on Maven Central soon 🚀

@augustl
Copy link
Contributor Author

augustl commented Aug 18, 2022

Thanks for the merge!

Wow, it's so exciting to hear that you are writing a Kotlin book and using this library in the code examples! Cannot wait to read the book!

Thanks, so am I! :D The book is titled Pro Kotlin Web Apps from Scratch, on Apress, where I teach how to write web apps without a framework. Kotliquery is the number 1 choice for that sort of setup imo, and I've used it in large real-world projects.

@augustl
Copy link
Contributor Author

augustl commented Jul 20, 2023

@seratch
Copy link
Owner

seratch commented Jul 21, 2023

@augustl Thank you for sharing this and congratulations on publishing the book!

I've just bought the Kindle version and read Chapter 6. I really appreciate the way you explain the value of a simple SQL database library, like Kotliquery, for web apps. Although I haven't gone through the other chapters in detail yet, I'm thoroughly enjoying the book so far. Thanks again for including this library in your book!

@augustl
Copy link
Contributor Author

augustl commented Jul 28, 2023

You're welcome! And thanks for making an awesome library :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants