From d455b72906ba7ae0446499f82d290de1c23450f3 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Fri, 8 Nov 2024 15:17:55 -0600 Subject: [PATCH] Hash filter params --- app/models/filter.rb | 2 +- app/models/filter/params.rb | 40 ++++++++----------- app/models/filter/resources.rb | 2 +- .../20241108205445_hash_filter_params.rb | 7 ++++ db/schema.rb | 6 +-- test/controllers/filters_controller_test.rb | 2 +- test/fixtures/filters.yml | 2 +- test/models/filter_test.rb | 8 ++-- 8 files changed, 35 insertions(+), 34 deletions(-) create mode 100644 db/migrate/20241108205445_hash_filter_params.rb diff --git a/app/models/filter.rb b/app/models/filter.rb index 58edae90f..c3d163c97 100644 --- a/app/models/filter.rb +++ b/app/models/filter.rb @@ -10,7 +10,7 @@ class Filter < ApplicationRecord filter.save! filter rescue ActiveRecord::RecordNotUnique - find_by!(params: filter.params).tap(&:touch) + find_by!(params_digest: filter.hashed_params).tap(&:touch) end end diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb index 2726e153a..1077203ab 100644 --- a/app/models/filter/params.rb +++ b/app/models/filter/params.rb @@ -4,34 +4,28 @@ module Filter::Params KNOWN_PARAMS = [ :indexed_by, :assignments, bucket_ids: [], assignee_ids: [], tag_ids: [] ] included do - after_initialize :derive_params + before_save { self.params_digest = hashed_params } + end + + def as_params + params = {}.tap do |h| + h["tag_ids"] = tags.ids + h["bucket_ids"] = buckets.ids + h["assignee_ids"] = assignees.ids + h["indexed_by"] = indexed_by + h["assignments"] = assignments + end + + params.compact_blank.reject { |k, v| default_fields[k] == v } end def to_params - ActionController::Parameters.new(params).permit(*KNOWN_PARAMS).tap do |params| + ActionController::Parameters.new(as_params).permit(*KNOWN_PARAMS).tap do |params| params[:filter_id] = id if persisted? end end - private - # `derive_params` stores a denormalized version of the filter in `params` to - # 1) Enforce uniqueness via db constraints - # 2) Look up identical filters by a single column - # 3) Easily turn all filter params into a query string - def derive_params - derive_params_from_resource_ids - derive_params_from_fields - params.compact_blank! - end - alias_method :derived_params, :derive_params - - def derive_params_from_resource_ids - params["tag_ids"] = tags.ids - params["bucket_ids"] = buckets.ids - params["assignee_ids"] = assignees.ids - end - - def derive_params_from_fields - self.params.merge! fields.reject { |k, v| default_fields[k] == v } - end + def hashed_params + Digest::MD5.hexdigest as_params.to_json + end end diff --git a/app/models/filter/resources.rb b/app/models/filter/resources.rb index 5215087f6..e44fd91f8 100644 --- a/app/models/filter/resources.rb +++ b/app/models/filter/resources.rb @@ -10,6 +10,6 @@ module Filter::Resources def resource_removed(resource) kind = resource.class.model_name.plural send "#{kind}=", send(kind).without(resource) - derived_params.blank? ? destroy! : save! + as_params.blank? ? destroy! : save! end end diff --git a/db/migrate/20241108205445_hash_filter_params.rb b/db/migrate/20241108205445_hash_filter_params.rb new file mode 100644 index 000000000..e403598d0 --- /dev/null +++ b/db/migrate/20241108205445_hash_filter_params.rb @@ -0,0 +1,7 @@ +class HashFilterParams < ActiveRecord::Migration[8.0] + def change + change_column :filters, :params, :string, null: false + change_column_default :filters, :params, from: {}, to: nil + rename_column :filters, :params, :params_digest + end +end diff --git a/db/schema.rb b/db/schema.rb index bbb0daf2c..41085fa06 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2024_11_05_224305) do +ActiveRecord::Schema[8.0].define(version: 2024_11_08_205445) do create_table "accesses", force: :cascade do |t| t.integer "bucket_id", null: false t.integer "user_id", null: false @@ -131,11 +131,11 @@ ActiveRecord::Schema[8.0].define(version: 2024_11_05_224305) do create_table "filters", force: :cascade do |t| t.integer "creator_id", null: false - t.json "params", default: {}, null: false + t.string "params_digest", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.json "fields", default: {}, null: false - t.index ["creator_id", "params"], name: "index_filters_on_creator_id_and_params", unique: true + t.index ["creator_id", "params_digest"], name: "index_filters_on_creator_id_and_params_digest", unique: true end create_table "filters_tags", id: false, force: :cascade do |t| diff --git a/test/controllers/filters_controller_test.rb b/test/controllers/filters_controller_test.rb index 2522fde07..8e422fae2 100644 --- a/test/controllers/filters_controller_test.rb +++ b/test/controllers/filters_controller_test.rb @@ -28,6 +28,6 @@ class FiltersControllerTest < ActionDispatch::IntegrationTest assert_difference "users(:david).filters.count", -1 do delete filter_url(filters(:jz_assignments)) end - assert_redirected_to bubbles_path(filters(:jz_assignments).params) + assert_redirected_to bubbles_path(filters(:jz_assignments).as_params) end end diff --git a/test/fixtures/filters.yml b/test/fixtures/filters.yml index b80de3a69..ff1f46bfa 100644 --- a/test/fixtures/filters.yml +++ b/test/fixtures/filters.yml @@ -3,4 +3,4 @@ jz_assignments: tags: mobile assignees: jz fields: <%= { indexed_by: :most_discussed }.to_json %> - params: <%= { indexed_by: :most_discussed, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }.to_json %> + params_digest: <%= Digest::MD5.hexdigest({ indexed_by: :most_discussed, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }.to_json) %> diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index 49c88942d..cffe18792 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -37,7 +37,7 @@ class FilterTest < ActiveSupport::TestCase test "param sanitization" do filter = users(:david).filters.new indexed_by: "most_active", tag_ids: "", assignee_ids: [ users(:jz).id ], bucket_ids: [ buckets(:writebook).id ] expected = { assignee_ids: [ users(:jz).id ], bucket_ids: [ buckets(:writebook).id ] } - assert_equal expected.stringify_keys, filter.params + assert_equal expected.stringify_keys, filter.as_params end test "cacheable" do @@ -48,15 +48,15 @@ class FilterTest < ActiveSupport::TestCase test "resource removal" do filter = users(:david).filters.create! tag_ids: [ tags(:mobile).id ], bucket_ids: [ buckets(:writebook).id ] - assert_includes filter.params["tag_ids"], tags(:mobile).id + assert_includes filter.as_params["tag_ids"], tags(:mobile).id assert_includes filter.tags, tags(:mobile) - assert_includes filter.params["bucket_ids"], buckets(:writebook).id + assert_includes filter.as_params["bucket_ids"], buckets(:writebook).id assert_includes filter.buckets, buckets(:writebook) assert_changes "filter.reload.updated_at" do tags(:mobile).destroy! end - assert_nil filter.reload.params["tag_ids"] + assert_nil filter.reload.as_params["tag_ids"] assert_changes "Filter.exists?(filter.id)" do buckets(:writebook).destroy!