This repository has been archived by the owner. It is now read-only.

WIP: Direct uploads #19

Merged
merged 12 commits into from Jul 9, 2017

Conversation

Projects
None yet
5 participants
@dhh
Member

dhh commented Jul 8, 2017

We can support direct uploads by exposing an endpoint that'll create the destination URL for the service and return a signed blob gid that can be submitted with the form on completed upload.

This pull request currently just exposes the url_for_direct_upload for the S3 service, but we should provide it for all services, including disk, so we can fully test it.

I'm having some trouble testing the generated URL. I can make it work with JavaScript in a browser, but stuck on something when submitting the upload request via HTTParty. Getting a signing error.

Secondly, I'd like to include the JavaScript needed to make this fully work as well. I'm thinking that we do something like form.file_field :avatar, 'data-direct-upload': rails_direct_uploads_path (which again should be wrapped in form.direct_upload_file_field :avatar, which is then picked up by a Rails UJS-like helped that on file picking does something like:

window.fileUpload = function(file) {
  // FIXME: Block the form from submitting while we are doing the direct upload

  let data = new FormData()
  data.append("blob[filename]", file.name)
  data.append("blob[byte_size]", file.size)
  data.append("blob[content_type]", file.type)
  
  // FIXME: Compute proper md5 in base64
  data.append("blob[checksum]", "xxx23423")

  fetch("/rails/active_storage/direct_uploads", { method: "POST", body: data }).then(r => r.json()).then(function(directUploadDetails) {
    const reader = new FileReader()

    reader.onload = function(event) {
      fetch(directUploadDetails["url"], { method: "PUT", body: event.currentTarget.result}).then(function() {
        // FIXME: Set a hidden field on the form with the value of directUploadDetails["sgid"] 
        // to reference the uploaded file
      })
    }

    reader.readAsBinaryString(file)
  })
  
  // FIXME: Release the form for submission now that the files are done uploading
  return true
}

If anyone would like to assist bringing this PR to completion, please do join the fun.

dhh and others added some commits Jul 8, 2017

Copypasta comments
# Conflicts:
#	lib/active_storage/engine.rb
#	lib/active_storage/service.rb
#	lib/active_storage/service/disk_service.rb
#	lib/active_storage/service/s3_service.rb
#	test/service/s3_service_test.rb
#	test/test_helper.rb
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 9, 2017

Member

Going to merge this partial PR as-is. The direct url creation works for S3 when tested in the browser. Still haven't found out why the test doesn't pass, but will continue to work on that.

We can work on the JS element in a separate PR.

(I got tired of keeping this branch up to date with rapid changes to master).

Member

dhh commented Jul 9, 2017

Going to merge this partial PR as-is. The direct url creation works for S3 when tested in the browser. Still haven't found out why the test doesn't pass, but will continue to work on that.

We can work on the JS element in a separate PR.

(I got tired of keeping this branch up to date with rapid changes to master).

@dhh dhh merged commit a19d943 into master Jul 9, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@dhh dhh deleted the direct-uploads branch Jul 9, 2017

config.after_initialize do |app|
app.routes.prepend do
get "/rails/blobs/:encoded_key/*filename" => "active_storage/disk#show", as: :rails_disk_blob
eval(File.read(File.expand_path("../routes.rb", __FILE__)))

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 10, 2017

Member

This is an engine so if we create a file config/routes.rb inside the gem it will be loaded without us having to add new initializer. Same with controllers. If the controllers are inside app/controllers they will be automatically required.

@rafaelfranca

rafaelfranca Jul 10, 2017

Member

This is an engine so if we create a file config/routes.rb inside the gem it will be loaded without us having to add new initializer. Same with controllers. If the controllers are inside app/controllers they will be automatically required.

This comment has been minimized.

@dhh

dhh Jul 10, 2017

Member

Ah, yes, of course. Will switch over to that 👍

@dhh

dhh Jul 10, 2017

Member

Ah, yes, of course. Will switch over to that 👍

@claudiob

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob Jul 10, 2017

Member

@dhh How do you feel about implementing this feature without depending on the aws-sdk gem?

On one hand, it's true that Amazon built that gem, so it will probably always be on sync with the HTTP calls needed to store/retrieve/delete files on S3.

On the other hand, aws-sdk is a little bloated because it supports many features of AWS, while here we only need a few specific ones.

I asked this question myself when I created a gem called star (https://github.com/claudiob/star) that lets users store files on S3 and retrieve them with expiring URLs. I started with aws-sdk but then found out that I only needed to write 10 lines of Ruby to store a file (https://github.com/claudiob/star/blob/master/lib/star/file.rb#L64-L75), to delete a file (https://github.com/claudiob/star/blob/master/lib/star/file.rb#L82-L90) or to copy a file (https://github.com/claudiob/star/blob/master/lib/star/file.rb#L99-L109).

Let me know if this is something that you wanna look out, or else if you are happy with sticking with aws-sdk.

Member

claudiob commented Jul 10, 2017

@dhh How do you feel about implementing this feature without depending on the aws-sdk gem?

On one hand, it's true that Amazon built that gem, so it will probably always be on sync with the HTTP calls needed to store/retrieve/delete files on S3.

On the other hand, aws-sdk is a little bloated because it supports many features of AWS, while here we only need a few specific ones.

I asked this question myself when I created a gem called star (https://github.com/claudiob/star) that lets users store files on S3 and retrieve them with expiring URLs. I started with aws-sdk but then found out that I only needed to write 10 lines of Ruby to store a file (https://github.com/claudiob/star/blob/master/lib/star/file.rb#L64-L75), to delete a file (https://github.com/claudiob/star/blob/master/lib/star/file.rb#L82-L90) or to copy a file (https://github.com/claudiob/star/blob/master/lib/star/file.rb#L99-L109).

Let me know if this is something that you wanna look out, or else if you are happy with sticking with aws-sdk.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 10, 2017

Member

oh good point @claudiob. aws-sdk includes some monkey patches to Net::Http and I'd feel more comfortable not shipping that gem by default to all Rails applications.

Member

rafaelfranca commented Jul 10, 2017

oh good point @claudiob. aws-sdk includes some monkey patches to Net::Http and I'd feel more comfortable not shipping that gem by default to all Rails applications.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Jul 10, 2017

Member

aws-sdk isn't a dep in the gemspec. Think it makes sense for us to track the official clients for the Big Three cloud storage providers. Implementing a service that uses a different client is pretty straightforward, and could even be bundled with the client 😊

Member

jeremy commented Jul 10, 2017

aws-sdk isn't a dep in the gemspec. Think it makes sense for us to track the official clients for the Big Three cloud storage providers. Implementing a service that uses a different client is pretty straightforward, and could even be bundled with the client 😊

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.