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

replace ActiveFedora where with Valkyrized where #4910

Merged
merged 2 commits into from
May 6, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
27 changes: 27 additions & 0 deletions app/services/hyrax/find_objects_via_solr_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# frozen_string_literal: true
module Hyrax
# 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_service = Hyrax::SolrService
self.query_service = Hyrax.query_service

class << self
# Find objects matching search criteria.
# @param model [Class]
# @param field_pairs [Hash] a list of pairs of property name and values
# @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 [ActiveFedora | 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?)
query = solr_query_builder.construct_query_for_model(model, field_pairs, join_with, type)
results = solr_service.query(query)
ids = results.map(&:id)
return query_service.find_many_by_ids(ids: ids) if use_valkyrie
ids.map { |id| query_service.find_by_alternate_identifier(alternate_identifier: id.to_str, use_valkyrie: false) }
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the advantage of using the valkyrie query service?

would there be fewer risks if we stuck with the the existing ActiveFedora::Base#where implementation in the case where what we want is to query AF objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated to use AF where unless use_valkyrie is true.

end
end
end
end
10 changes: 7 additions & 3 deletions app/services/hyrax/multiple_membership_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,15 @@ def check(collection_ids:, include_current_members: false)
def single_membership_collections(collection_ids)
return [] if collection_ids.blank?

::Collection.where(:id => collection_ids, Hyrax.config.collection_type_index_field.to_sym => collection_type_gids_that_disallow_multiple_membership)
field_pairs = {
:id => collection_ids,
Hyrax.config.collection_type_index_field.to_sym => collection_type_gids_that_disallow_multiple_membership
}
Hyrax::FindObjectsViaSolrService.find_for_model_by_field_pairs(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true)
end

def collection_type_gids_that_disallow_multiple_membership
Hyrax::CollectionType.gids_that_do_not_allow_multiple_membership
Hyrax::CollectionType.gids_that_do_not_allow_multiple_membership.map(&:to_s)
Copy link
Contributor

Choose a reason for hiding this comment

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

what's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests initially failed when refactoring for the new approach which required this change. But the final refactor really doesn't need this. So I set it back to what it was.

end

def build_error_message(problematic_collections)
Expand All @@ -74,7 +78,7 @@ def collection_type_title_from_gid(gid)
end

def collection_titles_from_list(collection_list)
collection_list.each do |collection|
collection_list.map do |collection|
collection.title.first
end.to_sentence
end
Expand Down
20 changes: 17 additions & 3 deletions app/services/hyrax/solr_query_builder_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ def construct_query_for_ids(id_array)
end

# Construct a solr query from a list of pairs (e.g. [field name, values])
# @param [Array<Array>] field_pairs a list of pairs of property name and values
# @param [String] join_with ('AND') the value we're joining the clauses with
# @param [String] type ('field') The type of query to run. Either 'raw' or 'field'
# @param [Hash] field_pairs a list of pairs of property name and values
# @param [String] join_with the value we're joining the clauses with (default: ' AND ')
# @param [String] type of query to run. Either 'raw' or 'field' (default: 'field')
# @return [String] a solr query
# @example
# construct_query([['library_id_ssim', '123'], ['owner_ssim', 'Fred']])
Expand All @@ -32,6 +32,20 @@ def default_join_with
' AND '
end

# Construct a solr query from a list of pairs (e.g. [field name, values]) including the model (e.g. Collection, Monograph)
# @param [Class] model class
# @param [Hash] field_pairs a list of pairs of property name and values
# @param [String] join_with the value we're joining the clauses with (default: ' AND ')
# @param [String] type of query to run. Either 'raw' or 'field' (default: 'field')
# @return [String] a solr query
# @example
# construct_query(Collection, [['library_id_ssim', '123'], ['owner_ssim', 'Fred']])
# # => "_query_:\"{!field f=has_model_ssim}Collection\" AND _query_:\"{!field f=library_id_ssim}123\" AND _query_:\"{!field f=owner_ssim}Fred\""
def construct_query_for_model(model, field_pairs, join_with = default_join_with, type = 'field')
field_pairs["has_model_ssim"] = model.to_s
construct_query(field_pairs, join_with, type)
end

