fix(storage): derive content type for signed URL uploads - #1158
fix(storage): derive content type for signed URL uploads#1158AndroidPoet wants to merge 2 commits into
Conversation
uploadToSignedURL fell back to defaultFileOptions when no options were passed, and that constant carries contentType "text/plain;charset=UTF-8". The fallback to the path extension could therefore never run, so a PNG or JPEG uploaded through a signed URL was stored and served as text/plain. upload() and update() already default to FileOptions(), whose contentType is nil so the extension is used, and the deprecated uploadToSignedURL overloads pass FileOptions() too. Use FileOptions() here as well. cacheControl and upsert are identical between the two, so only contentType changes. The two new tests capture the request body through StorageHTTPSession rather than snapshotRequest, so they fail when the emitted content type is wrong.
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Possibly related PRs
Suggested labels: 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: 1
🤖 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 `@Sources/Storage/StorageFileApi.swift`:
- Line 958: Update the uploadToSignedURLCleansPath snapshot to expect
Content-Length 283 and Content-Type text/plain for omitted FileOptions, while
preserving the existing signed-upload behavior and other snapshot assertions.
🪄 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: 5f754df1-32a3-4930-b330-fea49b2ef467
📒 Files selected for processing (2)
Sources/Storage/StorageFileApi.swiftTests/StorageTests/StorageFileAPITests.swift
| options: FileOptions? | ||
| ) async throws -> SignedURLUploadResponse { | ||
| let options = options ?? defaultFileOptions | ||
| let options = options ?? FileOptions() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Update the remaining signed-upload snapshot.
This fallback affects all signed uploads with omitted options. The existing uploadToSignedURLCleansPath snapshot at Lines 1393–1404 still expects Content-Length: 297 and Content-Type: text/plain;charset=UTF-8; with this change it should expect 283 and text/plain, so the test will fail in CI.
Proposed snapshot update
- --header "Content-Length: 297" \
+ --header "Content-Length: 283" \
...
- Content-Type: text/plain;charset=UTF-8\`#r`
+ Content-Type: text/plain\`#r`🤖 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 `@Sources/Storage/StorageFileApi.swift` at line 958, Update the
uploadToSignedURLCleansPath snapshot to expect Content-Length 283 and
Content-Type text/plain for omitted FileOptions, while preserving the existing
signed-upload behavior and other snapshot assertions.
Coverage Report for CI Build 30533615360Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.01%) to 84.037%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
2 participants
Problem
uploadToSignedURLstampsContent-Type: text/plain;charset=UTF-8on every upload whenoptionsis omitted, so a PNG or JPEG sent through a signed upload URL is stored with that mimetype and served back as text. Browsers then download the file instead of rendering it, and<img src>breaks.Both public overloads default
optionstonil:and
_uploadToSignedURLresolved thatnilagainstdefaultFileOptions, whosecontentTypeis"text/plain;charset=UTF-8". Because that value is nevernil, the extension-derived fallback on the encode path could not run:Fix
Resolve the missing options against
FileOptions()instead.cacheControl("3600") andupsert(false) are identical between the two, so the only change iscontentType, which becomesniland lets the path extension decide, as the property already documents:Four things line up behind this:
upload()andupdate()already default toFileOptions(). I confirmed by capturing the request body thatupload("file.txt", data:)emits bareContent-Type: text/plain, which is exactly whatuploadToSignedURLemits after this change. No new behavior is introduced anywhere in the SDK, a divergence is removed.uploadToSignedURLoverloads inDeprecated.swiftpassFileOptions()and are therefore already correct. Moving off the deprecated API silently changed the stored mimetype.DEFAULT_FILE_OPTIONS.contentTypeonly on the raw body branch (headers['content-type'] = options.contentType). On the Blob/FormData branch, the one this SDK always takes, it appends onlycacheControlandmetadataand lets the file's own type become the part'sContent-Type. SeeStorageFileApi.ts.?? mimeType(forPathExtension:)fallback forfileURLuploads, but on this pathdefaultFileOptionsmade it unreachable.One consequence worth naming:
.txtuploads through a signed URL no longer carry;charset=UTF-8. That matches whatupload()has always sent for the same file, so it is alignment rather than a regression.defaultFileOptionsis left in place because_uploadOrUpdatestill references it, though every caller there passes a non-optionalFileOptions, so that fallback is already unreachable. Happy to remove the constant in a follow-up if you would prefer.Tests
Two tests asserting the emitted
Content-Typefor a.png(data) and a.jpg(fileURL) upload withoptionsomitted. Both fail onmainwith 4 failed expectations and pass with the fix. 147 StorageTests green.They capture the request body through
StorageHTTPSessionrather than usingsnapshotRequest, becausesnapshotRequestassertions in this suite do not currently fail. I changed an existing expected value toContent-Type: totally/bogusanduploadToSignedURL()still passed, so the request-shape snapshots inStorageFileAPITestsare not gating anything since the Swift Testing migration. That looked worth reporting separately rather than working around silently, and it is why these two tests assert directly on the body.The existing
uploadToSignedURL()snapshot is updated to the values this change produces (text/plain,Content-Length297 to 283, verified by capturing the real body).No public API change.