feat(mongo): migrate from mongo-driver v1 to v2#487
Merged
Conversation
In-place upgrade of the MongoDB driver across commons/mongo, commons/outbox/mongo, commons/tenant-manager/core, and commons/tenant-manager/mongo. Module path stays at /v5; public function signatures are unchanged. Consumer services that hold the returned *mongo.Database/*mongo.Client will need to update their own MongoDB calls to the v2 driver (notably: Collection.Distinct now returns *DistinctResult, mongo.Connect no longer pings, primitive types live under bson, WithTransaction callback receives context.Context instead of mongo.SessionContext). Driver pinned at go.mongodb.org/mongo-driver/v2 v2.6.0. Unit (4534) and integration (142) suites pass. Lint clean. X-Lerian-Ref: 0x1
WalkthroughMigrates the codebase from MongoDB Go driver v1 to v2: update go.mod, replace imports across modules, adapt API changes (mongo.Connect signature, Collection.Distinct result decoding, DateTime type mapping, WithTransaction callback context), and add explicit Ping/disconnect handling for TLS connections. ChangesMongoDB Driver v1 to v2 Migration
Comment |
Contributor
🔒 Security Scan Results —
|
| Stage | Status | Blocking? |
|---|---|---|
| Filesystem Scan | ✅ Clean | — |
| Docker Image Scan | ➖ Skipped | — |
| Docker Hub Health Score | ➖ Skipped | — |
| Pre-release Version Check | ✅ Clean | — |
Trivy
Filesystem Scan
✅ No vulnerabilities or secrets found.
Pre-release Version Check
✅ No unstable version pins found.
lerian-studio
added
size/S
Contributor
🔍 PR Validation Summary✅ PR Mergeable — no blocking failures
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CHANGELOG.md`:
- Around line 5-6: The long parenthetical in the CHANGELOG entry should be split
into a clear bulleted list of the individual breaking changes: extract and list
each item (options.UpdateOne(), options.Find(), options.Index() now return
*XxxOptionsBuilder; primitive.ObjectID → bson.ObjectID; primitive.DateTime →
bson.DateTime; mongo.Connect no longer takes context.Context;
Collection.Distinct returns *DistinctResult instead of ([]any, error);
WithTransaction callbacks receive context.Context instead of
mongo.SessionContext) as separate bullets under the mongo migration note so
readers can scan each migration step easily while preserving the surrounding
explanation and version context.
- Around line 5-6: Update the changelog to mention that in v2 mongo.Connect no
longer performs an implicit ping/validation of the server, so consumers who
previously relied on that must explicitly call client.Ping(ctx, nil) (or
equivalent health-check) after mongo.Connect(...) to verify connectivity;
reference the changed symbol mongo.Connect and show the explicit call to
client.Ping(ctx, nil) as required follow-up in the migration notes so users are
aware to add that check when upgrading.
In `@commons/tenant-manager/mongo/manager.go`:
- Around line 161-163: The disconnect on Ping failure currently reuses the
incoming ctx (see mongoClient.Ping and mongoClient.Disconnect) which may be
canceled; change it to create a short-lived bounded context (e.g., via
context.WithTimeout or context.WithDeadline) specifically for the
mongoClient.Disconnect call, defer the cancel, and call Disconnect using that
new context so the driver can shut down monitoring goroutines reliably even if
the original request context is expired.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcb36519-94d4-4804-a024-89abe6975c99
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
CHANGELOG.mdcommons/mongo/mongo.gocommons/mongo/mongo_integration_test.gocommons/mongo/mongo_test.gocommons/outbox/mongo/claim.gocommons/outbox/mongo/coverage_boost_test.gocommons/outbox/mongo/document.gocommons/outbox/mongo/extra_test.gocommons/outbox/mongo/helpers.gocommons/outbox/mongo/indexes.gocommons/outbox/mongo/pure_functions_test.gocommons/outbox/mongo/repository.gocommons/outbox/mongo/repository_integration_test.gocommons/outbox/mongo/repository_test.gocommons/tenant-manager/core/context.gocommons/tenant-manager/core/context_test.gocommons/tenant-manager/mongo/manager.gocommons/tenant-manager/mongo/manager_test.gogo.mod
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 87.0% ✅ PASS |
| Threshold | 80% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/lib-commons/v5/commons/backoff |
91.1% |
github.com/LerianStudio/lib-commons/v5/commons/certificate |
88.8% |
github.com/LerianStudio/lib-commons/v5/commons/circuitbreaker |
86.8% |
github.com/LerianStudio/lib-commons/v5/commons/cron |
94.2% |
github.com/LerianStudio/lib-commons/v5/commons/crypto |
95.6% |
github.com/LerianStudio/lib-commons/v5/commons/dlq |
81.0% |
github.com/LerianStudio/lib-commons/v5/commons/errgroup |
86.1% |
github.com/LerianStudio/lib-commons/v5/commons/internal/nilcheck |
100.0% |
github.com/LerianStudio/lib-commons/v5/commons/jwt |
89.4% |
github.com/LerianStudio/lib-commons/v5/commons/license |
96.9% |
github.com/LerianStudio/lib-commons/v5/commons/mongo |
89.0% |
github.com/LerianStudio/lib-commons/v5/commons/net/http/idempotency |
93.0% |
github.com/LerianStudio/lib-commons/v5/commons/net/http/ratelimit |
90.8% |
github.com/LerianStudio/lib-commons/v5/commons/net/http |
96.1% |
github.com/LerianStudio/lib-commons/v5/commons/outbox |
91.9% |
github.com/LerianStudio/lib-commons/v5/commons/pointers |
100.0% |
github.com/LerianStudio/lib-commons/v5/commons/postgres |
84.6% |
github.com/LerianStudio/lib-commons/v5/commons/rabbitmq |
89.3% |
github.com/LerianStudio/lib-commons/v5/commons/redis |
89.5% |
github.com/LerianStudio/lib-commons/v5/commons/safe |
99.6% |
github.com/LerianStudio/lib-commons/v5/commons/secretsmanager |
98.7% |
github.com/LerianStudio/lib-commons/v5/commons/security/ssrf |
95.9% |
github.com/LerianStudio/lib-commons/v5/commons/security |
100.0% |
github.com/LerianStudio/lib-commons/v5/commons/server |
88.0% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/cache |
97.9% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/client |
93.0% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/consumer |
87.9% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/core |
99.0% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/event |
95.7% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/internal/eviction |
100.0% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/log |
100.0% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/middleware |
92.3% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/mongo |
76.3% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/postgres |
86.8% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/rabbitmq |
82.6% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/redis |
93.8% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/s3 |
96.3% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/tenantcache |
98.4% |
github.com/LerianStudio/lib-commons/v5/commons/tenant-manager/valkey |
100.0% |
github.com/LerianStudio/lib-commons/v5/commons/transaction |
95.1% |
github.com/LerianStudio/lib-commons/v5/commons/webhook |
91.5% |
github.com/LerianStudio/lib-commons/v5/commons |
96.4% |
Generated by Go PR Analysis workflow
- manager.go: use bounded context for Disconnect on Ping failure. Reusing the request context risked carrying an already-canceled ctx into Disconnect, which prevents the driver from cleanly tearing down topology/monitoring goroutines. - CHANGELOG: split the v5.3.0 migration notes into a scannable bulleted list and document that v2 mongo.Connect no longer pings the server implicitly — callers that relied on Connect-time reachability validation must add an explicit client.Ping(ctx, nil) after connecting. X-Lerian-Ref: 0x1
The pattern of passing a request-scoped context to mongo.Client.Disconnect risks leaking topology/monitoring goroutines when the caller's context has already been canceled. The driver requires a non-canceled context to complete its graceful shutdown sequence. Centralize the fix in a disconnectClient helper that always derives a fresh bounded context from context.Background() with a 10s timeout. Apply it to all 7 Disconnect call sites in tenant-manager/mongo/manager.go: - connectWithTLS (Ping-failure cleanup, addressed earlier in this PR) - swapMongoConnection (config-change replacement) - cacheConnection (closed-manager and excess-connection branches) - evictLRU (LRU pool eviction) - Close (Manager shutdown) - CloseConnection (per-tenant disconnect) The swapMongoConnection signature drops its unused ctx parameter as a consequence; updated the single test caller accordingly. X-Lerian-Ref: 0x1
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)
commons/tenant-manager/mongo/manager.go (1)
828-844:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid disconnecting Mongo clients while
p.muis held.These branches perform network teardown under the manager write lock, and
disconnectClientcan wait up to 10 seconds. One slow shutdown here will serialize unrelatedGetConnection/CloseConnectionwork across all tenants. Snapshot the client to close under the lock, release the lock, then disconnect outside.Applies to
cacheConnection(lines 828–836, 838–848) andevictLRU(line 878).🤖 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 `@commons/tenant-manager/mongo/manager.go` around lines 828 - 844, In cacheConnection and evictLRU, avoid calling disconnectClient while holding the manager lock p.mu; instead, under the lock snapshot and remove the client (e.g., read and delete p.connections[tenantID] and clear conn.DB) and store the DB client reference in a local variable, release the lock, then call disconnectClient on that local client; update the cacheConnection and evictLRU code paths to follow this pattern so that network teardown (disconnectClient) happens outside the locked region and p.mu only protects mutations of p.connections and related state.
♻️ Duplicate comments (1)
commons/tenant-manager/mongo/manager.go (1)
40-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute the remaining
Disconnectpaths through this helper.This helper fixes only part of the file:
disconnectMongoanddisconnectUnhealthyConnectionstill callClient.Disconnectwith request-scoped contexts, so reconnect cleanup and stale-connection cleanup can still short-circuit when the caller context is already canceled.🤖 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 `@commons/tenant-manager/mongo/manager.go` around lines 40 - 53, The file still calls Client.Disconnect with request-scoped contexts in disconnectMongo and disconnectUnhealthyConnection; update both to route all disconnects through the helper disconnectClient(*mongo.Client) so the driver always uses a fresh bounded context. Concretely, replace any calls like client.Disconnect(ctx) inside disconnectMongo and disconnectUnhealthyConnection with disconnectClient(client) (preserve nil checks and propagate returned errors unchanged) so the helper's contextWithTimeout behavior is used for all disconnect paths.
🤖 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 `@commons/tenant-manager/mongo/manager.go`:
- Around line 828-844: In cacheConnection and evictLRU, avoid calling
disconnectClient while holding the manager lock p.mu; instead, under the lock
snapshot and remove the client (e.g., read and delete p.connections[tenantID]
and clear conn.DB) and store the DB client reference in a local variable,
release the lock, then call disconnectClient on that local client; update the
cacheConnection and evictLRU code paths to follow this pattern so that network
teardown (disconnectClient) happens outside the locked region and p.mu only
protects mutations of p.connections and related state.
---
Duplicate comments:
In `@commons/tenant-manager/mongo/manager.go`:
- Around line 40-53: The file still calls Client.Disconnect with request-scoped
contexts in disconnectMongo and disconnectUnhealthyConnection; update both to
route all disconnects through the helper disconnectClient(*mongo.Client) so the
driver always uses a fresh bounded context. Concretely, replace any calls like
client.Disconnect(ctx) inside disconnectMongo and disconnectUnhealthyConnection
with disconnectClient(client) (preserve nil checks and propagate returned errors
unchanged) so the helper's contextWithTimeout behavior is used for all
disconnect paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e99d270d-761d-4a8b-9486-a29cbfb40c09
📒 Files selected for processing (2)
commons/tenant-manager/mongo/coverage_boost_test.gocommons/tenant-manager/mongo/manager.go
lerian-studio
added
size/XS
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
In-place upgrade of the MongoDB driver across
commons/mongo,commons/outbox/mongo,commons/tenant-manager/core, andcommons/tenant-manager/mongo./v5— no major bumptmcore.GetMBContextstill returns*mongo.Database, just from v2 now)go.mongodb.org/mongo-driver/v2 v2.6.0(latest stable)Why v2
The migration is motivated by the v2 driver itself — not by any specific defect in v1:
*XxxOptionsBuilder)*DistinctResultinstead of([]any, error))primitivepackage merged intobson)mongo.SessionContext)What consumer services need to know
The lib-commons API surface is unchanged, but the concrete
*mongo.Database/*mongo.Clienttypes now resolve to v2. Any code that holds these values and calls MongoDB methods on them needs to adapt:coll.Distinct(ctx, ...)returns([]any, error)*DistinctResult— use.Err()+.Decode(&values)mongo.Connect(ctx, opts)validates reachabilityclient.Ping(ctx, nil)if you relied on Connect to failprimitive.DateTime,primitive.ObjectID,primitive.NewObjectID(), etc.bsonroot:bson.DateTime,bson.ObjectID,bson.NewObjectID()WithTransaction(ctx, func(sessCtx mongo.SessionContext) ...)context.Context; usemongo.SessionFromContext(ctx)if you need the sessionoptions.UpdateOne()returns*UpdateOneOptions*UpdateOneOptionsBuilder— fluent.SetX()chain is identicalServices consuming lib-commons via
tmcore.GetMBContext,tmcore.ContextWithMB,tmmongo.Manager.GetDatabase*, orcommons/mongo.Client.Database()will need to update their query code accordingly.Note:
disconnectClienthelper is unrelated to v2The
refactor(mongo): use bounded context for all Disconnect call sitescommit introduces adisconnectClienthelper and applies it to everyDisconnectcall intenant-manager/mongo/manager.go. This is a defensive hygiene fix, not a v2 motivator. The issue (passing a possibly-canceled request context toClient.Disconnectleaks topology/monitoring goroutines) exists identically on v1 and v2 — the MongoDB driver has always required a non-canceled context for graceful shutdown.The fix lives in this PR because CodeRabbit surfaced the issue while reviewing the new
connectWithTLScode path we added to compensate for v2's removal of implicit pinging atConnect. Once flagged, it was easier to address all 7 sites consistently here than to open a follow-up PR.The v2 migration would still be worth doing without this commit, and this commit would still be worth doing without the v2 migration.
Files changed
20 files, +70/-59 lines (driver migration) plus the
disconnectClientrefactor:go.mod(drop v1, promote v2 from indirect to direct)go.sum(regenerated bygo mod tidy)CHANGELOG.md(entry under[5.3.0])Validation
go test -tags=unit ./...→ 4534 pass, 0 failgo test -tags=integration ./...→ 142 pass, 0 fail (testcontainersmongo:7)go vet ./...→ cleangolangci-lint run ./...→ 0 issuesgofmt -l ./commons→ cleanTag to follow
After merge:
v5.3.0.Test plan
🤖 Generated with Claude Code