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 7 commits into
Conversation
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
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe SQL provider adds a module-level PostgreSQL driver constant, a mutex-protected lazy driver registration helper, and calls that helper from NewProvider to auto-register PostgreSQL via pgx stdlib before opening the DB; MySQL/SQLite still require consumer imports. ChangesLazy PostgreSQL Driver Auto-Registration
sequenceDiagram
participant NewProvider
participant ensureDriverRegistered
participant sql.Drivers
participant pgx_stdlib
NewProvider->>ensureDriverRegistered: call with normalized driver name
ensureDriverRegistered->>sql.Drivers: list existing drivers
ensureDriverRegistered-->>pgx_stdlib: register defaultPostgreSQLDriver when missing
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 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.
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
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:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
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
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.
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.
Signed-off-by: jp-ayyappan <jp@as2max.com>
jp-ayyappan
force-pushed
the
fix/ers-sql-provider-register-postgres-driver
branch
from
0eed6d9 to
aa544af
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
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 43-46: The pre-check in ensureDriverRegistered incorrectly uses
case-insensitive comparison and can treat a mixed-case registered driver (e.g.,
"Postgres") as satisfying the canonical lowercase name, causing us to skip
registering "postgres"; update the check to compare canonical lowercase names
instead (e.g., use if strings.ToLower(d) == driver) so we only treat an existing
driver as registered when its lowercase form equals the canonical driver
variable, and apply the same change to the other occurrence referenced in the
file; refer to ensureDriverRegistered, registeredDrivers and the sql.Drivers()
loop when making the change.
🪄 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: 7cc5b123-3f36-4c03-9bb4-06317f2d3fe1
📒 Files selected for processing (2)
service/entityresolution/multi-strategy/providers/sql/sql_config.goservice/entityresolution/multi-strategy/providers/sql/sql_provider.go
Signed-off-by: jp-ayyappan <jp@as2max.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
…ider.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/entityresolution/multi-strategy/providers/sql/sql_provider.go (1)
68-77:⚠️ Potential issue | 🔴 CriticalFix duplicate
NewProviderinsql_provider.go(blocks compilation).
service/entityresolution/multi-strategy/providers/sql/sql_provider.godefinesNewProvidertwice (at lines 68 and 73), causing a Go syntax error. The first version normalizes withstrings.TrimSpace, while the second does not.Suggested fix
// NewProvider creates a new SQL provider func NewProvider(ctx context.Context, name string, config Config) (*Provider, error) { // Normalize the driver name so "Postgres", "POSTGRES", and "postgres" all // resolve correctly through ensureDriverRegistered and sql.Open, both of // which use case-sensitive driver name matching. config.Driver = strings.ToLower(strings.TrimSpace(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)🤖 Prompt for 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. In `@service/entityresolution/multi-strategy/providers/sql/sql_provider.go` around lines 68 - 77, There are two duplicate NewProvider function definitions causing a compile error; remove the duplicate and keep a single NewProvider implementation that normalizes config.Driver with strings.TrimSpace and strings.ToLower, then calls ensureDriverRegistered(config.Driver) and proceeds as before; specifically, consolidate into one NewProvider (retain the strings.TrimSpace usage on config.Driver and the ensureDriverRegistered call) so only one function named NewProvider exists in sql_provider.go.
🤖 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.
Outside diff comments:
In `@service/entityresolution/multi-strategy/providers/sql/sql_provider.go`:
- Around line 68-77: There are two duplicate NewProvider function definitions
causing a compile error; remove the duplicate and keep a single NewProvider
implementation that normalizes config.Driver with strings.TrimSpace and
strings.ToLower, then calls ensureDriverRegistered(config.Driver) and proceeds
as before; specifically, consolidate into one NewProvider (retain the
strings.TrimSpace usage on config.Driver and the ensureDriverRegistered call) so
only one function named NewProvider exists in sql_provider.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 064fce66-be87-4ec3-8810-2ff8638fffca
📒 Files selected for processing (1)
service/entityresolution/multi-strategy/providers/sql/sql_provider.go
Signed-off-by: jp-ayyappan <jp@as2max.com>
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.RegisterSummary by CodeRabbit
New Features
Improvements
Notes