Mongo db driver update#176
Conversation
…y instance of primitive.ObjectID to bson.ObjectId.
…nce of primitive.ObjectID to bson.ObjectId.
…exes by using statically defined default indexes, so default indexes are dropped reliably even when create was not run beforehand.
|
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:
📝 WalkthroughWalkthroughMigration to the MongoDB Go driver v2: import paths and BSON/ObjectID types updated, fluent options APIs adopted, index names centralized into constants with simplified drop logic, and mongo.Connect/context callsites adjusted across jobs, DB packages, services, models, and tests. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Greptile SummaryThis PR upgrades the MongoDB Go driver from v1 (
Confidence Score: 4/5Safe to merge once the open nil-pointer dereference issues on DeleteMany/UpdateMany/DeleteOne result access are resolved; the remainder of the v2 migration is mechanically correct. Multiple DB write functions across the study package return res.DeletedCount or res.ModifiedCount without first checking err; when the driver call fails it returns a nil result, so any DB error on those paths triggers a runtime panic rather than a graceful error return. These were flagged in the prior review round and remain unaddressed in the current HEAD. pkg/db/study/confidential-responses.go, pkg/db/study/responses.go, pkg/db/study/reports.go, pkg/db/study/study-rules.go, pkg/db/study/researcher-messages.go
|
| Filename | Overview |
|---|---|
| pkg/db/study/confidential-responses.go | New file from v1→v2 migration; DeleteConfidentialResponses and UpdateParticipantIDonConfidentialResponses access res.DeletedCount/res.ModifiedCount before checking err — nil pointer panic when DB call fails. |
| pkg/db/study/responses.go | UpdateParticipantIDonResponses accesses res.ModifiedCount before checking err from UpdateMany — nil pointer panic on DB failure. |
| pkg/db/study/study-rules.go | New file; DeleteStudyRulesByID accesses res.DeletedCount before checking err from DeleteOne — nil pointer panic on DB failure. |
| pkg/db/study/reports.go | UpdateParticipantIDonReports accesses res.ModifiedCount before checking err from UpdateMany; misleading error log in DropIndex path already flagged. |
| pkg/db/study/researcher-messages.go | DeleteResearcherMessages accesses res.DeletedCount before checking err from DeleteMany — nil pointer panic on DB failure. |
| pkg/db/messaging/outgoing-emails.go | Stray nil variadic arg on DeleteOne was fixed; remaining changes correctly updated to v2 API. |
| pkg/db/study/surveys.go | Options builder and Distinct API correctly migrated to v2; pre-existing DeleteSurveyVersion nil-dereference pattern not touched in this diff. |
| pkg/db/participant-user/users.go | AddUser and all FindOneAndReplace calls correctly migrated; options struct replaced with method-chaining API. |
| pkg/db/participant-user/otps.go | WithSession callback correctly updated from func(SessionContext) to func(context.Context) for v2 compatibility. |
| go.mod | Driver dependency bumped from v1.17.4 to v2.5.0; indirect dependencies updated accordingly. |
Reviews (8): Last reviewed commit: "Update mongo driver version" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pkg/db/study/researcher-messages.go (1)
65-66:⚠️ Potential issue | 🟡 MinorCheck
errbefore readingDeletedCount.Although
DeleteManyreturns aDeleteResultvalue (not a pointer), you should check the error before attempting to use the result. This aligns with error handling patterns used elsewhere in the codebase (seerenew-tokens.goandresponses.go). If the operation fails,errwill be non-nil and you should return it rather than proceeding with an empty or invalid count.🐛 Proposed fix
res, err := dbService.collectionResearcherMessages(instanceID, studyKey).DeleteMany(ctx, filter) - return res.DeletedCount, err + if err != nil { + return 0, err + } + return res.DeletedCount, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/study/researcher-messages.go` around lines 65 - 66, The code calls dbService.collectionResearcherMessages(...).DeleteMany and immediately returns res.DeletedCount without checking err; change the logic in the function that performs this DeleteMany (referencing DeleteMany, collectionResearcherMessages and the returned res.DeleteResult) to first check if err != nil and return 0, err (or return the error) before reading res.DeletedCount; only return res.DeletedCount when err is nil to match error-handling used in renew-tokens.go and responses.go.pkg/db/participant-user/otps.go (1)
76-108:⚠️ Potential issue | 🟠 Major
WithSessionwithout a transaction — count-then-insert still races.The callback signature is correct for v2, but
mongo.WithSessionalone only scopes a session tosessCtx; it does not start a transaction. TheCountDocuments→InsertOnesequence (lines 89–104) is not atomic—two concurrent calls can both observecount < maxOTPCountand each insert, exceeding the cap.To enforce
maxOTPCount, wrap the callback insession.WithTransaction:Transactional fix
- return mongo.WithSession(ctx, session, createOTPIfLimitNotReached) + _, err = session.WithTransaction(ctx, func(sessCtx context.Context) (interface{}, error) { + return nil, createOTPIfLimitNotReached(sessCtx) + }) + return errAlternatively, use a unique index or atomic
$inc-guarded upsert. Without an explicit transaction, the session adds overhead without benefit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/participant-user/otps.go` around lines 76 - 108, The CreateOTP flow currently opens a session with StartSession and calls mongo.WithSession but does not start a transaction, so the CountDocuments -> InsertOne sequence (in the anonymous createOTPIfLimitNotReached callback using collectionOTPs, CountDocuments and InsertOne) can race and exceed maxOTPCount; change the implementation to call session.WithTransaction(ctx, txnFunc) (or use session.StartTransaction + session.WithTransaction pattern) and perform the CountDocuments and InsertOne inside that transaction, or alternatively replace the check+insert with an atomic approach (unique index or an $inc-guarded upsert) to enforce maxOTPCount without races.pkg/db/messaging/db.go (1)
33-40:⚠️ Potential issue | 🟡 MinorRemove dead
context.WithTimeout— not accepted bymongo.Connectin driver v2.The timeout created on lines 33–34 is discarded (assigned to
_) and never applied to the connection setup, sincemongo.Connectin driver v2 no longer accepts a context parameter. The resultingdefer cancel()is a no-op. The timeout is properly applied toPingon line 46 instead, so this first block should be removed.♻️ Proposed cleanup
func NewMessagingDBService(configs db.DBConfig) (*MessagingDBService, error) { - _, cancel := context.WithTimeout(context.Background(), time.Duration(configs.Timeout)*time.Second) - defer cancel() - dbClient, err := mongo.Connect( options.Client().ApplyURI(configs.URI), options.Client().SetMaxConnIdleTime(time.Duration(configs.IdleConnTimeout)*time.Second), options.Client().SetMaxPoolSize(configs.MaxPoolSize), )Apply the same cleanup to the sibling DB service files:
pkg/db/participant-user/db.go,pkg/db/study/db.go,pkg/db/management-user/db.go, andpkg/db/global-infos/db.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/messaging/db.go` around lines 33 - 40, Remove the pointless context.WithTimeout call and its defer cancel in pkg/db/messaging/db.go because the returned context is discarded and mongo.Connect no longer accepts a context; leave the mongo.Connect(...) call as-is and rely on the existing Ping call (which already uses a proper context timeout) to enforce connection timeout. Specifically delete the context.WithTimeout(...) and defer cancel() lines surrounding the mongo.Connect invocation (symbols: context.WithTimeout, defer cancel, mongo.Connect, Ping) and run the same cleanup in the sibling DB files pkg/db/participant-user/db.go, pkg/db/study/db.go, pkg/db/management-user/db.go, and pkg/db/global-infos/db.go.pkg/db/participant-user/renew-tokens.go (1)
120-120:⚠️ Potential issue | 🟡 MinorRemove trailing
nilarguments fromDeleteOne/DeleteManycalls to match v2 driver idioms.In MongoDB Go Driver v2,
DeleteOneandDeleteManyaccept variadicoptsparameters. When no options are needed, omit the argument entirely rather than passingnil. The standard practice shown in driver documentation is to call these methods with justctxandfilter.♻️ Proposed fix
- res, err := dbService.collectionRenewTokens(instanceID).DeleteOne(ctx, filter, nil) + res, err := dbService.collectionRenewTokens(instanceID).DeleteOne(ctx, filter) @@ - res, err := dbService.collectionRenewTokens(instanceID).DeleteMany(ctx, filter, nil) + res, err := dbService.collectionRenewTokens(instanceID).DeleteMany(ctx, filter) @@ - res, err := dbService.collectionRenewTokens(instanceID).DeleteMany(ctx, filter, nil) + res, err := dbService.collectionRenewTokens(instanceID).DeleteMany(ctx, filter) @@ - res, err := dbService.collectionOutgoingEmails(instanceID).DeleteOne(ctx, filter, nil) + res, err := dbService.collectionOutgoingEmails(instanceID).DeleteOne(ctx, filter)Also applies to: 135-135, 147-147, and
pkg/db/messaging/outgoing-emails.go:114.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/participant-user/renew-tokens.go` at line 120, Remove the unnecessary trailing nil options from Mongo delete calls: update calls like dbService.collectionRenewTokens(instanceID).DeleteOne(ctx, filter, nil) and any DeleteMany(ctx, filter, nil) to call the v2 driver idiom without the nil options (e.g., DeleteOne(ctx, filter)); apply the same change to the other locations mentioned (lines calling DeleteOne/DeleteMany and pkg/db/messaging/outgoing-emails.go:114) so only ctx and filter are passed.pkg/db/management-user/sessions.go (1)
27-48:⚠️ Potential issue | 🟠 MajorSuppress
IndexNotFound(code 27) errors when dropping individual indexes.The PR decouples
DropIndexFor*fromCreateDefaultIndexesFor*, allowing drops to run independently. This meansDropOnewill encounter missing indexes and return aCommandErrorwith code27 / IndexNotFound, which is then logged asERROR. On fresh deployments or afterDropAllhas already run, every default index name logs a spurious error.This pattern is repeated across 28+ files in the codebase (not limited to the five mentioned):
permissions.go,service-users.go,sent-emails.go,study-rules.go,study-code-lists.go, and many others acrossstudy/,messaging/,participant-user/, andglobal-infos/packages.The codebase already uses
errors.As()withmongo.CommandErrorinpkg/db/utils.go(line ~26, checking code 26). Apply the same pattern to filter code 27:err := dbService.collectionSessions(instanceID).Indexes().DropOne(ctx, indexName) if err != nil { var cmdErr mongo.CommandError if errors.As(err, &cmdErr) && cmdErr.Code == 27 { // IndexNotFound continue } slog.Error("Error dropping index for sessions", slog.String("error", err.Error()), slog.String("indexName", indexName)) }Consider extracting a small helper function (e.g.,
dropIndexIfExists) inpkg/db/utils.goto avoid duplicating this across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/management-user/sessions.go` around lines 27 - 48, DropIndexForSessionsCollection currently logs errors when DropOne returns a CommandError for missing indexes; update it to ignore mongo.CommandError with Code == 27 (IndexNotFound) by using errors.As to type-assert to mongo.CommandError and continue instead of logging, when iterating defaultSessionIndexNames and calling collectionSessions(instanceID).Indexes().DropOne; to avoid duplication across the codebase, add a helper dropIndexIfExists(ctx, indexer, indexName) in pkg/db/utils.go that encapsulates the DropOne call and the errors.As/mongo.CommandError Code==27 check, then call that helper from DropIndexForSessionsCollection (and other similar DropIndexFor* functions).
🧹 Nitpick comments (6)
go.mod (1)
11-11: Consider bumping to the latest v2 patch or documenting the pin.
go.mongodb.org/mongo-driver/v2is pinned tov2.5.0, while pkg.go.dev and MongoDB’s release announcement currently listv2.5.1as available. Since this PR is already doing the driver-wide migration, consider taking the latest patch unlessv2.5.0is intentional.Sources: pkg.go.dev module page, MongoDB v2.5.1 release announcement.
🔧 Proposed dependency bump
- go.mongodb.org/mongo-driver/v2 v2.5.0 + go.mongodb.org/mongo-driver/v2 v2.5.1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 11, The go.mod currently pins the MongoDB driver to go.mongodb.org/mongo-driver/v2 v2.5.0; update the version to the latest v2 patch (v2.5.1) or add a comment documenting why v2.5.0 is intentionally pinned. Locate the module line for go.mongodb.org/mongo-driver/v2 in go.mod and either change the version token to v2.5.1 or add a short inline comment explaining the intentional pin to prevent accidental upgrades.pkg/db/messaging/scheduled-email.go (2)
108-114: Avoid shadowing theoptionspackage with a local variable.
options := options.FindOneAndReplace()...shadows the importedoptionspackage inside this scope. It compiles today only because the RHS is evaluated before the short-var declaration enters scope, sooptions.Afteron the last chained call still resolves to the package. Any future maintenance that adds anotheroptions.*reference below this line in the same block will silently bind to the local variable and fail in confusing ways. Other files in this PR (e.g.study-rules.go) already useopts— prefer consistency.♻️ Proposed rename
- options := options.FindOneAndReplace(). - SetUpsert(false). - SetReturnDocument(options.After) + opts := options.FindOneAndReplace(). + SetUpsert(false). + SetReturnDocument(options.After) elem := messagingTypes.ScheduledEmail{} err := dbService.collectionEmailSchedules(instanceID). - FindOneAndReplace(ctx, filter, scheduledEmail, options). + FindOneAndReplace(ctx, filter, scheduledEmail, opts). Decode(&elem)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/messaging/scheduled-email.go` around lines 108 - 114, The local variable named options shadows the imported options package in the FindOneAndReplace call; rename the local var to opts (or similar) to avoid shadowing so package symbols like options.After continue to refer to the imported package. Update the chain that currently does options := options.FindOneAndReplace()... to use opts := options.FindOneAndReplace()... and ensure subsequent references in this block (e.g., the call in dbService.collectionEmailSchedules(instanceID).FindOneAndReplace(ctx, filter, scheduledEmail, opts).Decode(&elem)) use the new opts variable.
17-25: Minor:dropAll=falsebranch is a no-op warning.Since the email-schedules collection has no default indexes, calling
DropIndexForEmailSchedulesCollection(instanceID, false)just logs a warning and returns. Consistent with the PR pattern, but consider whether callers should be surfaced this via return value instead of a log, or simply have the function early-return silently. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/messaging/scheduled-email.go` around lines 17 - 25, The no-op branch in DropIndexForEmailSchedulesCollection currently only logs a warning when dropAll is false; change this to either silently early-return or, preferably, surface the no-op to callers by returning a boolean (or error) indicating “nothing to drop” so callers can react instead of relying on logs—update the DropIndexForEmailSchedulesCollection signature and callers accordingly, and keep logging only for unexpected errors (retain the slog.Error in the err path and remove/replace the slog.Warn in the no-op branch); refer to DropIndexForEmailSchedulesCollection and dbService.collectionEmailSchedules(instanceID) when locating the code to change.pkg/db/study/confidential-responses.go (1)
106-110: Avoid passing explicitnilas variadicoptsIn v2,
Collection.Find's opts parameter is...options.Lister[options.FindOptions]. Passingnilexplicitly results in a one-element slice containing a nil interface. The driver currently skips nil entries inmongoutil.NewOptions, so this doesn't crash today, but it's non-idiomatic and fragile across driver updates — simply omit the argument.♻️ Proposed fix
- cur, err := dbService.collectionConfidentialResponses(instanceID, studyKey).Find( - ctx, - filter, - nil, - ) + cur, err := dbService.collectionConfidentialResponses(instanceID, studyKey).Find(ctx, filter)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/study/confidential-responses.go` around lines 106 - 110, The Find call passing an explicit nil as the variadic opts is non-idiomatic and can produce a one-element slice with a nil interface; update the call to collectionConfidentialResponses(instanceID, studyKey).Find(ctx, filter) (i.e., remove the trailing nil variadic argument) so no nil entry is created; also scan for other usages of collectionConfidentialResponses(...).Find or similar Collection.Find calls and remove explicit nil variadic opts to rely on the zero-argument form.pkg/db/messaging/email-templates.go (1)
119-128: Redundant ID assignment in the insert path
emailTemplate.ID = bson.NewObjectID()is set on the struct right beforeInsertOne, then the exact same value is read back viares.InsertedID.(bson.ObjectID)and reassigned. You can either drop the pre-assignment and rely on the driver-generated_id, or drop the post-assertion since you already know the ID you set. Not a functional bug, just dead work.♻️ Suggested simplification
if emailTemplate.ID.IsZero() { - emailTemplate.ID = bson.NewObjectID() // new email template res, err := messagingDBService.collectionEmailTemplates(instanceID).InsertOne(ctx, emailTemplate) if err != nil { return messagingTypes.EmailTemplate{}, err } emailTemplate.ID = res.InsertedID.(bson.ObjectID) return emailTemplate, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/messaging/email-templates.go` around lines 119 - 128, The code redundantly sets emailTemplate.ID = bson.NewObjectID() before InsertOne and then reassigns emailTemplate.ID = res.InsertedID.(bson.ObjectID) after insert; choose one approach and remove the dead work: either remove the pre-insert assignment (leave ID generation to the driver and keep using res.InsertedID from InsertOne) or keep the pre-insert bson.NewObjectID() and drop the post-insert reassignment. Update the block around InsertOne (symbols: emailTemplate.ID, bson.NewObjectID(), InsertOne, res.InsertedID) accordingly so only one assignment remains.pkg/db/management-user/db.go (1)
38-45: Remove the unused timeout context on lines 38–39.In MongoDB Go driver v2,
mongo.Connect()does not accept a context parameter. The context created on line 38 is discarded, and the timeout is only applied to thePing()call on line 51 (which creates its own context). The dead code allocation can be safely removed.♻️ Proposed cleanup
func NewManagementUserDBService(configs db.DBConfig) (*ManagementUserDBService, error) { - _, cancel := context.WithTimeout(context.Background(), time.Duration(configs.Timeout)*time.Second) - defer cancel() - dbClient, err := mongo.Connect( options.Client().ApplyURI(configs.URI), options.Client().SetMaxConnIdleTime(time.Duration(configs.IdleConnTimeout)*time.Second), options.Client().SetMaxPoolSize(configs.MaxPoolSize), )This pattern also appears in
pkg/db/study/db.go,pkg/db/messaging/db.go,pkg/db/participant-user/db.go, andpkg/db/global-infos/db.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/management-user/db.go` around lines 38 - 45, Remove the unused timeout context allocation: eliminate the call to context.WithTimeout(...) and the following defer cancel() since the returned context and cancel are never used around the mongo.Connect(...) call; keep the mongo.Connect(...) invocation as-is and ensure the existing Ping(...) call (which creates its own context) remains untouched. Specifically, remove the variables created by context.WithTimeout and the defer cancel near the mongo.Connect call (symbols: context.WithTimeout, cancel, mongo.Connect, configs) and apply the same cleanup to the identical pattern in the other DB files that create but never use that context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/db/messaging/outgoing-emails.go`:
- Around line 94-97: In the three functions ResetLastSendAttemptForOutgoing,
DeleteOutgoingEmail (pkg/db/messaging/outgoing-emails.go) and
SavePasswordResetTrigger (pkg/db/participant-user/users.go) stop discarding the
error returned by bson.ObjectIDFromHex: validate the emailID/input by checking
the returned error, and if non-nil return a clear validation error to the caller
instead of building a filter with the NilObjectID; only construct the filter
(e.g., bson.M{"_id": _id}) and call UpdateOne/DeleteOne/other DB ops when
ObjectIDFromHex succeeded so malformed IDs never generate NilObjectID queries.
In `@pkg/db/participant-user/users.go`:
- Around line 201-204: The code currently ignores the error from
bson.ObjectIDFromHex(userID), which can produce a zero ObjectID and make
UpdateOne report success for no-op updates; modify the logic around
bson.ObjectIDFromHex in the function using userID (the _id variable) to check
the returned err immediately, and if non-nil return or propagate that error
instead of continuing; only build the filter (bson.M{"_id": _id}) and call
dbService.collectionParticipantUsers(instanceID).UpdateOne(ctx, filter, update)
when the ObjectID parse succeeded.
In `@pkg/db/study/confidential-responses.go`:
- Around line 86-91: The local variable named options shadows the imported
options package; rename the local to something like opts or replaceOpts (e.g.,
replaceOpts := options.Replace().SetUpsert(upsert)) and update the ReplaceOne
call to pass replaceOpts so the package symbol options remains available in this
function (locate the code around ReplaceOne/collectionConfidentialResponses and
the upsert/options assignment).
- Around line 72-75: The code calls
dbService.collectionConfidentialResponses(...).InsertOne and immediately does
res.InsertedID.(bson.ObjectID) before checking err, which can panic if InsertOne
failed; update the InsertConfidentialResponse (or the function containing this
snippet) to first check if err != nil and return an empty id with the error,
then safely type-assert res.InsertedID (use the comma-ok form, e.g., idObj, ok
:= res.InsertedID.(bson.ObjectID)) and handle the unexpected type by returning a
clear error; only call idObj.Hex() when the assertion succeeds.
In `@pkg/db/study/participant-file-infos.go`:
- Around line 3-9: The DropOne loop that currently logs all errors should be
changed to treat Mongo error codes 26 (NamespaceNotFound) and 27 (IndexNotFound)
as benign no-ops: after each DropOne call, if err != nil, attempt to type-assert
to *mongo.ServerError and call HasErrorCode(26) or HasErrorCode(27); if
HasErrorCode returns true, skip logging/continue, otherwise log the error at
error-level; update the const declarations (currently defining the numeric
codes) to keep named constants for 26 and 27 and use those constants in the
HasErrorCode checks so the check is clear and only real failures produce error
logs.
In `@pkg/db/study/reports.go`:
- Around line 72-80: The DropOne error handling in the loop over
defaultReportIndexNames should ignore MongoDB error codes 26 (NamespaceNotFound)
and 27 (IndexNotFound) instead of logging them; update the error branch after
collection.Indexes().DropOne(ctx, indexName) to use errors.As to check for a
*mongo.CommandError and inspect its Code field, and only call slog.Error for
errors whose Code is not 26 or 27 (fall back to logging for non-CommandError
types); keep references to defaultReportIndexNames,
collection.Indexes().DropOne, instanceID and studyKey when logging unexpected
failures.
In `@pkg/db/study/responses.go`:
- Around line 108-111: Guard against a nil response before accessing InsertedID:
after calling dbService.collectionResponses(...).InsertOne(ctx, response) check
if err != nil and return an empty id with the error immediately instead of
unconditionally evaluating res.InsertedID; also perform a safe type/assertion
for the inserted ID (handle the correct driver type used in this diff—e.g.,
bson.ObjectID or primitive.ObjectID) so the code in collectionResponses /
InsertOne does not panic when the insert fails.
In `@services/participant-api/apihandlers/authentication.go`:
- Line 312: The call newUser.ID, _ = bson.ObjectIDFromHex(id) ignores the parse
error; change this to capture the error (idObj, err := bson.ObjectIDFromHex(id))
and handle it the same way other ObjectIDFromHex uses do in this codebase: if
err != nil, log/return an appropriate error response and stop further processing
(e.g., return a 400/invalid-id or propagate the error from the handler), then
assign newUser.ID = idObj; ensure you use the same error wrapping/logging style
as surrounding handlers to keep behavior consistent with AddUser and other
callers.
---
Outside diff comments:
In `@pkg/db/management-user/sessions.go`:
- Around line 27-48: DropIndexForSessionsCollection currently logs errors when
DropOne returns a CommandError for missing indexes; update it to ignore
mongo.CommandError with Code == 27 (IndexNotFound) by using errors.As to
type-assert to mongo.CommandError and continue instead of logging, when
iterating defaultSessionIndexNames and calling
collectionSessions(instanceID).Indexes().DropOne; to avoid duplication across
the codebase, add a helper dropIndexIfExists(ctx, indexer, indexName) in
pkg/db/utils.go that encapsulates the DropOne call and the
errors.As/mongo.CommandError Code==27 check, then call that helper from
DropIndexForSessionsCollection (and other similar DropIndexFor* functions).
In `@pkg/db/messaging/db.go`:
- Around line 33-40: Remove the pointless context.WithTimeout call and its defer
cancel in pkg/db/messaging/db.go because the returned context is discarded and
mongo.Connect no longer accepts a context; leave the mongo.Connect(...) call
as-is and rely on the existing Ping call (which already uses a proper context
timeout) to enforce connection timeout. Specifically delete the
context.WithTimeout(...) and defer cancel() lines surrounding the mongo.Connect
invocation (symbols: context.WithTimeout, defer cancel, mongo.Connect, Ping) and
run the same cleanup in the sibling DB files pkg/db/participant-user/db.go,
pkg/db/study/db.go, pkg/db/management-user/db.go, and pkg/db/global-infos/db.go.
In `@pkg/db/participant-user/otps.go`:
- Around line 76-108: The CreateOTP flow currently opens a session with
StartSession and calls mongo.WithSession but does not start a transaction, so
the CountDocuments -> InsertOne sequence (in the anonymous
createOTPIfLimitNotReached callback using collectionOTPs, CountDocuments and
InsertOne) can race and exceed maxOTPCount; change the implementation to call
session.WithTransaction(ctx, txnFunc) (or use session.StartTransaction +
session.WithTransaction pattern) and perform the CountDocuments and InsertOne
inside that transaction, or alternatively replace the check+insert with an
atomic approach (unique index or an $inc-guarded upsert) to enforce maxOTPCount
without races.
In `@pkg/db/participant-user/renew-tokens.go`:
- Line 120: Remove the unnecessary trailing nil options from Mongo delete calls:
update calls like dbService.collectionRenewTokens(instanceID).DeleteOne(ctx,
filter, nil) and any DeleteMany(ctx, filter, nil) to call the v2 driver idiom
without the nil options (e.g., DeleteOne(ctx, filter)); apply the same change to
the other locations mentioned (lines calling DeleteOne/DeleteMany and
pkg/db/messaging/outgoing-emails.go:114) so only ctx and filter are passed.
In `@pkg/db/study/researcher-messages.go`:
- Around line 65-66: The code calls
dbService.collectionResearcherMessages(...).DeleteMany and immediately returns
res.DeletedCount without checking err; change the logic in the function that
performs this DeleteMany (referencing DeleteMany, collectionResearcherMessages
and the returned res.DeleteResult) to first check if err != nil and return 0,
err (or return the error) before reading res.DeletedCount; only return
res.DeletedCount when err is nil to match error-handling used in renew-tokens.go
and responses.go.
---
Nitpick comments:
In `@go.mod`:
- Line 11: The go.mod currently pins the MongoDB driver to
go.mongodb.org/mongo-driver/v2 v2.5.0; update the version to the latest v2 patch
(v2.5.1) or add a comment documenting why v2.5.0 is intentionally pinned. Locate
the module line for go.mongodb.org/mongo-driver/v2 in go.mod and either change
the version token to v2.5.1 or add a short inline comment explaining the
intentional pin to prevent accidental upgrades.
In `@pkg/db/management-user/db.go`:
- Around line 38-45: Remove the unused timeout context allocation: eliminate the
call to context.WithTimeout(...) and the following defer cancel() since the
returned context and cancel are never used around the mongo.Connect(...) call;
keep the mongo.Connect(...) invocation as-is and ensure the existing Ping(...)
call (which creates its own context) remains untouched. Specifically, remove the
variables created by context.WithTimeout and the defer cancel near the
mongo.Connect call (symbols: context.WithTimeout, cancel, mongo.Connect,
configs) and apply the same cleanup to the identical pattern in the other DB
files that create but never use that context.
In `@pkg/db/messaging/email-templates.go`:
- Around line 119-128: The code redundantly sets emailTemplate.ID =
bson.NewObjectID() before InsertOne and then reassigns emailTemplate.ID =
res.InsertedID.(bson.ObjectID) after insert; choose one approach and remove the
dead work: either remove the pre-insert assignment (leave ID generation to the
driver and keep using res.InsertedID from InsertOne) or keep the pre-insert
bson.NewObjectID() and drop the post-insert reassignment. Update the block
around InsertOne (symbols: emailTemplate.ID, bson.NewObjectID(), InsertOne,
res.InsertedID) accordingly so only one assignment remains.
In `@pkg/db/messaging/scheduled-email.go`:
- Around line 108-114: The local variable named options shadows the imported
options package in the FindOneAndReplace call; rename the local var to opts (or
similar) to avoid shadowing so package symbols like options.After continue to
refer to the imported package. Update the chain that currently does options :=
options.FindOneAndReplace()... to use opts := options.FindOneAndReplace()... and
ensure subsequent references in this block (e.g., the call in
dbService.collectionEmailSchedules(instanceID).FindOneAndReplace(ctx, filter,
scheduledEmail, opts).Decode(&elem)) use the new opts variable.
- Around line 17-25: The no-op branch in DropIndexForEmailSchedulesCollection
currently only logs a warning when dropAll is false; change this to either
silently early-return or, preferably, surface the no-op to callers by returning
a boolean (or error) indicating “nothing to drop” so callers can react instead
of relying on logs—update the DropIndexForEmailSchedulesCollection signature and
callers accordingly, and keep logging only for unexpected errors (retain the
slog.Error in the err path and remove/replace the slog.Warn in the no-op
branch); refer to DropIndexForEmailSchedulesCollection and
dbService.collectionEmailSchedules(instanceID) when locating the code to change.
In `@pkg/db/study/confidential-responses.go`:
- Around line 106-110: The Find call passing an explicit nil as the variadic
opts is non-idiomatic and can produce a one-element slice with a nil interface;
update the call to collectionConfidentialResponses(instanceID,
studyKey).Find(ctx, filter) (i.e., remove the trailing nil variadic argument) so
no nil entry is created; also scan for other usages of
collectionConfidentialResponses(...).Find or similar Collection.Find calls and
remove explicit nil variadic opts to rely on the zero-argument form.
🪄 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: CHILL
Plan: Pro
Run ID: cfbc1fe3-622c-45f1-9bd2-c7a9bba98831
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (87)
.gitignorego.modjobs/db-migration/main.gojobs/messaging/participant-messages.gojobs/messaging/scheduled-messages.gojobs/study-daily-data-export/response-exporter.gojobs/study-timer/main.gojobs/user-management/generate-id-lookup.gojobs/user-management/main.gopkg/apihelpers/parse-paginated-query.gopkg/db/global-infos/blocked-jwts.gopkg/db/global-infos/db.gopkg/db/global-infos/temptoken.gopkg/db/management-user/app-roles.gopkg/db/management-user/db.gopkg/db/management-user/management-users.gopkg/db/management-user/permissions.gopkg/db/management-user/service-users.gopkg/db/management-user/sessions.gopkg/db/management-user/types.gopkg/db/messaging/db.gopkg/db/messaging/email-templates.gopkg/db/messaging/outgoing-emails.gopkg/db/messaging/scheduled-email.gopkg/db/messaging/sent-emails.gopkg/db/messaging/sent-sms.gopkg/db/messaging/sms-templates.gopkg/db/participant-user/db.gopkg/db/participant-user/failedOtpAttempts.gopkg/db/participant-user/otps.gopkg/db/participant-user/renew-tokens.gopkg/db/participant-user/user-attributes.gopkg/db/participant-user/users.gopkg/db/study/confidential-id-map.gopkg/db/study/confidential-responses.gopkg/db/study/db.gopkg/db/study/participant-file-infos.gopkg/db/study/participants.gopkg/db/study/reports.gopkg/db/study/researcher-messages.gopkg/db/study/responses.gopkg/db/study/study-code-lists.gopkg/db/study/study-counters.gopkg/db/study/study-infos.gopkg/db/study/study-rules.gopkg/db/study/study-variables.gopkg/db/study/surveys.gopkg/db/study/task-queue.gopkg/db/utils.gopkg/messaging/types/email-template.gopkg/messaging/types/outgoing_email.gopkg/messaging/types/scheduledEmails.gopkg/messaging/types/sms.gopkg/study/participant-data.gopkg/study/study-service.gopkg/study/studyengine/actions.gopkg/study/studyengine/expressions.gopkg/study/studyengine/expressions_test.gopkg/study/studyengine/types.gopkg/study/types/assigned-survey.gopkg/study/types/localised-object.gopkg/study/types/participant-file-info.gopkg/study/types/participant.gopkg/study/types/report.gopkg/study/types/study-code-list.gopkg/study/types/study-message.gopkg/study/types/study-rules.gopkg/study/types/study-variables.gopkg/study/types/study.gopkg/study/types/survey-item.gopkg/study/types/survey-response.gopkg/study/types/survey.gopkg/study/types/task.gopkg/study/utils/participantID_test.gopkg/user-management/types/contact-info.gopkg/user-management/types/profile.gopkg/user-management/types/temptoken.gopkg/user-management/types/user-attributes.gopkg/user-management/types/user.gopkg/user-management/utils/new-user-init.gopkg/user-management/utils/profiles_test.goservices/management-api/apihandlers/study-management.goservices/participant-api/apihandlers/authentication.goservices/participant-api/apihandlers/password-reset.goservices/participant-api/apihandlers/study-service.goservices/participant-api/apihandlers/user-management.gotest/mongodb-driver-update.md
| _id, _ := bson.ObjectIDFromHex(emailID) | ||
| filter := bson.M{"_id": _id} | ||
| update := bson.M{"$set": bson.M{"lastSendAttempt": 0}} | ||
| res, err := dbService.collectionOutgoingEmails(instanceID).UpdateOne(ctx, filter, update) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '(_id|[a-zA-Z0-9_]+)\s*,\s*_\s*:=\s*bson\.ObjectIDFromHex\(' -C2Repository: case-framework/case-backend
Length of output: 1058
🏁 Script executed:
# Read the exact code from the file to confirm the issue
head -120 pkg/db/messaging/outgoing-emails.go | tail -35Repository: case-framework/case-backend
Length of output: 1058
🏁 Script executed:
# Check the context around the users.go instance
head -210 pkg/db/participant-user/users.go | tail -20Repository: case-framework/case-backend
Length of output: 645
🏁 Script executed:
# Verify if there are any other instances not caught by the regex
rg 'ObjectIDFromHex' -A 1 --no-headingRepository: case-framework/case-backend
Length of output: 7123
Handle invalid ObjectID input before building the Mongo filter.
bson.ObjectIDFromHex errors are currently discarded in three locations, allowing malformed IDs to convert into NilObjectID filters. Return a validation error instead of issuing operations with invalid IDs.
Affected functions:
ResetLastSendAttemptForOutgoing(pkg/db/messaging/outgoing-emails.go:94)DeleteOutgoingEmail(pkg/db/messaging/outgoing-emails.go:111)SavePasswordResetTrigger(pkg/db/participant-user/users.go:201)
🛡️ Proposed fixes
func (dbService *MessagingDBService) ResetLastSendAttemptForOutgoing(instanceID string, emailID string) error {
ctx, cancel := dbService.getContext()
defer cancel()
- _id, _ := bson.ObjectIDFromHex(emailID)
+ _id, err := bson.ObjectIDFromHex(emailID)
+ if err != nil {
+ return errors.New("invalid outgoing email id")
+ }
filter := bson.M{"_id": _id}
update := bson.M{"$set": bson.M{"lastSendAttempt": 0}}
res, err := dbService.collectionOutgoingEmails(instanceID).UpdateOne(ctx, filter, update) func (dbService *MessagingDBService) DeleteOutgoingEmail(instanceID string, id string) error {
ctx, cancel := dbService.getContext()
defer cancel()
- _id, _ := bson.ObjectIDFromHex(id)
+ _id, err := bson.ObjectIDFromHex(id)
+ if err != nil {
+ return errors.New("invalid outgoing email id")
+ }
filter := bson.M{"_id": _id} func (dbService *ParticipantUserDBService) SavePasswordResetTrigger(instanceID string, userID string) error {
ctx, cancel := dbService.getContext()
defer cancel()
- _id, _ := bson.ObjectIDFromHex(userID)
+ _id, err := bson.ObjectIDFromHex(userID)
+ if err != nil {
+ return errors.New("invalid user id")
+ }
filter := bson.M{"_id": _id}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _id, _ := bson.ObjectIDFromHex(emailID) | |
| filter := bson.M{"_id": _id} | |
| update := bson.M{"$set": bson.M{"lastSendAttempt": 0}} | |
| res, err := dbService.collectionOutgoingEmails(instanceID).UpdateOne(ctx, filter, update) | |
| _id, err := bson.ObjectIDFromHex(emailID) | |
| if err != nil { | |
| return errors.New("invalid outgoing email id") | |
| } | |
| filter := bson.M{"_id": _id} | |
| update := bson.M{"$set": bson.M{"lastSendAttempt": 0}} | |
| res, err := dbService.collectionOutgoingEmails(instanceID).UpdateOne(ctx, filter, update) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/messaging/outgoing-emails.go` around lines 94 - 97, In the three
functions ResetLastSendAttemptForOutgoing, DeleteOutgoingEmail
(pkg/db/messaging/outgoing-emails.go) and SavePasswordResetTrigger
(pkg/db/participant-user/users.go) stop discarding the error returned by
bson.ObjectIDFromHex: validate the emailID/input by checking the returned error,
and if non-nil return a clear validation error to the caller instead of building
a filter with the NilObjectID; only construct the filter (e.g., bson.M{"_id":
_id}) and call UpdateOne/DeleteOne/other DB ops when ObjectIDFromHex succeeded
so malformed IDs never generate NilObjectID queries.
| _id, _ := bson.ObjectIDFromHex(userID) | ||
| filter := bson.M{"_id": _id} | ||
| update := bson.M{"$push": bson.M{"account.passwordResetTriggers": time.Now().Unix()}} | ||
| _, err := dbService.collectionParticipantUsers(instanceID).UpdateOne(ctx, filter, update) |
There was a problem hiding this comment.
Return the ObjectID parse error instead of silently using the zero ID.
Line 201 ignores ObjectIDFromHex errors, so malformed userID values can become _id: ObjectID("000...") and report success after updating nothing.
🐛 Proposed fix
- _id, _ := bson.ObjectIDFromHex(userID)
+ _id, err := bson.ObjectIDFromHex(userID)
+ if err != nil {
+ return err
+ }
filter := bson.M{"_id": _id}
update := bson.M{"$push": bson.M{"account.passwordResetTriggers": time.Now().Unix()}}
- _, err := dbService.collectionParticipantUsers(instanceID).UpdateOne(ctx, filter, update)
+ _, err = dbService.collectionParticipantUsers(instanceID).UpdateOne(ctx, filter, update)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _id, _ := bson.ObjectIDFromHex(userID) | |
| filter := bson.M{"_id": _id} | |
| update := bson.M{"$push": bson.M{"account.passwordResetTriggers": time.Now().Unix()}} | |
| _, err := dbService.collectionParticipantUsers(instanceID).UpdateOne(ctx, filter, update) | |
| _id, err := bson.ObjectIDFromHex(userID) | |
| if err != nil { | |
| return err | |
| } | |
| filter := bson.M{"_id": _id} | |
| update := bson.M{"$push": bson.M{"account.passwordResetTriggers": time.Now().Unix()}} | |
| _, err = dbService.collectionParticipantUsers(instanceID).UpdateOne(ctx, filter, update) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/participant-user/users.go` around lines 201 - 204, The code currently
ignores the error from bson.ObjectIDFromHex(userID), which can produce a zero
ObjectID and make UpdateOne report success for no-op updates; modify the logic
around bson.ObjectIDFromHex in the function using userID (the _id variable) to
check the returned err immediately, and if non-nil return or propagate that
error instead of continuing; only build the filter (bson.M{"_id": _id}) and call
dbService.collectionParticipantUsers(instanceID).UpdateOne(ctx, filter, update)
when the ObjectID parse succeeded.
|
|
||
| } | ||
| newUser.ID, _ = primitive.ObjectIDFromHex(id) | ||
| newUser.ID, _ = bson.ObjectIDFromHex(id) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the authentication.go file
fd -t f "authentication.go" | head -5Repository: case-framework/case-backend
Length of output: 125
🏁 Script executed:
# Find the file and check its line count
find . -name "authentication.go" -path "*/participant-api/apihandlers/*" -type fRepository: case-framework/case-backend
Length of output: 127
🏁 Script executed:
# Get the file size and read around line 312
wc -l ./services/participant-api/apihandlers/authentication.goRepository: case-framework/case-backend
Length of output: 132
🏁 Script executed:
# Read lines around 312 to see the context
sed -n '300,340p' ./services/participant-api/apihandlers/authentication.goRepository: case-framework/case-backend
Length of output: 1242
🏁 Script executed:
# Search for the AddUser method to understand what it returns
rg "func.*AddUser" --type go -A 5 | head -40Repository: case-framework/case-backend
Length of output: 1731
🏁 Script executed:
# Check for other usages of ObjectIDFromHex in the codebase to see error handling patterns
rg "ObjectIDFromHex" --type go -B 2 -A 2 | head -60Repository: case-framework/case-backend
Length of output: 3160
🏁 Script executed:
# Get more of AddUser implementation
sed -n '1,50p' ./pkg/db/participant-user/users.go | head -30Repository: case-framework/case-backend
Length of output: 1021
🏁 Script executed:
# Find the AddUser function and see full implementation
rg -A 30 "func.*ParticipantUserDBService.*AddUser" ./pkg/db/participant-user/users.goRepository: case-framework/case-backend
Length of output: 896
🏁 Script executed:
# Verify if there's any documentation or type hints about AddUser return format
rg -B 5 "func.*AddUser.*string.*error" ./pkg/db/participant-user/users.goRepository: case-framework/case-backend
Length of output: 322
🏁 Script executed:
# Check if there are any tests for AddUser that show expected behavior
fd -t f "*test*.go" ./pkg/db/participant-user/ -x grep -l "AddUser" {}Repository: case-framework/case-backend
Length of output: 359
🏁 Script executed:
# Search for tests with correct glob syntax
fd --glob "*test.go" ./pkg/db/participant-user/ -x grep -l "AddUser" {}Repository: case-framework/case-backend
Length of output: 53
🏁 Script executed:
# Also check if AddUser is tested inline or in another location
rg "AddUser" --type go -l ./pkg/db/participant-user/ | grep -i testRepository: case-framework/case-backend
Length of output: 53
🏁 Script executed:
# Check what zero ObjectID.Hex() produces and verify downstream impact
# Look at the email verification and JWT generation functions
rg -A 10 "prepAndSendEmailVerification" ./services/participant-api/apihandlers/authentication.go | head -20Repository: case-framework/case-backend
Length of output: 561
🏁 Script executed:
# Check GenerateNewParticipantUserToken to see if it validates the ID
rg "GenerateNewParticipantUserToken" --type go -B 2 -A 8 | head -40Repository: case-framework/case-backend
Length of output: 2809
Add error handling for ObjectIDFromHex to match codebase patterns.
Line 312 discards the parse error from bson.ObjectIDFromHex(id). While AddUser returns a valid hex string (derived directly from MongoDB's ObjectID.Hex()), this is the only place in the codebase where this error is ignored. All other ObjectIDFromHex calls check and handle the error. This inconsistency should be corrected for consistency, defensive programming, and to prevent silent failures if the source of id changes.
Proposed fix
- newUser.ID, _ = bson.ObjectIDFromHex(id)
+ newUser.ID, err = bson.ObjectIDFromHex(id)
+ if err != nil {
+ slog.Error("failed to parse new user id", slog.String("id", id), slog.String("error", err.Error()))
+ c.JSON(http.StatusInternalServerError, gin.H{"error": "internal server error"})
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| newUser.ID, _ = bson.ObjectIDFromHex(id) | |
| newUser.ID, err = bson.ObjectIDFromHex(id) | |
| if err != nil { | |
| slog.Error("failed to parse new user id", slog.String("id", id), slog.String("error", err.Error())) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "internal server error"}) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/participant-api/apihandlers/authentication.go` at line 312, The call
newUser.ID, _ = bson.ObjectIDFromHex(id) ignores the parse error; change this to
capture the error (idObj, err := bson.ObjectIDFromHex(id)) and handle it the
same way other ObjectIDFromHex uses do in this codebase: if err != nil,
log/return an appropriate error response and stop further processing (e.g.,
return a 400/invalid-id or propagate the error from the handler), then assign
newUser.ID = idObj; ensure you use the same error wrapping/logging style as
surrounding handlers to keep behavior consistent with AddUser and other callers.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/db/study/confidential-responses.go (1)
72-74:⚠️ Potential issue | 🟠 MajorCheck
InsertOneerrors before readingInsertedID.Line 73 still dereferences
resbefore Line 74 returnserr; ifInsertOnefails, this can panic instead of returning the database error. This is the same unresolved issue noted earlier.🐛 Proposed fix
res, err := dbService.collectionConfidentialResponses(instanceID, studyKey).InsertOne(ctx, response) - id := res.InsertedID.(bson.ObjectID) - return id.Hex(), err + if err != nil { + return "", err + } + id, ok := res.InsertedID.(bson.ObjectID) + if !ok { + return "", errors.New("unexpected InsertedID type") + } + return id.Hex(), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/study/confidential-responses.go` around lines 72 - 74, The InsertOne call's result is dereferenced before checking for errors; in the method using dbService.collectionConfidentialResponses(...).InsertOne(ctx, response) you must check err immediately after the call and return the error (e.g., return "", err) if non-nil, only then access res.InsertedID and convert it to a hex string; update the code around the InsertOne call so you don't do res.InsertedID.(bson.ObjectID) when err != nil to avoid a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/db/study/confidential-responses.go`:
- Around line 72-74: The InsertOne call's result is dereferenced before checking
for errors; in the method using
dbService.collectionConfidentialResponses(...).InsertOne(ctx, response) you must
check err immediately after the call and return the error (e.g., return "", err)
if non-nil, only then access res.InsertedID and convert it to a hex string;
update the code around the InsertOne call so you don't do
res.InsertedID.(bson.ObjectID) when err != nil to avoid a panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60306ab1-9d43-458e-a971-89ce333c9978
📒 Files selected for processing (1)
pkg/db/study/confidential-responses.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/db/study/reports.go (1)
72-76: Dead empty-name guard.
defaultReportIndexNamesis a package-level slice of non-empty string constants, soindexName == ""can never be true here. The check (and its error log) can be removed to simplify the loop.♻️ Proposed simplification
for _, indexName := range defaultReportIndexNames { - if indexName == "" { - slog.Error("Index name is empty for reports collection") - continue - } err := collection.Indexes().DropOne(ctx, indexName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/study/reports.go` around lines 72 - 76, The empty-name guard inside the loop over defaultReportIndexNames is dead code because defaultReportIndexNames contains non-empty constants; remove the conditional that checks indexName == "" and the accompanying slog.Error call to simplify the loop in the code that builds indexes for the reports collection (the loop iterating over defaultReportIndexNames in reports.go).pkg/db/messaging/scheduled-email.go (1)
101-125: LGTM — v2 options builder andbson.ObjectIDusage.The fluent
options.FindOneAndReplace().SetUpsert(false).SetReturnDocument(options.After)andbson.NewObjectID()/res.InsertedID.(bson.ObjectID)cast all match the v2 API surface. Call sites (e.g.,jobs/messaging/scheduled-messages.go:56-58) already consume the(ScheduledEmail, error)return shape unchanged, so this migration is transparent to callers.Minor nit (optional):
scheduledEmail.ID = bson.NewObjectID()on line 117 is redundant because it's immediately overwritten by theInsertedIDcast on line 122. You could drop one of the two assignments, but behavior is equivalent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/messaging/scheduled-email.go` around lines 101 - 125, Remove the redundant temporary ID assignment in SaveScheduledEmail: the call scheduledEmail.ID = bson.NewObjectID() before InsertOne is unnecessary because the inserted ID is read back from res.InsertedID and assigned to scheduledEmail.ID; delete the bson.NewObjectID() assignment in the else branch of MessagingDBService.SaveScheduledEmail so only the InsertOne result is used to set scheduledEmail.ID.pkg/db/global-infos/db.go (1)
39-45: Minor:defer conCancel()placed afterPing.Convention is to register
defer cancel()immediately aftercontext.WithTimeoutso the cancel is guaranteed to run even if the intervening call panics. HerePingis synchronous and can't actually leave the timer dangling, so it's not a real bug — just a readability nit.♻️ Suggested reorder
- ctx, conCancel := context.WithTimeout(context.Background(), time.Duration(configs.Timeout)*time.Second) - err = dbClient.Ping(ctx, nil) - defer conCancel() + ctx, conCancel := context.WithTimeout(context.Background(), time.Duration(configs.Timeout)*time.Second) + defer conCancel() + err = dbClient.Ping(ctx, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/global-infos/db.go` around lines 39 - 45, Move the defer call to ensure the cancel runs even if a later call panics: after creating the context with context.WithTimeout (the ctx, conCancel := context.WithTimeout(...) line) immediately add defer conCancel() and then call dbClient.Ping(ctx, nil); this updates the ordering around ctx/conCancel and dbClient.Ping to register the cancel right away.test/mongodb-driver-update.md (1)
376-376: Optional style adjustment (split infinitive).Static analysis flagged "used to manually assign" as a split infinitive. While grammatically acceptable in modern technical writing, you could optionally rephrase to "used to assign a survey manually" if you prefer to avoid split infinitives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mongodb-driver-update.md` at line 376, Replace the phrase "used to manually assign a survey to a participant" in the sentence "Opened the CASE management UI and checked the survey dropdown used to manually assign a survey to a participant – verified that all available survey keys were listed correctly." with a non–split-infinitive phrasing such as "used to assign a survey manually to a participant" (or "used to assign a survey manually") so the sentence reads without the split infinitive while preserving the original meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/mongodb-driver-update.md`:
- Around line 1-73: Update all incorrect type identifiers: replace occurrences
of primitive.ObjectID and the mistaken bson.ObjectID with the correct MongoDB Go
v2 type name bson.ObjectId throughout the codebase (any code that previously
migrated primitive.ObjectID → bson.ObjectID must instead use bson.ObjectId).
Search for uses in constructors, struct fields, function signatures and
conversions (e.g., places referenced by names like SaveParticipantState,
GetSurveyVersions, ReplaceConfidentialResponse, AddUser, CreateOTP) and update
imports if necessary so the bson package is used where these types appear.
---
Nitpick comments:
In `@pkg/db/global-infos/db.go`:
- Around line 39-45: Move the defer call to ensure the cancel runs even if a
later call panics: after creating the context with context.WithTimeout (the ctx,
conCancel := context.WithTimeout(...) line) immediately add defer conCancel()
and then call dbClient.Ping(ctx, nil); this updates the ordering around
ctx/conCancel and dbClient.Ping to register the cancel right away.
In `@pkg/db/messaging/scheduled-email.go`:
- Around line 101-125: Remove the redundant temporary ID assignment in
SaveScheduledEmail: the call scheduledEmail.ID = bson.NewObjectID() before
InsertOne is unnecessary because the inserted ID is read back from
res.InsertedID and assigned to scheduledEmail.ID; delete the bson.NewObjectID()
assignment in the else branch of MessagingDBService.SaveScheduledEmail so only
the InsertOne result is used to set scheduledEmail.ID.
In `@pkg/db/study/reports.go`:
- Around line 72-76: The empty-name guard inside the loop over
defaultReportIndexNames is dead code because defaultReportIndexNames contains
non-empty constants; remove the conditional that checks indexName == "" and the
accompanying slog.Error call to simplify the loop in the code that builds
indexes for the reports collection (the loop iterating over
defaultReportIndexNames in reports.go).
In `@test/mongodb-driver-update.md`:
- Line 376: Replace the phrase "used to manually assign a survey to a
participant" in the sentence "Opened the CASE management UI and checked the
survey dropdown used to manually assign a survey to a participant – verified
that all available survey keys were listed correctly." with a
non–split-infinitive phrasing such as "used to assign a survey manually to a
participant" (or "used to assign a survey manually") so the sentence reads
without the split infinitive while preserving the original meaning.
🪄 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: CHILL
Plan: Pro
Run ID: 889b2009-6500-48ea-b1a5-18bb1eb68e85
📒 Files selected for processing (10)
pkg/db/global-infos/db.gopkg/db/management-user/db.gopkg/db/messaging/db.gopkg/db/messaging/scheduled-email.gopkg/db/participant-user/db.gopkg/db/participant-user/renew-tokens.gopkg/db/study/confidential-responses.gopkg/db/study/db.gopkg/db/study/reports.gotest/mongodb-driver-update.md
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/db/study/db.go
- pkg/db/participant-user/db.go
- pkg/db/participant-user/renew-tokens.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/db/study/confidential-responses.go (1)
42-46: Dead-code guard on hardcoded constants.Since
defaultConfidentialResponseIndexNamesis populated exclusively from the non-empty constantidxConfidentialResponsesParticipantIDKey, theif indexName == ""branch is unreachable. Safe to drop for clarity (or keep if you want symmetry with other collections using the same template).♻️ Proposed simplification
for _, indexName := range defaultConfidentialResponseIndexNames { - if indexName == "" { - slog.Error("Index name is empty for confidential responses collection") - continue - } err := collection.Indexes().DropOne(ctx, indexName) if err != nil { slog.Error("Error dropping index for confidential responses", slog.String("error", err.Error()), slog.String("instanceID", instanceID), slog.String("studyKey", studyKey), slog.String("indexName", indexName)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/study/confidential-responses.go` around lines 42 - 46, The loop over defaultConfidentialResponseIndexNames contains an unreachable guard checking indexName == "" because the slice is built only from the non-empty constant idxConfidentialResponsesParticipantIDKey; remove the dead if branch and its slog.Error call to simplify the code in the loop that constructs/confirms indexes (the loop using defaultConfidentialResponseIndexNames), or alternatively leave a short comment explaining symmetry if you prefer to keep it—either remove the unreachable check or replace it with a clarifying comment.pkg/db/study/responses.go (2)
284-288: Nit: finalreturn erris alwaysnilhere (and inDeleteResponsesat line 318).
errhas already been checked and returned on the error path above, and theDeletedCount == 0branch returnsmongo.ErrNoDocumentsdirectly, so the trailingreturn erris guaranteed to benil. Prefer an explicitreturn nilto make intent unambiguous; the same applies toDeleteResponses(line 318).🧹 Proposed tidy-up
if res.DeletedCount == 0 { return mongo.ErrNoDocuments } - - return err + return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/study/responses.go` around lines 284 - 288, The final "return err" is misleading because err is always nil at that point; update the functions DeleteResponse (the block checking res.DeletedCount == 0) and DeleteResponses to explicitly return nil instead of returning err so the intent is clear and there is no confusion about returning a non-nil error value.
77-86: Remove dead empty-name guard inDropIndexForResponsesCollection.Every entry in
defaultResponseIndexNamesis a non-empty string constant, so theif indexName == ""check at lines 78–81 is unreachable. Remove it.The v2 driver signatures for
IndexView.DropOneandIndexView.DropAllhave been confirmed: both returnerroronly (nobson.Raw), matching the code's usage at lines 72 and 82.Proposed cleanup
} else { for _, indexName := range defaultResponseIndexNames { - if indexName == "" { - slog.Error("Index name is empty for responses collection") - continue - } err := dbService.collectionResponses(instanceID, studyKey).Indexes().DropOne(ctx, indexName) if err != nil { slog.Error("Error dropping index for responses", slog.String("error", err.Error()), slog.String("instanceID", instanceID), slog.String("studyKey", studyKey), slog.String("indexName", indexName)) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/study/responses.go` around lines 77 - 86, Remove the unreachable empty-name guard in DropIndexForResponsesCollection: delete the if indexName == "" { slog.Error(...); continue } block inside the loop over defaultResponseIndexNames and leave the existing call to dbService.collectionResponses(instanceID, studyKey).Indexes().DropOne(ctx, indexName) and its error logging intact; this keeps behavior unchanged while removing dead code around the defaultResponseIndexNames check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/db/study/confidential-responses.go`:
- Around line 42-46: The loop over defaultConfidentialResponseIndexNames
contains an unreachable guard checking indexName == "" because the slice is
built only from the non-empty constant idxConfidentialResponsesParticipantIDKey;
remove the dead if branch and its slog.Error call to simplify the code in the
loop that constructs/confirms indexes (the loop using
defaultConfidentialResponseIndexNames), or alternatively leave a short comment
explaining symmetry if you prefer to keep it—either remove the unreachable check
or replace it with a clarifying comment.
In `@pkg/db/study/responses.go`:
- Around line 284-288: The final "return err" is misleading because err is
always nil at that point; update the functions DeleteResponse (the block
checking res.DeletedCount == 0) and DeleteResponses to explicitly return nil
instead of returning err so the intent is clear and there is no confusion about
returning a non-nil error value.
- Around line 77-86: Remove the unreachable empty-name guard in
DropIndexForResponsesCollection: delete the if indexName == "" {
slog.Error(...); continue } block inside the loop over defaultResponseIndexNames
and leave the existing call to dbService.collectionResponses(instanceID,
studyKey).Indexes().DropOne(ctx, indexName) and its error logging intact; this
keeps behavior unchanged while removing dead code around the
defaultResponseIndexNames check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a795838-0104-495e-8722-208c0b9c810d
📒 Files selected for processing (3)
pkg/db/messaging/outgoing-emails.gopkg/db/study/confidential-responses.gopkg/db/study/responses.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/mongodb-driver-update.md (1)
7-10:⚠️ Potential issue | 🟠 MajorFix MongoDB Go v2 BSON ObjectId casing in migration notes.
Your doc says: “primitive.ObjectID → bson.ObjectID”, but the Go driver v2 type is
bson.ObjectId(capital O, lowercase “id”). This casing mistake can mislead implementers and cause compilation failures if followed. citeturn0search0🛠️ Proposed doc-only fix
- - **primitive package**: The bson/primitive package has been merged into the bson package —> changed any instance of primitive.ObjectID to bson.ObjectID. + - **primitive package**: The bson/primitive package has been merged into the bson package —> changed any instance of primitive.ObjectID to bson.ObjectId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mongodb-driver-update.md` around lines 7 - 10, The migration note incorrectly shows the Go BSON type as "bson.ObjectID"; update the doc to use the correct Go driver v2 casing "bson.ObjectId" (and show the original "primitive.ObjectID" → "bson.ObjectId" mapping) so readers don’t follow a typo that causes compile errors; search for occurrences of "primitive.ObjectID" and "bson.ObjectID" in the doc and replace the latter with "bson.ObjectId" while leaving the explanatory text intact.
🧹 Nitpick comments (1)
test/mongodb-driver-update.md (1)
372-377: Optional readability: simplify the “Opened … – verified …” sentence.The GetSurveyKeysForStudy “Scope/Test execution” bullet is a bit clunky with the dash + “verified …”. Consider a smoother phrasing for readability.
💡 Suggested wording
- 1. Opened the CASE management UI and checked the survey dropdown used to manually assign a survey to a participant – verified that all available survey keys were listed correctly. + 1. Opened the CASE management UI and checked the survey dropdown used to manually assign a survey to a participant, verifying that all available survey keys were listed correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mongodb-driver-update.md` around lines 372 - 377, Simplify the clunky “Opened … – verified …” sentence under the GetSurveyKeysForStudy Scope/Test execution bullet: replace the dash+verification phrasing with a single smoother sentence such as “Opened the CASE management UI and confirmed the survey dropdown used to manually assign a survey to a participant lists all available survey keys.” Keep the second step about exporting the study configuration unchanged; update the Scope/Test execution bullet for readability only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/mongodb-driver-update.md`:
- Around line 7-10: The migration note incorrectly shows the Go BSON type as
"bson.ObjectID"; update the doc to use the correct Go driver v2 casing
"bson.ObjectId" (and show the original "primitive.ObjectID" → "bson.ObjectId"
mapping) so readers don’t follow a typo that causes compile errors; search for
occurrences of "primitive.ObjectID" and "bson.ObjectID" in the doc and replace
the latter with "bson.ObjectId" while leaving the explanatory text intact.
---
Nitpick comments:
In `@test/mongodb-driver-update.md`:
- Around line 372-377: Simplify the clunky “Opened … – verified …” sentence
under the GetSurveyKeysForStudy Scope/Test execution bullet: replace the
dash+verification phrasing with a single smoother sentence such as “Opened the
CASE management UI and confirmed the survey dropdown used to manually assign a
survey to a participant lists all available survey keys.” Keep the second step
about exporting the study configuration unchanged; update the Scope/Test
execution bullet for readability only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6f29b1b-669c-41ca-8f4e-d02a51ab2c9f
📒 Files selected for processing (1)
test/mongodb-driver-update.md
1 participant
Update mongodb driver: v1 → v2
For detailed changes and test protocol see test/mongodb-driver-update.md
Summary by CodeRabbit