From a04640b3ae5f335f328ac6a689257835bcce15c8 Mon Sep 17 00:00:00 2001
From: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Date: Wed, 7 Feb 2024 09:20:11 -0500
Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Migrate=20persistence=20la?=
=?UTF-8?q?yer=20methods=20to=20object=20factory?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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:
- https://github.com/samvera/bulkrax/pull/895 and its discussion
---
app/factories/bulkrax/object_factory.rb | 34 ++++++++++++++++--
.../bulkrax/object_factory_interface.rb | 22 ++++++++----
.../bulkrax/valkyrie_object_factory.rb | 2 ++
app/helpers/bulkrax/validation_helper.rb | 2 +-
app/jobs/bulkrax/create_relationships_job.rb | 2 +-
.../concerns/bulkrax/dynamic_record_lookup.rb | 4 +--
.../concerns/bulkrax/export_behavior.rb | 2 +-
app/parsers/bulkrax/bagit_parser.rb | 2 +-
app/parsers/bulkrax/csv_parser.rb | 2 +-
.../bulkrax/parser_export_record_set.rb | 14 ++++----
lib/bulkrax.rb | 36 +------------------
lib/bulkrax/engine.rb | 3 --
.../active_fedora_adapter.rb | 31 ----------------
.../persistence_layer/valkyrie_adapter.rb | 8 -----
spec/bulkrax_spec.rb | 8 -----
spec/parsers/bulkrax/bagit_parser_spec.rb | 4 +--
spec/parsers/bulkrax/csv_parser_spec.rb | 2 +-
spec/support/dynamic_record_lookup.rb | 10 +++---
18 files changed, 72 insertions(+), 116 deletions(-)
rename lib/bulkrax/persistence_layer.rb => app/factories/bulkrax/object_factory_interface.rb (63%)
delete mode 100644 lib/bulkrax/persistence_layer/active_fedora_adapter.rb
delete mode 100644 lib/bulkrax/persistence_layer/valkyrie_adapter.rb
diff --git a/app/factories/bulkrax/object_factory.rb b/app/factories/bulkrax/object_factory.rb
index 8aa26133..81182883 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 Module.const_defined?(:Solrizer)
+ ::Solrizer.solr_name(base_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
@@ -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..00168447 100644
--- a/app/factories/bulkrax/valkyrie_object_factory.rb
+++ b/app/factories/bulkrax/valkyrie_object_factory.rb
@@ -2,6 +2,8 @@
module Bulkrax
class ValkyrieObjectFactory < ObjectFactory
+ include ObjectFactoryInterface
+
##
# Retrieve properties from M3 model
# @param klass the model
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..fcda383b 100644
--- a/lib/bulkrax.rb
+++ b/lib/bulkrax.rb
@@ -38,11 +38,7 @@ class Configuration
# @return [#call] with arity 2. The first parameter is a {Bulkrax::ApplicationParser} and the
# 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
-
+ allow(Bulkrax.persistence_adapter).to receive(:find).and_return(nil)
##
# @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,
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