Merge branch 'master' into activestorage-guide

17 files changed, 179 insertions, 39 deletions

diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index 2aa05d665e..acaf22fac1 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -270,7 +270,8 @@ class ActiveStorage::Blob < ActiveRecord::Base
   # deleted as well or you will essentially have a dead reference. It's recommended to use the +#purge+ and +#purge_later+
   # methods in most circumstances.
   def delete
-    service.delete key
+    service.delete(key)
+    service.delete_prefixed("variants/#{key}/") if image?
   end
 
   # Deletes the file on the service and then destroys the blob record. This is the recommended way to dispose of unwanted
diff --git a/activestorage/lib/active_storage/analyzer/video_analyzer.rb b/activestorage/lib/active_storage/analyzer/video_analyzer.rb
index 408b5e58e9..1c144baa37 100644
--- a/activestorage/lib/active_storage/analyzer/video_analyzer.rb
+++ b/activestorage/lib/active_storage/analyzer/video_analyzer.rb
@@ -19,6 +19,8 @@ module ActiveStorage
   # This analyzer requires the {ffmpeg}[https://www.ffmpeg.org] system library, which is not provided by Rails. You must
   # install ffmpeg yourself to use this analyzer.
   class Analyzer::VideoAnalyzer < Analyzer
+    class_attribute :ffprobe_path, default: "ffprobe"
+
     def self.accept?(blob)
       blob.video?
     end
@@ -29,10 +31,18 @@ module ActiveStorage
 
     private
       def width
-        Integer(video_stream["width"]) if video_stream["width"]
+        rotated? ? raw_height : raw_width
       end
 
       def height
+        rotated? ? raw_width : raw_height
+      end
+
+      def raw_width
+        Integer(video_stream["width"]) if video_stream["width"]
+      end
+
+      def raw_height
         Integer(video_stream["height"]) if video_stream["height"]
       end
 
@@ -50,6 +60,10 @@ module ActiveStorage
         end
       end
 
+      def rotated?
+        angle == 90 || angle == 270
+      end
+
 
       def tags
         @tags ||= video_stream["tags"] || {}
@@ -68,7 +82,7 @@ module ActiveStorage
       end
 
       def probe_from(file)
-        IO.popen([ "ffprobe", "-print_format", "json", "-show_streams", "-v", "error", file.path ]) do |output|
+        IO.popen([ ffprobe_path, "-print_format", "json", "-show_streams", "-v", "error", file.path ]) do |output|
           JSON.parse(output.read)
         end
       rescue Errno::ENOENT
diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb
index 6cf6635c4f..b870e6d4d6 100644
--- a/activestorage/lib/active_storage/engine.rb
+++ b/activestorage/lib/active_storage/engine.rb
@@ -15,7 +15,8 @@ module ActiveStorage
 
     config.active_storage = ActiveSupport::OrderedOptions.new
     config.active_storage.previewers = [ ActiveStorage::Previewer::PDFPreviewer, ActiveStorage::Previewer::VideoPreviewer ]
-    config.active_storage.analyzers  = [ ActiveStorage::Analyzer::ImageAnalyzer, ActiveStorage::Analyzer::VideoAnalyzer ]
+    config.active_storage.analyzers = [ ActiveStorage::Analyzer::ImageAnalyzer, ActiveStorage::Analyzer::VideoAnalyzer ]
+    config.active_storage.paths = ActiveSupport::OrderedOptions.new
 
     config.eager_load_namespaces << ActiveStorage
 
@@ -68,5 +69,21 @@ module ActiveStorage
         end
       end
     end
+
+    initializer "active_storage.paths" do
+      config.after_initialize do |app|
+        if ffprobe_path = app.config.active_storage.paths.ffprobe
+          ActiveStorage::Analyzer::VideoAnalyzer.ffprobe_path = ffprobe_path
+        end
+
+        if ffmpeg_path = app.config.active_storage.paths.ffmpeg
+          ActiveStorage::Previewer::VideoPreviewer.ffmpeg_path = ffmpeg_path
+        end
+
+        if mutool_path = app.config.active_storage.paths.mutool
+          ActiveStorage::Previewer::PDFPreviewer.mutool_path = mutool_path
+        end
+      end
+    end
   end
 end
