Merge pull request #2770 from projectblacklight/yaml-column-encoder

Use a custom YAML coder to restore backwards-compatible deserialization of serialized query parameters

.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

app/models/search.rb

@@ -3,7 +3,8 @@
 class Search < ApplicationRecord
   belongs_to :user, optional: true
 
-  serialize :query_params
+  # 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.
   def saved?

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

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:

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: '[email protected]', 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