Copilot commented on code in PR #1061: URL: https://github.com/apache/incubator-seata-go/pull/1061#discussion_r2986832017
########## pkg/datasource/sql/tx_xa_test.go: ########## @@ -0,0 +1,392 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package sql + +import ( + "context" + "errors" + "testing" + + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + + "seata.apache.org/seata-go/v2/pkg/datasource/sql/mock" + "seata.apache.org/seata-go/v2/pkg/datasource/sql/types" + "seata.apache.org/seata-go/v2/pkg/protocol/branch" + "seata.apache.org/seata-go/v2/pkg/rm" +) + +type mockXAConnection struct { + commitErr error + rollbackErr error + commitCalls int + rollbackCalls int +} + +func (m *mockXAConnection) Commit(ctx context.Context) error { + m.commitCalls++ + return m.commitErr +} + +func (m *mockXAConnection) Rollback(ctx context.Context) error { + m.rollbackCalls++ + return m.rollbackErr +} + +func TestXATx_commitOnXA_NoGlobalTransaction(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "" + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + }, + } + + err := xaTx.commitOnXA() + assert.NoError(t, err) +} + +func TestXATx_commitOnXA_MissingXAConn(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.TransactionMode = types.XAMode + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: nil, + }, + } + + err := xaTx.commitOnXA() + assert.Error(t, err) + assert.Contains(t, err.Error(), "xa transaction requires xaConn") +} + +func TestXATx_commitOnXA_CommitSuccess_BranchNotRegistered(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 0 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{} + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.commitOnXA() + assert.NoError(t, err) + assert.Equal(t, 1, mockConn.commitCalls) +} + +func TestXATx_commitOnXA_CommitFailure_BranchNotRegistered(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 0 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{ + commitErr: errors.New("XA PREPARE failed"), + } + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.commitOnXA() + assert.Error(t, err) + assert.Contains(t, err.Error(), "XA PREPARE failed") + assert.Equal(t, 1, mockConn.commitCalls) +} + +func TestXATx_commitOnXA_CommitSuccess_BranchRegisteredReportsSuccess(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockMgr := mock.NewMockDataSourceManager(ctrl) + mockMgr.SetBranchType(branch.BranchTypeXA) + rm.GetRmCacheInstance().RegisterResourceManager(mockMgr) Review Comment: This test registers a gomock `DataSourceManager` into the global `rm` cache but never restores the previous XA resource manager. Because the cache is a process-wide singleton, other tests can accidentally hit a finished gomock and panic/fail. Consider using `t.Cleanup` to restore the prior manager (or add a test helper to reset the cache) to keep tests isolated. ```suggestion mockMgr.SetBranchType(branch.BranchTypeXA) prevMgr := rm.GetRmCacheInstance().GetResourceManager(branch.BranchTypeXA) rm.GetRmCacheInstance().RegisterResourceManager(mockMgr) t.Cleanup(func() { rm.GetRmCacheInstance().RegisterResourceManager(prevMgr) }) ``` ########## pkg/datasource/sql/conn_xa.go: ########## @@ -128,11 +128,24 @@ func (c *XAConn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, c.txCtx.XID = tm.GetXID(ctx) c.txCtx.TransactionMode = types.XAMode - tx, err := c.Conn.BeginTx(ctx, opts) + physicalTx, err := c.beginPhysicalTx(ctx, opts) if err != nil { return nil, err } - c.tx = tx + c.tx = physicalTx + + tx, err := newTx( + withDriverConn(c.Conn), + withTxCtx(c.txCtx), + withOriginTx(physicalTx), + withXAConn(c), + ) + if err != nil { Review Comment: `BeginTx()` now starts a real underlying DB transaction via `beginPhysicalTx()` before issuing `XA START` (in `start()`), which can be incompatible with XA semantics (e.g., MySQL generally expects `XA START` when no non-XA transaction is active). Consider avoiding a real `BEGIN/START TRANSACTION` here (e.g., use a no-op `driver.Tx` for `Tx.target`, and/or make `XAConn.Rollback()` not depend on `c.tx.Rollback()`), so the only transaction lifecycle is driven by `XA START/END/PREPARE`. ########## pkg/datasource/sql/tx_xa.go: ########## @@ -32,15 +39,80 @@ func (tx *XATx) Commit() error { return tx.commitOnXA() } +// Rollback executes XA END(TMFAIL), XA ROLLBACK and reports to TC func (tx *XATx) Rollback() error { originTx := tx.tx - if originTx.tranCtx.OpenGlobalTransaction() && originTx.tranCtx.IsBranchRegistered() { - return originTx.report(false) + + if !originTx.tranCtx.OpenGlobalTransaction() { + return nil + } + + xid := originTx.tranCtx.XID + branchID := originTx.tranCtx.BranchID + + log.Infof("xa branch [%d/%s] executing XA rollback", branchID, xid) + + if originTx.xaConn != nil { + if err := originTx.xaConn.Rollback(context.Background()); err != nil { + log.Errorf("xa branch [%d/%s] XA END(TMFAIL) + XA ROLLBACK failed: %v", branchID, xid, err) + if originTx.tranCtx.IsBranchRegistered() { + if reportErr := originTx.report(false); reportErr != nil { + log.Errorf("xa branch [%d/%s] failed to report rollback failure to TC: %v", branchID, xid, reportErr) + } + } + return err + } + log.Infof("xa branch [%d/%s] XA END(TMFAIL) + XA ROLLBACK succeeded", branchID, xid) + } + + if originTx.tranCtx.IsBranchRegistered() { + if err := originTx.report(false); err != nil { + log.Errorf("xa branch [%d/%s] failed to report rollback to TC: %v", branchID, xid, err) Review Comment: `XATx.Rollback()` silently skips XA rollback when `originTx.xaConn` is nil, but still may report rollback to TC if the branch is registered. This can leave the RM state inconsistent while telling TC the branch rolled back. Consider returning an error when `xaConn` is missing (similar to `commitOnXA()`), or falling back to `originTx.target.Rollback()` if that is the intended behavior. ########## pkg/datasource/sql/tx_xa_test.go: ########## @@ -0,0 +1,392 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package sql + +import ( + "context" + "errors" + "testing" + + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + + "seata.apache.org/seata-go/v2/pkg/datasource/sql/mock" + "seata.apache.org/seata-go/v2/pkg/datasource/sql/types" + "seata.apache.org/seata-go/v2/pkg/protocol/branch" + "seata.apache.org/seata-go/v2/pkg/rm" +) + +type mockXAConnection struct { + commitErr error + rollbackErr error + commitCalls int + rollbackCalls int +} + +func (m *mockXAConnection) Commit(ctx context.Context) error { + m.commitCalls++ + return m.commitErr +} + +func (m *mockXAConnection) Rollback(ctx context.Context) error { + m.rollbackCalls++ + return m.rollbackErr +} + +func TestXATx_commitOnXA_NoGlobalTransaction(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "" + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + }, + } + + err := xaTx.commitOnXA() + assert.NoError(t, err) +} + +func TestXATx_commitOnXA_MissingXAConn(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.TransactionMode = types.XAMode + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: nil, + }, + } + + err := xaTx.commitOnXA() + assert.Error(t, err) + assert.Contains(t, err.Error(), "xa transaction requires xaConn") +} + +func TestXATx_commitOnXA_CommitSuccess_BranchNotRegistered(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 0 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{} + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.commitOnXA() + assert.NoError(t, err) + assert.Equal(t, 1, mockConn.commitCalls) +} + +func TestXATx_commitOnXA_CommitFailure_BranchNotRegistered(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 0 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{ + commitErr: errors.New("XA PREPARE failed"), + } + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.commitOnXA() + assert.Error(t, err) + assert.Contains(t, err.Error(), "XA PREPARE failed") + assert.Equal(t, 1, mockConn.commitCalls) +} + +func TestXATx_commitOnXA_CommitSuccess_BranchRegisteredReportsSuccess(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockMgr := mock.NewMockDataSourceManager(ctrl) + mockMgr.SetBranchType(branch.BranchTypeXA) + rm.GetRmCacheInstance().RegisterResourceManager(mockMgr) + mockMgr.EXPECT().BranchReport(gomock.Any(), gomock.Any()).DoAndReturn( + func(ctx context.Context, param rm.BranchReportParam) error { + assert.Equal(t, branch.BranchTypeXA, param.BranchType) + assert.Equal(t, int64(123), param.BranchId) + assert.EqualValues(t, branch.BranchStatusPhaseoneDone, param.Status) + assert.Equal(t, "test-xid", param.Xid) + return nil + }, + ).Times(1) + + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 123 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{} + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.commitOnXA() + assert.NoError(t, err) + assert.Equal(t, 1, mockConn.commitCalls) +} + +func TestXATx_commitOnXA_CommitFailure_BranchRegisteredReportsFailure(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockMgr := mock.NewMockDataSourceManager(ctrl) + mockMgr.SetBranchType(branch.BranchTypeXA) + rm.GetRmCacheInstance().RegisterResourceManager(mockMgr) + mockMgr.EXPECT().BranchReport(gomock.Any(), gomock.Any()).DoAndReturn( + func(ctx context.Context, param rm.BranchReportParam) error { + assert.Equal(t, branch.BranchTypeXA, param.BranchType) + assert.Equal(t, int64(123), param.BranchId) + assert.EqualValues(t, branch.BranchStatusPhaseoneFailed, param.Status) + assert.Equal(t, "test-xid", param.Xid) + return nil + }, + ).Times(1) + + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 123 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{ + commitErr: errors.New("XA PREPARE failed"), + } + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.commitOnXA() + assert.Error(t, err) + assert.Contains(t, err.Error(), "XA PREPARE failed") + assert.Equal(t, 1, mockConn.commitCalls) +} + +func TestXATx_Rollback_NoGlobalTransaction(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "" + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + }, + } + + err := xaTx.Rollback() + assert.NoError(t, err) +} + +func TestXATx_Rollback_BranchNotRegistered(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 0 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{} + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.Rollback() + assert.NoError(t, err) + assert.Equal(t, 1, mockConn.rollbackCalls) +} + +func TestXATx_Rollback_WithXAConn(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 0 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{} + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.Rollback() + assert.NoError(t, err) + assert.Equal(t, 1, mockConn.rollbackCalls) +} + +func TestXATx_Rollback_XAConnError(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 0 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{ + rollbackErr: errors.New("XA ROLLBACK failed"), + } + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.Rollback() + assert.Error(t, err) + assert.Contains(t, err.Error(), "XA ROLLBACK failed") + assert.Equal(t, 1, mockConn.rollbackCalls) +} + +func TestXATx_Rollback_BranchRegisteredReportsFailure(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockMgr := mock.NewMockDataSourceManager(ctrl) + mockMgr.SetBranchType(branch.BranchTypeXA) + rm.GetRmCacheInstance().RegisterResourceManager(mockMgr) + mockMgr.EXPECT().BranchReport(gomock.Any(), gomock.Any()).DoAndReturn( + func(ctx context.Context, param rm.BranchReportParam) error { + assert.Equal(t, branch.BranchTypeXA, param.BranchType) + assert.Equal(t, int64(123), param.BranchId) + assert.EqualValues(t, branch.BranchStatusPhaseoneFailed, param.Status) + assert.Equal(t, "test-xid", param.Xid) + return nil + }, + ).Times(1) + + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 123 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{} + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.Rollback() + assert.NoError(t, err) + assert.Equal(t, 1, mockConn.rollbackCalls) +} + +func TestXATx_Commit_CallsCommitOnXA(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.BranchID = 0 + tranCtx.TransactionMode = types.XAMode + + mockConn := &mockXAConnection{} + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.Commit() + assert.NoError(t, err) + assert.Equal(t, 1, mockConn.commitCalls) +} + +func TestTx_report_NoBranchID(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.BranchID = 0 + + tx := &Tx{ + tranCtx: tranCtx, + } + + err := tx.report(true) + assert.NoError(t, err) +} + +func TestXAConnection_Interface(t *testing.T) { + var _ XAConnection = (*mockXAConnection)(nil) +} + +func TestWithXAConn(t *testing.T) { + mockConn := &mockXAConnection{} + tx := &Tx{} + + opt := withXAConn(mockConn) + opt(tx) + + assert.Equal(t, mockConn, tx.xaConn) +} + +func TestXATx_Commit_BeforeCommitError(t *testing.T) { + tranCtx := types.NewTxCtx() + tranCtx.XID = "test-xid" + tranCtx.TransactionMode = types.XAMode + + hookCalled := false + RegisterTxHook(&mockTxHook{ + beforeCommit: func(tx *Tx) error { + hookCalled = true + return errors.New("hook error") + }, + }) + defer CleanTxHooks() + + mockConn := &mockXAConnection{} + + xaTx := &XATx{ + tx: &Tx{ + tranCtx: tranCtx, + xaConn: mockConn, + }, + } + + err := xaTx.Commit() + assert.Error(t, err) + assert.Contains(t, err.Error(), "hook error") + assert.True(t, hookCalled) + assert.Equal(t, 0, mockConn.commitCalls) +} + +func TestGetStatus(t *testing.T) { + assert.NotNil(t, getStatus(true)) + assert.NotNil(t, getStatus(false)) Review Comment: `assert.NotNil` on `branch.BranchStatus` (a value type) is effectively always true and doesn't validate behavior. Consider asserting the exact expected mapping (e.g., `PhaseoneDone` for `true` and `PhaseoneFailed` for `false`) to make this test meaningful. ```suggestion statusTrue := getStatus(true) assert.Equal(t, branch.PhaseoneDone, statusTrue) statusFalse := getStatus(false) assert.Equal(t, branch.PhaseoneFailed, statusFalse) ``` ########## pkg/datasource/sql/conn_xa_test.go: ########## @@ -169,6 +170,37 @@ func initXAConnTestResource(t *testing.T) (*gomock.Controller, *sql.DB, *mockSQL return ctrl, db, mi, ti } +func newMockXAConn(t *testing.T, ctrl *gomock.Controller, branchID int64) (*XAConn, *mock.MockDataSourceManager, *mock.MockTestDriverTx) { + t.Helper() + + mockMgr := mock.NewMockDataSourceManager(ctrl) + mockMgr.SetBranchType(branch.BranchTypeXA) + rm.GetRmCacheInstance().RegisterResourceManager(mockMgr) Review Comment: `newMockXAConn` registers a gomock `DataSourceManager` into the global `rm` cache but doesn't restore the prior manager. Since `rm` cache is a process-wide singleton, this can leak across tests and cause calls into a finished gomock controller. Consider restoring the previous XA manager in `t.Cleanup` (or introducing a reset/unregister helper for tests). ```suggestion mockMgr := initMockResourceManager(branch.BranchTypeXA, ctrl) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
