From 78abeaa608c54c06d215e221c72ed5918b23c202 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Wed, 7 Feb 2024 14:50:21 -0500 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=90=9B=20Restore=20`qt:=20'standard'`?= =?UTF-8?q?=20to=20SolrService?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In `ActiveFedora::SolrService` we always assign the `qt: 'standard'`.[1] However, with [this old commit][1] we dropped passing the `qt: 'standard'` when we had `use_valkyrie` of true. We discovered this bug when upgrading Hyku and Bulkrax to Hyrax v3.x and some. Closes https://github.com/samvera/hyrax/issues/6676 Related to: - https://github.com/samvera/hyrax/pull/3857 [1]:https://github.com/samvera/active_fedora/blob/8e7d365a087974b4ff9b9217f792c0c049789de6/lib/active_fedora/solr_service.rb#L40-L48 [2]:https://github.com/samvera/hyrax/commit/be6104fd13117f6a6c5b0252acd5b341247f04d3 --- app/services/hyrax/solr_service.rb | 6 ++-- spec/services/hyrax/solr_service_spec.rb | 36 +++++++++--------------- 2 files changed, 15 insertions(+), 27 deletions(-) diff --git a/app/services/hyrax/solr_service.rb b/app/services/hyrax/solr_service.rb index 5527fe28d2..60861be9e8 100644 --- a/app/services/hyrax/solr_service.rb +++ b/app/services/hyrax/solr_service.rb @@ -46,9 +46,8 @@ def select_path def get(query = nil, **args) # Make Hyrax.config.solr_select_path the default SOLR path solr_path = args.delete(:path) || Hyrax.config.solr_select_path - args = args.merge(q: query) if query.present? + args = args.merge(q: query, qt: 'standard') if query.present? - args = args.merge(qt: 'standard') unless query.blank? || use_valkyrie connection.get(solr_path, params: args) end @@ -66,9 +65,8 @@ def ping def post(query = nil, **args) # Make Hyrax.config.solr_select_path the default SOLR path solr_path = args.delete(:path) || Hyrax.config.solr_select_path - args = args.merge(q: query) if query.present? + args = args.merge(q: query, qt: 'standard') if query.present? - args = args.merge(qt: 'standard') unless query.blank? || use_valkyrie connection.post(solr_path, data: args) end diff --git a/spec/services/hyrax/solr_service_spec.rb b/spec/services/hyrax/solr_service_spec.rb index 3665bcd928..a240e5a639 100644 --- a/spec/services/hyrax/solr_service_spec.rb +++ b/spec/services/hyrax/solr_service_spec.rb @@ -15,7 +15,7 @@ describe "#get" do it "calls solr" do stub_result = double("Result") - params = use_valkyrie ? { q: 'querytext' } : { q: 'querytext', qt: 'standard' } + params = { q: 'querytext', qt: 'standard' } expect(mock_conn).to receive(:get).with('select', params: params).and_return(stub_result) allow(described_class).to receive(:instance).and_return(double("instance", conn: mock_conn)) expect(described_class.get('querytext')).to eq stub_result @@ -23,21 +23,11 @@ it "uses args as params" do stub_result = double("Result") - params = use_valkyrie ? { fq: ["id:\"1234\""], q: 'querytext' } : { fq: ["id:\"1234\""], q: 'querytext', qt: 'standard' } + params = { fq: ["id:\"1234\""], q: 'querytext', qt: 'standard' } expect(mock_conn).to receive(:get).with('select', params: params).and_return(stub_result) allow(described_class).to receive(:instance).and_return(double("instance", conn: mock_conn)) expect(described_class.get('querytext', fq: ["id:\"1234\""])).to eq stub_result end - - context "when use_valkyrie: true" do - subject(:service) { described_class.new(use_valkyrie: true) } - - it "uses valkyrie solr based on config query_index_from_valkyrie" do - stub_result = double("Valkyrie Result") - expect(mock_conn).to receive(:get).with('select', params: { q: 'querytext' }).and_return(stub_result) - expect(service.get('querytext')).to eq stub_result - end - end end describe '#ping' do @@ -64,7 +54,7 @@ describe "#post" do it "calls solr" do stub_result = double("Result") - data = use_valkyrie ? { q: 'querytext' } : { q: 'querytext', qt: 'standard' } + data = { q: 'querytext', qt: 'standard' } expect(mock_conn).to receive(:post).with('select', data: data).and_return(stub_result) allow(described_class).to receive(:instance).and_return(double("instance", conn: mock_conn)) expect(described_class.post('querytext')).to eq stub_result @@ -72,7 +62,7 @@ it "uses args as data" do stub_result = double("Result") - data = use_valkyrie ? { fq: ["id:\"1234\""], q: 'querytext' } : { fq: ["id:\"1234\""], q: 'querytext', qt: 'standard' } + data = { fq: ["id:\"1234\""], q: 'querytext', qt: 'standard' } expect(mock_conn).to receive(:post).with('select', data: data).and_return(stub_result) allow(described_class).to receive(:instance).and_return(double("instance", conn: mock_conn)) expect(described_class.post('querytext', fq: ["id:\"1234\""])).to eq stub_result @@ -84,7 +74,7 @@ it "uses valkyrie solr based on config query_index_from_valkyrie" do stub_result = double("Valkyrie Result") - expect(mock_conn).to receive(:post).with('select', data: { q: 'querytext' }).and_return(stub_result) + expect(mock_conn).to receive(:post).with('select', data: { q: 'querytext', qt: 'standard' }).and_return(stub_result) expect(service.post('querytext')).to eq stub_result end @@ -101,25 +91,25 @@ end it "defaults to HTTP POST method" do - data = use_valkyrie ? { rows: 2, q: 'querytext' } : { rows: 2, q: 'querytext', qt: 'standard' } + data = { rows: 2, q: 'querytext', qt: 'standard' } expect(mock_conn).to receive(:post).with('select', data: data).and_return(stub_result) described_class.query('querytext', rows: 2) end it "allows callers to specify HTTP GET method" do - params = use_valkyrie ? { rows: 2, q: 'querytext' } : { rows: 2, q: 'querytext', qt: 'standard' } + params = { rows: 2, q: 'querytext', qt: 'standard' } expect(mock_conn).to receive(:get).with('select', params: params).and_return(stub_result) described_class.query('querytext', rows: 2, method: :get) end it "allows callers to specify HTTP POST method" do - data = use_valkyrie ? { rows: 2, q: 'querytext' } : { rows: 2, q: 'querytext', qt: 'standard' } + data = { rows: 2, q: 'querytext', qt: 'standard' } expect(mock_conn).to receive(:post).with('select', data: data).and_return(stub_result) described_class.query('querytext', rows: 2, method: :post) end it "raises if method not GET or POST" do - data = use_valkyrie ? { rows: 2, q: 'querytext' } : { rows: 2, q: 'querytext', qt: 'standard' } + data = { rows: 2, q: 'querytext', qt: 'standard' } expect(mock_conn).not_to receive(:head).with('select', data: data) expect do described_class.query('querytext', rows: 2, method: :head) @@ -127,7 +117,7 @@ end it "wraps the solr response documents in Solr hits" do - data = use_valkyrie ? { rows: 2, q: 'querytext' } : { rows: 2, q: 'querytext', qt: 'standard' } + data = { rows: 2, q: 'querytext', qt: 'standard' } expect(mock_conn).to receive(:post).with('select', data: data).and_return(stub_result) result = described_class.query('querytext', rows: 2) expect(result.size).to eq 1 @@ -146,7 +136,7 @@ let(:doc) { { 'id' => 'valkyrie-x' } } it "uses valkyrie solr based on config query_index_from_valkyrie" do - expect(mock_conn).to receive(:post).with('select', data: { q: 'querytext' }).and_return(stub_result) + expect(mock_conn).to receive(:post).with('select', data: { q: 'querytext', qt: 'standard' }).and_return(stub_result) result = service.query('querytext') expect(result.first.id).to eq 'valkyrie-x' @@ -268,7 +258,7 @@ let(:stub_result) { { 'response' => { 'numFound' => '2' } } } it "calls solr" do - data = use_valkyrie ? { rows: 0, q: 'querytext' } : { rows: 0, q: 'querytext', qt: 'standard' } + data = { rows: 0, q: 'querytext', qt: 'standard' } expect(mock_conn).to receive(:post).with('select', data: data).and_return(stub_result) allow(described_class).to receive(:instance).and_return(double("instance", conn: mock_conn)) expect(described_class.count('querytext')).to eq 2 @@ -278,7 +268,7 @@ subject(:service) { described_class.new(use_valkyrie: true) } it "uses valkyrie solr based on config query_index_from_valkyrie" do - expect(mock_conn).to receive(:post).with('select', data: { rows: 0, q: 'querytext' }).and_return(stub_result) + expect(mock_conn).to receive(:post).with('select', data: { rows: 0, q: 'querytext', qt: 'standard' }).and_return(stub_result) expect(service.count('querytext')).to eq 2 end end From 67b872e7bab5db46be88c8e4b214182daf616836 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Tue, 6 Feb 2024 14:37:50 -0500 Subject: [PATCH 2/3] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Attempt=20to=20squash?= =?UTF-8?q?=20a=20flaky=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also remove an un-tested element. --- spec/features/default_workflow_spec.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/spec/features/default_workflow_spec.rb b/spec/features/default_workflow_spec.rb index 15b9924a94..90995bd24a 100644 --- a/spec/features/default_workflow_spec.rb +++ b/spec/features/default_workflow_spec.rb @@ -12,6 +12,8 @@ let(:attributes) { :LEGACY_UNUSED_ARGUMENT_WITH_NO_KNOWN_USE_CASE_SHOULD_NEVER_BE_REQUIRED } let(:workflow_factory) { Hyrax::Workflow::WorkflowFactory } + before { Hyrax::EnsureWellFormedAdminSetService.call } + it 'sets state to "deposited"' do workflow_factory.create(work, attributes, depositor) @@ -34,9 +36,5 @@ .to include(have_attributes(mode: :edit, agent: depositor.user_key)) end end - - context 'with members' do - it 'grants edit to depositor on members in background' - end end end From 0fed6dbb161fbf61d6a48ac52df7e18757658e2d Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 8 Feb 2024 09:47:11 -0500 Subject: [PATCH 3/3] Adding configurability to Hyrax::SolrService --- app/services/hyrax/solr_service.rb | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/app/services/hyrax/solr_service.rb b/app/services/hyrax/solr_service.rb index 60861be9e8..fca49d05f2 100644 --- a/app/services/hyrax/solr_service.rb +++ b/app/services/hyrax/solr_service.rb @@ -9,6 +9,22 @@ module Hyrax class SolrService COMMIT_PARAMS = { softCommit: true }.freeze + ## + # @!group Class Attributes + # + # @!attribute additional_solr_get_and_post_params + # + # ActiveFedora::SolrService always assumed that it would pass `qt: 'standard'`. However, as + # Valkyrie arrived, we have a case where we would not pass the `qt: standard`. We're favoring + # a default value to preserve prior expectations that ActiveFedora::SolrService set and to not + # suddenly break those that switch to Valkyrie and Hyrax::SolrService. + # + # @return [Hash] + # @see https://github.com/samvera/hyrax/pull/6677 + class_attribute :additional_solr_get_and_post_params, default: { qt: 'standard' } + # @!endgroup Class Attributes + ## + ## # @!attribute [r] use_valkyrie # @private @@ -46,7 +62,7 @@ def select_path def get(query = nil, **args) # Make Hyrax.config.solr_select_path the default SOLR path solr_path = args.delete(:path) || Hyrax.config.solr_select_path - args = args.merge(q: query, qt: 'standard') if query.present? + args = args.merge(additional_solr_get_and_post_params.merge(q: query)) if query.present? connection.get(solr_path, params: args) end @@ -65,7 +81,7 @@ def ping def post(query = nil, **args) # Make Hyrax.config.solr_select_path the default SOLR path solr_path = args.delete(:path) || Hyrax.config.solr_select_path - args = args.merge(q: query, qt: 'standard') if query.present? + args = args.merge(additional_solr_get_and_post_params.merge(q: query)) if query.present? connection.post(solr_path, data: args) end