From 253f8012f7ee0d1058f2c098856d3a4115ba96c3 Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Wed, 23 Apr 2025 13:29:02 +0300 Subject: [PATCH 1/8] * Added validation of WithTxControl option in non-interactive methods of Client and Session --- CHANGELOG.md | 2 ++ internal/query/client.go | 35 +++++++++++++++++++++++++++++++++++ internal/query/session.go | 24 ++++++++++++++++++++++-- 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 100fabf2d..55df5e7f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,5 @@ +* Added validation of WithTxControl option in non-interactive methods of Client and Session + ## v3.108.0 * Added `query.EmptyTxControl()` for empty transaction control (server-side defines transaction control by internal logic) * Marked as deprecated `query.NoTx()` because this is wrong name for server-side transaction control inference diff --git a/internal/query/client.go b/internal/query/client.go index 2dc0ad0fc..e71b1cdc7 100644 --- a/internal/query/client.go +++ b/internal/query/client.go @@ -2,6 +2,7 @@ package query import ( "context" + "errors" "time" "github.com/ydb-platform/ydb-go-genproto/Ydb_Query_V1" @@ -32,6 +33,10 @@ var ( _ sessionPool = (*pool.Pool[*Session, Session])(nil) ) +var ( + errNoCommit = xerrors.Wrap(errors.New("WithTxControl option is not allowed without CommitTx() option in Client methods, as these methods are non-interactive. You can either add the CommitTx() option to TxControl or use query.*TxControl methods (e.g., query.SnapshotReadOnlyTxControl) which already include the commit flag")) +) + type ( sessionPool interface { closer.Closer @@ -173,6 +178,10 @@ func (c *Client) ExecuteScript( ), } + if err := checkTxControlWithCommit(settings.TxControl()); err != nil { + return nil, err + } + request, grpcOpts, err := executeQueryScriptRequest(q, settings) if err != nil { return op, xerrors.WithStackTrace(err) @@ -320,6 +329,10 @@ func (c *Client) QueryRow(ctx context.Context, q string, opts ...options.Execute settings := options.ExecuteSettings(opts...) + if err := checkTxControlWithCommit(settings.TxControl()); err != nil { + return nil, err + } + onDone := trace.QueryOnQueryRow(c.config.Trace(), &ctx, stack.FunctionID("github.com/ydb-platform/ydb-go-sdk/v3/internal/query.(*Client).QueryRow"), q, settings.Label(), @@ -366,6 +379,11 @@ func (c *Client) Exec(ctx context.Context, q string, opts ...options.Execute) (f defer cancel() settings := options.ExecuteSettings(opts...) + + if err := checkTxControlWithCommit(settings.TxControl()); err != nil { + return err + } + onDone := trace.QueryOnExec(c.config.Trace(), &ctx, stack.FunctionID("github.com/ydb-platform/ydb-go-sdk/v3/internal/query.(*Client).Exec"), q, @@ -415,6 +433,11 @@ func (c *Client) Query(ctx context.Context, q string, opts ...options.Execute) ( defer cancel() settings := options.ExecuteSettings(opts...) + + if err := checkTxControlWithCommit(settings.TxControl()); err != nil { + return nil, err + } + onDone := trace.QueryOnQuery(c.config.Trace(), &ctx, stack.FunctionID("github.com/ydb-platform/ydb-go-sdk/v3/internal/query.(*Client).Query"), q, settings.Label(), @@ -470,6 +493,10 @@ func (c *Client) QueryResultSet( err error ) + if err := checkTxControlWithCommit(settings.TxControl()); err != nil { + return nil, err + } + onDone := trace.QueryOnQueryResultSet(c.config.Trace(), &ctx, stack.FunctionID("github.com/ydb-platform/ydb-go-sdk/v3/internal/query.(*Client).QueryResultSet"), q, settings.Label(), @@ -612,6 +639,14 @@ func New(ctx context.Context, cc grpc.ClientConnInterface, cfg *config.Config) * } } +// checkTxControlWithCommit checks that if WithTxControl is used, it must be with WithCommit +func checkTxControlWithCommit(txControl options.TxControl) error { + if txControl != nil && !txControl.Commit() { + return xerrors.WithStackTrace(errNoCommit) + } + return nil +} + func poolTrace(t *trace.Query) *pool.Trace { return &pool.Trace{ OnNew: func(ctx *context.Context, call stack.Caller) func(limit int) { diff --git a/internal/query/session.go b/internal/query/session.go index dce1cc130..87f8e070d 100644 --- a/internal/query/session.go +++ b/internal/query/session.go @@ -36,7 +36,12 @@ func (s *Session) QueryResultSet( onDone(finalErr) }() - r, err := s.execute(ctx, q, options.ExecuteSettings(opts...), withTrace(s.trace)) + settings := options.ExecuteSettings(opts...) + if err := checkTxControlWithCommit(settings.TxControl()); err != nil { + return nil, err + } + + r, err := s.execute(ctx, q, settings, withTrace(s.trace)) if err != nil { return nil, xerrors.WithStackTrace(err) } @@ -75,7 +80,12 @@ func (s *Session) QueryRow(ctx context.Context, q string, opts ...options.Execut onDone(finalErr) }() - row, err := s.queryRow(ctx, q, options.ExecuteSettings(opts...), withTrace(s.trace)) + settings := options.ExecuteSettings(opts...) + if err := checkTxControlWithCommit(settings.TxControl()); err != nil { + return nil, err + } + + row, err := s.queryRow(ctx, q, settings, withTrace(s.trace)) if err != nil { return nil, xerrors.WithStackTrace(err) } @@ -154,6 +164,11 @@ func (s *Session) execute( func (s *Session) Exec(ctx context.Context, q string, opts ...options.Execute) (finalErr error) { settings := options.ExecuteSettings(opts...) + + if err := checkTxControlWithCommit(settings.TxControl()); err != nil { + return err + } + onDone := trace.QueryOnSessionExec(s.trace, &ctx, stack.FunctionID("github.com/ydb-platform/ydb-go-sdk/v3/internal/query.(*Session).Exec"), s, @@ -182,6 +197,11 @@ func (s *Session) Exec(ctx context.Context, q string, opts ...options.Execute) ( func (s *Session) Query(ctx context.Context, q string, opts ...options.Execute) (_ query.Result, finalErr error) { settings := options.ExecuteSettings(opts...) + + if err := checkTxControlWithCommit(settings.TxControl()); err != nil { + return nil, err + } + onDone := trace.QueryOnSessionQuery(s.trace, &ctx, stack.FunctionID("github.com/ydb-platform/ydb-go-sdk/v3/internal/query.(*Session).Query"), s, From 813e20e965e0bbdc7ccf917160b28d776bfeb6e8 Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Thu, 24 Apr 2025 11:33:19 +0300 Subject: [PATCH 2/8] Better detect nil value under interface --- internal/query/client.go | 4 +- internal/xreflect/is_nil.go | 31 +++++++++++ internal/xreflect/is_nil_test.go | 93 ++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 internal/xreflect/is_nil.go create mode 100644 internal/xreflect/is_nil_test.go diff --git a/internal/query/client.go b/internal/query/client.go index e71b1cdc7..86523fb31 100644 --- a/internal/query/client.go +++ b/internal/query/client.go @@ -21,6 +21,7 @@ import ( "github.com/ydb-platform/ydb-go-sdk/v3/internal/types" "github.com/ydb-platform/ydb-go-sdk/v3/internal/xcontext" "github.com/ydb-platform/ydb-go-sdk/v3/internal/xerrors" + "github.com/ydb-platform/ydb-go-sdk/v3/internal/xreflect" "github.com/ydb-platform/ydb-go-sdk/v3/query" "github.com/ydb-platform/ydb-go-sdk/v3/retry" "github.com/ydb-platform/ydb-go-sdk/v3/trace" @@ -641,9 +642,10 @@ func New(ctx context.Context, cc grpc.ClientConnInterface, cfg *config.Config) * // checkTxControlWithCommit checks that if WithTxControl is used, it must be with WithCommit func checkTxControlWithCommit(txControl options.TxControl) error { - if txControl != nil && !txControl.Commit() { + if !xreflect.IsContainsNilPointer(txControl) && !txControl.Commit() { return xerrors.WithStackTrace(errNoCommit) } + return nil } diff --git a/internal/xreflect/is_nil.go b/internal/xreflect/is_nil.go new file mode 100644 index 000000000..cce594d6b --- /dev/null +++ b/internal/xreflect/is_nil.go @@ -0,0 +1,31 @@ +package xreflect + +import "reflect" + +func IsContainsNilPointer(v any) bool { + if v == nil { + return true + } + + rVal := reflect.ValueOf(v) + + return isValPointToNil(rVal) +} + +func isValPointToNil(v reflect.Value) bool { + kind := v.Kind() + var res bool + switch kind { + case reflect.Slice: + return false + case reflect.Chan, reflect.Func, reflect.Map, reflect.UnsafePointer: + res = v.IsNil() + case reflect.Pointer, reflect.Interface: + elem := v.Elem() + if v.IsNil() { + return true + } + res = isValPointToNil(elem) + } + return res +} diff --git a/internal/xreflect/is_nil_test.go b/internal/xreflect/is_nil_test.go new file mode 100644 index 000000000..545c88c87 --- /dev/null +++ b/internal/xreflect/is_nil_test.go @@ -0,0 +1,93 @@ +package xreflect + +import ( + "testing" +) + +func TestIsContainsNilPointer(t *testing.T) { + var nilIntPointer *int + var vInterface any + vInterface = nilIntPointer + + // Test cases for different nil and non-nil scenarios + tests := []struct { + name string + input any + expected bool + }{ + { + name: "nil interface", + input: nil, + expected: true, + }, + { + name: "nil pointer to int", + input: (*int)(nil), + expected: true, + }, + { + name: "non-nil pointer to int", + input: new(int), + expected: false, + }, + { + name: "nil slice", + input: []int(nil), + expected: false, + }, + { + name: "empty slice", + input: []int{}, + expected: false, + }, + { + name: "nil map", + input: map[string]int(nil), + expected: true, + }, + { + name: "empty map", + input: map[string]int{}, + expected: false, + }, + { + name: "nil channel", + input: (chan int)(nil), + expected: true, + }, + { + name: "non-nil channel", + input: make(chan int), + expected: false, + }, + { + name: "nil function", + input: (func())(nil), + expected: true, + }, + { + name: "nested nil pointer", + input: &nilIntPointer, + expected: true, + }, + { + name: "interface with stored nil pointer", + input: vInterface, + expected: true, + }, + { + name: "non-nil interface value", + input: interface{}("test"), + expected: false, + }, + } + + // Execute all test cases + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsContainsNilPointer(tt.input); got != tt.expected { + t.Errorf("IsContainsNilPointer() = %v, want %v", got, tt.expected) + } + }) + } +} From 0b6e2a34dedd7e0e6b27ca0f8bd7a1e4060eb9bc Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Thu, 24 Apr 2025 11:56:49 +0300 Subject: [PATCH 3/8] Fix tx control in tests --- tests/integration/query_regression_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/integration/query_regression_test.go b/tests/integration/query_regression_test.go index 9df274339..c32901f0f 100644 --- a/tests/integration/query_regression_test.go +++ b/tests/integration/query_regression_test.go @@ -45,7 +45,7 @@ DECLARE $val AS UUID; SELECT CAST($val AS Utf8)`, query.WithIdempotent(), query.WithParameters(ydb.ParamsBuilder().Param("$val").UUIDWithIssue1501Value(id).Build()), - query.WithTxControl(tx.SerializableReadWriteTxControl()), + query.WithTxControl(tx.SnapshotReadOnlyTxControl()), ) require.NoError(t, err) @@ -71,7 +71,7 @@ DECLARE $val AS Text; SELECT CAST($val AS UUID)`, query.WithIdempotent(), query.WithParameters(ydb.ParamsBuilder().Param("$val").Text(idString).Build()), - query.WithTxControl(tx.SerializableReadWriteTxControl()), + query.WithTxControl(tx.SnapshotReadOnlyTxControl()), ) require.NoError(t, err) @@ -97,7 +97,7 @@ DECLARE $val AS Text; SELECT CAST($val AS UUID)`, query.WithIdempotent(), query.WithParameters(ydb.ParamsBuilder().Param("$val").Text(idString).Build()), - query.WithTxControl(tx.SerializableReadWriteTxControl()), + query.WithTxControl(tx.SnapshotReadOnlyTxControl()), ) require.NoError(t, err) @@ -125,7 +125,7 @@ DECLARE $val AS Text; SELECT CAST($val AS UUID)`, query.WithIdempotent(), query.WithParameters(ydb.ParamsBuilder().Param("$val").Text(idString).Build()), - query.WithTxControl(tx.SerializableReadWriteTxControl()), + query.WithTxControl(tx.SnapshotReadOnlyTxControl()), ) require.NoError(t, err) @@ -151,7 +151,7 @@ DECLARE $val AS Text; SELECT CAST($val AS UUID)`, query.WithIdempotent(), query.WithParameters(ydb.ParamsBuilder().Param("$val").Text(idString).Build()), - query.WithTxControl(tx.SerializableReadWriteTxControl()), + query.WithTxControl(tx.SnapshotReadOnlyTxControl()), ) require.NoError(t, err) @@ -180,7 +180,7 @@ DECLARE $val AS UUID; SELECT $val`, query.WithIdempotent(), query.WithParameters(ydb.ParamsBuilder().Param("$val").UUIDWithIssue1501Value(id).Build()), - query.WithTxControl(tx.SerializableReadWriteTxControl()), + query.WithTxControl(tx.SnapshotReadOnlyTxControl()), ) require.NoError(t, err) @@ -207,7 +207,7 @@ DECLARE $val AS UUID; SELECT CAST($val AS Utf8)`, query.WithIdempotent(), - query.WithTxControl(query.SerializableReadWriteTxControl()), + query.WithTxControl(query.SnapshotReadOnlyTxControl()), query.WithParameters(ydb.ParamsBuilder().Param("$val").Uuid(id).Build()), ) @@ -233,7 +233,7 @@ DECLARE $val AS Utf8; SELECT CAST($val AS UUID)`, query.WithIdempotent(), query.WithParameters(ydb.ParamsBuilder().Param("$val").Text(idString).Build()), - query.WithTxControl(query.SerializableReadWriteTxControl()), + query.WithTxControl(query.SnapshotReadOnlyTxControl()), ) require.NoError(t, err) @@ -261,7 +261,7 @@ DECLARE $val AS UUID; SELECT $val`, query.WithIdempotent(), query.WithParameters(ydb.ParamsBuilder().Param("$val").Uuid(id).Build()), - query.WithTxControl(query.SerializableReadWriteTxControl()), + query.WithTxControl(query.SnapshotReadOnlyTxControl()), ) require.NoError(t, err) From da31bbae15f087bde24c09c4af09fe55cb9bd76f Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Thu, 24 Apr 2025 17:11:17 +0300 Subject: [PATCH 4/8] Fix tx control in tests --- examples/basic/native/query/series.go | 2 +- internal/query/client.go | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/examples/basic/native/query/series.go b/examples/basic/native/query/series.go index 40506f6f6..27fe774a4 100644 --- a/examples/basic/native/query/series.go +++ b/examples/basic/native/query/series.go @@ -25,7 +25,7 @@ func read(ctx context.Context, c query.Client, prefix string) error { FROM %s `, "`"+path.Join(prefix, "series")+"`"), - query.WithTxControl(query.TxControl(query.BeginTx(query.WithSnapshotReadOnly()))), + query.WithTxControl(query.SnapshotReadOnlyTxControl()), ) if err != nil { return err diff --git a/internal/query/client.go b/internal/query/client.go index 86523fb31..a407235e4 100644 --- a/internal/query/client.go +++ b/internal/query/client.go @@ -34,9 +34,7 @@ var ( _ sessionPool = (*pool.Pool[*Session, Session])(nil) ) -var ( - errNoCommit = xerrors.Wrap(errors.New("WithTxControl option is not allowed without CommitTx() option in Client methods, as these methods are non-interactive. You can either add the CommitTx() option to TxControl or use query.*TxControl methods (e.g., query.SnapshotReadOnlyTxControl) which already include the commit flag")) -) +var errNoCommit = xerrors.Wrap(errors.New("WithTxControl option is not allowed without CommitTx() option in Client methods, as these methods are non-interactive. You can either add the CommitTx() option to TxControl or use query.*TxControl methods (e.g., query.SnapshotReadOnlyTxControl) which already include the commit flag")) type ( sessionPool interface { From 8d2094ec5cd74908ad68867d91a22e4ee6f7e4c5 Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Thu, 24 Apr 2025 17:15:39 +0300 Subject: [PATCH 5/8] fix linter --- internal/query/client.go | 2 +- internal/xreflect/is_nil.go | 1 + internal/xreflect/is_nil_test.go | 3 +-- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/query/client.go b/internal/query/client.go index a407235e4..283f76304 100644 --- a/internal/query/client.go +++ b/internal/query/client.go @@ -34,7 +34,7 @@ var ( _ sessionPool = (*pool.Pool[*Session, Session])(nil) ) -var errNoCommit = xerrors.Wrap(errors.New("WithTxControl option is not allowed without CommitTx() option in Client methods, as these methods are non-interactive. You can either add the CommitTx() option to TxControl or use query.*TxControl methods (e.g., query.SnapshotReadOnlyTxControl) which already include the commit flag")) +var errNoCommit = xerrors.Wrap(errors.New("WithTxControl option is not allowed without CommitTx() option in Client methods, as these methods are non-interactive. You can either add the CommitTx() option to TxControl or use query.*TxControl methods (e.g., query.SnapshotReadOnlyTxControl) which already include the commit flag")) //nolint:lll type ( sessionPool interface { diff --git a/internal/xreflect/is_nil.go b/internal/xreflect/is_nil.go index cce594d6b..6b8634fe0 100644 --- a/internal/xreflect/is_nil.go +++ b/internal/xreflect/is_nil.go @@ -27,5 +27,6 @@ func isValPointToNil(v reflect.Value) bool { } res = isValPointToNil(elem) } + return res } diff --git a/internal/xreflect/is_nil_test.go b/internal/xreflect/is_nil_test.go index 545c88c87..a78c2c43a 100644 --- a/internal/xreflect/is_nil_test.go +++ b/internal/xreflect/is_nil_test.go @@ -6,8 +6,7 @@ import ( func TestIsContainsNilPointer(t *testing.T) { var nilIntPointer *int - var vInterface any - vInterface = nilIntPointer + var vInterface = nilIntPointer // Test cases for different nil and non-nil scenarios tests := []struct { From 57df8ae8cd26c8bb60087833be1cf10d2b72621f Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Sat, 3 May 2025 13:12:15 +0300 Subject: [PATCH 6/8] fix linter --- internal/xreflect/is_nil_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/xreflect/is_nil_test.go b/internal/xreflect/is_nil_test.go index a78c2c43a..6f3713f42 100644 --- a/internal/xreflect/is_nil_test.go +++ b/internal/xreflect/is_nil_test.go @@ -6,7 +6,7 @@ import ( func TestIsContainsNilPointer(t *testing.T) { var nilIntPointer *int - var vInterface = nilIntPointer + vInterface := nilIntPointer // Test cases for different nil and non-nil scenarios tests := []struct { From 0c1d3ab14a6f94459c58d5e3f2c9a75b28e9c6d8 Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Sat, 3 May 2025 13:41:15 +0300 Subject: [PATCH 7/8] fix controltx --- internal/query/client.go | 2 +- tests/integration/database_sql_with_tx_control_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/query/client.go b/internal/query/client.go index 283f76304..84b30eb9e 100644 --- a/internal/query/client.go +++ b/internal/query/client.go @@ -638,7 +638,7 @@ func New(ctx context.Context, cc grpc.ClientConnInterface, cfg *config.Config) * } } -// checkTxControlWithCommit checks that if WithTxControl is used, it must be with WithCommit +// checkTxControlWithCommit validates the transaction control object to ensure it includes a commit flag. func checkTxControlWithCommit(txControl options.TxControl) error { if !xreflect.IsContainsNilPointer(txControl) && !txControl.Commit() { return xerrors.WithStackTrace(errNoCommit) diff --git a/tests/integration/database_sql_with_tx_control_test.go b/tests/integration/database_sql_with_tx_control_test.go index 7ad16b394..f0949f6b8 100644 --- a/tests/integration/database_sql_with_tx_control_test.go +++ b/tests/integration/database_sql_with_tx_control_test.go @@ -38,9 +38,9 @@ func TestDatabaseSqlWithTxControl(t *testing.T) { ydb.WithTxControl( tx.WithTxControlHook(ctx, func(txControl *tx.Control) { hookCalled = true - require.Equal(t, tx.SerializableReadWriteTxControl(), txControl) + require.Equal(t, tx.SerializableReadWriteTxControl(tx.CommitTx()), txControl) }), - tx.SerializableReadWriteTxControl(), + tx.SerializableReadWriteTxControl(tx.CommitTx()), ), db, func(ctx context.Context, cc *sql.Conn) error { _, err := db.QueryContext(ctx, "SELECT 1") @@ -56,9 +56,9 @@ func TestDatabaseSqlWithTxControl(t *testing.T) { ydb.WithTxControl( tx.WithTxControlHook(ctx, func(txControl *tx.Control) { hookCalled = true - require.Equal(t, tx.SerializableReadWriteTxControl(), txControl) + require.Equal(t, tx.SerializableReadWriteTxControl(tx.CommitTx()), txControl) }), - tx.SerializableReadWriteTxControl(), + tx.SerializableReadWriteTxControl(tx.CommitTx()), ), db, func(ctx context.Context, cc *sql.Conn) error { _, err := db.QueryContext(ctx, "SELECT 1") From 50f36af20a5da70ce92cc1fede58e6ec68780a3a Mon Sep 17 00:00:00 2001 From: Timofey Koolin Date: Mon, 5 May 2025 17:56:06 +0300 Subject: [PATCH 8/8] Update CHANGELOG.md --- CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 439c06f44..8e5285c4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,5 @@ -* Added validation of WithTxControl option in non-interactive methods of Client and Session +* Added validation for the WithTxControl option in the non-interactive methods of Client and Session in the query service. -======= ## v3.108.1 * Supported `json.Marshaller` query parameter in `database/sql` driver