From 5e9a1e055453123fce50088a6df0f9eb5450017b Mon Sep 17 00:00:00 2001 From: bwatson78 Date: Thu, 16 Nov 2023 08:32:07 -0600 Subject: [PATCH 1/7] Updates downloads_controller_spec to process testing successfully. --- .../hyrax/valkyrie_downloads_controller_behavior.rb | 2 +- spec/controllers/hyrax/downloads_controller_spec.rb | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb b/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb index 6ff5189cad..4186161c00 100644 --- a/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb @@ -17,7 +17,7 @@ def send_file_contents_valkyrie(file_set) file_metadata = find_file_metadata(file_set: file_set, use: use, mime_type: mime_type) return unless stale?(last_modified: file_metadata.updated_at, template: false) - file = Valkyrie::StorageAdapter.find_by(id: file_metadata.file_identifier) + file = Hyrax.storage_adapter.find_by(id: file_metadata.file_identifier) prepare_file_headers_valkyrie(metadata: file_metadata, file: file) # Warning - using the range header will load the range selection in to memory diff --git a/spec/controllers/hyrax/downloads_controller_spec.rb b/spec/controllers/hyrax/downloads_controller_spec.rb index dc0edd462e..312e47a650 100644 --- a/spec/controllers/hyrax/downloads_controller_spec.rb +++ b/spec/controllers/hyrax/downloads_controller_spec.rb @@ -9,7 +9,7 @@ let(:user) { FactoryBot.create(:user) } let(:original_file_use) { Hyrax::FileMetadata::Use::ORIGINAL_FILE } - let(:original_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: original_file_use, file_identifier: "disk://#{file_path}") } + let(:original_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: original_file_use, file_identifier: file_path) } let(:file_set) do if Hyrax.config.use_valkyrie? FactoryBot.valkyrie_create(:hyrax_file_set, @@ -64,8 +64,8 @@ let(:original_file_use) { Hyrax::FileMetadata::Use::ORIGINAL_FILE } let(:thumbnail_use) { Hyrax::FileMetadata::Use::THUMBNAIL } let(:file_path) { fixture_path + '/image.png' } - let(:original_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: original_file_use, file_identifier: "disk://#{file_path}") } - let(:thumbnail_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: thumbnail_use, file_identifier: "disk://#{file_path}") } + let(:original_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: original_file_use, file_identifier: file_path) } + let(:thumbnail_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: thumbnail_use, file_identifier: file_path) } let(:original_file) { File.open(file_path) } let(:file_set) do if Hyrax.config.use_valkyrie? @@ -98,7 +98,7 @@ context 'with video file' do let(:service_file_use) { Hyrax::FileMetadata::Use::SERVICE_FILE } let(:file_path) { fixture_path + '/sample_mpeg4.mp4' } - let(:service_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: service_file_use, mime_type: 'video/webm', file_identifier: "disk://#{file_path}") } + let(:service_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: service_file_use, mime_type: 'video/webm', file_identifier: file_path) } let(:file_set) do if Hyrax.config.use_valkyrie? FactoryBot.valkyrie_create(:hyrax_file_set, @@ -140,10 +140,10 @@ let(:thumbnail_use) { Hyrax::FileMetadata::Use::THUMBNAIL } let(:file_path) { fixture_path + '/world.png' } let(:original_file_metadata) do - FactoryBot.valkyrie_create(:hyrax_file_metadata, mime_type: 'image/png', original_filename: 'world.png', use: original_file_use, file_identifier: "disk://#{file_path}") + FactoryBot.valkyrie_create(:hyrax_file_metadata, mime_type: 'image/png', original_filename: 'world.png', use: original_file_use, file_identifier: file_path) end let(:thumbnail_file_metadata) do - FactoryBot.valkyrie_create(:hyrax_file_metadata, use: thumbnail_use, mime_type: 'image/png', original_filename: 'world.png', file_identifier: "disk://#{file_path}") + FactoryBot.valkyrie_create(:hyrax_file_metadata, use: thumbnail_use, mime_type: 'image/png', original_filename: 'world.png', file_identifier: file_path) end let(:original_file) { File.open(file_path) } From 073bd0936c350e2b64558a7fa09f6f3c0e921b69 Mon Sep 17 00:00:00 2001 From: Daniel Pierce Date: Fri, 17 Nov 2023 13:04:25 -0500 Subject: [PATCH 2/7] Refactor downloads controller spec for better coverage This was not testing non-valkyrie mode due to use of `valkyrie_adapter` which is now removed. Added tests for expanded cache control features. Added Range tests for original_file since that can be a different code path. Refactored and deduplicated test setups. --- .../hyrax/downloads_controller_spec.rb | 185 ++++++++++-------- 1 file changed, 107 insertions(+), 78 deletions(-) diff --git a/spec/controllers/hyrax/downloads_controller_spec.rb b/spec/controllers/hyrax/downloads_controller_spec.rb index 312e47a650..d4435663e9 100644 --- a/spec/controllers/hyrax/downloads_controller_spec.rb +++ b/spec/controllers/hyrax/downloads_controller_spec.rb @@ -1,25 +1,26 @@ # frozen_string_literal: true -RSpec.describe Hyrax::DownloadsController, valkyrie_adapter: :test_adapter, storage_adapter: :test_disk do +RSpec.describe Hyrax::DownloadsController do routes { Hyrax::Engine.routes } describe '#show' do - let(:file_path) { fixture_path + '/world.png' } + let(:file_path) { fixture_path + '/image.png' } let(:original_file) { File.open(file_path) } + let(:original_content) { IO.binread(file_path) } + let(:uploaded_file) { FactoryBot.create(:uploaded_file, file: original_file) } let(:user) { FactoryBot.create(:user) } - let(:original_file_use) { Hyrax::FileMetadata::Use::ORIGINAL_FILE } - let(:original_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: original_file_use, file_identifier: file_path) } + let(:original_file_metadata) do + FactoryBot.valkyrie_create(:hyrax_file_metadata, :with_file, :original_file, :image, + original_filename: 'image.png', + file_set: file_set, + file: uploaded_file) + end + let(:file_set) do - if Hyrax.config.use_valkyrie? - FactoryBot.valkyrie_create(:hyrax_file_set, - :in_work, - files: [original_file_metadata], - edit_users: [user], - visibility_setting: Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED) - else - create(:file_with_work, user: user, content: original_file) - end + FactoryBot.valkyrie_create(:hyrax_file_set, + edit_users: [user], + visibility_setting: Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED) end it 'raises an error if the object does not exist' do @@ -47,7 +48,6 @@ context "when user isn't logged in" do context "and the unauthorized image exists" do before do - allow(File).to receive(:exist?).and_return(true) allow(subject).to receive(:authorize!).and_return(false) allow(subject).to receive(:workflow_restriction?).and_return(true) end @@ -61,63 +61,101 @@ end context "when the user has access" do - let(:original_file_use) { Hyrax::FileMetadata::Use::ORIGINAL_FILE } - let(:thumbnail_use) { Hyrax::FileMetadata::Use::THUMBNAIL } - let(:file_path) { fixture_path + '/image.png' } - let(:original_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: original_file_use, file_identifier: file_path) } - let(:thumbnail_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: thumbnail_use, file_identifier: file_path) } - let(:original_file) { File.open(file_path) } - let(:file_set) do - if Hyrax.config.use_valkyrie? - FactoryBot.valkyrie_create(:hyrax_file_set, - :in_work, - files: [original_file_metadata, thumbnail_file_metadata], - edit_users: [user], - visibility_setting: Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED) - else - create(:file_with_work, user: user, content: original_file) - end - end - before do sign_in user end context 'with original file' do before do + original_file_metadata allow(subject).to receive(:authorize!).and_return(true) allow(subject).to receive(:workflow_restriction?).and_return(false) end - it 'sends the original file' do + it 'sends the original file and sends 304 response when client has valid cached data' do + head :show, params: { id: file_set } + head_headers = response.headers get :show, params: { id: file_set } + expect(response).to have_http_status :success + expect(response.headers['Content-Length']).to eq '19102' + expect(response.headers['Accept-Ranges']).to eq "bytes" + expect(response.headers).to eq head_headers expect(response.body).to eq IO.binread(file_path) + + # Caching + request.env['HTTP_IF_MODIFIED_SINCE'] = response.headers['Last-Modified'] + request.env['HTTP_IF_NONE_MATCH'] = response.headers['ETag'] + get :show, params: { id: file_set } + expect(response).to have_http_status :not_modified end - end - context 'with video file' do - let(:service_file_use) { Hyrax::FileMetadata::Use::SERVICE_FILE } - let(:file_path) { fixture_path + '/sample_mpeg4.mp4' } - let(:service_file_metadata) { FactoryBot.valkyrie_create(:hyrax_file_metadata, use: service_file_use, mime_type: 'video/webm', file_identifier: file_path) } - let(:file_set) do - if Hyrax.config.use_valkyrie? - FactoryBot.valkyrie_create(:hyrax_file_set, - :in_work, - files: [original_file_metadata, service_file_metadata], - edit_users: [user], - visibility_setting: Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED) - else - create(:file_with_work, user: user, content: original_file) + context "stream" do + it "head request" do + request.env["HTTP_RANGE"] = 'bytes=0-15' + head :show, params: { id: file_set } + expect(response.headers['Content-Length']).to eq '16' + expect(response.headers['Accept-Ranges']).to eq 'bytes' + expect(response.headers['Content-Type']).to start_with 'image/png' + expect(response.body).to be_blank + end + + it "sends the whole thing" do + request.env["HTTP_RANGE"] = 'bytes=0-19101' + get :show, params: { id: file_set } + expect(response.headers["Content-Range"]).to eq 'bytes 0-19101/19102' + expect(response.headers["Content-Length"]).to eq '19102' + expect(response.headers['Accept-Ranges']).to eq 'bytes' + expect(response.headers['Content-Type']).to start_with "image/png" + expect(response.headers["Content-Disposition"]).to include "attachment; filename=\"image.png\"" + expect(response.body).to eq original_content + expect(response.status).to eq 206 + end + + it "sends the whole thing when the range is open ended" do + request.env["HTTP_RANGE"] = 'bytes=0-' + get :show, params: { id: file_set } + expect(response.body).to eq original_content + end + + it "gets a range not starting at the beginning" do + request.env["HTTP_RANGE"] = 'bytes=19000-19101' + get :show, params: { id: file_set } + expect(response.headers["Content-Range"]).to eq 'bytes 19000-19101/19102' + expect(response.headers["Content-Length"]).to eq '102' + end + + it "gets a range not ending at the end" do + request.env["HTTP_RANGE"] = 'bytes=4-11' + get :show, params: { id: file_set } + expect(response.headers["Content-Range"]).to eq 'bytes 4-11/19102' + expect(response.headers["Content-Length"]).to eq '8' end end + end + + # service file is only supported in valkyrie mode + context 'with video file', skip: !Hyrax.config.use_valkyrie? do + let(:service_file_path) { fixture_path + '/sample_mpeg4.mp4' } + let(:service_file) { File.open(service_file_path) } + let(:service_content) { IO.binread(service_file_path) } + let(:service_uploaded_file) { FactoryBot.create(:uploaded_file, file: service_file) } + let(:service_file_metadata) do + FactoryBot.valkyrie_create(:hyrax_file_metadata, :with_file, :service_file, :video_file, + original_filename: 'sample_mpeg4.mp4', + file_set: file_set, + file: service_uploaded_file) + end + before do + original_file_metadata + service_file_metadata allow(subject).to receive(:authorize!).and_return(true) allow(subject).to receive(:workflow_restriction?).and_return(false) end it 'accepts a mime_type param' do - get :show, params: { id: file_set, file: "webm", mime_type: 'video/webm' } - expect(response.body).to eq IO.binread(file_path) + get :show, params: { id: file_set.id, file: "mp4", mime_type: 'video/mp4' } + expect(response.body).to eq service_content end end @@ -136,45 +174,35 @@ context "with an alternative file" do context "that is persisted" do - let(:original_file_use) { Hyrax::FileMetadata::Use::ORIGINAL_FILE } - let(:thumbnail_use) { Hyrax::FileMetadata::Use::THUMBNAIL } - let(:file_path) { fixture_path + '/world.png' } - let(:original_file_metadata) do - FactoryBot.valkyrie_create(:hyrax_file_metadata, mime_type: 'image/png', original_filename: 'world.png', use: original_file_use, file_identifier: file_path) - end + let(:thumb_file_path) { fixture_path + '/world.png' } + let(:thumb_file) { File.open(thumb_file_path) } + let(:thumb_content) { IO.binread(thumb_file_path) } + let(:thumb_uploaded_file) { FactoryBot.create(:uploaded_file, file: thumb_file) } let(:thumbnail_file_metadata) do - FactoryBot.valkyrie_create(:hyrax_file_metadata, use: thumbnail_use, mime_type: 'image/png', original_filename: 'world.png', file_identifier: file_path) - end - let(:original_file) { File.open(file_path) } - - let(:file_set) do - if Hyrax.config.use_valkyrie? - allow(subject).to receive(:authorize_download!).and_return(true) - FactoryBot.valkyrie_create(:hyrax_file_set, :in_work, files: [original_file_metadata, thumbnail_file_metadata], edit_users: [user], - visibility_setting: Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED) - else - FactoryBot.create(:file_with_work, user: user, content: original_file) - end + FactoryBot.valkyrie_create(:hyrax_file_metadata, :with_file, :thumbnail, :image, + original_filename: 'world.png', + file_set: file_set, + file: thumb_uploaded_file) end - let(:file) { File.open(fixture_path + '/world.png', 'rb') } - let(:content) { file.read } - before do + original_file_metadata + thumbnail_file_metadata + allow(subject).to receive(:authorize_download!).and_return(true) allow(Hyrax::DerivativePath).to receive(:derivative_path_for_reference).and_return(fixture_path + '/world.png') end - it 'sends requested file content' do + it 'sends requested file content and sends 304 response when client has valid cached data' do + head :show, params: { id: file_set, file: 'thumbnail' } + head_headers = response.headers get :show, params: { id: file_set, file: 'thumbnail' } expect(response).to be_successful - expect(response.body).to eq content + expect(response.body).to eq thumb_content expect(response.headers['Content-Length']).to eq '4218' expect(response.headers['Accept-Ranges']).to eq "bytes" - end + expect(response.headers).to eq head_headers - it 'sends 304 response when client has valid cached data' do - get :show, params: { id: file_set, file: 'thumbnail' } - expect(response).to have_http_status :success + # Caching request.env['HTTP_IF_MODIFIED_SINCE'] = response.headers['Last-Modified'] request.env['HTTP_IF_NONE_MATCH'] = response.headers['ETag'] get :show, params: { id: file_set, file: 'thumbnail' } @@ -188,6 +216,7 @@ expect(response.headers['Content-Length']).to eq '16' expect(response.headers['Accept-Ranges']).to eq 'bytes' expect(response.headers['Content-Type']).to start_with 'image/png' + expect(response.body).to be_blank end it "sends the whole thing" do @@ -198,14 +227,14 @@ expect(response.headers['Accept-Ranges']).to eq 'bytes' expect(response.headers['Content-Type']).to start_with "image/png" expect(response.headers["Content-Disposition"]).to include "inline; filename=\"world.png\"" - expect(response.body).to eq content + expect(response.body).to eq thumb_content expect(response.status).to eq 206 end it "sends the whole thing when the range is open ended" do request.env["HTTP_RANGE"] = 'bytes=0-' get :show, params: { id: file_set, file: 'thumbnail' } - expect(response.body).to eq content + expect(response.body).to eq thumb_content end it "gets a range not starting at the beginning" do From af0bdf47caa3454e79a8874d63050e34dbd1cb13 Mon Sep 17 00:00:00 2001 From: Daniel Pierce Date: Fri, 17 Nov 2023 13:05:37 -0500 Subject: [PATCH 3/7] Fill in some missing service_file pieces --- app/models/hyrax/file_metadata.rb | 2 ++ spec/factories/hyrax_file_metadata.rb | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/app/models/hyrax/file_metadata.rb b/app/models/hyrax/file_metadata.rb index 2ea6dce37d..03fd4ec00d 100644 --- a/app/models/hyrax/file_metadata.rb +++ b/app/models/hyrax/file_metadata.rb @@ -52,6 +52,8 @@ def uri_for(use:) EXTRACTED_TEXT when :thumbnail_file THUMBNAIL + when :service_file + SERVICE_FILE else raise ArgumentError, "No PCDM use is recognized for #{use}" end diff --git a/spec/factories/hyrax_file_metadata.rb b/spec/factories/hyrax_file_metadata.rb index c07038c9b1..90dbbcf2f3 100644 --- a/spec/factories/hyrax_file_metadata.rb +++ b/spec/factories/hyrax_file_metadata.rb @@ -44,6 +44,10 @@ use { Hyrax::FileMetadata::Use.uri_for(use: :extracted_file) } end + trait :service_file do + use { Hyrax::FileMetadata::Use.uri_for(use: :service_file) } + end + trait :image do mime_type { 'image/png' } end From 2a36644b155696b90503075ce6783f36913848b1 Mon Sep 17 00:00:00 2001 From: Daniel Pierce Date: Fri, 17 Nov 2023 13:08:10 -0500 Subject: [PATCH 4/7] Allow the factory to override the autodetected mime type. Important if autodetection insists an .mp4 file is application/mp4 and not video/mp4 --- spec/factories/hyrax_file_metadata.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/factories/hyrax_file_metadata.rb b/spec/factories/hyrax_file_metadata.rb index 90dbbcf2f3..b0647a0755 100644 --- a/spec/factories/hyrax_file_metadata.rb +++ b/spec/factories/hyrax_file_metadata.rb @@ -69,7 +69,7 @@ after(:build) do |file_metadata, evaluator| file_metadata.label = evaluator.file.uploader.filename - file_metadata.mime_type = evaluator.file.uploader.content_type + file_metadata.mime_type = evaluator.file.uploader.content_type if file_metadata.mime_type == Hyrax::FileMetadata::GENERIC_MIME_TYPE file_metadata.original_filename = evaluator.file.uploader.filename file_metadata.recorded_size = evaluator.file.uploader.size file_metadata.file_set_id = evaluator.file_set.id From 1d5752c1c17444bcf785e21416f807b710f01416 Mon Sep 17 00:00:00 2001 From: Daniel Pierce Date: Fri, 17 Nov 2023 13:13:18 -0500 Subject: [PATCH 5/7] Add Range and ETag support for stream downloads --- ...ream_file_downloads_controller_behavior.rb | 54 +++++++++++++++++++ app/controllers/hyrax/downloads_controller.rb | 1 + 2 files changed, 55 insertions(+) create mode 100644 app/controllers/concerns/hyrax/stream_file_downloads_controller_behavior.rb diff --git a/app/controllers/concerns/hyrax/stream_file_downloads_controller_behavior.rb b/app/controllers/concerns/hyrax/stream_file_downloads_controller_behavior.rb new file mode 100644 index 0000000000..d868652c09 --- /dev/null +++ b/app/controllers/concerns/hyrax/stream_file_downloads_controller_behavior.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true +module Hyrax + # Overrides Hydra::Controller:DownloadBehavior handing of HEAD requests to + # respond with same headers as a GET request would receive. + module StreamFileDownloadsControllerBehavior + protected + + # Handle the HTTP show request + def send_content + response.headers['Accept-Ranges'] = 'bytes' + if request.headers['HTTP_RANGE'] + send_range + else + send_file_contents + end + end + + # rubocop:disable Metrics/AbcSize + def send_range + _, range = request.headers['HTTP_RANGE'].split('bytes=') + from, to = range.split('-').map(&:to_i) + to = file.size - 1 unless to + length = to - from + 1 + response.headers['Content-Range'] = "bytes #{from}-#{to}/#{file.size}" + response.headers['Content-Length'] = length.to_s + self.status = 206 + prepare_file_headers + + if request.head? + head status + else + stream_body file.stream(request.headers['HTTP_RANGE']) + end + end + # rubocop:enable Metrics/AbcSize + + def send_file_contents + return unless stale?(last_modified: file_last_modified, template: false) + + self.status = 200 + prepare_file_headers + + if request.head? + head status + else + stream_body file.stream + end + end + + def file_last_modified + file.modified_date + end + end +end diff --git a/app/controllers/hyrax/downloads_controller.rb b/app/controllers/hyrax/downloads_controller.rb index 950e22f4b1..857244de0c 100644 --- a/app/controllers/hyrax/downloads_controller.rb +++ b/app/controllers/hyrax/downloads_controller.rb @@ -2,6 +2,7 @@ module Hyrax class DownloadsController < ApplicationController include Hydra::Controller::DownloadBehavior + include Hyrax::StreamFileDownloadsControllerBehavior include Hyrax::LocalFileDownloadsControllerBehavior include Hyrax::ValkyrieDownloadsControllerBehavior include Hyrax::WorkflowsHelper # Provides #workflow_restriction? From 240d0542e85998d2d89a366b0327a013281e6000 Mon Sep 17 00:00:00 2001 From: Daniel Pierce Date: Fri, 17 Nov 2023 13:18:25 -0500 Subject: [PATCH 6/7] Make HEAD and GET headers match; match disposition behavior --- ...ocal_file_downloads_controller_behavior.rb | 31 +++++------- .../valkyrie_downloads_controller_behavior.rb | 47 +++++++++++++++---- 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/app/controllers/concerns/hyrax/local_file_downloads_controller_behavior.rb b/app/controllers/concerns/hyrax/local_file_downloads_controller_behavior.rb index d77123bfac..0e07da1590 100644 --- a/app/controllers/concerns/hyrax/local_file_downloads_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/local_file_downloads_controller_behavior.rb @@ -6,9 +6,7 @@ module LocalFileDownloadsControllerBehavior # Handle the HTTP show request def send_local_content response.headers['Accept-Ranges'] = 'bytes' - if request.head? - local_content_head - elsif request.headers['Range'] + if request.headers['Range'] send_range_for_local_file else send_local_file_contents @@ -26,7 +24,11 @@ def send_range_for_local_file self.status = 206 prepare_local_file_headers # For derivatives stored on the local file system - send_data IO.binread(file, length, from), local_derivative_download_options.merge(status: status) + if request.head? + head status + else + send_data IO.binread(file, length, from), local_derivative_download_options.merge(status: status) + end end def send_local_file_contents @@ -34,7 +36,11 @@ def send_local_file_contents self.status = 200 prepare_local_file_headers # For derivatives stored on the local file system - send_file file, local_derivative_download_options + if request.head? + head status + else + send_file file, local_derivative_download_options + end end def local_file_size @@ -54,16 +60,9 @@ def local_file_last_modified File.mtime(file) if file.is_a? String end - # Override - # render an HTTP HEAD response - def local_content_head - response.headers['Content-Length'] = local_file_size.to_s - head :ok, content_type: local_file_mime_type - end - # Override def prepare_local_file_headers - send_file_headers! local_content_options + send_file_headers! local_derivative_download_options response.headers['Content-Type'] = local_file_mime_type response.headers['Content-Length'] ||= local_file_size.to_s # Prevent Rack::ETag from calculating a digest over body @@ -73,12 +72,6 @@ def prepare_local_file_headers private - # Override the Hydra::Controller::DownloadBehavior#content_options so that - # we have an attachement rather than 'inline' - def local_content_options - { type: local_file_mime_type, filename: local_file_name, disposition: 'attachment' } - end - # Override this method if you want to change the options sent when downloading # a derivative file def local_derivative_download_options diff --git a/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb b/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb index 4186161c00..9c81c1075e 100644 --- a/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb @@ -9,10 +9,10 @@ def show_valkyrie private + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def send_file_contents_valkyrie(file_set) response.headers["Accept-Ranges"] = "bytes" self.status = 200 - use = params.fetch(:file, :original_file).to_sym mime_type = params[:mime_type] file_metadata = find_file_metadata(file_set: file_set, use: use, mime_type: mime_type) return unless stale?(last_modified: file_metadata.updated_at, template: false) @@ -23,23 +23,49 @@ def send_file_contents_valkyrie(file_set) # Warning - using the range header will load the range selection in to memory # this can cause memory bloat if request.headers['Range'] - file.rewind - send_data send_range_valkyrie(file: file), data_options(file_metadata) + if request.head? + prepare_range_headers_valkyrie(file: file) + head status + else + send_data send_range_valkyrie(file: file), data_options(file_metadata) + end + elsif request.head? + head status else send_file file.disk_path, data_options(file_metadata).except(:status) end end + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength def data_options(file_metadata) { type: file_metadata.mime_type, filename: file_metadata.original_filename, - disposition: "inline", + disposition: disposition, status: status } end + def use + params.fetch(:file, :original_file).to_sym + end + + def disposition + if ActiveRecord::Type::Boolean.new.cast(params.fetch(:inline, use != :original_file)) + 'inline' + else + 'attachment' + end + end + def send_range_valkyrie(file:) + from, length = prepare_range_headers_valkyrie(file: file) + file.rewind + file.read from # Seek to start of requested range + file.read length + end + + def prepare_range_headers_valkyrie(file:) _, range = request.headers['Range'].split('bytes=') from, to = range.split('-').map(&:to_i) to = file.size - 1 unless to @@ -47,19 +73,22 @@ def send_range_valkyrie(file:) response.headers['Content-Range'] = "bytes #{from}-#{to}/#{file.size}" response.headers['Content-Length'] = length.to_s self.status = 206 - file.read from # Seek to start of requested range - file.read length + [from, length] end - def prepare_file_headers_valkyrie(metadata:, file:, inline: false) - inline_display = ActiveRecord::Type::Boolean.new.cast(params.fetch(:inline, inline)) - response.headers["Content-Disposition"] = "#{inline_display ? 'inline' : 'attachment'}; filename=#{metadata.original_filename}" + # rubocop:disable Metrics/AbcSize + def prepare_file_headers_valkyrie(metadata:, file:) + response.headers["Content-Disposition"] = + ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: metadata.original_filename) response.headers["Content-Type"] = metadata.mime_type response.headers["Content-Length"] ||= (file.try(:size) || metadata.size.first).to_s + headers["Content-Transfer-Encoding"] = "binary" # Prevent Rack::ETag from calculating a digest over body response.headers["Last-Modified"] = metadata.updated_at.utc.strftime("%a, %d %b %Y %T GMT") self.content_type = metadata.mime_type + response.cache_control[:public] ||= false end + # rubocop:enable Metrics/AbcSize def find_file_metadata(file_set:, use: :original_file, mime_type: nil) if mime_type.nil? From 1ff979dea9aa75b161eebf003e930b36fa32abb2 Mon Sep 17 00:00:00 2001 From: Daniel Pierce Date: Fri, 17 Nov 2023 13:18:34 -0500 Subject: [PATCH 7/7] Revert to using `Valkyrie::StorageAdapter.find_by` It is designed to detect which registered storage adapter to use based on the protocol of the file id. --- .../concerns/hyrax/valkyrie_downloads_controller_behavior.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb b/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb index 9c81c1075e..604515e066 100644 --- a/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/valkyrie_downloads_controller_behavior.rb @@ -17,7 +17,7 @@ def send_file_contents_valkyrie(file_set) file_metadata = find_file_metadata(file_set: file_set, use: use, mime_type: mime_type) return unless stale?(last_modified: file_metadata.updated_at, template: false) - file = Hyrax.storage_adapter.find_by(id: file_metadata.file_identifier) + file = Valkyrie::StorageAdapter.find_by(id: file_metadata.file_identifier) prepare_file_headers_valkyrie(metadata: file_metadata, file: file) # Warning - using the range header will load the range selection in to memory