fix(ers): use pgx database/sql driver in SQL ERS provider#3543
Open
jp-ayyappan wants to merge 13 commits into
Open
fix(ers): use pgx database/sql driver in SQL ERS provider#3543jp-ayyappan wants to merge 13 commits into
jp-ayyappan wants to merge 13 commits into
Conversation
Contributor
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 | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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}
Contributor
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.
Contributor
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
Contributor
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
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
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
Contributor
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
Contributor
Contributor
Signed-off-by: jp-ayyappan <jp@as2max.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
…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
Contributor
Signed-off-by: jp-ayyappan <jp@as2max.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Signed-off-by: jp-ayyappan <jp@as2max.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: jp-ayyappan <jp@as2max.com>
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Signed-off-by: jp-ayyappan <jp@as2max.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
jp-ayyappan
changed the title
Signed-off-by: jp-ayyappan <jp@as2max.com>
Contributor
Signed-off-by: jp-ayyappan <jp@as2max.com>
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
c-r33d
reviewed
Jun 23, 2026
|
|
||
| const ( | ||
| // Default SQL configuration values | ||
| defaultPostgreSQLDriver = "pgx/v5" |
Contributor
There was a problem hiding this comment.
I'd recommend just keeping it postgres, less to test and probably provides the same value.
c-r33d
reviewed
Jun 23, 2026
Comment on lines
+75
to
+77
| if err != nil { | ||
| t.Fatalf("expected postgres alias to build connection string: %v", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| if err != nil { | |
| t.Fatalf("expected postgres alias to build connection string: %v", err) | |
| } | |
| t.Require().NoError(err) |
Use the testify module with require.
c-r33d
reviewed
Jun 23, 2026
Comment on lines
+78
to
+80
| if !strings.Contains(connStr, "dbname=identity_db") { | ||
| t.Fatalf("expected database name in connection string, got %q", connStr) | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| if !strings.Contains(connStr, "dbname=identity_db") { | |
| t.Fatalf("expected database name in connection string, got %q", connStr) | |
| } | |
| t.Contains(connStr, "dbname=identity_db") |
c-r33d
reviewed
Jun 23, 2026
| // _ "github.com/mattn/go-sqlite3" // SQLite driver | ||
|
|
||
| // Register the pgx/v5 database/sql driver for SQL providers. | ||
| _ "github.com/jackc/pgx/v5/stdlib" |
Contributor
There was a problem hiding this comment.
Suggested change
| _ "github.com/jackc/pgx/v5/stdlib" | |
| "github.com/jackc/pgx/v5/stdlib" |
Signed-off-by: jp-ayyappan <jp@as2max.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3539.
The multi-strategy ERS SQL provider called
sql.Open(config.Driver, ...)but did not ensure the PostgreSQLdatabase/sqldriver was available in the platform service binary. This PR makes PostgreSQL SQL provider configs work without requiring consumers to add their own blank import.Changes
github.com/jackc/pgx/v5/stdlibin the SQL provider package so the pgxdatabase/sqldrivers are registered when the provider package is loaded.pgx/v5driver name.pgx->pgx/v5postgres->pgx/v5postgresql->pgx/v5driver: pgx/v5.Behavior
pgx/v5driver: pgx/v5pgx/v5driver: pgxpgx/v5driver: postgresordriver: postgresqlpgx/v5driver: Postgrespgx/v5github.com/lib/pqpgx/v5; the existingpostgresregistration is ignored because the provider normalizes to the versioned pgx driver namedriver: mysqldriver: sqliteordriver: sqlite3Testing
go test ./entityresolution/multi-strategy/providers/sql -racego test ./entityresolution/multi-strategy/...GOCACHE=/private/tmp/go-build-cache GOLANGCI_LINT_CACHE=/private/tmp/golangci-lint-cache golangci-lint run --config ../.golangci.yaml ./entityresolution/multi-strategy/providers/sql --new-from-rev=origin/mainNotes
driver: postgres,driver: postgresql, ordriver: pgxcontinue to work via alias normalization.driver: pgx/v5to avoid ambiguity with the process-global unversionedpgxdriver name.Summary by CodeRabbit
New Features
Improvements
pgx/v5driver name for consistent resolution.Notes