From a7b787fca9f56056c319d44a6bf165b09ecde129 Mon Sep 17 00:00:00 2001
From: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Date: Thu, 25 Jan 2024 11:07:46 -0500
Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=93=9A=20Adding=20documentation=20for?=
 =?UTF-8?q?=20configuration=20(#896)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This builds on a [question asked in Slack][1]

[1]: https://samvera.slack.com/archives/C03S9FS60KW/p1705681632335919
---
 lib/bulkrax.rb | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/bulkrax.rb b/lib/bulkrax.rb
index 3ca09999..b5a89837 100644
--- a/lib/bulkrax.rb
+++ b/lib/bulkrax.rb
@@ -19,7 +19,6 @@ class Configuration
                   :export_path,
                   :field_mappings,
                   :file_model_class,
-                  :fill_in_blank_source_identifiers,
                   :generated_metadata_mapping,
                   :import_path,
                   :multi_value_element_join_on,
@@ -35,12 +34,21 @@ class Configuration
                   :reserved_properties,
                   :server_name
 
+    ##
+    # @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
+
+    ##
+    # Configure which persistence adapter you'd prefer to favor.
+    #
+    # @param adapter [Class<Bulkrax::PersistenceLayer::AbstractAdapter>]
     attr_writer :persistence_adapter
 
     ##
     # Configure the persistence adapter used for persisting imported data.
     #
-    # @return [Bulkrax::PersistenceLayer::AbstractAdapter]
+    # @return [Class<Bulkrax::PersistenceLayer::AbstractAdapter>]
     # @see Bulkrax::PersistenceLayer
     def persistence_adapter
       @persistence_adapter || derived_persistence_adapter

From 4f8f2253f056134b40c69609b19258d2d8ccb415 Mon Sep 17 00:00:00 2001
From: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Date: Thu, 25 Jan 2024 17:46:09 -0500
Subject: [PATCH 2/3] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Extract=20Bulkrax::Fac?=
 =?UTF-8?q?toryClassFinder=20(#900)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This refactor introduces consolidating logic for determining an entry's
factory_class.

The goal is to begin to allow for us to have a CSV record that says
"model = Work" and to use a "WorkResource".

Note, there are downstream implementations that overwrite
`factory_class` and we'll need to consider how we approach that.
---
 app/models/bulkrax/csv_collection_entry.rb    |  4 +-
 app/models/bulkrax/entry.rb                   |  2 +
 app/models/bulkrax/oai_set_entry.rb           |  4 +-
 app/models/bulkrax/rdf_collection_entry.rb    |  5 +-
 .../bulkrax/file_set_entry_behavior.rb        |  6 +-
 .../concerns/bulkrax/import_behavior.rb       | 20 ++-----
 app/services/bulkrax/factory_class_finder.rb  | 56 +++++++++++++++++++
 lib/bulkrax.rb                                |  2 -
 .../models/bulkrax/csv_file_set_entry_spec.rb |  5 ++
 .../models/bulkrax/rdf_file_set_entry_spec.rb | 17 ++++++
 spec/spec_helper.rb                           |  2 +
 11 files changed, 93 insertions(+), 30 deletions(-)
 create mode 100644 app/services/bulkrax/factory_class_finder.rb
 create mode 100644 spec/models/bulkrax/rdf_file_set_entry_spec.rb

diff --git a/app/models/bulkrax/csv_collection_entry.rb b/app/models/bulkrax/csv_collection_entry.rb
index ea6df632..cc113c5f 100644
--- a/app/models/bulkrax/csv_collection_entry.rb
+++ b/app/models/bulkrax/csv_collection_entry.rb
@@ -2,9 +2,7 @@
 
 module Bulkrax
   class CsvCollectionEntry < CsvEntry
-    def factory_class
-      Collection
-    end
+    self.default_work_type = "Collection"
 
     # Use identifier set by CsvParser#unique_collection_identifier, which falls back
     # on the Collection's first title if record[source_identifier] is not present
diff --git a/app/models/bulkrax/entry.rb b/app/models/bulkrax/entry.rb
index be087de0..551ccfc6 100644
--- a/app/models/bulkrax/entry.rb
+++ b/app/models/bulkrax/entry.rb
@@ -8,6 +8,8 @@ class OAIError < RuntimeError; end
   class Entry < ApplicationRecord
     include Bulkrax::HasMatchers
     include Bulkrax::ImportBehavior
+    self.class_attribute :default_work_type, default: Bulkrax.default_work_type
+
     include Bulkrax::ExportBehavior
     include Bulkrax::StatusInfo
     include Bulkrax::HasLocalProcessing
diff --git a/app/models/bulkrax/oai_set_entry.rb b/app/models/bulkrax/oai_set_entry.rb
index 9555ebd9..11e3740b 100644
--- a/app/models/bulkrax/oai_set_entry.rb
+++ b/app/models/bulkrax/oai_set_entry.rb
@@ -2,9 +2,7 @@
 
 module Bulkrax
   class OaiSetEntry < OaiEntry
-    def factory_class
-      Collection
-    end
+    self.default_work_type = "Collection"
 
     def build_metadata
       self.parsed_metadata = self.raw_metadata
diff --git a/app/models/bulkrax/rdf_collection_entry.rb b/app/models/bulkrax/rdf_collection_entry.rb
index ce314920..bf4bded5 100644
--- a/app/models/bulkrax/rdf_collection_entry.rb
+++ b/app/models/bulkrax/rdf_collection_entry.rb
@@ -2,6 +2,7 @@
 
 module Bulkrax
   class RdfCollectionEntry < RdfEntry
+    self.default_work_type = "Collection"
     def record
       @record ||= self.raw_metadata
     end
@@ -11,9 +12,5 @@ def build_metadata
       add_local
       return self.parsed_metadata
     end
-
-    def factory_class
-      Collection
-    end
   end
 end
diff --git a/app/models/concerns/bulkrax/file_set_entry_behavior.rb b/app/models/concerns/bulkrax/file_set_entry_behavior.rb
index 1cf94a08..883df9de 100644
--- a/app/models/concerns/bulkrax/file_set_entry_behavior.rb
+++ b/app/models/concerns/bulkrax/file_set_entry_behavior.rb
@@ -2,8 +2,10 @@
 
 module Bulkrax
   module FileSetEntryBehavior
-    def factory_class
-      ::FileSet
+    extend ActiveSupport::Concern
+
+    included do
+      self.default_work_type = "::FileSet"
     end
 
     def file_reference
diff --git a/app/models/concerns/bulkrax/import_behavior.rb b/app/models/concerns/bulkrax/import_behavior.rb
index eea56afa..6e2f3c2d 100644
--- a/app/models/concerns/bulkrax/import_behavior.rb
+++ b/app/models/concerns/bulkrax/import_behavior.rb
@@ -189,22 +189,10 @@ def factory
     end
 
     def factory_class
-      fc = if self.parsed_metadata&.[]('model').present?
-             self.parsed_metadata&.[]('model').is_a?(Array) ? self.parsed_metadata&.[]('model')&.first : self.parsed_metadata&.[]('model')
-           elsif self.mapping&.[]('work_type').present?
-             self.parsed_metadata&.[]('work_type').is_a?(Array) ? self.parsed_metadata&.[]('work_type')&.first : self.parsed_metadata&.[]('work_type')
-           else
-             Bulkrax.default_work_type
-           end
-
-      # return the name of the collection or work
-      fc.tr!(' ', '_')
-      fc.downcase! if fc.match?(/[-_]/)
-      fc.camelcase.constantize
-    rescue NameError
-      nil
-    rescue
-      Bulkrax.default_work_type.constantize
+      # ATTENTION: Do not memoize this here; tests should catch the problem, but through out the
+      # lifecycle of parsing a CSV row or what not, we end up having different factory classes based
+      # on the encountered metadata.
+      FactoryClassFinder.find(entry: self)
     end
   end
 end
diff --git a/app/services/bulkrax/factory_class_finder.rb b/app/services/bulkrax/factory_class_finder.rb
new file mode 100644
index 00000000..bd85f8cd
--- /dev/null
+++ b/app/services/bulkrax/factory_class_finder.rb
@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+module Bulkrax
+  class FactoryClassFinder
+    ##
+    # @param entry [Bulkrax::Entry]
+    # @return [Class]
+    def self.find(entry:)
+      new(entry: entry).find
+    end
+
+    def initialize(entry:)
+      @entry = entry
+    end
+    attr_reader :entry
+
+    ##
+    # @return [Class] when we are able to derive the class based on the {#name}.
+    # @return [Nil] when we encounter errors with constantizing the {#name}.
+    # @see #name
+    def find
+      # TODO: We have a string, now we want to consider how we coerce.  Let's say we have Work and
+      # WorkResource in our upstream application.  Work extends ActiveFedora::Base and is legacy.
+      # And WorkResource extends Valkyrie::Resource and is where we want to be moving.  We may want
+      # to coerce the "Work" name into "WorkResource"
+      name.constantize
+    rescue NameError
+      nil
+    rescue
+      entry.default_work_type.constantize
+    end
+
+    ##
+    # @api private
+    # @return [String]
+    def name
+      fc = if entry.parsed_metadata&.[]('model').present?
+             Array.wrap(entry.parsed_metadata['model']).first
+           elsif entry.importerexporter&.mapping&.[]('work_type').present?
+             # Because of delegation's nil guard, we're reaching rather far into the implementation
+             # details.
+             Array.wrap(entry.parsed_metadata['work_type']).first
+           else
+             # The string might be frozen, so lets duplicate
+             entry.default_work_type.dup
+           end
+
+      # Let's coerce this into the right shape.
+      fc.tr!(' ', '_')
+      fc.downcase! if fc.match?(/[-_]/)
+      fc.camelcase
+    rescue
+      entry.default_work_type
+    end
+  end
+end
diff --git a/lib/bulkrax.rb b/lib/bulkrax.rb
index b5a89837..3c4c08a0 100644
--- a/lib/bulkrax.rb
+++ b/lib/bulkrax.rb
@@ -40,8 +40,6 @@ class Configuration
     attr_accessor :fill_in_blank_source_identifiers
 
     ##
-    # Configure which persistence adapter you'd prefer to favor.
-    #
     # @param adapter [Class<Bulkrax::PersistenceLayer::AbstractAdapter>]
     attr_writer :persistence_adapter
 
diff --git a/spec/models/bulkrax/csv_file_set_entry_spec.rb b/spec/models/bulkrax/csv_file_set_entry_spec.rb
index 70a6f568..a7d4523b 100644
--- a/spec/models/bulkrax/csv_file_set_entry_spec.rb
+++ b/spec/models/bulkrax/csv_file_set_entry_spec.rb
@@ -6,6 +6,11 @@ module Bulkrax
   RSpec.describe CsvFileSetEntry, type: :model do
     subject(:entry) { described_class.new }
 
+    describe '#default_work_type' do
+      subject { entry.default_work_type }
+      it { is_expected.to eq("::FileSet") }
+    end
+
     describe '#file_reference' do
       context 'when parsed_metadata includes the "file" property' do
         before do
diff --git a/spec/models/bulkrax/rdf_file_set_entry_spec.rb b/spec/models/bulkrax/rdf_file_set_entry_spec.rb
new file mode 100644
index 00000000..319e5563
--- /dev/null
+++ b/spec/models/bulkrax/rdf_file_set_entry_spec.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+module Bulkrax
+  RSpec.describe RdfFileSetEntry, type: :model do
+    describe '#default_work_type' do
+      subject { described_class.new.default_work_type }
+      it { is_expected.to eq("::FileSet") }
+    end
+
+    describe '#factory_class' do
+      subject { described_class.new.factory_class }
+      it { is_expected.to eq(::FileSet) }
+    end
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 01f7c974..2a874101 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'dry/monads'
+
 # This file was generated by the `rails generate rspec:install` command. Conventionally, all
 # specs live under a `spec` directory, which RSpec adds to the `$LOAD_PATH`.
 # The generated `.rspec` file contains `--require spec_helper` which will cause

From acb01389ca70fa94eb64c7dd7e980b3b9f9f8d32 Mon Sep 17 00:00:00 2001
From: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Date: Fri, 26 Jan 2024 11:06:42 -0500
Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=8E=81=20Config=20option=20for=20coer?=
 =?UTF-8?q?cing=20a=20factory=20class=20name=20(#901)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Prior to this commit, once we had a factory class name we always called
`#constantize` on the string.

With this commit, we retain that default behavior by:

1. Introducing a function that does this thing
2. Exposing a configuration end-point for that function

Further, we add a looming Valkyrie convention; namely that for each
legacy named model (e.g. `Work`) there will likely be a corresponding
model with the Resrouce suffix (e.g. `WorkResource`).

Related to:

- https://github.com/scientist-softserv/hykuup_knapsack/issues/125
---
 app/services/bulkrax/factory_class_finder.rb  | 62 ++++++++++++++-----
 lib/bulkrax.rb                                | 21 +++++++
 spec/lib/bulkrax_spec.rb                      | 10 +++
 .../bulkrax/factory_class_finder_spec.rb      | 60 ++++++++++++++++++
 4 files changed, 139 insertions(+), 14 deletions(-)
 create mode 100644 spec/services/bulkrax/factory_class_finder_spec.rb

diff --git a/app/services/bulkrax/factory_class_finder.rb b/app/services/bulkrax/factory_class_finder.rb
index bd85f8cd..ea3a31fd 100644
--- a/app/services/bulkrax/factory_class_finder.rb
+++ b/app/services/bulkrax/factory_class_finder.rb
@@ -2,28 +2,62 @@
 
 module Bulkrax
   class FactoryClassFinder
+    ##
+    # The v6.0.0 default coercer.  Responsible for converting a factory class name to a constant.
+    module DefaultCoercer
+      ##
+      # @param name [String]
+      # @return [Class] when the name is a coercible constant.
+      # @raise [NameError] when the name is not coercible to a constant.
+      def self.call(name)
+        name.constantize
+      end
+    end
+
+    ##
+    # A name coercer that favors classes that end with "Resource" but will attempt to fallback to
+    # those that don't.
+    module ValkyrieMigrationCoercer
+      SUFFIX = "Resource"
+
+      ##
+      # @param name [String]
+      # @param suffix [String] the suffix we use for a naming convention.
+      #
+      # @return [Class] when the name is a coercible constant.
+      # @raise [NameError] when the name is not coercible to a constant.
+      def self.call(name, suffix: SUFFIX)
+        if name.end_with?(suffix)
+          name.constantize
+        else
+          begin
+            "#{name}#{suffix}".constantize
+          rescue NameError
+            name.constantize
+          end
+        end
+      end
+    end
+
     ##
     # @param entry [Bulkrax::Entry]
     # @return [Class]
-    def self.find(entry:)
-      new(entry: entry).find
+    def self.find(entry:, coercer: Bulkrax.factory_class_name_coercer || DefaultCoercer)
+      new(entry: entry, coercer: coercer).find
     end
 
-    def initialize(entry:)
+    def initialize(entry:, coercer:)
       @entry = entry
+      @coercer = coercer
     end
-    attr_reader :entry
+    attr_reader :entry, :coercer
 
     ##
     # @return [Class] when we are able to derive the class based on the {#name}.
     # @return [Nil] when we encounter errors with constantizing the {#name}.
     # @see #name
     def find
-      # TODO: We have a string, now we want to consider how we coerce.  Let's say we have Work and
-      # WorkResource in our upstream application.  Work extends ActiveFedora::Base and is legacy.
-      # And WorkResource extends Valkyrie::Resource and is where we want to be moving.  We may want
-      # to coerce the "Work" name into "WorkResource"
-      name.constantize
+      coercer.call(name)
     rescue NameError
       nil
     rescue
@@ -41,13 +75,13 @@ def name
              # details.
              Array.wrap(entry.parsed_metadata['work_type']).first
            else
-             # The string might be frozen, so lets duplicate
-             entry.default_work_type.dup
+             entry.default_work_type
            end
 
-      # Let's coerce this into the right shape.
-      fc.tr!(' ', '_')
-      fc.downcase! if fc.match?(/[-_]/)
+      # Let's coerce this into the right shape; we're not mutating the string because it might well
+      # be frozen.
+      fc = fc.tr(' ', '_')
+      fc = fc.downcase if fc.match?(/[-_]/)
       fc.camelcase
     rescue
       entry.default_work_type
diff --git a/lib/bulkrax.rb b/lib/bulkrax.rb
index 3c4c08a0..e88f608b 100644
--- a/lib/bulkrax.rb
+++ b/lib/bulkrax.rb
@@ -43,6 +43,25 @@ class Configuration
     # @param adapter [Class<Bulkrax::PersistenceLayer::AbstractAdapter>]
     attr_writer :persistence_adapter
 
+    ##
+    # @param coercer [#call]
+    # @see Bulkrax::FactoryClassFinder
+    attr_writer :factory_class_name_coercer
+
+    ##
+    # A function responsible for converting the name of a factory class to the corresponding
+    # constant.
+    #
+    # @return [#call, Bulkrax::FactoryClassFinder::DefaultCoercer] an object responding to call,
+    #         with one positional parameter (e.g. arity == 1)
+    #
+    # @example
+    #   Bulkrax.factory_class_name_coercer.call("Work")
+    #   => Work
+    def factory_class_name_coercer
+      @factory_class_name_coercer || Bulkrax::FactoryClassFinder::DefaultCoercer
+    end
+
     ##
     # Configure the persistence adapter used for persisting imported data.
     #
@@ -99,6 +118,8 @@ def config
                  :default_work_type=,
                  :export_path,
                  :export_path=,
+                 :factory_class_name_coercer,
+                 :factory_class_name_coercer=,
                  :field_mappings,
                  :field_mappings=,
                  :file_model_class,
diff --git a/spec/lib/bulkrax_spec.rb b/spec/lib/bulkrax_spec.rb
index 4701b004..e66e3fa7 100644
--- a/spec/lib/bulkrax_spec.rb
+++ b/spec/lib/bulkrax_spec.rb
@@ -205,4 +205,14 @@
     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 }
+
+    it { is_expected.to respond_to(:call) }
+
+    it "has a method arity of 1" do
+      expect(subject.method(:call).arity).to eq 1
+    end
+  end
 end
diff --git a/spec/services/bulkrax/factory_class_finder_spec.rb b/spec/services/bulkrax/factory_class_finder_spec.rb
new file mode 100644
index 00000000..1ebdbd4f
--- /dev/null
+++ b/spec/services/bulkrax/factory_class_finder_spec.rb
@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Bulkrax::FactoryClassFinder do
+  let(:legacy_work_class) { Class.new }
+  let(:legacy_work_resource_class) { Class.new }
+  let(:valkyrie_only_resource_class) { Class.new }
+  let(:active_fedora_only_class) { Class.new }
+
+  before do
+    Object.const_set(:LegacyWork, legacy_work_class)
+    Object.const_set(:LegacyWorkResource, legacy_work_resource_class)
+    Object.const_set(:ValkyrieOnlyResource, valkyrie_only_resource_class)
+    Object.const_set(:ActiveFedoraOnly, active_fedora_only_class)
+  end
+
+  after do
+    Object.send(:remove_const, :LegacyWork)
+    Object.send(:remove_const, :LegacyWorkResource)
+    Object.send(:remove_const, :ValkyrieOnlyResource)
+    Object.send(:remove_const, :ActiveFedoraOnly)
+  end
+
+  describe "DefaultCoercer" do
+    it "simply constantizes (unsafely) the given string" do
+      factory_class_name = "Work"
+      expect(described_class::DefaultCoercer.call(factory_class_name)).to eq(Work)
+    end
+  end
+
+  describe "ValkyrieMigrationCoercer" do
+    it 'favors mapping names to those ending in Resource' do
+      expect(described_class::ValkyrieMigrationCoercer.call("LegacyWork")).to eq(legacy_work_resource_class)
+      expect(described_class::ValkyrieMigrationCoercer.call("LegacyWorkResource")).to eq(legacy_work_resource_class)
+      expect(described_class::ValkyrieMigrationCoercer.call("ValkyrieOnlyResource")).to eq(valkyrie_only_resource_class)
+      expect(described_class::ValkyrieMigrationCoercer.call("ValkyrieOnly")).to eq(valkyrie_only_resource_class)
+      expect(described_class::ValkyrieMigrationCoercer.call("ActiveFedoraOnly")).to eq(active_fedora_only_class)
+      expect { described_class::ValkyrieMigrationCoercer.call("ActiveFedoraOnlyResource") }.to raise_error(NameError)
+    end
+  end
+
+  describe '.find' do
+    let(:entry) { double(Bulkrax::Entry, parsed_metadata: { "model" => model_name }, default_work_type: "Work") }
+    subject(:finder) { described_class.find(entry: entry, coercer: coercer) }
+
+    [
+      [Bulkrax::FactoryClassFinder::DefaultCoercer, "Legacy Work", "LegacyWork"],
+      [Bulkrax::FactoryClassFinder::ValkyrieMigrationCoercer, "Legacy Work", "LegacyWorkResource"],
+      [Bulkrax::FactoryClassFinder::DefaultCoercer, "Legacy Work Resource", "LegacyWorkResource"]
+    ].each do |given_coercer, given_model_name, expected_class_name|
+      context "with an entry with model: #{given_model_name} and coercer: #{given_coercer}" do
+        let(:model_name) { given_model_name }
+        let(:coercer) { given_coercer }
+
+        it { is_expected.to eq expected_class_name.constantize }
+      end
+    end
+  end
+end