Handle NoSuchKey error gracefully

[betschki]

I am using Ghos3 for all Ghost instances on magicpages.co and noticed an issue a few weeks ago:

When a Ghost export (JSON) is imported into Ghost, images aren't included. Usually, Ghost would simply show a broken image, since it resolves to a 404 error. With Ghos3, however, missing images weren't locally stored and Ghos3 would look for them in the attached S3 storage.

Now, what happens if the image isn't found is that a NoSuchKey error is thrown – but Ghost doesn't know how to handle this. Therefore, it runs into a timeout, which temporarily makes the Ghost instances unusable.

I have implemented a graceful error handling for the NoSuchKey error, which solves the problem and correctly resolves the image as not found.

The Ghost logs before:

[2024-03-12 13:35:33] ERROR "GET /content/images/wp/2024/01/image.webp" 500 194702ms

An unexpected error occurred, please try again.

0=T 1=h 2=e 3= 4=s 5=p 6=e 7=c 8=i 9=f 10=i 11=e 12=d 13= 14=k 15=e 16=y 17= 18=d 19=o 20=e 21=s 22= 23=n 24=o 25=t 26= 27=e 28=x 29=i 30=s 31=t 32=.

Error ID:
32a47c20-e075-11ee-890b-63a89edeff5c

Error Code:
UNEXPECTED_ERROR

NoSuchKey: The specified key does not exist.
at module.exports.prepareError (/var/www/ghost/versions/5.80.2/node_modules/@tryghost/mw-error-handler/lib/mw-error-handler.js:102:19)
at de_NoSuchKeyRes (/var/www/ghost/content/adapters/storage/ghos3/node_modules/@aws-sdk/client-s3/dist-cjs/protocols/Aws_restXml.js:6082:23)
at de_GetObjectCommandError (/var/www/ghost/content/adapters/storage/ghos3/node_modules/@aws-sdk/client-s3/dist-cjs/protocols/Aws_restXml.js:4327:25)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async /var/www/ghost/content/adapters/storage/ghos3/node_modules/@smithy/middleware-serde/dist-cjs/deserializerMiddleware.js:7:24
at async /var/www/ghost/content/adapters/storage/ghos3/node_modules/@aws-sdk/middleware-signing/dist-cjs/awsAuthMiddleware.js:14:20
at async /var/www/ghost/content/adapters/storage/ghos3/node_modules/@smithy/middleware-retry/dist-cjs/retryMiddleware.js:27:46
at async /var/www/ghost/content/adapters/storage/ghos3/node_modules/@aws-sdk/middleware-flexible-checksums/dist-cjs/flexibleChecksumsMiddleware.js:63:20
at async /var/www/ghost/content/adapters/storage/ghos3/node_modules/@aws-sdk/middleware-sdk-s3/dist-cjs/region-redirect-endpoint-middleware.js:14:24
at async /var/www/ghost/content/adapters/storage/ghos3/node_modules/@aws-sdk/middleware-sdk-s3/dist-cjs/region-redirect-middleware.js:9:20
at async /var/www/ghost/content/adapters/storage/ghos3/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26
at async /var/www/ghost/content/adapters/storage/ghos3/index.js:145:24

The logs after implementing my changes:

[2024-03-12 18:29:36] INFO "GET /content/images/wp/2024/01/image.webp" 404 182ms

I am already using this successfully from my fork and think this can benefit all Ghos3 users.

[laosb]

I haven't checked it against Ghost source, but are we supposed to finalize the response in this handler by using send? If so, should we also finalize it in 500 case?

[betschki]

Good question! Generally, I would expect Ghost to handle it, but I have just done some tests which would indicate the opposite.

Once I finalize the response with the .send(), there is no timeout.

So, the simplest solution to the timeout error would indeed be removal of the next(err) and implementation of send().

In my eyes, the 500 error does, however, not accurately reflect the issue at hand – the missing key.

After thinking about this once again, the more elegant (and extendable) solution would probably be this:

} catch (err) {
    switch (err.name) {
        case 'NoSuchKey':
            res.status(404).send('Specified key does not exist');
            break;
        default:
            res.status(500).send('Internal server error');
    }
}

Let me know what you think. Happy to adapt :)

[laosb]

I just checked Ghost's own local file storage adapter, it seems we should call next() with the error type defined in @tryghost/errors.

I think we should use that approach instead of finalizing the response.

[laosb]

Thank you for your contribution, but let's do it the correct way:

we should call next() with the error type defined in @tryghost/errors.

[laosb]

Thank you for your contribution from a while ago! I've mended this PR to use the official helper packages and it's landed in #21 with your original commit included.