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

fix: Upgrade to AWS JS SDK v3 #218

Closed
wants to merge 2 commits into from
Closed

Conversation

mman
Copy link

@mman mman commented Feb 27, 2024

Closes: #197

The changes appear to be rather minimal and for now working in my staging environment.

Need a way to actually run proper tests and fix any remaining issues.

@mtrezza Could you help me understand how to run the tests? Or will they run automatically on PR?

Copy link

parse-github-assistant bot commented Feb 27, 2024

Thanks for opening this pull request!

@mtrezza
Copy link
Member

mtrezza commented Feb 27, 2024

@mman Did you try npm test locally? The tests won't run automatically in this PR, I believe because this is your first PR in this repo so we need to approve them manually to run. But if you get them to pass locally, then the CI here is just to ensure it works with multiple Node versions.

@mtrezza mtrezza changed the title Update to AWS JS SDK v3 fix: Upgrade to AWS JS SDK v3 Feb 27, 2024
@mman
Copy link
Author

mman commented Feb 28, 2024

@mtrezza So I made some good progress on making npm test pass on my local machine talking to AWS S3. Couple issues remain:

1/ The interface for FilesAdapter.getFileLocation (https://github.com/mman/parse-server/blob/af72a4753967ad5c015f800830510bf928bdc107/src/Adapters/Files/FilesAdapter.js#L64) declares the method to return Absolute URL string synchronously.

However notes on migration to v3 mention that there is no sync method to generate pre-signed URL anymore (https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/migrating/notable-changes/). My JS experience is lacking in what to do now. Basically all remaining tests now fail because I was not able to make getFileLocation work as intended, and I am not sure I even understand what the intended behaviour in case of S3 is.

2/ Using V3 putObject method instead of V2 upload method returns different Body type back.

The original V2 upload (https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#upload-property) returns some kind of stream or Buffer?

V3 returns https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-smithy-types/TypeAlias/NodeJsRuntimeStreamingBlobPayloadOutputTypes/ which is basically a http response wrapper that can be converted to byte array or string (https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-smithy-types/TypeAlias/SdkStream/). I have for now used conversion to string, but I feel it is not appropriate.

The FilesAdatapter interface does not mention what getFileData should return, but one of the tests assumes it is instance of Buffer.

Need some help here to make progress.

@@ -190,22 +190,23 @@ describe('S3Adapter tests', () => {
});

it('should accept endpoint as an override option in args', () => {
Copy link
Author

Choose a reason for hiding this comment

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

Looks like the AWS.Endpoint API simply disappeared and I am not sure what to do here.

// if (!this._baseUrl) {
// return presignedUrl;
// }
// }

Copy link
Author

Choose a reason for hiding this comment

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

I have disabled this test as I do not know how to properly migrate to AWS SDK v3 which does not offer a sync version of getSignedUrl anymore.

@@ -165,7 +178,7 @@ class S3Adapter {
if (data && !data.Body) {
return reject(data);
}
return resolve(data.Body);
Copy link
Author

Choose a reason for hiding this comment

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

Here I cast data.Body to string which is most probably wrong as we should pass up a stream?

@thphuccoder
Copy link

Hi @mman Did you get time to fix this?

@mtrezza
Copy link
Member

mtrezza commented Aug 10, 2024

See #220

@mman
Copy link
Author

mman commented Aug 10, 2024

See #220

Much cleaner implementation, will close this PR in favor of #220

@mman mman closed this Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to AWS JS SDK v3 needed
3 participants