diff --git a/activestorage/lib/active_storage/log_subscriber.rb b/activestorage/lib/active_storage/log_subscriber.rb
index 5cbf4bd1a5..a4e148c1a5 100644
--- a/activestorage/lib/active_storage/log_subscriber.rb
+++ b/activestorage/lib/active_storage/log_subscriber.rb
@@ -18,6 +18,10 @@ module ActiveStorage
       info event, color("Deleted file from key: #{key_in(event)}", RED)
     end
 
+    def service_delete_prefixed(event)
+      info event, color("Deleted files by key prefix: #{event.payload[:prefix]}", RED)
+    end
+
     def service_exist(event)
       debug event, color("Checked if file exists at key: #{key_in(event)} (#{event.payload[:exist] ? "yes" : "no"})", BLUE)
     end
diff --git a/activestorage/lib/active_storage/previewer.rb b/activestorage/lib/active_storage/previewer.rb
index ed75bae3b5..3d485988e9 100644
--- a/activestorage/lib/active_storage/previewer.rb
+++ b/activestorage/lib/active_storage/previewer.rb
@@ -54,5 +54,9 @@ module ActiveStorage
         IO.popen(argv) { |out| IO.copy_stream(out, to) }
         to.rewind
       end
+
+      def logger
+        ActiveStorage.logger
+      end
   end
 end
diff --git a/activestorage/lib/active_storage/previewer/pdf_previewer.rb b/activestorage/lib/active_storage/previewer/pdf_previewer.rb
index a2f05c74a6..b84aefcc9c 100644
--- a/activestorage/lib/active_storage/previewer/pdf_previewer.rb
+++ b/activestorage/lib/active_storage/previewer/pdf_previewer.rb
@@ -2,16 +2,23 @@
 
 module ActiveStorage
   class Previewer::PDFPreviewer < Previewer
+    class_attribute :mutool_path, default: "mutool"
+
     def self.accept?(blob)
       blob.content_type == "application/pdf"
     end
 
     def preview
       download_blob_to_tempfile do |input|
-        draw "mutool", "draw", "-F", "png", "-o", "-", input.path, "1" do |output|
+        draw_first_page_from input do |output|
           yield io: output, filename: "#{blob.filename.base}.png", content_type: "image/png"
         end
       end
     end
+
+    private
+      def draw_first_page_from(file, &block)
+        draw mutool_path, "draw", "-F", "png", "-o", "-", file.path, "1", &block
+      end
   end
 end
diff --git a/activestorage/lib/active_storage/previewer/video_previewer.rb b/activestorage/lib/active_storage/previewer/video_previewer.rb
index 49f128d142..5d06e33f44 100644
--- a/activestorage/lib/active_storage/previewer/video_previewer.rb
+++ b/activestorage/lib/active_storage/previewer/video_previewer.rb
@@ -2,6 +2,8 @@
 
 module ActiveStorage
   class Previewer::VideoPreviewer < Previewer
+    class_attribute :ffmpeg_path, default: "ffmpeg"
+
     def self.accept?(blob)
       blob.video?
     end
@@ -16,7 +18,7 @@ module ActiveStorage
 
     private
       def draw_relevant_frame_from(file, &block)
-        draw "ffmpeg", "-i", file.path, "-y", "-vcodec", "png",
+        draw ffmpeg_path, "-i", file.path, "-y", "-vcodec", "png",
           "-vf", "thumbnail", "-vframes", "1", "-f", "image2", "-", &block
       end
   end
