search

parse-community / parse-server-s3-adapter

AWS S3 file storage adapter for Parse Server
Other
80 stars 83 forks source link

fix: Upgrade to AWS JS SDK v3 #218

Closed mman closed 1 month ago

mman commented 6 months ago

Closes: https://github.com/parse-community/parse-server-s3-adapter/issues/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?

parse-github-assistant[bot] commented 6 months ago

Thanks for opening this pull request!

mtrezza commented 6 months ago

@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.

mman commented 6 months ago

@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.

pdkcoder commented 1 month ago

Hi @mman Did you get time to fix this?

mtrezza commented 1 month ago

See https://github.com/parse-community/parse-server-s3-adapter/pull/220

mman commented 1 month ago

See #220

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