Extract characterization service to allow ActiveFedora and Valkyrie filesets #4213

aaron-collier · 2020-01-24T19:08:38Z

This extracts the majority of the current CharacterizerJob into a Characterizer that implements individual characterize methods per FileSet type.

The pattern followed here was based on suggestion by @no-reply

Note: the remainder of characterizer_job_spec can likely be removed after #4207

@samvera/hyrax-code-reviewers

jcoyne · 2020-01-24T19:26:49Z

app/services/hyrax/characterizer.rb

+    # @return [#characterize]
+    def self.for(source:)
+      case source
+      when Hyrax::FileSetBehavior # ActiveFedora


would this ever be the case? FileSetBehavior is a module not a class.

hyrax/app/models/concerns/hyrax/file_set_behavior.rb

Line 2 in addb09f

module FileSetBehavior

That is the case when passing a FileSet and not a Hyrax::FileSet. We see the same style here: https://github.com/samvera/hyrax/blob/master/app/services/hyrax/visibility_propagator.rb#L12. We verified this when stepping through this.

@jcoyne yeah. @aaron-collier and I verified this yesterday.

@jcoyne see:

2.5.5 :001 > file_set = FileSet.new => #<FileSet id: nil, head: [], tail: [], depositor: nil, title: [], date_uploaded: nil, date_modified: nil, alternative_title: [], label: nil, relative_path: nil, import_url: nil, resource_type: [], creator: [], contributor: [], description: [], abstract: [], keyword: [], license: [], rights_notes: [], rights_statement: [], access_right: [], publisher: [], date_created: [], subject: [], language: [], identifier: [], based_near: [], related_url: [], bibliographic_citation: [], source: [], access_control_id: nil, embargo_id: nil, lease_id: nil> 2.5.5 :002 > Hyrax::FileSetBehavior === file_set => true

This seems really unclear and easy to violate this assumption.

Could you use ActiveFedora::Base instead?

i'm 👍 on using ActiveFedora::Base, with the idea that as long as we're in a position where we need to do type checks we should do the least strict type checks that will work.

in theory, folks can implement a valid AF File Set class that doesn't include Hyrax::FileSetBehavior, right?

mjgiarlo

Have some questions and comments for you to consider. Happy to pair!

mjgiarlo · 2020-01-24T19:19:02Z

app/jobs/characterize_job.rb

@@ -1,34 +1,10 @@
+# Characterizes the file at 'filepath' if available, otherwise, pulls a copy from the repository
+# and runs characterization on that file.
+


A trivial thing: would you consider dropping the newline between the class documentation and the class declaration?

mjgiarlo · 2020-01-24T19:19:55Z

app/jobs/characterize_job.rb

+  def perform(work)
+    Hyrax::Characterizer.for(source: work).characterize


How would you feel about renaming work on lines 7 and 8 to file_set?

mjgiarlo · 2020-01-24T19:20:50Z

app/models/hyrax/file_set.rb

@@ -7,6 +7,8 @@ module Hyrax
  # @see https://wiki.duraspace.org/display/samvera/Hydra%3A%3AWorks+Shared+Modeling
  class FileSet < Hyrax::Resource
    include Hyrax::Schema(:core_metadata)
+    include Hyrax::FileSet::Characterization


Both @aaron-collier and I believe this is the smallest possible set of includes to make sure resource-backed file sets respond to messages that are sent in the course of characterizing them.

