From cc0f75b3061f386dee07ab8a8b9ab1f5eeb1056e Mon Sep 17 00:00:00 2001 From: nomand-zc Date: Thu, 27 Nov 2025 14:00:37 +0800 Subject: [PATCH 1/6] session/redis: refactor new service logic --- session/redis/service.go | 34 +++++++--------------------------- 1 file changed, 7 insertions(+), 27 deletions(-) diff --git a/session/redis/service.go b/session/redis/service.go index 5837875b3..5691c86ca 100644 --- a/session/redis/service.go +++ b/session/redis/service.go @@ -104,39 +104,19 @@ func NewService(options ...ServiceOpt) (*Service, error) { option(&opts) } - var redisClient redis.UniversalClient - var err error - builder := storage.GetClientBuilder() - + builderOpts := []storage.ClientBuilderOpt{ + storage.WithClientBuilderURL(opts.url), + storage.WithExtraOptions(opts.extraOptions...), + } // if instance name set, and url not set, use instance name to create redis client if opts.url == "" && opts.instanceName != "" { - builderOpts, ok := storage.GetRedisInstance(opts.instanceName) - if !ok { + var ok bool + if builderOpts, ok = storage.GetRedisInstance(opts.instanceName); !ok { return nil, fmt.Errorf("redis instance %s not found", opts.instanceName) } - redisClient, err = builder(builderOpts...) - if err != nil { - return nil, fmt.Errorf("create redis client from instance name failed: %w", err) - } - s := &Service{ - opts: opts, - redisClient: redisClient, - sessionTTL: opts.sessionTTL, - appStateTTL: opts.appStateTTL, - userStateTTL: opts.userStateTTL, - } - if opts.enableAsyncPersist { - s.startAsyncPersistWorker() - } - // Always start async summary workers by default. - s.startAsyncSummaryWorker() - return s, nil } - redisClient, err = builder( - storage.WithClientBuilderURL(opts.url), - storage.WithExtraOptions(opts.extraOptions...), - ) + redisClient, err := storage.GetClientBuilder()(builderOpts...) if err != nil { return nil, fmt.Errorf("create redis client from url failed: %w", err) } From 68796533c76119c98e3f6dd169a25a7f6a674a62 Mon Sep 17 00:00:00 2001 From: nomand-zc Date: Thu, 27 Nov 2025 14:56:55 +0800 Subject: [PATCH 2/6] update --- memory/mysql/service.go | 38 +++++++++++---------------------- memory/mysql/service_test.go | 8 ------- memory/postgres/service.go | 34 ++++++++++------------------- memory/postgres/service_test.go | 11 ++-------- memory/redis/service.go | 35 ++++++------------------------ session/mysql/service.go | 35 +++++++++++------------------- session/mysql/service_test.go | 6 +++--- session/postgres/service.go | 34 ++++++++++------------------- 8 files changed, 59 insertions(+), 142 deletions(-) diff --git a/memory/mysql/service.go b/memory/mysql/service.go index e42e7b8c9..c6d53d176 100644 --- a/memory/mysql/service.go +++ b/memory/mysql/service.go @@ -15,7 +15,6 @@ import ( "crypto/sha256" "database/sql" "encoding/json" - "errors" "fmt" "sort" "strings" @@ -69,33 +68,22 @@ func NewService(options ...ServiceOpt) (*Service, error) { option(&opts) } - // Create MySQL client - builder := storage.GetClientBuilder() - var db storage.Client - var err error - + builderOpts := []storage.ClientBuilderOpt{ + storage.WithClientBuilderDSN(opts.dsn), + storage.WithExtraOptions(opts.extraOptions...), + } // Priority: dsn > instanceName. - if opts.dsn != "" { - // Method 1: Use DSN directly (recommended). - db, err = builder( - storage.WithClientBuilderDSN(opts.dsn), - storage.WithExtraOptions(opts.extraOptions...), - ) - if err != nil { - return nil, fmt.Errorf("create mysql client from dsn failed: %w", err) - } - } else if opts.instanceName != "" { - // Method 2: Use pre-registered MySQL instance. - builderOpts, ok := storage.GetMySQLInstance(opts.instanceName) - if !ok { + if opts.dsn == "" && opts.instanceName != "" { + var ok bool + if builderOpts, ok = storage.GetMySQLInstance(opts.instanceName); !ok { return nil, fmt.Errorf("mysql instance %s not found", opts.instanceName) } - db, err = builder(builderOpts...) - if err != nil { - return nil, fmt.Errorf("create mysql client from instance name failed: %w", err) - } - } else { - return nil, errors.New("either dsn or instance name must be provided") + } + + // Method 1: Use DSN directly (recommended). + db, err := storage.GetClientBuilder()(builderOpts...) + if err != nil { + return nil, fmt.Errorf("create mysql client from dsn failed: %w", err) } s := &Service{ diff --git a/memory/mysql/service_test.go b/memory/mysql/service_test.go index 36ef5a32a..387f697b8 100644 --- a/memory/mysql/service_test.go +++ b/memory/mysql/service_test.go @@ -180,14 +180,6 @@ func TestNewService_DSNPriority(t *testing.T) { assert.NoError(t, mock.ExpectationsWereMet()) } -// TestNewService_MissingDSNAndInstance tests that service creation fails when neither DSN nor instanceName is provided. -func TestNewService_MissingDSNAndInstance(t *testing.T) { - service, err := NewService() - require.Error(t, err) - assert.Nil(t, service) - assert.Contains(t, err.Error(), "either dsn or instance name must be provided") -} - // TestNewService_WithSkipDBInit tests that skipDBInit option skips database initialization. func TestNewService_WithSkipDBInit(t *testing.T) { mockDB, mock := setupMockDB(t) diff --git a/memory/postgres/service.go b/memory/postgres/service.go index 8048511ac..23aac2b8d 100644 --- a/memory/postgres/service.go +++ b/memory/postgres/service.go @@ -69,36 +69,24 @@ func NewService(options ...ServiceOpt) (*Service, error) { option(&opts) } - var db storage.Client - var err error - builder := storage.GetClientBuilder() - + builderOpts := []storage.ClientBuilderOpt{ + storage.WithClientConnString(buildConnString(opts)), + storage.WithExtraOptions(opts.extraOptions...), + } // Priority: direct connection settings > instance name // If direct connection settings are provided, use them. - if opts.host != "" { - connString := buildConnString(opts) - db, err = builder( - context.Background(), - storage.WithClientConnString(connString), - storage.WithExtraOptions(opts.extraOptions...), - ) - if err != nil { - return nil, fmt.Errorf("create postgres client from connection settings failed: %w", err) - } - } else if opts.instanceName != "" { + if opts.host == "" && opts.instanceName != "" { // Otherwise, use instance name if provided. - builderOpts, ok := storage.GetPostgresInstance(opts.instanceName) - if !ok { + var ok bool + if builderOpts, ok = storage.GetPostgresInstance(opts.instanceName); !ok { return nil, fmt.Errorf("postgres instance %s not found", opts.instanceName) } - db, err = builder(context.Background(), builderOpts...) - if err != nil { - return nil, fmt.Errorf("create postgres client from instance name failed: %w", err) - } - } else { - return nil, fmt.Errorf("either connection settings (host, port, etc.) or instance name must be provided") } + db, err := storage.GetClientBuilder()(context.Background(), builderOpts...) + if err != nil { + return nil, fmt.Errorf("create postgres client failed: %w", err) + } // Build full table name with schema. fullTableName := sqldb.BuildTableNameWithSchema(opts.schema, "", opts.tableName) diff --git a/memory/postgres/service_test.go b/memory/postgres/service_test.go index 6f7c81d7a..4880c5a26 100644 --- a/memory/postgres/service_test.go +++ b/memory/postgres/service_test.go @@ -1370,7 +1370,7 @@ func TestNewService_ConnectionSettingsBuilderError(t *testing.T) { _, err := NewService(WithHost("localhost"), WithPort(5432), WithDatabase("testdb")) require.Error(t, err) - assert.Contains(t, err.Error(), "create postgres client from connection settings failed") + assert.Contains(t, err.Error(), "create postgres client failed") } func TestNewService_InstanceNameBuilderError(t *testing.T) { @@ -1387,7 +1387,7 @@ func TestNewService_InstanceNameBuilderError(t *testing.T) { _, err := NewService(WithPostgresInstance("test-instance")) require.Error(t, err) - assert.Contains(t, err.Error(), "create postgres client from instance name failed") + assert.Contains(t, err.Error(), "create postgres client failed") } func TestNewService_ConnectionSettingsPriority(t *testing.T) { @@ -1789,13 +1789,6 @@ func TestBuildCreateIndexSQL(t *testing.T) { } } -// Test NewService with missing connection settings -func TestNewService_MissingConnectionSettings(t *testing.T) { - _, err := NewService() - require.Error(t, err) - assert.Contains(t, err.Error(), "either connection settings") -} - // Test NewService with skipDBInit func TestNewService_WithSkipDBInit(t *testing.T) { db, mock := setupMockDB(t) diff --git a/memory/redis/service.go b/memory/redis/service.go index 0a0e5f751..703064a47 100644 --- a/memory/redis/service.go +++ b/memory/redis/service.go @@ -63,41 +63,20 @@ func NewService(options ...ServiceOpt) (*Service, error) { option(&opts) } - builder := storage.GetClientBuilder() - var ( - redisClient redis.UniversalClient - err error - ) + builderOpts := []storage.ClientBuilderOpt{ + storage.WithClientBuilderURL(opts.url), + storage.WithExtraOptions(opts.extraOptions...), + } // if instance name set, and url not set, use instance name to create redis client if opts.url == "" && opts.instanceName != "" { - builderOpts, ok := storage.GetRedisInstance(opts.instanceName) - if !ok { + var ok bool + if builderOpts, ok = storage.GetRedisInstance(opts.instanceName); !ok { return nil, fmt.Errorf("redis instance %s not found", opts.instanceName) } - redisClient, err = builder(builderOpts...) - if err != nil { - return nil, fmt.Errorf("create redis client from instance name failed: %w", err) - } - - // Test connection with Ping to ensure Redis is accessible. - ctx, cancel := context.WithTimeout(context.Background(), defaultConnectionTimeout) - defer cancel() - if err := redisClient.Ping(ctx).Err(); err != nil { - return nil, fmt.Errorf("redis connection test failed: %w", err) - } - - return &Service{ - opts: opts, - redisClient: redisClient, - cachedTools: make(map[string]tool.Tool), - }, nil } - redisClient, err = builder( - storage.WithClientBuilderURL(opts.url), - storage.WithExtraOptions(opts.extraOptions...), - ) + redisClient, err := storage.GetClientBuilder()(builderOpts...) if err != nil { return nil, fmt.Errorf("create redis client from url failed: %w", err) } diff --git a/session/mysql/service.go b/session/mysql/service.go index 88d526d5a..d77f6622d 100644 --- a/session/mysql/service.go +++ b/session/mysql/service.go @@ -102,32 +102,21 @@ func NewService(options ...ServiceOpt) (*Service, error) { } // Create MySQL client - builder := storage.GetClientBuilder() - var mysqlClient storage.Client - var err error - - // Priority: dsn > instanceName - if opts.dsn != "" { - // Method 1: Use DSN directly (recommended) - mysqlClient, err = builder( - storage.WithClientBuilderDSN(opts.dsn), - storage.WithExtraOptions(opts.extraOptions...), - ) - if err != nil { - return nil, fmt.Errorf("create mysql client from dsn failed: %w", err) - } - } else if opts.instanceName != "" { + builderOpts := []storage.ClientBuilderOpt{ + storage.WithClientBuilderDSN(opts.dsn), + storage.WithExtraOptions(opts.extraOptions...), + } + if opts.dsn == "" && opts.instanceName != "" { // Method 2: Use pre-registered MySQL instance - builderOpts, ok := storage.GetMySQLInstance(opts.instanceName) - if !ok { + var ok bool + if builderOpts, ok = storage.GetMySQLInstance(opts.instanceName); !ok { return nil, fmt.Errorf("mysql instance %s not found", opts.instanceName) } - mysqlClient, err = builder(builderOpts...) - if err != nil { - return nil, fmt.Errorf("create mysql client from instance name failed: %w", err) - } - } else { - return nil, fmt.Errorf("either dsn or instance name must be provided") + } + + mysqlClient, err := storage.GetClientBuilder()(builderOpts...) + if err != nil { + return nil, fmt.Errorf("create mysql client failed: %w", err) } // Build table names with prefix diff --git a/session/mysql/service_test.go b/session/mysql/service_test.go index 80051aa2e..24f965f5a 100644 --- a/session/mysql/service_test.go +++ b/session/mysql/service_test.go @@ -1667,7 +1667,7 @@ func TestNewService_MissingDSNAndInstance(t *testing.T) { svc, err := NewService() assert.Error(t, err) assert.Nil(t, svc) - assert.Contains(t, err.Error(), "either dsn or instance name must be provided") + assert.Contains(t, err.Error(), "create mysql client failed") } func TestNewService_WithInstance_Success(t *testing.T) { @@ -1726,7 +1726,7 @@ func TestNewService_ClientBuilderError(t *testing.T) { ) assert.Error(t, err) assert.Nil(t, svc) - assert.Contains(t, err.Error(), "create mysql client from dsn failed") + assert.Contains(t, err.Error(), "create mysql client failed") // Test with instance name storage.RegisterMySQLInstance("test-error-instance", @@ -1738,7 +1738,7 @@ func TestNewService_ClientBuilderError(t *testing.T) { ) assert.Error(t, err) assert.Nil(t, svc) - assert.Contains(t, err.Error(), "create mysql client from instance name failed") + assert.Contains(t, err.Error(), "create mysql client failed") } func TestNewService_DBInitFailure(t *testing.T) { diff --git a/session/postgres/service.go b/session/postgres/service.go index aef84fef6..163f90e90 100644 --- a/session/postgres/service.go +++ b/session/postgres/service.go @@ -158,34 +158,22 @@ func NewService(options ...ServiceOpt) (*Service, error) { } } - var pgClient storage.Client - var err error - builder := storage.GetClientBuilder() - + builderOpts := []storage.ClientBuilderOpt{ + storage.WithClientConnString(buildConnString(opts)), + storage.WithExtraOptions(opts.extraOptions...), + } // Priority: direct connection settings > instance name // If direct connection settings are provided, use them - if opts.host != "" { - connString := buildConnString(opts) - pgClient, err = builder( - context.Background(), - storage.WithClientConnString(connString), - storage.WithExtraOptions(opts.extraOptions...), - ) - if err != nil { - return nil, fmt.Errorf("create postgres client from connection settings failed: %w", err) - } - } else if opts.instanceName != "" { + if opts.host == "" && opts.instanceName != "" { // Otherwise, use instance name if provided - builderOpts, ok := storage.GetPostgresInstance(opts.instanceName) - if !ok { + var ok bool + if builderOpts, ok = storage.GetPostgresInstance(opts.instanceName); !ok { return nil, fmt.Errorf("postgres instance %s not found", opts.instanceName) } - pgClient, err = builder(context.Background(), builderOpts...) - if err != nil { - return nil, fmt.Errorf("create postgres client from instance name failed: %w", err) - } - } else { - return nil, fmt.Errorf("either connection settings (host, port, etc.) or instance name must be provided") + } + pgClient, err := storage.GetClientBuilder()(context.Background(), builderOpts...) + if err != nil { + return nil, fmt.Errorf("create postgres client failed: %w", err) } s := &Service{ From deff8688acbaa29cbce905f6b743129b02433a18 Mon Sep 17 00:00:00 2001 From: nomand-zc Date: Thu, 27 Nov 2025 15:25:49 +0800 Subject: [PATCH 3/6] add unit test --- session/postgres/service_test.go | 37 ++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/session/postgres/service_test.go b/session/postgres/service_test.go index 656a4b707..ddc06f154 100644 --- a/session/postgres/service_test.go +++ b/session/postgres/service_test.go @@ -2138,3 +2138,40 @@ func TestClose_Multiple(t *testing.T) { err = s.Close() require.NoError(t, err) } + +func TestNewService_MissingDSNAndInstance(t *testing.T) { + svc, err := NewService() + assert.Error(t, err) + assert.Nil(t, svc) + assert.Contains(t, err.Error(), "create postgres client failed") +} + +func TestNewService_WithInstance_Success(t *testing.T) { + db, mock, err := sqlmock.New(sqlmock.MonitorPingsOption(true)) + require.NoError(t, err) + defer db.Close() + + originalBuilder := storage.GetClientBuilder() + defer storage.SetClientBuilder(originalBuilder) + + storage.SetClientBuilder(func(ctx context.Context, builderOpts ...storage.ClientBuilderOpt) (storage.Client, error) { + return &mockPostgresClient{db: db}, nil + }) + + // Register instance + instanceName := "test-instance-success" + storage.RegisterPostgresInstance(instanceName, + storage.WithClientConnString("test:test@tcp(localhost:3306)/testdb"), + ) + + svc, err := NewService( + WithPostgresInstance(instanceName), + WithSkipDBInit(true), + ) + require.NoError(t, err) + require.NotNil(t, svc) + + err = svc.Close() + assert.NoError(t, err) + assert.NoError(t, mock.ExpectationsWereMet()) +} From 8ba26bdd3660ffbe66de03a2c7421a3004b3add4 Mon Sep 17 00:00:00 2001 From: nomand-zc Date: Thu, 27 Nov 2025 17:42:47 +0800 Subject: [PATCH 4/6] update comment --- memory/mysql/service.go | 1 - 1 file changed, 1 deletion(-) diff --git a/memory/mysql/service.go b/memory/mysql/service.go index c6d53d176..8c1b19fa9 100644 --- a/memory/mysql/service.go +++ b/memory/mysql/service.go @@ -80,7 +80,6 @@ func NewService(options ...ServiceOpt) (*Service, error) { } } - // Method 1: Use DSN directly (recommended). db, err := storage.GetClientBuilder()(builderOpts...) if err != nil { return nil, fmt.Errorf("create mysql client from dsn failed: %w", err) From 7e04294d3c4385ace2ca4700df76e039f14a0a59 Mon Sep 17 00:00:00 2001 From: nomand-zc Date: Fri, 28 Nov 2025 17:35:42 +0800 Subject: [PATCH 5/6] fixed --- memory/mysql/service.go | 2 +- memory/redis/service.go | 2 +- session/redis/service.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/memory/mysql/service.go b/memory/mysql/service.go index 8c1b19fa9..c5eeef9ca 100644 --- a/memory/mysql/service.go +++ b/memory/mysql/service.go @@ -82,7 +82,7 @@ func NewService(options ...ServiceOpt) (*Service, error) { db, err := storage.GetClientBuilder()(builderOpts...) if err != nil { - return nil, fmt.Errorf("create mysql client from dsn failed: %w", err) + return nil, fmt.Errorf("create mysql client failed: %w", err) } s := &Service{ diff --git a/memory/redis/service.go b/memory/redis/service.go index 703064a47..40c43b3a6 100644 --- a/memory/redis/service.go +++ b/memory/redis/service.go @@ -78,7 +78,7 @@ func NewService(options ...ServiceOpt) (*Service, error) { redisClient, err := storage.GetClientBuilder()(builderOpts...) if err != nil { - return nil, fmt.Errorf("create redis client from url failed: %w", err) + return nil, fmt.Errorf("create redis client failed: %w", err) } // Test connection with Ping to ensure Redis is accessible. diff --git a/session/redis/service.go b/session/redis/service.go index 5691c86ca..56bede70f 100644 --- a/session/redis/service.go +++ b/session/redis/service.go @@ -118,7 +118,7 @@ func NewService(options ...ServiceOpt) (*Service, error) { redisClient, err := storage.GetClientBuilder()(builderOpts...) if err != nil { - return nil, fmt.Errorf("create redis client from url failed: %w", err) + return nil, fmt.Errorf("create redis client failed: %w", err) } s := &Service{ From 1e50458f9f1ab943f3d0d6f49a89d559219fa41c Mon Sep 17 00:00:00 2001 From: nomand-zc Date: Fri, 28 Nov 2025 17:39:15 +0800 Subject: [PATCH 6/6] fix unit test --- session/redis/service_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/session/redis/service_test.go b/session/redis/service_test.go index bf37a7784..ee35444c8 100644 --- a/session/redis/service_test.go +++ b/session/redis/service_test.go @@ -49,7 +49,7 @@ func TestNewService(t *testing.T) { name: "invalid redis URL", options: []ServiceOpt{WithRedisClientURL("invalid://url")}, expectError: true, - errorMsg: "create redis client from url failed", + errorMsg: "create redis client failed", }, { name: "empty options",