Conversation
CahidArda
left a comment
There was a problem hiding this comment.
Static review. I couldn't run the tests locally, so this comes from reading the code.
Stack note: #22 → #24 → #25. This PR deletes #22's upload.ts and its tests, so of #22 only --token, the deps, the engines bump and the release-tag change survive. I'd merge the three as one unit, or squash #22 into this one.
Should fix
mvcan delete files outside the source directory.walk()now follows symlinks (#22 deliberately didn't), and after uploadperform()callsunlink(source.path).mv ./dir blob://b/ -rwheredir/link → ~/importantdeletes the real files in~/important. aws does the same but has--no-follow-symlinks. Please add that flag, or at least don't follow symlinks formvsources.- stdin upload without
--expected-sizebuffers up to 5 GB in RAM (maxSize: "5gb"). That can take down a machine or CI runner. Lower the cap a lot, or spill the stream to a temp file. - Same-bucket server-side
copyhas no retries.from.copy()sits outsidewithRetries, while streaming and every other operation retry. Transient 5xx and 429 errors fail immediately.
Worth a look
listDirectoryis a hand-rolled SigV4 signer plus a regex XML parser. It doesn't decode numeric entities ( ) and doesn't requestencoding-type=url. Delimiter listing belongs in@upstash/blob. I'd upstream it and drop about 90 lines here.tokenBucketIddecodes the token by fixed byte offsets (raw[2]= length, id at offset 6). That couples the CLI to an undocumented format. It should come from the SDK.findAccountBucketpicks the first bucket whose name matches. If names aren't unique per account, this silently targets the wrong bucket, andrb -fthen empties it.lswith no argument prints the raw bucket list. If that includestoken/token_next, the most casual command dumps every bucket token.getalready has--hide-credentials; I'd hide tokens by default here.- Same-bucket copy with
--metadatabut no--content-type: check whether the SDK's copy uses REPLACE semantics and resets the content type. rbdetects "not empty" with a regex on the error message (/not empty/), which is fragile. Also,deleteandrbnow do the same job.- The README lost the note on leftover multipart uploads. A killed process still leaves them behind, and only
rb -fcleans them up. - Nits: the
filterOrdercounter is module-global and leaks across tests.--metadata " =x"is accepted with an empty key.
Good parts
Temp-file-and-rename downloads, the mtime alignment for sync, the local path-escape guard, nested-prefix detection keyed on the real S3 bucket, and case/symlink handling in sync -d are careful work. Keeping list as an alias and a hidden --bucket-id preserves backward compatibility.
Before merging: this PR shows 2/3 checks while #25 (same head) is 3/3, so rerun the Tests job. Also, v1.4.0-canary.4 already ships these commands, including the symlink-following mv.
Adds
upstash blob ls|cp|mv|rm|sync|presign|mb|rbwithblob://<bucket>/<key>URIs, mirroringaws s3flags and semantics, plus short flags (-r,-d,-n,-q).Replaces
blob uploadwithcp -r/sync, mergeslistintols, and letsget/delete/credentialstake a bucket name or id (--bucket-idstill works).A Blob token is only used for the bucket it was issued for; names resolve through the account.