diff options
Diffstat (limited to 'activestorage')
27 files changed, 320 insertions, 149 deletions
diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 957591ec0a..1475a7a786 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,64 @@ +* Add `config.active_storage.draw_routes` to disable Active Storage routes. + + *Gannon McGibbon* + +* Image analysis is skipped if ImageMagick returns an error. + + `ActiveStorage::Analyzer::ImageAnalyzer#metadata` would previously raise a + `MiniMagick::Error`, which caused persistent `ActiveStorage::AnalyzeJob` + failures. It now logs the error and returns `{}`, resulting in no metadata + being added to the offending image blob. + + *George Claghorn* + +* Method calls on singular attachments return `nil` when no file is attached. + + Previously, assuming the following User model, `user.avatar.filename` would + raise a `Module::DelegationError` if no avatar was attached: + + ```ruby + class User < ApplicationRecord + has_one_attached :avatar + end + ``` + + They now return `nil`. + + *Matthew Tanous* + +* The mirror service supports direct uploads. + + New files are directly uploaded to the primary service. When a + directly-uploaded file is attached to a record, a background job is enqueued + to copy it to each secondary service. + + Configure the queue used to process mirroring jobs by setting + `config.active_storage.queues.mirror`. The default is `:active_storage_mirror`. + + *George Claghorn* + +* The S3 service now permits uploading files larger than 5 gigabytes. + + When uploading a file greater than 100 megabytes in size, the service + transparently switches to [multipart uploads](https://docs.aws.amazon.com/AmazonS3/latest/dev/mpuoverview.html) + using a part size computed from the file's total size and S3's part count limit. + + No application changes are necessary to take advantage of this feature. You + can customize the default 100 MB multipart upload threshold in your S3 + service's configuration: + + ```yaml + production: + service: s3 + access_key_id: <%= Rails.application.credentials.dig(:aws, :access_key_id) %> + secret_access_key: <%= Rails.application.credentials.dig(:aws, :secret_access_key) %> + region: us-east-1 + bucket: my-bucket + upload: + multipart_threshold: <%= 250.megabytes %> + ``` + + *George Claghorn* Please check [6-0-stable](https://github.com/rails/rails/blob/6-0-stable/activestorage/CHANGELOG.md) for previous changes. diff --git a/activestorage/app/jobs/active_storage/mirror_job.rb b/activestorage/app/jobs/active_storage/mirror_job.rb new file mode 100644 index 0000000000..e34faedb56 --- /dev/null +++ b/activestorage/app/jobs/active_storage/mirror_job.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +# Provides asynchronous mirroring of directly-uploaded blobs. +class ActiveStorage::MirrorJob < ActiveStorage::BaseJob + queue_as { ActiveStorage.queues[:mirror] } + + discard_on ActiveStorage::FileNotFoundError + retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :exponentially_longer + + def perform(key, checksum:) + ActiveStorage::Blob.service.try(:mirror, key, checksum: checksum) + end +end diff --git a/activestorage/app/models/active_storage/attachment.rb b/activestorage/app/models/active_storage/attachment.rb index 874ba80ca8..1ee43b1cd5 100644 --- a/activestorage/app/models/active_storage/attachment.rb +++ b/activestorage/app/models/active_storage/attachment.rb @@ -13,7 +13,7 @@ class ActiveStorage::Attachment < ActiveRecord::Base delegate_missing_to :blob - after_create_commit :analyze_blob_later, :identify_blob + after_create_commit :mirror_blob_later, :analyze_blob_later, :identify_blob after_destroy_commit :purge_dependent_blob_later # Synchronously deletes the attachment and {purges the blob}[rdoc-ref:ActiveStorage::Blob#purge]. @@ -37,6 +37,10 @@ class ActiveStorage::Attachment < ActiveRecord::Base blob.analyze_later unless blob.analyzed? end + def mirror_blob_later + blob.mirror_later + end + def purge_dependent_blob_later blob&.purge_later if dependent == :purge_later end diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index c9fbafad1f..6a486dd899 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -207,6 +207,9 @@ class ActiveStorage::Blob < ActiveRecord::Base name: [ "ActiveStorage-#{id}-", filename.extension_with_delimiter ], tmpdir: tmpdir, &block end + def mirror_later #:nodoc: + ActiveStorage::MirrorJob.perform_later(key, checksum: checksum) if service.respond_to?(:mirror) + end # Deletes the files on the service associated with the 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 #purge and #purge_later diff --git a/activestorage/app/models/active_storage/variant.rb b/activestorage/app/models/active_storage/variant.rb index bc0058967a..1859b8482e 100644 --- a/activestorage/app/models/active_storage/variant.rb +++ b/activestorage/app/models/active_storage/variant.rb @@ -96,19 +96,13 @@ class ActiveStorage::Variant end def process - blob.open do |image| - transform(image) { |output| upload(output) } + blob.open do |input| + variation.transform(input, format: format) do |output| + service.upload(key, output) + end end end - def transform(image, &block) - variation.transform(image, format: format, &block) - end - - def upload(file) - service.upload(key, file) - end - def specification @specification ||= diff --git a/activestorage/config/routes.rb b/activestorage/config/routes.rb index 3af7361cff..bde53e72f3 100644 --- a/activestorage/config/routes.rb +++ b/activestorage/config/routes.rb @@ -29,4 +29,4 @@ Rails.application.routes.draw do resolve("ActiveStorage::Blob") { |blob, options| route_for(:rails_blob, blob, options) } resolve("ActiveStorage::Attachment") { |attachment, options| route_for(:rails_blob, attachment.blob, options) } -end +end if ActiveStorage.draw_routes diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb index 5c5da551ae..c35a9920d6 100644 --- a/activestorage/lib/active_storage.rb +++ b/activestorage/lib/active_storage.rb @@ -43,17 +43,26 @@ module ActiveStorage mattr_accessor :logger mattr_accessor :verifier + mattr_accessor :variant_processor, default: :mini_magick + mattr_accessor :queues, default: {} + mattr_accessor :previewers, default: [] - mattr_accessor :analyzers, default: [] - mattr_accessor :variant_processor, default: :mini_magick + mattr_accessor :analyzers, default: [] + mattr_accessor :paths, default: {} - mattr_accessor :variable_content_types, default: [] + + mattr_accessor :variable_content_types, default: [] + mattr_accessor :binary_content_type, default: "application/octet-stream" mattr_accessor :content_types_to_serve_as_binary, default: [] - mattr_accessor :content_types_allowed_inline, default: [] - mattr_accessor :binary_content_type, default: "application/octet-stream" + mattr_accessor :content_types_allowed_inline, default: [] + mattr_accessor :service_urls_expire_in, default: 5.minutes + mattr_accessor :routes_prefix, default: "/rails/active_storage" + mattr_accessor :draw_routes, default: true + + mattr_accessor :replace_on_assign_to_many, default: false module Transformers extend ActiveSupport::Autoload diff --git a/activestorage/lib/active_storage/analyzer/image_analyzer.rb b/activestorage/lib/active_storage/analyzer/image_analyzer.rb index c8bc8fe953..bd1bef3076 100644 --- a/activestorage/lib/active_storage/analyzer/image_analyzer.rb +++ b/activestorage/lib/active_storage/analyzer/image_analyzer.rb @@ -43,6 +43,9 @@ module ActiveStorage rescue LoadError logger.info "Skipping image analysis because the mini_magick gem isn't installed" {} + rescue MiniMagick::Error => error + logger.error "Skipping image analysis due to an ImageMagick error: #{error.message}" + {} end def rotated_image?(image) diff --git a/activestorage/lib/active_storage/attached/model.rb b/activestorage/lib/active_storage/attached/model.rb index ae7f0685f2..06864a846f 100644 --- a/activestorage/lib/active_storage/attached/model.rb +++ b/activestorage/lib/active_storage/attached/model.rb @@ -93,12 +93,19 @@ module ActiveStorage end def #{name}=(attachables) - attachment_changes["#{name}"] = - if attachables.nil? || Array(attachables).none? - ActiveStorage::Attached::Changes::DeleteMany.new("#{name}", self) - else - ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, attachables) + if ActiveStorage.replace_on_assign_to_many + attachment_changes["#{name}"] = + if attachables.nil? || Array(attachables).none? + ActiveStorage::Attached::Changes::DeleteMany.new("#{name}", self) + else + ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, attachables) + end + else + if !attachables.nil? || Array(attachables).any? + attachment_changes["#{name}"] = + ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, #{name}.blobs + attachables) end + end end CODE diff --git a/activestorage/lib/active_storage/attached/one.rb b/activestorage/lib/active_storage/attached/one.rb index c039226fcd..003be1cb43 100644 --- a/activestorage/lib/active_storage/attached/one.rb +++ b/activestorage/lib/active_storage/attached/one.rb @@ -3,7 +3,7 @@ module ActiveStorage # Representation of a single attachment to a model. class Attached::One < Attached - delegate_missing_to :attachment + delegate_missing_to :attachment, allow_nil: true # Returns the associated attachment record. # diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb index cbb205627e..9d9cd02d12 100644 --- a/activestorage/lib/active_storage/engine.rb +++ b/activestorage/lib/active_storage/engine.rb @@ -24,7 +24,7 @@ module ActiveStorage config.active_storage.previewers = [ ActiveStorage::Previewer::PopplerPDFPreviewer, ActiveStorage::Previewer::MuPDFPreviewer, ActiveStorage::Previewer::VideoPreviewer ] config.active_storage.analyzers = [ ActiveStorage::Analyzer::ImageAnalyzer, ActiveStorage::Analyzer::VideoAnalyzer ] config.active_storage.paths = ActiveSupport::OrderedOptions.new - config.active_storage.queues = ActiveSupport::OrderedOptions.new + config.active_storage.queues = ActiveSupport::InheritableOptions.new(mirror: :active_storage_mirror) config.active_storage.variable_content_types = %w( image/png @@ -73,12 +73,15 @@ module ActiveStorage ActiveStorage.analyzers = app.config.active_storage.analyzers || [] ActiveStorage.paths = app.config.active_storage.paths || {} ActiveStorage.routes_prefix = app.config.active_storage.routes_prefix || "/rails/active_storage" + ActiveStorage.draw_routes = app.config.active_storage.draw_routes != false ActiveStorage.variable_content_types = app.config.active_storage.variable_content_types || [] ActiveStorage.content_types_to_serve_as_binary = app.config.active_storage.content_types_to_serve_as_binary || [] ActiveStorage.service_urls_expire_in = app.config.active_storage.service_urls_expire_in || 5.minutes ActiveStorage.content_types_allowed_inline = app.config.active_storage.content_types_allowed_inline || [] ActiveStorage.binary_content_type = app.config.active_storage.binary_content_type || "application/octet-stream" + + ActiveStorage.replace_on_assign_to_many = app.config.active_storage.replace_on_assign_to_many || false end end @@ -130,7 +133,7 @@ module ActiveStorage "config.active_storage.queue is deprecated and will be removed in Rails 6.1. " \ "Set config.active_storage.queues.purge and config.active_storage.queues.analysis instead." - ActiveStorage.queues = { purge: queue, analysis: queue } + ActiveStorage.queues = { purge: queue, analysis: queue, mirror: queue } else ActiveStorage.queues = app.config.active_storage.queues || {} end diff --git a/activestorage/lib/active_storage/log_subscriber.rb b/activestorage/lib/active_storage/log_subscriber.rb index 6c0b4c30e7..f823f31093 100644 --- a/activestorage/lib/active_storage/log_subscriber.rb +++ b/activestorage/lib/active_storage/log_subscriber.rb @@ -32,6 +32,12 @@ module ActiveStorage debug event, color("Generated URL for file at key: #{key_in(event)} (#{event.payload[:url]})", BLUE) end + def service_mirror(event) + message = "Mirrored file at key: #{key_in(event)}" + message += " (checksum: #{event.payload[:checksum]})" if event.payload[:checksum] + debug event, color(message, GREEN) + end + def logger ActiveStorage.logger end diff --git a/activestorage/lib/active_storage/service/azure_storage_service.rb b/activestorage/lib/active_storage/service/azure_storage_service.rb index 993cc0e5f7..8d77e9b20f 100644 --- a/activestorage/lib/active_storage/service/azure_storage_service.rb +++ b/activestorage/lib/active_storage/service/azure_storage_service.rb @@ -17,10 +17,10 @@ module ActiveStorage @container = container end - def upload(key, io, checksum: nil, **) + def upload(key, io, checksum: nil, content_type: nil, **) instrument :upload, key: key, checksum: checksum do handle_errors do - blobs.create_block_blob(container, key, IO.try_convert(io) || io, content_md5: checksum) + blobs.create_block_blob(container, key, IO.try_convert(io) || io, content_md5: checksum, content_type: content_type) end end end diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb index 67892d43b2..764a447c69 100644 --- a/activestorage/lib/active_storage/service/disk_service.rb +++ b/activestorage/lib/active_storage/service/disk_service.rb @@ -84,8 +84,12 @@ module ActiveStorage purpose: :blob_key } ) + current_uri = URI.parse(current_host) + generated_url = url_helpers.rails_disk_service_url(verified_key_with_expiration, - host: current_host, + protocol: current_uri.scheme, + host: current_uri.host, + port: current_uri.port, disposition: content_disposition, content_type: content_type, filename: filename diff --git a/activestorage/lib/active_storage/service/mirror_service.rb b/activestorage/lib/active_storage/service/mirror_service.rb index aa41df304e..c44bd1f360 100644 --- a/activestorage/lib/active_storage/service/mirror_service.rb +++ b/activestorage/lib/active_storage/service/mirror_service.rb @@ -4,12 +4,17 @@ require "active_support/core_ext/module/delegation" module ActiveStorage # Wraps a set of mirror services and provides a single ActiveStorage::Service object that will all - # have the files uploaded to them. A +primary+ service is designated to answer calls to +download+, +exists?+, - # and +url+. + # have the files uploaded to them. A +primary+ service is designated to answer calls to: + # * +download+ + # * +exists?+ + # * +url+ + # * +url_for_direct_upload+ + # * +headers_for_direct_upload+ class Service::MirrorService < Service attr_reader :primary, :mirrors - delegate :download, :download_chunk, :exist?, :url, :path_for, to: :primary + delegate :download, :download_chunk, :exist?, :url, + :url_for_direct_upload, :headers_for_direct_upload, :path_for, to: :primary # Stitch together from named services. def self.build(primary:, mirrors:, configurator:, **options) #:nodoc: @@ -26,7 +31,8 @@ module ActiveStorage # ensure a match when the upload has completed or raise an ActiveStorage::IntegrityError. def upload(key, io, checksum: nil, **options) each_service.collect do |service| - service.upload key, io.tap(&:rewind), checksum: checksum, **options + io.rewind + service.upload key, io, checksum: checksum, **options end end @@ -40,6 +46,21 @@ module ActiveStorage perform_across_services :delete_prefixed, prefix end + + # Copy the file at the +key+ from the primary service to each of the mirrors where it doesn't already exist. + def mirror(key, checksum:) + instrument :mirror, key: key, checksum: checksum do + if (mirrors_in_need_of_mirroring = mirrors.select { |service| !service.exist?(key) }).any? + primary.open(key, checksum: checksum) do |io| + mirrors_in_need_of_mirroring.each do |service| + io.rewind + service.upload key, io, checksum: checksum + end + end + end + end + 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 c7e4ec96a2..e4bd57048a 100644 --- a/activestorage/lib/active_storage/service/s3_service.rb +++ b/activestorage/lib/active_storage/service/s3_service.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +gem "aws-sdk-s3", "~> 1.14" + require "aws-sdk-s3" require "active_support/core_ext/numeric/bytes" @@ -7,20 +9,24 @@ module ActiveStorage # Wraps the Amazon Simple Storage Service (S3) as an Active Storage service. # See ActiveStorage::Service for the generic API documentation that applies to all services. class Service::S3Service < Service - attr_reader :client, :bucket, :upload_options + attr_reader :client, :bucket + attr_reader :multipart_upload_threshold, :upload_options def initialize(bucket:, upload: {}, **options) @client = Aws::S3::Resource.new(**options) @bucket = @client.bucket(bucket) + @multipart_upload_threshold = upload.fetch(:multipart_threshold, 100.megabytes) @upload_options = upload end def upload(key, io, checksum: nil, content_type: nil, **) instrument :upload, key: key, checksum: checksum do - object_for(key).put(upload_options.merge(body: io, content_md5: checksum, content_type: content_type)) - rescue Aws::S3::Errors::BadDigest - raise ActiveStorage::IntegrityError + if io.size < multipart_upload_threshold + upload_with_single_part key, io, checksum: checksum, content_type: content_type + else + upload_with_multipart key, io, content_type: content_type + end end end @@ -94,6 +100,24 @@ module ActiveStorage end private + MAXIMUM_UPLOAD_PARTS_COUNT = 10000 + MINIMUM_UPLOAD_PART_SIZE = 5.megabytes + + def upload_with_single_part(key, io, checksum: nil, content_type: nil) + object_for(key).put(body: io, content_md5: checksum, content_type: content_type, **upload_options) + rescue Aws::S3::Errors::BadDigest + raise ActiveStorage::IntegrityError + end + + def upload_with_multipart(key, io, content_type: nil) + part_size = [ io.size.fdiv(MAXIMUM_UPLOAD_PARTS_COUNT).ceil, MINIMUM_UPLOAD_PART_SIZE ].max + + object_for(key).upload_stream(content_type: content_type, part_size: part_size, **upload_options) do |out| + IO.copy_stream(io, out) + end + end + + def object_for(key) bucket.object(key) end diff --git a/activestorage/test/analyzer/video_analyzer_test.rb b/activestorage/test/analyzer/video_analyzer_test.rb index d30f49315a..172a2f0aae 100644 --- a/activestorage/test/analyzer/video_analyzer_test.rb +++ b/activestorage/test/analyzer/video_analyzer_test.rb @@ -24,7 +24,6 @@ class ActiveStorage::Analyzer::VideoAnalyzerTest < ActiveSupport::TestCase assert_equal 480, metadata[:width] assert_equal 640, metadata[:height] assert_equal [4, 3], metadata[:display_aspect_ratio] - assert_equal 5.227975, metadata[:duration] assert_equal 90, metadata[:angle] end diff --git a/activestorage/test/controllers/blobs_controller_test.rb b/activestorage/test/controllers/blobs_controller_test.rb index 9bf2641de6..9c811df895 100644 --- a/activestorage/test/controllers/blobs_controller_test.rb +++ b/activestorage/test/controllers/blobs_controller_test.rb @@ -20,28 +20,3 @@ class ActiveStorage::BlobsControllerTest < ActionDispatch::IntegrationTest assert_equal "max-age=300, private", @response.headers["Cache-Control"] end end - -if SERVICE_CONFIGURATIONS[:s3] && SERVICE_CONFIGURATIONS[:s3][:access_key_id].present? - class ActiveStorage::S3BlobsControllerTest < ActionDispatch::IntegrationTest - setup do - @old_service = ActiveStorage::Blob.service - ActiveStorage::Blob.service = ActiveStorage::Service.configure(:s3, SERVICE_CONFIGURATIONS) - end - - teardown do - ActiveStorage::Blob.service = @old_service - end - - test "allow redirection to the different host" do - blob = create_file_blob filename: "racecar.jpg" - - assert_nothing_raised { get rails_blob_url(blob) } - assert_response :redirect - assert_no_match @request.host, @response.headers["Location"] - ensure - blob.purge - end - end -else - puts "Skipping S3 redirection tests because no S3 configuration was supplied" -end diff --git a/activestorage/test/controllers/representations_controller_test.rb b/activestorage/test/controllers/representations_controller_test.rb index 4ae0ff877e..2662cc5283 100644 --- a/activestorage/test/controllers/representations_controller_test.rb +++ b/activestorage/test/controllers/representations_controller_test.rb @@ -59,33 +59,3 @@ class ActiveStorage::RepresentationsControllerWithPreviewsTest < ActionDispatch: assert_response :not_found end end - -if SERVICE_CONFIGURATIONS[:s3] && SERVICE_CONFIGURATIONS[:s3][:access_key_id].present? - class ActiveStorage::S3RepresentationsControllerWithVariantsTest < ActionDispatch::IntegrationTest - setup do - @old_service = ActiveStorage::Blob.service - ActiveStorage::Blob.service = ActiveStorage::Service.configure(:s3, SERVICE_CONFIGURATIONS) - end - - teardown do - ActiveStorage::Blob.service = @old_service - end - - test "allow redirection to the different host" do - blob = create_file_blob filename: "racecar.jpg" - - assert_nothing_raised do - get rails_blob_representation_url( - filename: blob.filename, - signed_blob_id: blob.signed_id, - variation_key: ActiveStorage::Variation.encode(resize: "100x100")) - end - assert_response :redirect - assert_no_match @request.host, @response.headers["Location"] - ensure - blob.purge - end - end -else - puts "Skipping S3 redirection tests because no S3 configuration was supplied" -end diff --git a/activestorage/test/fixtures/files/racecar.tif b/activestorage/test/fixtures/files/racecar.tif Binary files differindex 0a11b22896..97430741e2 100644 --- a/activestorage/test/fixtures/files/racecar.tif +++ b/activestorage/test/fixtures/files/racecar.tif diff --git a/activestorage/test/models/attached/many_test.rb b/activestorage/test/models/attached/many_test.rb index e826109874..39ddecb041 100644 --- a/activestorage/test/models/attached/many_test.rb +++ b/activestorage/test/models/attached/many_test.rb @@ -269,44 +269,22 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase end end - test "analyzing a new blob from an uploaded file after attaching it to an existing record" do - perform_enqueued_jobs do - @user.highlights.attach fixture_file_upload("racecar.jpg") - end - - assert @user.highlights.reload.first.analyzed? - assert_equal 4104, @user.highlights.first.metadata[:width] - assert_equal 2736, @user.highlights.first.metadata[:height] - end - - test "analyzing a new blob from an uploaded file after attaching it to an existing record via update" do - perform_enqueued_jobs do - @user.update! highlights: [ fixture_file_upload("racecar.jpg") ] - end - - assert @user.highlights.reload.first.analyzed? - assert_equal 4104, @user.highlights.first.metadata[:width] - assert_equal 2736, @user.highlights.first.metadata[:height] - end + test "updating an existing record with attachments when appending on assign" do + append_on_assign do + @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") - test "analyzing a directly-uploaded blob after attaching it to an existing record" do - perform_enqueued_jobs do - @user.highlights.attach directly_upload_file_blob(filename: "racecar.jpg") - end + assert_difference -> { @user.reload.highlights.count }, +2 do + @user.update! highlights: [ create_blob(filename: "whenever.jpg"), create_blob(filename: "wherever.jpg") ] + end - assert @user.highlights.reload.first.analyzed? - assert_equal 4104, @user.highlights.first.metadata[:width] - assert_equal 2736, @user.highlights.first.metadata[:height] - end + assert_no_difference -> { @user.reload.highlights.count } do + @user.update! highlights: [ ] + end - test "analyzing a directly-uploaded blob after attaching it to an existing record via update" do - perform_enqueued_jobs do - @user.update! highlights: [ directly_upload_file_blob(filename: "racecar.jpg") ] + assert_no_difference -> { @user.reload.highlights.count } do + @user.update! highlights: nil + end end - - assert @user.highlights.reload.first.analyzed? - assert_equal 4104, @user.highlights.first.metadata[:width] - assert_equal 2736, @user.highlights.first.metadata[:height] end test "attaching existing blobs to a new record" do @@ -422,24 +400,6 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase assert_equal "Could not find or build blob: expected attachable, got :foo", error.message end - test "analyzing a new blob from an uploaded file after attaching it to a new record" do - perform_enqueued_jobs do - user = User.create!(name: "Jason", highlights: [ fixture_file_upload("racecar.jpg") ]) - assert user.highlights.reload.first.analyzed? - assert_equal 4104, user.highlights.first.metadata[:width] - assert_equal 2736, user.highlights.first.metadata[:height] - end - end - - test "analyzing a directly-uploaded blob after attaching it to a new record" do - perform_enqueued_jobs do - user = User.create!(name: "Jason", highlights: [ directly_upload_file_blob(filename: "racecar.jpg") ]) - assert user.highlights.reload.first.analyzed? - assert_equal 4104, user.highlights.first.metadata[:width] - assert_equal 2736, user.highlights.first.metadata[:height] - end - end - test "detaching" do [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs| @user.highlights.attach blobs @@ -596,4 +556,12 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase User.remove_method :highlights end end + + private + def append_on_assign + ActiveStorage.replace_on_assign_to_many, previous = false, ActiveStorage.replace_on_assign_to_many + yield + ensure + ActiveStorage.replace_on_assign_to_many = previous + end end diff --git a/activestorage/test/models/attachment_test.rb b/activestorage/test/models/attachment_test.rb new file mode 100644 index 0000000000..94f354d116 --- /dev/null +++ b/activestorage/test/models/attachment_test.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require "test_helper" +require "database/setup" + +class ActiveStorage::AttachmentTest < ActiveSupport::TestCase + include ActiveJob::TestHelper + + setup do + @user = User.create!(name: "Josh") + end + + teardown { ActiveStorage::Blob.all.each(&:delete) } + + test "analyzing a directly-uploaded blob after attaching it" do + blob = directly_upload_file_blob(filename: "racecar.jpg") + assert_not blob.analyzed? + + perform_enqueued_jobs do + @user.highlights.attach(blob) + end + + assert blob.reload.analyzed? + assert_equal 4104, blob.metadata[:width] + assert_equal 2736, blob.metadata[:height] + end + + test "mirroring a directly-uploaded blob after attaching it" do + previous_service, ActiveStorage::Blob.service = ActiveStorage::Blob.service, build_mirror_service + + blob = directly_upload_file_blob + assert_not ActiveStorage::Blob.service.mirrors.second.exist?(blob.key) + + perform_enqueued_jobs do + @user.highlights.attach(blob) + end + + assert ActiveStorage::Blob.service.mirrors.second.exist?(blob.key) + ensure + ActiveStorage::Blob.service = previous_service + end + + private + def build_mirror_service + ActiveStorage::Service::MirrorService.new \ + primary: build_disk_service("primary"), + mirrors: 3.times.collect { |i| build_disk_service("mirror_#{i + 1}") } + end + + def build_disk_service(purpose) + ActiveStorage::Service::DiskService.new(root: Dir.mktmpdir("active_storage_tests_#{purpose}")) + end +end diff --git a/activestorage/test/service/azure_storage_service_test.rb b/activestorage/test/service/azure_storage_service_test.rb index 2b07902d07..fc7b86ccb0 100644 --- a/activestorage/test/service/azure_storage_service_test.rb +++ b/activestorage/test/service/azure_storage_service_test.rb @@ -9,6 +9,20 @@ if SERVICE_CONFIGURATIONS[:azure] include ActiveStorage::Service::SharedServiceTests + test "upload with content_type" do + key = SecureRandom.base58(24) + data = "Foobar" + + @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain") + + url = @service.url(key, expires_in: 2.minutes, disposition: :attachment, content_type: nil, filename: ActiveStorage::Filename.new("test.html")) + response = Net::HTTP.get_response(URI(url)) + assert_equal "text/plain", response.content_type + assert_match(/attachment;.*test\.html/, response["Content-Disposition"]) + ensure + @service.delete key + end + test "signed URL generation" do url = @service.url(@key, expires_in: 5.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("avatar.png"), content_type: "image/png") diff --git a/activestorage/test/service/disk_service_test.rb b/activestorage/test/service/disk_service_test.rb index f3c4dd26bd..b766cc3f56 100644 --- a/activestorage/test/service/disk_service_test.rb +++ b/activestorage/test/service/disk_service_test.rb @@ -8,8 +8,14 @@ class ActiveStorage::Service::DiskServiceTest < ActiveSupport::TestCase include ActiveStorage::Service::SharedServiceTests test "URL generation" do - assert_match(/^https:\/\/example.com\/rails\/active_storage\/disk\/.*\/avatar\.png\?content_type=image%2Fpng&disposition=inline/, - @service.url(@key, expires_in: 5.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("avatar.png"), content_type: "image/png")) + original_url_options = Rails.application.routes.default_url_options.dup + Rails.application.routes.default_url_options.merge!(protocol: "http", host: "test.example.com", port: 3001) + begin + assert_match(/^https:\/\/example.com\/rails\/active_storage\/disk\/.*\/avatar\.png\?content_type=image%2Fpng&disposition=inline/, + @service.url(@key, expires_in: 5.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("avatar.png"), content_type: "image/png")) + ensure + Rails.application.routes.default_url_options = original_url_options + end end test "headers_for_direct_upload generation" do diff --git a/activestorage/test/service/mirror_service_test.rb b/activestorage/test/service/mirror_service_test.rb index 249a5652fb..aa4d610753 100644 --- a/activestorage/test/service/mirror_service_test.rb +++ b/activestorage/test/service/mirror_service_test.rb @@ -53,6 +53,20 @@ class ActiveStorage::Service::MirrorServiceTest < ActiveSupport::TestCase end end + test "mirroring a file from the primary service to secondary services where it doesn't exist" do + key = SecureRandom.base58(24) + data = "Something else entirely!" + checksum = Digest::MD5.base64digest(data) + + @service.primary.upload key, StringIO.new(data), checksum: checksum + @service.mirrors.third.upload key, StringIO.new("Surprise!") + + @service.mirror key, checksum: checksum + assert_equal data, @service.mirrors.first.download(key) + assert_equal data, @service.mirrors.second.download(key) + assert_equal "Surprise!", @service.mirrors.third.download(key) + end + test "URL generation in primary service" do filename = ActiveStorage::Filename.new("test.txt") diff --git a/activestorage/test/service/s3_service_test.rb b/activestorage/test/service/s3_service_test.rb index 74c0aa0405..b9120770e6 100644 --- a/activestorage/test/service/s3_service_test.rb +++ b/activestorage/test/service/s3_service_test.rb @@ -46,8 +46,7 @@ if SERVICE_CONFIGURATIONS[:s3] end test "uploading with server-side encryption" do - config = SERVICE_CONFIGURATIONS.deep_merge(s3: { upload: { server_side_encryption: "AES256" } }) - service = ActiveStorage::Service.configure(:s3, config) + service = build_service(upload: { server_side_encryption: "AES256" }) begin key = SecureRandom.base58(24) @@ -77,6 +76,25 @@ if SERVICE_CONFIGURATIONS[:s3] ensure @service.delete key end + + test "uploading a large object in multiple parts" do + service = build_service(upload: { multipart_threshold: 5.megabytes }) + + begin + key = SecureRandom.base58(24) + data = SecureRandom.bytes(8.megabytes) + + service.upload key, StringIO.new(data), checksum: Digest::MD5.base64digest(data) + assert data == service.download(key) + ensure + service.delete key + end + end + + private + def build_service(configuration) + ActiveStorage::Service.configure :s3, SERVICE_CONFIGURATIONS.deep_merge(s3: configuration) + end end else puts "Skipping S3 Service tests because no S3 configuration was supplied" diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index b34d0d64bb..ac38b9362c 100644 --- a/activestorage/test/test_helper.rb +++ b/activestorage/test/test_helper.rb @@ -7,6 +7,7 @@ require "bundler/setup" require "active_support" require "active_support/test_case" require "active_support/testing/autorun" +require "active_storage/service/mirror_service" require "image_processing/mini_magick" begin @@ -67,7 +68,8 @@ class ActiveSupport::TestCase checksum = Digest::MD5.file(file).base64digest create_blob_before_direct_upload(filename: filename, byte_size: byte_size, checksum: checksum, content_type: content_type).tap do |blob| - ActiveStorage::Blob.service.upload(blob.key, file.open) + service = ActiveStorage::Blob.service.try(:primary) || ActiveStorage::Blob.service + service.upload(blob.key, file.open) end end |