diff --git a/activestorage/lib/active_storage/service.rb b/activestorage/lib/active_storage/service.rb
index 343e5587d5..f2e1269f27 100644
--- a/activestorage/lib/active_storage/service.rb
+++ b/activestorage/lib/active_storage/service.rb
@@ -78,6 +78,11 @@ module ActiveStorage
       raise NotImplementedError
     end
 
+    # Delete files at keys starting with the +prefix+.
+    def delete_prefixed(prefix)
+      raise NotImplementedError
+    end
+
     # Return +true+ if a file exists at the +key+.
     def exist?(key)
       raise NotImplementedError
@@ -104,10 +109,10 @@ module ActiveStorage
     end
 
     private
-      def instrument(operation, key, payload = {}, &block)
+      def instrument(operation, payload = {}, &block)
         ActiveSupport::Notifications.instrument(
           "service_#{operation}.active_storage",
-          payload.merge(key: key, service: service_name), &block)
+          payload.merge(service: service_name), &block)
       end
 
       def service_name
diff --git a/activestorage/lib/active_storage/service/azure_storage_service.rb b/activestorage/lib/active_storage/service/azure_storage_service.rb
index f3877ad9c9..19b09991b3 100644
--- a/activestorage/lib/active_storage/service/azure_storage_service.rb
+++ b/activestorage/lib/active_storage/service/azure_storage_service.rb
@@ -19,7 +19,7 @@ module ActiveStorage
     end
 
     def upload(key, io, checksum: nil)
-      instrument :upload, key, checksum: checksum do
+      instrument :upload, key: key, checksum: checksum do
         begin
           blobs.create_block_blob(container, key, io, content_md5: checksum)
         rescue Azure::Core::Http::HTTPError
@@ -30,11 +30,11 @@ module ActiveStorage
 
     def download(key, &block)
       if block_given?
-        instrument :streaming_download, key do
+        instrument :streaming_download, key: key do
           stream(key, &block)
         end
       else
-        instrument :download, key do
+        instrument :download, key: key do
           _, io = blobs.get_blob(container, key)
           io.force_encoding(Encoding::BINARY)
         end
@@ -42,7 +42,7 @@ module ActiveStorage
     end
 
     def delete(key)
-      instrument :delete, key do
+      instrument :delete, key: key do
         begin
           blobs.delete_blob(container, key)
         rescue Azure::Core::Http::HTTPError
@@ -51,8 +51,24 @@ module ActiveStorage
       end
     end
 
+    def delete_prefixed(prefix)
+      instrument :delete_prefixed, prefix: prefix do
+        marker = nil
+
+        loop do
+          results = blobs.list_blobs(container, prefix: prefix, marker: marker)
+
+          results.each do |blob|
+            blobs.delete_blob(container, blob.name)
+          end
+
+          break unless marker = results.continuation_token.presence
+        end
+      end
+    end
+
     def exist?(key)
-      instrument :exist, key do |payload|
+      instrument :exist, key: key do |payload|
         answer = blob_for(key).present?
         payload[:exist] = answer
         answer
@@ -60,7 +76,7 @@ module ActiveStorage
     end
 
     def url(key, expires_in:, filename:, disposition:, content_type:)
