Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix single use links for Valkyrie. #6226

Merged
merged 5 commits into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def send_file_contents_valkyrie(file_set)
file.rewind
send_data send_range_valkyrie(file: file), data_options(file_metadata)
else
send_file file.disk_path
send_file file.disk_path, data_options(file_metadata).except(:status)
end
end

Expand Down
18 changes: 16 additions & 2 deletions app/controllers/hyrax/single_use_links_viewer_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ def show

private

def send_content
if @asset.is_a?(Hyrax::FileSet)
send_file_contents_valkyrie(@asset)
else
super
end
end

def curation_concern
response, _document_list = search_service.search_results
response.documents.first
Expand All @@ -52,6 +60,12 @@ def content_options
end
end

def data_options(file_metadata)
super.tap do |options|
options[:disposition] = 'attachment' if action_name == 'download'
end
end

# This is called in a before filter. It causes @asset to be set.
def authorize_download!
authorize! :read, asset
Expand All @@ -66,7 +80,7 @@ def not_found_exception
end

def asset
@asset ||= Hyrax.query_service.find_by_alternate_identifier(alternate_identifier: single_use_link.item_id, use_valkyrie: false)
@asset ||= Hyrax.query_service.find_by(id: single_use_link.item_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏🏻

end

def current_ability
Expand Down Expand Up @@ -94,7 +108,7 @@ def initialize(user, single_use_link)
return unless single_use_link

@single_use_link = single_use_link
can :read, [ActiveFedora::Base, ::SolrDocument] do |obj|
can :read, [ActiveFedora::Base, ::SolrDocument, Hyrax::Resource] do |obj|
single_use_link.valid? && single_use_link.item_id == obj.id && single_use_link.destroy!
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
routes { Hyrax::Engine.routes }
let(:user) { build(:user) }
let(:file) do
create(:file_set, label: 'world.png', user: user)
Hyrax.query_service.find_by(id: work.member_ids.first)
end
let(:file_metadata) { Hyrax.custom_queries.find_file_metadata_by(id: file.original_file_id) }
let(:disk_file) { Hyrax.storage_adapter.find_by(id: file_metadata.file_identifier) }
let(:work) do
FactoryBot.valkyrie_create(:hyrax_work, uploaded_files: [FactoryBot.create(:uploaded_file)])
end

describe "retrieval links" do
Expand All @@ -12,17 +17,16 @@
end

let :download_link do
Hydra::Works::AddFileToFileSet.call(file, File.open(fixture_path + '/world.png'), :original_file)
SingleUseLink.create item_id: file.id, path: Hyrax::Engine.routes.url_helpers.download_path(id: file, locale: 'en')
SingleUseLink.create item_id: file.id, path: Hyrax::Engine.routes.url_helpers.download_path(id: file.id.to_s, locale: 'en')
end

let(:show_link_hash) { show_link.download_key }
let(:download_link_hash) { download_link.download_key }

describe "GET 'download'" do
let(:expected_content) { Hyrax.query_service.find_by_alternate_identifier(alternate_identifier: file.id, use_valkyrie: false).original_file.content }
let(:expected_content) { disk_file.read }
it "downloads the file and deletes the link from the database" do
expect(controller).to receive(:send_file_headers!).with({ filename: 'world.png', disposition: 'attachment', type: 'image/png' })
expect(controller).to receive(:send_file_headers!).with({ filename: 'image.jp2', disposition: 'attachment', type: file_metadata.mime_type })
get :download, params: { id: download_link_hash }
expect(response.body).to eq expected_content
expect(response).to be_successful
Expand Down
10 changes: 10 additions & 0 deletions spec/factories/hyrax_work.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
members { nil }
visibility_setting { nil }
with_index { true }
uploaded_files { [] }
end

after(:build) do |work, evaluator|
Expand All @@ -41,6 +42,15 @@
.new(resource: work)
.assign_access_for(visibility: evaluator.visibility_setting)
end
if evaluator.uploaded_files.present?
Hyrax::WorkUploadsHandler.new(work: work).add(files: evaluator.uploaded_files).attach
evaluator.uploaded_files.each do |file|
allow(Hyrax.config.characterization_service).to receive(:run).and_return(true)
# I don't love this - we might want to just run background jobs so
# this is more real, but we'd have to stub some things.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we do

ActiveJob::Base.queue_adapter.filter += [ValkyrieIngestJob]

Hyrax already makes use of the ActiveJob test adapter. i guess the real implementation is probably a bit fussy, since we need to reach into config and then reset it to be really clean:

old_perform_enqueued_setting = ActiveJob::Base.queue_adapter.perform_enqueued_jobs
old_job_filter = ActiveJob::Base.queue_adapter.filter 
ActiveJob::Base.queue_adapter.perform_enqueued_jobs = true
ActiveJob::Base.queue_adapter.filter += [ValkyrieIngestJob]

allow(Hyrax.config.characterization_service).to receive(:run).and_return(true)
Hyrax::WorkUploadsHandler.new(work: work).add(files: evaluator.uploaded_files).attach

ActiveJob::Base.queue_adapter.perform_enqueued_jobs = old_perform_enqueued_setting
ActiveJob::Base.queue_adapter.filter = old_job_filter

maybe more than is worth messing with?


the stub on Hyrax.config.characterization_service feels like a code smell to me. currently, if i publish file.uploaded from any synchronous code, i'll trigger a long running characterization_service.run inline. since any app can publish that event from any code, that might not be the best.

this issue is probably out of scope here, but fixing it would let us rely on the ActiveJob adapter filter for the characterization behavior too, which would offer a lot of flexiblitiy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a version that did perform_enqueued jobs

The big problem is just fits tries to run and it's not installed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the stub not take effect through the adapter?

ValkyrieIngestJob.perform_now(file)
end
end

work.permission_manager.edit_groups = evaluator.edit_groups
work.permission_manager.edit_users = evaluator.edit_users
Expand Down
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def ci_build?
FactoryBot.register_strategy(:json, JsonStrategy)
FactoryBot.definition_file_paths = [File.expand_path("../factories", __FILE__)]
FactoryBot.find_definitions
require 'rspec/mocks'

require 'shoulda/matchers'
require 'shoulda/callback/matchers'
Expand Down Expand Up @@ -142,6 +143,7 @@ def clean_active_fedora_repository
config.use_transactional_fixtures = false

config.before :suite do
FactoryBot::SyntaxRunner.include RSpec::Mocks::ExampleMethods
Hyrax::RedisEventStore.instance.then(&:flushdb)
DatabaseCleaner.clean_with(:truncation)
# Noid minting causes extra LDP requests which slow the test suite.
Expand Down