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

feat: Revise createFile logic to return modified filenames and location #242

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

AdrianCurtin
Copy link

@AdrianCurtin AdrianCurtin commented Jan 16, 2025

  • Add error handling for key generation
  • Standardize return object with location, url (possibly signed), and filename / key without prefix
  • Add support for optional config parameter to allow getFileLocation to be called internally (prevent a double call from the serverside potentially)
  • Return s3 response explicitly as a separate variable

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

- 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
Copy link

parse-github-assistant bot commented Jan 16, 2025

Thanks for opening this pull request!

@AdrianCurtin AdrianCurtin changed the title Revise createFile logic to preserve key & location bug: Revise createFile logic to preserve key & location Jan 16, 2025
@AdrianCurtin AdrianCurtin changed the title bug: Revise createFile logic to preserve key & location feat: Revise createFile logic to return modified filenames and location Jan 17, 2025
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.24%. Comparing base (d2230e8) to head (b9df8b6).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Not sure what the status of this PR is, but there are tests failing.

@AdrianCurtin
Copy link
Author

@mtrezza Which tests are failing?

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2025

See the CI job panel

@AdrianCurtin
Copy link
Author

@mtrezza Added tests for the two conditions that were missing

@mtrezza mtrezza requested a review from a team February 7, 2025 01:13
@AdrianCurtin
Copy link
Author

@mtrezza
that should clear up the lint and codecov report

@mtrezza
Copy link
Member

mtrezza commented Mar 8, 2025

@vahidalizad What do you think?


let key_without_prefix = filename;
if (this._generateKey instanceof Function) {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe removing the try block would achieve the same result while preserving the original error trace and improving readability.

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.

Objects uploaded with generateKey return incorrect locations
3 participants