-      instrument :url, key do |payload|
+      instrument :url, key: key do |payload|
         base_url = url_for(key)
         generated_url = signer.signed_uri(
           URI(base_url), false,
@@ -77,7 +93,7 @@ module ActiveStorage
     end
 
     def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
-      instrument :url, key do |payload|
+      instrument :url, key: key do |payload|
         base_url = url_for(key)
         generated_url = signer.signed_uri(URI(base_url), false, permissions: "rw",
           expiry: format_expiry(expires_in)).to_s
diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb
index 52eaba4e7b..a8728c5bc3 100644
--- a/activestorage/lib/active_storage/service/disk_service.rb
+++ b/activestorage/lib/active_storage/service/disk_service.rb
@@ -16,7 +16,7 @@ module ActiveStorage
     end
 
     def upload(key, io, checksum: nil)
-      instrument :upload, key, checksum: checksum do
+      instrument :upload, key: key, checksum: checksum do
         IO.copy_stream(io, make_path_for(key))
         ensure_integrity_of(key, checksum) if checksum
       end
@@ -24,7 +24,7 @@ module ActiveStorage
 
     def download(key)
       if block_given?
-        instrument :streaming_download, key do
+        instrument :streaming_download, key: key do
           File.open(path_for(key), "rb") do |file|
             while data = file.read(64.kilobytes)
               yield data
@@ -32,14 +32,14 @@ module ActiveStorage
           end
         end
       else
-        instrument :download, key do
+        instrument :download, key: key do
           File.binread path_for(key)
         end
       end
     end
 
     def delete(key)
-      instrument :delete, key do
+      instrument :delete, key: key do
         begin
           File.delete path_for(key)
         rescue Errno::ENOENT
@@ -48,8 +48,16 @@ module ActiveStorage
       end
     end
 
+    def delete_prefixed(prefix)
+      instrument :delete_prefixed, prefix: prefix do
+        Dir.glob(path_for("#{prefix}*")).each do |path|
+          FileUtils.rm_rf(path)
+        end
+      end
+    end
+
     def exist?(key)
-      instrument :exist, key do |payload|
+      instrument :exist, key: key do |payload|
         answer = File.exist? path_for(key)
         payload[:exist] = answer
         answer
@@ -57,7 +65,7 @@ module ActiveStorage
     end
 
     def url(key, expires_in:, filename:, disposition:, content_type:)
-      instrument :url, key do |payload|
+      instrument :url, key: key do |payload|
         verified_key_with_expiration = ActiveStorage.verifier.generate(key, expires_in: expires_in, purpose: :blob_key)
 
         generated_url =
@@ -77,7 +85,7 @@ module ActiveStorage
     end
 
     def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
-      instrument :url, key do |payload|
+      instrument :url, key: key do |payload|
         verified_token_with_expiration = ActiveStorage.verifier.generate(
           {
             key: key,
diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb
index fd9916634a..6f6f4105fe 100644
--- a/activestorage/lib/active_storage/service/gcs_service.rb
+++ b/activestorage/lib/active_storage/service/gcs_service.rb
@@ -14,9 +14,15 @@ module ActiveStorage
     end
 
     def upload(key, io, checksum: nil)
-      instrument :upload, key, checksum: checksum do
+      instrument :upload, key: key, checksum: checksum do
         begin
-          bucket.create_file(io, key, md5: checksum)
+          # The official GCS client library doesn't allow us to create a file with no Content-Type metadata.
+          # We need the file we create to have no Content-Type so we can control it via the response-content-type
+          # param in signed URLs. Workaround: let the GCS client create the file with an inferred
+          # Content-Type (usually "application/octet-stream") then clear it.
+          bucket.create_file(io, key, md5: checksum).update do |file|
+            file.content_type = nil
+          end
         rescue Google::Cloud::InvalidArgumentError
           raise ActiveStorage::IntegrityError
         end
@@ -25,7 +31,7 @@ module ActiveStorage
 
     # FIXME: Download in chunks when given a block.
     def download(key)
-      instrument :download, key do
+      instrument :download, key: key do
         io = file_for(key).download
         io.rewind
 
@@ -38,7 +44,7 @@ module ActiveStorage
     end
 
     def delete(key)
-      instrument :delete, key do
+      instrument :delete, key: key do
         begin
           file_for(key).delete
         rescue Google::Cloud::NotFoundError
@@ -47,8 +53,14 @@ module ActiveStorage
       end
     end
 
+    def delete_prefixed(prefix)
+      instrument :delete_prefixed, prefix: prefix do
+        bucket.files(prefix: prefix).all(&:delete)
+      end
+    end
+
     def exist?(key)
-      instrument :exist, key do |payload|
+      instrument :exist, key: key do |payload|
         answer = file_for(key).exists?
         payload[:exist] = answer
         answer
@@ -56,7 +68,7 @@ module ActiveStorage
     end
 
     def url(key, expires_in:, filename:, content_type:, disposition:)
-      instrument :url, key do |payload|
+      instrument :url, key: key do |payload|
         generated_url = file_for(key).signed_url expires: expires_in, query: {
           "response-content-disposition" => content_disposition_with(type: disposition, filename: filename),
           "response-content-type" => content_type
@@ -69,7 +81,7 @@ module ActiveStorage
     end
 
     def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
-      instrument :url, key do |payload|
+      instrument :url, key: key do |payload|
         generated_url = bucket.signed_url key, method: "PUT", expires: expires_in,
           content_type: content_type, content_md5: checksum
 
diff --git a/activestorage/lib/active_storage/service/mirror_service.rb b/activestorage/lib/active_storage/service/mirror_service.rb
index 39e922f7ab..7eca8ce7f4 100644
--- a/activestorage/lib/active_storage/service/mirror_service.rb
+++ b/activestorage/lib/active_storage/service/mirror_service.rb
@@ -35,6 +35,11 @@ module ActiveStorage
       perform_across_services :delete, key
     end
 
+    # Delete files at keys starting with the +prefix+ on all services.
+    def delete_prefixed(prefix)
+      perform_across_services :delete_prefixed, prefix
+    end
+
     private
       def each_service(&block)
         [ primary, *mirrors ].each(&block)
diff --git a/activestorage/lib/active_storage/service/s3_service.rb b/activestorage/lib/active_storage/service/s3_service.rb
index 6957119780..c95672f338 100644
--- a/activestorage/lib/active_storage/service/s3_service.rb
+++ b/activestorage/lib/active_storage/service/s3_service.rb
@@ -17,7 +17,7 @@ module ActiveStorage
     end
 
     def upload(key, io, checksum: nil)
-      instrument :upload, key, checksum: checksum do
+      instrument :upload, key: key, checksum: checksum do
         begin
           object_for(key).put(upload_options.merge(body: io, content_md5: checksum))
         rescue Aws::S3::Errors::BadDigest
@@ -28,24 +28,30 @@ module ActiveStorage
 
     def download(key, &block)
       if block_given?
-        instrument :streaming_download, key do
+        instrument :streaming_download, key: key do
           stream(key, &block)
         end
       else
-        instrument :download, key do
+        instrument :download, key: key do
           object_for(key).get.body.read.force_encoding(Encoding::BINARY)
         end
       end
     end
 
     def delete(key)
-      instrument :delete, key do
+      instrument :delete, key: key do
         object_for(key).delete
       end
     end
 
+    def delete_prefixed(prefix)
+      instrument :delete_prefixed, prefix: prefix do
+        bucket.objects(prefix: prefix).batch_delete!
+      end
+    end
+
     def exist?(key)
-      instrument :exist, key do |payload|
+      instrument :exist, key: key do |payload|
         answer = object_for(key).exists?
         payload[:exist] = answer
         answer
@@ -53,7 +59,7 @@ module ActiveStorage
     end
 
     def url(key, expires_in:, filename:, disposition:, content_type:)
-      instrument :url, key do |payload|
+      instrument :url, key: key do |payload|
         generated_url = object_for(key).presigned_url :get, expires_in: expires_in.to_i,
           response_content_disposition: content_disposition_with(type: disposition, filename: filename),
           response_content_type: content_type
@@ -65,7 +71,7 @@ module ActiveStorage
     end
 
     def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
-      instrument :url, key do |payload|
+      instrument :url, key: key do |payload|
         generated_url = object_for(key).presigned_url :put, expires_in: expires_in.to_i,
           content_type: content_type, content_length: content_length, content_md5: checksum
 
diff --git a/activestorage/test/analyzer/video_analyzer_test.rb b/activestorage/test/analyzer/video_analyzer_test.rb
index 4a3c4a8bfc..b3b9c97fe4 100644
--- a/activestorage/test/analyzer/video_analyzer_test.rb
+++ b/activestorage/test/analyzer/video_analyzer_test.rb
@@ -21,8 +21,8 @@ class ActiveStorage::Analyzer::VideoAnalyzerTest < ActiveSupport::TestCase
     blob = create_file_blob(filename: "rotated_video.mp4", content_type: "video/mp4")
     metadata = blob.tap(&:analyze).metadata
 
-    assert_equal 640, metadata[:width]
-    assert_equal 480, metadata[:height]
+    assert_equal 480, metadata[:width]
+    assert_equal 640, metadata[:height]
     assert_equal [4, 3], metadata[:aspect_ratio]
     assert_equal 5.227975, metadata[:duration]
     assert_equal 90, metadata[:angle]
diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb
index 6e815997ba..f94e65ed77 100644
--- a/activestorage/test/models/blob_test.rb
+++ b/activestorage/test/models/blob_test.rb
@@ -41,13 +41,21 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
-  test "purge removes from external service" do
+  test "purge deletes file from external service" do
     blob = create_blob
 
     blob.purge
     assert_not ActiveStorage::Blob.service.exist?(blob.key)
   end
 
+  test "purge deletes variants from external service" do
+    blob = create_file_blob
+    variant = blob.variant(resize: "100>").processed
+
+    blob.purge
+    assert_not ActiveStorage::Blob.service.exist?(variant.key)
+  end
+
   private
     def expected_url_for(blob, disposition: :inline)
       query_string = { content_type: blob.content_type, disposition: "#{disposition}; #{blob.filename.parameters}" }.to_param
diff --git a/activestorage/test/service/gcs_service_test.rb b/activestorage/test/service/gcs_service_test.rb
index 1860149da9..7efcd60fb7 100644
--- a/activestorage/test/service/gcs_service_test.rb
+++ b/activestorage/test/service/gcs_service_test.rb
@@ -35,6 +35,20 @@ if SERVICE_CONFIGURATIONS[:gcs]
       assert_match(/storage\.googleapis\.com\/.*response-content-disposition=inline.*test\.txt.*response-content-type=text%2Fplain/,
         @service.url(FIXTURE_KEY, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain"))
     end
+
+    test "signed URL response headers" do
+      begin
+        key  = SecureRandom.base58(24)
+        data = "Something else entirely!"
+        @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data))
+
+        url = @service.url(key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")
+        response = Net::HTTP.get_response(URI(url))
+        assert_equal "text/plain", response.header["Content-Type"]
+      ensure
+        @service.delete key
+      end
+    end
   end
 else
   puts "Skipping GCS Service tests because no GCS configuration was supplied"
diff --git a/activestorage/test/service/shared_service_tests.rb b/activestorage/test/service/shared_service_tests.rb
index ade91ab89a..ce28c4393a 100644
--- a/activestorage/test/service/shared_service_tests.rb
+++ b/activestorage/test/service/shared_service_tests.rb
@@ -75,5 +75,22 @@ module ActiveStorage::Service::SharedServiceTests
         @service.delete SecureRandom.base58(24)
       end
     end
+
+    test "deleting by prefix" do
+      begin
+        @service.upload("a/a/a", StringIO.new(FIXTURE_DATA))
+        @service.upload("a/a/b", StringIO.new(FIXTURE_DATA))
+        @service.upload("a/b/a", StringIO.new(FIXTURE_DATA))
+
+        @service.delete_prefixed("a/a/")
+        assert_not @service.exist?("a/a/a")
+        assert_not @service.exist?("a/a/b")
+        assert @service.exist?("a/b/a")
+      ensure
+        @service.delete("a/a/a")
+        @service.delete("a/a/b")
+        @service.delete("a/b/a")
+      end
+    end
   end
 end