fix(ers): lazily register database/sql driver in SQL ERS provider#3543
fix(ers): lazily register database/sql driver in SQL ERS provider#3543jp-ayyappan wants to merge 3 commits into
Conversation
Previously the SQL provider required consumers to add a blank driver import (e.g. _ "github.com/lib/pq") to their own binaries, causing the driver to be registered globally at init time even when the SQL ERS tier was not configured. This change moves driver registration into NewProvider() using pgx/v5/stdlib (already a direct dependency of this module), guarded by a sync.Mutex and a sql.Drivers() pre-check to prevent duplicate-register panics when consumers have already registered the driver themselves. The registration is now lazy: it happens only when a SQL ERS provider is actually instantiated, and only if the driver has not already been registered by the consumer or another code path. Fixes: #3539
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the SQL ERS provider to handle database driver registration internally and lazily. By moving registration from global init() calls to the provider instantiation phase, the module reduces unnecessary dependencies for consumers who do not utilize the SQL ERS tier, while maintaining backward compatibility for those who continue to register drivers manually. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The driver waits in silent sleep, Until the provider starts to creep. No global import needed now, We register with a lazy bow. Footnotes
|
📝 WalkthroughWalkthroughThe SQL provider now auto-registers the PostgreSQL database/sql driver lazily and concurrency-safely: a mutex-backed registry and ensureDriverRegistered normalize/check drivers and register ChangesLazy PostgreSQL Driver Auto-Registration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces lazy driver registration for SQL providers to avoid requiring consumers to manually import database drivers. It adds a helper function ensureDriverRegistered that registers the postgres driver using pgx/v5/stdlib if it is not already registered. Feedback on this change highlights a potential issue where mixed-case driver names (e.g., 'Postgres') can bypass the registration check, leading to a duplicate registration panic or an unknown driver error. Normalizing config.Driver to lowercase before registration is recommended to prevent these issues.
| func NewProvider(ctx context.Context, name string, config Config) (*Provider, error) { | ||
| // Register the database/sql driver for this provider's configured driver name | ||
| // if it has not already been registered. | ||
| ensureDriverRegistered(config.Driver) |
There was a problem hiding this comment.
If config.Driver is provided with mixed casing (e.g., "Postgres" or "POSTGRES"), it can lead to two issues:
- Panic in
ensureDriverRegistered: If"postgres"is already registered externally (e.g., vialib/pqin the consumer binary), the case-sensitive checkd == driver(wheredis"postgres"anddriveris"Postgres") will fail. The function will then proceed to callsql.Register("postgres", ...)which panics because the driver is already registered. - Unknown driver error in
sql.Open:sql.Open(config.Driver, ...)will be called with"Postgres", which will fail because the registered driver name is lowercase"postgres".
Normalizing config.Driver to lowercase at the beginning of NewProvider resolves both issues cleanly.
| func NewProvider(ctx context.Context, name string, config Config) (*Provider, error) { | |
| // Register the database/sql driver for this provider's configured driver name | |
| // if it has not already been registered. | |
| ensureDriverRegistered(config.Driver) | |
| func NewProvider(ctx context.Context, name string, config Config) (*Provider, error) { | |
| config.Driver = strings.ToLower(config.Driver) | |
| // Register the database/sql driver for this provider's configured driver name | |
| // if it has not already been registered. | |
| ensureDriverRegistered(config.Driver) |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@service/entityresolution/multi-strategy/providers/sql/sql_provider.go`:
- Around line 32-48: The ensureDriverRegistered function should normalize the
driver name (e.g., via strings.ToLower) before any checks and keying to avoid
duplicate registration panics: use a lowercasedDriver variable for lookups into
registeredDrivers and comparisons against sql.Drivers(), store the normalized
key into registeredDrivers, and use the normalized value in the switch so
sql.Register("postgres", ...) is only called when no existing "postgres" driver
was found; update references to registeredDrivers, sql.Drivers(), and the switch
in ensureDriverRegistered accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 065a65f0-ebb5-452a-9d63-c23933b085c3
📒 Files selected for processing (1)
service/entityresolution/multi-strategy/providers/sql/sql_provider.go
Both the registeredDrivers map lookup and the sql.Drivers() pre-check were case-sensitive, while the switch used strings.ToLower. A consumer passing driver: Postgres (capital P) would bypass the pre-check and trigger a duplicate sql.Register panic. Normalize to strings.ToLower(strings.TrimSpace(driver)) at the top of ensureDriverRegistered and use strings.EqualFold for the sql.Drivers() comparison so all case variants map to a single canonical key. Addresses review feedback from gemini-code-assist and coderabbitai.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@service/entityresolution/multi-strategy/providers/sql/sql_provider.go`:
- Around line 29-31: ensureDriverRegistered lowercases the local driver but
NewProvider still calls sql.Open(config.Driver,...), causing unknown driver
errors; change the flow so the normalized driver is used for opening the DB —
either have ensureDriverRegistered return the normalized driver (or set
config.Driver = normalized inside it) and then call sql.Open(normalizedDriver,
cfg.ConnectionString) in NewProvider (references: ensureDriverRegistered,
NewProvider, sql.Open, config.Driver).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76124089-096c-473f-bda9-db8191e3b3cd
📒 Files selected for processing (1)
service/entityresolution/multi-strategy/providers/sql/sql_provider.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
ensureDriverRegistered already normalizes its input to lowercase, but NewProvider was still passing the raw config.Driver to sql.Open. A mixed-case driver (e.g. 'Postgres') would pass through ensureDriverRegistered correctly but then fail sql.Open with 'unknown driver' since database/sql matches driver names by exact string. Normalize config.Driver at the top of NewProvider so both ensureDriverRegistered and sql.Open receive the canonical lowercase name. Addresses review feedback from coderabbitai and gemini-code-assist.
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
1 participant
Summary
Fixes #3539.
The multi-strategy ERS SQL provider called
sql.Open(config.Driver, ...)but relied on consumers to register the named driver via a blank import (e.g._ "github.com/lib/pq"). This forced every consumer — even those not using the SQL ERS tier — to register the driver globally atinit()time.Changes
Lazy driver registration in
NewProvider()viaensureDriverRegistered():config.Driverto lowercase + trimmed at the top ofNewProvider— ensures bothensureDriverRegisteredandsql.Openreceive the canonical lowercase name, preventing mixed-case inputs (e.g.,"Postgres") from bypassing the pre-check or failingsql.Openwith "unknown driver"registeredDriversmap (fast path for repeated calls)sql.Drivers()withstrings.EqualFold— gracefully handles drivers already registered by the consumer binary"postgres"viapgx/v5/stdlib.GetDefaultDriver()on first use — no new dependency (pgx/v5is already a direct dependency of this module)Uses a
sync.Mutexguard so concurrentNewProvider()calls are safe.Behavior
"postgres"not yet registeredregisteredDrivers)"postgres"(blank import)sql.Drivers()pre-check)driver: Postgres(mixed case)"postgres"inNewProvider— no panic, no "unknown driver" errordriver: mysqlTesting
go test ./entityresolution/multi-strategy/providers/sql/...)go build ./entityresolution/multi-strategy/...)Notes
mysqlandsqlitedrivers are documented as requiring consumer-side imports until their dependencies are added to this module'sgo.mod_ "github.com/lib/pq"continue to work correctly — thesql.Drivers()pre-check detects the existing registration and skipssql.Register