From 1b8ed4af57250cfa9779416a2652a15c3b7352e2 Mon Sep 17 00:00:00 2001 From: Chris Beer Date: Wed, 13 Jul 2022 07:48:30 -0700 Subject: [PATCH 1/4] Use a custom YAML coder to restore backwards-compatible deserialization of serialzied query parameters --- app/models/search.rb | 2 +- .../blacklight/search_params_yaml_coder.rb | 48 +++++++++++++++++++ lib/blacklight/engine.rb | 2 + 3 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 app/services/blacklight/search_params_yaml_coder.rb diff --git a/app/models/search.rb b/app/models/search.rb index 9eae75c2e..c01e0a9ed 100644 --- a/app/models/search.rb +++ b/app/models/search.rb @@ -3,7 +3,7 @@ class Search < ApplicationRecord belongs_to :user, optional: true - serialize :query_params + serialize :query_params, Blacklight::SearchParamsYamlCoder # A Search instance is considered a saved search if it has a user_id. def saved? diff --git a/app/services/blacklight/search_params_yaml_coder.rb b/app/services/blacklight/search_params_yaml_coder.rb new file mode 100644 index 000000000..a5daffa79 --- /dev/null +++ b/app/services/blacklight/search_params_yaml_coder.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Blacklight + # This is a custom YAML coder for (de)serializing blacklight search parameters that + # supports deserializing HashWithIndifferentAccess parameters (as was historically done by Blacklight). + class SearchParamsYamlCoder + # Serializes an attribute value to a string that will be stored in the database. + def self.dump(obj) + # Convert HWIA to an ordinary hash so we have some hope of using the regular YAML encoder in the future + obj = obj.to_hash if obj.is_a?(ActiveSupport::HashWithIndifferentAccess) + + YAML.dump(obj) + end + + # Deserializes a string from the database to an attribute value. + def self.load(yaml) + return yaml unless yaml.is_a?(String) && yaml.start_with?("---") + + params = yaml_load(yaml) + + params.with_indifferent_access + end + + # rubocop:disable Security/YAMLLoad + if YAML.respond_to?(:unsafe_load) + def self.yaml_load(payload) + if ActiveRecord.try(:use_yaml_unsafe_load) || ActiveRecord::Base.try(:use_yaml_unsafe_load) + YAML.unsafe_load(payload) + else + permitted_classes = (ActiveRecord.try(:yaml_column_permitted_classes) || ActiveRecord::Base.try(:yaml_column_permitted_classes) || []) + + Blacklight::Engine.config.blacklight.search_params_permitted_classes + YAML.safe_load(payload, permitted_classes: permitted_classes, aliases: true) + end + end + else + def self.yaml_load(payload) + if ActiveRecord.try(:use_yaml_unsafe_load) || ActiveRecord::Base.try(:use_yaml_unsafe_load) + YAML.load(payload) + else + permitted_classes = (ActiveRecord.try(:yaml_column_permitted_classes) || ActiveRecord::Base.try(:yaml_column_permitted_classes) || []) + + Blacklight::Engine.config.blacklight.search_params_permitted_classes + YAML.safe_load(payload, permitted_classes: permitted_classes, aliases: true) + end + end + end + # rubocop:enable Security/YAMLLoad + end +end diff --git a/lib/blacklight/engine.rb b/lib/blacklight/engine.rb index efcb877a9..3cf0202e0 100644 --- a/lib/blacklight/engine.rb +++ b/lib/blacklight/engine.rb @@ -59,6 +59,8 @@ class Engine < Rails::Engine outer_window: 2 } + bl_global_config.search_params_permitted_classes = [ActiveSupport::HashWithIndifferentAccess, Symbol] + # Anything that goes into Blacklight::Engine.config is stored as a class # variable on Railtie::Configuration. we're going to encapsulate all the # Blacklight specific stuff in this single struct: From 367f1b6f12f5212ff875627b5010252f7e7000fb Mon Sep 17 00:00:00 2001 From: Benjamin Armintor Date: Wed, 13 Jul 2022 10:02:58 -0400 Subject: [PATCH 2/4] test against updated Rails dependencies --- .github/workflows/ruby.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ruby.yml b/.github/workflows/ruby.yml index 121710679..c521f2a37 100644 --- a/.github/workflows/ruby.yml +++ b/.github/workflows/ruby.yml @@ -76,11 +76,11 @@ jobs: - name: Install dependencies run: bundle install env: - RAILS_VERSION: 6.0.3.7 + RAILS_VERSION: 6.0.5.1 - name: Run tests run: bundle exec rake ci env: - RAILS_VERSION: 6.0.3.7 + RAILS_VERSION: 6.0.5.1 ENGINE_CART_RAILS_OPTIONS: '--skip-git --skip-listen --skip-spring --skip-keeps --skip-action-cable --skip-coffee --skip-test' test_rails5_2: runs-on: ubuntu-latest @@ -96,11 +96,11 @@ jobs: - name: Install dependencies run: bundle install env: - RAILS_VERSION: 5.2.4.6 + RAILS_VERSION: 5.2.8.1 - name: Run tests run: bundle exec rake ci env: - RAILS_VERSION: 5.2.4.6 + RAILS_VERSION: 5.2.8.1 ENGINE_CART_RAILS_OPTIONS: '--skip-git --skip-listen --skip-spring --skip-keeps --skip-action-cable --skip-coffee --skip-test' test_rails6_1: @@ -117,11 +117,11 @@ jobs: - name: Install dependencies run: bundle install env: - RAILS_VERSION: 6.1.5 + RAILS_VERSION: 6.1.6.1 - name: Run tests run: bundle exec rake ci env: - RAILS_VERSION: 6.1.5 + RAILS_VERSION: 6.1.6.1 ENGINE_CART_RAILS_OPTIONS: '--skip-git --skip-keeps --skip-action-cable --skip-test' api_test: runs-on: ubuntu-latest From d2c451953e88ddc2cb1f91761f0a2fa93a7d7dea Mon Sep 17 00:00:00 2001 From: Benjamin Armintor Date: Wed, 13 Jul 2022 11:32:12 -0400 Subject: [PATCH 3/4] update search_spec to cover query_params scenarios --- spec/models/search_spec.rb | 47 ++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/spec/models/search_spec.rb b/spec/models/search_spec.rb index 92aa27a7f..1fac93081 100644 --- a/spec/models/search_spec.rb +++ b/spec/models/search_spec.rb @@ -1,32 +1,51 @@ # frozen_string_literal: true RSpec.describe Search do + let(:search) { described_class.new(user: user) } let(:user) { User.create! email: 'xyz@example.com', password: 'xyz12345' } + let(:hash_params) { { q: "query", f: { facet: "filter" } } } + let(:query_params) { hash_params } describe "query_params" do - before do - @search = described_class.new(user: user) - @query_params = { q: "query", f: "facet" } + shared_examples "persisting query_params" do + it "can save and retrieve the hash" do + search.query_params = query_params + search.save! + expect(described_class.find(search.id).query_params).to eq query_params.with_indifferent_access + end end - it "can save and retrieve the hash" do - @search.query_params = @query_params - @search.save! - expect(described_class.find(@search.id).query_params).to eq @query_params + context "are an indifferent hash" do + include_context "persisting query_params" do + let(:query_params) { hash_params.with_indifferent_access } + end + end + + context "are a string-keyed hash" do + include_context "persisting query_params" do + let(:query_params) { hash_params.with_indifferent_access.to_hash } + end + end + + context "include symbol keys" do + include_context "persisting query_params" do + let(:query_params) { hash_params } + end end end describe "saved?" do it "is true when user_id is not NULL and greater than 0" do - @search = described_class.new(user: user) - @search.save! - - expect(@search).to be_saved + search.save! + expect(search).to be_saved end - it "is false when user_id is NULL or less than 1" do - @search = described_class.create - expect(@search).not_to be_saved + context "when user_id is NULL or less than 1" do + let(:search) { described_class.create } + + it "is false" do + expect(search).not_to be_saved + end end end From 5abe70d4d3d63d300928a22c6541224963696fda Mon Sep 17 00:00:00 2001 From: Chris Beer Date: Wed, 13 Jul 2022 10:24:41 -0700 Subject: [PATCH 4/4] Update app/models/search.rb Co-authored-by: Benjamin Armintor --- app/models/search.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/search.rb b/app/models/search.rb index c01e0a9ed..02f50edaa 100644 --- a/app/models/search.rb +++ b/app/models/search.rb @@ -3,6 +3,7 @@ class Search < ApplicationRecord belongs_to :user, optional: true + # use a backwards-compatible serializer until the Rails API stabilizes and we can evaluate for major-revision compatibility serialize :query_params, Blacklight::SearchParamsYamlCoder # A Search instance is considered a saved search if it has a user_id.