allow for extensions of valkyrie indexer's to_solr that depends on resource custom metadata #4463

elrayle · 2020-07-20T19:46:16Z

Description

Allow shared spec testing of indexers with customizations to #to_solr that depend on properties defined in the resource class that the indexer indexes.

Resolution

Update shared indexers spec to have a valid resource passed in to the shared spec. The shared spec sets values it depends on in a before block.

@samvera/hyrax-code-reviewers

elrayle · 2020-07-20T19:47:24Z

lib/hyrax/specs/shared_specs/indexers.rb

@@ -3,7 +3,8 @@
 RSpec.shared_examples 'a Hyrax::Resource indexer' do
  subject(:indexer) { indexer_class.new(resource: resource) }
  let(:ids)         { ['id1', 'id2'] }
-  let(:resource)    { Hyrax::Resource.new(alternate_ids: ids) }
+  let(:the_resource_class) { defined?(resource_class) ? resource_class : Hyrax::Work }


Setting resource_class to Hyrax::Work when it isn't defined allows for backward compatibility.

Passing in the resource_class allows #to_solr to successfully create the solr_doc when the indexer is expecting the resource to have custom metadata defined.

elrayle · 2020-07-20T19:47:59Z

spec/indexers/hyrax/valkyrie_collection_indexer_spec.rb

@@ -3,7 +3,12 @@
 require 'hyrax/specs/shared_specs'

 RSpec.describe Hyrax::ValkyrieCollectionIndexer do
-  subject(:indexer) { described_class.new(resource: resource) }


The subject is not used anywhere, so it was removed.

elrayle · 2020-07-20T19:51:38Z

spec/indexers/hyrax/valkyrie_collection_indexer_spec.rb

-  subject(:indexer) { described_class.new(resource: resource) }
+  let(:resource_class) do
+    Class.new(Hyrax::Resource) do
+      # include ::Hyrax::CollectionBehavior # TODO: How is this represented in the Valkyrie::Resource version of a Collection?


I could not find any example of a defined or generated Collection that is a Valkyrie::Resource. The AF version includes CollectionBehavior. I commented it out and left it here as a placeholder for any behaviors that are expected/required for a Valkyrie version of a Hyrax Collection. I'd be interested in what others think should be included in this class. NOTE: Prior to this PR, the Collection indexer was being tested with a Hyrax::Work as the resource passed to the indexer.

Found it, this should be Hyrax::PcdmCollection

elrayle · 2020-07-20T19:52:08Z

spec/indexers/hyrax/valkyrie_work_indexer_spec.rb

@@ -3,8 +3,7 @@
 require 'hyrax/specs/shared_specs'

 RSpec.describe Hyrax::ValkyrieWorkIndexer do
-  let(:indexer) { described_class.new(resource: resource) }
-  let(:resource) { FactoryBot.valkyrie_create(:hyrax_work) }


indexer and resource were not used by this test, so they were removed.

elrayle · 2020-07-20T19:54:08Z

spec/indexers/hyrax/valkyrie_work_indexer_spec.rb

@@ -21,4 +20,11 @@

    it_behaves_like 'a Basic metadata indexer'
  end
+
+  context 'when extending with custom metadata' do


This context is the spec that tests the described behavior of an indexer that uses custom metadata in the resource. Without the fix that allows the resource_class to be passed to the shared spec, this will fail with the Actual Behavior described in the PR comments. With the fix it passes with the Expected Behavior.

elrayle · 2020-07-21T15:00:25Z

lib/hyrax/specs/shared_specs/indexers.rb

-  let(:resource)    { Hyrax::Resource.new(alternate_ids: ids) }
+  subject(:indexer)  { indexer_class.new(resource: the_resource) }
+  let(:ids)          { ['id1', 'id2'] }
+  let(:the_resource) { defined?(resource) ? resource : Hyrax::Resource.new }


This is providing backward compatibility. If the caller does not create a resource, then one is created in the shared spec. If backward compatibility is not required, this setup would become...

subject(:indexer) { indexer_class.new(resource: resource) } let(:ids) { ['id1', 'id2'] }

NOTE: All references to the_resource are providing backward compatibility for each shared spec. Similar simplifications would occur if resource was required to be defined by the caller.

Based on conversations, the reason for these conditionals is for backwards compatibility.

As this was not released in a Hyrax version, we can remove the conditionals and tighten up the logic. Anyone that's using this as written will have a broken spec.

elrayle · 2020-07-21T15:01:24Z

lib/hyrax/specs/shared_specs/indexers.rb


  describe '#to_solr' do
+    before { the_resource.alternate_ids = ids }


Makes sure the alternate_ids are set whether the resource is created in the shared spec or by the caller.

NOTE: Similar use of before blocks or setting in the let(:the_resource) statement in other shared specs to be sure the resource has the values set that are being tested.

no-reply · 2020-07-21T17:27:13Z

this seems on the right track, in general. it might be simplified by using shared_example parameters as document here: https://relishapp.com/rspec/rspec-core/docs/example-groups/shared-examples

elrayle · 2020-07-22T00:28:54Z

lib/hyrax/specs/shared_specs/indexers.rb

    it 'indexes basic metadata' do
      expect(indexer.to_solr)
