feat: custom converter interface for database/sql query parameters #1967
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive custom converter system for the database/sql driver, allowing users to register custom converters for domain-specific types without modifying the SDK core.
Key Changes:
- Added
ConverterandNamedValueConverterinterfaces for custom parameter conversion - Implemented global converter registries with ordered execution
- Provided built-in converters for JSON and UUID types
- Added
WithCustomConverter()andWithCustomNamedValueConverter()configuration options
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/bind/converter.go | Core converter implementation with registry system and built-in JSON/UUID converters |
| internal/bind/converter_test.go | Comprehensive unit tests for converter functionality |
| internal/bind/params.go | Integration of custom converters into parameter conversion pipeline |
| internal/bind/bind.go | Marker types for enabling custom converters in binding system |
| sql.go | Public API functions for registering custom converters with connectors |
| bind/converter.go | Public API wrapper exposing converter interfaces and registration functions |
| bind/converter_test.go | Public API tests for converter registration and usage |
| tests/integration/database_sql_custom_converter_test.go | End-to-end integration tests for custom converter functionality |
| examples/custom_converter/main.go | Example demonstrating usage of custom converters |
| CHANGELOG.md | Documentation of the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // RegisterConverter registers a custom converter with the default registry | ||
| // | ||
| // Custom converters are tried before the standard conversion logic, allowing | ||
| // you to override or extend the default behavior for specific types. | ||
| // | ||
| // Example: | ||
| // | ||
| // bind.RegisterConverter(&MyCustomConverter{}) | ||
| func RegisterConverter(converter Converter) { | ||
| bind.RegisterConverter(converter) | ||
| } |
There was a problem hiding this comment.
The documentation should mention that RegisterConverter registers converters globally and that registrations persist for the lifetime of the application. This is important for users to understand potential side effects, especially in testing scenarios where converters may need to be cleaned up.
| require.NoError(t, err) | ||
|
|
||
| // Create connector with custom converters | ||
| connector, err := ydb.Connector( | ||
| &ydb.Driver{}, | ||
| ydb.WithCustomConverter(timeConverter), | ||
| ydb.WithCustomConverter(idConverter), | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| // Replace db connection with custom connector | ||
| db = sql.OpenDB(connector) | ||
| defer func() { |
There was a problem hiding this comment.
The database connection opened with sql.Open on line 51 is immediately replaced by sql.OpenDB(connector) on line 63 without being closed, causing a resource leak. Either remove the initial sql.Open call or close the first connection before replacing it.
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| db = sql.OpenDB(connector) |
There was a problem hiding this comment.
The database connection opened with sql.Open on line 126 is immediately replaced by sql.OpenDB(connector) on line 139 without being closed, causing a resource leak. Either remove the initial sql.Open call or close the first connection before replacing it.
| db = sql.OpenDB(connector) | |
| _ = db.Close() // Close the first db before reassigning |
|
|
||
| func (c *UUIDConverter) Convert(v any) (value.Value, bool) { | ||
| uuidType := reflect.TypeOf(uuid.UUID{}) | ||
| uuidPtrType := reflect.TypeOf((*uuid.UUID)(nil)) | ||
|
|
||
| switch reflect.TypeOf(v) { | ||
| case uuidType: | ||
| vv, ok := v.(uuid.UUID) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
|
|
||
| return value.Uuid(vv), true | ||
| case uuidPtrType: | ||
| vv, ok := v.(*uuid.UUID) | ||
| if !ok { | ||
| return nil, false | ||
| } |
There was a problem hiding this comment.
The Convert method will panic if v is nil because reflect.TypeOf(nil) returns nil, and then the switch statement tries to compare it. Add a nil check at the beginning: if v == nil { return nil, false }
| "time" | ||
|
|
||
| "github.com/ydb-platform/ydb-go-sdk/v3" | ||
| "github.com/ydb-platform/ydb-go-sdk/v3/bind" |
There was a problem hiding this comment.
Missing import for the value package. The code uses value.Value, value.TextValue(), and value.Int64Value() but doesn't import "github.com/ydb-platform/ydb-go-sdk/v3/internal/value".
| "github.com/ydb-platform/ydb-go-sdk/v3/bind" | |
| "github.com/ydb-platform/ydb-go-sdk/v3/bind" | |
| "github.com/ydb-platform/ydb-go-sdk/v3/internal/value" |
| require.NoError(t, err) | ||
| defer func() { | ||
| _ = db.Close() | ||
| }() | ||
|
|
||
| connector, err := ydb.Connector(&ydb.Driver{}) | ||
| require.NoError(t, err) | ||
|
|
||
| db = sql.OpenDB(connector) | ||
| defer func() { |
There was a problem hiding this comment.
The database connection opened with sql.Open on line 249 is immediately replaced by sql.OpenDB(connector) on line 258 without being closed, causing a resource leak. Either remove the initial sql.Open call or close the first connection before replacing it.
| require.NoError(t, err) | ||
| defer func() { | ||
| _ = db.Close() | ||
| }() | ||
|
|
||
| connector, err := ydb.Connector(&ydb.Driver{}) | ||
| require.NoError(t, err) | ||
|
|
||
| db = sql.OpenDB(connector) | ||
| defer func() { |
There was a problem hiding this comment.
The database connection opened with sql.Open on line 304 is immediately replaced by sql.OpenDB(connector) on line 313 without being closed, causing a resource leak. Either remove the initial sql.Open call or close the first connection before replacing it.
|
|
||
| // Register adds a named value converter to the registry | ||
| func (r *NamedValueConverterRegistry) Register(converter NamedValueConverter) { | ||
| r.converters = append(r.converters, converter) |
There was a problem hiding this comment.
The Register method modifies the converters slice without synchronization. Since DefaultNamedValueConverterRegistry is a global variable that can be accessed concurrently (e.g., via RegisterNamedValueConverter and Convert), this creates a potential race condition. Consider adding a mutex to protect concurrent access to the converters slice, or document that registration should only happen during initialization.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
asmyasnikov
force-pushed
the
feat/custom-converter-interface
branch
from
8644886 to
ac26a81
Compare
2 participants
feat: custom converter interface for database/sql query parameters
Pull request type
What is the current behavior?
Currently, the ydb-go-sdk database/sql driver only supports standard Go types for parameter conversion. Users with custom domain types or special conversion requirements must manually convert values before passing them to the database driver, leading to boilerplate code and potential inconsistencies.
Issue Number: #433
What is the new behavior?
This PR introduces a comprehensive custom converter system that allows users to:
Key Features Added:
Converter Interfaces:
bind.Converter- Basic value conversion interfacebind.NamedValueConverter- Extended interface with parameter name accessRegistry System:
Built-in Converters:
JSONConverter- Handlesjson.MarshalertypesUUIDConverter- Handlesgoogle/uuid.UUIDtypesCustomTypeConverter- Generic configurable converterDatabase/SQL Integration:
ydb.WithCustomConverter()- Register converters with connectorsydb.WithCustomNamedValueConverter()- Register named value converters