refactor: Migrate S3 Client from AWS SDK v2 to v3

[vahidalizad]

Summary

This pull request migrates the S3 client implementation from AWS SDK v2 to v3.

Changes

1. Initialization: Updated the S3 client initialization to use S3Client from @aws-sdk/client-s3 package.
2. Commands: Replaced v2 commands (getObject, putObject, etc.) with their v3 equivalents (GetObjectCommand, PutObjectCommand, etc.).
3. Response Handling: Modified response handling to accommodate changes in the v3 API, particularly in how streams and buffers are managed.
4. Testing: Added unit tests to cover new functionality and ensure compatibility with the existing codebase. Implemented mocks for offline testing using Jasmine.

Release Dependency

This pull request is intended for after the release of the asynchronous getFileLocation function on the parse-server repository. Please ensure that this dependency is met before merging.

The relevant pull request for adding support of asynchronous getFileLocation in adapters can be found here: parse-server PR #9271.

References

• AWS SDK for JavaScript v3 Documentation

Please review the changes and provide feedback. Thank you for your time and consideration.

Closes: #197

[parse-github-assistant[bot]]

Thanks for opening this pull request!

[mtrezza]

Continuing from https://github.com/parse-community/parse-server-s3-adapter/pull/220#issuecomment-2395340067.

I notice that this CI does not test against Parse Server, so with or without https://github.com/parse-community/parse-server/pull/9271, the CI passes. That is not good, because w/o integration test we cannot really know whether this all works together. That has been less of an issue in the past, where this adapter just had to comply with the interface. But now that it depends on a specific implementation of the interface (async support for interface methods), it would be good to test. Since none of the parse server adapters does integration tests, this is not an exception, and we could go ahead and merge this, but someone should test this end-to-end and confirm this is working before we merge.

[vahidalizad]

Continuing from #220 (comment).

I notice that this CI does not test against Parse Server, so with or without parse-community/parse-server#9271, the CI passes. That is not good, because w/o integration test we cannot really know whether this all works together. That has been less of an issue in the past, where this adapter just had to comply with the interface. But now that it depends on a specific implementation of the interface (async support for interface methods), it would be good to test. Since none of the parse server adapters does integration tests, this is not an exception, and we could go ahead and merge this, but someone should test this end-to-end and confirm this is working before we merge.

I completely agree that testing integrations is crucial, especially given the dependency on async support for interface methods. That said, I think we could merge this into a separate branch for now and make adding integration tests a separate task. We could raise an issue for that and have others contribute to the testing. Once the tests are complete and we’ve confirmed everything works end-to-end, we can merge it into the master branch.

[mtrezza]

Our CD automation setup currently does not allow maintaining feature branches. Hence we'll have to merge this into master.

I think it would be enough if you could test this out with the latest version of Parse Server that supports the async interface. Maybe @mman also has time to test this out, just to confirm it works. Then we can go ahead and merge this and open a Server e2e test as a separate issue for later on.

[mman]

@mtrezza I would love to test it but currently I am low-priority stuck on two conflicting issues:

const ParsePushAdapter = require('@parse/push-adapter').ParsePushAdapter;

that I used to work around push adapter version issues fails with: Error [ERR_REQUIRE_ESM]: require() of ES Module not supported..

And when I convert my code to ESM, parse server fails to start with (simplified):

Error [ERR_REQUIRE_ESM]: require() of ES Module ./cloud/main.js from ./node_modules/parse-server/lib/ParseServer.js not supported.

not sure what is the solution yet but will have a time to look into it next week...

[mtrezza]

@mman does this help you? https://github.com/parse-community/parse-server/issues/9316#issuecomment-2379043778

[mman]

@mman does this help you? parse-community/parse-server#9316 (comment)

I think it's actually this one: https://github.com/parse-community/parse-server/issues/7559 with workaround mentioned here: https://stackoverflow.com/questions/77638089/how-to-make-parse-server-detect-im-using-esm-now where instead of passing a cloud code path to Parse Server constructor, you actually have to import cloud code in your main .js file and pass the imported code to the constructor...

[vahidalizad]

OK in that case I'm gonna add the new integration tests too and try to sync the dependencies with the parse-server itself lets have a cleaning task as well because all the codes have changed since the last update

[mtrezza]

That would be great. We should add them in a different PR and keep this one open. Just because we now have a presumably working PR that we could be merged in case the integration test PR stales. I have opened a new issue https://github.com/parse-community/parse-server-s3-adapter/issues/222 for that.

