fix: add transport-neutral database contracts - #1242
Conversation
Coverage Report for CI Build 29827004718Coverage increased (+0.1%) to 79.69%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
8b8e397 to
3ea2a04
Compare
to prepare for watt adapter Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
3ea2a04 to
ed47081
Compare
|
@claude review |
There was a problem hiding this comment.
I didn't find any bugs, but this is a large refactor (43 files) that changes the core database transport boundary (transaction/connection contracts used throughout the storage service), so it's worth a human look given the blast radius.
Extended reasoning...
Overview
This PR extracts transport-neutral interfaces (DatabaseExecutor, DatabaseTransaction, DatabaseTransactionalExecutor, TenantConnection) into a new src/internal/database/connection.ts module, and updates ~40 call sites across auth, queue, sharding, iceberg, vector, and S3 protocol code to depend on these interfaces instead of the concrete PgExecutor/PgTransaction/PgTenantConnection classes. It also replaces several instanceof PgTransaction checks with a duck-typed isDatabaseTransaction() guard, and changes a few call sites (e.g. runUnscopedQuery, iceberg/bucket-deletion events, several tests) to pass the tenant connection itself instead of connection.pool.acquire(), relying on the connection's query() method acquiring from the pool internally.
Security risks
No new security-sensitive surface (auth, crypto, permissions) is introduced — this is a type/interface-boundary refactor. The behavior of query execution, transaction commit/rollback, and scope-setting (role/JWT claims) is preserved; only the static types and a duck-typing check change.
Level of scrutiny
This warrants more than a rubber-stamp pass because it touches the database transaction/connection layer that underlies almost every storage operation (object CRUD, multipart uploads, iceberg catalog, vector store, migrations, pg-boss queue). Even though the diff is largely mechanical renames, the switch from instanceof PgTransaction to a duck-typed isDatabaseTransaction() check and the removal of explicit pool.acquire() calls are subtle semantic changes in code paths responsible for correct transaction/savepoint handling — exactly the kind of critical-path change that benefits from a second set of eyes, even absent a concrete bug.
Other factors
Test coverage is extensive and was updated alongside the production code (mirrored type changes in ~15 test files), and coveralls reports 100% patch coverage with no regressions. The PR is explicitly described as a precursor/refactor for a follow-up integration (#1230), which further supports treating it as infrastructural and higher-risk despite the lack of found bugs.
What kind of change does this PR introduce?
refactor
What is the current behavior?
Current storage worker owns pools, client and serialization so it passes concrete implementations (PgTransaction, PgTenantConnection, etc.)
What is the new behavior?
When we have a different implementation (a separate Watt application #1230), it should pretend. This splits the boundary and prepares for a new implementation.
Additional context
Precursor to #1230