janko / uppy-s3_multipart

Provides Ruby endpoints for AWS S3 multipart uploads in Uppy
https://uppy.io/docs/aws-s3/
MIT License
65 stars 21 forks source link

Add support for per-request prefix generation #10

Closed kdonovan closed 5 years ago

kdonovan commented 5 years ago

This allows Shrine users to specify a lambda to dynamically determine the bucket's prefix on a per-request basis.

Motivation: I'm setting up a multi-tenant app where each tenant's users should be able to upload files directly to S3. Our S3 architecture separates tenant files by prefix. Before this PR I couldn't figure out any other way of setting a non-static prefix, but now I'm able to do the following:

Shrine.plugin :uppy_s3_multipart, options: {
  dynamic_prefix: -> (env) {
    raise "Unauthorized upload attempt" unless current_user = env['warden'].user
    # ... determine appropriate prefix for this user's org
  }
}
hmistry commented 5 years ago

Thank you for creating the PR and explaining the use case. It looks good except it lacks a test. Would you mind adding a test?

janko commented 5 years ago

Looking at your PR, I think this is already possible to achieve by overriding the :key option in :create_multipart_upload operation:

Shrine.plugin :uppy_s3_multipart, options: {
  create_multipart_upload: -> (request) {
     raise "Unauthorized upload attempt" unless current_user = request.env['warden'].user

     prefix = determine_prefix(current_user)
     extension = File.extname(request.params["filename"].to_s)

     { key: "#{prefix}/#{SecureRandom.hex + extension}"}
  }
}

Now I'm thinking it would be nice if the block also yielded default options, so that this could be a bit simpler:

Shrine.plugin :uppy_s3_multipart, options: {
  create_multipart_upload: -> (request, options) {
     raise "Unauthorized upload attempt" unless current_user = request.env['warden'].user

     { key: "#{determine_prefix(current_user)}/#{options[:key]}"}
  }
}

Would that work for you?

kdonovan commented 5 years ago

@janko Yep, that'll do the trick! Thanks for pointing out the existing solution. 👍