There are repos that have integration tests which you could use as a guideline on how to set up Parse Server. See for example the Parse JS SDK - it may not have the cleanest implementation because it developed over time, but as a starting point it may be fine.

[mtrezza]

@vahidalizad Great to see you being so quick to add the changes :-) However, could you please revert the changes that are unrelated? I believe we should leave this PR in the state of https://github.com/parse-community/parse-server-s3-adapter/pull/221/commits/d4062cbc78a5514a7ee0ead554a9094fd093f555.

The changes you made in the last 2 commits may have side effects. I see for example some dependency upgrades that cannot be done like that will make workflows fail. That's why we always have separate PRs for separate issues, see also our Contribution Policy, section Scope.

[vahidalizad]

@vahidalizad Great to see you being so quick to add the changes :-) However, could you please revert the changes that are unrelated? I believe we should leave this PR in the state of d4062cb.

The changes you made in the last 2 commits may have side effects. I see for example some dependency upgrades that cannot be done like that will make workflows fail. That's why we always have separate PRs for separate issues, see also our Contribution Policy, section Scope.

I apologize for any confusion. My initial intention was to refactor the entire codebase to reduce technical debt. However, I've reverted the dependency changes and only updated Jasmine and NYC. If that poses any issues, I can revert those as well.

I also implemented integration tests with the latest version of Parse Server. Due to my new environment being on Windows, I was unable to run the test:integration command directly. Instead, I used Docker to run my MongoDB instance rather than using mongodb-runner. If you have the opportunity, it would be great if you could take a look at that.

[mtrezza]

only updated Jasmine and NYC.

If it's not related to this PR, then it would be better to revert. I would also suggest to move the integration tests to a separate PR. I appreciate your intention and ambition, but unfortunately we have seen in the past that these combined changes of unrelated things can sometimes consume a lot of time when there are issues down the road. Also, if there is an issue with a single one of them, we may have to revert the whole PR.

I suggest to refactor like so:

• Submit a new PR that only adds the integration test setup with at least 1 integration test.
• Change this PR to only contain the changes from https://github.com/parse-community/parse-server-s3-adapter/commit/d4062cbc78a5514a7ee0ead554a9094fd093f555 plus the integration tests related to this feature.
• nyc, jasmine upgrades are done automatically, you don't need to worry about them unless there is a reason you need them upgraded for a PR.
• Any other large scale refactoring (you mentioned you intended to refactor the entire code base) is a separate issue we should discuss first. Refactoring is always a risk for bugs, so we should make sure we know what and why we'd want to refactor and whether the refactor is worth the risk.

Hope that makes sense, in any case it's much appreciated that you are taking these things on!

[vahidalizad]

Thank you for the feedback! This makes total sense, and I understand this is the standard practice. The reason I combined the changes in a single PR was to avoid dealing with the merge conflict that would arise from writing integration tests for the old makeS3Adapter function (from the v2 version) and later resolving conflicts once this branch with the new makeS3Adapter function is merged.

That said, I understand the importance of keeping PRs focused, so I’ll follow your suggestion and create a separate PR for the integration tests.

I also noticed that the nyc upgrade affected the CI progress, which I’ll revert accordingly.

[mtrezza]

with the merge conflict that would arise from writing integration tests for the old makeS3Adapter function (from the v2 version) and later resolving conflicts once this branch with the new makeS3Adapter function is merged.

You are right, there is no need to write extensive integration tests for code that would be discarded anyway. But I assume there is basic adapter functionality that will not change, regardless of which AWS SDK is used. If you could just write 1 integration test for that, that's all we need. It's just to show that the integration setup works, to move it into a separate PR. I assume that the integration test can be added without any changes in the adapter code itself, as it's an addition rather than a modification, so there really shouldn't be any merge conflict.

[vahidalizad]

with the merge conflict that would arise from writing integration tests for the old makeS3Adapter function (from the v2 version) and later resolving conflicts once this branch with the new makeS3Adapter function is merged.

You are right, there is no need to write extensive integration tests for code that would be discarded anyway. But I assume there is basic adapter functionality that will not change, regardless of which AWS SDK is used. If you could just write 1 integration test for that, that's all we need. It's just to show that the integration setup works, to move it into a separate PR. I assume that the integration test can be added without any changes in the adapter code itself, as it's an addition rather than a modification, so there really shouldn't be any merge conflict.

