diff options
| -rw-r--r-- | activerecord/lib/active_record/associations/belongs_to_association.rb | 25 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations/has_one_association.rb | 56 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations/has_one_through_association.rb | 10 | ||||
| -rw-r--r-- | activestorage/CHANGELOG.md | 5 | ||||
| -rw-r--r-- | activestorage/app/models/active_storage/blob.rb | 7 | ||||
| -rw-r--r-- | activestorage/app/models/active_storage/variant.rb | 8 | ||||
| -rw-r--r-- | activestorage/lib/active_storage/analyzer.rb | 9 | ||||
| -rw-r--r-- | activestorage/lib/active_storage/downloader.rb | 40 | ||||
| -rw-r--r-- | activestorage/lib/active_storage/downloading.rb | 8 | ||||
| -rw-r--r-- | activestorage/lib/active_storage/previewer.rb | 15 | ||||
| -rw-r--r-- | activestorage/test/models/blob_test.rb | 9 | ||||
| -rw-r--r-- | guides/source/active_storage_overview.md | 51 |
12 files changed, 139 insertions, 104 deletions
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb index 1109fee462..c8716741b0 100644 --- a/activerecord/lib/active_record/associations/belongs_to_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_association.rb @@ -16,19 +16,6 @@ module ActiveRecord end end - def replace(record) - if record - raise_on_type_mismatch!(record) - update_counters_on_replace(record) - set_inverse_instance(record) - @updated = true - else - decrement_counters - end - - self.target = record - end - def target=(record) replace_keys(record) super @@ -56,6 +43,18 @@ module ActiveRecord end private + def replace(record) + if record + raise_on_type_mismatch!(record) + update_counters_on_replace(record) + set_inverse_instance(record) + @updated = true + else + decrement_counters + end + + self.target = record + end def update_counters(by) if require_counter_update? && foreign_key_present? diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 4e8e7ee43b..d211884135 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -23,35 +23,6 @@ module ActiveRecord end end - def replace(record, save = true) - raise_on_type_mismatch!(record) if record - load_target - - return target unless target || record - - assigning_another_record = target != record - if assigning_another_record || record.has_changes_to_save? - save &&= owner.persisted? - - transaction_if(save) do - remove_target!(options[:dependent]) if target && !target.destroyed? && assigning_another_record - - if record - set_owner_attributes(record) - set_inverse_instance(record) - - if save && !record.save - nullify_owner_attributes(record) - set_owner_attributes(target) if target - raise RecordNotSaved, "Failed to save the new associated #{reflection.name}." - end - end - end - end - - self.target = record - end - def delete(method = options[:dependent]) if load_target case method @@ -68,6 +39,33 @@ module ActiveRecord end private + def replace(record, save = true) + raise_on_type_mismatch!(record) if record + + return target unless load_target || record + + assigning_another_record = target != record + if assigning_another_record || record.has_changes_to_save? + save &&= owner.persisted? + + transaction_if(save) do + remove_target!(options[:dependent]) if target && !target.destroyed? && assigning_another_record + + if record + set_owner_attributes(record) + set_inverse_instance(record) + + if save && !record.save + nullify_owner_attributes(record) + set_owner_attributes(target) if target + raise RecordNotSaved, "Failed to save the new associated #{reflection.name}." + end + end + end + end + + self.target = record + end # The reason that the save param for replace is false, if for create (not just build), # is because the setting of the foreign keys is actually handled by the scoping when diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb index 019bf0729f..10978b2d93 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -6,12 +6,12 @@ module ActiveRecord class HasOneThroughAssociation < HasOneAssociation #:nodoc: include ThroughAssociation - def replace(record, save = true) - create_through_record(record, save) - self.target = record - end - private + def replace(record, save = true) + create_through_record(record, save) + self.target = record + end + def create_through_record(record, save) ensure_not_nested diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 679ca0df03..761d8f5021 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,8 @@ +* Add `ActiveStorage::Blob#open`, which downloads a blob to a tempfile on disk + and yields the tempfile. Deprecate `ActiveStorage::Downloading`. + + *David Robertson*, *George Claghorn* + * Pass in `identify: false` as an argument when providing a `content_type` for `ActiveStorage::Attached::{One,Many}#attach` to bypass automatic content type inference. For example: diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index b8b3b62f22..9bf0766843 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "active_storage/downloader" + # A blob is a record that contains the metadata about a file and a key for where that file resides on the service. # Blobs can be created in two ways: # @@ -164,6 +166,11 @@ class ActiveStorage::Blob < ActiveRecord::Base service.download key, &block end + # Downloads the blob to a tempfile on disk. Yields the tempfile. + def open(&block) + ActiveStorage::Downloader.new(self).download_blob_to_tempfile(&block) + end + # Deletes the file on the service that's associated with this blob. This should only be done if the blob is going to be # deleted as well or you will essentially have a dead reference. It's recommended to use the +#purge+ and +#purge_later+ diff --git a/activestorage/app/models/active_storage/variant.rb b/activestorage/app/models/active_storage/variant.rb index b782489a92..1df36e37d9 100644 --- a/activestorage/app/models/active_storage/variant.rb +++ b/activestorage/app/models/active_storage/variant.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require "active_storage/downloading" - # Image blobs can have variants that are the result of a set of transformations applied to the original. # These variants are used to create thumbnails, fixed-size avatars, or any other derivative image from the # original. @@ -44,7 +42,7 @@ require "active_storage/downloading" # You can combine any number of ImageMagick/libvips operations into a variant, as well as any macros provided by the # ImageProcessing gem (such as +resize_to_fit+): # -# avatar.variant(resize_to_fit: [800, 800], monochrome: true, flip: "-90") +# avatar.variant(resize_to_fit: [800, 800], monochrome: true, rotate: "-90") # # Visit the following links for a list of available ImageProcessing commands and ImageMagick/libvips operations: # @@ -53,8 +51,6 @@ require "active_storage/downloading" # * {ImageProcessing::Vips}[https://github.com/janko-m/image_processing/blob/master/doc/vips.md#methods] # * {ruby-vips reference}[http://www.rubydoc.info/gems/ruby-vips/Vips/Image] class ActiveStorage::Variant - include ActiveStorage::Downloading - WEB_IMAGE_CONTENT_TYPES = %w( image/png image/jpeg image/jpg image/gif ) attr_reader :blob, :variation @@ -98,7 +94,7 @@ class ActiveStorage::Variant end def process - download_blob_to_tempfile do |image| + blob.open do |image| transform image do |output| upload output end diff --git a/activestorage/lib/active_storage/analyzer.rb b/activestorage/lib/active_storage/analyzer.rb index 7c4168c1a0..8dcfb9d7bd 100644 --- a/activestorage/lib/active_storage/analyzer.rb +++ b/activestorage/lib/active_storage/analyzer.rb @@ -1,13 +1,9 @@ # frozen_string_literal: true -require "active_storage/downloading" - module ActiveStorage # This is an abstract base class for analyzers, which extract metadata from blobs. See # ActiveStorage::Analyzer::ImageAnalyzer for an example of a concrete subclass. class Analyzer - include Downloading - attr_reader :blob # Implement this method in a concrete subclass. Have it return true when given a blob from which @@ -26,6 +22,11 @@ module ActiveStorage end private + # Downloads the blob to a tempfile on disk. Yields the tempfile. + def download_blob_to_tempfile(&block) #:doc: + blob.open(&block) + end + def logger #:doc: ActiveStorage.logger end diff --git a/activestorage/lib/active_storage/downloader.rb b/activestorage/lib/active_storage/downloader.rb new file mode 100644 index 0000000000..7f5b4936a5 --- /dev/null +++ b/activestorage/lib/active_storage/downloader.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module ActiveStorage + class Downloader + def initialize(blob) + @blob = blob + end + + def download_blob_to_tempfile + open_tempfile do |file| + download_blob_to file + yield file + end + end + + private + attr_reader :blob + + def open_tempfile + file = Tempfile.open([ "ActiveStorage", tempfile_extension_with_delimiter ]) + + begin + yield file + ensure + file.close! + end + end + + def download_blob_to(file) + file.binmode + blob.download { |chunk| file.write(chunk) } + file.flush + file.rewind + end + + def tempfile_extension_with_delimiter + blob.filename.extension_with_delimiter + end + end +end diff --git a/activestorage/lib/active_storage/downloading.rb b/activestorage/lib/active_storage/downloading.rb index 7c3d20ade0..df820bc088 100644 --- a/activestorage/lib/active_storage/downloading.rb +++ b/activestorage/lib/active_storage/downloading.rb @@ -1,9 +1,17 @@ # frozen_string_literal: true require "tmpdir" +require "active_support/core_ext/string/filters" module ActiveStorage module Downloading + def self.included(klass) + ActiveSupport::Deprecation.warn <<~MESSAGE.squish, caller_locations(2) + ActiveStorage::Downloading is deprecated and will be removed in Active Storage 6.1. + Use ActiveStorage::Blob#open instead. + MESSAGE + end + private # Opens a new tempfile in #tempdir and copies blob data into it. Yields the tempfile. def download_blob_to_tempfile #:doc: diff --git a/activestorage/lib/active_storage/previewer.rb b/activestorage/lib/active_storage/previewer.rb index cf19987d72..f09dc25879 100644 --- a/activestorage/lib/active_storage/previewer.rb +++ b/activestorage/lib/active_storage/previewer.rb @@ -1,14 +1,10 @@ # frozen_string_literal: true -require "active_storage/downloading" - module ActiveStorage # This is an abstract base class for previewers, which generate images from blobs. See # ActiveStorage::Previewer::PDFPreviewer and ActiveStorage::Previewer::VideoPreviewer for examples of # concrete subclasses. class Previewer - include Downloading - attr_reader :blob # Implement this method in a concrete subclass. Have it return true when given a blob from which @@ -28,6 +24,11 @@ module ActiveStorage end private + # Downloads the blob to a tempfile on disk. Yields the tempfile. + def download_blob_to_tempfile(&block) #:doc: + blob.open(&block) + end + # Executes a system command, capturing its binary output in a tempfile. Yields the tempfile. # # Use this method to shell out to a system library (e.g. mupdf or ffmpeg) for preview image @@ -41,7 +42,7 @@ module ActiveStorage # end # end # - # The output tempfile is opened in the directory returned by ActiveStorage::Downloading#tempdir. + # The output tempfile is opened in the directory returned by #tempdir. def draw(*argv) #:doc: ActiveSupport::Notifications.instrument("preview.active_storage") do open_tempfile_for_drawing do |file| @@ -70,5 +71,9 @@ module ActiveStorage def logger #:doc: ActiveStorage.logger end + + def tempdir #:doc: + Dir.tmpdir + end end end diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 17151117db..1bc5951d79 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -84,6 +84,15 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase assert_equal "a" * 64.kilobytes, chunks.second end + test "open" do + create_file_blob(filename: "racecar.jpg").open do |file| + assert file.binmode? + assert_equal 0, file.pos + assert_match(/\.jpg\z/, file.path) + assert_equal file_fixture("racecar.jpg").binread, file.read, "Expected downloaded file to match fixture file" + end + end + test "urls expiring in 5 minutes" do blob = create_blob diff --git a/guides/source/active_storage_overview.md b/guides/source/active_storage_overview.md index 292928488b..ffd81f855e 100644 --- a/guides/source/active_storage_overview.md +++ b/guides/source/active_storage_overview.md @@ -380,58 +380,25 @@ Rails.application.routes.url_helpers.rails_blob_path(user.avatar, only_path: tru Downloading Files ----------------- -If you need to process the blobs on the server side, such as, when performing -analysis or further conversions, you can download the blob and get a binary -object: +Sometimes you need to process a blob after it’s uploaded—for example, to convert +it to a different format. Use `ActiveStorage::Blob#download` to read a blob’s +binary data into memory: ```ruby binary = user.avatar.download ``` -In some cases you might want to convert that into an actual file on the disk to -pass the file path to external programs (such as virus scanners, converters, -optimizers, minifiers, etc.). In this case you can include the -`ActiveStorage::Downloading` module into your class which provides helpers to -download directly into files while avoiding to store the file into memory. -`ActiveStorage::Downloading` expects a `blob` method to be defined. +You might want to download a blob to a file on disk so an external program (e.g. +a virus scanner or media transcoder) can operate on it. Use +`ActiveStorage::Blob#open` to download a blob to a tempfile on disk: -```ruby -class VirusScanner - include ActiveStorage::Downloading - - attr_reader :blob - - def initialize(blob) - @blob = blob - end - - def scan - download_blob_to_tempfile do |file| - system 'scan_virus', file.path - end - end -end -``` - -By default, `download_blob_to_tempfile` creates files in `Dir.tmpdir`. If you need to use another directory, override ActiveStorage::Downloading#tempdir in your class: - -```ruby -class VirusScanner - include ActiveStorage::Downloading +````ruby +message.video.open do |file| + system '/path/to/virus/scanner', file.path # ... - - private - def tempdir - '/path/to/tmp' - end end ``` -If the external program is run as a separate program, you might also want to -`chmod` the file and it's directory, as it is inaccessible by other users -because `Tempfile` will set the permissions to `0600`. - - Transforming Images ------------------- |