Skip to content
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

support: Upgrade aws-sdk to v3 #5863

Merged
merged 13 commits into from
May 31, 2022

Conversation

mudana-grune
Copy link
Contributor

https://youtrack.weseek.co.jp/issue/GW-7807

  • Upgrade package to aws-sdk v3
  • Make async / await function available
  • Change syntax to typescript

Testing Steps

Configure AWS

  • Configure AWS in File upload settings (admin/app)
    image

Upload attachment from wiki page

  • Upload attachment from wiki page and make sure attachment displayed in the preview

image

  • Check url of the attchment (make sure the url already points to the specified url bucket and make sure we can download the attachment)
    image
  • Download and remove attachment from attachment data (make sure download and delete atachment success without error)
    image

Upload Attachment from user profile picture setting

  • Make sure upload and delete profile picture is success in Set Profile Picture
    image
  • Check profile picture source and make sure the url already points to specified bucket url
    image

mudana-grune and others added 5 commits April 5, 2022 17:40
https://youtrack.weseek.co.jp/issue/GW-7758
- Install @aws-sdk/client-s3
- Convert file-uploader aws to typescript (on progress)
- Install @aws-sdk/s3-request-presigner to get signedUrl
https://youtrack.weseek.co.jp/issue/GW-7758
- Add delete file and upload file
- Add forcePathStyle in aws config
https://youtrack.weseek.co.jp/issue/GW-7631
- Remove aws.js
- Remove Expires options in getSignedUrl function param
- Update getSignedUrl, add expiresIn option
@mudana-grune mudana-grune temporarily deployed to VRT May 18, 2022 10:19 Inactive
@hakumizuki hakumizuki temporarily deployed to VRT May 24, 2022 05:45 Inactive
@@ -207,7 +212,7 @@ module.exports = function(crowi) {

let stream;
try {
stream = s3.getObject(params).createReadStream();
stream = s3.send(new GetObjectCommand(params));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type of S3Client.send is not Readable.

I think s3.send(new GetObjectCommand(params)).Body is what we want here. Please check and change if necessary.

See:

export interface GetObjectOutput {
    Body?: Readable | ReadableStream | Blob;

    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to return stream.Body

@hakumizuki
Copy link
Contributor

I have also improved type system code in this commit. Please check this as well.

@hakumizuki hakumizuki temporarily deployed to VRT May 30, 2022 08:58 Inactive
@hakumizuki hakumizuki temporarily deployed to VRT May 30, 2022 09:14 Inactive
https://youtrack.weseek.co.jp/issue/GW-7807
- Change return type of findDeliveryFile method
- Remove GetObjectCommandOutput import
@mudana-grune mudana-grune temporarily deployed to VRT May 30, 2022 11:25 Inactive
https://youtrack.weseek.co.jp/issue/GW-7807
- Update return type of findDeliveryFilemethod
@mudana-grune mudana-grune temporarily deployed to VRT May 30, 2022 11:42 Inactive
Copy link
Contributor

@hakumizuki hakumizuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, most of your code looks good.
To close this PR, please make sure all the apis works correctly.

}
catch (err) {
logger.error(err);
throw new Error(`Coudn't get file from AWS for the Attachment (${attachment._id.toString()})`);
}

// return stream.Readable
return stream;
return stream.Body;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should get the Body attribute at L216 so the variable name matches to its value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!! Important !!

Please make sure the findDeliveryFile method works correctly.
Especially, please test that the app.get('/download/:id([0-9a-z]{24})' , loginRequired, attachment.api.download); endpoint works correctly. Please write down what and how you checked this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract Body attribute from GetObjectCommandOutput as type for stream variable type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upload , delete and get attachment already woking properly

Download testing

  • Open attachment data
    attachment-data
  • Attachment list will shown
    image
  • Click download button
    download
  • Files will be downloaded properly
    image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

オリジナルのファイル名でダウンロードできないといけないのでは?

routes/attachment.js には

'Content-Disposition': `attachment;filename*=UTF-8''${encodeURIComponent(attachment.originalName)}`

をセットしてる箇所があるので。

https://youtrack.weseek.co.jp/issue/GW-7807
- Extract Body attribute from GetObjectCommandOutput Interface as type
@mudana-grune mudana-grune temporarily deployed to VRT May 31, 2022 04:25 Inactive
@hakumizuki hakumizuki self-requested a review May 31, 2022 05:45
@hakumizuki hakumizuki temporarily deployed to VRT May 31, 2022 05:52 Inactive
@reg-suit
Copy link

reg-suit bot commented May 31, 2022

✨✨ That's perfect, there is no visual difference! ✨✨

Check out the report here.

@yuki-takei yuki-takei changed the title feat: gw7807 upgrade aws to v3 and typescriptize aws support: Upgrade aws-sdk to v3 May 31, 2022
@yuki-takei yuki-takei merged commit a550bc3 into master May 31, 2022
@yuki-takei yuki-takei deleted the feat/gw7807-upgrade-aws-to-v3-and-typescriptize-aws branch May 31, 2022 07:00
@github-actions github-actions bot mentioned this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants