Skip to content
This repository was archived by the owner on Nov 9, 2017. It is now read-only.

WIP: Direct uploads #19

Merged
merged 12 commits into from
Jul 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ gem 'rake'
gem 'byebug'

gem 'sqlite3'
gem 'httparty'

gem 'aws-sdk', require: false
gem 'google-cloud-storage', require: false
4 changes: 4 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ GEM
multi_json (~> 1.11)
os (~> 0.9)
signet (~> 0.7)
httparty (0.15.5)
multi_xml (>= 0.5.2)
httpclient (2.8.3)
i18n (0.8.4)
jmespath (1.3.1)
Expand All @@ -100,6 +102,7 @@ GEM
mini_portile2 (2.1.0)
minitest (5.10.2)
multi_json (1.12.1)
multi_xml (0.6.0)
multipart-post (2.0.0)
nokogiri (1.7.2)
mini_portile2 (~> 2.1.0)
Expand Down Expand Up @@ -139,6 +142,7 @@ DEPENDENCIES
bundler (~> 1.15)
byebug
google-cloud-storage
httparty
rake
sqlite3

Expand Down
8 changes: 8 additions & 0 deletions lib/active_storage/blob.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ def build_after_upload(io:, filename:, content_type: nil, metadata: nil)
def create_after_upload!(io:, filename:, content_type: nil, metadata: nil)
build_after_upload(io: io, filename: filename, content_type: content_type, metadata: metadata).tap(&:save!)
end

def create_before_direct_upload!(filename:, byte_size:, checksum:, content_type: nil, metadata: nil)
create! filename: filename, byte_size: byte_size, checksum: checksum, content_type: content_type, metadata: metadata
end
end

# We can't wait until the record is first saved to have a key for it
Expand All @@ -40,6 +44,10 @@ def url(expires_in: 5.minutes, disposition: :inline)
service.url key, expires_in: expires_in, disposition: disposition, filename: filename
end

def url_for_direct_upload(expires_in: 5.minutes)
service.url_for_direct_upload key, expires_in: expires_in, content_type: content_type, content_length: byte_size
end


def upload(io)
self.checksum = compute_checksum_in_chunks(io)
Expand Down
14 changes: 14 additions & 0 deletions lib/active_storage/direct_uploads_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
require "action_controller"
require "active_storage/blob"

class ActiveStorage::DirectUploadsController < ActionController::Base
def create
blob = ActiveStorage::Blob.create_before_direct_upload!(blob_args)
render json: { url: blob.url_for_direct_upload, sgid: blob.to_sgid.to_param }
end

private
def blob_args
params.require(:blob).permit(:filename, :byte_size, :checksum, :content_type, :metadata).to_h.symbolize_keys
end
end
3 changes: 2 additions & 1 deletion lib/active_storage/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ class Engine < Rails::Engine # :nodoc:

initializer "active_storage.routes" do
require "active_storage/disk_controller"
require "active_storage/direct_uploads_controller"

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__)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
end
end
Expand Down
3 changes: 0 additions & 3 deletions lib/active_storage/log_subscriber.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
require "active_support/log_subscriber"

# Implements the ActiveSupport::LogSubscriber for logging notifications when
# email is delivered or received.
class ActiveStorage::LogSubscriber < ActiveSupport::LogSubscriber
def service_upload(event)
message = color("Uploaded file to key: #{key_in(event)}", GREEN)
Expand All @@ -25,7 +23,6 @@ def service_url(event)
debug event, color("Generated URL for file at key: #{key_in(event)} (#{event.payload[:url]})", BLUE)
end

# Use the logger configured for ActiveStorage::Base.logger
def logger
ActiveStorage::Service.logger
end
Expand Down
2 changes: 2 additions & 0 deletions lib/active_storage/routes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
get "/rails/active_storage/disk/:encoded_key/*filename" => "active_storage/disk#show", as: :rails_disk_blob
post "/rails/active_storage/direct_uploads" => "active_storage/direct_uploads#create", as: :rails_direct_uploads
4 changes: 4 additions & 0 deletions lib/active_storage/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ def url(key, expires_in:, disposition:, filename:)
raise NotImplementedError
end

