feat(image-adapter): improve error handling and status codes #893
Conversation
…e optimization failures
…h between client and server errors
…des and error classification
…h unified error classification
🦋 Changeset detectedLatest commit: aa36783 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
commit: |
vicb
changed the title
|
Just an update. This seems to be working the dream in our prod environment so far. |
| try { | ||
| // Case 1: remote image URL => download the image from the URL | ||
| // EXTERNAL IMAGE HANDLING |
There was a problem hiding this comment.
| // EXTERNAL IMAGE HANDLING | |
| // remote image URL => download the image from the URL |
|
|
||
| // INTERNAL IMAGE HANDLING (S3) |
There was a problem hiding this comment.
| // INTERNAL IMAGE HANDLING (S3) | |
| // local image => download the image from the provided ImageLoader (default is S3) |
| } | ||
| const message = "Empty response body from the S3 request."; | ||
| const clientError = new IgnorableError(message, 400); | ||
| error("Empty response from S3", clientError); |
There was a problem hiding this comment.
| error("Empty response from S3", clientError); | |
| error("Empty response from ImageLoader", clientError); |
|
|
||
| // Special handling for S3 ListBucket permission errors | ||
| // AWS SDK v3 nests error details deeply within the error object | ||
| const isListBucketError = |
There was a problem hiding this comment.
All the S3 related stuff should not be here. https://github.com/opennextjs/opennextjs-aws/blob/main/packages/open-next/src/overrides/imageLoader/s3.ts
This is the ImageLoader and where you should handle S3 related error, not here.
Here we should only handle our error, or basic error
|
|
||
| // Still include error details in headers for debugging only | ||
| const errorMessage = isListBucketError ? "Access denied" : message; | ||
| res.setHeader("x-nextjs-internal-error", errorMessage); |
There was a problem hiding this comment.
Does this one propagates to the end user ?
| @@ -114,10 +115,19 @@ export async function defaultHandler( | |||
| ); | |||
| return buildSuccessResponse(result, options?.streamCreator, etag); | |||
| } catch (e: any) { | |||
| error("Failed to optimize image", e); | |||
| // All image-related errors should be treated as client errors | |||
There was a problem hiding this comment.
Not true, if something fails on the server it is not client errors
Moved to new PR against a non-main branch.
Original PR here: #886
Fixes #885