fix(storage): wrap non-StorageError exceptions in StorageUnknownError - #2554
fix(storage): wrap non-StorageError exceptions in StorageUnknownError#2554PedroHenrique0713 wants to merge 1 commit into
Conversation
handleOperation's documented contract is to return { data, error } tuples.
When a non-StorageError was thrown, the error was re-thrown instead of
being wrapped, breaking the expected caller contract.
📝 WalkthroughSummary by CodeRabbit
Walkthrough
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/storage-js/src/lib/common/BaseApiClient.ts`:
- Around line 101-104: Update the error construction in BaseApiClient to pass
this.namespace as the third argument to StorageUnknownError, preserving the
configured vectors or storage namespace while leaving the existing message and
cause unchanged.
- Line 103: Update the error handling around _getErrorMessage in BaseApiClient
so extracting a message cannot throw for arbitrary values, including circular
references or bigint; ensure the catch always returns the documented { data:
null, error } tuple by using a safe fallback message, either locally or by
making _getErrorMessage total.
🪄 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 Plus
Run ID: d0305f78-e429-402a-8d09-af385c0dfd86
📒 Files selected for processing (1)
packages/core/storage-js/src/lib/common/BaseApiClient.ts
| return { | ||
| data: null, | ||
| error: new StorageUnknownError(_getErrorMessage(error), error) as unknown as TError, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the client error namespace.
BaseApiClient supports the vectors namespace, but this constructor call uses StorageUnknownError’s default 'storage' namespace. Vector clients will therefore receive the wrong error name/type. Pass this.namespace as the third constructor argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/storage-js/src/lib/common/BaseApiClient.ts` around lines 101 -
104, Update the error construction in BaseApiClient to pass this.namespace as
the third argument to StorageUnknownError, preserving the configured vectors or
storage namespace while leaving the existing message and cause unchanged.
| throw error | ||
| return { | ||
| data: null, | ||
| error: new StorageUnknownError(_getErrorMessage(error), error) as unknown as TError, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not let message extraction escape the error handler.
_getErrorMessage falls back to JSON.stringify, which throws for values containing circular references or bigint. Because it runs inside this catch, such inputs are re-thrown instead of returning the documented { data: null, error } tuple. Guard the extraction with a fallback message, or make _getErrorMessage total for arbitrary thrown values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/storage-js/src/lib/common/BaseApiClient.ts` at line 103, Update
the error handling around _getErrorMessage in BaseApiClient so extracting a
message cannot throw for arbitrary values, including circular references or
bigint; ensure the catch always returns the documented { data: null, error }
tuple by using a safe fallback message, either locally or by making
_getErrorMessage total.
|
Closing this one. I opened it today as part of a batch of stacked branches, and it went out without the test coverage I normally include. I already have open PRs in this repo (#2514, #2518, #2529, #2530) that carry unit and end-to-end tests, and I would rather keep the review queue focused on those. If this fix still applies, I will resubmit it properly tested on top of master. Sorry for the noise. |
1 participant
Summary
handleOperation's documented contract is to always return{ data, error }tuples. However, when the operation threw a non-StorageError exception (JSON parse failure, TypeError from missing fetch, etc.), the error was re-thrown instead of being wrapped.Fix: Wrap non-StorageError exceptions in
StorageUnknownErrorand return{ data: null, error }.