private

# @param [Array<Array>] pairs a list of (key, value) pairs. The value itself may
Expand Down
20 changes: 20 additions & 0 deletions spec/services/hyrax/find_objects_via_solr_service_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# frozen_string_literal: true
require 'spec_helper'

RSpec.describe Hyrax::FindObjectsViaSolrService do
describe ".find_for_model_by_field_pairs", clean_repo: true do
let(:collection1) { valkyrie_create(:hyrax_collection, title: ['Foo']) }
let(:collection2) { valkyrie_create(:hyrax_collection, title: ['Too']) }
let(:collection_ids) { [collection1.id, collection2.id] }
let(:field_pairs) do
{ id: collection_ids.map(&:to_s) }
end

it "returns ActiveFedora objects matching the query" do
expect(described_class.find_for_model_by_field_pairs(model: ::Collection, field_pairs: field_pairs, use_valkyrie: false).map(&:title)).to match_array [['Foo'], ['Too']]
end
it "returns Valkyrie::Resources matching the query" do
expect(described_class.find_for_model_by_field_pairs(model: ::Collection, field_pairs: field_pairs, use_valkyrie: true).map(&:title)).to match_array [['Foo'], ['Too']]
end
end
end
70 changes: 45 additions & 25 deletions spec/services/hyrax/multiple_membership_checker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,22 @@
let(:collection_ids) { ['foobar'] }
let(:included) { false }
let!(:collection_type) { create(:collection_type, title: 'Greedy', allow_multiple_membership: false) }
let(:collection_type_gids) { [collection_type] }
let(:collection_types) { [collection_type] }
let(:collection_type_gids) { [collection_type.to_global_id] }
let(:field_pairs) do
{
id: collection_ids,
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.map(&:to_s)
}
end
let(:use_valkyrie) { true }

before do
allow(Hyrax::CollectionType).to receive(:gids_that_do_not_allow_multiple_membership).and_return(collection_type_gids)
end
Expand All @@ -34,46 +49,43 @@

it 'returns nil' do
expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_call_original
expect(Collection).not_to receive(:where)
expect(Hyrax::FindObjectsViaSolrService).not_to receive(:find_for_model_by_field_pairs)
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(Collection).not_to receive(:where)
expect(Hyrax::FindObjectsViaSolrService).not_to receive(:find_for_model_by_field_pairs)
expect(subject).to be nil
end
end

context 'when multiple single-membership collection instances are not in the list' do
let(:collection) { build(:collection_lw, id: 'collection0', collection_type: collection_type, with_solr_document: true) }
let(:collection) { create(:collection_lw, id: 'collection0', collection_type: collection_type, with_solr_document: true) }
let(:collections) { [collection] }
let(:collection_ids) { collections.map(&:id) }

it 'returns nil' do
expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_call_original
expect(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: collection_type_gids).once.and_return(collections)
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(subject).to be nil
end
end

context 'when multiple single-membership collection instances are in the list, not including current members' do
let(:collection1) { build(:collection_lw, id: 'collection1', title: ['Foo'], collection_type: collection_type, with_solr_document: true) }
let(:collection2) { build(:collection_lw, id: 'collection2', title: ['Bar'], collection_type: collection_type, with_solr_document: true) }
let(:collection1) { create(:collection_lw, id: 'collection1', title: ['Foo'], collection_type: collection_type, with_solr_document: true) }
let(:collection2) { create(:collection_lw, id: 'collection2', title: ['Bar'], collection_type: collection_type, with_solr_document: true) }
let(:collections) { [collection1, collection2] }
let(:collection_ids) { collections.map(&:id) }