def url_for_direct_upload(key, expires_in:, content_type:, content_length:)
raise NotImplementedError
end

private
def instrument(operation, key, payload = {}, &block)
ActiveSupport::Notifications.instrument(
Expand Down
2 changes: 1 addition & 1 deletion lib/active_storage/service/disk_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def url(key, expires_in:, disposition:, filename:)
if defined?(Rails) && defined?(Rails.application)
Rails.application.routes.url_helpers.rails_disk_blob_path(verified_key_with_expiration, disposition: disposition, filename: filename)
else
"/rails/blobs/#{verified_key_with_expiration}/#{filename}?disposition=#{disposition}"
"/rails/active_storage/disk/#{verified_key_with_expiration}/#{filename}?disposition=#{disposition}"
end

payload[:url] = generated_url
Expand Down
11 changes: 11 additions & 0 deletions lib/active_storage/service/s3_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,17 @@ def url(key, expires_in:, disposition:, filename:)
end
end

def url_for_direct_upload(key, expires_in:, content_type:, content_length:)
instrument :url, key do |payload|
generated_url = object_for(key).presigned_url :put, expires_in: expires_in,
content_type: content_type, content_length: content_length

payload[:url] = generated_url

generated_url
end
end

private
def object_for(key)
bucket.object(key)
Expand Down
2 changes: 1 addition & 1 deletion test/blob_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase

private
def expected_url_for(blob, disposition: :inline)
"/rails/blobs/#{ActiveStorage::VerifiedKeyWithExpiration.encode(blob.key, expires_in: 5.minutes)}/#{blob.filename}?disposition=#{disposition}"
"/rails/active_storage/disk/#{ActiveStorage::VerifiedKeyWithExpiration.encode(blob.key, expires_in: 5.minutes)}/#{blob.filename}?disposition=#{disposition}"
end
end
36 changes: 36 additions & 0 deletions test/direct_uploads_controller_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
require "test_helper"
require "database/setup"

require "action_controller"
require "action_controller/test_case"

require "active_storage/direct_uploads_controller"

if SERVICE_CONFIGURATIONS[:s3]
class ActiveStorage::DirectUploadsControllerTest < ActionController::TestCase
setup do
@blob = create_blob
@routes = Routes
@controller = ActiveStorage::DirectUploadsController.new

@old_service = ActiveStorage::Blob.service
ActiveStorage::Blob.service = ActiveStorage::Service.configure(:s3, SERVICE_CONFIGURATIONS)
end

teardown do
ActiveStorage::Blob.service = @old_service
end

test "creating new direct upload" do
post :create, params: { blob: {
filename: "hello.txt", byte_size: 6, checksum: Digest::MD5.base64digest("Hello"), content_type: "text/plain" } }

details = JSON.parse(@response.body)

assert_match /rails-activestorage\.s3.amazonaws\.com/, details["url"]
assert_equal "hello.txt", GlobalID::Locator.locate_signed(details["sgid"]).filename.to_s
end
end
else
puts "Skipping Direct Upload tests because no S3 configuration was supplied"
end
9 changes: 0 additions & 9 deletions test/disk_controller_test.rb
Original file line number Diff line number Diff line change
@@ -1,19 +1,10 @@
require "test_helper"
require "database/setup"

require "action_controller"
require "action_controller/test_case"

require "active_storage/disk_controller"
require "active_storage/verified_key_with_expiration"

class ActiveStorage::DiskControllerTest < ActionController::TestCase
Routes = ActionDispatch::Routing::RouteSet.new.tap do |routes|
routes.draw do
get "/rails/blobs/:encoded_key/*filename" => "active_storage/disk#show", as: :rails_disk_blob
end
end

setup do
@blob = create_blob
@routes = Routes
Expand Down
2 changes: 1 addition & 1 deletion test/service/disk_service_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class ActiveStorage::Service::DiskServiceTest < ActiveSupport::TestCase
include ActiveStorage::Service::SharedServiceTests

test "url generation" do
assert_match /rails\/blobs\/.*\/avatar\.png\?disposition=inline/,
assert_match /rails\/active_storage\/disk\/.*\/avatar\.png\?disposition=inline/,
@service.url(FIXTURE_KEY, expires_in: 5.minutes, disposition: :inline, filename: "avatar.png")
end
end
31 changes: 31 additions & 0 deletions test/service/s3_service_test.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,42 @@
require "service/shared_service_tests"
require "httparty"
require "uri"

if SERVICE_CONFIGURATIONS[:s3]
class ActiveStorage::Service::S3ServiceTest < ActiveSupport::TestCase
SERVICE = ActiveStorage::Service.configure(:s3, SERVICE_CONFIGURATIONS)

include ActiveStorage::Service::SharedServiceTests

test "direct upload" do
# FIXME: This test is failing because of a mismatched request signature, but it works in the browser.
skip

begin
key = SecureRandom.base58(24)
data = "Something else entirely!"
direct_upload_url = @service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: "text/plain", content_length: data.size)

