Conversation
📝 WalkthroughWalkthroughAdded a new Firestore connector with URI parsing, batched sink write operations supporting BSON and JSON type conversion, comprehensive unit and emulator integration tests, CLI registration and flags, and updated module dependencies to support Firestore client libraries. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Connector as Firestore Connector
participant BulkWriter as Bulk Writer
participant Firestore as Firestore Service
Client->>Connector: WriteData(items)
Connector->>Connector: Parse URI, validate settings
Connector->>Connector: Extract document IDs, convert BSON/JSON
Connector->>BulkWriter: Add Set/Delete operations (batching)
BulkWriter->>Firestore: Commit batch
Firestore-->>BulkWriter: Commit result
BulkWriter-->>Connector: Job result / errors
Connector-->>Client: Return success/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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: 3
🧹 Nitpick comments (4)
connectors/firestore/connector.go (3)
91-93: BatchSize validation silently clamps invalid values.When
BatchSize > DefaultBatchSize, it's silently reset to the default. Consider logging a warning so users know their configured value was adjusted.Proposed improvement
if settings.BatchSize <= 0 || settings.BatchSize > DefaultBatchSize { + if settings.BatchSize > DefaultBatchSize { + slog.Warn("batch size exceeds Firestore limit, clamping to max", "requested", settings.BatchSize, "max", DefaultBatchSize) + } settings.BatchSize = DefaultBatchSize }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/firestore/connector.go` around lines 91 - 93, The code silently clamps invalid settings.BatchSize to DefaultBatchSize; update the if block that checks settings.BatchSize to emit a warning before changing the value so users know their configured value was adjusted—e.g., log a message referencing the original settings.BatchSize and DefaultBatchSize (use the connector's existing logger, e.g., logger.Warnf or log.Printf) then set settings.BatchSize = DefaultBatchSize. Ensure you reference settings.BatchSize and DefaultBatchSize in the warning so it's clear which values were changed.
322-325: Potential precision loss when formatting large uint64 values.For
uint64values exceeding JavaScript's safe integer limit (2^53 - 1), the%dformat will preserve the value, but downstream JSON consumers may lose precision. This is a known limitation when using numeric document IDs with Firestore and JSON.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/firestore/connector.go` around lines 322 - 325, The current switch cases for numeric types (case int/int32/int64 and case uint/uint32/uint64) can cause precision loss for values > JS_SAFE_INT (2^53-1); update the handling so uint64 (and unsigned types promoted to uint64) are detected and for values > 9007199254740991 you return their decimal representation as a string (use strconv.FormatUint(val, 10)), otherwise return the numeric formatting; specifically modify the uint64 handling branch (and any code paths that cast to uint64) to compare against the JS max safe integer and return a string for large values to avoid downstream JSON precision loss.
199-220: Batch write lacks error context for identifying problematic documents.When
extractDocumentIDAndDatafails, the error doesn't include which document in the batch caused the failure. This makes debugging harder when processing large batches.Proposed improvement
for _, raw := range data { docID, docData, err := extractDocumentIDAndData(raw, dataType) if err != nil { - return fmt.Errorf("failed to extract document ID: %w", err) + return fmt.Errorf("failed to extract document ID (batch index may help identify doc): %w", err) }Alternatively, consider adding the document index to the error message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/firestore/connector.go` around lines 199 - 220, In writeBatch, include context about which item failed by changing the loop to capture the item index and include it in the returned error when extractDocumentIDAndData fails; update the error returned from extractDocumentIDAndData inside writeBatch (and any subsequent errors) to wrap the original error with fmt.Errorf including the index (and optionally a short snippet or hex of raw) and the collectionName so it's straightforward to identify the problematic document when debugging; reference conn.writeBatch and extractDocumentIDAndData to locate and modify the error wrapping logic.connectors/firestore/connector_test.go (1)
104-125: Consider adding error case and BSON type coverage forvalueToString.The test covers basic scalar types but misses:
bson.ObjectIDconversion (returnsHex())bson.Binaryconversion (returns hex-encoded data)- Verifying the fallback
%vformatting for unknown typesThese are exercised in the connector's actual use with BSON data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/firestore/connector_test.go` around lines 104 - 125, Update TestValueToString to include BSON and error scenarios: add table entries that pass a bson.ObjectId (expecting its Hex() string) and a bson.Binary (expecting hex-encoded data) to valueToString and assert the returned strings, add a case with an unknown/complex type (e.g., a custom struct) to assert the fallback fmt.Sprintf("%v") behavior, and add at least one input that should cause valueToString to return an error and assert.Error; reference the existing TestValueToString and valueToString function names when locating where to add these cases.
🤖 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/firestore/connector_integration_test.go`:
- Line 199: The test currently ignores the error returned from encodeJSON
(encoded, _ := encodeJSON(doc)), which can hide setup failures; update the test
to check the error and fail fast—e.g., replace the ignored error with error
handling using the test helper (t.Fatalf or require.NoError) to assert
encodeJSON(doc) returns no error and only then use the encoded value; reference
the encodeJSON call and ensure the test fails immediately if encoding fails.
- Line 180: The deferred type assertion connector.(interface{ Teardown()
}).Teardown() can panic if connector lacks that exact Teardown() method; change
it to perform a safe comma‑ok assertion inside the deferred function (e.g.,
defer func() { if td, ok := connector.(interface{ Teardown() }); ok {
td.Teardown() } }) so Teardown is only called when implemented and the test
won’t panic unexpectedly; optionally log or t.Log when Teardown is absent for
visibility.
In `@go.mod`:
- Line 45: Update the grpc dependency entry "google.golang.org/grpc v1.76.0" in
go.mod to v1.80.0 and ensure the module graph is refreshed (e.g., run go get
google.golang.org/grpc@v1.80.0 and go mod tidy) so the fix for CVE-2026-33186 is
applied; afterwards run the test suite/build to verify no regressions.
---
Nitpick comments:
In `@connectors/firestore/connector_test.go`:
- Around line 104-125: Update TestValueToString to include BSON and error
scenarios: add table entries that pass a bson.ObjectId (expecting its Hex()
string) and a bson.Binary (expecting hex-encoded data) to valueToString and
assert the returned strings, add a case with an unknown/complex type (e.g., a
custom struct) to assert the fallback fmt.Sprintf("%v") behavior, and add at
least one input that should cause valueToString to return an error and
assert.Error; reference the existing TestValueToString and valueToString
function names when locating where to add these cases.
In `@connectors/firestore/connector.go`:
- Around line 91-93: The code silently clamps invalid settings.BatchSize to
DefaultBatchSize; update the if block that checks settings.BatchSize to emit a
warning before changing the value so users know their configured value was
adjusted—e.g., log a message referencing the original settings.BatchSize and
DefaultBatchSize (use the connector's existing logger, e.g., logger.Warnf or
log.Printf) then set settings.BatchSize = DefaultBatchSize. Ensure you reference
settings.BatchSize and DefaultBatchSize in the warning so it's clear which
values were changed.
- Around line 322-325: The current switch cases for numeric types (case
int/int32/int64 and case uint/uint32/uint64) can cause precision loss for values
> JS_SAFE_INT (2^53-1); update the handling so uint64 (and unsigned types
promoted to uint64) are detected and for values > 9007199254740991 you return
their decimal representation as a string (use strconv.FormatUint(val, 10)),
otherwise return the numeric formatting; specifically modify the uint64 handling
branch (and any code paths that cast to uint64) to compare against the JS max
safe integer and return a string for large values to avoid downstream JSON
precision loss.
- Around line 199-220: In writeBatch, include context about which item failed by
changing the loop to capture the item index and include it in the returned error
when extractDocumentIDAndData fails; update the error returned from
extractDocumentIDAndData inside writeBatch (and any subsequent errors) to wrap
the original error with fmt.Errorf including the index (and optionally a short
snippet or hex of raw) and the collectionName so it's straightforward to
identify the problematic document when debugging; reference conn.writeBatch and
extractDocumentIDAndData to locate and modify the error wrapping logic.
🪄 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: 6ee06f1d-1eb1-4af4-88df-61acc4fd43a7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
connectors/firestore/connector.goconnectors/firestore/connector_integration_test.goconnectors/firestore/connector_test.gogo.modinternal/app/options/connectorflags.go
| BatchSize: 100, | ||
| }) | ||
| assert.NoError(t, err) | ||
| defer connector.(interface{ Teardown() }).Teardown() |
There was a problem hiding this comment.
Type assertion may panic if Teardown method signature changes.
The type assertion connector.(interface{ Teardown() }).Teardown() will panic at runtime if the connector doesn't implement this exact interface. Consider using a safe type assertion with the comma-ok idiom.
Proposed safer cleanup
- defer connector.(interface{ Teardown() }).Teardown()
+ defer func() {
+ if t, ok := connector.(interface{ Teardown() }); ok {
+ t.Teardown()
+ }
+ }()📝 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.
Suggested change
| defer connector.(interface{ Teardown() }).Teardown() | |
| defer func() { | |
| if t, ok := connector.(interface{ Teardown() }); ok { | |
| t.Teardown() | |
| } | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connectors/firestore/connector_integration_test.go` at line 180, The deferred
type assertion connector.(interface{ Teardown() }).Teardown() can panic if
connector lacks that exact Teardown() method; change it to perform a safe
comma‑ok assertion inside the deferred function (e.g., defer func() { if td, ok
:= connector.(interface{ Teardown() }); ok { td.Teardown() } }) so Teardown is
only called when implemented and the test won’t panic unexpectedly; optionally
log or t.Log when Teardown is absent for visibility.
| "id": i, // JSON_ID uses "id" field | ||
| "value": i * 10, | ||
| } | ||
| encoded, _ := encodeJSON(doc) |
There was a problem hiding this comment.
Ignored error from encodeJSON could mask test setup failures.
If JSON encoding fails, the test would proceed with nil/empty data, potentially causing confusing failures later. Consider asserting no error.
Proposed fix
- encoded, _ := encodeJSON(doc)
+ encoded, err := encodeJSON(doc)
+ assert.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connectors/firestore/connector_integration_test.go` at line 199, The test
currently ignores the error returned from encodeJSON (encoded, _ :=
encodeJSON(doc)), which can hide setup failures; update the test to check the
error and fail fast—e.g., replace the ignored error with error handling using
the test helper (t.Fatalf or require.NoError) to assert encodeJSON(doc) returns
no error and only then use the encoded value; reference the encodeJSON call and
ensure the test fails immediately if encoding fails.
| google.golang.org/grpc v1.73.0 | ||
| google.golang.org/protobuf v1.36.6 | ||
| golang.org/x/time v0.14.0 | ||
| google.golang.org/grpc v1.76.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest patched version of google.golang.org/grpc that fixes the authorization bypass vulnerability GHSA-p77j-4mvh-x3m3?
💡 Result:
The latest patched version of google.golang.org/grpc that fixes the authorization bypass vulnerability GHSA-p77j-4mvh-x3m3 (CVE-2026-33186) is v1.80.0, released on 2026-04-01. This version includes all prior fixes, including the specific patch in v1.79.3 released on 2026-03-17.
Citations:
- 1: GHSA-p77j-4mvh-x3m3
- 2: https://github.com/grpc/grpc-go/releases/tag/v1.80.0
- 3: https://github.com/grpc/grpc-go
- 4: https://github.com/grpc/grpc-go/releases/tag/v1.79.3
Upgrade google.golang.org/grpc to v1.80.0 to fix a critical authorization bypass vulnerability.
gRPC v1.76.0 contains GHSA-p77j-4mvh-x3m3 (CVE-2026-33186), a critical authorization bypass vulnerability caused by missing leading slash validation in the :path pseudo-header. This allows attackers to bypass authorization checks. Upgrade to v1.80.0 or later to resolve this.
🧰 Tools
🪛 OSV Scanner (2.3.5)
[CRITICAL] 45-45: google.golang.org/grpc 1.76.0: Authorization bypass in gRPC-Go via missing leading slash in :path in google.golang.org/grpc
(GO-2026-4762)
[CRITICAL] 45-45: google.golang.org/grpc 1.76.0: gRPC-Go has an authorization bypass via missing leading slash in :path
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 45, Update the grpc dependency entry "google.golang.org/grpc
v1.76.0" in go.mod to v1.80.0 and ensure the module graph is refreshed (e.g.,
run go get google.golang.org/grpc@v1.80.0 and go mod tidy) so the fix for
CVE-2026-33186 is applied; afterwards run the test suite/build to verify no
regressions.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/firestore/connector.go`:
- Around line 250-282: The switch currently treats UPDATE_TYPE_PARTIAL_UPDATE as
a full upsert; add an explicit case for
adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATE in the switch and handle it by
converting update.GetData() via rawToMap (same as other branches), deleting
idKey, and calling the incremental update method on the batch writer (e.g.,
bw.Update(docRef, partialData)) with the same bw.End() and error wrapping
pattern; if the batch writer does not support partial updates, return a clear
error instead of falling through to bw.Set to avoid overwriting full documents.
- Around line 435-451: The valueToString function currently falls back to
fmt.Sprintf("%v", val) which silently converts complex types (maps, slices,
structs) into unstable IDs; change valueToString to only accept explicit types:
string, bson.ObjectID (Hex), integer/unsigned/float families (formatted),
bson.Binary (hex of Data), []byte (hex), and any type implementing fmt.Stringer
(use .String()); for any other type return a descriptive error like "unsupported
id type: %T" instead of the %v fallback so callers can handle invalid ID types
instead of producing unstable document IDs.
🪄 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: 0471fea1-61f9-45f2-96b2-7dd758da67b8
📒 Files selected for processing (3)
connectors/firestore/bson_conversion_test.goconnectors/firestore/connector.goconnectors/firestore/connector_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- connectors/firestore/connector_integration_test.go
Comment on lines
+250
to
+282
| switch update.GetType() { | ||
| case adiomv1.UpdateType_UPDATE_TYPE_DELETE: | ||
| job, err = bw.Delete(docRef) | ||
| if err != nil { | ||
| bw.End() | ||
| return fmt.Errorf("failed to queue delete operation: %w", err) | ||
| } | ||
| case adiomv1.UpdateType_UPDATE_TYPE_UPDATE, adiomv1.UpdateType_UPDATE_TYPE_INSERT: | ||
| docData, err := rawToMap(update.GetData(), dataType) | ||
| if err != nil { | ||
| bw.End() | ||
| return fmt.Errorf("failed to convert update data: %w", err) | ||
| } | ||
| delete(docData, idKey) | ||
| job, err = bw.Set(docRef, docData) | ||
| if err != nil { | ||
| bw.End() | ||
| return fmt.Errorf("failed to queue set operation: %w", err) | ||
| } | ||
| default: | ||
| slog.Warn("unknown update type, treating as upsert", "type", update.GetType()) | ||
| docData, err := rawToMap(update.GetData(), dataType) | ||
| if err != nil { | ||
| bw.End() | ||
| return fmt.Errorf("failed to convert update data: %w", err) | ||
| } | ||
| delete(docData, idKey) | ||
| job, err = bw.Set(docRef, docData) | ||
| if err != nil { | ||
| bw.End() | ||
| return fmt.Errorf("failed to queue set operation: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
UPDATE_TYPE_PARTIAL_UPDATE currently falls into full upsert behavior.
This can overwrite full documents with partial payloads and cause data loss. Handle partial updates explicitly (or reject them) instead of routing through the default upsert path.
Proposed safe handling
switch update.GetType() {
case adiomv1.UpdateType_UPDATE_TYPE_DELETE:
job, err = bw.Delete(docRef)
@@
case adiomv1.UpdateType_UPDATE_TYPE_UPDATE, adiomv1.UpdateType_UPDATE_TYPE_INSERT:
docData, err := rawToMap(update.GetData(), dataType)
if err != nil {
bw.End()
return fmt.Errorf("failed to convert update data: %w", err)
}
delete(docData, idKey)
job, err = bw.Set(docRef, docData)
if err != nil {
bw.End()
return fmt.Errorf("failed to queue set operation: %w", err)
}
+ case adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATE:
+ bw.End()
+ return fmt.Errorf("partial updates are not supported by firestore sink")
default:
- slog.Warn("unknown update type, treating as upsert", "type", update.GetType())
- docData, err := rawToMap(update.GetData(), dataType)
- if err != nil {
- bw.End()
- return fmt.Errorf("failed to convert update data: %w", err)
- }
- delete(docData, idKey)
- job, err = bw.Set(docRef, docData)
- if err != nil {
- bw.End()
- return fmt.Errorf("failed to queue set operation: %w", err)
- }
+ bw.End()
+ return fmt.Errorf("unsupported update type: %v", update.GetType())
}📝 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.
Suggested change
| switch update.GetType() { | |
| case adiomv1.UpdateType_UPDATE_TYPE_DELETE: | |
| job, err = bw.Delete(docRef) | |
| if err != nil { | |
| bw.End() | |
| return fmt.Errorf("failed to queue delete operation: %w", err) | |
| } | |
| case adiomv1.UpdateType_UPDATE_TYPE_UPDATE, adiomv1.UpdateType_UPDATE_TYPE_INSERT: | |
| docData, err := rawToMap(update.GetData(), dataType) | |
| if err != nil { | |
| bw.End() | |
| return fmt.Errorf("failed to convert update data: %w", err) | |
| } | |
| delete(docData, idKey) | |
| job, err = bw.Set(docRef, docData) | |
| if err != nil { | |
| bw.End() | |
| return fmt.Errorf("failed to queue set operation: %w", err) | |
| } | |
| default: | |
| slog.Warn("unknown update type, treating as upsert", "type", update.GetType()) | |
| docData, err := rawToMap(update.GetData(), dataType) | |
| if err != nil { | |
| bw.End() | |
| return fmt.Errorf("failed to convert update data: %w", err) | |
| } | |
| delete(docData, idKey) | |
| job, err = bw.Set(docRef, docData) | |
| if err != nil { | |
| bw.End() | |
| return fmt.Errorf("failed to queue set operation: %w", err) | |
| } | |
| } | |
| switch update.GetType() { | |
| case adiomv1.UpdateType_UPDATE_TYPE_DELETE: | |
| job, err = bw.Delete(docRef) | |
| if err != nil { | |
| bw.End() | |
| return fmt.Errorf("failed to queue delete operation: %w", err) | |
| } | |
| case adiomv1.UpdateType_UPDATE_TYPE_UPDATE, adiomv1.UpdateType_UPDATE_TYPE_INSERT: | |
| docData, err := rawToMap(update.GetData(), dataType) | |
| if err != nil { | |
| bw.End() | |
| return fmt.Errorf("failed to convert update data: %w", err) | |
| } | |
| delete(docData, idKey) | |
| job, err = bw.Set(docRef, docData) | |
| if err != nil { | |
| bw.End() | |
| return fmt.Errorf("failed to queue set operation: %w", err) | |
| } | |
| case adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATE: | |
| bw.End() | |
| return fmt.Errorf("partial updates are not supported by firestore sink") | |
| default: | |
| bw.End() | |
| return fmt.Errorf("unsupported update type: %v", update.GetType()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connectors/firestore/connector.go` around lines 250 - 282, The switch
currently treats UPDATE_TYPE_PARTIAL_UPDATE as a full upsert; add an explicit
case for adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATE in the switch and handle
it by converting update.GetData() via rawToMap (same as other branches),
deleting idKey, and calling the incremental update method on the batch writer
(e.g., bw.Update(docRef, partialData)) with the same bw.End() and error wrapping
pattern; if the batch writer does not support partial updates, return a clear
error instead of falling through to bw.Set to avoid overwriting full documents.
Comment on lines
+435
to
+451
| func valueToString(v any) (string, error) { | ||
| switch val := v.(type) { | ||
| case string: | ||
| return val, nil | ||
| case bson.ObjectID: | ||
| return val.Hex(), nil | ||
| case int, int32, int64: | ||
| return fmt.Sprintf("%d", val), nil | ||
| case uint, uint32, uint64: | ||
| return fmt.Sprintf("%d", val), nil | ||
| case float32, float64: | ||
| return fmt.Sprintf("%v", val), nil | ||
| case bson.Binary: | ||
| return fmt.Sprintf("%x", val.Data), nil | ||
| default: | ||
| return fmt.Sprintf("%v", val), nil | ||
| } |
There was a problem hiding this comment.
valueToString silently accepts unsupported ID types.
The default %v fallback converts maps/slices/structs into ad-hoc strings, which can create unstable/colliding document IDs. This should return an error for unsupported types.
Proposed stricter ID conversion
func valueToString(v any) (string, error) {
switch val := v.(type) {
case string:
return val, nil
case bson.ObjectID:
return val.Hex(), nil
case int, int32, int64:
return fmt.Sprintf("%d", val), nil
case uint, uint32, uint64:
return fmt.Sprintf("%d", val), nil
case float32, float64:
return fmt.Sprintf("%v", val), nil
case bson.Binary:
return fmt.Sprintf("%x", val.Data), nil
default:
- return fmt.Sprintf("%v", val), nil
+ return "", fmt.Errorf("unsupported document ID type: %T", v)
}
}📝 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.
Suggested change
| func valueToString(v any) (string, error) { | |
| switch val := v.(type) { | |
| case string: | |
| return val, nil | |
| case bson.ObjectID: | |
| return val.Hex(), nil | |
| case int, int32, int64: | |
| return fmt.Sprintf("%d", val), nil | |
| case uint, uint32, uint64: | |
| return fmt.Sprintf("%d", val), nil | |
| case float32, float64: | |
| return fmt.Sprintf("%v", val), nil | |
| case bson.Binary: | |
| return fmt.Sprintf("%x", val.Data), nil | |
| default: | |
| return fmt.Sprintf("%v", val), nil | |
| } | |
| func valueToString(v any) (string, error) { | |
| switch val := v.(type) { | |
| case string: | |
| return val, nil | |
| case bson.ObjectID: | |
| return val.Hex(), nil | |
| case int, int32, int64: | |
| return fmt.Sprintf("%d", val), nil | |
| case uint, uint32, uint64: | |
| return fmt.Sprintf("%d", val), nil | |
| case float32, float64: | |
| return fmt.Sprintf("%v", val), nil | |
| case bson.Binary: | |
| return fmt.Sprintf("%x", val.Data), nil | |
| default: | |
| return "", fmt.Errorf("unsupported document ID type: %T", v) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connectors/firestore/connector.go` around lines 435 - 451, The valueToString
function currently falls back to fmt.Sprintf("%v", val) which silently converts
complex types (maps, slices, structs) into unstable IDs; change valueToString to
only accept explicit types: string, bson.ObjectID (Hex), integer/unsigned/float
families (formatted), bson.Binary (hex of Data), []byte (hex), and any type
implementing fmt.Stringer (use .String()); for any other type return a
descriptive error like "unsupported id type: %T" instead of the %v fallback so
callers can handle invalid ID types instead of producing unstable document IDs.
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
1 participant
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 by CodeRabbit
New Features
Tests