before do
allow(Collection).to receive(:find).with(collection1.id).and_return(collection1)
allow(Collection).to receive(:find).with(collection2.id).and_return(collection2)
end

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(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: collection_type_gids).once.and_return(collections)
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(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 @@ -84,15 +96,16 @@
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(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: collection_type_gids).once.and_return(collections)
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(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
end

context 'when multiple single-membership collection instances are in the list, including current members' do
let(:collection1) { build(:collection_lw, id: 'collection1', title: ['Foo'], collection_type: collection_type, with_solr_document: true) }
let(:collection2) { build(:collection_lw, id: 'collection2', title: ['Bar'], collection_type: collection_type, with_solr_document: true) }
let(:collection1) { create(:collection_lw, id: 'collection1', title: ['Foo'], collection_type: collection_type, with_solr_document: true) }
let(:collection2) { create(:collection_lw, id: 'collection2', title: ['Bar'], collection_type: collection_type, with_solr_document: true) }
let(:collections) { [collection1] }
let(:collection_ids) { collections.map(&:id) }
let(:included) { true }
Expand All @@ -103,8 +116,10 @@

it 'returns an error' do
expect(item).to receive(:member_of_collection_ids)
expect(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: collection_type_gids).once.and_return(collections)
expect(Collection).to receive(:where).with(id: [collection2.id], collection_type_gid_ssim: collection_type_gids).once.and_return([collection2])
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(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 @@ -114,16 +129,18 @@

it 'returns an error' do
expect(item).to receive(:member_of_collection_ids)
expect(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: collection_type_gids).once.and_return(collections)
expect(Collection).to receive(:where).with(id: [collection2.id], collection_type_gid_ssim: collection_type_gids).once.and_return([collection2])
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(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
end

context 'when multiple single-membership collection instances are in the list, but are different collection types' do
let(:collection1) { build(:collection_lw, title: ['Foo'], collection_type: collection_type, with_solr_document: true) }
let(:collection2) { build(:collection_lw, title: ['Bar'], collection_type: collection_type_2, with_solr_document: true) }
let(:collection1) { create(:collection_lw, title: ['Foo'], collection_type: collection_type, with_solr_document: true) }
let(:collection2) { create(:collection_lw, title: ['Bar'], collection_type: collection_type_2, with_solr_document: true) }
let(:collections) { [collection1, collection2] }
let(:collection_ids) { collections.map(&:id) }
let(:collection_type_2) { create(:collection_type, title: 'Doc', allow_multiple_membership: false) }
Expand All @@ -132,7 +149,8 @@
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(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: collection_type_gids).once.and_return(collections)
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(subject).to be nil
end

Expand All @@ -146,8 +164,10 @@

it 'returns nil' do
expect(item).to receive(:member_of_collection_ids)
expect(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: collection_type_gids).once.and_return(collections)
expect(Collection).to receive(:where).with(id: [collection2.id], collection_type_gid_ssim: collection_type_gids).once.and_return([collection2])
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(subject).to be nil
end
end
Expand Down
11 changes: 9 additions & 2 deletions spec/services/hyrax/solr_query_builder_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,24 @@
require 'spec_helper'

RSpec.describe Hyrax::SolrQueryBuilderService do
describe '#construct_query_for_ids' do
describe '.construct_query_for_ids' do
it "generates a useable solr query from an array of Fedora ids" do
expect(described_class.construct_query_for_ids(["my:_ID1_", "my:_ID2_", "my:_ID3_"])).to eq '{!terms f=id}my:_ID1_,my:_ID2_,my:_ID3_'
end
it "returns a valid solr query even if given an empty array as input" do
expect(described_class.construct_query_for_ids([""])).to eq "id:NEVER_USE_THIS_ID"
end
end
describe "construct_query" do

describe ".construct_query" do
it "generates a query clause" do
expect(described_class.construct_query('id' => "my:_ID1_")).to eq '_query_:"{!field f=id}my:_ID1_"'
end
end

describe ".construct_query_for_model" do
it "generates a query clause" do
expect(described_class.construct_query_for_model(::Collection, 'id' => "my:_ID1_")).to eq '(_query_:"{!field f=id}my:_ID1_" AND _query_:"{!field f=has_model_ssim}Collection")'
end
end
end