url = URI.parse(direct_upload_url).to_s.split("?").first
query = CGI::parse(URI.parse(direct_upload_url).query).collect { |(k, v)| [ k, v.first ] }.to_h

HTTParty.post(
url,
query: query,
body: data,
headers: {
"Content-Type": "text/plain",
"Origin": "http://localhost:3000"
},
debug_output: STDOUT
)

assert_equal data, @service.download(key)
ensure
@service.delete key
end
end

test "signed URL generation" do
assert_match /rails-activestorage\.s3\.amazonaws\.com.*response-content-disposition=inline.*avatar\.png/,
@service.url(FIXTURE_KEY, expires_in: 5.minutes, disposition: :inline, filename: "avatar.png")
Expand Down
8 changes: 0 additions & 8 deletions test/service/shared_service_tests.rb
Original file line number Diff line number Diff line change
@@ -1,13 +1,5 @@
require "test_helper"
require "active_support/core_ext/securerandom"
require "yaml"

SERVICE_CONFIGURATIONS = begin
YAML.load_file(File.expand_path("../configurations.yml", __FILE__)).deep_symbolize_keys
rescue Errno::ENOENT
puts "Missing service configuration file in test/service/configurations.yml"
{}
end

module ActiveStorage::Service::SharedServiceTests
extend ActiveSupport::Concern
Expand Down
22 changes: 22 additions & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@
require "byebug"

require "active_storage"

require "active_storage/service"
require "yaml"
SERVICE_CONFIGURATIONS = begin
YAML.load_file(File.expand_path("../service/configurations.yml", __FILE__)).deep_symbolize_keys
rescue Errno::ENOENT
puts "Missing service configuration file in test/service/configurations.yml"
{}
end


require "active_storage/service/disk_service"
ActiveStorage::Blob.service = ActiveStorage::Service::DiskService.new(root: File.join(Dir.tmpdir, "active_storage"))
ActiveStorage::Service.logger = ActiveSupport::Logger.new(STDOUT)
Expand All @@ -19,10 +30,21 @@ def create_blob(data: "Hello world!", filename: "hello.txt", content_type: "text
end
end

require "action_controller"
require "action_controller/test_case"

class ActionController::TestCase
Routes = ActionDispatch::Routing::RouteSet.new.tap do |routes|
routes.draw do
eval(File.read(File.expand_path("../../lib/active_storage/routes.rb", __FILE__)))
end
end
end

require "active_storage/attached"
ActiveRecord::Base.send :extend, ActiveStorage::Attached::Macros

require "global_id"
GlobalID.app = "ActiveStorageExampleApp"
ActiveRecord::Base.send :include, GlobalID::Identification
SignedGlobalID.verifier = ActiveStorage::VerifiedKeyWithExpiration.verifier