Skip to content

Commit

Permalink
support optimistic locking for ResourceForm
Browse files Browse the repository at this point in the history
introduces a field and prepopulator for the (maybe confusingly named?) `version`
virtual attribute. `Hyrax::WorkForm` implements this by calling `model.etag`,
but we don't have that luxury here (since non-fedora backed valkyrie may not
have an etag at all). the logic added here is specific Wings adapter, pulling
out the etag for compatibility.

it's probably okay to keep this wings-specific indefinitely. other adapters
should support optimistic locking internally (indeed, so does Wings) so we can
phase out application-layer lock validation in favor of leaning on
Valkyrie/backend datastore features.
  • Loading branch information
tom johnson committed May 26, 2020
1 parent 812bb89 commit 1d66c92
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 6 deletions.
36 changes: 36 additions & 0 deletions app/forms/hyrax/forms/resource_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ def self.ResourceForm(work_class)
self.model_class = work_class

include Hyrax::FormFields(:core_metadata)

##
# @return [String]
def self.inspect
"Hyrax::Forms::ResourceForm(#{model_class})"
end
end
end

Expand All @@ -36,6 +42,26 @@ class Permission < Hyrax::ChangeSet
property :access, virtual: true, prepopulator: ->(_opts) { self.access = model.mode }
end

##
# @api private
#
# @note includes special handling for Wings, to support compatibility
# with `etag`-driven, application-side lock checks. for non-wings adapters
# we want to move away from application side lock validation and rely
# on the adapter/database features instead.
LockKeyPopulator = lambda do |_options|
self.version =
case Hyrax.metadata_adapter
when Wings::Valkyrie::MetadataAdapter
model.persisted? ? Wings::ActiveFedoraConverter.convert(resource: model).etag : ''
else
Hyrax.logger.info 'trying to prepopulate a lock token for ' \
"#{self.class.inspect}, but optimistic locking isn't " \
"supported for the configured adapter: #{Hyrax.metadata_adapter.class}"
''
end
end

class_attribute :model_class

delegate :depositor, :human_readable_type, to: :model
Expand All @@ -60,6 +86,16 @@ class Permission < Hyrax::ChangeSet
property :member_ids, default: []
property :member_of_collection_ids, default: []

# provide a lock token for optimistic locking; we name this `version` for
# backwards compatibility
#
# Hyrax handles lock token validation on the application side for legacy
# models and Wings so we provide a token even if optimistic locking on
# the model is disabled
#
# @see https://github.com/samvera/valkyrie/wiki/Optimistic-Locking
property :version, virtual: true, prepopulator: LockKeyPopulator

# backs the child work search element;
# @todo: look for a way for the view template not to depend on this
property :find_child_work, default: nil, virtual: true
Expand Down
37 changes: 37 additions & 0 deletions spec/forms/hyrax/forms/resource_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,43 @@
end
end

describe '#version' do
context 'when using wings', valkyrie_adapter: :wings_adapter do
it 'prepopulates as empty before save' do
form.prepopulate!
expect(form.version).to eq ''
end

context 'with a saved work' do
let(:work) { FactoryBot.valkyrie_create(:hyrax_work) }

it 'prepopulates with the etag' do
af_object = Wings::ActiveFedoraConverter.convert(resource: work)

form.prepopulate!
expect(form.version).to eq af_object.etag
end
end
end

context 'when using a generic valkyrie adapter', valkyrie_adapter: :test_adapter do
it 'prepopulates as empty before save' do
form.prepopulate!
expect(form.version).to eq ''
end

context 'with a saved work' do
let(:work) { FactoryBot.valkyrie_create(:hyrax_work) }

it 'prepopulates empty' do
form.prepopulate!

expect(form.version).to eq ''
end
end
end
end

describe '#visibility' do
it 'can set visibility' do
form.visibility = 'open'
Expand Down
8 changes: 2 additions & 6 deletions spec/views/hyrax/base/_form.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
# mock the admin set options presenter to avoid hitting Solr
allow(Hyrax::AdminSetOptionsPresenter).to receive(:new).and_return(options_presenter)
allow(controller).to receive(:current_user).and_return(stub_model(User))
allow(view).to receive(:curation_concern).and_return(work)
assign(:form, form)
allow(controller).to receive(:action_name).and_return(controller_action)
allow(controller).to receive(:repository).and_return(controller_class.new.repository)
Expand Down Expand Up @@ -39,12 +40,8 @@
context 'with an existing object' do
let(:work) { FactoryBot.valkyrie_create(:monograph) }

xit 'renders a form' do
# pending handling for #version/optimistic locking, and maybe other
# issues. a good way to make progress on ChangeSet forms is to enable
# this test, analyze the stacktrace, and try to patch.
it 'renders a form' do
render

expect(rendered).to have_selector("form[action='/concern/monographs/#{work.id}']")
end
end
Expand All @@ -65,7 +62,6 @@
# TODO: stub_model is not stubbing new_record? correctly on ActiveFedora models.
allow(work).to receive(:new_record?).and_return(true)
allow(work).to receive(:member_ids).and_return([1, 2])
allow(view).to receive(:curation_concern).and_return(work)
allow(controller).to receive(:controller_name).and_return('batch_uploads')
allow(form).to receive(:permissions).and_return([])
allow(form).to receive(:visibility).and_return('public')
Expand Down

0 comments on commit 1d66c92

Please sign in to comment.