diff --git a/app/factories/bulkrax/object_factory.rb b/app/factories/bulkrax/object_factory.rb index 8aa26133..648bffe7 100644 --- a/app/factories/bulkrax/object_factory.rb +++ b/app/factories/bulkrax/object_factory.rb @@ -2,10 +2,42 @@ module Bulkrax class ObjectFactory # rubocop:disable Metrics/ClassLength + include ObjectFactoryInterface + extend ActiveModel::Callbacks include Bulkrax::FileFactory include DynamicRecordLookup + ## + # @!group Class Method Interface + # + # @see Bulkrax::ObjectFactoryInterface + def self.find(id) + ActiveFedora::Base.find(id) + rescue ActiveFedora::ObjectNotFoundError => e + raise ObjectFactoryInterface::ObjectNotFoundError, e.message + end + + def self.query(q, **kwargs) + ActiveFedora::SolrService.query(q, **kwargs) + end + + def self.clean! + super do + ActiveFedora::Cleaner.clean! + end + end + + def self.solr_name(field_name) + if defined?(Hyrax) + Hyrax.index_field_mapper.solr_name(field_name) + else + ActiveFedora.index_field_mapper.solr_name(field_name) + end + end + # @!endgroup Class Method Interface + ## + # @api private # # These are the attributes that we assume all "work type" classes (e.g. the given :klass) will @@ -66,7 +98,7 @@ def run def run! self.run # Create the error exception if the object is not validly saved for some reason - raise ActiveFedora::RecordInvalid, object if !object.persisted? || object.changed? + raise ObjectFactoryInterface::RecordInvalid, object if !object.persisted? || object.changed? object end @@ -95,7 +127,6 @@ def find end def find_by_id - # TODO: Push logic into Bulkrax.persistence_adapter; maybe # Rails / Ruby upgrade, we moved from :exists? to :exist? However we want to continue (for a # bit) to support older versions. method_name = klass.respond_to?(:exist?) ? :exist? : :exists? @@ -111,7 +142,6 @@ def find_or_create end def search_by_identifier - # TODO: Push logic into Bulkrax.persistence_adapter; maybe query = { work_identifier_search_field => source_identifier_value } # Query can return partial matches (something6 matches both something6 and something68) diff --git a/lib/bulkrax/persistence_layer.rb b/app/factories/bulkrax/object_factory_interface.rb similarity index 63% rename from lib/bulkrax/persistence_layer.rb rename to app/factories/bulkrax/object_factory_interface.rb index 361e72e4..72e58e93 100644 --- a/lib/bulkrax/persistence_layer.rb +++ b/app/factories/bulkrax/object_factory_interface.rb @@ -2,8 +2,15 @@ module Bulkrax ## - # The target data layer where we write and read our imported {Bulkrax::Entry} objects. - module PersistenceLayer + # A module that helps define the expected interface for object factory interactions. + # + # The abstract class methods are useful for querying the underlying persistence layer when you are + # not in the context of an instance of an {Bulkrax::ObjectFactory} and therefore don't have access + # to it's {#find} instance method. + # + # @abstract + module ObjectFactoryInterface + extend ActiveSupport::Concern # We're inheriting from an ActiveRecord exception as that is something we know will be here; and # something that the main_app will be expect to be able to handle. class ObjectNotFoundError < ActiveRecord::RecordNotFound @@ -14,23 +21,24 @@ class ObjectNotFoundError < ActiveRecord::RecordNotFound class RecordInvalid < ActiveRecord::RecordInvalid end - class AbstractAdapter + class_methods do + ## # @see ActiveFedora::Base.find - def self.find(id) + def find(id) raise NotImplementedError, "#{self}.#{__method__}" end - def self.solr_name(field_name) + def solr_name(field_name) raise NotImplementedError, "#{self}.#{__method__}" end # @yield when Rails application is running in test environment. - def self.clean! + def clean! return true unless Rails.env.test? yield end - def self.query(q, **kwargs) + def query(q, **kwargs) raise NotImplementedError, "#{self}.#{__method__}" end end diff --git a/app/factories/bulkrax/valkyrie_object_factory.rb b/app/factories/bulkrax/valkyrie_object_factory.rb index b6a504a3..939ae56f 100644 --- a/app/factories/bulkrax/valkyrie_object_factory.rb +++ b/app/factories/bulkrax/valkyrie_object_factory.rb @@ -1,7 +1,41 @@ # frozen_string_literal: true module Bulkrax + # rubocop:disable Metrics/ClassLength class ValkyrieObjectFactory < ObjectFactory + include ObjectFactoryInterface + + def self.find(id) + if defined?(Hyrax) + begin + Hyrax.query_service.find_by(id: id) + # Because Hyrax is not a hard dependency, we need to transform the Hyrax exception into a + # common exception so that callers can handle a generalize exception. + rescue Hyrax::ObjectNotFoundError => e + raise ObjectFactoryInterface::ObjectNotFoundError, e.message + end + else + # NOTE: Fair warning; you might might need a custom query for find by alternate id. + Valkyrie.query_service.find_by(id: id) + end + rescue Valkyrie::Persistence::ObjectNotFoundError => e + raise ObjectFactoryInterface::ObjectNotFoundError, e.message + end + + def self.solr_name(field_name) + # It's a bit unclear what this should be if we can't rely on Hyrax. + # TODO: Downstream implementers will need to figure this out. + raise NotImplementedError, "#{self}.#{__method__}" unless defined?(Hyrax) + Hyrax.index_field_mapper.solr_name(field_name) + end + + def self.query(q, **kwargs) + # TODO: Without the Hyrax::QueryService, what are we left with? Someone could choose + # ActiveFedora::QueryService. + raise NotImplementedError, "#{self}.#{__method__}" unless defined?(Hyrax) + Hyrax::QueryService.query(q, **kwargs) + end + ## # Retrieve properties from M3 model # @param klass the model @@ -19,7 +53,7 @@ def run! run return object if object.persisted? - raise(RecordInvalid, object) + raise(ObjectFactoryInterface::RecordInvalid, object) end def find_by_id @@ -178,7 +212,5 @@ def fetch_child_file_sets(resource:) Hyrax.custom_queries.find_child_file_sets(resource: resource) end end - - class RecordInvalid < StandardError - end + # rubocop:enable Metrics/ClassLength end diff --git a/app/helpers/bulkrax/validation_helper.rb b/app/helpers/bulkrax/validation_helper.rb index da66887a..4139da76 100644 --- a/app/helpers/bulkrax/validation_helper.rb +++ b/app/helpers/bulkrax/validation_helper.rb @@ -25,7 +25,7 @@ def check_admin_set AdminSet.find(params[:importer][:admin_set_id]) end return true - rescue ActiveFedora::ObjectNotFoundError, Bulkrax::PersistenceLayer::ObjectNotFoundError + rescue ActiveFedora::ObjectNotFoundError, Bulkrax::ObjectFactoryInterface::ObjectNotFoundError logger.warn("AdminSet #{params[:importer][:admin_set_id]} not found. Using default admin set.") params[:importer][:admin_set_id] = AdminSet::DEFAULT_ID return true diff --git a/app/jobs/bulkrax/create_relationships_job.rb b/app/jobs/bulkrax/create_relationships_job.rb index d6bc29c1..cc954947 100644 --- a/app/jobs/bulkrax/create_relationships_job.rb +++ b/app/jobs/bulkrax/create_relationships_job.rb @@ -80,7 +80,7 @@ def perform(parent_identifier:, importer_run_id:) # rubocop:disable Metrics/AbcS # save record if members were added if @parent_record_members_added parent_record.save! - # TODO: Push logic into Bulkrax.persistence_adapter + # TODO: Push logic into Bulkrax.object_factory # Ensure that the new relationship gets indexed onto the children if parent_record.is_a?(Valkyrie::Resource) @child_members_added.each do |child| diff --git a/app/models/concerns/bulkrax/dynamic_record_lookup.rb b/app/models/concerns/bulkrax/dynamic_record_lookup.rb index 27cc53cb..9f81a506 100644 --- a/app/models/concerns/bulkrax/dynamic_record_lookup.rb +++ b/app/models/concerns/bulkrax/dynamic_record_lookup.rb @@ -18,9 +18,9 @@ def find_record(identifier, importer_run_id = nil) begin # the identifier parameter can be a :source_identifier or the id of an object record = Entry.find_by(default_scope.merge({ importerexporter_id: importer_id })) || Entry.find_by(default_scope) - record ||= Bulkrax.persistence_adapter.find(identifier) + record ||= Bulkrax.object_factory.find(identifier) # NameError for if ActiveFedora isn't installed - rescue NameError, ActiveFedora::ObjectNotFoundError, Bulkrax::PersistenceLayer::ObjectNotFoundError + rescue NameError, ActiveFedora::ObjectNotFoundError, Bulkrax::OjbectFactoryInterface::ObjectNotFoundError record = nil end diff --git a/app/models/concerns/bulkrax/export_behavior.rb b/app/models/concerns/bulkrax/export_behavior.rb index 16994eb3..41b262e3 100644 --- a/app/models/concerns/bulkrax/export_behavior.rb +++ b/app/models/concerns/bulkrax/export_behavior.rb @@ -22,7 +22,7 @@ def build_export_metadata end def hyrax_record - @hyrax_record ||= Bulkrax.persistence_adapter.find(self.identifier) + @hyrax_record ||= Bulkrax.object_factory.find(self.identifier) end # Prepend the file_set id to ensure a unique filename and also one that is not longer than 255 characters diff --git a/app/parsers/bulkrax/bagit_parser.rb b/app/parsers/bulkrax/bagit_parser.rb index 7485e9d3..29b2f580 100644 --- a/app/parsers/bulkrax/bagit_parser.rb +++ b/app/parsers/bulkrax/bagit_parser.rb @@ -100,7 +100,7 @@ def write_files file_set_entries = importerexporter.entries.where(type: file_set_entry_class.to_s) work_entries[0..limit || total].each do |entry| - record = Bulkrax.persistence_adapter.find(entry.identifier) + record = Bulkrax.object_factory.find(entry.identifier) next unless record bag_entries = [entry] diff --git a/app/parsers/bulkrax/csv_parser.rb b/app/parsers/bulkrax/csv_parser.rb index c8fc89a2..f93fe32f 100644 --- a/app/parsers/bulkrax/csv_parser.rb +++ b/app/parsers/bulkrax/csv_parser.rb @@ -286,7 +286,7 @@ def write_files end def store_files(identifier, folder_count) - record = Bulkrax.persistence_adapter.find(identifier) + record = Bulkrax.object_factory.find(identifier) return unless record file_sets = record.file_set? ? Array.wrap(record) : record.file_sets diff --git a/app/parsers/bulkrax/parser_export_record_set.rb b/app/parsers/bulkrax/parser_export_record_set.rb index 86ffa252..369a1136 100644 --- a/app/parsers/bulkrax/parser_export_record_set.rb +++ b/app/parsers/bulkrax/parser_export_record_set.rb @@ -149,12 +149,12 @@ def extra_filters end def works - @works ||= Bulkrax.persistence_adapter.query(works_query, **works_query_kwargs) + @works ||= Bulkrax.object_factory.query(works_query, **works_query_kwargs) end def collections @collections ||= if collections_query - Bulkrax.persistence_adapter.query(collections_query, **collections_query_kwargs) + Bulkrax.object_factory.query(collections_query, **collections_query_kwargs) else [] end @@ -175,7 +175,7 @@ def file_sets @file_sets ||= ParserExportRecordSet.in_batches(candidate_file_set_ids) do |batch_of_ids| fsq = "has_model_ssim:#{Bulkrax.file_model_class} AND id:(\"" + batch_of_ids.join('" OR "') + "\")" fsq += extra_filters if extra_filters.present? - Bulkrax.persistence_adapter.query( + Bulkrax.object_factory.query( fsq, { fl: "id", method: :post, rows: batch_of_ids.size } ) @@ -183,7 +183,7 @@ def file_sets end def solr_name(base_name) - Bulkrax.persistence_adapter.solr_name(base_name) + Bulkrax.object_factory.solr_name(base_name) end end @@ -243,7 +243,7 @@ def complete_entry_identifiers def works @works ||= ParserExportRecordSet.in_batches(complete_entry_identifiers) do |ids| - Bulkrax.persistence_adapter.query( + Bulkrax.object_factory.query( extra_filters.to_s, **query_kwargs.merge( fq: [ @@ -258,7 +258,7 @@ def works def collections @collections ||= ParserExportRecordSet.in_batches(complete_entry_identifiers) do |ids| - Bulkrax.persistence_adapter.query( + Bulkrax.object_factory.query( "has_model_ssim:Collection #{extra_filters}", **query_kwargs.merge( fq: [ @@ -277,7 +277,7 @@ def collections # @see Bulkrax::ParserExportRecordSet::Base#file_sets def file_sets @file_sets ||= ParserExportRecordSet.in_batches(complete_entry_identifiers) do |ids| - Bulkrax.persistence_adapter.query( + Bulkrax.object_factory.query( extra_filters, query_kwargs.merge( fq: [ diff --git a/lib/bulkrax.rb b/lib/bulkrax.rb index e88f608b..4a017b45 100644 --- a/lib/bulkrax.rb +++ b/lib/bulkrax.rb @@ -39,10 +39,6 @@ class Configuration # second parameter is an Integer for the index of the record encountered in the import. attr_accessor :fill_in_blank_source_identifiers - ## - # @param adapter [Class] - attr_writer :persistence_adapter - ## # @param coercer [#call] # @see Bulkrax::FactoryClassFinder @@ -62,34 +58,6 @@ def factory_class_name_coercer @factory_class_name_coercer || Bulkrax::FactoryClassFinder::DefaultCoercer end - ## - # Configure the persistence adapter used for persisting imported data. - # - # @return [Class] - # @see Bulkrax::PersistenceLayer - def persistence_adapter - @persistence_adapter || derived_persistence_adapter - end - - def derived_persistence_adapter - if defined?(Hyrax) - # There's probably some configuration of Hyrax we could use to better refine this; but it's - # likely a reasonable guess. The main goal is to not break existing implementations and - # maintain an upgrade path. - if Gem::Version.new(Hyrax::VERSION) >= Gem::Version.new('6.0.0') - Bulkrax::PersistenceLayer::ValkyrieAdapter - else - Bulkrax::PersistenceLayer::ActiveFedoraAdapter - end - elsif defined?(ActiveFedora) - Bulkrax::PersistenceLayer::ActiveFedoraAdapter - elsif defined?(Valkyrie) - Bulkrax::PersistenceLayer::ValkyrieAdapter - else - raise "Unable to derive a persistence adapter" - end - end - attr_writer :use_locking def use_locking @@ -138,8 +106,6 @@ def config :object_factory=, :parsers, :parsers=, - :persistence_adapter, - :persistence_adapter=, :qa_controlled_properties, :qa_controlled_properties=, :related_children_field_mapping, diff --git a/lib/bulkrax/engine.rb b/lib/bulkrax/engine.rb index 85eb11cf..d74da149 100644 --- a/lib/bulkrax/engine.rb +++ b/lib/bulkrax/engine.rb @@ -17,9 +17,6 @@ class Engine < ::Rails::Engine end initializer 'requires' do - require 'bulkrax/persistence_layer' - require 'bulkrax/persistence_layer/active_fedora_adapter' if defined?(ActiveFedora) - require 'bulkrax/persistence_layer/valkyrie_adapter' if defined?(Valkyrie) require 'bulkrax/transactions' if defined?(Hyrax::Transactions) end diff --git a/lib/bulkrax/persistence_layer/active_fedora_adapter.rb b/lib/bulkrax/persistence_layer/active_fedora_adapter.rb deleted file mode 100644 index 884dc372..00000000 --- a/lib/bulkrax/persistence_layer/active_fedora_adapter.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module Bulkrax - module PersistenceLayer - class ActiveFedoraAdapter < AbstractAdapter - def self.find(id) - ActiveFedora::Base.find(id) - rescue ActiveFedora::ObjectNotFoundError => e - raise PersistenceLayer::ObjectNotFoundError, e.message - end - - def self.query(q, **kwargs) - ActiveFedora::SolrService.query(q, **kwargs) - end - - def self.clean! - super do - ActiveFedora::Cleaner.clean! - end - end - - def self.solr_name(field_name) - if Module.const_defined?(:Solrizer) - ::Solrizer.solr_name(base_name) - else - ActiveFedora.index_field_mapper.solr_name(field_name) - end - end - end - end -end diff --git a/lib/bulkrax/persistence_layer/valkyrie_adapter.rb b/lib/bulkrax/persistence_layer/valkyrie_adapter.rb deleted file mode 100644 index cfa334bb..00000000 --- a/lib/bulkrax/persistence_layer/valkyrie_adapter.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -module Bulkrax - module PersistenceLayer - class ValkyrieAdapter < AbstractAdapter - end - end -end diff --git a/spec/bulkrax_spec.rb b/spec/bulkrax_spec.rb index e66e3fa7..41d76d41 100644 --- a/spec/bulkrax_spec.rb +++ b/spec/bulkrax_spec.rb @@ -198,14 +198,6 @@ end end - context '.persistence_adapter' do - subject { described_class.persistence_adapter } - it { is_expected.to respond_to(:find) } - it { is_expected.to respond_to(:query) } - it { is_expected.to respond_to(:solr_name) } - it { is_expected.to respond_to(:clean!) } - end - context '.factory_class_name_coercer' do subject { described_class.factory_class_name_coercer } diff --git a/spec/parsers/bulkrax/bagit_parser_spec.rb b/spec/parsers/bulkrax/bagit_parser_spec.rb index 5fb0d765..6c73778f 100644 --- a/spec/parsers/bulkrax/bagit_parser_spec.rb +++ b/spec/parsers/bulkrax/bagit_parser_spec.rb @@ -288,12 +288,12 @@ module Bulkrax let(:fileset_entry_2) { FactoryBot.create(:bulkrax_csv_entry_file_set, importerexporter: exporter) } before do - allow(Bulkrax.persistence_adapter).to receive(:query).and_return(work_ids_solr) + allow(Bulkrax.object_factory).to receive(:query).and_return(work_ids_solr) allow(exporter.entries).to receive(:where).and_return([work_entry_1, work_entry_2, fileset_entry_1, fileset_entry_2]) end it 'attempts to find the related record' do - expect(Bulkrax.persistence_adapter).to receive(:find).with('csv_entry').and_return(nil) + expect(Bulkrax.object_factory).to receive(:find).with('csv_entry').and_return(nil) subject.write_files end diff --git a/spec/parsers/bulkrax/csv_parser_spec.rb b/spec/parsers/bulkrax/csv_parser_spec.rb index eb0e4725..29dfa516 100644 --- a/spec/parsers/bulkrax/csv_parser_spec.rb +++ b/spec/parsers/bulkrax/csv_parser_spec.rb @@ -633,7 +633,7 @@ module Bulkrax end before do - allow(Bulkrax.persistence_adapter).to receive(:query).and_return(SolrDocument.new(id: work_id)) + allow(Bulkrax.object_factory).to receive(:query).and_return(SolrDocument.new(id: work_id)) allow(exporter.entries).to receive(:where).and_return([entry]) allow(parser).to receive(:headers).and_return(entry.parsed_metadata.keys) end diff --git a/spec/support/dynamic_record_lookup.rb b/spec/support/dynamic_record_lookup.rb index 2c03d2c2..342093ac 100644 --- a/spec/support/dynamic_record_lookup.rb +++ b/spec/support/dynamic_record_lookup.rb @@ -10,7 +10,7 @@ module Bulkrax allow(::Hyrax.config).to receive(:curation_concerns).and_return([Work]) # DRY spec setup -- by default, assume #find_record doesn't find anything allow(Entry).to receive(:find_by).and_return(nil) - allow(Bulkrax.persistence_adapter).to receive(:find).and_return(nil) + allow(Bulkrax.object_factory).to receive(:find).and_return(nil) end describe '#find_record' do @@ -19,7 +19,7 @@ module Bulkrax it 'looks through entries and all work types' do expect(Entry).to receive(:find_by).with({ identifier: source_identifier, importerexporter_type: 'Bulkrax::Importer', importerexporter_id: importer_id }).once - expect(Bulkrax.persistence_adapter).to receive(:find).with(source_identifier).once.and_return(Bulkrax::PersistenceLayer::ObjectNotFoundError) + expect(Bulkrax.object_factory).to receive(:find).with(source_identifier).once.and_return(Bulkrax::ObjectFactoryInterface::ObjectNotFoundError) subject.find_record(source_identifier, importer_run_id) end @@ -61,7 +61,7 @@ module Bulkrax it 'looks through entries and all work types' do expect(Entry).to receive(:find_by).with({ identifier: id, importerexporter_type: 'Bulkrax::Importer', importerexporter_id: importer_id }).once - expect(Bulkrax.persistence_adapter).to receive(:find).with(id).once.and_return(nil) + expect(Bulkrax.object_factory).to receive(:find).with(id).once.and_return(nil) subject.find_record(id, importer_run_id) end @@ -70,7 +70,7 @@ module Bulkrax let(:collection) { instance_double(::Collection) } before do - allow(Bulkrax.persistence_adapter).to receive(:find).with(id).and_return(collection) + allow(Bulkrax.object_factory).to receive(:find).with(id).and_return(collection) end it 'returns the collection' do @@ -82,7 +82,7 @@ module Bulkrax let(:work) { instance_double(::Work) } before do - allow(Bulkrax.persistence_adapter).to receive(:find).with(id).and_return(work) + allow(Bulkrax.object_factory).to receive(:find).with(id).and_return(work) end it 'returns the work' do