Merge branch 'master' into activestorage-guide

9 files changed, 169 insertions, 17 deletions

diff --git a/activestorage/app/controllers/active_storage/disk_controller.rb b/activestorage/app/controllers/active_storage/disk_controller.rb
index a4fd427cb2..8caecfff49 100644
--- a/activestorage/app/controllers/active_storage/disk_controller.rb
+++ b/activestorage/app/controllers/active_storage/disk_controller.rb
@@ -5,6 +5,8 @@
 # Always go through the BlobsController, or your own authenticated controller, rather than directly
 # to the service url.
 class ActiveStorage::DiskController < ActionController::Base
+  skip_forgery_protection
+
   def show
     if key = decode_verified_key
       send_data disk_service.download(key),
diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index 99823e14c6..2aa05d665e 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -249,7 +249,7 @@ class ActiveStorage::Blob < ActiveRecord::Base
   # You won't ordinarily need to call this method from a Rails application. New blobs are automatically and asynchronously
   # analyzed via #analyze_later when they're attached for the first time.
   def analyze
-    update! metadata: extract_metadata_via_analyzer
+    update! metadata: metadata.merge(extract_metadata_via_analyzer)
   end
 
   # Enqueues an ActiveStorage::AnalyzeJob which calls #analyze.
diff --git a/activestorage/lib/active_storage/attached/macros.rb b/activestorage/lib/active_storage/attached/macros.rb
index f0256718ac..2b38a9b887 100644
--- a/activestorage/lib/active_storage/attached/macros.rb
+++ b/activestorage/lib/active_storage/attached/macros.rb
@@ -32,6 +32,10 @@ module ActiveStorage
         def #{name}
           @active_storage_attached_#{name} ||= ActiveStorage::Attached::One.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"})
         end
+
+        def #{name}=(attachable)
+          #{name}.attach(attachable)
+        end
       CODE
 
       has_one :"#{name}_attachment", -> { where(name: name) }, class_name: "ActiveStorage::Attachment", as: :record
@@ -73,6 +77,10 @@ module ActiveStorage
         def #{name}
           @active_storage_attached_#{name} ||= ActiveStorage::Attached::Many.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"})
         end
+
+        def #{name}=(attachables)
+          #{name}.attach(attachables)
+        end
       CODE
 
       has_many :"#{name}_attachments", -> { where(name: name) }, as: :record, class_name: "ActiveStorage::Attachment"
diff --git a/activestorage/lib/active_storage/attached/many.rb b/activestorage/lib/active_storage/attached/many.rb
index 1e0657c33c..6eace65b79 100644
--- a/activestorage/lib/active_storage/attached/many.rb
+++ b/activestorage/lib/active_storage/attached/many.rb
@@ -13,7 +13,6 @@ module ActiveStorage
     end
 
     # Associates one or several attachments with the current record, saving them to the database.
-    # Examples:
     #
     #   document.images.attach(params[:images]) # Array of ActionDispatch::Http::UploadedFile objects
     #   document.images.attach(params[:signed_blob_id]) # Signed reference to blob from direct upload
@@ -21,7 +20,11 @@ module ActiveStorage
     #   document.images.attach([ first_blob, second_blob ])
     def attach(*attachables)
       attachables.flatten.collect do |attachable|
-        attachments.create!(name: name, blob: create_blob_from(attachable))
+        if record.new_record?
+          attachments.build(record: record, blob: create_blob_from(attachable))
+        else
+          attachments.create!(record: record, blob: create_blob_from(attachable))
+        end
       end
     end
 
@@ -36,6 +39,11 @@ module ActiveStorage
       attachments.any?
     end
 
+    # Deletes associated attachments without purging them, leaving their respective blobs in place.
+    def detach
+      attachments.destroy_all if attached?
+    end
+
     # Directly purges each associated attachment (i.e. destroys the blobs and
     # attachments and deletes the files on the service).
     def purge
diff --git a/activestorage/lib/active_storage/attached/one.rb b/activestorage/lib/active_storage/attached/one.rb
index dc19512484..0244232b2c 100644
--- a/activestorage/lib/active_storage/attached/one.rb
+++ b/activestorage/lib/active_storage/attached/one.rb
@@ -14,7 +14,6 @@ module ActiveStorage
     end
 
     # Associates a given attachment with the current record, saving it to the database.
-    # Examples:
     #
     #   person.avatar.attach(params[:avatar]) # ActionDispatch::Http::UploadedFile object
     #   person.avatar.attach(params[:signed_blob_id]) # Signed reference to blob from direct upload
@@ -24,7 +23,7 @@ module ActiveStorage
       if attached? && dependent == :purge_later
         replace attachable
       else
-        write_attachment create_attachment_from(attachable)
+        write_attachment build_attachment_from(attachable)
       end
     end
 
