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

Conversation

cbeer
Copy link
Member

@cbeer cbeer commented Jul 13, 2022

Fixes #2768

@cbeer cbeer force-pushed the yaml-column-encoder branch from 764cbbd to 2050d9d Compare July 13, 2022 14:57
@cbeer cbeer force-pushed the yaml-column-encoder branch 2 times, most recently from c6f1c3e to b0c7286 Compare July 13, 2022 15:42
@cbeer cbeer force-pushed the yaml-column-encoder branch from b0c7286 to efb1264 Compare July 13, 2022 15:44
@barmintor
Copy link
Contributor

The Rails 6.1 failure is surprising - I just checked the Rails patch files and it looks like the config is exposed on ActiveRecord::Base.use_yaml_unsafe_load and not ActiveRecord.use_yaml_unsafe_load. Suggested a change.

@cbeer cbeer force-pushed the yaml-column-encoder branch 3 times, most recently from ae493cd to abc2084 Compare July 13, 2022 16:18
@cbeer cbeer marked this pull request as ready for review July 13, 2022 16:29
@jrochkind
Copy link
Member

Anything that would make it hard to backport to BL 7.x too?

@@ -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 cbeer force-pushed the yaml-column-encoder branch from abc2084 to d2c4519 Compare July 13, 2022 17:00
@cbeer cbeer changed the title Use a custom YAML coder to restore backwards-compatible deserialization of serialzied query parameters Use a custom YAML coder to restore backwards-compatible deserialization of serialized query parameters Jul 13, 2022
Copy link
Contributor

@barmintor barmintor left a comment

Choose a reason for hiding this comment

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

Suggested one explanatory comment, otherwise I think this is good while Rails sorts itself out

Co-authored-by: Benjamin Armintor <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants