Add TransactionListener interface

[aboisvert]

Add TransactionListener interface to support notification and conditional action based on {before, after} {commit, rollback} events.

Use-Case

My present need is to dispatch events conditional on the commit of the current transaction. Using a post-commit hook allows for sending a message (e.g. on a AWS SQS queue) only after the transaction commits. This prevents race conditions where the message is picked up for processing before changes have been effected in the database and are visible to other parties. (Concretely, sending a "User Created" event only after the user record has been persisted.)

Of course this is possible without post-commit hooks, however hooks provide a modular abstraction that decouples the code (e.g. send message) from the specific commit point. Transactional event listeners are a common pattern, and can be seen in other libraries such as the Spring Framework (see https://docs.spring.io/spring-framework/reference/data-access/transaction/event.html)

Other Use-Cases

Hooks provide modularity to support decoupling in several other cases.

Before-Commit Hook

• Perform final validation checks before committing changes
• Update aggregate or summary tables based on transactional changes
• Trigger notifications or events based on the impending commit

After Commit Hook

• Clean up temporary resources used during the transaction
• Update caches or search indexes with newly committed data
• Trigger asynchronous processes based on successful commits (already elaborated above)

Before Rollback Hook

• Log detailed information about the state leading to rollback
• Notify relevant components/systems about the impending rollback
• Eagerly release resources used during the transaction

After Rollback Hook

• Reset application state or clear caches affected by the rolled-back transaction
• Log or report on the reasons for rollback for analysis
• Trigger compensating actions or notifications due to the failed transaction

[aboisvert]

I've removed the rollBack0 argument since it was only used to handle the difference between autoCommit true/false behavior.

With the new interface, client code can now hook into the rollback process if needed.

[aboisvert]

I've considered making listeners thread-safe, but the safepoints member isn't so it seems more consistent this way. Client code that requires thread-safety bears the responsibility of synchronizing around mutations to listeners (and safepoints, as before)

[lihaoyi]

@aboisvert could you write a bit about your use case in the PR description? that will help contextualize what this feature is meant for

[aboisvert]

@lihaoyi I updated the PR description with more detail on the use-case(s).

I also put the PR in draft state since I intend to add/change a few things. I will notify you when this is ready to review + merge.

[aboisvert]

This is now ready for review and feedback.

I'm still doing some integration in our app to see if it fulfills all our needs, along with some real-world testing. No rush on merging. I wanted to get it out earlier for feedback.

[lihaoyi]

Perhaps the biggest question for me is whether this mutable addTransactionListener API is what we want, or whether we should encourage people to pass in fixed Seq[TransactionListener] into DbClient's constructors, or alternatively having them subclass DbClient and DbApi to inject the logic they need before delegating to super (or providing our own helpers to do so).

Have you considered other APIs to inject these hooks into the transaction lifecycle? If so, why did you pick this one?

[aboisvert]

Perhaps the biggest question for me is whether this mutable addTransactionListener API is what we want, or whether we should encourage people to pass in fixed Seq[TransactionListener] into DbClient's constructors, or alternatively having them subclass DbClient and DbApi to inject the logic they need before delegating to super (or providing our own helpers to do so).

Mutation isn't great but I think it's the most user-friendly approach given the current API design.

Have you considered other APIs to inject these hooks into the transaction lifecycle? If so, why did you pick this one?

I think there's a spectrum of possible solutions, in broad strokes:

1. Do nothing / status-quo - This boils down to asking users to wrap the currently transaction API and do their thing.

2. Minimal support variations

a) Only allow passing in one or more listeners at DbClient/Connection-creation time . For users, this would avoid having to wrap the API and still provide basic functionality. The trade-off is that it's not very flexible / user-friendly. I believe it's pretty common that people want to add post-commit work, such as dispatching events/notifications after the transaction commits, and most often it is dependent on what happens during the transaction. If users can't add post-commit work dynamically, they'll end up re-creating the proposed API on their own.

b) Some variation of subclassing (as you mentioned above). This is still somewhat a "roll-up your sleeves and do the work" option. Except that it's more fragile IMO, as it binds user's code more tightly into scalasql's class hierarchy. A solution based on composition rather than inheritance seems more appealing from a forward-compatibility standpoint, so I'd rate this as strictly worse than 2a.

3. Proposed solution - I guess this is perhaps the "batteries included" option, and arguably has the most API footprint and the added footgun of a mutable listener list. That being said, listeners are well understood practice, the lifecycle is very clear and bounded to the transaction's lifecycle for dynamically-added listeners, so leaks are unlikely. There's already a precedent of mutability in the API related to savepoints, and that API seems consistent with the overall scalasql design. The weakest part of the proposal as it stands, IMO, is the concurrency aspect, which could be strengthened by making the listener collection (and, arguably, the savepoints) thread-safe by default. Since JDBC connections are thread-safe by default, this would follow the principle of least surprise, albeit there's a slight overhead related to that.

Are there other options you think we should explore? I'm open to ideas.

Also, if you're not ready to decide on this and prefer to wait, that's fine too. In such case, we'll wrap the transaction API, move on, and probably not look back.

[aboisvert]

Actually after double-checking, I just realized JDBC connections are not thread-safe -- although some drivers (e.g., PostgreSQL) do enforce thread safety; so the current savepoint and the proposed listener APIs are technically correct, although they may still benefit from some added safety.

(I misremembered since I also used XA transactions before with a JTA transaction manager where multiple connections can participate in the same transaction, albeit they still shouldn't be shared across threads. Anyway, digressing...)

[lihaoyi]

Overall I think your reasoning makes sense.

For txn.addTransactionListener, I think having a mutable API makes sense, since transactions are inherently mutable things.

dbClient.addTransactionListener I find a bit more awkward. DbClient is meant to be a pretty stateless wrapper around a DataSource or equivalent. Could we do a copy-constructor dbClient.withTransactionListener instead?

Otherwise I think it looks reasonable to me

[aboisvert]

Sounds good. I'll update the PR with a non-mutable version of DbClient and copy-constructor.

[aboisvert]

I'll update the PR with a non-mutable version of DbClient and copy-constructor.

Change implemented in 375e8a3

I decided against putting withTransactionListener on DbClient, as one of the implementation is Connection, and it seems ill-suited to provide a copy-constructor on an object that owns an underlying resource (jdbc.Connection) without copying that resource as well.

Instead, I only added withTransactionListener to DataSource, which is a provider of connection/DbClient.

[lihaoyi]

@aboisvert looks good to me, if you're done with this PR I'll merge it and cut a numbered release

[aboisvert]

Yes, all done. Ready to merge.

[lihaoyi]

Pushed a tag for 0.1.16, should become available in an hour or two once it makes it through CI and publishing