Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate FindObjectsViaSolrService replaced by SolrQueryService#get_objects #5023

Merged
merged 3 commits into from
Jul 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions app/services/hyrax/find_objects_via_solr_service.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
# frozen_string_literal: true
module Hyrax
# @deprecated This class is being replaced by Hyrax::SolrQueryService #get_objects method.
#
# Methods in this class search solr to get the ids and then use the query service to find the objects.
class FindObjectsViaSolrService
class_attribute :solr_query_builder, :solr_service, :query_service
self.solr_query_builder = Hyrax::SolrQueryBuilderService
self.solr_query_builder = Hyrax::SolrQueryService
self.solr_service = Hyrax::SolrService
self.query_service = Hyrax.query_service

Expand All @@ -14,13 +16,15 @@ class << self
# @param join_with [String] the value we're joining the clauses with (default: ' OR ' for backward compatibility with ActiveFedora where)
# @param type [String] The type of query to run. Either 'raw' or 'field' (default: 'field')
# @param use_valkyrie [Boolean] if true, return Valkyrie resource(s); otherwise, return ActiveFedora object(s)
# @return [Array<ActiveFedora|Valkyrie::Resource>] objects matching the query
# @return [Array<ActiveFedora::Base|Valkyrie::Resource>] objects matching the query
def find_for_model_by_field_pairs(model:, field_pairs:, join_with: ' OR ', type: 'field', use_valkyrie: Hyrax.config.use_valkyrie?)
return model.where(field_pairs).to_a unless use_valkyrie
query = solr_query_builder.construct_query_for_model(model, field_pairs, join_with, type)
results = solr_service.query(query)
ids = results.map(&:id)
query_service.find_many_by_ids(ids: ids).to_a
Deprecation.warn("'##{__method__}' will be removed in Hyrax 4.0. " \
"Instead, use 'Hyrax::SolrQueryService.new.with_model(...).with_field_pairs(...).get_objects'.")

solr_query_builder.new
.with_model(model: model)
.with_field_pairs(field_pairs: field_pairs, join_with: join_with, type: type)
.get_objects(use_valkyrie: use_valkyrie).to_a
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FindObjectViaSolrService #find_for_model_by_field_pairs needs to continue to return an Array for backward compatibility.

This method is no longer used in any Hyrax code.

end
end
end
Expand Down
7 changes: 5 additions & 2 deletions app/services/hyrax/multiple_membership_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,12 @@ def single_membership_collections(collection_ids)

field_pairs = {
:id => collection_ids,
Hyrax.config.collection_type_index_field.to_sym => collection_type_gids_that_disallow_multiple_membership
Hyrax.config.collection_type_index_field.to_sym => collection_type_gids_that_disallow_multiple_membership&.map(&:to_s)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map to strings is required to turn GlobalIDs into string IDs. I'm not sure why this worked before. It definitely does not work with the SolrQueryService.

}
Hyrax::FindObjectsViaSolrService.find_for_model_by_field_pairs(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true)
Hyrax::SolrQueryService.new
.with_model(model: ::Collection)
.with_field_pairs(field_pairs: field_pairs, join_with: ' OR ')
.get_objects(use_valkyrie: true).to_a
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result is required to be an Array because later it is used in...

collections_to_check |= single_membership_collections(item.member_of_collection_ids)

where the result is collections_to_check.

end

def collection_type_gids_that_disallow_multiple_membership
Expand Down
18 changes: 18 additions & 0 deletions app/services/hyrax/solr_query_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ module Hyrax
# Methods in this class are providing functionality previously supported by ActiveFedora::SolrQueryBuilder.
# It includes methods to build and execute a query.
class SolrQueryService
class_attribute :query_service
self.query_service = Hyrax.query_service

attr_reader :query, :solr_service

def initialize(query: [], solr_service: Hyrax::SolrService)
Expand All @@ -17,6 +20,21 @@ def get
solr_service.get(build)
end

##
# @return [Array<String>] ids of documents matching the current query
def get_ids # rubocop:disable Naming/AccessorMethodName
results = get
results['response']['docs'].map { |doc| doc['id'] }
end

##
# @return [Array<Valkyrie::Resource|ActiveFedora::Base>] objects matching the current query
def get_objects(use_valkyrie: Hyrax.config.use_valkyrie?)
ids = get_ids
return ids.map { |id| ActiveFedora::Base.find(id) } unless use_valkyrie
query_service.find_many_by_ids(ids: ids)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed .to_a on the returned result to maintain the Enumerable.

end

##
# @return [Integer] the number of results that match the query in solr
def count
Expand Down
65 changes: 41 additions & 24 deletions spec/services/hyrax/multiple_membership_checker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@
let(:field_pairs) do
{
id: collection_ids,
collection_type_gid_ssim: collection_type_gids
collection_type_gid_ssim: collection_type_gids.map(&:to_s)
}
end
let(:field_pairs_for_col2) do
{
id: [collection2.id],
collection_type_gid_ssim: collection_type_gids
collection_type_gid_ssim: collection_type_gids.map(&:to_s)
}
end
let(:use_valkyrie) { true }
Expand All @@ -49,15 +49,15 @@

it 'returns nil' do
expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_call_original
expect(Hyrax::FindObjectsViaSolrService).not_to receive(:find_for_model_by_field_pairs)
expect(Hyrax::SolrQueryService).not_to receive(:new)
expect(subject).to be nil
end
end

context 'when there are no single-membership collection instances' do
it 'returns nil' do
expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_return([])
expect(Hyrax::FindObjectsViaSolrService).not_to receive(:find_for_model_by_field_pairs)
expect(Hyrax::SolrQueryService).not_to receive(:new)
expect(subject).to be nil
end
end
Expand All @@ -69,8 +69,10 @@

it 'returns nil' do
expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_call_original
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true).once.and_return(collections)
expect(Hyrax::SolrQueryService).to receive(:new).and_return(inst = double)
expect(inst).to receive(:with_model).with(model: ::Collection).once.and_return(inst_with_model = double)
expect(inst_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs, join_with: ' OR ').once.and_return(inst_with_full_query = double)
expect(inst_with_full_query).to receive(:get_objects).with(use_valkyrie: true).once.and_return(collections)
expect(subject).to be nil
end
end
Expand All @@ -84,8 +86,10 @@
it 'returns an error' do
expect(item).not_to receive(:member_of_collection_ids)
expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_call_original
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true).once.and_return(collections)
expect(Hyrax::SolrQueryService).to receive(:new).and_return(inst = double)
expect(inst).to receive(:with_model).with(model: ::Collection).once.and_return(inst_with_model = double)
expect(inst_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs, join_with: ' OR ').once.and_return(inst_with_full_query = double)
expect(inst_with_full_query).to receive(:get_objects).with(use_valkyrie: true).once.and_return(collections)
expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection type (type: Greedy, collections: Foo and Bar)'
end

Expand All @@ -96,8 +100,10 @@
it 'returns an error' do
expect(item).not_to receive(:member_of_collection_ids)
expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_call_original
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true).once.and_return(collections)
expect(Hyrax::SolrQueryService).to receive(:new).and_return(inst = double)
expect(inst).to receive(:with_model).with(model: ::Collection).once.and_return(inst_with_model = double)
expect(inst_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs, join_with: ' OR ').once.and_return(inst_with_full_query = double)
expect(inst_with_full_query).to receive(:get_objects).with(use_valkyrie: true).once.and_return(collections)
expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection type (type: Greedy, collections: Foo and Bar)'
end
end
Expand All @@ -116,10 +122,13 @@

it 'returns an error' do
expect(item).to receive(:member_of_collection_ids)
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true).once.and_return(collections)
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs_for_col2, use_valkyrie: true).once.and_return([collection2])
expect(Hyrax::SolrQueryService).to receive(:new).and_return(inst1 = double, inst2 = double)
expect(inst1).to receive(:with_model).with(model: ::Collection).and_return(inst1_with_model = double)
expect(inst1_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs, join_with: ' OR ').and_return(inst1_with_full_query = double)
expect(inst1_with_full_query).to receive(:get_objects).with(use_valkyrie: true).and_return(collections)
expect(inst2).to receive(:with_model).with(model: ::Collection).and_return(inst2_with_model = double)
expect(inst2_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs_for_col2, join_with: ' OR ').and_return(inst2_with_full_query = double)
expect(inst2_with_full_query).to receive(:get_objects).with(use_valkyrie: true).and_return([collection2])
expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection type (type: Greedy, collections: Foo and Bar)'
end

Expand All @@ -129,10 +138,13 @@

