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

Protocol is always s3-multipart for remote file uploads with the AwsS3 uploader even with shouldUseMultipart set to false #5515

Open
2 tasks done
StrixOSG opened this issue Nov 15, 2024 · 12 comments · May be fixed by #5558
Open
2 tasks done
Labels

Comments

@StrixOSG
Copy link

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

No response

Steps to reproduce

Choose a remote provider such as Google Drive with AwsS3 as the uploader and uploading files will always be 's3-multipart' even with shouldUseMultipart set to false.

Expected behavior

In previous versions as we recently upgraded from v3 to v4 of Uppy it used 'multipart' and not 's3-multipart' for uploading remote files. I would expect this to still be the case or that you can choose with either passing in the protocol, or with shouldUseMultipart being set to false. This is a problem as we generate the pre-signed urls beforehand to upload to and does not fit nicely in our flow to have to switch to 's3-multipart'.

Actual behavior

When AwsS3 is the uploader, uploading files from remote sources like Google Drive will always be 's3-multipart' instead of 'multipart' even with shouldUseMultipart set to false.

@StrixOSG StrixOSG added the Bug label Nov 15, 2024
@shabanbx
Copy link

shabanbx commented Nov 15, 2024

@StrixOSG did you find any solution?

@StrixOSG
Copy link
Author

@StrixOSG did you find any solution?

Nothing yet. I tried intercepting the request as a workaround and changing the protocol to 'multipart' but it appears to be more involved then that as it now says "no destination specified"

@StrixOSG
Copy link
Author

@mifi any thoughts?

@mifi
Copy link
Contributor

mifi commented Nov 16, 2024

In previous versions as we recently upgraded from v3 to v4 of Uppy it used 'multipart' and not 's3-multipart' for uploading remote files.

do you mean it used s3 instead of s3-multipart in v3? the multipart protocol would not make any sense for S3 because it's basically a normal HTTP PUT/POST multipart:

return this.#uploadMultipart(this.readStream)

AFAIK Companion always uses (and has always used) s3-multipart when the S3 plugin (previously s3 or s3-multipart plugin) is used, regardless of whether or not Uppy is configured for multipart, as can be seen here:

protocol: 's3-multipart',

Companion has never supported s3 non multipart for remote files (e.g. google drive). So I'm not sure exactly what's the problem here.

@shabanbx

This comment was marked as off-topic.

@mifi

This comment was marked as off-topic.

@Murderlon
Copy link
Member

I wonder if this is actually an issue. With the rights bucket settings, you can easily accept both?

@mifi
Copy link
Contributor

mifi commented Nov 18, 2024

I vaguely remember that we never implemented a non-multipart s3 in companion because the backend is not so sensitive to bandwidth, like the frontend (multipart has a lot more HTTP calls and orchestration in order to upload a file compared to s3 non-multipart, but in a backend with fast connection this shouldn't make much of a difference). So unless there are any good reasons to implement s3 non-multipart in the backend, then we probably shouldn't prioritize it.

This is a problem as we generate the pre-signed urls beforehand to upload to and does not fit nicely in our flow to have to switch to 's3-multipart'.

Are you saying that in v3 you're able to use pre-signed URLs in companion with s3 non-multipart? if so, can you show a code example of how you do that?

@StrixOSG
Copy link
Author

StrixOSG commented Nov 18, 2024

So you're right it still used "multipart" in the backend but it was multipart and not s3-multipart for v3 for remote files with the AwsS3 plugin. I had modified the source files and added some console logs to see exactly what it was using in v3 compared to v4.

Here's an example of the front-end code we have going:

const s3Options = {
      getUploadParameters: (file: UppyFile): any => {
        //Prep observable data and make call to our backend to create the file in the DB and get a pre-signed url
        return createFileAndGetPresignedUrlObservable.toPromise().then((fileResponse: FileResponse) => {
          return {
            method: 'PUT',
            url: fileResponse.preSignedUploadUrl,
            fields: {},
            headers: {
              'Content-Type': file.type
            }
          };
        });
      },
      timeout: 0
};

createFileAndGetPresignedUrlObservable this function here just represents that we use an rxjs observable to hit our backend and create the file in the DB and return a pre-signed url so it can be uploaded. This is the critical part here because if the user didn't have permission at this point we just wouldn't upload/return a pre-signed url.

In the backend we don't have any S3 configuration at all, and of course that caused errors in the new version for v4 of uppy as it is now telling companion to use s3-multipart which needs the bucket config and all that instead of just uploading to the pre-signed url with multipart. Of course like you say multipart may not have been true multipart and may have just been a single http request due to it being complicated.

@mifi
Copy link
Contributor

mifi commented Jan 15, 2025

In v3 aws-s3 plugin, the option protocol defaulted to multipart: https://github.com/transloadit/uppy/blob/1ddae8008b1bc0bdd1f3ad1b9b7304a68e3150f6/packages/%40uppy/aws-s3/src/index.js#L258C1-L259C1

From v3 to v4, the aws-s3 plugin was refactored to include support multipart in the commit 04839fd - however the default value protocol: 'multipart' 04839fd#diff-6a6cd2d22eef9f764e7652b9e470dea3d82fd3b549c7a0e9ee2e166a99c0e005L258 was changed to protocol: 's3-multipart': 04839fd#diff-f50056401470bc654a94132a87b1babf2b8399ad7dab983fa15cf10fa65bcc26L922

And you want to instead change it to:

- protocol: 's3-multipart',
+ protocol: this.opts.shouldUseMultipart ? 's3-multipart' : 'multipart',

However I'm a bit confused by this in the first place why the @aws-s3 plugin should use multipart (which has nothing to do with S3). Ideally I think we should have a s3 protocol implementation in Companion too, but as mention earlier, for now we don't so s3-multipart is used instead. If I make the above code change and test a Google Drive upload, then it works when shouldUseMultipart: true, however if I set shouldUseMultipart: false, I see that protocol: multipart gets sent to Companion, and Companion then errors out with controller.googlePicker.error ValidationError: no destination specified. This leads me to believe that protocol should still remain s3-multipart when the S3 plugin is used.

@Murderlon
Copy link
Member

Note that when #5558 is merged you can do this: uppy.setState({ remoteUploader: 'multipart' }).

@nishitaku
Copy link

This issue is also affecting my project quite a bit, and I’d really appreciate it if it could be addressed soon. Let me know if there’s any way I can help!

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants