diff options
18 files changed, 216 insertions, 59 deletions
diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 5a06bf86e3..46c0e80194 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -560,12 +560,14 @@ module ActionController # Returns a parameter for the given +key+. If the +key+ # can't be found, there are several options: With no other arguments, # it will raise an <tt>ActionController::ParameterMissing</tt> error; - # if more arguments are given, then that will be returned; if a block + # if a second argument is given, then that is returned (converted to an + # instance of ActionController::Parameters if possible); if a block # is given, then that will be run and its result returned. # # params = ActionController::Parameters.new(person: { name: "Francesco" }) # params.fetch(:person) # => <ActionController::Parameters {"name"=>"Francesco"} permitted: false> # params.fetch(:none) # => ActionController::ParameterMissing: param is missing or the value is empty: none + # params.fetch(:none, {}) # => <ActionController::Parameters {} permitted: false> # params.fetch(:none, "Francesco") # => "Francesco" # params.fetch(:none) { "Francesco" } # => "Francesco" def fetch(key, *args) diff --git a/activemodel/lib/active_model/type/helpers/time_value.rb b/activemodel/lib/active_model/type/helpers/time_value.rb index 250c4021c6..cb6aa67a9d 100644 --- a/activemodel/lib/active_model/type/helpers/time_value.rb +++ b/activemodel/lib/active_model/type/helpers/time_value.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require "active_support/core_ext/string/zones" require "active_support/core_ext/time/zones" module ActiveModel diff --git a/activemodel/lib/active_model/type/time.rb b/activemodel/lib/active_model/type/time.rb index c094ee0013..b3056b1333 100644 --- a/activemodel/lib/active_model/type/time.rb +++ b/activemodel/lib/active_model/type/time.rb @@ -18,6 +18,8 @@ module ActiveModel case value when ::String value = "2000-01-01 #{value}" + time_hash = ::Date._parse(value) + return if time_hash[:hour].nil? when ::Time value = value.change(year: 2000, day: 1, month: 1) end diff --git a/activemodel/test/cases/type/time_test.rb b/activemodel/test/cases/type/time_test.rb index f7102d1e97..3fbae1a169 100644 --- a/activemodel/test/cases/type/time_test.rb +++ b/activemodel/test/cases/type/time_test.rb @@ -17,6 +17,21 @@ module ActiveModel assert_equal ::Time.utc(2000, 1, 1, 16, 45, 54), type.cast("2015-06-13T19:45:54+03:00") assert_equal ::Time.utc(1999, 12, 31, 21, 7, 8), type.cast("06:07:08+09:00") end + + def test_user_input_in_time_zone + ::Time.use_zone("Pacific Time (US & Canada)") do + type = Type::Time.new + assert_nil type.user_input_in_time_zone(nil) + assert_nil type.user_input_in_time_zone("") + assert_nil type.user_input_in_time_zone("ABC") + + offset = ::Time.zone.formatted_offset + time_string = "2015-02-09T19:45:54#{offset}" + + assert_equal 19, type.user_input_in_time_zone(time_string).hour + assert_equal offset, type.user_input_in_time_zone(time_string).formatted_offset + end + end end end end diff --git a/activerecord/test/cases/attribute_methods_test.rb b/activerecord/test/cases/attribute_methods_test.rb index 4a7427158a..434d32846c 100644 --- a/activerecord/test/cases/attribute_methods_test.rb +++ b/activerecord/test/cases/attribute_methods_test.rb @@ -736,6 +736,16 @@ class AttributeMethodsTest < ActiveRecord::TestCase end end + test "setting invalid string to a zone-aware time attribute" do + in_time_zone "Pacific Time (US & Canada)" do + record = @target.new + time_string = "ABC" + + record.bonus_time = time_string + assert_nil record.bonus_time + end + end + test "removing time zone-aware types" do with_time_zone_aware_types(:datetime) do in_time_zone "Pacific Time (US & Canada)" do diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 679ca0df03..7b724b7b81 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,14 @@ +* Generated attachment getter and setter methods are created + within the model's `GeneratedAssociationMethods` module to + allow overriding and composition using `super`. + + *Josh Susser*, *Jamon Douglas* + +* 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..134d3bb2d9 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(tempdir: nil, &block) + ActiveStorage::Downloader.new(self, tempdir: tempdir).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..caa25418a5 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,8 +22,17 @@ module ActiveStorage end private + # Downloads the blob to a tempfile on disk. Yields the tempfile. + def download_blob_to_tempfile(&block) #:doc: + blob.open tempdir: tempdir, &block + end + def logger #:doc: ActiveStorage.logger end + + def tempdir #:doc: + Dir.tmpdir + end end end diff --git a/activestorage/lib/active_storage/attached/macros.rb b/activestorage/lib/active_storage/attached/macros.rb index 819f00cc06..f99cf35680 100644 --- a/activestorage/lib/active_storage/attached/macros.rb +++ b/activestorage/lib/active_storage/attached/macros.rb @@ -28,7 +28,7 @@ module ActiveStorage # If the +:dependent+ option isn't set, the attachment will be purged # (i.e. destroyed) whenever the record is destroyed. def has_one_attached(name, dependent: :purge_later) - class_eval <<-CODE, __FILE__, __LINE__ + 1 + generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1 def #{name} @active_storage_attached_#{name} ||= ActiveStorage::Attached::One.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"}) end @@ -75,7 +75,7 @@ module ActiveStorage # If the +:dependent+ option isn't set, all the attachments will be purged # (i.e. destroyed) whenever the record is destroyed. def has_many_attached(name, dependent: :purge_later) - class_eval <<-CODE, __FILE__, __LINE__ + 1 + generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1 def #{name} @active_storage_attached_#{name} ||= ActiveStorage::Attached::Many.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"}) end diff --git a/activestorage/lib/active_storage/downloader.rb b/activestorage/lib/active_storage/downloader.rb new file mode 100644 index 0000000000..0e7039e104 --- /dev/null +++ b/activestorage/lib/active_storage/downloader.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module ActiveStorage + class Downloader #:nodoc: + def initialize(blob, tempdir: nil) + @blob = blob + @tempdir = tempdir + end + + def download_blob_to_tempfile + open_tempfile do |file| + download_blob_to file + yield file + end + end + + private + attr_reader :blob, :tempdir + + def open_tempfile + file = Tempfile.open([ "ActiveStorage", tempfile_extension_with_delimiter ], tempdir) + + 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..bff5e42d41 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 tempdir: tempdir, &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/attached_test.rb b/activestorage/test/models/attached_test.rb new file mode 100644 index 0000000000..14395e12df --- /dev/null +++ b/activestorage/test/models/attached_test.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require "test_helper" +require "database/setup" + +class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase + include ActiveJob::TestHelper + + setup do + @user = User.create!(name: "Josh") + end + + teardown { ActiveStorage::Blob.all.each(&:purge) } + + test "overriding has_one_attached methods works" do + # attach blob before messing with getter, which breaks `#attach` + @user.avatar.attach create_blob(filename: "funky.jpg") + + # inherited only + assert_equal "funky.jpg", @user.avatar.filename.to_s + + User.class_eval do + def avatar + super.filename.to_s.reverse + end + end + + # override with super + assert_equal "funky.jpg".reverse, @user.avatar + + User.send(:remove_method, :avatar) + end + + test "overriding has_many_attached methods works" do + # attach blobs before messing with getter, which breaks `#attach` + @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "wonky.jpg") + + # inherited only + assert_equal "funky.jpg", @user.highlights.first.filename.to_s + assert_equal "wonky.jpg", @user.highlights.second.filename.to_s + + User.class_eval do + def highlights + super.reverse + end + end + + # override with super + assert_equal "wonky.jpg", @user.highlights.first.filename.to_s + assert_equal "funky.jpg", @user.highlights.second.filename.to_s + + User.send(:remove_method, :highlights) + end +end diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 17151117db..a013b7a924 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -84,6 +84,27 @@ 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 "open in a custom tempdir" do + tempdir = Dir.mktmpdir + + create_file_blob(filename: "racecar.jpg").open(tempdir: tempdir) do |file| + assert file.binmode? + assert_equal 0, file.pos + assert_match(/\.jpg\z/, file.path) + assert file.path.starts_with?(tempdir) + 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/activesupport/lib/active_support/deprecation/behaviors.rb b/activesupport/lib/active_support/deprecation/behaviors.rb index 66d6f3225a..3abd25aa85 100644 --- a/activesupport/lib/active_support/deprecation/behaviors.rb +++ b/activesupport/lib/active_support/deprecation/behaviors.rb @@ -94,6 +94,10 @@ module ActiveSupport private def arity_coerce(behavior) + unless behavior.respond_to?(:call) + raise ArgumentError, "#{behavior.inspect} is not a valid deprecation behavior." + end + if behavior.arity == 4 || behavior.arity == -1 behavior else diff --git a/activesupport/test/deprecation_test.rb b/activesupport/test/deprecation_test.rb index 60673c032b..105153584d 100644 --- a/activesupport/test/deprecation_test.rb +++ b/activesupport/test/deprecation_test.rb @@ -182,6 +182,14 @@ class DeprecationTest < ActiveSupport::TestCase end end + def test_default_invalid_behavior + e = assert_raises(ArgumentError) do + ActiveSupport::Deprecation.behavior = :invalid + end + + assert_equal ":invalid is not a valid deprecation behavior.", e.message + end + def test_deprecated_instance_variable_proxy assert_not_deprecated { @dtc.request.size } diff --git a/guides/source/active_storage_overview.md b/guides/source/active_storage_overview.md index 292928488b..91ad089d40 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 +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 ------------------- |