diff options
Diffstat (limited to 'activestorage')
15 files changed, 178 insertions, 39 deletions
diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index f6f195770c..92e300a440 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,30 @@ +* `ActiveStorage::Service::AzureStorageService` only handles specifically + relevant types of `Azure::Core::Http::HTTPError`. It previously obscured + other types of `HTTPError`, which is the azure-storage gem’s catch-all + exception class. + + *Cameron Bothner* + +* `ActiveStorage::DiskController#show` generates a 404 Not Found response when + the requested file is missing from the disk service. It previously raised + `Errno::ENOENT`. + + *Cameron Bothner* + +* `ActiveStorage::Blob#download` and `ActiveStorage::Blob#open` raise + `ActiveStorage::FileNotFoundError` when the corresponding file is missing + from the storage service. Services translate service-specific missing object + exceptions (e.g. `Google::Cloud::NotFoundError` for the GCS service and + `Errno::ENOENT` for the disk service) into + `ActiveStorage::FileNotFoundError`. + + *Cameron Bothner* + +* Added the `ActiveStorage::SetCurrent` concern for custom Active Storage + controllers that can't inherit from `ActiveStorage::BaseController`. + + *George Claghorn* + * Active Storage error classes like `ActiveStorage::IntegrityError` and `ActiveStorage::UnrepresentableError` now inherit from `ActiveStorage::Error` instead of `StandardError`. This permits rescuing `ActiveStorage::Error` to diff --git a/activestorage/app/assets/javascripts/activestorage.js b/activestorage/app/assets/javascripts/activestorage.js index d3fd795a3a..375eb6b533 100644 --- a/activestorage/app/assets/javascripts/activestorage.js +++ b/activestorage/app/assets/javascripts/activestorage.js @@ -855,14 +855,22 @@ return DirectUploadsController; }(); var processingAttribute = "data-direct-uploads-processing"; + var submitButtonsByForm = new WeakMap(); var started = false; function start() { if (!started) { started = true; + document.addEventListener("click", didClick, true); document.addEventListener("submit", didSubmitForm); document.addEventListener("ajax:before", didSubmitRemoteElement); } } + function didClick(event) { + var target = event.target; + if (target.tagName == "INPUT" && target.type == "submit" && target.form) { + submitButtonsByForm.set(target.form, target); + } + } function didSubmitForm(event) { handleFormSubmissionEvent(event); } @@ -894,7 +902,7 @@ } } function submitForm(form) { - var button = findElement(form, "input[type=submit]"); + var button = submitButtonsByForm.get(form) || findElement(form, "input[type=submit]"); if (button) { var _button = button, disabled = _button.disabled; button.disabled = false; @@ -909,6 +917,7 @@ button.click(); form.removeChild(button); } + submitButtonsByForm.delete(form); } function disable(input) { input.disabled = true; diff --git a/activestorage/app/controllers/active_storage/base_controller.rb b/activestorage/app/controllers/active_storage/base_controller.rb index 59312ac8df..b27d2bd8aa 100644 --- a/activestorage/app/controllers/active_storage/base_controller.rb +++ b/activestorage/app/controllers/active_storage/base_controller.rb @@ -1,10 +1,8 @@ # frozen_string_literal: true -# The base controller for all ActiveStorage controllers. +# The base class for all Active Storage controllers. class ActiveStorage::BaseController < ActionController::Base - protect_from_forgery with: :exception + include ActiveStorage::SetCurrent - before_action do - ActiveStorage::Current.host = request.base_url - end + protect_from_forgery with: :exception end diff --git a/activestorage/app/controllers/active_storage/disk_controller.rb b/activestorage/app/controllers/active_storage/disk_controller.rb index 75cc11d6ff..7bd641ab9a 100644 --- a/activestorage/app/controllers/active_storage/disk_controller.rb +++ b/activestorage/app/controllers/active_storage/disk_controller.rb @@ -13,6 +13,8 @@ class ActiveStorage::DiskController < ActiveStorage::BaseController else head :not_found end + rescue Errno::ENOENT + head :not_found end def update diff --git a/activestorage/app/controllers/concerns/active_storage/set_current.rb b/activestorage/app/controllers/concerns/active_storage/set_current.rb new file mode 100644 index 0000000000..597afe7064 --- /dev/null +++ b/activestorage/app/controllers/concerns/active_storage/set_current.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +# Sets the <tt>ActiveStorage::Current.host</tt> attribute, which the disk service uses to generate URLs. +# Include this concern in custom controllers that call ActiveStorage::Blob#service_url, +# ActiveStorage::Variant#service_url, or ActiveStorage::Preview#service_url so the disk service can +# generate URLs using the same host, protocol, and base path as the current request. +module ActiveStorage::SetCurrent + extend ActiveSupport::Concern + + included do + before_action do + ActiveStorage::Current.host = request.base_url + end + end +end diff --git a/activestorage/app/javascript/activestorage/ujs.js b/activestorage/app/javascript/activestorage/ujs.js index 08c535470d..f5353389ef 100644 --- a/activestorage/app/javascript/activestorage/ujs.js +++ b/activestorage/app/javascript/activestorage/ujs.js @@ -2,16 +2,25 @@ import { DirectUploadsController } from "./direct_uploads_controller" import { findElement } from "./helpers" const processingAttribute = "data-direct-uploads-processing" +const submitButtonsByForm = new WeakMap let started = false export function start() { if (!started) { started = true + document.addEventListener("click", didClick, true) document.addEventListener("submit", didSubmitForm) document.addEventListener("ajax:before", didSubmitRemoteElement) } } +function didClick(event) { + const { target } = event + if (target.tagName == "INPUT" && target.type == "submit" && target.form) { + submitButtonsByForm.set(target.form, target) + } +} + function didSubmitForm(event) { handleFormSubmissionEvent(event) } @@ -49,7 +58,8 @@ function handleFormSubmissionEvent(event) { } function submitForm(form) { - let button = findElement(form, "input[type=submit]") + let button = submitButtonsByForm.get(form) || findElement(form, "input[type=submit]") + if (button) { const { disabled } = button button.disabled = false @@ -64,6 +74,7 @@ function submitForm(form) { button.click() form.removeChild(button) } + submitButtonsByForm.delete(form) } function disable(input) { diff --git a/activestorage/app/jobs/active_storage/analyze_job.rb b/activestorage/app/jobs/active_storage/analyze_job.rb index 2a952f9f74..804ee4557a 100644 --- a/activestorage/app/jobs/active_storage/analyze_job.rb +++ b/activestorage/app/jobs/active_storage/analyze_job.rb @@ -2,6 +2,8 @@ # Provides asynchronous analysis of ActiveStorage::Blob records via ActiveStorage::Blob#analyze_later. class ActiveStorage::AnalyzeJob < ActiveStorage::BaseJob + retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :exponentially_longer + def perform(blob) blob.analyze end diff --git a/activestorage/lib/active_storage/errors.rb b/activestorage/lib/active_storage/errors.rb index f4bf66a615..6475c1d076 100644 --- a/activestorage/lib/active_storage/errors.rb +++ b/activestorage/lib/active_storage/errors.rb @@ -19,4 +19,8 @@ module ActiveStorage # Raised when uploaded or downloaded data does not match a precomputed checksum. # Indicates that a network error or a software bug caused data corruption. class IntegrityError < Error; end + + # Raised when ActiveStorage::Blob#download is called on a blob where the + # backing file is no longer present in its service. + class FileNotFoundError < Error; end end diff --git a/activestorage/lib/active_storage/service/azure_storage_service.rb b/activestorage/lib/active_storage/service/azure_storage_service.rb index b26234c722..8de3889cb5 100644 --- a/activestorage/lib/active_storage/service/azure_storage_service.rb +++ b/activestorage/lib/active_storage/service/azure_storage_service.rb @@ -19,10 +19,8 @@ module ActiveStorage def upload(key, io, checksum: nil) instrument :upload, key: key, checksum: checksum do - begin + handle_errors do blobs.create_block_blob(container, key, IO.try_convert(io) || io, content_md5: checksum) - rescue Azure::Core::Http::HTTPError - raise ActiveStorage::IntegrityError end end end @@ -34,16 +32,20 @@ module ActiveStorage end else instrument :download, key: key do - _, io = blobs.get_blob(container, key) - io.force_encoding(Encoding::BINARY) + handle_errors do + _, io = blobs.get_blob(container, key) + io.force_encoding(Encoding::BINARY) + end end end end def download_chunk(key, range) instrument :download_chunk, key: key, range: range do - _, io = blobs.get_blob(container, key, start_range: range.begin, end_range: range.exclude_end? ? range.end - 1 : range.end) - io.force_encoding(Encoding::BINARY) + handle_errors do + _, io = blobs.get_blob(container, key, start_range: range.begin, end_range: range.exclude_end? ? range.end - 1 : range.end) + io.force_encoding(Encoding::BINARY) + end end end @@ -51,7 +53,8 @@ module ActiveStorage instrument :delete, key: key do begin blobs.delete_blob(container, key) - rescue Azure::Core::Http::HTTPError + rescue Azure::Core::Http::HTTPError => e + raise unless e.type == "BlobNotFound" # Ignore files already deleted end end @@ -139,11 +142,26 @@ module ActiveStorage chunk_size = 5.megabytes offset = 0 + raise ActiveStorage::FileNotFoundError unless blob.present? + while offset < blob.properties[:content_length] _, chunk = blobs.get_blob(container, key, start_range: offset, end_range: offset + chunk_size - 1) yield chunk.force_encoding(Encoding::BINARY) offset += chunk_size end end + + def handle_errors + yield + rescue Azure::Core::Http::HTTPError => e + case e.type + when "BlobNotFound" + raise ActiveStorage::FileNotFoundError + when "Md5Mismatch" + raise ActiveStorage::IntegrityError + else + raise + end + end end end diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb index 9f304b7e01..52f3a3df16 100644 --- a/activestorage/lib/active_storage/service/disk_service.rb +++ b/activestorage/lib/active_storage/service/disk_service.rb @@ -22,27 +22,31 @@ module ActiveStorage end end - def download(key) + def download(key, &block) if block_given? instrument :streaming_download, key: key do - File.open(path_for(key), "rb") do |file| - while data = file.read(5.megabytes) - yield data - end - end + stream key, &block end else instrument :download, key: key do - File.binread path_for(key) + begin + File.binread path_for(key) + rescue Errno::ENOENT + raise ActiveStorage::FileNotFoundError + end end end end def download_chunk(key, range) instrument :download_chunk, key: key, range: range do - File.open(path_for(key), "rb") do |file| - file.seek range.begin - file.read range.size + begin + File.open(path_for(key), "rb") do |file| + file.seek range.begin + file.read range.size + end + rescue Errno::ENOENT + raise ActiveStorage::FileNotFoundError end end end @@ -122,6 +126,16 @@ module ActiveStorage end private + def stream(key) + File.open(path_for(key), "rb") do |file| + while data = file.read(5.megabytes) + yield data + end + end + rescue Errno::ENOENT + raise ActiveStorage::FileNotFoundError + end + def folder_for(key) [ key[0..1], key[2..3] ].join("/") end diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb index eb46973509..18c0f14cfc 100644 --- a/activestorage/lib/active_storage/service/gcs_service.rb +++ b/activestorage/lib/active_storage/service/gcs_service.rb @@ -34,14 +34,22 @@ module ActiveStorage end else instrument :download, key: key do - file_for(key).download.string + begin + file_for(key).download.string + rescue Google::Cloud::NotFoundError + raise ActiveStorage::FileNotFoundError + end end end end def download_chunk(key, range) instrument :download_chunk, key: key, range: range do - file_for(key).download(range: range).string + begin + file_for(key).download(range: range).string + rescue Google::Cloud::NotFoundError + raise ActiveStorage::FileNotFoundError + end end end @@ -116,6 +124,8 @@ module ActiveStorage chunk_size = 5.megabytes offset = 0 + raise ActiveStorage::FileNotFoundError unless file.present? + while offset < file.size yield file.download(range: offset..(offset + chunk_size - 1)).string offset += chunk_size diff --git a/activestorage/lib/active_storage/service/s3_service.rb b/activestorage/lib/active_storage/service/s3_service.rb index 0286e7ff21..89a9e54158 100644 --- a/activestorage/lib/active_storage/service/s3_service.rb +++ b/activestorage/lib/active_storage/service/s3_service.rb @@ -33,14 +33,22 @@ module ActiveStorage end else instrument :download, key: key do - object_for(key).get.body.string.force_encoding(Encoding::BINARY) + begin + object_for(key).get.body.string.force_encoding(Encoding::BINARY) + rescue Aws::S3::Errors::NoSuchKey + raise ActiveStorage::FileNotFoundError + end end end end def download_chunk(key, range) instrument :download_chunk, key: key, range: range do - object_for(key).get(range: "bytes=#{range.begin}-#{range.exclude_end? ? range.end - 1 : range.end}").body.read.force_encoding(Encoding::BINARY) + begin + object_for(key).get(range: "bytes=#{range.begin}-#{range.exclude_end? ? range.end - 1 : range.end}").body.read.force_encoding(Encoding::BINARY) + rescue Aws::S3::Errors::NoSuchKey + raise ActiveStorage::FileNotFoundError + end end end @@ -103,6 +111,8 @@ module ActiveStorage chunk_size = 5.megabytes offset = 0 + raise ActiveStorage::FileNotFoundError unless object.exists? + while offset < object.content_length yield object.get(range: "bytes=#{offset}-#{offset + chunk_size - 1}").body.read.force_encoding(Encoding::BINARY) offset += chunk_size diff --git a/activestorage/test/controllers/disk_controller_test.rb b/activestorage/test/controllers/disk_controller_test.rb index c053052f6f..4bc61d13f3 100644 --- a/activestorage/test/controllers/disk_controller_test.rb +++ b/activestorage/test/controllers/disk_controller_test.rb @@ -31,6 +31,14 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest assert_equal " worl", response.body end + test "showing blob that does not exist" do + blob = create_blob + blob.delete + + get blob.service_url + assert_response :not_found + end + test "directly uploading blob with integrity" do data = "Something else entirely!" diff --git a/activestorage/test/jobs/purge_job_test.rb b/activestorage/test/jobs/purge_job_test.rb index ed4100b78d..251022a96f 100644 --- a/activestorage/test/jobs/purge_job_test.rb +++ b/activestorage/test/jobs/purge_job_test.rb @@ -24,14 +24,4 @@ class ActiveStorage::PurgeJobTest < ActiveJob::TestCase end end end - - test "ignores attached blob" do - User.create! name: "DHH", avatar: @blob - - perform_enqueued_jobs do - assert_nothing_raised do - ActiveStorage::PurgeJob.perform_later @blob - end - end - end end diff --git a/activestorage/test/service/shared_service_tests.rb b/activestorage/test/service/shared_service_tests.rb index 30cfca4e36..58f189af2b 100644 --- a/activestorage/test/service/shared_service_tests.rb +++ b/activestorage/test/service/shared_service_tests.rb @@ -50,6 +50,13 @@ module ActiveStorage::Service::SharedServiceTests assert_equal FIXTURE_DATA, @service.download(@key) end + test "downloading a nonexistent file" do + assert_raises(ActiveStorage::FileNotFoundError) do + @service.download(SecureRandom.base58(24)) + end + end + + test "downloading in chunks" do key = SecureRandom.base58(24) expected_chunks = [ "a" * 5.megabytes, "b" ] @@ -68,11 +75,25 @@ module ActiveStorage::Service::SharedServiceTests end end + test "downloading a nonexistent file in chunks" do + assert_raises(ActiveStorage::FileNotFoundError) do + @service.download(SecureRandom.base58(24)) {} + end + end + + test "downloading partially" do assert_equal "\x10\x00\x00", @service.download_chunk(@key, 19..21) assert_equal "\x10\x00\x00", @service.download_chunk(@key, 19...22) end + test "partially downloading a nonexistent file" do + assert_raises(ActiveStorage::FileNotFoundError) do + @service.download_chunk(SecureRandom.base58(24), 19..21) + end + end + + test "existing" do assert @service.exist?(@key) assert_not @service.exist?(@key + "nonsense") |