diff --git a/db/migrate/20190415200643_add_default_to_lock_version.rb b/db/migrate/20190415200643_add_default_to_lock_version.rb new file mode 100644 index 000000000..2c6aee783 --- /dev/null +++ b/db/migrate/20190415200643_add_default_to_lock_version.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +class AddDefaultToLockVersion < ActiveRecord::Migration[5.1] + def change + change_column_default :orm_resources, :lock_version, from: nil, to: 0 + end +end diff --git a/lib/valkyrie/persistence/fedora/persister/model_converter.rb b/lib/valkyrie/persistence/fedora/persister/model_converter.rb index 085a8fdf4..7c0baad9a 100644 --- a/lib/valkyrie/persistence/fedora/persister/model_converter.rb +++ b/lib/valkyrie/persistence/fedora/persister/model_converter.rb @@ -36,7 +36,15 @@ def properties resource_attributes.keys - [:new_record] end - delegate :attributes, to: :resource, prefix: true + # Always generate a token, in case it's enabled later. + def resource_attributes + @resource_attributes ||= + begin + attributes = resource.attributes.dup + attributes[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK] ||= [Valkyrie::Persistence::OptimisticLockToken.new(adapter_id: adapter.id, token: Time.now.to_r)] + attributes + end + end # Construct the LDP Basic Container modeling the Valkyrie Resource in Fedora # @see https://www.w3.org/TR/ldp/#ldpc diff --git a/lib/valkyrie/persistence/fedora/persister/orm_converter.rb b/lib/valkyrie/persistence/fedora/persister/orm_converter.rb index e734d76bb..be27a102b 100644 --- a/lib/valkyrie/persistence/fedora/persister/orm_converter.rb +++ b/lib/valkyrie/persistence/fedora/persister/orm_converter.rb @@ -42,7 +42,7 @@ def populate_native_lock(resource) return resource unless lastmod token = Valkyrie::Persistence::OptimisticLockToken.new(adapter_id: "native-#{adapter.id}", token: DateTime.parse(lastmod.to_s).httpdate) - resource.send(Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK) << token + resource.set_value(Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK, resource[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK] + [token]) resource end diff --git a/lib/valkyrie/persistence/memory/metadata_adapter.rb b/lib/valkyrie/persistence/memory/metadata_adapter.rb index 1609269cc..fcdcaae41 100644 --- a/lib/valkyrie/persistence/memory/metadata_adapter.rb +++ b/lib/valkyrie/persistence/memory/metadata_adapter.rb @@ -27,7 +27,7 @@ def cache # @return [Valkyrie::ID] Identifier for this metadata adapter. def id - @id ||= Valkyrie::ID.new(Digest::MD5.hexdigest(self.class.to_s)) + @id ||= Valkyrie::ID.new(Digest::MD5.hexdigest(self.class.to_s + SecureRandom.uuid)) end end end diff --git a/lib/valkyrie/persistence/memory/persister.rb b/lib/valkyrie/persistence/memory/persister.rb index b4c88c400..7b591ac02 100644 --- a/lib/valkyrie/persistence/memory/persister.rb +++ b/lib/valkyrie/persistence/memory/persister.rb @@ -84,8 +84,10 @@ def normalize_date_value(value) # Create a new lock token based on the current timestamp. def generate_lock_token(resource) - return unless resource.optimistic_locking_enabled? token = Valkyrie::Persistence::OptimisticLockToken.new(adapter_id: adapter.id, token: Time.now.to_r) + cache[:versions] ||= {} + cache[:versions][resource.id] = token + return unless resource.optimistic_locking_enabled? resource.set_value(Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK, token) end @@ -96,11 +98,12 @@ def valid_lock?(resource) cached_resource = cache[resource.id] return true if cached_resource.blank? - resource_lock_tokens = resource[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK] + resource_lock_tokens = resource[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK] || [] resource_value = resource_lock_tokens.find { |lock_token| lock_token.adapter_id == adapter.id } - return true if resource_value.blank? + cached_token = cached_resource[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK] || [] + cached_value = cached_token.find { |lock_token| lock_token.adapter_id == adapter.id } + return true if resource_value.nil? - cached_value = cached_resource[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK].first cached_value == resource_value end end diff --git a/lib/valkyrie/persistence/memory/query_service.rb b/lib/valkyrie/persistence/memory/query_service.rb index 5cba044f6..86ea473be 100644 --- a/lib/valkyrie/persistence/memory/query_service.rb +++ b/lib/valkyrie/persistence/memory/query_service.rb @@ -24,7 +24,14 @@ def initialize(adapter:) def find_by(id:) id = Valkyrie::ID.new(id.to_s) if id.is_a?(String) validate_id(id) - cache[id] || raise(::Valkyrie::Persistence::ObjectNotFoundError) + fetch_from_cache(id) || raise(::Valkyrie::Persistence::ObjectNotFoundError) + end + + def fetch_from_cache(id) + result = cache[id] + return result if result.nil? || !result.optimistic_locking_enabled? || result[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK].present? + result.set_value(Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK, cache[:versions][id]) + result end # Get a single resource by `alternate_identifier`. Alternate identifiers are identifiers (like NOIDs, @@ -62,7 +69,9 @@ def find_many_by_ids(ids:) # Get all objects. # @return [Array] All objects in the persistence backend. def find_all - cache.values + cache.except(:versions).values.map do |resource| + fetch_from_cache(resource.id) + end end # Get all objects of a given model. @@ -70,7 +79,7 @@ def find_all # @return [Array] All objects in the persistence backend # with the given class. def find_all_of_model(model:) - cache.values.select do |obj| + find_all.select do |obj| obj.is_a?(model) end end diff --git a/lib/valkyrie/persistence/postgres/orm_converter.rb b/lib/valkyrie/persistence/postgres/orm_converter.rb index 8814c0c12..270919e1c 100644 --- a/lib/valkyrie/persistence/postgres/orm_converter.rb +++ b/lib/valkyrie/persistence/postgres/orm_converter.rb @@ -35,7 +35,7 @@ def resource # Construct the optimistic lock token using the adapter and lock version for the Resource # @return [Valkyrie::Persistence::OptimisticLockToken] def lock_token - return lock_token_warning unless orm_object.class.column_names.include?("lock_version") + return lock_token_warning unless orm_object.class.column_names.include?("lock_version") && orm_object.class.columns.find { |x| x.name == "lock_version" }.default == "0" @lock_token ||= Valkyrie::Persistence::OptimisticLockToken.new( adapter_id: resource_factory.adapter_id, diff --git a/lib/valkyrie/specs/shared_specs/persister.rb b/lib/valkyrie/specs/shared_specs/persister.rb index c83f5e448..f1c8c3338 100644 --- a/lib/valkyrie/specs/shared_specs/persister.rb +++ b/lib/valkyrie/specs/shared_specs/persister.rb @@ -313,6 +313,33 @@ class CustomResource < Valkyrie::Resource expect(query_service.find_all.to_a.length).to eq 0 end + context "optimistic locking enabled later" do + before do + class MyLockingResource < Valkyrie::Resource + attribute :title + end + end + describe "#save" do + it "will error appropriately if optimistic locking is enabled later" do + resource = MyLockingResource.new + saved_resource = persister.save(resource: resource) + + MyLockingResource.enable_optimistic_locking + + saved_resource = query_service.find_by(id: saved_resource.id) + reloaded = query_service.find_by(id: saved_resource.id) + saved_resource.title = "Don't Save Me" + reloaded.title = "Save Me" + + persister.save(resource: reloaded) + expect { persister.save(resource: saved_resource) }.to raise_error Valkyrie::Persistence::StaleObjectError + end + end + after do + Object.send(:remove_const, :MyLockingResource) + end + end + context "optimistic locking" do before do class MyLockingResource < Valkyrie::Resource diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 8f3026a11..bd632fdaf 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -24,6 +24,7 @@ WebMock.disable_net_connect!(allow_localhost: true) RSpec.configure do |config| + config.example_status_persistence_file_path = "tmp/rspec_examples.txt" config.order = :random Kernel.srand config.seed diff --git a/spec/valkyrie/persistence/memory/metadata_adapter_spec.rb b/spec/valkyrie/persistence/memory/metadata_adapter_spec.rb index b325dee79..a847ade72 100644 --- a/spec/valkyrie/persistence/memory/metadata_adapter_spec.rb +++ b/spec/valkyrie/persistence/memory/metadata_adapter_spec.rb @@ -7,9 +7,8 @@ it_behaves_like "a Valkyrie::MetadataAdapter" describe "#id" do - it "creates an md5 hash from the class name" do - expected = Digest::MD5.hexdigest described_class.to_s - expect(adapter.id.to_s).to eq expected + it "creates a unique identifier for each instance" do + expect(adapter.id.to_s).not_to eq described_class.new.id.to_s end end end diff --git a/spec/valkyrie/persistence/postgres/persister_spec.rb b/spec/valkyrie/persistence/postgres/persister_spec.rb index 392cefd62..98a3020d0 100644 --- a/spec/valkyrie/persistence/postgres/persister_spec.rb +++ b/spec/valkyrie/persistence/postgres/persister_spec.rb @@ -131,6 +131,15 @@ class CustomResource < Valkyrie::Resource expect { adapter.persister.save(resource: resource) }.to output(/\[MIGRATION REQUIRED\]/).to_stderr end end + context "and only some of the migrations have been run" do + before do + allow(adapter.resource_factory.orm_class.columns.find { |x| x.name == "lock_version" }).to receive(:default).and_return(nil) + end + it "loads the object, but sends a warning with instructions" do + resource = MyLockingResource.new + expect { adapter.persister.save(resource: resource) }.to output(/\[MIGRATION REQUIRED\]/).to_stderr + end + end context "and locking isn't enabled" do it "doesn't use the lock" do resource = CustomResource.new