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

Add DSLContext.transactionCoroutine overload accepting CoroutineContext #15103

Closed
trietsch opened this issue May 23, 2023 · 10 comments
Closed

Add DSLContext.transactionCoroutine overload accepting CoroutineContext #15103

trietsch opened this issue May 23, 2023 · 10 comments

Comments

@trietsch
Copy link

Use case

The Kotlin Coroutines extension functions for jOOQ don't allow to set a coroutine context. This is necessary if the context contains information needed in the transaction. Currently, the mono function from Reactor creates a new, empty coroutine context.

Possible solution

Already made a PR, it is quite trivial.

Possible workarounds

Rewrite the function ourselves, like we've currently done. But of course it's better if it's in the official library.

jOOQ Version

jOOQ Open Source 3.18.4

Database product and version

PostgresQL 12

Java Version

Temurin 18

OS Version

No response

JDBC driver name and version (include name if unofficial driver)

No response

@lukaseder
Copy link
Member

Thanks for your feature request and implementation suggestion. Can you:

  • Show an example of where this would be useful in your client code?
  • Explain why a seemingly backwards incompatible change is OK (i.e. is it really backwards incompatible, or will the change be source compatible in kotlin)?

@trietsch
Copy link
Author

trietsch commented May 24, 2023

@lukaseder thanks for your reply! An example how we would use this, is to pass on a CoroutineContext from a parent. Here you can see how we're using it with our workaround.

val context = checkNotNull(coroutineContext[ZedTokenContext]) {
    "No ZedTokenContext found in coroutine context - should be set by the ZedTokenRequestServerInterceptor"
}
jooq.transactionCoroutine(context) {
    // do DB work
}

This allows to pass on relevant context from a parent coroutine context, which I extract first.

This is not a backwards incompatible change, due to the fact that a default value has been provided in the suggested code change, that is identical to the default value that mono uses itself, namely EmptyCoroutineContext. The code therefore offers two functions, one without an argument, and one with an argument. Hence, the current way is still possible after this change jooq.transactionCoroutine { // do DB work }

@lukaseder
Copy link
Member

Can you please elaborate with a real world example of what this is useful for?

It's certainly binary incompatible, not sure if there are source incompatible implications. I agree that for some calls it might not matter, but I would really like to avoid incompatibilities if it's possible.

@trietsch
Copy link
Author

All I can provide in terms of an example is our use case. In an RPC call (we're using gRPC with Kotlin), a token that is provided by an end-user of our APIs is added to the coroutine context. This call eventually ends up in a DAO, where we use a transaction to ensure that the database actions are in the same transaction as the interaction with an external permissions system, that needs to be modified as well, after the database actions are done. Hence, both the database actions and the interaction with the external permissions system are in the same transaction.
For the interaction with the external permissions system, we need to use the token that was set in the coroutine context. Therefore, we need to be able to access that value from the coroutine context within the transaction, as it is used there.

AFAIK it is source compatible, as it doesn't break existing usages of the current transactionCoroutine extension function.

By the way, great that you're thinking about compatibility and usages :)

@lukaseder
Copy link
Member

Thanks for the feedback. I'm still having a hard time understanding the background of this context, from prose descriptions. I've already found it hard to understand reactor's context object (which is in violation with the reactive streams SPI and generally a pain from an interoperability perspective. I really hope Loom cleans this all up very soon!). What I was looking for was smoe examples in code that show clearly how this context is a superior approach to any alternative (including just capturing scope at the call site). For example, an MCVE would be helpful: https://github.com/jOOQ/jOOQ-mcve.

AFAIK it is source compatible, as it doesn't break existing usages of the current transactionCoroutine extension function.

You probably jumped to conclusions a bit quickly here. Adding a parameter to a method seems quite the change, especially if it's the first parameter. I've documented a backwards incompatibility in your PR, which I've rejected for this reason: #15102 (comment)

In that comment, I've stated that I prefer not to take hasty action here. The extension function is just a few lines of code, so it can easily be copied to user code. I'm not closing the issue, would just like to collect more user feedback and better understand the problem first.

@trietsch
Copy link
Author

Alright, seems like a good conclusion for now. I won't make a MCVE for now, as that would just be setting a coroutine context with our specific coroutine context element, and then showing that we're going to access that in the transaction.
A real world example would include a gRPC app, that sets the context value in an interceptor, and then showing that that should still be accessible in the jooq transaction.

@lukaseder
Copy link
Member

I've given this some additional thought. There's probably no issue with the overload approach, specifically if there's no default value for the context parameter. Will do this for 3.19

@lukaseder lukaseder changed the title jOOQ Kotlin Coroutines do not allow to provide a coroutine context Add DSLContext.transactionCoroutine overload accepting CoroutineContext May 25, 2023
3.14 Better Kotlin and Scala Support automation moved this from To do to Done May 25, 2023
@trietsch
Copy link
Author

Great, thanks for your help!

@trietsch
Copy link
Author

trietsch commented May 26, 2023 via email

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

No branches or pull requests

2 participants