update mongo driver - #376
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates the codebase to the MongoDB Go driver v2: updating import paths, replacing legacy Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
connectors/mongo/conn.go (1)
954-965:⚠️ Potential issue | 🟡 MinorDisconnect the client when
Pingfails.If
Pingfails aftermongo.Connectsucceeds, the function returns without callingclient.Disconnect(), leaving the client resource undisposed on the startup error path. The MongoDB v2 driver requires explicit disconnection to clean up resources.Also, line 967 should return
nilinstead oferron success (err is nil at that point).🛠️ Suggested fix
client, err := mongo.Connect(clientOptions) if err != nil { return nil, err } // Check the connection ctxPing, cancelPing := context.WithTimeout(ctx, settings.PingTimeout) defer cancelPing() err = client.Ping(ctxPing, nil) if err != nil { + _ = client.Disconnect(context.Background()) return nil, err } - return client, err + return client, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/mongo/conn.go` around lines 954 - 965, If mongo.Connect succeeds but client.Ping fails, the code currently returns without disconnecting the client; update the Ping error path to call client.Disconnect(...) (e.g., client.Disconnect(context.Background())) and handle or log any disconnect error before returning the Ping error so resources are cleaned up; also correct the successful return to return the connected client and nil error (replace returning err with nil) so the function returns (client, nil) on success.
🧹 Nitpick comments (2)
connectors/cosmos/check_deletes.go (1)
181-181: Consider adding defensive type assertion handling formissingIdsLine 181 uses an unchecked type assertion to
bson.A. While the MongoDB driver v2 guarantees BSON arrays decode asbson.Aunder standard codec configuration, defensive handling prevents panic if a custom registry differs from the default behavior.The existing nil check at line 175 provides some safety, but the type assertion itself remains unguarded.
Suggested defensive fix
- missingIds = []interface{}(res["missingIds"].(bson.A)) + switch arr := res["missingIds"].(type) { + case bson.A: + missingIds = []interface{}(arr) + case []interface{}: + missingIds = arr + default: + slog.Error(fmt.Sprintf("Unexpected missingIds type: %T", res["missingIds"])) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/cosmos/check_deletes.go` at line 181, The unchecked type assertion on res["missingIds"] to bson.A can panic if a custom codec returns a different type; update the code around the missingIds assignment in checkDeletes (variable missingIds and map res) to use a guarded type assertion (ok idiom) or a type switch: attempt to cast to bson.A, else handle []interface{} or nil cases, and fall back to constructing missingIds safely (e.g., iterate and append elements) or return an error; ensure the subsequent code uses the safely constructed missingIds variable.connectors/postgres/bson_conv.go (1)
10-15: Make nested document decoding explicit.Without an explicit decoder mode, embedded docs inside
map[string]interface{}still decode asbson.Din v2. If callers expect recursive plain maps, opt intoDefaultDocumentMap()on v2.5+ or set the equivalentTypeEmbeddedDocumentmapping in the registry. (pkg.go.dev)♻️ Suggested tweak (v2.5+)
func bsonToMap(reg *bson.Registry, data []byte) (map[string]interface{}, error) { var m map[string]interface{} dec := bson.NewDecoder(bson.NewDocumentReader(bytes.NewReader(data))) dec.SetRegistry(reg) + dec.DefaultDocumentMap() if err := dec.Decode(&m); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/postgres/bson_conv.go` around lines 10 - 15, bsonToMap currently decodes embedded documents into bson.D because the registry lacks the explicit document-map mapping; update bsonToMap to opt into recursive plain maps by using a registry built with the DefaultDocumentMap option (or by setting the TypeEmbeddedDocument mapping in the provided registry) before calling dec.SetRegistry(reg) so nested documents decode into map[string]interface{} instead of bson.D; target the bsonToMap function and adjust registry construction/registration accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@connectors/dynamodb/conv.go`:
- Around line 117-123: In the case handling *types.AttributeValueMemberBS in
connectors/dynamodb/conv.go (inside the conversion function), remove the extra
nil being appended so each DynamoDB BS element appends only the bson.Binary
value; replace the current append(arr, bson.Binary{Subtype:
bson.TypeBinaryGeneric, Data: v}, nil) with a single-value append like
append(arr, bson.Binary{Subtype: bson.TypeBinaryGeneric, Data: v}) so binary
sets no longer get interleaved nils.
In `@connectors/s3vector/conn.go`:
- Line 117: The code currently casts first.GetType() (a uint32) directly to
bson.Type (a byte) before calling bson.UnmarshalValue in function/method around
conn.go, which can silently truncate values >255; add explicit range validation:
read the uint32 via first.GetType(), if it is >255 or less than 0 (checking
upper bound is sufficient for unsigned) return an error (or propagate)
indicating invalid BSON type, otherwise cast to bson.Type and proceed to call
bson.UnmarshalValue(&idAny) as before; update the error path where
bson.UnmarshalValue(bson.Type(first.GetType()), first.GetData(), &idAny) is used
so invalid type values fail fast with a clear error message.
---
Outside diff comments:
In `@connectors/mongo/conn.go`:
- Around line 954-965: If mongo.Connect succeeds but client.Ping fails, the code
currently returns without disconnecting the client; update the Ping error path
to call client.Disconnect(...) (e.g., client.Disconnect(context.Background()))
and handle or log any disconnect error before returning the Ping error so
resources are cleaned up; also correct the successful return to return the
connected client and nil error (replace returning err with nil) so the function
returns (client, nil) on success.
---
Nitpick comments:
In `@connectors/cosmos/check_deletes.go`:
- Line 181: The unchecked type assertion on res["missingIds"] to bson.A can
panic if a custom codec returns a different type; update the code around the
missingIds assignment in checkDeletes (variable missingIds and map res) to use a
guarded type assertion (ok idiom) or a type switch: attempt to cast to bson.A,
else handle []interface{} or nil cases, and fall back to constructing missingIds
safely (e.g., iterate and append elements) or return an error; ensure the
subsequent code uses the safely constructed missingIds variable.
In `@connectors/postgres/bson_conv.go`:
- Around line 10-15: bsonToMap currently decodes embedded documents into bson.D
because the registry lacks the explicit document-map mapping; update bsonToMap
to opt into recursive plain maps by using a registry built with the
DefaultDocumentMap option (or by setting the TypeEmbeddedDocument mapping in the
provided registry) before calling dec.SetRegistry(reg) so nested documents
decode into map[string]interface{} instead of bson.D; target the bsonToMap
function and adjust registry construction/registration 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d37048a9-c65b-4df6-9761-3b1091e5a72e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
connectors/airbyte/helpers.goconnectors/common/base.goconnectors/common/hash_test.goconnectors/cosmos/change_streams.goconnectors/cosmos/check_deletes.goconnectors/cosmos/conn.goconnectors/cosmos/connector_test.goconnectors/cosmos/planner.goconnectors/cosmos/range_partitioning.goconnectors/cosmos/token_map.goconnectors/cosmos/util.goconnectors/dynamodb/conv.goconnectors/file/connector.goconnectors/file/connector_test.goconnectors/file/util.goconnectors/mongo/conn.goconnectors/mongo/connector_test.goconnectors/mongo/docdb.goconnectors/mongo/planner.goconnectors/mongo/util.goconnectors/null/connector.goconnectors/postgres/bson_conv.goconnectors/postgres/bson_registry.goconnectors/postgres/conn.goconnectors/random/connv2.goconnectors/random/util.goconnectors/s3/connector.goconnectors/s3vector/conn.goconnectors/sqlbatch/cs.goconnectors/testconn/connector.goconnectors/vector/conn.gogo.modinternal/app/verify.gopkg/test/connector.goprotocol/test/connector_testsuite.gostatestores/mongo/statestore.go
| var res string | ||
| var idAny any | ||
| if err := bson.UnmarshalValue(bsontype.Type(first.GetType()), first.GetData(), &idAny); err != nil { | ||
| if err := bson.UnmarshalValue(bson.Type(first.GetType()), first.GetData(), &idAny); err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In go.mongodb.org/mongo-driver/v2/bson, what is the underlying type and valid numeric range for bson.Type, and is converting from uint32 to bson.Type potentially lossy?
💡 Result:
In go.mongodb.org/mongo-driver/v2/bson, bson.Type is a type alias for byte (type Type byte). Its valid numeric range corresponds to defined BSON type constants: standard types from 0x01 (Double) to 0x13 (Decimal128), plus MinKey (0xFF) and MaxKey (0x7F). Values outside these are invalid per the bsontype.IsValid method (deprecated but indicative). Converting from uint32 to bson.Type is potentially lossy because uint32 (0 to 4,294,967,295) exceeds byte's range (0 to 255), truncating values >255.
Citations:
- 1: https://pkg.go.dev/go.mongodb.org/mongo-driver/v2/bson
- 2: https://pkg.go.dev/go.mongodb.org/mongo-driver/bson/bsontype
- 3: https://www.mongodb.com/docs/manual/reference/bson-types/
🏁 Script executed:
cat -n connectors/s3vector/conn.go | sed -n '100,130p'Repository: adiom-data/dsync
Length of output: 1023
🏁 Script executed:
# Search for the definition of GetType() to understand what type it returns
rg "GetType\(\)" connectors/s3vector/conn.go -A 2 -B 2Repository: adiom-data/dsync
Length of output: 704
🏁 Script executed:
# Find the type of 'first' variable to understand the full context
ast-grep --pattern 'first := $_'Repository: adiom-data/dsync
Length of output: 108
🏁 Script executed:
fd -e proto -e go | xargs grep -l "BsonValue" | head -20Repository: adiom-data/dsync
Length of output: 631
🏁 Script executed:
rg "type BsonValue" --type go -A 3Repository: adiom-data/dsync
Length of output: 459
🏁 Script executed:
rg "func.*GetType" adiomv1 --type go -A 1Repository: adiom-data/dsync
Length of output: 107
🏁 Script executed:
rg "func.*GetType\(\)" gen/adiom/v1/messages.pb.go -A 2Repository: adiom-data/dsync
Length of output: 618
🏁 Script executed:
head -30 connectors/s3vector/conn.go | grep -E "^import|^$|^\s+\"fmt\""Repository: adiom-data/dsync
Length of output: 78
🏁 Script executed:
cat -n connectors/s3vector/conn.go | sed -n '1,20p'Repository: adiom-data/dsync
Length of output: 794
Add range validation before converting uint32 to bson.Type.
Line 117 converts first.GetType() (which returns uint32 from protobuf) directly to bson.Type (which is a byte with range 0–255). Values above 255 silently truncate without error. Add an explicit check to fail fast on invalid BSON type values:
Proposed fix
- if err := bson.UnmarshalValue(bson.Type(first.GetType()), first.GetData(), &idAny); err != nil {
+ t := first.GetType()
+ if t > 255 {
+ return "", fmt.Errorf("unsupported bson type value: %d", t)
+ }
+ if err := bson.UnmarshalValue(bson.Type(t), first.GetData(), &idAny); err != nil {
return "", fmt.Errorf("err unmarshalling id: %w", err)
}📝 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.
| if err := bson.UnmarshalValue(bson.Type(first.GetType()), first.GetData(), &idAny); err != nil { | |
| t := first.GetType() | |
| if t > 255 { | |
| return "", fmt.Errorf("unsupported bson type value: %d", t) | |
| } | |
| if err := bson.UnmarshalValue(bson.Type(t), first.GetData(), &idAny); err != nil { | |
| return "", fmt.Errorf("err unmarshalling id: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connectors/s3vector/conn.go` at line 117, The code currently casts
first.GetType() (a uint32) directly to bson.Type (a byte) before calling
bson.UnmarshalValue in function/method around conn.go, which can silently
truncate values >255; add explicit range validation: read the uint32 via
first.GetType(), if it is >255 or less than 0 (checking upper bound is
sufficient for unsigned) return an error (or propagate) indicating invalid BSON
type, otherwise cast to bson.Type and proceed to call
bson.UnmarshalValue(&idAny) as before; update the error path where
bson.UnmarshalValue(bson.Type(first.GetType()), first.GetData(), &idAny) is used
so invalid type values fail fast with a clear error message.
1 participant
Summary by CodeRabbit