-
Notifications
You must be signed in to change notification settings - Fork 213
@tus/azure-store: fix metadata parsing bug #790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: a224979 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 |
WalkthroughChanged AzureStore.getMetadata to parse raw Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
CI is failing |
Removes serialization that wraps the metadata (that's already a string) in quotes.
These quote caused
Metadata.parse
to fail with the following error: "Metadata string is not valid"Closes #791