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

@uppy/aws-s3: Fixed default shouldUseMultipart #5613

Conversation

tvt-mika
Copy link
Contributor

@tvt-mika tvt-mika commented Jan 20, 2025

Fixes #5458

Fixed a bug in the shouldUseMultipart function where it failed to return the expected result for large file sizes.

For example, a file size of ~70GB incorrectly returned false.

const shouldUseMultipart = (size) => (
  size >> 10 >> 10 > 100
)
shouldUseMultipart(75161927680) // Output: false

This issue was caused by improper use of the bitwise shift operator for size comparison.

Also, added some tests for the defaultOptions to ensure reliability and prevent similar issues in the future.

Copy link
Contributor

Diff output files
diff --git a/packages/@uppy/aws-s3/lib/index.js b/packages/@uppy/aws-s3/lib/index.js
index 7dc8cc0..b0484e6 100644
--- a/packages/@uppy/aws-s3/lib/index.js
+++ b/packages/@uppy/aws-s3/lib/index.js
@@ -58,7 +58,7 @@ const defaultOptions = {
   allowedMetaFields: true,
   limit: 6,
   getTemporarySecurityCredentials: false,
-  shouldUseMultipart: file => file.size >> 10 >> 10 > 100,
+  shouldUseMultipart: file => (file.size || 0) > 100 * 1024 * 1024,
   retryDelays: [0, 1000, 3000, 5000],
 };
 var _companionCommunicationQueue = _classPrivateFieldLooseKey("companionCommunicationQueue");

@tvt-mika
Copy link
Contributor Author

Should solve also #5458

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Thanks! One small comment

packages/@uppy/aws-s3/src/index.ts Show resolved Hide resolved
@Murderlon Murderlon merged commit b8dec76 into transloadit:main Jan 20, 2025
5 checks passed
@tvt-mika tvt-mika deleted the fix-aws-s3-plugin-default-should-use-multipart branch January 21, 2025 05:40
github-actions bot added a commit that referenced this pull request Jan 22, 2025
| Package         | Version | Package         | Version |
| --------------- | ------- | --------------- | ------- |
| @uppy/aws-s3    |   4.2.3 | @uppy/tus       |   4.2.2 |
| @uppy/companion |   5.5.1 | uppy            |  4.13.1 |

- @uppy/tus: fix resumeFromPreviousUpload race condition (Merlijn Vos / #5616)
- @uppy/aws-s3: Fixed default shouldUseMultipart (Mika Laitinen / #5613)
- meta: build(deps): bump docker/build-push-action from 6.11.0 to 6.12.0 (dependabot[bot] / #5611)
- @uppy/aws-s3: remove console.error (Mikael Finstad / #5607)
- @uppy/companion: unify http error responses (Mikael Finstad / #5595)
Murderlon added a commit that referenced this pull request Jan 23, 2025
* main: (38 commits)
  Release: [email protected] (#5617)
  @uppy/tus: fix resumeFromPreviousUpload race condition (#5616)
  @uppy/aws-s3: Fixed default shouldUseMultipart (#5613)
  build(deps): bump docker/build-push-action from 6.11.0 to 6.12.0 (#5611)
  @uppy/aws-s3: remove console.error (#5607)
  @uppy/companion: unify http error responses (#5595)
  Release: [email protected] (#5605)
  @uppy/aws-s3: always set S3 meta to UppyFile & include key (#5602)
  @uppy/companion: fix forcePathStyle boolean conversion (#5308)
  Fix Webpack CI (#5604)
  @uppy/aws-s3: allow uploads to fail/succeed independently (#5603)
  Revert "@uppy/aws-s3: allow uploads to fail/succeed independently"
  @uppy/aws-s3: allow uploads to fail/succeed independently
  Add types for css files (#5591)
  @uppy/unsplash: make utmSource optional (#5601)
  build(deps): bump docker/setup-qemu-action from 3.2.0 to 3.3.0 (#5599)
  build(deps): bump docker/build-push-action from 6.10.0 to 6.11.0 (#5600)
  @uppy/companion: add COMPANION_TUS_DEFERRED_UPLOAD_LENGTH (#5561)
  Release: [email protected] (#5590)
  Import types consistently from @uppy/core (#5589)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default shouldUseMultipart setting for AWS S3 plugin doesn't always work correctly for really large files
2 participants