diff --git a/.rubocop.yml b/.rubocop.yml index 961ef651a..73ec09652 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -39,3 +39,10 @@ Naming/FileName: Exclude: - 'Appraisals' - 'Gemfile' +Metrics/MethodLength: + Exclude: + - 'lib/valkyrie/persistence/postgres/persister.rb' + - 'lib/valkyrie/persistence/fedora/persister.rb' +Metrics/CyclomaticComplexity: + Exclude: + - 'lib/valkyrie/persistence/postgres/persister.rb' diff --git a/lib/valkyrie/persistence/buffered_persister.rb b/lib/valkyrie/persistence/buffered_persister.rb index 5541d6e72..deae6e2cb 100644 --- a/lib/valkyrie/persistence/buffered_persister.rb +++ b/lib/valkyrie/persistence/buffered_persister.rb @@ -24,8 +24,8 @@ def initialize(persister, buffer_class: Valkyrie::Persistence::DeleteTrackingBuf @buffer_class = buffer_class end - def save(resource:) - persister.save(resource: resource) + def save(resource:, external_resource: false) + persister.save(resource: resource, external_resource: external_resource) end def save_all(resources:) diff --git a/lib/valkyrie/persistence/composite_persister.rb b/lib/valkyrie/persistence/composite_persister.rb index 8b5b71f67..e137765bc 100644 --- a/lib/valkyrie/persistence/composite_persister.rb +++ b/lib/valkyrie/persistence/composite_persister.rb @@ -24,14 +24,14 @@ def initialize(*persisters) end # (see Valkyrie::Persistence::Memory::Persister#save) - def save(resource:) + def save(resource:, external_resource: false) # Assume the first persister is the canonical data store; that's the optlock we want first, *rest = *persisters - cached_resource = first.save(resource: resource) + cached_resource = first.save(resource: resource, external_resource: external_resource) # Don't pass opt lock tokens to other persisters internal_resource = cached_resource.dup internal_resource.clear_optimistic_lock_token! - rest.inject(internal_resource) { |m, persister| persister.save(resource: m) } + rest.inject(internal_resource) { |m, persister| persister.save(resource: m, external_resource: true) } # return the one with the desired opt lock token cached_resource end diff --git a/lib/valkyrie/persistence/fedora/persister.rb b/lib/valkyrie/persistence/fedora/persister.rb index 026f59729..fed97d134 100644 --- a/lib/valkyrie/persistence/fedora/persister.rb +++ b/lib/valkyrie/persistence/fedora/persister.rb @@ -13,7 +13,7 @@ def initialize(adapter:) end # (see Valkyrie::Persistence::Memory::Persister#save) - def save(resource:) + def save(resource:, external_resource: false) initialize_repository internal_resource = resource.dup internal_resource.created_at ||= Time.current @@ -35,6 +35,8 @@ def save(resource:) alternate_resources ? save_reference_to_resource(persisted_resource, alternate_resources) : persisted_resource rescue Ldp::PreconditionFailed raise Valkyrie::Persistence::StaleObjectError, "The object #{internal_resource.id} has been updated by another process." + rescue Ldp::Gone + raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." end # (see Valkyrie::Persistence::Memory::Persister#save_all) diff --git a/lib/valkyrie/persistence/memory/persister.rb b/lib/valkyrie/persistence/memory/persister.rb index 594e28a9f..f8754176b 100644 --- a/lib/valkyrie/persistence/memory/persister.rb +++ b/lib/valkyrie/persistence/memory/persister.rb @@ -5,7 +5,7 @@ module Valkyrie::Persistence::Memory # @note Documentation for persisters in general is maintained here. class Persister attr_reader :adapter - delegate :cache, to: :adapter + delegate :cache, :query_service, to: :adapter # @param adapter [Valkyrie::Persistence::Memory::MetadataAdapter] The memory adapter which # holds the cache for this persister. @@ -16,11 +16,18 @@ def initialize(adapter) # Save a single resource. # @param resource [Valkyrie::Resource] The resource to save. + # @param external_resource [Boolean] Whether the resource to be saved comes + # from a different metadata store. Allows a resource to be saved even if + # it's not already in the store. For example, if you're indexing a + # resource into Solr - it's saved in your primary metadata store, but not + # in Solr, so it's okay if it doesn't exist in Solr but is marked as + # persisted. # @return [Valkyrie::Resource] The resource with an `#id` value generated by the # persistence backend. # @raise [Valkyrie::Persistence::StaleObjectError] - def save(resource:) + def save(resource:, external_resource: false) raise Valkyrie::Persistence::StaleObjectError, "The object #{resource.id} has been updated by another process." unless valid_lock?(resource) + raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." unless external_resource || valid_for_save?(resource) # duplicate the resource so we are not creating side effects on the caller's resource internal_resource = resource.dup @@ -34,6 +41,15 @@ def save(resource:) cache[internal_resource.id] = internal_resource end + # return true if resource is + # persisted and found + # or + # not persisted + def valid_for_save?(resource) + return true unless resource.persisted? # a new resource + query_service.find_by(id: resource.id).present? # a persisted resource must be found + end + # Save a batch of resources. # @param resources [Array] List of resources to save. # @return [Array] List of resources with an `#id` value diff --git a/lib/valkyrie/persistence/postgres/persister.rb b/lib/valkyrie/persistence/postgres/persister.rb index eb218794c..f64f2e51a 100644 --- a/lib/valkyrie/persistence/postgres/persister.rb +++ b/lib/valkyrie/persistence/postgres/persister.rb @@ -17,9 +17,12 @@ def initialize(adapter:) # @return [Valkyrie::Resource] the persisted/updated resource # @raise [Valkyrie::Persistence::StaleObjectError] raised if the resource # was modified in the database between been read into memory and persisted - def save(resource:) + def save(resource:, external_resource: false) orm_object = resource_factory.from_resource(resource: resource) orm_object.transaction do + if !external_resource && resource.persisted? && !orm_object.persisted? + raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." + end orm_object.save! if resource.id && resource.id.to_s != orm_object.id raise Valkyrie::Persistence::UnsupportedDatatype, diff --git a/lib/valkyrie/persistence/solr/metadata_adapter.rb b/lib/valkyrie/persistence/solr/metadata_adapter.rb index c17631b38..5c44f783d 100644 --- a/lib/valkyrie/persistence/solr/metadata_adapter.rb +++ b/lib/valkyrie/persistence/solr/metadata_adapter.rb @@ -39,7 +39,7 @@ def initialize(connection:, resource_indexer: NullIndexer, write_only: false) # @return [Valkyrie::Persistence::Solr::Persister] The solr persister. def persister - Valkyrie::Persistence::Solr::Persister.new(adapter: self) + @persister ||= Valkyrie::Persistence::Solr::Persister.new(adapter: self) end def write_only? diff --git a/lib/valkyrie/persistence/solr/persister.rb b/lib/valkyrie/persistence/solr/persister.rb index b76e2edb9..fbdc638e1 100644 --- a/lib/valkyrie/persistence/solr/persister.rb +++ b/lib/valkyrie/persistence/solr/persister.rb @@ -6,7 +6,7 @@ module Valkyrie::Persistence::Solr # Most methods are delegated to {Valkyrie::Persistence::Solr::Repository} class Persister attr_reader :adapter - delegate :connection, :resource_factory, :write_only?, to: :adapter + delegate :connection, :query_service, :resource_factory, :write_only?, to: :adapter # @param adapter [Valkyrie::Persistence::Solr::MetadataAdapter] The adapter with the # configured solr connection. @@ -16,14 +16,20 @@ def initialize(adapter:) # (see Valkyrie::Persistence::Memory::Persister#save) # @return [Boolean] If write_only, whether saving succeeded. - def save(resource:) + def save(resource:, external_resource: false) if write_only? repository([resource]).persist else + raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." unless external_resource || valid_for_save?(resource) repository([resource]).persist.first end end + def valid_for_save?(resource) + return true unless resource.persisted? # a new resource + query_service.find_by(id: resource.id).present? # a persisted resource must be found + end + # (see Valkyrie::Persistence::Memory::Persister#save_all) # @return [Boolean] If write_only, whether saving succeeded. def save_all(resources:) diff --git a/lib/valkyrie/specs/shared_specs/persister.rb b/lib/valkyrie/specs/shared_specs/persister.rb index f82af60d7..fbb2dbb8f 100644 --- a/lib/valkyrie/specs/shared_specs/persister.rb +++ b/lib/valkyrie/specs/shared_specs/persister.rb @@ -50,6 +50,24 @@ class CustomResource < Valkyrie::Resource expect(reloaded.nested_resource.first.title).to eq ["Nested"] end + context "when a persisted resource is not in the database" do + it "throws an ObjectNotFoundError" do + expect(resource).not_to be_persisted + saved = persister.save(resource: resource) + + expect(saved).to be_persisted + persister.delete(resource: saved) + + expect { persister.save(resource: saved) }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + end + it "is okay if it's from another persister" do + memory_adapter = Valkyrie::Persistence::Memory::MetadataAdapter.new + saved = memory_adapter.persister.save(resource: resource) + + expect { persister.save(resource: saved, external_resource: true) }.not_to raise_error + end + end + it "can persist single values" do resource.single_value = "A single value" diff --git a/spec/valkyrie/persistence/buffered_persister_spec.rb b/spec/valkyrie/persistence/buffered_persister_spec.rb index fd4de90ac..46dde9d17 100644 --- a/spec/valkyrie/persistence/buffered_persister_spec.rb +++ b/spec/valkyrie/persistence/buffered_persister_spec.rb @@ -30,5 +30,15 @@ class Resource < Valkyrie::Resource end expect(buffer.query_service.find_all.length).to eq 1 end + it "can handle saving an object which was previously saved" do + buffer = nil + resource = Resource.new + output = adapter.persister.save(resource: resource) + persister.with_buffer do |inner_persister, memory_buffer| + inner_persister.save(resource: output) + buffer = memory_buffer + end + expect(buffer.query_service.find_all.length).to eq 1 + end end end diff --git a/spec/valkyrie/persistence/composite_persister_spec.rb b/spec/valkyrie/persistence/composite_persister_spec.rb index aa1a948b1..fba6c973e 100644 --- a/spec/valkyrie/persistence/composite_persister_spec.rb +++ b/spec/valkyrie/persistence/composite_persister_spec.rb @@ -17,10 +17,13 @@ let(:client) { RSolr.connect(url: SOLR_TEST_URL) } let(:persister) do described_class.new( - Valkyrie::Persistence::Postgres::MetadataAdapter.new.persister, + postgres_adapter.persister, adapter.persister ) end + let(:postgres_adapter) do + Valkyrie::Persistence::Postgres::MetadataAdapter.new + end let(:adapter) { Valkyrie::Persistence::Solr::MetadataAdapter.new(connection: client) } before do @@ -35,5 +38,9 @@ class CustomResource < Valkyrie::Resource book = persister.save(resource: CustomResource.new) expect { query_service.find_by(id: book.id) }.not_to raise_error(Valkyrie::Persistence::ObjectNotFoundError) end + it "can save in postgres and then index freshly into solr" do + book = postgres_adapter.persister.save(resource: CustomResource.new) + expect { persister.save(resource: book) }.not_to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + end end end