Extract content types from blob data

Gemfile

@@ -19,6 +19,7 @@ gem "rack-cache", "~> 1.2"
 gem "coffee-rails"
 gem "sass-rails"
 gem "turbolinks", "~> 5"
+gem "webmock"
 
 # require: false so bcrypt is loaded only when has_secure_password is used.
 # This is to avoid Active Model (and by extension the entire framework)

Gemfile.lock

@@ -69,6 +69,7 @@ PATH
     activestorage (5.2.0.beta2)
       actionpack (= 5.2.0.beta2)
       activerecord (= 5.2.0.beta2)
+      marcel (~> 0.3.1)
     activesupport (5.2.0.beta2)
       concurrent-ruby (~> 1.0, >= 1.0.2)
       i18n (~> 0.7)
@@ -194,6 +195,8 @@ GEM
     concurrent-ruby (1.0.5-java)
     connection_pool (2.2.1)
     cookiejar (0.3.3)
+    crack (0.4.3)
+      safe_yaml (~> 1.0.0)
     crass (1.0.3)
     curses (1.0.2)
     daemons (1.2.4)
@@ -266,6 +269,7 @@ GEM
       multi_json (~> 1.11)
       os (~> 0.9)
       signet (~> 0.7)
+    hashdiff (0.3.7)
     hiredis (0.6.1)
     hiredis (0.6.1-java)
     http_parser.rb (0.6.0)
@@ -297,12 +301,15 @@ GEM
       nokogiri (>= 1.5.9)
     mail (2.7.0)
       mini_mime (>= 0.1.1)
+    marcel (0.3.1)
+      mimemagic (~> 0.3.2)
     memoist (0.16.0)
     metaclass (0.0.4)
     method_source (0.9.0)
     mime-types (3.1)
       mime-types-data (~> 3.2015)
     mime-types-data (3.2016.0521)
+    mimemagic (0.3.2)
     mini_magick (4.8.0)
     mini_mime (0.1.4)
     mini_portile2 (2.3.0)
@@ -402,6 +409,7 @@ GEM
     rubyzip (1.2.1)
     rufus-scheduler (3.4.2)
       et-orbi (~> 1.0)
+    safe_yaml (1.0.4)
     sass (3.5.3)
       sass-listen (~> 4.0.0)
     sass-listen (4.0.0)
@@ -481,6 +489,10 @@ GEM
       json (>= 1.8)
       nokogiri (~> 1.6)
     wdm (0.1.1)
+    webmock (3.2.1)
+      addressable (>= 2.3.6)
+      crack (>= 0.3.2)
+      hashdiff
     websocket (1.2.4)
     websocket-driver (0.6.5)
       websocket-extensions (>= 0.1.0)
@@ -557,6 +569,7 @@ DEPENDENCIES
   uglifier (>= 1.3.0)
   w3c_validators
   wdm (>= 0.1.0)
+  webmock
   websocket-client-simple!
 
 BUNDLED WITH

activestorage/activestorage.gemspec

@@ -27,4 +27,8 @@ Gem::Specification.new do |s|
 
   s.add_dependency "actionpack", version
   s.add_dependency "activerecord", version
+
+  s.add_dependency "marcel", "~> 0.3.1"
+
+  s.add_development_dependency "webmock", "~> 3.2.1"
 end

activestorage/app/models/active_storage/attachment.rb

@@ -14,7 +14,7 @@ class ActiveStorage::Attachment < ActiveRecord::Base
 
   delegate_missing_to :blob
 
-  after_create_commit :analyze_blob_later
+  after_create_commit :identify_blob, :analyze_blob_later
 
   # Synchronously purges the blob (deletes it from the configured service) and destroys the attachment.
   def purge
@@ -29,6 +29,10 @@ def purge_later
   end
 
   private
+    def identify_blob
+      blob.identify
+    end
+
     def analyze_blob_later
       blob.analyze_later unless blob.analyzed?
     end

activestorage/app/models/active_storage/blob.rb

@@ -14,12 +14,12 @@
 # update a blob's metadata on a subsequent pass, but you should not update the key or change the uploaded file.
 # If you need to create a derivative or otherwise change the blob, simply create a new blob and purge the old one.
 class ActiveStorage::Blob < ActiveRecord::Base
-  include Analyzable, Representable
+  include Analyzable, Identifiable, Representable
 
   self.table_name = "active_storage_blobs"
 
   has_secure_token :key
-  store :metadata, accessors: [ :analyzed ], coder: JSON
+  store :metadata, accessors: [ :analyzed, :identified ], coder: JSON
 
   class_attribute :service
 
@@ -136,8 +136,10 @@ def service_headers_for_direct_upload
   # Normally, you do not have to call this method directly at all. Use the factory class methods of +build_after_upload+
   # and +create_after_upload!+.
   def upload(io)
-    self.checksum  = compute_checksum_in_chunks(io)
-    self.byte_size = io.size
+    self.checksum     = compute_checksum_in_chunks(io)
+    self.content_type = extract_content_type(io)
+    self.byte_size    = io.size
+    self.identified   = true
 
     service.upload(key, io, checksum: checksum)
   end