Hyrax::FileSet::Characterization is needed to set up the characterization proxy, and Hydra::Works::MimeTypes is needed to ask a file set what sort of files it wraps (e.g., #image?), based on mime types.

+1 on including MimeTypes, the derivatives job also needs to know that for a resource-backed file set.

if these are required FileSet interface, let's get them into the 'a Hyrax::FileSet' shared specs in lib/hyrax/specs/shared_specs/hydra_works.rb?

i'm tempted to look for a way around making this the interface, though. so far we've been mostly successful in avoiding putting business logic in the models. while there's definitely space for compromise on that, it seems worth at least poking at other possibilities. any ideas?

re: MimeTypes,

Hyrax::FileSet doesn't (yet) have a #mime_type property, so i think the include is injecting a bunch of methods that will currently raise errors?

ah... noticing that ::FileSet also doesn't have a mime_type property and just delegates it to #characterization_proxy. it seems like we should follow suit and leave #mime_type at the file level?

wondering about this as a possible solution to the #mime_type issue #4215

mjgiarlo · 2020-01-24T19:22:52Z

app/services/hyrax/characterizer.rb

@@ -0,0 +1,19 @@
+module Hyrax
+  ##
+  # @abstract Determins which characterizer to run based on the file_set type


Typo: Determins

app/services/hyrax/characterizer.rb

mjgiarlo · 2020-01-24T19:32:42Z

app/services/hyrax/resource_characterizer.rb

+    ##
+    # @param source the object to characterize
+    def initialize(source:)
+      self.source = source


Should this be @source = source?

mjgiarlo · 2020-01-24T19:32:57Z

app/services/hyrax/resource_characterizer.rb

+    ##
+    # @return [void]
+    #
+    # @raise [RuntimeError] if visibility propogation fails


I think the text here needs to change.

mjgiarlo · 2020-01-24T19:35:42Z

app/services/hyrax/resource_characterizer.rb

+      Hydra::Works::CharacterizationService.run(characterization_proxy, filepath)
+      Rails.logger.debug "Ran characterization on #{characterization_proxy.id} (#{characterization_proxy.mime_type})"
+      characterization_proxy.alpha_channels = channels(filepath) if source.image? && Hyrax.config.iiif_image_server?
+      persist


Similar to the above comment, I wonder about the utility of moving the persistence code into this method.

mjgiarlo · 2020-01-24T19:35:58Z

app/services/hyrax/resource_characterizer.rb

+      def characterization_proxy
+        raise "#{source.class.characterization_proxy} was not found for FileSet #{source.id}" unless source.characterization_proxy?
+        source.characterization_proxy
+      end


Same comment on this method as above.

mjgiarlo · 2020-01-24T19:36:36Z

app/services/hyrax/resource_characterizer.rb

+      end
+
+      def filepath
+        Hyrax::WorkingDirectory.find_or_retrieve(source.original_file.id, source.id.id)


what would you think about tossing a comment here calling out the double id call is intentional (and required)

mjgiarlo

Have some test-related comments and questions as well. Thanks for considering!

mjgiarlo · 2020-01-24T19:40:45Z

spec/jobs/characterize_job_spec.rb

-    FileSet.new(id: file_set_id).tap do |fs|
-      allow(fs).to receive(:original_file).and_return(file)
-      allow(fs).to receive(:update_index)
+  context 'when the work is an ActiveFedora FileSet' do


Is this a context we want to keep here? the job is not switching behavior anymore.

mjgiarlo · 2020-01-24T19:42:08Z

spec/jobs/characterize_job_spec.rb

-    it "reindexes the collection" do
-      expect(collection).to receive(:update_index)
-      described_class.perform_now(file_set, file.id)
+      before do


I think this test should be removed, since the characterizejob no longer handles this behavior. I believe the test should only ensure that the characterizer factory returns a thing that #characterize gets called on.

spec/services/hyrax/file_set_characterizer_spec.rb

mjgiarlo · 2020-01-24T19:47:47Z

spec/services/hyrax/file_set_characterizer_spec.rb

+  context 'when the characterization proxy content is absent' do
+    before { allow(file_set).to receive(:characterization_proxy?).and_return(false) }
+    it 'raises an error' do
+      expect { characterizer.characterize }.to raise_error(StandardError, /original_file was not found/)


I am wondering if this can/should change to RuntimeError

mjgiarlo · 2020-01-24T19:48:51Z

spec/services/hyrax/resource_characterizer_spec.rb

@@ -0,0 +1,32 @@
+RSpec.describe Hyrax::ResourceCharacterizer do
+  subject(:characterizer) { described_class.new(source: resource) }
+  let(:resource) { FactoryBot.create(:file_set).valkyrie_resource }


I think this is dead code since each context below has its own resource let block

mjgiarlo · 2020-01-24T19:49:05Z

spec/services/hyrax/resource_characterizer_spec.rb

+    before { allow(resource).to receive(:characterization_proxy?).and_return(false) }
+
+    it 'runs Hydra::Works::CharacterizationService and creates a CreateDerivativesJob' do
+      expect { characterizer.characterize }.to raise_error(StandardError, /original_file was not found/)


RuntimeError?

no-reply · 2020-01-24T23:30:29Z

spec/services/hyrax/resource_characterizer_spec.rb

+  subject(:characterizer) { described_class.new(source: resource) }
+
+  before do
+    allow(Hyrax::FileSet).to receive(:find).with(resource.id).and_return(resource)


can Hyrax::FileSet do this? is this coming from one of the includes??

no-reply · 2020-01-24T23:36:16Z

spec/services/hyrax/resource_characterizer_spec.rb

+    allow(Hyrax::FileSet).to receive(:find).with(resource.id).and_return(resource)
+    allow(Hydra::Works::CharacterizationService).to receive(:run)
+    allow(Hyrax.persister).to receive(:save)
+    allow(CreateDerivativesJob).to receive(:perform_later)


is this just to avoid running the job? if so, it shouldn't be needed since we use the test queue adapter by default in the test suite.

no-reply · 2020-01-24T23:40:02Z

spec/services/hyrax/resource_characterizer_spec.rb

+  before do
+    allow(Hyrax::FileSet).to receive(:find).with(resource.id).and_return(resource)
+    allow(Hydra::Works::CharacterizationService).to receive(:run)
+    allow(Hyrax.persister).to receive(:save)


okay to just actually call save on the persister?

if we want to avoid round tripping wings, we could do allow(Hyrax).to receive(:metadata_adapter).and_return(Valkyrie::MetadataAdapter.find(:test_adapter)) to use the test/memory adapter for the duration.

i've considered rigging this into some RSpec metadata, but it hasn't made it's way into the codebase yet.

no-reply · 2020-01-24T23:42:07Z

spec/services/hyrax/resource_characterizer_spec.rb

+    it 'runs Hydra::Works::CharacterizationService and creates a CreateDerivativesJob' do
+      expect(Hydra::Works::CharacterizationService).to receive(:run).with(resource.original_file, anything)
+      expect(CreateDerivativesJob).to receive(:perform_later).with(resource, resource.original_file.id, anything)
+      expect(Hyrax.persister).to receive(:save).twice


twice with_args? or, again, consider just actually saving the objects to memory and expecting them to be saved.

no-reply · 2020-01-24T23:45:17Z

app/services/hyrax/resource_characterizer.rb

+    #
+    # @raise [RuntimeError] if FileSet is missing the characterization_proxy
+    def characterize
+      Hydra::Works::CharacterizationService.run(characterization_proxy, filepath)


what do you think about injecting this dependency?

## # @param service [#run] def characterize(service: Hydra::Works::CharacterizationService) service.run(characterization_proxy, filepath) ... end

in this case, we could inject a spy service in the test suite and check its arguments, instead of hard-stubbing the class method.

no-reply · 2020-01-24T23:46:14Z

app/services/hyrax/file_set_characterizer.rb

+    # @return [void]
+    #
+    # @raise [RuntimeError] if FileSet is missing the characterization_proxy
+    def characterize


same method signature/dependency injection suggestion as https://github.com/samvera/hyrax/pull/4213/files#r370888617

no-reply · 2020-01-24T23:48:57Z

app/services/hyrax/resource_characterizer.rb

+      Rails.logger.debug "Ran characterization on #{characterization_proxy.id} (#{characterization_proxy.mime_type})"
+      characterization_proxy.alpha_channels = channels(filepath) if source.image? && Hyrax.config.iiif_image_server?
+      Hyrax.persister.save(resource: characterization_proxy)
+      Hyrax.persister.save(resource: source)


should this trigger a reindex of source in some way, or is it not needed?

stale · 2020-02-24T02:03:28Z

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

aaron-collier force-pushed the characterization_service branch from 4880788 to 0c68ba8 Compare January 24, 2020 19:13

aaron-collier requested a review from no-reply January 24, 2020 19:17

jcoyne reviewed Jan 24, 2020

View reviewed changes

mjgiarlo requested changes Jan 24, 2020

View reviewed changes

aaron-collier force-pushed the characterization_service branch from 0c68ba8 to df67e5e Compare January 24, 2020 20:22

aaron-collier added 12 commits January 24, 2020 12:25

Move active fedora fileset characterizer into a service

e80bfa0

Initial valyrie resource test

dbbd474

WIP

10076f8

Initial valkyrie file set characterization

a5c7b0e

Raise if missing characterizer_proxy

3282092

Reduce method complexity

5ed9a6f

Reduce method complexity

0dcf42b

Add ResourceCharacterizer test

d4ca105

Remove duplicated test context

0eef6ec

Add FileSetCharacterizer spec

2d0d3f5

Update ResourceCharactizer spec

872ba30

Remove duplicate test

88aab47

aaron-collier force-pushed the characterization_service branch from df67e5e to 2845fb5 Compare January 24, 2020 20:26

Cleanup comment cruft and style considerations

328c6a9

aaron-collier force-pushed the characterization_service branch from 2845fb5 to 328c6a9 Compare January 24, 2020 20:48

no-reply reviewed Jan 24, 2020

View reviewed changes

no-reply mentioned this pull request Jan 25, 2020

Create FileSetDescription to find characterization for FileSets #4215

Merged

jeremyf mentioned this pull request Feb 1, 2020

Refactoring for hopefully making 4195 easier #4243

Merged

stale bot added the stale label Feb 24, 2020

stale bot closed this Mar 9, 2020

		@@ -1,34 +1,10 @@
		# Characterizes the file at 'filepath' if available, otherwise, pulls a copy from the repository
		# and runs characterization on that file.

		def perform(work)
		Hyrax::Characterizer.for(source: work).characterize

Extract characterization service to allow ActiveFedora and Valkyrie filesets #4213

Extract characterization service to allow ActiveFedora and Valkyrie filesets #4213

Conversation

aaron-collier commented Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjgiarlo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

no-reply Jan 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjgiarlo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

no-reply Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Feb 24, 2020

aaron-collier commented Jan 24, 2020 •

edited

Loading

no-reply Jan 25, 2020 •

edited

Loading

no-reply Jan 24, 2020 •

edited

Loading