@@ -39,6 +38,14 @@ module ActiveStorage
       attachment.present?
     end
 
+    # Deletes the attachment without purging it, leaving its blob in place.
+    def detach
+      if attached?
+        attachment.destroy
+        write_attachment nil
+      end
+    end
+
     # Directly purges the attachment (i.e. destroys the blob and
     # attachment and deletes the file on the service).
     def purge
@@ -59,18 +66,14 @@ module ActiveStorage
       def replace(attachable)
         blob.tap do
           transaction do
-            destroy_attachment
-            write_attachment create_attachment_from(attachable)
+            detach
+            write_attachment build_attachment_from(attachable)
           end
         end.purge_later
       end
 
-      def destroy_attachment
-        attachment.destroy
-      end
-
-      def create_attachment_from(attachable)
-        ActiveStorage::Attachment.create!(record: record, name: name, blob: create_blob_from(attachable))
+      def build_attachment_from(attachable)
+        ActiveStorage::Attachment.new(record: record, name: name, blob: create_blob_from(attachable))
       end
 
       def write_attachment(attachment)
diff --git a/activestorage/test/dummy/config/application.rb b/activestorage/test/dummy/config/application.rb
index 7ee6625bb5..06cbc453a2 100644
--- a/activestorage/test/dummy/config/application.rb
+++ b/activestorage/test/dummy/config/application.rb
@@ -19,7 +19,7 @@ Bundler.require(*Rails.groups)
 
 module Dummy
   class Application < Rails::Application
-    config.load_defaults 5.1
+    config.load_defaults 5.2
 
     config.active_storage.service = :local
   end
diff --git a/activestorage/test/dummy/config/environments/test.rb b/activestorage/test/dummy/config/environments/test.rb
index ce0889e8ae..74a802d98c 100644
--- a/activestorage/test/dummy/config/environments/test.rb
+++ b/activestorage/test/dummy/config/environments/test.rb
@@ -30,6 +30,9 @@ Rails.application.configure do
   # Print deprecation notices to the stderr.
   config.active_support.deprecation = :stderr
 
+  # Disable request forgery protection in test environment.
+  config.action_controller.allow_forgery_protection = false
+
   # Raises error for missing translations
   # config.action_view.raise_on_missing_translations = true
 end
diff --git a/activestorage/test/models/attachments_test.rb b/activestorage/test/models/attachments_test.rb
index 47f2bd7911..20eec3c220 100644
--- a/activestorage/test/models/attachments_test.rb
+++ b/activestorage/test/models/attachments_test.rb
@@ -27,7 +27,7 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
 
   test "attach new blob from an UploadedFile" do
     file = file_fixture "racecar.jpg"
-    @user.avatar.attach Rack::Test::UploadedFile.new file
+    @user.avatar.attach Rack::Test::UploadedFile.new file.to_s
     assert_equal "racecar.jpg", @user.avatar.filename.to_s
   end
 
@@ -56,6 +56,40 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
     assert ActiveStorage::Blob.service.exist?(@user.avatar.key)
   end
 
