♻️ Migrate persistence layer methods to object factory (#911)

* ♻️ Migrate persistence layer methods to object factory

In review of the code and in brief discussion with @orangewolf, the
methods of the persistence layer could be added to the object factory.

We already were configuring the corresponding object factory for each
implementation of Bulkrax; so leveraging that configuration made
tremendous sense.

The methods on the persistence layer remain helpful (perhaps necessary)
for documented reasons in the `Bulkrax::ObjectFactoryInterface` module.

See:

- #895 and its discussion

* 🎁 Add Valkyrie object factory interface methods

* 🧹 Favor interface based exception

Given that we are not directly exposing ActiveFedora nor Hyrax nor
Valkyrie objects, we want to translate/transform exceptions into a
common exception based on an interface.

That way downstream implementers can catch the Bulkrax specific error
and not need to do things such as `if
defined?(ActiveFedora::RecordInvalid) rescue
ActiveFedora::RecordInvalid`

It's just funny looking.

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)

lib/bulkrax/persistence_layer.rb → 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

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

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

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|

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
 

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

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]

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

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,15 +175,15 @@ 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 }
           )
         end
       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: [

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<Bulkrax::PersistenceLayer::AbstractAdapter>]
-    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<Bulkrax::PersistenceLayer::AbstractAdapter>]
-    # @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,

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