multiple function calls within single transaction#160
Conversation
salamonpavel
requested review from
Zejnilovic,
benedeki,
dk1844,
jakipatryk,
lsulak and
miroslavpojer
as code owners
WalkthroughAdds ConnectionIO-returning APIs to Doobie function wrappers, a DoobieEngine.runConnectionIO transaction runner, and integration tests that compose multiple function calls into a single database transaction and verify rollback and compatibility behaviours. ChangesTransactional composition of database function calls
Sequence DiagramsequenceDiagram
participant Caller
participant DoobieFunction
participant ConnectionIO
participant DoobieEngine
participant Database
Caller->>DoobieFunction: toConnectionIO(values)
DoobieFunction->>ConnectionIO: returns unevaluated program
Caller->>DoobieFunction: toConnectionIOSingle(values2)
DoobieFunction->>ConnectionIO: returns unevaluated program
Caller->>ConnectionIO: compose programs
Caller->>DoobieEngine: runConnectionIO(composed)
DoobieEngine->>Database: transact(composed)
Database-->>DoobieEngine: result / rollback
DoobieEngine-->>Caller: F[Result]
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JaCoCo
|
| Overall Project | 59.84% | 🍏 |
|---|
There is no coverage information present for the Files changed
JaCoCo
|
| Overall Project | 70.29% -3.02% |
🍏 |
|---|---|---|
| Files changed | 67.74% | ❌ |
| File | Coverage | |
|---|---|---|
| DoobieEngine.scala | 95.29% -10.59% |
❌ |
| DoobieFunction.scala | 73.49% -3.74% |
❌ |
JaCoCo
|
| Overall Project | 91.42% | 🍏 |
|---|
There is no coverage information present for the Files changed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doobie/src/main/scala/za/co/absa/db/fadb/doobie/DoobieFunction.scala`:
- Around line 131-134: The eager evaluation of toFragmentsSeq(values) in
toConnectionIO (and the similar status/single/optional variants) allows
exceptions to escape before entering the ConnectionIO context; change these to
defer evaluation by wrapping the call in the library's effectful delay (e.g.,
ConnectionIO/FC delay) and then flatMap into composeFragments(...).query...
(i.e., replace the val fragments = toFragmentsSeq(values) with a delayed effect
that computes fragments inside ConnectionIO and then .flatMap(fragments =>
composeFragments(fragments).query[R].to[Seq]/.unique/.option/.status) so any
thrown exceptions are captured by the ConnectionIO/MonadError machinery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c36cd8b5-9b2d-4f4d-af81-41290e8dfd3b
📒 Files selected for processing (3)
doobie/src/main/scala/za/co/absa/db/fadb/doobie/DoobieEngine.scaladoobie/src/main/scala/za/co/absa/db/fadb/doobie/DoobieFunction.scaladoobie/src/test/scala/za/co/absa/db/fadb/doobie/DoobieTransactionCompositionIntegrationTests.scala
|
Read the code, easy to understand. It is really good and will be useful. Just some random thoughts:
|
jakipatryk
left a comment
jakipatryk
left a comment
There was a problem hiding this comment.
Makes sense, the comment from CodeRabbit is probably worth addressing though.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
doobie/src/main/scala/za/co/absa/db/fadb/doobie/DoobieEngine.scala (1)
90-101: ⚡ Quick winConsider enhancing documentation to address transaction semantics questions.
The implementation correctly uses
transactfor atomic transaction execution. However, the PR objectives note that a reviewer asked whether the transaction will roll back all operations on failure. Consider making the rollback-on-failure and commit-on-success semantics explicit in the documentation to address this concern.📝 Proposed documentation enhancement
/** * Executes an arbitrary `ConnectionIO` program in a single transaction. * This enables composing multiple database operations (including multiple fa-db function calls) * into a single atomic transaction. + * + * Transaction semantics: All operations will be committed on success. If any operation fails, + * the entire transaction will be rolled back and the error will be propagated in the effect `F`. * * `@param` cio the `ConnectionIO` program to execute * `@tparam` R the result type of the program * `@return` the result wrapped in the effect type `F` */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doobie/src/main/scala/za/co/absa/db/fadb/doobie/DoobieEngine.scala` around lines 90 - 101, Update the ScalaDoc for runConnectionIO to explicitly state transaction semantics: that cio.transact(transactor) will run the ConnectionIO program in a single atomic transaction, commit when the program completes successfully, and automatically roll back all operations if the program raises an exception (or the effect F fails); reference the runConnectionIO method and the use of ConnectionIO.transact with the transactor so readers know where this behavior comes from.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@doobie/src/main/scala/za/co/absa/db/fadb/doobie/DoobieEngine.scala`:
- Around line 90-101: Update the ScalaDoc for runConnectionIO to explicitly
state transaction semantics: that cio.transact(transactor) will run the
ConnectionIO program in a single atomic transaction, commit when the program
completes successfully, and automatically roll back all operations if the
program raises an exception (or the effect F fails); reference the
runConnectionIO method and the use of ConnectionIO.transact with the transactor
so readers know where this behavior comes from.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db62e4a8-d07e-422f-b922-4d169773ec35
📒 Files selected for processing (2)
doobie/src/main/scala/za/co/absa/db/fadb/doobie/DoobieEngine.scaladoobie/src/main/scala/za/co/absa/db/fadb/doobie/DoobieFunction.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- doobie/src/main/scala/za/co/absa/db/fadb/doobie/DoobieFunction.scala
benedeki
left a comment
benedeki
left a comment
There was a problem hiding this comment.
The code looks elegant, indeed 👏
The problem I have with the introduction of transaction is, that it goes against the basic paradigm of the library.
Separation of duties and responsibilities between the database and the application
Particularly you rely on the fact that on the database side, there is a single instance supporting transaction. Or conversely you enforce the fact (of single instance) preventing to migrate the database from single instance to cluster.
So from that point of view the less idiomatic method name serves well, as it prevents thoughtless use to some extent.
Speaking off, I am not sure in what state id the documentation, but this method surely wasn't added there (if any exists).
| * @tparam R the result type of the program | ||
| * @return the result wrapped in the effect type `F` | ||
| */ | ||
| def runConnectionIO[R](cio: ConnectionIO[R]): F[R] = |
There was a problem hiding this comment.
The method name doesn't sound too idiomatic to me. But at the end it might be a good thing, in regard of my final comment (coming 😉 )
| val program: ConnectionIO[Unit] = for { | ||
| _ <- createActor.toConnectionIOSingle(CreateActorRequestBody("BatchActor1", lastName)) | ||
| _ <- createActor.toConnectionIOSingle(CreateActorRequestBody("BatchActor2", lastName)) | ||
| _ <- createActor.toConnectionIOSingle(CreateActorRequestBody("BatchActor3", lastName)) |
There was a problem hiding this comment.
What would happen if a "old fashion" call to apply is in the middle here. E.g.
_ <- createActor.toConnectionIOSingle(CreateActorRequestBody("BatchActor2", lastName))
val foo = createActor(CreateActorRequestBody("Intruder1", uniqueLast)).unsafeRunSync()
bar <= createActor(CreateActorRequestBody("Intruder2", uniqueLast)).unsafeRunSync()
_ <- createActor.toConnectionIOSingle(CreateActorRequestBody("BatchActor3", lastName))
| values => Seq(fr"${values.firstName}", fr"${values.lastName}") | ||
| ) | ||
| with StandardStatusHandling { | ||
| override def fieldsToSelect: Seq[String] = super.fieldsToSelect ++ Seq("o_actor_id") |
There was a problem hiding this comment.
ah damn, I see it's here: https://github.com/AbsaOSS/fa-db/blob/master/demo_database/src/main/postgres/integration/V1.2.5__create_actor.sql#L39
but basically the convention we made a while ago was i_ prefix for input but no prefix for output parameters
5 participants
Closes #159
Adds support for multiple function calls within single transaction in Doobie module.
Summary by CodeRabbit
New Features
Tests