Expire caching when a download fail while proxying in ActiveStorage
Fix #51284 Both Proxy controllers in Active Storage set the caching headers early before streaming. In some instances (see #51284), it is possible for the file download (from the service to server) to fail before we send the first byte to the client (response not yet committed). In those instances, this change would invalidate the cache and return a better response status before closing the stream.
This commit is contained in:
parent
3342f13d12
commit
f7ecde8331
@ -61,6 +61,15 @@ def send_blob_stream(blob, disposition: nil) # :doc:
|
||||
blob.download do |chunk|
|
||||
stream.write chunk
|
||||
end
|
||||
rescue ActiveStorage::FileNotFoundError
|
||||
expires_now
|
||||
head :not_found
|
||||
rescue
|
||||
# Status and caching headers are already set, but not commited.
|
||||
# Change the status to 500 manually.
|
||||
expires_now
|
||||
head :internal_server_error
|
||||
raise
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -16,6 +16,32 @@ class ActiveStorage::Blobs::ProxyControllerTest < ActionDispatch::IntegrationTes
|
||||
assert_equal "max-age=3155695200, public", response.headers["Cache-Control"]
|
||||
end
|
||||
|
||||
test "invalidates cache and returns a 404 if the file is not found on download" do
|
||||
blob = create_file_blob(filename: "racecar.jpg")
|
||||
mock_download = lambda do |_|
|
||||
raise ActiveStorage::FileNotFoundError.new "File still uploading!"
|
||||
end
|
||||
blob.service.stub(:download, mock_download) do
|
||||
get rails_storage_proxy_url(blob)
|
||||
end
|
||||
assert_response :not_found
|
||||
assert_equal "no-cache", response.headers["Cache-Control"]
|
||||
end
|
||||
|
||||
|
||||
test "invalidates cache and returns a 500 if an error is raised on download" do
|
||||
blob = create_file_blob(filename: "racecar.jpg")
|
||||
mock_download = lambda do |_|
|
||||
raise StandardError.new "Something is not cool!"
|
||||
end
|
||||
blob.service.stub(:download, mock_download) do
|
||||
get rails_storage_proxy_url(blob)
|
||||
end
|
||||
assert_response :internal_server_error
|
||||
assert_equal "no-cache", response.headers["Cache-Control"]
|
||||
end
|
||||
|
||||
|
||||
test "forcing Content-Type to binary" do
|
||||
get rails_storage_proxy_url(create_blob(content_type: "text/html"))
|
||||
assert_equal "application/octet-stream", response.headers["Content-Type"]
|
||||
|
@ -2,6 +2,7 @@
|
||||
|
||||
require "test_helper"
|
||||
require "database/setup"
|
||||
require "minitest/mock"
|
||||
|
||||
class ActiveStorage::Representations::ProxyControllerWithVariantsTest < ActionDispatch::IntegrationTest
|
||||
setup do
|
||||
@ -84,6 +85,39 @@ class ActiveStorage::Representations::ProxyControllerWithVariantsWithStrictLoadi
|
||||
assert_match(/^inline/, response.headers["Content-Disposition"])
|
||||
assert_equal @blob.variant(@transformations).download, response.body
|
||||
end
|
||||
|
||||
|
||||
test "invalidates cache and returns a 404 if the file is not found on download" do
|
||||
# This mock requires a pre-processed variant as processing the variant will call to download
|
||||
mock_download = lambda do |_|
|
||||
raise ActiveStorage::FileNotFoundError.new "File still uploading!"
|
||||
end
|
||||
|
||||
@blob.service.stub(:download, mock_download) do
|
||||
get rails_blob_representation_proxy_url(
|
||||
filename: @blob.filename,
|
||||
signed_blob_id: @blob.signed_id,
|
||||
variation_key: ActiveStorage::Variation.encode(@transformations))
|
||||
end
|
||||
assert_response :not_found
|
||||
assert_equal "no-cache", response.headers["Cache-Control"]
|
||||
end
|
||||
|
||||
test "invalidates cache and returns a 500 if the an error is raised on download" do
|
||||
# This mock requires a pre-processed variant as processing the variant will call to download
|
||||
mock_download = lambda do |_|
|
||||
raise StandardError.new "Something is not cool!"
|
||||
end
|
||||
|
||||
@blob.service.stub(:download, mock_download) do
|
||||
get rails_blob_representation_proxy_url(
|
||||
filename: @blob.filename,
|
||||
signed_blob_id: @blob.signed_id,
|
||||
variation_key: ActiveStorage::Variation.encode(@transformations))
|
||||
end
|
||||
assert_response :internal_server_error
|
||||
assert_equal "no-cache", response.headers["Cache-Control"]
|
||||
end
|
||||
end
|
||||
|
||||
class ActiveStorage::Representations::ProxyControllerWithPreviewsTest < ActionDispatch::IntegrationTest
|
||||
|
Loading…
Reference in New Issue
Block a user