Add indexing for Hyrax::FileSet through valkyrie indexing #4780

elrayle · 2021-03-09T22:52:22Z

Description

This creates indexing for Hyrax::FileSet, which is a Valkyrie::Resource, using the Valkyrie indexing process called from the MetadataIndexListener.

Questions

There are two questions posed in the comments in the file changes.

Is the refactor of Hyrax::FileSetIndexer to Hyrax::ActiveFedoraFileSetIndexer acceptable? It is necessary for Valkyrie indexing naming conventions, but does represent a public API change.
Do we want to index all file metadata?

Known remaining issues

Jobs that need to add support for Valkyrie

CharacterizationJob (Allow CharacterizeJob to receive either an AF object or a Valkyrie resource as the FileSet #4100)
CreateDerivativesJob (Allow CreateDerivativesJob to receive either an AF object or a Valkyrie resource #4191)
possible others as well

Valkyrie works do not include FileSets as members in solr docs.

@samvera/hyrax-code-reviewers

app/actors/hyrax/actors/file_actor.rb

app/indexers/hyrax/active_fedora_file_set_indexer.rb

elrayle · 2021-03-12T17:18:35Z

app/indexers/hyrax/file_set_indexer.rb

+  ##
+  # Indexes Hyrax::FileSet objects
+  class FileSetIndexer < Hyrax::ValkyrieIndexer
+    include Hyrax::ResourceIndexer


This is the refactor of Hyrax::FileSetIndexer to be the indexer for Hyrax::FileSet (valkyrie resource) instead of ::FileSet (AF object). This is needed because Hyrax::ValkyrieIndexer #for method uses naming convention to determine the indexer to use for a given Valkyrie resource.

elrayle · 2021-03-12T17:20:24Z

app/indexers/hyrax/file_set_indexer.rb

+        solr_doc['type_tesim']              = file_metadata.type if file_metadata.type.present?
+
+        # attributes set by fits
+        solr_doc['format_label_tesim']      = file_metadata.format_label if file_metadata.format_label.present?


Do we want to include all file metadata in the solr_doc? Right now, that is what this does.

elrayle · 2021-03-12T17:23:14Z

app/indexers/hyrax/file_set_indexer.rb

+
+        # attributes set by fits
+        solr_doc['format_label_tesim']      = file_metadata.format_label if file_metadata.format_label.present?
+        solr_doc['size_tesim']              = file_metadata.size if file_metadata.size.present?


Size is recorded in the solr doc twice.

At line 37, it is recorded as a long. (Consistent with how it is done in the AF generated solr doc.)

Here is is recorded as a string. (Consistent with how all the other FileMetadata fields are indexed.)

elrayle · 2021-03-12T17:25:54Z

app/models/concerns/hyrax/file_set/indexing.rb

@@ -6,7 +6,7 @@ module Indexing

      included do
        # the default indexing service
-        self.indexer = Hyrax::FileSetIndexer
+        self.indexer = Hyrax::ActiveFedoraFileSetIndexer


Completes the refactor moving the AF indexing to Hyrax::ActiveFedoraFileSetIndexer (for AF objects) out of Hyrax::FileSetIndexer (now being used for Valkyrie resource Hyrax::FileSet).

app/models/hyrax/file_set.rb

app/models/job_io_wrapper.rb

elrayle · 2021-03-12T17:35:52Z

spec/actors/hyrax/actors/create_with_remote_files_actor_spec.rb

@@ -134,4 +134,114 @@
      expect(actor.send(:validate_remote_url, URI('https://example.com/test.txt'))).to be true
    end
  end
+
+  context 'when work is a valkyrie resource' do
+    let(:work) { valkyrie_create(:monograph) }


These are the same tests as above, but run with a Valkyrie resource instead of AF Object.

elrayle · 2021-03-12T17:38:59Z

spec/indexers/hyrax/active_fedora_file_set_indexer_spec.rb

+RSpec.describe Hyrax::ActiveFedoraFileSetIndexer do
+  include Hyrax::FactoryHelpers
+
+  let(:file_set) do


This is a straight up move of the code that was in file_set_indexer_spec.rb (which used to test the indexer for AF ::FileSet, but now tests indexer for Valkyrie Hyrax::FileSet).

elrayle · 2021-03-12T17:40:06Z

spec/indexers/hyrax/file_set_indexer_spec.rb

@@ -2,148 +2,249 @@
 RSpec.describe Hyrax::FileSetIndexer do
  include Hyrax::FactoryHelpers

+  let(:fileset_id) { 'fs1' }


This spec used to test the indexer for the AF ::FileSet, but now tests the indexer for the Valkyrie Hyrax::FileSet.

app/models/hyrax/work.rb

no-reply · 2021-03-12T18:09:14Z

app/indexers/hyrax/file_set_indexer.rb

 module Hyrax
-  class FileSetIndexer < ActiveFedora::IndexingService


i'm worried about this name swap. this would be a high-impact breaking change, right?

no-reply · 2021-03-12T18:13:38Z

app/actors/hyrax/actors/file_set_actor.rb

@@ -87,7 +87,7 @@ def attach_to_work(work, file_set_params = {})
      def attach_to_valkyrie_work(work, file_set_params)
        work = Hyrax.query_service.find_by(id: work.id) unless work.new_record
        file_set.visibility = work.visibility unless assign_visibility?(file_set_params)
-        Hyrax.persister.save(resource: file_set)
+        @file_set = Hyrax.persister.save(resource: file_set)


can you say more about this instance variable?

it will be cached with this actor in the ActionDispatch::MiddlewareStack pool, which will hold it in memory until the next time its set. this can lead to unexpected behavior. see #3722

no-reply · 2021-03-12T18:22:59Z

app/actors/hyrax/actors/create_with_remote_files_actor.rb

@@ -61,9 +61,17 @@ def attach_files(env, remote_files)
        true
      end

+      def create_file_from_url(env, uri, file_name, auth_header)
+        if env.curation_concern.is_a? Valkyrie::Resource


this is small and optional, but i'm preferring switch statements for behavior that's switching on type:

case env.curation_concern when Valkyrie::Resource create_file_from_url_through_valkyrie(env, uri, file_name, auth_header) else create_file_from_url_through_active_fedora(env, uri, file_name, auth_header) end

app/actors/hyrax/actors/file_actor.rb

elrayle · 2021-03-12T20:49:00Z

app/indexers/hyrax/valkyrie_file_set_indexer.rb

+        solr_doc['type_tesim']              = file_metadata.type if file_metadata.type.present?
+
+        # attributes set by fits
+        solr_doc['format_label_tesim']      = file_metadata.format_label if file_metadata.format_label.present?


Do we want to include all file metadata in the solr_doc? Right now, that is what this does.

👍🏻 seems okay to add a lot now and try to tidy up another time.

app/indexers/hyrax/valkyrie_file_set_indexer.rb

app/indexers/hyrax/valkyrie_indexer.rb

This adds a basic solr doc for Hyrax::FileSet which is a Valkryie::Resource.

app/actors/hyrax/actors/file_set_actor.rb

no-reply · 2021-03-16T09:37:52Z

app/indexers/hyrax/valkyrie_file_set_indexer.rb

+        solr_doc['type_tesim']              = file_metadata.type if file_metadata.type.present?
+
+        # attributes set by fits
+        solr_doc['format_label_tesim']      = file_metadata.format_label if file_metadata.format_label.present?


👍🏻 seems okay to add a lot now and try to tidy up another time.

app/models/hyrax/work.rb

elrayle force-pushed the fix/4766_fileset_valk-solr branch 7 times, most recently from 1f649a6 to e2bb038 Compare March 12, 2021 17:06