-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate from s3 sdk v2 to v3 #6731
Conversation
Need to polish about |
Appreciate the help here 🙏 |
package.json
Outdated
@@ -47,6 +47,11 @@ | |||
"> 0.25%, not dead" | |||
], | |||
"dependencies": { | |||
"@aws-sdk/client-s3": "3.535.0", | |||
"@aws-sdk/lib-storage": "3.535.0", | |||
"@aws-sdk/s3-presigned-post": "3.535.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My local env can work without importing @aws-sdk/s3-presigned-post
as expected. But CI cannot find this module so I imported it here. Don't know the root cause and I found a similar issue with aws-sdk/middleware-endpoint
before(aws/aws-sdk-js-v3#3983).
region: env.AWS_REGION, | ||
endpoint: this.getEndpoint(), | ||
signatureVersion: "v4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is deprecated and now is replaced by import "@aws-sdk/signature-v4-crt";
according to https://github.com/aws/aws-sdk-js-v3#functionality-requiring-aws-common-runtime-crt
ready for review |
So, there is a bug here – when downloading a document that includes an embedded image (Document -> ... -> Download -> Markdown), the image in the generated zip file is 0 bytes. This would suggest that getFileStream is not working correctly. I checked out |
server/storage/files/S3Storage.ts
Outdated
try { | ||
return this.client | ||
.getObject({ | ||
this.client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response is no longer returned, can we restore this to async/await style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in d520b03
(#6731) . I'm not so familiar with all promise, await, and async things. Please feel free to correct me if I'm doing wrong.
From my local test, it's been fixed by |
I really don't want to change the signature of the storage interface here, it's weird to have a method called |
I assume this means using async/await in |
I think you're right, this is a terrible limitation of the new version – how annoying. |
invariant( | ||
env.AWS_S3_UPLOAD_BUCKET_NAME, | ||
"AWS_S3_UPLOAD_BUCKET_NAME is required" | ||
); | ||
|
||
this.client | ||
return this.client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed to be a bug – nothing returned here, but with this change I think it's ready to merge
close #5683. Nothing breaking, test with my local setup and everything works.
Noticeable change:
The s3 SDK v3 method
PutObjectCommand
doesn't support stream input and it requires acontent-length
field to be set, which is discussed in aws/aws-sdk-js-v3#2348.So I didn't use
PutObjectCommand
because I think loading whole files into the buffer may cause a burden. As the issue comment above suggests, I use theUpload
class from@aws-sdk/lib-storage
which is compatible with v2.