Thanks for the clarification! To answer your point, there is a mock adapter that changes between v2 and v3, which is used in both unit and integration tests. I was trying to avoid conflicts related to that since it's in the tests and not the main code, but it doesn’t matter anymore.

I’ve created a new PR (#224) that adds the integration tests as requested. Please review and merge it when possible. After that, I’ll revert this PR to the state before adding integration tests, resolve the conflicts, and commit the updates so this PR can be finalized after we merge #224.

[mtrezza]

@vahidalizad I hope resolving the conflicts won't be too much work, but great that we have the integration tests now

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 95.52239% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.18%. Comparing base (0e0d721) to head (becb402). Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/optionsFromArguments.js	50.00%	2 Missing :warning:
index.js	98.41%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
+ Coverage   95.60%   97.18%   +1.57%     
==========================================
  Files           2        2              
  Lines         205      213       +8     
  Branches       44        0      -44     
==========================================
+ Hits          196      207      +11     
+ Misses          9        6       -3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[vahidalizad]

@vahidalizad I hope resolving the conflicts won't be too much work, but great that we have the integration tests now

I believe I’ve resolved all the conflicts and pushed the changes, but due to the latest updates, I need to create an S3 bucket and test it locally just to ensure nothing has changed there. I wrote a mock S3 adapter, but double-checking won’t hurt, so please hold off on merging for now. Also, there seems to be an issue with Parse Server 6, which I think might be related to the async function we modified.

[vahidalizad]

I've tested this adapter with a real S3 bucket, and it worked flawlessly. I believe it's ready for migration. However, before proceeding, please review the code and feel free to clean it up if you'd like to create a more standardized adapter template.

[mtrezza]

The integration tests are failing for Parse Server 6. I assume that is expected, since the adapter will require Parse Server >=7.3.0 due to https://github.com/parse-community/parse-server/issues/9268, correct? That would mean that we couldn't merge this PR before Jan 2024, as Parse Server 6 is still in LTS. But since this has been such a complex PR, I'd say let's merge this asap to avoid possibly more refactoring.

[mtrezza]

Thanks for updating this PR; I've left a few comment above; just some minor things

[mman]

@mtrezza @vahidalizad Just wanted to confirm that I have finally updated my alpha staging environment with this PR and have not seen any issues in testing. Good job!

[mtrezza]

Great, maybe someone could address the open review comments, then we can go ahead and merge.

[vahidalizad]

Sorry for the delay, I’ve been quite busy with work recently. I really appreciate your review and feedback! I’ll address the comments over the coming weekend.

[mtrezza]

Thanks @vahidalizad much appreciated!

[AdrianCurtin]

The integration tests are failing for Parse Server 6. I assume that is expected, since the adapter will require Parse Server >=7.3.0 due to parse-community/parse-server#9268, correct? That would mean that we couldn't merge this PR before Jan 2024, as Parse Server 6 is still in LTS. But since this has been such a complex PR, I'd say let's merge this asap to avoid possibly more refactoring.

It's been some time for this, but I would say you could move this as an alpha version for those who don't have parse 6 anymore.

[mtrezza]

It would be great if someone could get this over the finish line and address the open review questions.

[vahidalizad]

I believe it’s ready for a final review. Apologies for the delay in getting this done work has been particularly busy lately.

[mtrezza]

Great work, just a few tiny things that are quickly to resolve. One thing however is that instead of spying in some tests it is using a mock S3 client, which we should avoid if possible. Could you revert that back to spying?

[jimnor0xF]

Tried running this in my env, ran into some issues since I am using the s3overrides config option and passing credentials explicitly at the moment.

Erroring out with: error: Could not load credentials from any providers

import S3Adapter from '@parse/s3-files-adapter';

const awsS3Config = {
  bucket: R2_BUCKET,
  region: 'auto',
  directAccess: true,
  fileAcl: 'none',
  baseUrl: R2_BASE_URL,
  baseUrlDirect: true,
  signatureVersion: 'v4',
  s3overrides: {
    accessKeyId: R2_ACCESS_KEY_ID,
    secretAccessKey: R2_ACCESS_KEY_SECRET,
    endpoint: `https://${CF_ACCOUNT_NUMBER}.r2.cloudflarestorage.com`,
  },
};


export const config = {
...
  filesAdapter: new S3Adapter(awsS3Config),
}

However, I see that in README.md the following is mentioned, so this should be working still?

If for some reason you really need to be able to set the key and secret explicitly, you can still do it using s3overrides as described below and setting accessKeyId and secretAccessKey in the s3Overrides object.