feat: Revise createFile logic to return modified filenames and location #242
Conversation
- Add error handling for key generation - Standardize return object with location, url, and filename - Add support for optional config parameter - Return s3 response explicitly as a separate variable
Thanks for opening this pull request! |
AdrianCurtin
changed the title
AdrianCurtin
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #242 +/- ##
==========================================
+ Coverage 97.18% 97.24% +0.06%
==========================================
Files 2 2
Lines 213 218 +5
==========================================
+ Hits 207 212 +5
Misses 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mtrezza Which tests are failing? |
|
See the CI job panel |
|
@mtrezza Added tests for the two conditions that were missing |
|
@mtrezza |
|
@vahidalizad What do you think? |
|
@mtrezza I think this is a good one! I had some minor change suggestions, but other than that, it looks great. I also reviewed the code in the Parse Server PR, which is necessary for this to work, and I think that looks good as well. |
|
Great! Are you able to post your change suggestion here using the Review feature on GitHub? Or do you still require permissions for that? |
Yes, I created a review successfully! |
|
Are you able to post the review in the "Files changed" tab? It should generate a GitHub comment here with your review feedback. |
|
|
||
| let key_without_prefix = filename; | ||
| if (this._generateKey instanceof Function) { | ||
| try { |
There was a problem hiding this comment.
I believe removing the try block would achieve the same result while preserving the original error trace and improving readability.
| return { | ||
| location: location, // actual upload location, used for tests | ||
| name: key_without_prefix, // filename in storage, consistent with other adapters | ||
| s3_response: response, // raw s3 response |
There was a problem hiding this comment.
I think we don't need the s3_response key and we can remove it.
| @@ -180,7 +187,17 @@ class S3Adapter { | |||
| const endpoint = this._endpoint || `https://${this._bucket}.s3.${this._region}.amazonaws.com`; | |||
| const location = `${endpoint}/${params.Key}`; | |||
|
|
|||
| return Object.assign(response || {}, { Location: location }); | |||
| let url; | |||
| if (Object.keys(config).length != 0) { // if config is passed, we can generate a presigned url here | |||
There was a problem hiding this comment.
Instead of checking only for the presence of keys, let's ensure we are checking for the correct keys.
Maybe change to:
if (config?.mount && config?.applicationId)
If parse-community/parse-server#9557 is merged,
this will address #237
This change will affect specific tests that check the response.Location argument and may affect any direct adapter usage of createFile that explicitly uses the s3 server response.
Closes: #237