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

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

Merged
merged 4 commits into from
Jul 13, 2022
Merged
Show file tree
Hide file tree
Changes from 3 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
12 changes: 6 additions & 6 deletions .github/workflows/ruby.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/models/search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
class Search < ApplicationRecord
belongs_to :user, optional: true

serialize :query_params
serialize :query_params, Blacklight::SearchParamsYamlCoder
Copy link
Member

@jrochkind jrochkind Jul 13, 2022

Choose a reason for hiding this comment

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

Maybe a comment explaining why this is necessary, and that it becomes no longer necessary if in a Rails version including rails/rails#45591

(If that rails gets merged, as it looks like it will, then at some future point where BL only supports Rails versions where the latest patch includes that rails change, this custom coder will no longer be necessary)

Or the comment could be down with the custom coder class itself, maybe.

cbeer marked this conversation as resolved.
Show resolved Hide resolved

# A Search instance is considered a saved search if it has a user_id.
def saved?
Expand Down
48 changes: 48 additions & 0 deletions app/services/blacklight/search_params_yaml_coder.rb
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions lib/blacklight/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
47 changes: 33 additions & 14 deletions spec/models/search_spec.rb
Original file line number Diff line number Diff line change
@@ -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

Expand Down