it 'returns an error' do
expect(item).to receive(:member_of_collection_ids)
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true).once.and_return(collections)
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs_for_col2, use_valkyrie: true).once.and_return([collection2])
expect(Hyrax::SolrQueryService).to receive(:new).and_return(inst1 = double, inst2 = double)
expect(inst1).to receive(:with_model).with(model: ::Collection).and_return(inst1_with_model = double)
expect(inst1_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs, join_with: ' OR ').and_return(inst1_with_full_query = double)
expect(inst1_with_full_query).to receive(:get_objects).with(use_valkyrie: true).and_return(collections)
expect(inst2).to receive(:with_model).with(model: ::Collection).and_return(inst2_with_model = double)
expect(inst2_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs_for_col2, join_with: ' OR ').and_return(inst2_with_full_query = double)
expect(inst2_with_full_query).to receive(:get_objects).with(use_valkyrie: true).and_return([collection2])
expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection type (type: Greedy, collections: Foo and Bar)'
end
end
Expand All @@ -149,8 +161,10 @@
it 'returns nil' do
expect(item).not_to receive(:member_of_collection_ids)
expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_call_original
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true).once.and_return(collections)
expect(Hyrax::SolrQueryService).to receive(:new).and_return(inst = double)
expect(inst).to receive(:with_model).with(model: ::Collection).once.and_return(inst_with_model = double)
expect(inst_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs, join_with: ' OR ').once.and_return(inst_with_full_query = double)
expect(inst_with_full_query).to receive(:get_objects).with(use_valkyrie: true).once.and_return(collections)
expect(subject).to be nil
end

Expand All @@ -164,10 +178,13 @@

it 'returns nil' do
expect(item).to receive(:member_of_collection_ids)
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true).once.and_return(collections)
expect(Hyrax::FindObjectsViaSolrService).to receive(:find_for_model_by_field_pairs)
.with(model: ::Collection, field_pairs: field_pairs_for_col2, use_valkyrie: true).once.and_return([collection2])
expect(Hyrax::SolrQueryService).to receive(:new).and_return(inst1 = double, inst2 = double)
expect(inst1).to receive(:with_model).with(model: ::Collection).and_return(inst1_with_model = double)
expect(inst1_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs, join_with: ' OR ').and_return(inst1_with_full_query = double)
expect(inst1_with_full_query).to receive(:get_objects).with(use_valkyrie: true).and_return(collections)
expect(inst2).to receive(:with_model).with(model: ::Collection).and_return(inst2_with_model = double)
expect(inst2_with_model).to receive(:with_field_pairs).with(field_pairs: field_pairs_for_col2, join_with: ' OR ').and_return(inst2_with_full_query = double)
expect(inst2_with_full_query).to receive(:get_objects).with(use_valkyrie: true).and_return([collection2])
expect(subject).to be nil
end
end
Expand Down
42 changes: 42 additions & 0 deletions spec/services/hyrax/solr_query_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,48 @@
end
end

describe '#get_ids' do
let!(:work1) { create(:work, id: 'wk1', creator: ['Mark']) }
let!(:work2) { create(:work, id: 'wk2', creator: ['Fred']) }
let!(:work3) { create(:work, id: 'wk3', creator: ['Mark']) }
subject(:solr_query_service) { described_class.new }

before { solr_query_service.with_field_pairs(field_pairs: { creator_tesim: 'Mark' }) }

it 'get ids for solr document matching the query' do
ids = solr_query_service.get_ids
expect(ids.count).to eq 2
expect(ids).to match_array ['wk1', 'wk3']
end
end

describe '#get_objects' do
let!(:work1) { create(:work, id: 'wk1', creator: ['Mark']) }
let!(:work2) { create(:work, id: 'wk2', creator: ['Fred']) }
let!(:work3) { create(:work, id: 'wk3', creator: ['Mark']) }
subject(:solr_query_service) { described_class.new }

before { solr_query_service.with_field_pairs(field_pairs: { creator_tesim: 'Mark' }) }

context "when use_valkyrie is false" do
it 'get ActiveFedora::Base objects matching the query' do
objects = solr_query_service.get_objects(use_valkyrie: false)
expect(objects.count).to eq 2
expect(objects.first).to be_kind_of ActiveFedora::Base
expect(objects.map(&:id)).to match_array ['wk1', 'wk3']
end
end

context "when use_valkyrie is true" do
it 'get Valkyrie::Resource objects matching the query' do
objects = solr_query_service.get_objects(use_valkyrie: true)
expect(objects.count).to eq 2
expect(objects.first).to be_kind_of Valkyrie::Resource
expect(objects.map(&:id)).to match_array ['wk1', 'wk3']
end
end
end

describe '#count' do
let!(:work1) { create(:work, id: 'wk1', creator: ['Mark']) }
let!(:work2) { create(:work, id: 'wk2', creator: ['Fred']) }
Expand Down