@tus/azure-store: fix metadata parsing bug#790
Conversation
🦋 Changeset detectedLatest commit: 5461fc0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
WalkthroughChanged AzureStore.getMetadata to robustly handle upload.metadata as either a base64/string value, an object, or missing: if it's a string parse directly with Metadata.parse, if it's an object stringify then parse, and if absent default to an empty object. Replaced the previous unconditional JSON.stringify wrapper. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/azure-store/src/index.ts (3)
118-121: Make parsing resilient to unexpected shapes and parse errorsDefend against cases where upload.metadata might already be an object (older records/tests) or an invalid string. Fallback to {} instead of throwing.
Apply:
- upload.metadata = upload.metadata ? Metadata.parse(upload.metadata) : {} + try { + if (typeof upload.metadata === 'string' && upload.metadata.length > 0) { + upload.metadata = Metadata.parse(upload.metadata) + } else if (upload.metadata && typeof upload.metadata === 'object') { + // already parsed; keep as-is + } else { + upload.metadata = {} + } + } catch (e) { + log('Invalid TUS metadata string; defaulting to {}', e as Error) + upload.metadata = {} + }
71-73: Typos in docstring“metada” → “metadata”, “everytime” → “every time”.
- * Saves upload metadata to blob metada. Also upload metadata - * gets saved in local cache as well to avoid calling azure server everytime. + * Saves upload metadata to blob metadata. Also saves it + * in local cache to avoid calling the Azure service every time.
145-156: Optional: add a targeted test to prevent regressionsConsider adding an AzureStore metadata round‑trip test (including non‑ASCII and null values) similar to packages/utils/src/test/stores.ts to lock this fix.
I can draft a minimal test that creates an upload with metadata {filename: '世界.pdf', is_confidential: null}, persists it, reads it back, and deepStrictEquals the metadata. Want me to open a follow‑up PR with that?
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/azure-store/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/azure-store/src/index.ts (1)
packages/gcs-store/src/index.ts (1)
upload(172-180)
🔇 Additional comments (1)
packages/azure-store/src/index.ts (1)
118-121: Good fix: parse the raw TUS metadata string instead of JSON-stringifying itThis removes the extra quoting that broke Metadata.parse and restores round‑trip symmetry with saveMetadata’s Metadata.stringify.
Murderlon
had a problem deploying
to
external-testing
GitHub Actions
Error
Murderlon
had a problem deploying
to
external-testing
GitHub Actions
Error
Murderlon
had a problem deploying
to
external-testing
GitHub Actions
Failure
|
CI is failing |
|
ping |
osarogie
had a problem deploying
to
external-testing
GitHub Actions
Error
osarogie
had a problem deploying
to
external-testing
GitHub Actions
Error
osarogie
had a problem deploying
to
external-testing
GitHub Actions
Error
|
Apologies for the delay @Murderlon. I've fix the bug |
osarogie
requested a deployment
to
external-testing
GitHub Actions
Waiting
| let metadataStr | ||
| if (typeof upload.metadata === 'string') { | ||
| metadataStr = upload.metadata | ||
| } else if (upload.metadata && typeof upload.metadata === 'object') { | ||
| metadataStr = JSON.stringify(upload.metadata ?? {}) | ||
| } else { | ||
| metadataStr = '{}' | ||
| } | ||
| upload.metadata = Metadata.parse(metadataStr) |
There was a problem hiding this comment.
This is overprotective for no reason? It should be as simple as this? Maybe undefined instead of {} not sure.
| let metadataStr | |
| if (typeof upload.metadata === 'string') { | |
| metadataStr = upload.metadata | |
| } else if (upload.metadata && typeof upload.metadata === 'object') { | |
| metadataStr = JSON.stringify(upload.metadata ?? {}) | |
| } else { | |
| metadataStr = '{}' | |
| } | |
| upload.metadata = Metadata.parse(metadataStr) | |
| upload.metadata = upload.metadata ? Metadata.parse(JSON.stringify(upload.metadata)) : {} |
There was a problem hiding this comment.
There were cases when upload.metadata was already a string.
That's why I added this check
if (typeof upload.metadata === 'string') {
metadataStr = upload.metadata
}I could simplify it to look like this
let metadataStr
if (typeof upload.metadata === 'string') {
metadataStr = upload.metadata
} else if (upload.metadata && typeof upload.metadata === 'object') {
metadataStr = JSON.stringify(upload.metadata ?? {})
}
upload.metadata = metadataStr ? Metadata.parse(metadataStr) : {}2 participants
Removes serialization that wraps the metadata (that's already a string) in quotes.
These quote caused
Metadata.parseto fail with the following error: "Metadata string is not valid"Closes #791