Copilot commented on code in PR #1090:
URL:
https://github.com/apache/incubator-seata-go/pull/1090#discussion_r3006062273
##########
pkg/datasource/sql/db.go:
##########
@@ -221,7 +221,7 @@ func (db *DBResource) ConnectionForXA(ctx context.Context,
xaXid XAXid) (*XAConn
if err != nil {
return nil, fmt.Errorf("get xa new connection failure, xid:%s,
err:%v", xaXid.String(), err)
}
- xaResource, err := xa.CreateXAResource(newDriverConn, types.DBTypeMySQL)
+ xaResource, err := xa.CreateXAResource(newDriverConn, db.dbType)
if err != nil {
return nil, fmt.Errorf("create xa resoruce err:%w", err)
Review Comment:
`ConnectionForXA` now passes `db.dbType` into `xa.CreateXAResource`. For
MariaDB connections (`DBTypeMARIADB`), there is currently no registered XA
factory, so XA mode will start failing with "no XA resource factory
registered..." (regression from the previous hardcoded MySQL resource). Either
register the MySQL factory for `DBTypeMARIADB` as well, or add an explicit
fallback/mapping here.
##########
pkg/datasource/sql/xa/xa_resource.go:
##########
@@ -69,3 +75,63 @@ type XAResource interface {
SetTransactionTimeout(duration time.Duration) bool
Start(ctx context.Context, xid string, flags int) error
}
+
+// XAErrorClassifier abstracts database-specific XA error classification.
+// This allows the upper layer (conn_xa.go) to handle XA errors without
+// importing database-specific driver packages.
+type XAErrorClassifier interface {
+ // IsAlreadyEnded checks if the error indicates the XA branch is
already ended.
+ // For MySQL: XAER_RMFAIL with IDLE state (error 1399).
+ // For PostgreSQL: transaction already committed/rolled back.
+ // For Oracle: ORA-24756 (transaction does not exist).
+ IsAlreadyEnded(err error) bool
+}
+
+// defaultErrorClassifier is a no-op classifier that never matches any error.
+// Used as a fallback when no database-specific classifier is registered.
+type defaultErrorClassifier struct{}
+
+func (c *defaultErrorClassifier) IsAlreadyEnded(err error) bool { return false
}
+
+// XAResourceFactory creates database-specific XA resources and error
classifiers.
+type XAResourceFactory interface {
+ // CreateXAResource creates a new XAResource for the given driver
connection.
+ CreateXAResource(conn driver.Conn) XAResource
+ // CreateErrorClassifier creates a database-specific error classifier.
+ CreateErrorClassifier() XAErrorClassifier
+}
+
+// registry holds registered XAResourceFactory instances per DBType.
+var registry = map[types.DBType]XAResourceFactory{}
+
+// RegisterXAResourceFactory registers a factory for the given database type.
+// Each database driver package should call this in its init() function.
+func RegisterXAResourceFactory(dbType types.DBType, factory XAResourceFactory)
{
+ registry[dbType] = factory
+}
+
+// GetXAResourceFactory returns the registered factory for the given database
type.
+func GetXAResourceFactory(dbType types.DBType) (XAResourceFactory, bool) {
+ f, ok := registry[dbType]
+ return f, ok
+}
+
+// CreateXAResource creates an XAResource for the given database type and
connection.
+// It uses the registered factory for the database type.
+func CreateXAResource(conn driver.Conn, dbType types.DBType) (XAResource,
error) {
+ factory, ok := GetXAResourceFactory(dbType)
+ if !ok {
+ return nil, fmt.Errorf("no XA resource factory registered for
db type: %s", dbType.String())
+ }
+ return factory.CreateXAResource(conn), nil
+}
Review Comment:
`RegisterXAResourceFactory` accepts a potentially nil `factory` and
`CreateXAResource` unconditionally calls `factory.CreateXAResource(conn)`. A
nil factory (or a factory that returns nil) will lead to a later nil
dereference. Consider validating inputs in `RegisterXAResourceFactory` and
returning an error from `CreateXAResource` if the resolved factory/resource is
nil.
##########
pkg/datasource/sql/xa/mysql_xa_connection.go:
##########
@@ -26,17 +26,51 @@ import (
"strings"
"time"
+ "github.com/go-sql-driver/mysql"
+
+ "seata.apache.org/seata-go/v2/pkg/datasource/sql/types"
"seata.apache.org/seata-go/v2/pkg/util/log"
)
-type MysqlXAConn struct {
- driver.Conn
+func init() {
+ RegisterXAResourceFactory(types.DBTypeMySQL, &mysqlXAResourceFactory{})
Review Comment:
The MySQL XA factory is only registered for `DBTypeMySQL`. Since
`ParseDBType` now recognizes `mariadb` and `ConnectionForXA` uses the actual
`dbType`, XA for MariaDB will fail unless you also register this factory for
`types.DBTypeMARIADB` (MariaDB uses MySQL-style XA SQL).
```suggestion
RegisterXAResourceFactory(types.DBTypeMySQL, &mysqlXAResourceFactory{})
RegisterXAResourceFactory(types.DBTypeMARIADB,
&mysqlXAResourceFactory{})
```
##########
pkg/datasource/sql/types/types_test.go:
##########
@@ -114,7 +114,9 @@ func TestParseDBType(t *testing.T) {
{"mysql", "mysql", DBTypeMySQL},
{"MySQL uppercase", "MySQL", DBTypeMySQL},
{"MYSQL", "MYSQL", DBTypeMySQL},
- {"postgres", "postgres", DBTypeUnknown},
+ {"postgres", "postgres", DBTypePostgreSQL},
+ {"postgresql", "postgresql", DBTypePostgreSQL},
+ {"oracle", "oracle", DBTypeOracle},
Review Comment:
`ParseDBType` adds several new aliases (`pgx`, `godror`, `go-ora`,
`sqlserver`/`mssql`, `mariadb`), but the table-driven test only covers a
subset. Please add test cases for the newly supported driver names so
regressions in the mapping are caught early.
```suggestion
{"postgresql", "postgresql", DBTypePostgreSQL},
{"pgx", "pgx", DBTypePostgreSQL},
{"oracle", "oracle", DBTypeOracle},
{"godror", "godror", DBTypeOracle},
{"go-ora", "go-ora", DBTypeOracle},
{"sqlserver", "sqlserver", DBTypeSQLServer},
{"mssql", "mssql", DBTypeSQLServer},
{"mariadb", "mariadb", DBTypeMARIADB},
```
##########
pkg/datasource/sql/xa/xa_resource.go:
##########
@@ -69,3 +75,63 @@ type XAResource interface {
SetTransactionTimeout(duration time.Duration) bool
Start(ctx context.Context, xid string, flags int) error
}
+
+// XAErrorClassifier abstracts database-specific XA error classification.
+// This allows the upper layer (conn_xa.go) to handle XA errors without
+// importing database-specific driver packages.
+type XAErrorClassifier interface {
+ // IsAlreadyEnded checks if the error indicates the XA branch is
already ended.
+ // For MySQL: XAER_RMFAIL with IDLE state (error 1399).
+ // For PostgreSQL: transaction already committed/rolled back.
+ // For Oracle: ORA-24756 (transaction does not exist).
+ IsAlreadyEnded(err error) bool
+}
+
+// defaultErrorClassifier is a no-op classifier that never matches any error.
+// Used as a fallback when no database-specific classifier is registered.
+type defaultErrorClassifier struct{}
+
+func (c *defaultErrorClassifier) IsAlreadyEnded(err error) bool { return false
}
+
+// XAResourceFactory creates database-specific XA resources and error
classifiers.
+type XAResourceFactory interface {
+ // CreateXAResource creates a new XAResource for the given driver
connection.
+ CreateXAResource(conn driver.Conn) XAResource
+ // CreateErrorClassifier creates a database-specific error classifier.
+ CreateErrorClassifier() XAErrorClassifier
+}
+
+// registry holds registered XAResourceFactory instances per DBType.
+var registry = map[types.DBType]XAResourceFactory{}
+
+// RegisterXAResourceFactory registers a factory for the given database type.
+// Each database driver package should call this in its init() function.
+func RegisterXAResourceFactory(dbType types.DBType, factory XAResourceFactory)
{
+ registry[dbType] = factory
+}
Review Comment:
`registry` is a plain map with no synchronization. Since
`RegisterXAResourceFactory` is exported, calling it after init (or concurrently
with `GetXAResourceFactory`) can cause data races / `concurrent map writes`
panics. Consider guarding `registry` with a `sync.RWMutex` (or using
`sync.Map`) and documenting/enforcing that registration is init-time only
(e.g., panic or return error on duplicate/late registration).
##########
pkg/datasource/sql/types/types.go:
##########
@@ -96,6 +96,14 @@ func ParseDBType(driverName string) DBType {
switch strings.ToLower(driverName) {
case "mysql":
return DBTypeMySQL
+ case "postgres", "postgresql", "pgx":
+ return DBTypePostgreSQL
+ case "oracle", "godror", "go-ora":
+ return DBTypeOracle
+ case "sqlserver", "mssql":
+ return DBTypeSQLServer
+ case "mariadb":
+ return DBTypeMARIADB
Review Comment:
`DBType` has a generated `String()` implementation in
`pkg/datasource/sql/types/dbtype_string.go`, but it currently does not include
`DBTypeMARIADB`. Now that `ParseDBType` can return `DBTypeMARIADB`, errors/logs
using `dbType.String()` will show `DBType(6)` instead of a stable name.
Regenerate/update the stringer output to include the new constant(s).
```suggestion
return DBTypeMySQL
```
--
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]