@@ -182,6 +184,10 @@ def compute_checksum_in_chunks(io)
       end.base64digest
     end
 
+    def extract_content_type(io)
+      Marcel::MimeType.for io, name: filename.to_s, declared_type: content_type
+    end
+
     def forcibly_serve_as_binary?
       ActiveStorage.content_types_to_serve_as_binary.include?(content_type)
     end

activestorage/app/models/active_storage/blob/identifiable.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module ActiveStorage::Blob::Identifiable
+  def identify
+    ActiveStorage::Identification.new(self).apply
+  end
+
+  def identified?
+    identified
+  end
+end

activestorage/app/models/active_storage/identification.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+class ActiveStorage::Identification
+  attr_reader :blob
+
+  def initialize(blob)
+    @blob = blob
+  end
+
+  def apply
+    blob.update!(content_type: content_type, identified: true) unless blob.identified?
+  end
+
+  private
+    def content_type
+      Marcel::MimeType.for(identifiable_chunk, name: filename, declared_type: declared_content_type)
+    end
+
+
+    def identifiable_chunk
+      Net::HTTP.start(uri.host, uri.port, use_ssl: uri.scheme == "https") do |client|
+        client.get(uri, "Range" => "0-4096").body
+      end
+    end
+
+    def uri
+      @uri ||= URI.parse(blob.service_url)
+    end
+
+
+    def filename
+      blob.filename.to_s
+    end
+
+    def declared_content_type
+      blob.content_type
+    end
+end

activestorage/lib/active_storage.rb

@@ -30,6 +30,8 @@
 require "active_storage/version"
 require "active_storage/errors"
 
+require "marcel"
+
 module ActiveStorage
   extend ActiveSupport::Autoload
 

activestorage/lib/active_storage/service/disk_service.rb

@@ -9,10 +9,10 @@ module ActiveStorage
   # Wraps a local disk path as an Active Storage service. See ActiveStorage::Service for the generic API
   # documentation that applies to all services.
   class Service::DiskService < Service
-    attr_reader :root
+    attr_reader :root, :host
 
-    def initialize(root:)
-      @root = root
+    def initialize(root:, host:)
+      @root, @host = root, host
     end
 
     def upload(key, io, checksum: nil)
@@ -69,14 +69,13 @@ def url(key, expires_in:, filename:, disposition:, content_type:)
         verified_key_with_expiration = ActiveStorage.verifier.generate(key, expires_in: expires_in, purpose: :blob_key)
 
         generated_url =
-          if defined?(Rails.application)
-            Rails.application.routes.url_helpers.rails_disk_service_path \
-              verified_key_with_expiration,
-              filename: filename, disposition: content_disposition_with(type: disposition, filename: filename), content_type: content_type
-          else
-            "/rails/active_storage/disk/#{verified_key_with_expiration}/#{filename}?content_type=#{content_type}" \
-              "&disposition=#{content_disposition_with(type: disposition, filename: filename)}"
-          end
+          Rails.application.routes.url_helpers.rails_disk_service_url(
+            verified_key_with_expiration,
+            filename: filename,
+            disposition: content_disposition_with(type: disposition, filename: filename),
+            content_type: content_type,
+            host: host
+          )
 
         payload[:url] = generated_url
 
@@ -97,12 +96,7 @@ def url_for_direct_upload(key, expires_in:, content_type:, content_length:, chec
           purpose: :blob_token }
         )
 
-        generated_url =
-          if defined?(Rails.application)
-            Rails.application.routes.url_helpers.update_rails_disk_service_path verified_token_with_expiration
-          else
-            "/rails/active_storage/disk/#{verified_token_with_expiration}"
-          end
+        generated_url = Rails.application.routes.url_helpers.update_rails_disk_service_url(verified_token_with_expiration, host: host)
 
         payload[:url] = generated_url
 

activestorage/test/analyzer/video_analyzer_test.rb

@@ -30,6 +30,6 @@ class ActiveStorage::Analyzer::VideoAnalyzerTest < ActiveSupport::TestCase
 
   test "analyzing a video without a video stream" do
     blob = create_file_blob(filename: "video_without_video_stream.mp4", content_type: "video/mp4")
-    assert_equal({ "analyzed" => true }, blob.tap(&:analyze).metadata)
+    assert_equal({ "analyzed" => true, "identified" => true }, blob.tap(&:analyze).metadata)
   end
 end

activestorage/test/dummy/config/storage.yml

@@ -1,3 +1,4 @@
 local:
   service: Disk
   root: <%= Rails.root.join("storage") %>
+  host: http://localhost:3000

activestorage/test/models/attachments_test.rb

@@ -97,6 +97,29 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
     assert_equal "funky.jpg", @user.avatar_attachment.blob.filename.to_s
   end
 
