fix: gracefully handle download errors #1058
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: aaf79ef 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 |
WalkthroughThe changes introduce improvements to error handling in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
More templates
commit: |
📦 Bundle size comparison
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
packages/uploadthing/test/sdk.test.ts (2)
221-245: Consider enhancing error message and test description.The test case effectively verifies download error handling, but could be improved:
- The test name could be more specific, e.g., "handles 404 error when downloading non-existent file"
- The error message could explicitly mention the HTTP status code for better debugging
- it("gracefully handles download errors", async ({ db }) => { + it("handles 404 error when downloading non-existent file", async ({ db }) => { // ... test implementation ... expect(result).toEqual({ data: null, error: { code: "BAD_REQUEST", data: expect.objectContaining({ _tag: "ResponseError", description: "non 2xx status code", }), message: - "Failed to download requested file: StatusCode: non 2xx status code (404 GET https://cdn.foo.com/does-not-exist.txt)", + "Failed to download requested file: HTTP 404 Not Found (GET https://cdn.foo.com/does-not-exist.txt)", }, }); });
221-289: Consider adding more error scenarios.The error handling test coverage could be enhanced by adding the following scenarios:
- Network errors (connection refused, timeout)
- Different HTTP status codes (500, 403, etc.)
- Malformed responses
Example test case:
it("handles network timeout when downloading file", async ({ db }) => { const utapi = new UTApi({ token: testToken.encoded }); msw.use( http.get("https://cdn.foo.com/timeout.txt", () => { return new Promise(() => {}); // Never resolves, simulating timeout }), ); const result = await utapi.uploadFilesFromUrl( "https://cdn.foo.com/timeout.txt", ); expect(result.error?.code).toBe("BAD_REQUEST"); expect(result.error?.message).toContain("timeout"); });packages/uploadthing/src/sdk/utils.ts (4)
35-84: Validate URLs to mitigate security risksDownloading files from arbitrary URLs can pose security risks like Server-Side Request Forgery (SSRF). Consider implementing URL validation to ensure downloads are only performed from trusted sources or domains.
66-78: Set a timeout for HTTP requests to prevent hangingThe HTTP GET request to download the file does not specify a timeout. Without a timeout, the request could hang indefinitely if the remote server is unresponsive. It's advisable to set a reasonable timeout to handle such scenarios gracefully.
Apply this diff to add a timeout to the HTTP request:
const arrayBuffer = yield* HttpClientRequest.get(url).pipe( HttpClientRequest.modify({ headers: {} }), + HttpClientRequest.withTimeout(10000), // Set timeout to 10 seconds httpClient.execute, Effect.flatMap((_) => _.arrayBuffer), Effect.mapError((cause) => { return { code: "BAD_REQUEST", message: `Failed to download requested file: ${cause.message}`, data: cause.toJSON() as Json, } satisfies SerializedUploadThingError; }), Effect.scoped, );
66-78: Handle network errors more robustlyWhile mapping errors, consider differentiating between client-side errors (e.g., network issues) and server-side errors to provide more precise feedback.
Apply this diff to enhance error handling:
Effect.mapError((cause) => { + let errorCode: string; + if (cause instanceof HttpClientError.RequestTimeout) { + errorCode = "REQUEST_TIMEOUT"; + } else if (cause instanceof HttpClientError.ConnectionError) { + errorCode = "NETWORK_ERROR"; + } else { + errorCode = "BAD_REQUEST"; + } return { - code: "BAD_REQUEST", + code: errorCode, message: `Failed to download requested file: ${cause.message}`, data: cause.toJSON() as Json, } satisfies SerializedUploadThingError; }),
112-144: Ensure all possible errors are properly handledCurrently,
uploadFilehandlesConfigErrorandResponseError, but other unexpected errors may not be caught, potentially leading to unhandled exceptions. Consider adding a catch-all error handler.Apply this diff to include a generic error handler:
const presigned = yield* generatePresignedUrl( file, opts?.contentDisposition ?? "inline", opts?.acl, ).pipe( Effect.catchTag("ConfigError", () => Effect.fail({ code: "INVALID_SERVER_CONFIG", message: "Failed to generate presigned URL", } satisfies SerializedUploadThingError), + ), + Effect.catchAll((e) => + Effect.fail({ + code: "INTERNAL_SERVER_ERROR", + message: "An unexpected error occurred during presigned URL generation", + data: e.toJSON() as Json, + } satisfies SerializedUploadThingError), ), ); const response = yield* uploadWithoutProgress(file, presigned).pipe( Effect.catchTag("ResponseError", (e) => Effect.fail({ code: "UPLOAD_FAILED", message: "Failed to upload file", data: e.toJSON() as Json, } satisfies SerializedUploadThingError), + ), + Effect.catchAll((e) => + Effect.fail({ + code: "INTERNAL_SERVER_ERROR", + message: "An unexpected error occurred during file upload", + data: e.toJSON() as Json, + } satisfies SerializedUploadThingError), ), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.changeset/modern-numbers-cough.md(1 hunks)packages/uploadthing/src/sdk/index.ts(5 hunks)packages/uploadthing/src/sdk/utils.ts(3 hunks)packages/uploadthing/test/sdk.test.ts(2 hunks)
🔇 Additional comments (3)
.changeset/modern-numbers-cough.md (1)
1-5: LGTM! The changeset correctly documents the version bump and changes.
The patch version bump is appropriate for this bug fix, and the description clearly communicates the improvement in error handling for the utapi.uploadFilesFromUrl method.
packages/uploadthing/src/sdk/utils.ts (1)
86-110: Verify presigned URL generation for correctness
Ensure that generatePresignedUrl correctly generates presigned URLs with all necessary parameters and proper signing.
Run the following script to verify the presigned URL generation:
✅ Verification successful
Presigned URL generation implementation is correct and well-tested
The implementation of generatePresignedUrl is verified to be correct based on:
- All required parameters (file metadata, content disposition, ACL) are properly passed to the underlying
generateSignedURLfunction - The
generateSignedURLimplementation includes:- Proper URL signing using HMAC-SHA256
- Expiration time handling
- Secure parameter encoding
- Comprehensive test coverage including signature verification
- The implementation is protected against tampering through signature verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that generatePresignedUrl includes all required data fields.
# Expected result: The presigned URL should contain query parameters with correct signatures.
# Test: Search for the generateSignedURL invocation and check the data fields.
ast-grep --pattern $'
const generatePresignedUrl = ($$_) => {
$$$
const url = yield* generateSignedURL($_, $_, {
data: {
"x-ut-identifier": $_,
"x-ut-file-name": $_,
"x-ut-file-size": $_,
"x-ut-file-type": $_,
"x-ut-custom-id": $_,
"x-ut-content-disposition": $_,
"x-ut-acl": $_,
},
});
$$$
}'
Length of output: 352
Script:
#!/bin/bash
# Let's try a different approach to verify the presigned URL generation
# 1. Find the implementation of generateSignedURL
rg -A 10 "export const generateSignedURL"
# 2. Find any tests related to presigned URL generation
rg -l "generatePresignedUrl|generateSignedURL" --glob "**/*.test.*" --glob "**/*.spec.*"
# 3. Check for any related utility functions
rg "function generateKey|const generateKey" -A 5
Length of output: 1700
Script:
#!/bin/bash
# Let's examine the test file to verify the implementation
# Check the test implementation for generateSignedURL
cat packages/shared/test/crypto.test.ts
# Also check the full implementation of generateSignedURL
cat packages/shared/src/crypto.ts
Length of output: 10057
packages/uploadthing/src/sdk/index.ts (1)
183-188: Add error handling for downloadFile failures.
Ensure that errors from downloadFile are properly propagated and handled, so that uploadFilesFromUrl can respond appropriately to download failures.
Run the following script to verify error handling in downloadFile:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/uploadthing/src/sdk/utils.ts (2)
52-55: Improve filename fallback logicThe current filename fallback logic could be enhanced to handle edge cases better.
Consider this improved implementation:
- name = url.pathname.split("/").pop() ?? "unknown-filename", + name = url.pathname.split("/").pop()?.replace(/[#?].*$/, "") || + `download-${new Date().toISOString()}`,
93-101: Add metadata validationConsider validating the file metadata before including it in the presigned URL to prevent potential issues with invalid or malformed data.
Consider adding these validations:
+ // Validate metadata before generating URL + if (!file.name || !file.size) { + return yield* Effect.fail({ + code: "BAD_REQUEST", + message: "Invalid file metadata: name and size are required", + } satisfies SerializedUploadThingError); + } data: { "x-ut-identifier": appId, "x-ut-file-name": file.name,packages/uploadthing/src/sdk/index.ts (2)
147-152: Consider enhancing error context in uploadFiles.While the error handling is good, consider adding more context about the specific file that failed when an error occurs.
uploadFile(file, opts ?? {}).pipe( Effect.match({ onSuccess: (data) => ({ data, error: null }), - onFailure: (error) => ({ data: null, error }), + onFailure: (error) => ({ + data: null, + error: error instanceof UploadThingError + ? error + : new UploadThingError({ + code: "UPLOAD_FAILED", + message: `Failed to upload file: ${file.name}`, + cause: error + }) + }), }), ),
198-204: Consider enhancing error context for URL downloads.While the error handling is good, consider adding more context about the specific URL that failed when an error occurs.
downloadFile(url).pipe( Effect.flatMap((file) => uploadFile(file, opts ?? {})), Effect.match({ onSuccess: (data) => ({ data, error: null }), - onFailure: (error) => ({ data: null, error }), + onFailure: (error) => ({ + data: null, + error: error instanceof UploadThingError + ? error + : new UploadThingError({ + code: "DOWNLOAD_FAILED", + message: `Failed to download file from URL: ${ + typeof url === 'string' ? url : url.url + }`, + cause: error + }) + }), }), ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/uploadthing/src/sdk/index.ts(5 hunks)packages/uploadthing/src/sdk/utils.ts(2 hunks)
🔇 Additional comments (2)
packages/uploadthing/src/sdk/utils.ts (1)
50-50: Add error handling for URL constructor
The URL constructor can throw if given an invalid URL. This needs to be handled as mentioned in the previous review.
packages/uploadthing/src/sdk/index.ts (1)
82-94: Good improvement in configuration error handling!
The addition of Effect.catchTag for ConfigError with a detailed error message improves the debugging experience by providing clear context about configuration issues.
ref: https://discord.com/channels/966627436387266600/1307658378968961138
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests