From adc7d02674c0dbaf3f3b641158293234dc3450d9 Mon Sep 17 00:00:00 2001 From: LaRita Robinson Date: Fri, 6 Oct 2017 18:08:27 -0400 Subject: [PATCH] Update nested reindexing Resolves https://github.com/samvera/hyrax/issues/1775 Resolves https://github.com/samvera/hyrax/issues/1785 Also removes ActiveFedora caching from the nesting index adapter. Nested reindexing occurs midway through the transaction, so AF was attempting to use the cache. However, this resulted in the updated solr document not including all of the AF updates. Reindexing changes have been separated into this one PR to isolate the indexing changes from other changes. In addition, unpins the solr version since incompatibilities have been previously resolved. --- .rubocop_fixme.yml | 1 + .solr_wrapper | 1 - .../concerns/hyrax/collection_behavior.rb | 16 +++++++--- .../hyrax/adapters/nesting_index_adapter.rb | 31 +++++++++++-------- spec/factories/collections_factory.rb | 16 +++++++++- spec/factories/generic_works.rb | 4 +++ spec/models/collection_spec.rb | 24 +++++++++++--- 7 files changed, 69 insertions(+), 24 deletions(-) diff --git a/.rubocop_fixme.yml b/.rubocop_fixme.yml index e86d00e2bd..7ddf4953cd 100644 --- a/.rubocop_fixme.yml +++ b/.rubocop_fixme.yml @@ -29,6 +29,7 @@ Metrics/ModuleLength: - 'app/models/concerns/hyrax/ability.rb' - 'app/services/hyrax/workflow/permission_query.rb' - 'spec/services/hyrax/workflow/permission_query_spec.rb' + - 'app/models/concerns/hyrax/collection_behavior.rb' Metrics/MethodLength: Exclude: diff --git a/.solr_wrapper b/.solr_wrapper index 1935e70ac2..a425df6d1d 100644 --- a/.solr_wrapper +++ b/.solr_wrapper @@ -1,6 +1,5 @@ # Place any default configuration for solr_wrapper here # port: 8983 -version: 6.6.1 collection: persist: true dir: solr/config/ diff --git a/app/models/concerns/hyrax/collection_behavior.rb b/app/models/concerns/hyrax/collection_behavior.rb index bd04794c3b..8537542168 100644 --- a/app/models/concerns/hyrax/collection_behavior.rb +++ b/app/models/concerns/hyrax/collection_behavior.rb @@ -126,10 +126,18 @@ def permission_template # Calculate and update who should have edit access based on who # has "manage" access in the PermissionTemplateAccess def update_access_controls! - update!(edit_users: permission_template.agent_ids_for(access: 'manage', agent_type: 'user'), - edit_groups: permission_template.agent_ids_for(access: 'manage', agent_type: 'group'), - read_users: permission_template.agent_ids_for(access: 'view', agent_type: 'user'), - read_groups: (permission_template.agent_ids_for(access: 'view', agent_type: 'group') + visibility_group).uniq) + edit_users = permission_template.agent_ids_for(access: 'manage', agent_type: 'user') + edit_groups = permission_template.agent_ids_for(access: 'manage', agent_type: 'group') + read_users = permission_template.agent_ids_for(access: 'view', agent_type: 'user') + read_groups = (permission_template.agent_ids_for(access: 'view', agent_type: 'group') + visibility_group).uniq + update!(edit_users: edit_users, + edit_groups: edit_groups, + read_users: read_users, + read_groups: read_groups) + # added because the collection indexing happens before Hydra::AccessControls::Permission is + # saved, and the after_update_index callback in the nested_relationship_reindexer removes the + # permissions from the Solr document. + update_index end private diff --git a/app/services/hyrax/adapters/nesting_index_adapter.rb b/app/services/hyrax/adapters/nesting_index_adapter.rb index 85abae6fff..bc806364ac 100644 --- a/app/services/hyrax/adapters/nesting_index_adapter.rb +++ b/app/services/hyrax/adapters/nesting_index_adapter.rb @@ -19,11 +19,14 @@ def self.find_preservation_document_by(id:) def self.find_preservation_parent_ids_for(id:) # Not everything is guaranteed to have library_collection_ids # If it doesn't have it, what do we do? - fedora_object = ActiveFedora::Base.find(id) - if fedora_object.respond_to?(:member_of_collection_ids) - fedora_object.member_of_collection_ids - else - [] + ActiveFedora::Base.uncached do + fedora_object = ActiveFedora::Base.find(id) + + if fedora_object.respond_to?(:member_of_collection_ids) + fedora_object.member_of_collection_ids + else + [] + end end end @@ -65,14 +68,16 @@ def self.each_perservation_document_id_and_parent_ids(&block) # @param pathnames [Array] # @return Hash - the attributes written to the indexing layer def self.write_document_attributes_to_index_layer(id:, parent_ids:, ancestors:, pathnames:) - solr_doc = ActiveFedora::Base.find(id).to_solr # What is the current state of the solr document - - # Now add the details from the nesting indexor to the document - solr_doc[solr_field_name_for_storing_ancestors] = ancestors - solr_doc[solr_field_name_for_storing_parent_ids] = parent_ids - solr_doc[solr_field_name_for_storing_pathnames] = pathnames - ActiveFedora::SolrService.add(solr_doc, commit: true) - solr_doc + ActiveFedora::Base.uncached do + solr_doc = ActiveFedora::Base.find(id).to_solr # What is the current state of the solr document + + # Now add the details from the nesting indexor to the document + solr_doc[solr_field_name_for_storing_ancestors] = ancestors + solr_doc[solr_field_name_for_storing_parent_ids] = parent_ids + solr_doc[solr_field_name_for_storing_pathnames] = pathnames + ActiveFedora::SolrService.add(solr_doc, commit: true) + solr_doc + end end # @api public diff --git a/spec/factories/collections_factory.rb b/spec/factories/collections_factory.rb index c8970286ee..ecb5c52812 100644 --- a/spec/factories/collections_factory.rb +++ b/spec/factories/collections_factory.rb @@ -18,11 +18,25 @@ end after(:create) do |collection, evaluator| - if evaluator.with_permission_template + # create the permission template if it was requested, OR if + # nested reindexing is included (so we can apply the user's + # permissions). + if evaluator.with_permission_template || RSpec.current_example.metadata[:with_nested_reindexing] attributes = { source_id: collection.id, source_type: 'collection' } attributes = evaluator.with_permission_template.merge(attributes) if evaluator.with_permission_template.respond_to?(:merge) create(:permission_template, attributes) unless Hyrax::PermissionTemplate.find_by(source_id: collection.id) end + # Nested indexing requires that the user's permissions be saved + # on the Fedora object... if simply in local memory, they are + # lost when the adapter pulls the object from Fedora to reindex. + if RSpec.current_example.metadata[:with_nested_reindexing] + create(:permission_template_access, + :manage, + permission_template: collection.permission_template, + agent_type: 'user', + agent_id: evaluator.user.user_key) + collection.update_access_controls! + end end factory :public_collection, traits: [:public] diff --git a/spec/factories/generic_works.rb b/spec/factories/generic_works.rb index 37b60c09c7..41210136d8 100644 --- a/spec/factories/generic_works.rb +++ b/spec/factories/generic_works.rb @@ -18,6 +18,10 @@ end end + after(:create) do |work, _evaluator| + work.save! if work.member_of_collections.present? + end + title ["Test title"] visibility Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE diff --git a/spec/models/collection_spec.rb b/spec/models/collection_spec.rb index 392a986f2b..d39081bcf0 100644 --- a/spec/models/collection_spec.rb +++ b/spec/models/collection_spec.rb @@ -243,13 +243,27 @@ class Member < ActiveFedora::Base end end - describe 'factories' do - it 'will create a permission_template when one is requested' do - expect { create(:collection, with_permission_template: true) }.to change { Hyrax::PermissionTemplate.count }.by(1) + context 'collection factory' do + describe 'when creating permission templates' do + it 'will create a permission_template when one is requested' do + expect { create(:collection, with_permission_template: true) }.to change { Hyrax::PermissionTemplate.count }.by(1) + end + + it 'will not create a permission_template by default' do + expect { create(:collection) }.not_to change { Hyrax::PermissionTemplate.count } + end end - it 'will not create a permission_template by default' do - expect { create(:collection) }.not_to change { Hyrax::PermissionTemplate.count } + describe 'when including nesting indexing', with_nested_indexing: true do + # Nested indexing requires that the user's permissions be saved + # on the Fedora object... if simply in local memory, they are + # lost when the adapter pulls the object from Fedora to reindex. + let(:user) { create(:user) } + let(:collection) { create(:collection, user: user) } + + it 'will authorize the creating user' do + expect(user.can?(:edit, collection)).to be true + end end end