+  test "identify newly-attached, directly-uploaded blob" do
+    # Simulate a direct upload.
+    blob = create_blob_before_direct_upload(filename: "racecar.jpg", content_type: "application/octet-stream", byte_size: 1124062, checksum: "7GjDDNEQb4mzMzsW+MS0JQ==")
+    ActiveStorage::Blob.service.upload(blob.key, file_fixture("racecar.jpg").open)
+
+    stub_request(:get, %r{localhost:3000/rails/active_storage/disk/.*}).to_return(body: file_fixture("racecar.jpg"))
+    @user.avatar.attach(blob)
+
+    assert_equal "image/jpeg", @user.avatar.reload.content_type
+    assert @user.avatar.identified?
+  end
+
+  test "identify newly-attached blob only once" do
+    blob = create_file_blob
+    assert blob.identified?
+
+    # The blob's backing file is a PNG image. Fudge its content type so we can tell if it's identified when we attach it.
+    blob.update! content_type: "application/octet-stream"
+
+    @user.avatar.attach blob
+    assert_equal "application/octet-stream", blob.content_type
+  end
+
   test "analyze newly-attached blob" do
     perform_enqueued_jobs do
       @user.avatar.attach create_file_blob
@@ -115,7 +138,7 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
 
     assert blob.reload.analyzed?
 
-    @user.avatar.attachment.destroy
+    @user.avatar.detach
 
     assert_no_enqueued_jobs do
       @user.reload.avatar.attach blob

activestorage/test/models/blob_test.rb

@@ -13,6 +13,16 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     assert_equal Digest::MD5.base64digest(data), blob.checksum
   end
 
+  test "create after upload extracts content type from data" do
+    blob = create_file_blob content_type: "application/octet-stream"
+    assert_equal "image/jpeg", blob.content_type
+  end
+
+  test "create after upload extracts content type from filename" do
+    blob = create_blob content_type: "application/octet-stream"
+    assert_equal "text/plain", blob.content_type
+  end
+
   test "text?" do
     blob = create_blob data: "Hello world!"
     assert blob.text?
@@ -79,6 +89,6 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     def expected_url_for(blob, disposition: :inline, filename: nil)
       filename ||= blob.filename
       query_string = { content_type: blob.content_type, disposition: "#{disposition}; #{filename.parameters}" }.to_param
-      "/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query_string}"
+      "http://localhost:3000/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query_string}"
     end
 end

activestorage/test/models/variant_test.rb

@@ -67,6 +67,6 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   test "service_url doesn't grow in length despite long variant options" do
     blob = create_file_blob(filename: "racecar.jpg")
     variant = blob.variant(font: "a" * 10_000).processed
-    assert_operator variant.service_url.length, :<, 500
+    assert_operator variant.service_url.length, :<, 525
   end
 end

activestorage/test/service/configurator_test.rb

@@ -4,8 +4,11 @@
 
 class ActiveStorage::Service::ConfiguratorTest < ActiveSupport::TestCase
   test "builds correct service instance based on service name" do
-    service = ActiveStorage::Service::Configurator.build(:foo, foo: { service: "Disk", root: "path" })
+    service = ActiveStorage::Service::Configurator.build(:foo, foo: { service: "Disk", root: "path", host: "http://localhost:3000" })
+
     assert_instance_of ActiveStorage::Service::DiskService, service
+    assert_equal "path", service.root
+    assert_equal "http://localhost:3000", service.host
   end
 
   test "raises error when passing non-existent service name" do

activestorage/test/service/disk_service_test.rb

@@ -3,7 +3,7 @@
 require "service/shared_service_tests"
 
 class ActiveStorage::Service::DiskServiceTest < ActiveSupport::TestCase
-  SERVICE = ActiveStorage::Service::DiskService.new(root: File.join(Dir.tmpdir, "active_storage"))
+  SERVICE = ActiveStorage::Service::DiskService.new(root: File.join(Dir.tmpdir, "active_storage"), host: "http://localhost:3000")
 
   include ActiveStorage::Service::SharedServiceTests
 

activestorage/test/service/mirror_service_test.rb

@@ -6,12 +6,13 @@ class ActiveStorage::Service::MirrorServiceTest < ActiveSupport::TestCase
   mirror_config = (1..3).map do |i|
     [ "mirror_#{i}",
       service: "Disk",
-      root: Dir.mktmpdir("active_storage_tests_mirror_#{i}") ]
+      root: Dir.mktmpdir("active_storage_tests_mirror_#{i}"),
+      host: "http://localhost:3000" ]
   end.to_h
 
   config = mirror_config.merge \
-    mirror:   { service: "Mirror", primary: "primary", mirrors: mirror_config.keys },
-    primary:  { service: "Disk", root: Dir.mktmpdir("active_storage_tests_primary") }
+    mirror:  { service: "Mirror", primary: "primary", mirrors: mirror_config.keys },
+    primary: { service: "Disk", root: Dir.mktmpdir("active_storage_tests_primary"), host: "http://localhost:3000" }
 
   SERVICE = ActiveStorage::Service.configure :mirror, config