+  test "attach blob to new record" do
+    user = User.new(name: "Jason")
+
+    assert_no_changes -> { user.new_record? } do
+      assert_no_difference -> { ActiveStorage::Attachment.count } do
+        user.avatar.attach create_blob(filename: "funky.jpg")
+      end
+    end
+
+    assert user.avatar.attached?
+    assert_equal "funky.jpg", user.avatar.filename.to_s
+
+    assert_difference -> { ActiveStorage::Attachment.count }, +1 do
+      user.save!
+    end
+
+    assert user.reload.avatar.attached?
+    assert_equal "funky.jpg", user.avatar.filename.to_s
+  end
+
+  test "build new record with attached blob" do
+    assert_no_difference -> { ActiveStorage::Attachment.count } do
+      @user = User.new(name: "Jason", avatar: { io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg" })
+    end
+
+    assert @user.new_record?
+    assert @user.avatar.attached?
+    assert_equal "town.jpg", @user.avatar.filename.to_s
+
+    @user.save!
+    assert @user.reload.avatar.attached?
+    assert_equal "town.jpg", @user.avatar.filename.to_s
+  end
+
   test "access underlying associations of new blob" do
     @user.avatar.attach create_blob(filename: "funky.jpg")
     assert_equal @user, @user.avatar_attachment.record
@@ -88,6 +122,27 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
     end
   end
 
+  test "preserve existing metadata when analyzing a newly-attached blob" do
+    blob = create_file_blob(metadata: { foo: "bar" })
+
+    perform_enqueued_jobs do
+      @user.avatar.attach blob
+    end
+
+    assert_equal "bar", blob.reload.metadata[:foo]
+  end
+
+  test "detach blob" do
+    @user.avatar.attach create_blob(filename: "funky.jpg")
+    avatar_blob_id = @user.avatar.blob.id
+    avatar_key = @user.avatar.key
+
+    @user.avatar.detach
+    assert_not @user.avatar.attached?
+    assert ActiveStorage::Blob.exists?(avatar_blob_id)
+    assert ActiveStorage::Blob.service.exist?(avatar_key)
+  end
+
   test "purge attached blob" do
     @user.avatar.attach create_blob(filename: "funky.jpg")
     avatar_key = @user.avatar.key
@@ -139,6 +194,48 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
     assert_equal "country.jpg", @user.highlights.second.filename.to_s
   end
 
+  test "attach blobs to new record" do
+    user = User.new(name: "Jason")
+
+    assert_no_changes -> { user.new_record? } do
+      assert_no_difference -> { ActiveStorage::Attachment.count } do
+        user.highlights.attach(
+          { io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg" },
+          { io: StringIO.new("IT"), filename: "country.jpg", content_type: "image/jpg" })
+      end
+    end
+
+    assert user.highlights.attached?
+    assert_equal "town.jpg", user.highlights.first.filename.to_s
+    assert_equal "country.jpg", user.highlights.second.filename.to_s
+
+    assert_difference -> { ActiveStorage::Attachment.count }, +2 do
+      user.save!
+    end
+
+    assert user.reload.highlights.attached?
+    assert_equal "town.jpg", user.highlights.first.filename.to_s
+    assert_equal "country.jpg", user.highlights.second.filename.to_s
+  end
+
+  test "build new record with attached blobs" do
+    assert_no_difference -> { ActiveStorage::Attachment.count } do
+      @user = User.new(name: "Jason", highlights: [
+        { io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg" },
+        { io: StringIO.new("IT"), filename: "country.jpg", content_type: "image/jpg" }])
+    end
+
+    assert @user.new_record?
+    assert @user.highlights.attached?
+    assert_equal "town.jpg", @user.highlights.first.filename.to_s
+    assert_equal "country.jpg", @user.highlights.second.filename.to_s
+
+    @user.save!
+    assert @user.reload.highlights.attached?
+    assert_equal "town.jpg", @user.highlights.first.filename.to_s
+    assert_equal "country.jpg", @user.highlights.second.filename.to_s
+  end
+
   test "find attached blobs" do
     @user.highlights.attach(
       { io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg" },
@@ -193,6 +290,36 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
     end
   end
 
+  test "preserve existing metadata when analyzing newly-attached blobs" do
+    blobs = [
+      create_file_blob(filename: "racecar.jpg", content_type: "image/jpeg", metadata: { foo: "bar" }),
+      create_file_blob(filename: "video.mp4", content_type: "video/mp4", metadata: { foo: "bar" })
+    ]
+
+    perform_enqueued_jobs do
+      @user.highlights.attach(blobs)
+    end
+
+    blobs.each do |blob|
+      assert_equal "bar", blob.reload.metadata[:foo]
+    end
+  end
+
+  test "detach blobs" do
+    @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "wonky.jpg")
+    highlight_blob_ids = @user.highlights.collect { |highlight| highlight.blob.id }
+    highlight_keys = @user.highlights.collect(&:key)
+
+    @user.highlights.detach
+    assert_not @user.highlights.attached?
+
+    assert ActiveStorage::Blob.exists?(highlight_blob_ids.first)
+    assert ActiveStorage::Blob.exists?(highlight_blob_ids.second)
+
+    assert ActiveStorage::Blob.service.exist?(highlight_keys.first)
+    assert ActiveStorage::Blob.service.exist?(highlight_keys.second)
+  end
+
   test "purge attached blobs" do
     @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "wonky.jpg")
     highlight_keys = @user.highlights.collect(&:key)
diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb
index 38408cdad3..aaf1d452ea 100644
--- a/activestorage/test/test_helper.rb
+++ b/activestorage/test/test_helper.rb
@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+ENV["RAILS_ENV"] ||= "test"
 require_relative "dummy/config/environment.rb"
 
 require "bundler/setup"
@@ -45,8 +46,8 @@ class ActiveSupport::TestCase
       ActiveStorage::Blob.create_after_upload! io: StringIO.new(data), filename: filename, content_type: content_type
     end
 
-    def create_file_blob(filename: "racecar.jpg", content_type: "image/jpeg")
-      ActiveStorage::Blob.create_after_upload! io: file_fixture(filename).open, filename: filename, content_type: content_type
+    def create_file_blob(filename: "racecar.jpg", content_type: "image/jpeg", metadata: nil)
+      ActiveStorage::Blob.create_after_upload! io: file_fixture(filename).open, filename: filename, content_type: content_type, metadata: metadata
     end
 
     def create_blob_before_direct_upload(filename: "hello.txt", byte_size:, checksum:, content_type: "text/plain")