diff --git a/README.md b/README.md index cb68fc8..828c455 100644 --- a/README.md +++ b/README.md @@ -358,7 +358,10 @@ Four prints in small type: never a condition and is never touched. * **Anything that reads `params[:q]` for the effective filters** (drill-down links, exports) should read `filtering_params` instead - on the rendered page they agree, but only - `filtering_params` is also right on the non-redirected formats. + `filtering_params` is also right on the non-redirected formats. A page that translates those + values before passing them on - renaming a key an input suffixed, turning a sentinel into + another predicate - wants `applied_filter_params`: the same values as an indifferent copy, + safe to read by symbol and to mutate. Both are helpers, so a view can call them too. Tested against Active Admin 3.5 on Rails 8.0 and 8.1, under Ruby 3.3, 3.4 and 4.0. diff --git a/lib/active_admin_filter_extensions.rb b/lib/active_admin_filter_extensions.rb index 4d23a9b..cfc4871 100644 --- a/lib/active_admin_filter_extensions.rb +++ b/lib/active_admin_filter_extensions.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "activeadmin" +require "active_support/core_ext/object/deep_dup" require "active_admin_filter_extensions/version" # The violation messages live in plain i18n - config/locales/en.yml here, overridable by the @@ -45,7 +46,8 @@ def notice_flash_key ActiveAdmin::ResourceController.include ActiveAdminFilterExtensions::SearchKeys ActiveAdmin::ResourceController.include ActiveAdminFilterExtensions::RequiredFilters ActiveAdmin::ResourceController.include ActiveAdminFilterExtensions::FilterDefaults - ActiveAdmin::ResourceController.helper_method :filtering_params, :filter_default_values, :visible_filters + ActiveAdmin::ResourceController.helper_method :filtering_params, :applied_filter_params, + :filter_default_values, :visible_filters ActiveAdmin::Resource.prepend ActiveAdminFilterExtensions::ResourceExtension ActiveAdmin::ResourceDSL.include ActiveAdminFilterExtensions::Policies::ResourcePolicyDSL diff --git a/lib/active_admin_filter_extensions/filter_defaults.rb b/lib/active_admin_filter_extensions/filter_defaults.rb index 3af37a6..e08a271 100644 --- a/lib/active_admin_filter_extensions/filter_defaults.rb +++ b/lib/active_admin_filter_extensions/filter_defaults.rb @@ -28,7 +28,7 @@ module FilterDefaults # #filter_defaults_apply? which lets defaults through on a partly filtered request keeps # the values that were actually asked for. # - # @return [Hash, ActionController::Parameters] values passed to Ransack + # @return [Hash] values passed to Ransack def filtering_params requested = requested_filtering_params # Materialized on every path: computing it is also what validates, loudly, that every @@ -56,6 +56,20 @@ def filtering_params defaults.merge(requested || {}) end + # The same values, as an indifferent deep copy: readable by symbol and safe to mutate. A page + # that builds its own collection or its own links usually has to translate those values + # into whatever it talks to - renaming a key an input suffixed, turning a sentinel into a + # different predicate - and must not do that to the Hash #filtering_params itself returns, + # which is also what fills the filters form and decides the canonicalizing redirect. + # + # @return [ActiveSupport::HashWithIndifferentAccess] + def applied_filter_params + # deep_dup, not just with_indifferent_access: that copies the containers but keeps the + # value objects, so a `gsub!` or `replace` on a String value would reach back into the + # request's own and change what the next #filtering_params answers. + filtering_params.deep_dup.with_indifferent_access + end + def complete_with_groups(defaults) with_groups.each do |keys| next unless keys.any? { |key| defaults.key?(key) } diff --git a/lib/active_admin_filter_extensions/version.rb b/lib/active_admin_filter_extensions/version.rb index 93c00d7..57c4b7a 100644 --- a/lib/active_admin_filter_extensions/version.rb +++ b/lib/active_admin_filter_extensions/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module ActiveAdminFilterExtensions - VERSION = "1.1.0" + VERSION = "1.2.0" end diff --git a/spec/data_access_spec.rb b/spec/data_access_spec.rb index ac9e70c..1550c93 100644 --- a/spec/data_access_spec.rb +++ b/spec/data_access_spec.rb @@ -53,6 +53,36 @@ end end + describe "#applied_filter_params" do + let(:filters) { { position_eq: { extensions: { default: "20" } } } } + + it "reads by symbol what the request spelled as a String" do + expect(controller.send(:applied_filter_params)[:position_eq]).to eq("20") + end + + it "hands out a copy, so a caller translating the values cannot rewrite the search" do + applied = controller.send(:applied_filter_params) + applied[:position_gt] = applied.delete(:position_eq) + + expect(controller.send(:filtering_params)).to eq("position_eq" => "20") + expect(searched_with).to eq("position_eq" => "20") + end + + context "when the request carries a value of its own" do + let(:filters) { { title_cont: {} } } + # Mutable on purpose: with_indifferent_access alone copies the Hash but hands out this + # very String, so editing it in place would reach back into the request. + let(:params) { { q: { "title_cont" => String.new("hello") } } } + + it "copies the values too, so editing one in place cannot reach the request" do + controller.send(:applied_filter_params)[:title_cont].replace("bye") + + expect(controller.send(:filtering_params)).to eq("title_cont" => "hello") + expect(searched_with).to eq("title_cont" => "hello") + end + end + end + describe "a Hash default" do let(:filters) { { created_at: { as: :date_range, extensions: { default: { gteq: "2020-01-01" } } } } } diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 07dc014..fdea2db 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "active_support/core_ext/object/blank" +require "active_support/core_ext/object/deep_dup" require "active_support/core_ext/hash/indifferent_access" require "active_support/hash_with_indifferent_access" # Pure Ruby, no Active Admin boot required - `visible_filters` resolves `:if` / `:unless` with it.