From 6251d5cb875a5db541343122199a4c3ed718ea20 Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 5 Aug 2026 13:26:53 +0530 Subject: [PATCH 1/2] fix(db): report rollback outcome correctly in WithTxn WithTxn wrapped every failed transaction as a rollback failure with a nil rollback error in the message and added the "rollback:" prefix twice. Check the rollback's own error instead, wrap once, and drop the dead error writes in the panic path. Fix inverted error assertions in the user repository tests that the double wrap was masking. Co-Authored-By: Claude Fable 5 --- .../store/postgres/user_repository_test.go | 16 +- pkg/db/db.go | 11 +- pkg/db/db_test.go | 139 ++++++++++++++++++ 3 files changed, 149 insertions(+), 17 deletions(-) create mode 100644 pkg/db/db_test.go diff --git a/internal/store/postgres/user_repository_test.go b/internal/store/postgres/user_repository_test.go index f00aa0940..6b23c9ddf 100644 --- a/internal/store/postgres/user_repository_test.go +++ b/internal/store/postgres/user_repository_test.go @@ -366,8 +366,8 @@ func (s *UserRepositoryTestSuite) TestUpdateByEmail() { s.Run(tc.Description, func() { got, err := s.repository.UpdateByEmail(s.ctx, tc.UserToUpdate) if tc.Err != nil && tc.Err.Error() != "" { - if errors.Unwrap(err) == tc.Err { - s.T().Fatalf("got error %s, expected was %s", err.Error(), tc.Err) + if !errors.Is(err, tc.Err) { + s.T().Fatalf("got error %v, expected was %v", err, tc.Err) } } @@ -451,8 +451,8 @@ func (s *UserRepositoryTestSuite) TestUpdateByID() { s.Run(tc.Description, func() { got, err := s.repository.UpdateByID(s.ctx, tc.UserToUpdate) if tc.Err != nil && tc.Err.Error() != "" { - if errors.Unwrap(err) == tc.Err { - s.T().Fatalf("got error %s, expected was %s", err.Error(), tc.Err) + if !errors.Is(err, tc.Err) { + s.T().Fatalf("got error %v, expected was %v", err, tc.Err) } } // TODO(kushsharma): remove metadata field from ignore once metadata is refactored @@ -559,8 +559,8 @@ func (s *UserRepositoryTestSuite) TestGetByName() { s.Run(tc.Description, func() { got, err := s.repository.GetByName(s.ctx, tc.Name) if tc.Err != nil && tc.Err.Error() != "" { - if errors.Unwrap(err) == tc.Err { - s.T().Fatalf("got error %s, expected was %s", err.Error(), tc.Err) + if !errors.Is(err, tc.Err) { + s.T().Fatalf("got error %v, expected was %v", err, tc.Err) } } if !cmp.Equal(got, tc.ExpectedUser) { @@ -610,8 +610,8 @@ func (s *UserRepositoryTestSuite) TestUpdateByName() { s.Run(tc.Description, func() { got, err := s.repository.UpdateByName(s.ctx, tc.UserToUpdate) if tc.Err != nil && tc.Err.Error() != "" { - if errors.Unwrap(err) == tc.Err { - s.T().Fatalf("got error %s, expected was %s", err.Error(), tc.Err) + if !errors.Is(err, tc.Err) { + s.T().Fatalf("got error %v, expected was %v", err, tc.Err) } } diff --git a/pkg/db/db.go b/pkg/db/db.go index 92f00e97c..8b8ebb84e 100644 --- a/pkg/db/db.go +++ b/pkg/db/db.go @@ -76,21 +76,14 @@ func (c Client) WithTxn(ctx context.Context, txnOptions sql.TxOptions, txFunc fu defer func() { if p := recover(); p != nil { - switch p := p.(type) { - case error: - err = p - default: - err = fmt.Errorf("%s", p) - } - err = txn.Rollback() + _ = txn.Rollback() panic(p) } else if err != nil { - if rlbErr := txn.Rollback(); err != nil { + if rlbErr := txn.Rollback(); rlbErr != nil { err = fmt.Errorf("rollback error: %s while executing: %w", rlbErr, err) } else { err = fmt.Errorf("rollback: %w", err) } - err = fmt.Errorf("rollback: %w", err) } else { err = txn.Commit() } diff --git a/pkg/db/db_test.go b/pkg/db/db_test.go new file mode 100644 index 000000000..8a134586f --- /dev/null +++ b/pkg/db/db_test.go @@ -0,0 +1,139 @@ +package db + +import ( + "context" + "database/sql" + "database/sql/driver" + "errors" + "testing" + + "github.com/jmoiron/sqlx" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeConn is a minimal database/sql/driver connection, in the same style as +// failConnector in internal/store/postgres/fakes_test.go. It hands out fake +// transactions, records commits and rollbacks, and can be told to fail any of +// them, so WithTxn can be tested without a real database. +type fakeConn struct { + beginErr error + commitErr error + rollbackErr error + commits int + rollbacks int +} + +func (c *fakeConn) Prepare(string) (driver.Stmt, error) { return nil, errors.New("not implemented") } +func (c *fakeConn) Close() error { return nil } +func (c *fakeConn) Begin() (driver.Tx, error) { + if c.beginErr != nil { + return nil, c.beginErr + } + return &fakeTx{conn: c}, nil +} + +type fakeTx struct{ conn *fakeConn } + +func (t *fakeTx) Commit() error { t.conn.commits++; return t.conn.commitErr } +func (t *fakeTx) Rollback() error { t.conn.rollbacks++; return t.conn.rollbackErr } + +type fakeConnector struct{ conn *fakeConn } + +func (f fakeConnector) Connect(context.Context) (driver.Conn, error) { return f.conn, nil } +func (f fakeConnector) Driver() driver.Driver { return nil } + +func newFakeClient(t *testing.T, conn *fakeConn) Client { + t.Helper() + client := Client{DB: sqlx.NewDb(sql.OpenDB(fakeConnector{conn: conn}), "postgres")} + t.Cleanup(func() { _ = client.Close() }) + return client +} + +func TestWithTxn(t *testing.T) { + t.Run("commits when the callback succeeds", func(t *testing.T) { + conn := &fakeConn{} + client := newFakeClient(t, conn) + + err := client.WithTxn(context.Background(), sql.TxOptions{}, func(*sqlx.Tx) error { + return nil + }) + + require.NoError(t, err) + assert.Equal(t, 1, conn.commits) + assert.Equal(t, 0, conn.rollbacks) + }) + + t.Run("returns the begin error without running the callback", func(t *testing.T) { + beginErr := errors.New("begin failed") + client := newFakeClient(t, &fakeConn{beginErr: beginErr}) + + called := false + err := client.WithTxn(context.Background(), sql.TxOptions{}, func(*sqlx.Tx) error { + called = true + return nil + }) + + assert.ErrorIs(t, err, beginErr) + assert.False(t, called) + }) + + t.Run("wraps the callback error once when rollback succeeds", func(t *testing.T) { + callbackErr := errors.New("insert failed") + conn := &fakeConn{} + client := newFakeClient(t, conn) + + err := client.WithTxn(context.Background(), sql.TxOptions{}, func(*sqlx.Tx) error { + return callbackErr + }) + + assert.EqualError(t, err, "rollback: insert failed") + assert.ErrorIs(t, err, callbackErr) + assert.Equal(t, 1, conn.rollbacks) + assert.Equal(t, 0, conn.commits) + }) + + t.Run("reports both errors when rollback fails", func(t *testing.T) { + callbackErr := errors.New("insert failed") + conn := &fakeConn{rollbackErr: errors.New("connection lost")} + client := newFakeClient(t, conn) + + err := client.WithTxn(context.Background(), sql.TxOptions{}, func(*sqlx.Tx) error { + return callbackErr + }) + + assert.EqualError(t, err, "rollback error: connection lost while executing: insert failed") + assert.ErrorIs(t, err, callbackErr) + assert.Equal(t, 1, conn.rollbacks) + }) + + t.Run("returns the commit error", func(t *testing.T) { + commitErr := errors.New("commit failed") + conn := &fakeConn{commitErr: commitErr} + client := newFakeClient(t, conn) + + err := client.WithTxn(context.Background(), sql.TxOptions{}, func(*sqlx.Tx) error { + return nil + }) + + assert.ErrorIs(t, err, commitErr) + assert.Equal(t, 1, conn.commits) + }) + + t.Run("rolls back and repanics when the callback panics", func(t *testing.T) { + conn := &fakeConn{} + client := newFakeClient(t, conn) + + defer func() { + p := recover() + require.NotNil(t, p) + assert.Equal(t, "boom", p) + assert.Equal(t, 1, conn.rollbacks) + assert.Equal(t, 0, conn.commits) + }() + + _ = client.WithTxn(context.Background(), sql.TxOptions{}, func(*sqlx.Tx) error { + panic("boom") + }) + }) +} From 7cdc215726cce006b013276e642351b196ce9dbd Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 5 Aug 2026 14:43:09 +0530 Subject: [PATCH 2/2] fix(db): wrap the rollback error with %w in WithTxn Both the rollback error and the callback error are now matchable with errors.Is. The message text is unchanged. Co-Authored-By: Claude Fable 5 --- pkg/db/db.go | 2 +- pkg/db/db_test.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/db/db.go b/pkg/db/db.go index 8b8ebb84e..3d179019b 100644 --- a/pkg/db/db.go +++ b/pkg/db/db.go @@ -80,7 +80,7 @@ func (c Client) WithTxn(ctx context.Context, txnOptions sql.TxOptions, txFunc fu panic(p) } else if err != nil { if rlbErr := txn.Rollback(); rlbErr != nil { - err = fmt.Errorf("rollback error: %s while executing: %w", rlbErr, err) + err = fmt.Errorf("rollback error: %w while executing: %w", rlbErr, err) } else { err = fmt.Errorf("rollback: %w", err) } diff --git a/pkg/db/db_test.go b/pkg/db/db_test.go index 8a134586f..2f3a11128 100644 --- a/pkg/db/db_test.go +++ b/pkg/db/db_test.go @@ -95,7 +95,8 @@ func TestWithTxn(t *testing.T) { t.Run("reports both errors when rollback fails", func(t *testing.T) { callbackErr := errors.New("insert failed") - conn := &fakeConn{rollbackErr: errors.New("connection lost")} + rollbackErr := errors.New("connection lost") + conn := &fakeConn{rollbackErr: rollbackErr} client := newFakeClient(t, conn) err := client.WithTxn(context.Background(), sql.TxOptions{}, func(*sqlx.Tx) error { @@ -104,6 +105,7 @@ func TestWithTxn(t *testing.T) { assert.EqualError(t, err, "rollback error: connection lost while executing: insert failed") assert.ErrorIs(t, err, callbackErr) + assert.ErrorIs(t, err, rollbackErr) assert.Equal(t, 1, conn.rollbacks) })