-        .to include(keyword_sim: a_collection_containing_exactly(*attributes[:keyword]),
+        .to include(keyword_sim:   a_collection_containing_exactly(*attributes[:keyword]),


This change just makes the a_collection... align with a_collection... in the following 2 lines. No real code change.

elrayle · 2020-07-22T00:45:32Z

spec/support/book_resource.rb

+      attribute :created,   Valkyrie::Types::Date
+      attribute :isbn,      Valkyrie::Types::String
+      attribute :publisher, Valkyrie::Types::String
+      attribute :title,     Valkyrie::Types::String


Most of this is just an alignment change. The only real change is fixing the typo from pubisher to publisher.

…source custom metadata Fixes #4464 Allow shared spec testing of indexers with customizations to #to_solr that depend on properties defined in the resource class that the indexer indexes. Update shared indexers spec to have a valid resource passed in to the shared spec. The shared spec sets values it depends on in a before block.

elrayle · 2020-07-22T13:09:29Z

app/services/hyrax/thumbnail_path_service.rb

@@ -5,7 +5,7 @@ class << self
      # @param [#id] object - to get the thumbnail for
      # @return [String] a path to the thumbnail
      def call(object)
-        return default_image unless object.try(:thumbnail_id)
+        return default_image unless object.try(:thumbnail_id)&.to_s&.present?


For collections shared spec, the thumbnail_id was a Valkyrie ID with id="". I don't know if this would happen in the application, but it doesn't hurt anything to have the extra code. If thumbnail_id is nil, it will still use the default_image.

This may be a separate commit. There may be other problems involved. @no-reply can you take action on this?

We may need to test that Valkyrie::ID with an empty string is not present.

elrayle · 2020-07-22T13:21:15Z

@no-reply RE: parameters to shared specs... I like the idea of making the parameters more transparent. I don't think it would simplify the code if we still have the requirement of providing a default if the parameter does not have a value. With a change to parameterized shared specs, would you see it as ok to remove the creation a default resource when one is not provided making parameters required? That would simplify the code.

elrayle · 2020-07-22T14:35:28Z

Problem with parameters for shared specs. You cannot use let or before to do setup on the parameters. They have to be created on the it_behaves_like statement.

WORKS

  it_behaves_like 'a Hyrax::Resource indexer', described_class, Hyrax::Work.new

DOESN'T WORK (error loading spec file)

  let(:indexer_class) { described_class }
  let(:resource) { Hyrax::Work.new }

  it_behaves_like 'a Hyrax::Resource indexer', indexer_class, resource

This example is simple. But if you needed to make a modification to resource before sending it, there isn't a way to do that.

Also, you can't set up the resource once and have it used for all shared specs. You have to copy the creation code to every it_behaves_like statement which isn't DRY.

  it_behaves_like 'a Hyrax::Resource indexer', described_class, Hyrax::Work.new
  it_behaves_like 'a Core metadata indexer', described_class, Hyrax::Work.new
  it_behaves_like 'a permission indexer', described_class, Hyrax::Work.new
  it_behaves_like 'a visibility indexer', described_class, Hyrax::Work.new

Any change to the resource you create requires every it_behaves_like to be updated.

elrayle · 2020-07-22T15:11:27Z

Example preprocessing from our app. We default our works to public instead of private. So I have to fake this to allow our indexer to pass the visibility indexer shared spec.

  context 'FAKE work defaulting to private visibility for shared specs' do
    before do
      Hyrax::VisibilityWriter.new(resource: resource)
        .assign_access_for(visibility: Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE)
    end
    it_behaves_like 'a visibility indexer'
  end

jeremyf · 2020-07-22T16:33:19Z

spec/indexers/hyrax/valkyrie_work_indexer_spec.rb

+    after { Hyrax::Test.send(:remove_const, :Custom) }
+
+    let(:resource) { Hyrax.persister.save(resource: Hyrax::Test::Custom::Work.new(broader: ['term1', 'term2'])) }
+    let(:indexer_class) { Hyrax::Test::Custom::WorkIndexer }


Per conversation, this was already supported.

jeremyf · 2020-07-22T16:33:30Z

spec/indexers/hyrax/valkyrie_work_indexer_spec.rb

+    end
+    after { Hyrax::Test.send(:remove_const, :Custom) }
+
+    let(:resource) { Hyrax.persister.save(resource: Hyrax::Test::Custom::Work.new(broader: ['term1', 'term2'])) }


Per tech call, this is the line that the PR is supporting.

jeremyf · 2020-07-22T16:35:11Z

spec/indexers/hyrax/resource_indexer_spec.rb

@@ -3,6 +3,7 @@
 require 'hyrax/specs/shared_specs'

 RSpec.describe Hyrax::ResourceIndexer do
+  let(:resource) { Hyrax::Resource.new }


This is added to clarify and ensure the proper behavior. Prior to this, we had misleading successful test.

resource is now required

jeremyf

Discussed on the tech call https://wiki.lyrasis.org/display/samvera/Samvera+Tech+Call+2020-07-22,

elrayle · 2020-07-22T18:47:53Z

app/indexers/hyrax/valkyrie_collection_indexer.rb

@@ -7,6 +7,7 @@ class ValkyrieCollectionIndexer < Hyrax::ValkyrieIndexer
    include Hyrax::ResourceIndexer
    include Hyrax::PermissionIndexer
    include Hyrax::VisibilityIndexer
+    include Hyrax::Indexer(:core_metadata)


Collection resource defines core metadata, so seems like the collection indexer should index it.

elrayle · 2020-07-22T18:48:34Z

app/services/hyrax/thumbnail_path_service.rb

@@ -5,7 +5,7 @@ class << self
      # @param [#id] object - to get the thumbnail for
      # @return [String] a path to the thumbnail
      def call(object)
-        return default_image unless object.try(:thumbnail_id)
+        return default_image unless object.try(:thumbnail_id)&.present?


Catches the case where thumbnail_id is nil and when it is a Valkyrie::ID with an empty string.

elrayle · 2020-07-22T18:53:03Z

lib/hyrax/specs/shared_specs/indexers.rb

@@ -61,9 +82,37 @@
  end
 end

+RSpec.shared_examples 'a Core metadata indexer' do


This is mostly the same with the similar changes for variable validation and removing the default resource. It was moved to make the ordering of the various shared specs easier to follow.

elrayle · 2020-07-22T19:00:28Z

lib/hyrax/specs/shared_specs/indexers.rb

+  it_behaves_like 'a Hyrax::Resource indexer'
+  it_behaves_like 'a Core metadata indexer'
+  it_behaves_like 'a permission indexer'
+  it_behaves_like 'a visibility indexer'


The shared spec defines the minimal requirements for a work instead of expecting the individual work tests to know which parts are required.

elrayle · 2020-07-22T19:01:43Z

lib/hyrax/specs/shared_specs/indexers.rb


+  it_behaves_like 'a Hyrax::Resource indexer'
+  it_behaves_like 'a Core metadata indexer'


The shared spec defines the minimal requirements for a collection instead of expecting the individual collection tests to know which parts are required.

elrayle · 2020-07-22T19:03:21Z

lib/hyrax/specs/shared_specs/indexers.rb

@@ -107,21 +170,3 @@
    end
  end
 end
-
-RSpec.shared_examples 'a Core metadata indexer' do


Moved higher in the file

elrayle · 2020-07-22T19:07:07Z

spec/indexers/hyrax/valkyrie_collection_indexer_spec.rb

  let(:indexer_class) { described_class }

-  it_behaves_like 'a Hyrax::Resource indexer'


All shared specs representing the minimal required parts of a collection are now defined and tested in the shared spec 'a Collection indexer'.

elrayle · 2020-07-22T19:07:27Z

spec/indexers/hyrax/valkyrie_work_indexer_spec.rb

-  it_behaves_like 'a Core metadata indexer'
-  it_behaves_like 'a permission indexer'
-  it_behaves_like 'a visibility indexer'
+  it_behaves_like 'a Work indexer'


All shared specs representing the minimal required parts of a work are now defined and tested in the shared spec 'a Work indexer'.

elrayle · 2020-07-22T19:16:06Z

spec/hyrax/indexer_spec.rb

@@ -13,6 +13,7 @@
  end

  context 'with core metadata schema' do
+    let(:resource) { work }


resource is required for shared indexer specs

elrayle · 2020-07-22T19:16:15Z

spec/indexers/hyrax/permission_indexer_spec.rb

@@ -8,6 +8,7 @@
      include Hyrax::PermissionIndexer
    end
  end
+  let(:resource) { Hyrax::Resource.new }


resource is required for shared indexer specs

app/services/hyrax/thumbnail_path_service.rb

no-reply · 2020-07-22T20:12:42Z

lib/hyrax/specs/shared_specs/indexers.rb

@@ -1,9 +1,19 @@
 # frozen_string_literal: true

+# These shared specs test various aspects of valkyrie based indexers by calling the #to_solr method.


the docs are really appreciated!

no-reply · 2020-07-22T20:14:00Z

lib/hyrax/specs/shared_specs/indexers.rb

-  let(:resource)    { Hyrax::Resource.new(alternate_ids: ids) }
+  before do
+    raise 'indexer_class must be set with `let(:indexer_class)`' unless defined? indexer_class
+    raise 'resource must be set with `let(:resource)` and is expected to be a kind of Hyrax::Resource' unless defined?(resource) && resource.kind_of?(Hyrax::Resource)


i feel like the type checks might be heavy-handed, but let's keep them for now.

* adds documentation of usage * raise exceptions if required indexer_class and resource are not set * add shared spec for ‘a Work indexer’ that calls shared specs related to works * add core metadata test to ‘a Collection indexer’ * allow default visibility to be passed in using ‘restricted’ if not passed in

elrayle commented Jul 20, 2020

View reviewed changes

elrayle changed the title ~~allow for extensions of valkyrie indexer's to_solr that depends on resource custom metadata~~ [WIP] allow for extensions of valkyrie indexer's to_solr that depends on resource custom metadata Jul 20, 2020

elrayle mentioned this pull request Jul 21, 2020

indexer shared specs are not usable outside hyrax when they depend on resource custom fields #4464

Closed

elrayle force-pushed the indexer_for_resource branch from 9a8f725 to 009f772 Compare July 21, 2020 14:55

elrayle commented Jul 21, 2020

View reviewed changes

elrayle force-pushed the indexer_for_resource branch from 009f772 to cb3d5de Compare July 21, 2020 16:55

elrayle force-pushed the indexer_for_resource branch from cb3d5de to 6f5de63 Compare July 22, 2020 00:24

elrayle commented Jul 22, 2020

View reviewed changes

elrayle force-pushed the indexer_for_resource branch from 6f5de63 to b345289 Compare July 22, 2020 01:51

elrayle force-pushed the indexer_for_resource branch from b345289 to 36a43b9 Compare July 22, 2020 13:02

elrayle commented Jul 22, 2020

View reviewed changes

elrayle changed the title ~~[WIP] allow for extensions of valkyrie indexer's to_solr that depends on resource custom metadata~~ allow for extensions of valkyrie indexer's to_solr that depends on resource custom metadata Jul 22, 2020

jeremyf reviewed Jul 22, 2020

View reviewed changes

elrayle commented Jul 22, 2020

View reviewed changes

elrayle force-pushed the indexer_for_resource branch from e469d86 to 2cd0868 Compare July 22, 2020 18:55

elrayle commented Jul 22, 2020

View reviewed changes

elrayle force-pushed the indexer_for_resource branch from 2cd0868 to c986456 Compare July 22, 2020 19:13

elrayle commented Jul 22, 2020

View reviewed changes

elrayle force-pushed the indexer_for_resource branch from c986456 to 8ced73e Compare July 22, 2020 19:41

no-reply reviewed Jul 22, 2020

View reviewed changes

app/services/hyrax/thumbnail_path_service.rb Outdated Show resolved Hide resolved

no-reply reviewed Jul 22, 2020

View reviewed changes

no-reply previously approved these changes Jul 22, 2020

View reviewed changes

elrayle dismissed no-reply’s stale review via cec491d July 22, 2020 21:18

no-reply previously approved these changes Jul 22, 2020

View reviewed changes

elrayle dismissed no-reply’s stale review via c9fd729 July 22, 2020 21:30

elrayle force-pushed the indexer_for_resource branch from cec491d to c9fd729 Compare July 22, 2020 21:30

no-reply approved these changes Jul 22, 2020

View reviewed changes

no-reply merged commit 88e4731 into master Jul 23, 2020

no-reply deleted the indexer_for_resource branch July 23, 2020 00:02


		describe '#to_solr' do
		before { the_resource.alternate_ids = ids }


		it_behaves_like 'a Hyrax::Resource indexer'
		it_behaves_like 'a Core metadata indexer'

		let(:indexer_class) { described_class }

		it_behaves_like 'a Hyrax::Resource indexer'

		@@ -1,9 +1,19 @@
		# frozen_string_literal: true

		# These shared specs test various aspects of valkyrie based indexers by calling the #to_solr method.

allow for extensions of valkyrie indexer's to_solr that depends on resource custom metadata #4463

allow for extensions of valkyrie indexer's to_solr that depends on resource custom metadata #4463

Conversation

elrayle commented Jul 20, 2020 • edited Loading

Description

Resolution

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

elrayle Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

elrayle Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

no-reply commented Jul 21, 2020

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

elrayle commented Jul 22, 2020

elrayle commented Jul 22, 2020

elrayle commented Jul 22, 2020

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

jeremyf 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

elrayle Jul 22, 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

Choose a reason for hiding this comment

elrayle commented Jul 20, 2020 •

edited

Loading

elrayle Jul 20, 2020 •

edited

Loading

elrayle Jul 21, 2020 •

edited

Loading

elrayle Jul 21, 2020 •

edited

Loading

elrayle Jul 22, 2020 •

edited

Loading