-
-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat: Migrate S3 Client from AWS SDK v2 (deprecated) to v3 #221
base: master
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! |
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. |
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. |
@mtrezza I would love to test it but currently I am low-priority stuck on two conflicting issues:
that I used to work around push adapter version issues fails with: And when I convert my code to ESM, parse server fails to start with (simplified):
not sure what is the solution yet but will have a time to look into it next week... |
@mman does this help you? parse-community/parse-server#9316 (comment) |
I think it's actually this one: parse-community/parse-server#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 |
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 |
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 #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. |
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? |
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.
Looks good! Thanks for your effort. I think this is good to merge. Just to confirm, there is no breaking change in this PR, correct? In other words there's no action required for the developer when updating the s3 storage adapter - not in code nor in AWS S3 config.
@mman have you found anything unexpected while you've been testing the adapter?
Tried running this in my env, ran into some issues since I am using the Erroring out with: 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?
|
@vahidalizad I see that in v3 you now need to put the secret key/id under s3Options.credentials. That case is handled when you have secretKey and accessKey right under options. But not in the case when the secrets are put into Besides above, I tested with a working configuration and it seems to be working perfectly. Thanks for your efforts! 😄 |
In general we want to avoid breaking changes if possible. If the benefit is worth a breaking change, then we can adopt it and release as a new major version. Otherwise, we'd add the missing test case for how @jimnor0xF sets the credentials in v2 and we'd adapt this PR to make the same test pass with v3. |
Thank you for the feedback! You're correct, the AWS SDK v3 moved all credential-related values from the main parameters into a dedicated credentials key. I believe this change was made to consolidate the growing number of credential-related parameters into a single, structured object for better organization. In my opinion, it would be better to align with the SDK v3 approach to avoid potential confusion and ensure consistency with their design. We could document this change clearly in the README That said, if you’d prefer to avoid the breaking change, I can adjust the PR to extend the main parameters for backwards compatibility. Should we limit this extension to just Let me know your thoughts |
I'd agree. Could you add a small migration guide to the README? And I think we'd need an additional test case, because the credential issue hasn't been picked up by the CI. The test would use the v3 credential structure to identify a similar issue in case the credential structure changes again in the future. We'll then publish a release 4.0 with your PR. |
@vahidalizad Could you please take a look at my issue and suggested fixes at #230. If I am not mistaken in my investigation the AWS S3 JS SDK v3 changed the behavior from of |
Summary
This pull request migrates the S3 client implementation from AWS SDK v2 to v3.
Changes
S3Client
from@aws-sdk/client-s3
package.getObject
,putObject
, etc.) with their v3 equivalents (GetObjectCommand
,PutObjectCommand
, etc.).Release Dependency
This pull request is intended for after the release of the asynchronous
getFileLocation
function on theparse-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
Please review the changes and provide feedback. Thank you for your time and consideration.
Closes: #197