Skip to content

fix: prevent Orm.Connection() cached path from poisoning dbConfig - #1545

Open
goravel-coder wants to merge 1 commit into
v1.17.xfrom
fix/v1.17-orm-connection-poisoned-dbconfig
Open

fix: prevent Orm.Connection() cached path from poisoning dbConfig#1545
goravel-coder wants to merge 1 commit into
v1.17.xfrom
fix/v1.17-orm-connection-poisoned-dbconfig

Conversation

@goravel-coder

Copy link
Copy Markdown
Contributor

Summary

  • Orm.Connection() now resolves each connection's own database config from a shared, concurrency-safe QueriesCache, so Name()/DatabaseName() and connection-scoped behavior (like where the migration ledger row is written) reflect the requested connection instead of the default's.
  • Concurrent Connection() calls that lazily build the same connection are now serialized by double-checked locking, eliminating the data race on the connection cache — TestOrmConnectionConcurrentSafe passes under -race and fails on the unlocked version.
  • The exported database/orm/NewOrm constructor now takes a *QueriesCache built via the new NewQueriesCache(queries, dbConfigs) — breaking only for direct callers of the concrete constructor; contractsorm.Orm is unchanged, so no mock regeneration is needed.

Closes goravel/goravel#987

Why

This is a backport of #1540 to the v1.17.x release branch (original issue: goravel/goravel#987). Orm.Connection() had a cached fast path that ignored the requested connection and reused the receiver's dbConfig. Once a non-default connection had been resolved, every later Connection() call for it returned an Orm whose Name()/DatabaseName() — and therefore derived behavior like where the migration ledger row gets written — still pointed at the default connection. Running two migrations on the same non-default Connection() wrote the second ledger row to the wrong database.

// app/database/migrations — two consecutive migrations on the same non-default connection
func (r *M20260826160940CreateUsersTable) Connection() string {
	return "sqlite"
}

func (r *M20260826161140CreateUserTokensTable) Connection() string {
	return "sqlite"
}

Before the fix, the second migration's ledger row was written to the sqlite connection, where no migrations table exists, failing the run. After the fix, both migrations run on their declared sqlite connection while both ledger rows are recorded on the default connection, matching Laravel's behavior.

The same fix hardens the connection cache for concurrent use. Lazy connection builds now flow through a shared QueriesCache guarded by an RWMutex with double-checked locking, so concurrent cold-path calls from different goroutines no longer race on the shared maps; Observe() snapshots the cache under a read lock before invoking user code. As a cleanup, the ambiguous internal cache field was renamed from m to connections.

// Concurrent resolution of the same lazily-built connection from multiple goroutines
var wg sync.WaitGroup
for range 10 {
	wg.Add(1)
	go func() {
		defer wg.Done()
		orm := facades.Orm().Connection("sqlite")
		// ...
	}()
}
wg.Wait()

Before the fix, these concurrent cold-path calls raced on the shared connection maps (a DATA RACE under -race). After the fix, double-checked locking serializes the build and every caller gets an Orm bound to the requested connection.

@goravel-coder
goravel-coder requested a review from a team as a code owner September 1, 2026 10:44
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (v1.17.x@bd3ec82). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff             @@
##             v1.17.x    #1545   +/-   ##
==========================================
  Coverage           ?   70.54%           
==========================================
  Files              ?      281           
  Lines              ?    14876           
  Branches           ?        0           
==========================================
  Hits               ?    10495           
  Misses             ?     4381           
  Partials           ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants