Skip to content

Add postgres transactions - #15

Open
Rory-Reid wants to merge 2 commits into
mainfrom
add-postgres-transactions
Open

Add postgres transactions#15
Rory-Reid wants to merge 2 commits into
mainfrom
add-postgres-transactions

Conversation

@Rory-Reid

Copy link
Copy Markdown
Collaborator

This tracks the active transaction id within the database connection and clears it when committed/rolled back. The RDS Data API requires that the transaction id is sent along with any successive requests within a transaction.

@pkg-pr-new

pkg-pr-new Bot commented Sep 4, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@kysely-org/aws@15

commit: c247f4b

@Rory-Reid

Copy link
Copy Markdown
Collaborator Author

Test output. Only two new tests:

     ✓ transactions (2)
       ✓ Should COMMIT an INSERT 207ms
       ✓ Should ROLLBACK an INSERT 206ms

Full output:

✗ pnpm test
$ vitest
Testing types with tsc and vue-tsc is an experimental feature.
Breaking changes might not follow SemVer, please pin Vitest's version when using it.                                                                                                 

 DEV  v4.1.9 /Users/rory/Projects/kysely-aws-2

 ✓ test/rds-data-api-smoke.test.ts (32 tests) 5866ms
   ✓ Smoke tests (32)
     ✓ Basic connection and schemaless statements (2)
       ✓ Should SELECT 1 225ms
       ✓ Should SELECT now() 68ms
     ✓ CRUD (18)
       ✓ Should INSERT a single row 141ms
       ✓ Should INSERT multiple rows 140ms
       ✓ Should SELECT all rows 148ms
       ✓ Should SELECT inserted row 149ms
       ✓ Should SELECT a subset of columns 139ms
       ✓ Should SELECT zero rows from empty table 75ms
       ✓ Should filter WHERE equal 222ms
       ✓ Should filter WHERE IS NULL 223ms
       ✓ Should filter WHERE IS NOT NULL 227ms
       ✓ Should filter WHERE LIKE 229ms
       ✓ Should filter WHERE IN 295ms
       ✓ Should LIMIT and OFFSET 291ms
       ✓ Should ORDER results 236ms
       ✓ Should UPDATE 225ms
       ✓ Should not UPDATE with non-matching WHERE clause 228ms
       ✓ Should DELETE 215ms
       ✓ Should not DELETE with non-matching WHERE clause 225ms
       ✓ Should COUNT and aggregate 215ms
     ✓ joins (2)
       ✓ Should INNER JOIN pet to person 228ms
       ✓ Should LEFT JOIN include people without pets  303ms
     ✓ RETURNING (4)
       ✓ Should RETURNING specific columns on INSERT 72ms
       ✓ Should RETURNING all columns on INSERT 71ms
       ✓ Should RETURNING changed rows on UPDATE 143ms
       ✓ Should RETURNING deleted rows on DELETE 145ms
     ✓ raw sql (3)
       ✓ Should execute a sql template tag query with a parameter 139ms
       ✓ Should use sql.ref for dynamic references 139ms
       ✓ Should append a raw WHERE fragment to a query builder query 145ms
     ✓ edge cases (1)
       ✓ Should error with column aliased as empty string 145ms
     ✓ transactions (2)
       ✓ Should COMMIT an INSERT 207ms
       ✓ Should ROLLBACK an INSERT 206ms

 Test Files  1 passed (1)
      Tests  32 passed (32)
Type Errors  no errors
   Start at  22:34:58
   Duration  7.65s (transform 25ms, setup 0ms, import 242ms, tests 5.87s, environment 0ms)

 PASS  

Comment thread src/rds-data-api/driver.ts
This supports concurrent transactions as transaction ids are held as connection object state
@Rory-Reid Rory-Reid changed the title Add transactions Add postgres transactions Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

merge this file into config.ts, no reason these should not be co-located.

Comment on lines +41 to +44
return new RDSDataAPIDatabaseConnection({
...this.#config,
client: this.#client,
typeMapper: this.#config.typeMapper,
executeStatementCommand: this.#config.executeStatementCommand,
}))
})

@igalklebanov igalklebanov Sep 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

keep it flat, no need to spread everywhere.

Suggested change
return new RDSDataAPIDatabaseConnection({
...this.#config,
client: this.#client,
typeMapper: this.#config.typeMapper,
executeStatementCommand: this.#config.executeStatementCommand,
}))
})
return new RDSDataAPIDatabaseConnection(this.#client, this.#config)

Comment on lines 20 to 23
this.#config = {
client: config.client,
...config,
typeMapper: config.typeMapper ?? new DefaultRDSDataAPITypeMapper(),
executeStatementCommand: config.executeStatementCommand,
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nothing should be done here but:

this.#config: freeze({ ...config })

the mapper should be instantiated on driver init.

export type CreateBeginTransactionCommand =
() => RDSDataAPIBeginTransactionCommand

export type RDSDataAPIBeginTransactionCommand = object

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is not helpful.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I can enforce anything better than "A factory function that creates an object" here without direct dependency on the RDS SDK. We don't have any parameters to provide ourselves here. What would you suggest?

@igalklebanov igalklebanov Sep 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

something from the class' public api maybe, copied, not even precisely - just need something to hint at what this is, and fail compiling if anything but something that might be the real thing is passed.

new RollbackTransactionCommand({
...connection,
...input,
}),

@igalklebanov igalklebanov Sep 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah, we need to provide a helper, something like:

function createCommands(config): Pick<RDSDataAPIPostgresDialectConfig, keyof RDSDataAPIPostgresDialectConfig & `${string}Command`> {
  const {
    BeginTransactionCommand,
    CommitTransactionCommand,
    ExecuteStatementCommand,
		RollbackTransactionCommand,
    ...connection,
  } = config

	return {
    beginTransactionCommand: () => new BeginTransactionCommand(connection),
    commitTransactionCommand: (input) => new CommitTransactionCommand({
      ...connection,
      ...input,
    }),
    executeStatementCommand: (input) => new ExecuteStatementCommand({
      ...connection,
      ...input,
    }),
    rollbackTransactionCommand: (input) => new RollbackTransactionCommand({
			...connection,
			...input,
		}),
  }
}

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants