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 · 9 comments
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.

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

No branches or pull requests

4 participants