Conversation
Reviewer's GuideIntroduce a new Tx interface abstraction for transactions, refactor the existing Tx struct into TxContext, and change DB opening APIs to support both *sql.DB and custom Database/Tx implementations while maintaining backward compatibility paths. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58 +/- ##
==========================================
+ Coverage 78.26% 78.51% +0.24%
==========================================
Files 47 48 +1
Lines 1988 2015 +27
==========================================
+ Hits 1556 1582 +26
- Misses 310 312 +2
+ Partials 122 121 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider adding an explicit compile-time assertion like
var _ Tx = (*sql.Tx)(nil)next to theTxinterface andsqlDBWrapperto ensure future changes to*sql.Txkeep satisfying the new abstraction. - Since
Addnow requires callers to provideDatabaseimplementations and cannot auto-wrap*sql.DBlikeOpen, you might want to add a public helper (e.g.,AddDB(...*sql.DB)orWrapSQLDB(*sql.DB) Database) to avoid pushing every consumer to write their own trivialDatabaseadapter. - In
tx_interface_test.go, thecustomTxtype trackscommitCallsandrollbackCallsbut the tests never assert on those counters, so either add assertions to ensure the hooks are exercised or remove the unused tracking fields to keep the test intent clear.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding an explicit compile-time assertion like `var _ Tx = (*sql.Tx)(nil)` next to the `Tx` interface and `sqlDBWrapper` to ensure future changes to `*sql.Tx` keep satisfying the new abstraction.
- Since `Add` now requires callers to provide `Database` implementations and cannot auto-wrap `*sql.DB` like `Open`, you might want to add a public helper (e.g., `AddDB(...*sql.DB)` or `WrapSQLDB(*sql.DB) Database`) to avoid pushing every consumer to write their own trivial `Database` adapter.
- In `tx_interface_test.go`, the `customTx` type tracks `commitCalls` and `rollbackCalls` but the tests never assert on those counters, so either add assertions to ensure the hooks are exercised or remove the unused tracking fields to keep the test intent clear.
## Individual Comments
### Comment 1
<location path="tx.go" line_range="8-13" />
<code_context>
- *sql.Tx
- noCopy //nolint
- stmts map[string]*sql.Stmt
+// TxContext wraps a Tx interface with a per-transaction prepared
+// statement cache. It is the concrete type returned by Client.BeginTx
+// and Client.Transaction so callers can use the cached prepared
+// statements via the Query/Exec/QueryBuilder/ExecBuilder methods.
+//
+// TxContext implements the Tx interface implicitly — its own Query/Exec
+// methods return sqle's *Rows/*Row (which add binding) and take
+// precedence over the embedded Tx interface methods that return the
</code_context>
<issue_to_address>
**issue:** TxContext does not implement Tx due to method signature differences on Query/Exec.
Because TxContext’s methods return *Rows instead of *sql.Rows, its method set does not match the Tx interface and it does not actually satisfy Tx, despite embedding it. You may want to either reword the comment to avoid claiming interface conformance, or change the method signatures (and possibly expose the sqle-specific methods under different names) if TxContext is intended to implement Tx directly.
</issue_to_address>
### Comment 2
<location path="tx_interface_test.go" line_range="52-61" />
<code_context>
+ return &customTx{Tx: tx}, nil
+}
+
+// TestCustomTxCommitHook proves that a custom Tx implementation
+// (here, customTx) reaches Commit/Rollback when used through
+// *sqle.TxContext.
+func TestCustomTxCommitHook(t *testing.T) {
+ raw := createSQLite3()
+ _, err := raw.Exec("CREATE TABLE `t` (`v` int)")
+ require.NoError(t, err)
+
+ wrapped := &customDB{DB: raw}
+ db := Open(wrapped)
+
+ tx, err := db.Begin(nil)
+ require.NoError(t, err)
+
+ _, err = tx.Exec("INSERT INTO `t` (`v`) VALUES (1)")
+ require.NoError(t, err)
+
+ require.NoError(t, tx.Commit())
+
+ // Pull the row back to verify the commit went through.
+ var v int
+ row := db.QueryRow("SELECT `v` FROM `t` WHERE `v` = 1")
+ require.NoError(t, row.Scan(&v))
+ require.Equal(t, 1, v)
+}
+
</code_context>
<issue_to_address>
**issue (testing):** The test does not assert that the customTx.Commit hook was actually invoked.
Right now this only verifies that the transaction commits and data is visible; it never checks `customTx.commitCalls`. As written, you could remove the `Commit` override and this test would still pass.
To verify the hook is actually invoked, assert on the underlying `customTx`, e.g.:
```go
ctx, err := db.Begin(nil)
require.NoError(t, err)
custom, ok := ctx.Tx.(*customTx)
require.True(t, ok)
require.Equal(t, int32(0), custom.commitCalls.Load())
_, err = ctx.Exec("INSERT INTO `t` (`v`) VALUES (1)")
require.NoError(t, err)
require.NoError(t, ctx.Commit())
require.Equal(t, int32(1), custom.commitCalls.Load())
```
This ensures the test fails if the custom `Commit` hook stops being exercised through `TxContext`.
</issue_to_address>
### Comment 3
<location path="tx_interface_test.go" line_range="78-87" />
<code_context>
+ require.Equal(t, 1, v)
+}
+
+// TestCustomTxRollbackHook proves that Rollback on the underlying
+// customTx is invoked when Transaction falls back to rollback.
+func TestCustomTxRollbackHook(t *testing.T) {
+ raw := createSQLite3()
+ _, err := raw.Exec("CREATE TABLE `t` (`v` int)")
+ require.NoError(t, err)
+
+ wrapped := &customDB{DB: raw}
+ db := Open(wrapped)
+
+ sentinel := errors.New("nope")
+ err = db.Transaction(context.Background(), nil, func(ctx context.Context, tx *TxContext) error {
+ _, err := tx.Exec("INSERT INTO `t` (`v`) VALUES (1)")
+ require.NoError(t, err)
+ return sentinel
+ })
+ require.ErrorIs(t, err, sentinel)
+
+ // The row should not be visible (transaction rolled back).
+ var count int
+ row := db.QueryRow("SELECT COUNT(*) FROM `t`")
+ require.NoError(t, row.Scan(&count))
+ require.Equal(t, 0, count)
+}
+
</code_context>
<issue_to_address>
**issue (testing):** Similarly, the rollback test does not verify that the customTx.Rollback override was used.
This verifies rollback behavior via the absence of rows, but not that the `customTx` wrapper’s `Rollback` was invoked. To ensure the hook is exercised, capture the underlying `customTx` in the transaction callback and assert `rollbackCalls` after the transaction:
```go
wrapped := &customDB{DB: raw}
db := Open(wrapped)
var underlying *customTx
err = db.Transaction(context.Background(), nil, func(ctx context.Context, tx *TxContext) error {
custom, ok := tx.Tx.(*customTx)
require.True(t, ok)
underlying = custom
_, err := tx.Exec("INSERT INTO `t` (`v`) VALUES (1)")
require.NoError(t, err)
return sentinel
})
require.ErrorIs(t, err, sentinel)
require.NotNil(t, underlying)
require.Equal(t, int32(1), underlying.rollbackCalls.Load())
```
This confirms `TxContext.Rollback` delegates to `customTx.Rollback`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+52
to
+61
| // TestCustomTxCommitHook proves that a custom Tx implementation | ||
| // (here, customTx) reaches Commit/Rollback when used through | ||
| // *sqle.TxContext. | ||
| func TestCustomTxCommitHook(t *testing.T) { | ||
| raw := createSQLite3() | ||
| _, err := raw.Exec("CREATE TABLE `t` (`v` int)") | ||
| require.NoError(t, err) | ||
|
|
||
| wrapped := &customDB{DB: raw} | ||
| db := Open(wrapped) |
There was a problem hiding this comment.
issue (testing): The test does not assert that the customTx.Commit hook was actually invoked.
Right now this only verifies that the transaction commits and data is visible; it never checks customTx.commitCalls. As written, you could remove the Commit override and this test would still pass.
To verify the hook is actually invoked, assert on the underlying customTx, e.g.:
ctx, err := db.Begin(nil)
require.NoError(t, err)
custom, ok := ctx.Tx.(*customTx)
require.True(t, ok)
require.Equal(t, int32(0), custom.commitCalls.Load())
_, err = ctx.Exec("INSERT INTO `t` (`v`) VALUES (1)")
require.NoError(t, err)
require.NoError(t, ctx.Commit())
require.Equal(t, int32(1), custom.commitCalls.Load())This ensures the test fails if the custom Commit hook stops being exercised through TxContext.
Comment on lines
+78
to
+87
| // TestCustomTxRollbackHook proves that Rollback on the underlying | ||
| // customTx is invoked when Transaction falls back to rollback. | ||
| func TestCustomTxRollbackHook(t *testing.T) { | ||
| raw := createSQLite3() | ||
| _, err := raw.Exec("CREATE TABLE `t` (`v` int)") | ||
| require.NoError(t, err) | ||
|
|
||
| wrapped := &customDB{DB: raw} | ||
| db := Open(wrapped) | ||
|
|
There was a problem hiding this comment.
issue (testing): Similarly, the rollback test does not verify that the customTx.Rollback override was used.
This verifies rollback behavior via the absence of rows, but not that the customTx wrapper’s Rollback was invoked. To ensure the hook is exercised, capture the underlying customTx in the transaction callback and assert rollbackCalls after the transaction:
wrapped := &customDB{DB: raw}
db := Open(wrapped)
var underlying *customTx
err = db.Transaction(context.Background(), nil, func(ctx context.Context, tx *TxContext) error {
custom, ok := tx.Tx.(*customTx)
require.True(t, ok)
underlying = custom
_, err := tx.Exec("INSERT INTO `t` (`v`) VALUES (1)")
require.NoError(t, err)
return sentinel
})
require.ErrorIs(t, err, sentinel)
require.NotNil(t, underlying)
require.Equal(t, int32(1), underlying.rollbackCalls.Load())This confirms TxContext.Rollback delegates to customTx.Rollback.
Mirror the Database abstraction for *sql.Tx so custom database implementations can return custom transaction types from Begin/BeginTx. - Add Tx interface in database.go (mirrors Database subset: Query/Exec/Prepare/Commit/Rollback). - Database.Begin/BeginTx now return the Tx interface; *sql.Tx satisfies it by default. - Rename the existing Tx struct to TxContext so the new Tx interface can coexist. - TxContext wraps a Tx interface (was *sql.Tx) and keeps the per-transaction prepared-statement cache. - Add sqlDBWrapper (unexported) to adapt *sql.DB to the new Database signature. - Open(...any) accepts Database or *sql.DB; add OpenDB(...*sql.DB) as a type-safe convenience. - Tests: TestCustomTxCommitHook, TestCustomTxRollbackHook, TestCustomTxBeginReturnsInterface, TestOpenDBBackwardCompat, TestOpenMixedArgs. Co-Authored-By: Claude <noreply@anthropic.com>
- Reword TxContext doc: clarify it does NOT implement the Tx interface (Query/Exec return *Rows instead of *sql.Rows). - TestCustomTxCommitHook now asserts the customTx.commitCalls counter so removing the Commit override would break the test. - TestCustomTxRollbackHook captures the underlying *customTx in the transaction callback and asserts rollbackCalls after. - Add public AddDB(*sql.DB) helper so callers don't have to wrap each *sql.DB with sqlDBWrapper manually when calling Add. - Add public WrapSQLDB(*sql.DB) Database for ad-hoc conversion. - Add compile-time assertion that *sqlDBWrapper satisfies Database alongside the existing *sql.Tx satisfies Tx assertion. Co-Authored-By: Claude <noreply@anthropic.com>
The struct is sqle's wrapper that adds a per-transaction prepared statement cache on top of a Tx interface. Renaming to RawTx better conveys the role (a thin wrapper over the underlying transaction) and matches the user's preferred mental model where: - Tx (interface) is the public abstraction - RawTx (struct) is the internal-ish wrapper returned from Client.BeginTx / Client.Transaction Also updates AGENTS.md, CHANGELOG.md, dtc.go, client.go, migrate/migrator.go, tx_test.go, and tx_interface_test.go to match. Co-Authored-By: Claude <noreply@anthropic.com>
The struct is sqle's wrapper that adds a per-transaction prepared statement cache on top of a Tx interface. Renaming to Transaction better conveys the role (a concrete transactional object exposed to callers) and matches the user's preferred mental model where: - Tx (interface) is the public abstraction - Transaction (struct) is the wrapper returned from Client.BeginTx / Client.Transaction This also makes the public API self-documenting: `*sqle.Transaction` reads naturally in user code, while still leaving the `Tx` interface free for the abstraction layer. Also updates AGENTS.md, CHANGELOG.md, dtc.go, client.go, migrate/migrator.go, tx_test.go, and tx_interface_test.go to match. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mirrors the Database abstraction for *sql.Tx so custom database implementations can return custom transaction types from Begin/BeginTx.
Changes
Txinterface indatabase.go(mirrors the Database subset used by sqle: Query/QueryContext/QueryRow/QueryRowContext/Exec/ExecContext/Prepare/PrepareContext/Commit/Rollback).*sql.Txsatisfies it by default.Database.Begin/BeginTxnow return theTxinterface instead of*sql.Tx. Custom database implementations can return any type satisfying the interface.Txstruct renamed toTxContextso the newTxinterface can coexist.TxContextwraps aTxinterface and keeps the per-transaction prepared-statement cache.sqlDBWrapper(unexported) adapts*sql.DBto the newDatabasesignature.Open(...any)accepts any mix ofDatabaseand*sql.DB(runtime detection auto-wraps*sql.DB).OpenDB(...*sql.DB)added as a type-safe convenience for raw*sql.DBslices.db.Addkeeps theDatabaseinterface signature; callers wrap*sql.DBvia&sqlDBWrapper{DB: db}if needed.Tests
New file
tx_interface_test.go:TestCustomTxCommitHook— custom Tx'sCommitfires throughdb.Begin/tx.Commit.TestCustomTxRollbackHook— custom Tx'sRollbackfires whenTransactionreturns an error.TestCustomTxBeginReturnsInterface—Client.BeginTxwraps the custom Tx inTxContext.TestOpenDBBackwardCompat—OpenDB(...*sql.DB)works.TestOpenMixedArgs—Openaccepts both*sql.DBand aDatabasewrapper in one call.Backward compatibility
Open(*sql.DB)keeps working —Opendetects*sql.DBand wraps it viasqlDBWrapperinternally.*sqle.Txis renamed to*sqle.TxContext; this is a breaking change to the struct name.migrate/,dtc.go, andtx_test.goare updated to match.🤖 Generated with Claude Code
Summary by Sourcery
Introduce a Tx interface abstraction and update transaction handling and DB opening APIs to support custom transaction implementations while preserving *sql.DB compatibility.
New Features:
Enhancements:
Documentation:
Tests: