feat: database transactions #48
Conversation
| } | ||
| } | ||
| await this.transactionManager.runInTransaction(async (tx) => { | ||
| this.logger.debug("Starting transaction..."); |
There was a problem hiding this comment.
maybe would say "starting changeset transaction" or something since I imagine the repo may have other transactions happening
There was a problem hiding this comment.
ideally somewhere there's a completed message too
There was a problem hiding this comment.
addressed: 6d5b44e
| @@ -96,25 +96,23 @@ export class KyselyApplicationRepository implements IApplicationRepository { | |||
| } | |||
|
|
|||
| /* @inheritdoc */ | |||
| async insertApplication(application: NewApplication): Promise<void> { | |||
| async insertApplication(application: NewApplication, tx?: Kysely<Database>): Promise<void> { | |||
There was a problem hiding this comment.
shouldn't this be called db not tx? elsewhere in the repo you define db: Kysely
| } | ||
|
|
||
| /* @inheritdoc */ | ||
| async updateApplication( | ||
| where: { id: string; chainId: ChainId; roundId: string }, | ||
| application: PartialApplication, | ||
| tx?: Kysely<Database>, |
| import { Database, ITransactionManager } from "../../internal.js"; | ||
|
|
||
| export class KyselyTransactionManager implements ITransactionManager<Kysely<Database>> { | ||
| constructor(private readonly db: Kysely<Database>) {} |
There was a problem hiding this comment.
same thing, it's a db not a tx right 🤔
| @@ -0,0 +1,8 @@ | |||
| // packages/repository/src/types/transaction.types.ts | |||
|
|
||
| import { Database } from "../internal.js"; | ||
|
|
||
| export type KyselyTransaction = Kysely<Database>; |
There was a problem hiding this comment.
why are we using Kysely<Database> for the params that are "tx" rather than using KyselyTransaction? It might be nice to use KyselyTransaction for those if these really are txs so that it looks intentional rather than both txs and db params typed as Kysely<Database>
There was a problem hiding this comment.
good catch ser, will update to the Transaction type from Kysely, this also addresses your prev comments and avoids the confussion
There was a problem hiding this comment.
addressed: 6d5b44e
There was a problem hiding this comment.
is there a way to test reveted records on the database, like an in memory pg ?
There was a problem hiding this comment.
Maybe i am not sure how txs work. Changes are applied one by one and if some fails it reverts the applied ? or it applies all the changes at the same time and if something fails nothing is applied ?
There was a problem hiding this comment.
txs are all or nothing, so all changes are applied or neither of them. regarding tests, yeah that would be nice but these are e2e tests not unit so i was planning on attacking them on the e2e milestone, we don't want to test Kysely library itself but rather that the whole system works as expected
There was a problem hiding this comment.
should we add a todo or linear task for testing invalid inputs to handlers ?
There was a problem hiding this comment.
add it to Enhancements milestone 🫡
🤖 Linear
Closes GIT-217 GIT-214
Description
As part of advanced recovery & error handling, we implement database transactions so we have a more granular control on DB operations, having the ability to rollback changes when -in a set of Changesets- one of the operation fails.
Note: the callback pattern isn't the best option but we're limited to it given the current Transaction API from Kysely
Checklist before requesting a review