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

dynamic S3 bucket doesnt work with Google Drive provider #4763

Closed
2 tasks done
ivadenis opened this issue Oct 25, 2023 · 7 comments · Fixed by #4770
Closed
2 tasks done

dynamic S3 bucket doesnt work with Google Drive provider #4763

ivadenis opened this issue Oct 25, 2023 · 7 comments · Fixed by #4770
Assignees
Labels

Comments

@ivadenis
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

Im trying to set a bucket for S3 clien in companion dynamically based on the metadata set from the client. However, when upload is done through Google Drive (or any other provider), bucket function property on the companion does not receive any arguments.

Screenshot 2023-10-24 at 8 05 32 PM

this is my rought setup

UI:

uppy.setMeta(s3Object);

uppy.use(GoogleDrive, {
  companionUrl: COMPANION_URL
})

uppy.use(Dashboard, {
  inline: true,
  target: 'body',
  plugins: ['GoogleDrive'],
})

uppy.use(AwsS3, {
  companionUrl: COMPANION_URL,
  shouldUseMultipart: true,
})

Companion:

const options = {
  providerOptions: {
    drive: {
      key: process.env.COMPANION_GOOGLE_KEY,
      secret: process.env.COMPANION_GOOGLE_SECRET,
    },
  },
  s3: {
    getKey: (req, filename, metadata) => {
      return `${crypto.randomUUID()}-${filename}`;
    },
    bucket: (req) => {
      // req is undefined
      return process.env.COMPANION_AWS_BUCKET;
    },
    secret: process.env.COMPANION_AWS_SECRET,
    region: process.env.COMPANION_AWS_REGION,
  },
  server: { host: process.env.COMPANION_DOMAIN },
  filePath: DATA_DIR,
  secret: SECRET,
  debug: true,
}

const { app: companionApp } = companion.app(options)

I actually traced down the issue to this line:

https://github.com/transloadit/uppy/blob/main/packages/%40uppy/companion/src/server/Uploader.js#L658

Uploader.js
const params = {
      Bucket: getBucket(options.bucket), // <------
      Key: options.getKey(null, filename, this.options.metadata),
      ContentType: this.options.metadata.type,
      Metadata: rfc2047EncodeMetadata(this.options.metadata),
      Body: stream,
    }

it doesn't seem that a request object is passed down to getBucket util.

In fact, it would be very helpful if getBucket passed this.options.metadata as a second argument (similar to getKey).

Expected behavior

Bucket property as a function should receive a request object when used with server-to-server providers.
Additionally, it would be nice if it could receive options.metadata just like getKey.

 bucket: (req) => {
      ....
},

Actual behavior

argument req of function property bucket on companion is undefined.

@mifi
Copy link
Contributor

mifi commented Oct 25, 2023

Actually getKey is also not passed any req:

Key: options.getKey(null, filename, this.options.metadata),

I think this was intentional, because the req object is not stored inside the Uploader object because the Uploader lives outside the lifetime of a http request. And passing the whole req object feels like a leaky abstraction because IMO req contains internal data structures that might change at any time. But I agree that we could pass metadata to getBuckry, as well as other useful data, but not the whole req object I think

@roshchyn
Copy link

@mifi how it is possible to pass bucket name from Uppy (Frontend) -> Companion (BackEnd) ? One way was to pass as Uppy's Metadata param but it is only passed to companion when aws-s3 is using the legacy uploader, which does not support bucket as a function. Or am I missing the flow, cause with aws-s3-multipart it first make request to backend s3/ endpoint to get creds and bucket name without passing anything custom from Uppy. Even param companionHeaders are ignored (although they are specified in the uppy docs). So there is not way today it looks like to pass bucket name or any data at that matter from Uppy to Companion backend out of the box ? Thank you for your answer.

@mifi
Copy link
Contributor

mifi commented Oct 26, 2023

Hey. I just tested s3 multipart upload from google drive as well as from a local file, and it does correctly set metadata on the uploaded s3 object. correct me if I'm wrong @Murderlon but I think this is how users are supposed to pass data from Uppy to Companion. As for directly passing the bucket name to which to upload to from the client, I think that is not very safe because someone could abuse it to make your companion server upload to other arbitrary buckets

@ivadenis
Copy link
Author

@mifi you are describing slightly different issue. Im not trying to set metadata on the s3 object itself, but pass some meta info down to bucket function prop as an argument. Similar way how getKey prop works.

We dont have to pass a bucket name explicitly, but I could chose a bucket based on the metadata that is passed from the UI.

@mifi
Copy link
Contributor

mifi commented Oct 27, 2023

Ok, so if I understand you correctly, you want to pass some side-metadata that should not be stored on the uploaded file, only to be used by companion user-code, passed into getBucket/getKey? @arturi @Murderlon wdyt, does it make sense to invent such a new concept of side-metadata? And what should we name it?

also, how/when do you want to be able to attach this metadata in Uppy? On a per file-basis, right?

@Murderlon
Copy link
Member

Wouldn't aligning the bucket signature with the one from getKey be enough, without inventing something new? We document the request is passed but that's not the case so that should be documented and probably changed in a major.

@mifi
Copy link
Contributor

mifi commented Oct 30, 2023

ok let's do that first. created a PR #4770

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.

4 participants