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: always set S3 meta to UppyFile & include key #5602

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Jan 14, 2025

Closes #5513
Closes #5586
Closes #5565
Closes #5569

  • uppy.on('upload-success', (file, response) => {}) now correctly has response.body populated.
  • uppy.getFile(someId).s3Multipart is now always populated (previously only for multipart uploads).
  • uppy.getFile(someId).meta does not contain S3 related data. Use file.s3Multipart for that.

@Murderlon Murderlon requested a review from mifi January 14, 2025 10:44
@Murderlon Murderlon self-assigned this Jan 14, 2025
@Murderlon Murderlon changed the title @uppy/aws-s3: always set S3 to UppyFile & include key @uppy/aws-s3: always set S3 meta to UppyFile & include key Jan 14, 2025
Copy link
Contributor

github-actions bot commented Jan 14, 2025

Diff output files
diff --git a/packages/@uppy/aws-s3/lib/HTTPCommunicationQueue.js b/packages/@uppy/aws-s3/lib/HTTPCommunicationQueue.js
index 29c5f4b..d1bfe35 100644
--- a/packages/@uppy/aws-s3/lib/HTTPCommunicationQueue.js
+++ b/packages/@uppy/aws-s3/lib/HTTPCommunicationQueue.js
@@ -305,7 +305,7 @@ export class HTTPCommunicationQueue {
         signature = await _classPrivateFieldLooseBase(this, _fetchSignature)[_fetchSignature](
           _classPrivateFieldLooseBase(this, _getFile)[_getFile](file),
           {
-            uploadId,
+            uploadId: uploadId,
             key,
             partNumber,
             body: chunkData,
@@ -389,6 +389,7 @@ async function _shouldRetry2(err, retryDelayIterator) {
   return true;
 }
 async function _nonMultipartUpload2(file, chunk, signal) {
+  var _ref3;
   const {
     method = "POST",
     url,
@@ -429,8 +430,17 @@ async function _nonMultipartUpload2(file, chunk, signal) {
     onComplete,
     signal,
   }).abortOn(signal);
-  return "location" in result ? result : {
-    location: removeMetadataFromURL(url),
+  const key = fields == null ? void 0 : fields.key;
+  if (!key) {
+    console.error("Expected `fields.key` to be returend but the backend/Companion");
+  }
+  _classPrivateFieldLooseBase(this, _setS3MultipartState)[_setS3MultipartState](file, {
+    key: key,
+  });
+  return {
     ...result,
+    location: (_ref3 = result.location) != null ? _ref3 : removeMetadataFromURL(url),
+    bucket: fields == null ? void 0 : fields.bucket,
+    key,
   };
 }
diff --git a/packages/@uppy/companion/lib/server/controllers/s3.js b/packages/@uppy/companion/lib/server/controllers/s3.js
index 7526f30..ff54520 100644
--- a/packages/@uppy/companion/lib/server/controllers/s3.js
+++ b/packages/@uppy/companion/lib/server/controllers/s3.js
@@ -124,6 +124,7 @@ module.exports = function s3(config) {
       res.json({
         key: data.Key,
         uploadId: data.UploadId,
+        bucket: data.Bucket,
       });
     }, next);
   }
@@ -359,6 +360,8 @@ module.exports = function s3(config) {
     ).then(data => {
       res.json({
         location: data.Location,
+        key: data.Key,
+        bucket: data.Bucket,
       });
     }, next);
   }

@Murderlon Murderlon merged commit 45aecae into main Jan 15, 2025
19 checks passed
@Murderlon Murderlon deleted the s3-meta-in-files branch January 15, 2025 09:30
@github-actions github-actions bot mentioned this pull request Jan 15, 2025
github-actions bot added a commit that referenced this pull request Jan 15, 2025
| Package         | Version | Package         | Version |
| --------------- | ------- | --------------- | ------- |
| @uppy/aws-s3    |   4.2.2 | @uppy/unsplash  |   4.3.2 |
| @uppy/companion |   5.5.0 | uppy            |  4.13.0 |

- @uppy/aws-s3: always set S3 meta to UppyFile & include key (Merlijn Vos / #5602)
- @uppy/companion: fix forcePathStyle boolean conversion (Mikael Finstad / #5308)
- meta: Fix Webpack CI (Merlijn Vos / #5604)
- @uppy/aws-s3: allow uploads to fail/succeed independently (Merlijn Vos / #5603)
- meta: Add types for css files (Merlijn Vos / #5591)
- @uppy/unsplash: make utmSource optional (Merlijn Vos / #5601)
- meta: build(deps): bump docker/setup-qemu-action from 3.2.0 to 3.3.0 (dependabot[bot] / #5599)
- meta: build(deps): bump docker/build-push-action from 6.10.0 to 6.11.0 (dependabot[bot] / #5600)
- @uppy/companion: add COMPANION_TUS_DEFERRED_UPLOAD_LENGTH (Dominik Schmidt / #5561)
const key = fields?.key
if (!key) {
console.error(
'Expected `fields.key` to be returend but the backend/Companion',
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we remove this console.log? make it into a comment instead?

// Expected `fields.key` to be returned by Companion. This means the Companion version is too old

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe that makes more sense yes 👍

Copy link

Choose a reason for hiding this comment

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

Does anyone mind please explaining why I'm getting this error after updating?
Everything was working before (and still does) but now this comes up as an error in my react overlay.

Using AwsS3 with getUploadParameters.

I think it has something to do with this return:

      return await res.json().then((data: DataObject) => {
        return {
          method: data?.method,
          url: data?.url,
          fields: {}, // Add an empty fields property
          expires: undefined, // Add expires property with value undefined
          headers: {
            'Content-Type': file.type,
            'Content-Disposition': `${disposition}; filename="${sanitizedFilename}"`,
          },
        };
      });

Can I just add key: '' inside empty fields object?

mifi added a commit that referenced this pull request Jan 16, 2025
mifi added a commit that referenced this pull request Jan 17, 2025
Murderlon added a commit that referenced this pull request Jan 23, 2025
* main: (38 commits)
  Release: uppy@4.13.1 (#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: uppy@4.13.0 (#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: uppy@4.12.2 (#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