From 01d16f96d47f47a7dab8922d441f70a77cf95922 Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Fri, 21 Nov 2025 12:06:32 +0000 Subject: [PATCH] Remove Search::Record.for_account Instead we'll compute the table name dynamically based on Current.account where needed. Also we'll prevent searchable records from being saved if Current.account is not set, otherwise the after commit callbacks will fail. --- app/models/card/searchable.rb | 6 ++---- app/models/concerns/searchable.rb | 17 ++++++++++------- app/models/search.rb | 2 +- app/models/search/record/sqlite.rb | 4 ---- app/models/search/record/trilogy.rb | 8 ++------ lib/tasks/search.rake | 11 +++++++++-- test/models/card/searchable_test.rb | 10 ++++++++++ test/models/comment/searchable_test.rb | 18 ++++++++++++++---- 8 files changed, 48 insertions(+), 28 deletions(-) diff --git a/app/models/card/searchable.rb b/app/models/card/searchable.rb index 81e704e43..e2c0c11dc 100644 --- a/app/models/card/searchable.rb +++ b/app/models/card/searchable.rb @@ -5,10 +5,8 @@ module Card::Searchable include ::Searchable scope :mentioning, ->(query, user:) do - search_records = Search::Record.for_account(user.account_id) - - joins(search_records.card_join) - .merge(search_records.for_query(query: Search::Query.wrap(query), user: user)) + joins(Search::Record.card_join) + .merge(Search::Record.for_query(query: Search::Query.wrap(query), user: user)) end end diff --git a/app/models/concerns/searchable.rb b/app/models/concerns/searchable.rb index 39e5d95f8..97534bbd7 100644 --- a/app/models/concerns/searchable.rb +++ b/app/models/concerns/searchable.rb @@ -2,6 +2,7 @@ module Searchable extend ActiveSupport::Concern included do + before_save :ensure_current_account after_create_commit :create_in_search_index after_update_commit :update_in_search_index after_destroy_commit :remove_from_search_index @@ -12,22 +13,24 @@ module Searchable end private + def ensure_current_account + unless Current.account + raise "Current.account must be set to save #{self.class.name}" + end + end + def create_in_search_index - search_record_class.create!(search_record_attributes) + Search::Record.create!(search_record_attributes) end def update_in_search_index - search_record_class.find_or_initialize_by(searchable_type: self.class.name, searchable_id: id).tap do |record| + Search::Record.find_or_initialize_by(searchable_type: self.class.name, searchable_id: id).tap do |record| record.update!(search_record_attributes) end end def remove_from_search_index - search_record_class.find_by(searchable_type: self.class.name, searchable_id: id)&.destroy - end - - def search_record_class - Search::Record.for_account(account_id) + Search::Record.find_by(searchable_type: self.class.name, searchable_id: id)&.destroy end def search_record_attributes diff --git a/app/models/search.rb b/app/models/search.rb index 264194f03..68dbbcd21 100644 --- a/app/models/search.rb +++ b/app/models/search.rb @@ -4,6 +4,6 @@ module Search end def self.results(query:, user:) - Record.for_account(user.account_id).search(query: Query.wrap(query), user: user) + Record.search(query: Query.wrap(query), user: user) end end diff --git a/app/models/search/record/sqlite.rb b/app/models/search/record/sqlite.rb index 9517f9219..989f8d834 100644 --- a/app/models/search/record/sqlite.rb +++ b/app/models/search/record/sqlite.rb @@ -11,10 +11,6 @@ module Search::Record::SQLite end class_methods do - def for_account(account_id) - self - end - def matching_scope(query, account_id) joins("INNER JOIN search_records_fts ON search_records_fts.rowid = #{table_name}.id") .where("search_records_fts MATCH ?", query) diff --git a/app/models/search/record/trilogy.rb b/app/models/search/record/trilogy.rb index 3620e530d..e9f1d74ab 100644 --- a/app/models/search/record/trilogy.rb +++ b/app/models/search/record/trilogy.rb @@ -8,11 +8,11 @@ module Search::Record::Trilogy end class_methods do - def table_name + def compute_table_name if Current.account "search_records_#{shard_id_for_account(Current.account.id)}" else - super + raise "Current.account is not set; cannot determine shard for Search::Record" end end @@ -20,10 +20,6 @@ module Search::Record::Trilogy Zlib.crc32(account_id.to_s) % SHARD_COUNT end - def for_account(account_id) - self - end - def matching_scope(query, account_id) stemmed_query = Search::Stemmer.stem(query) account_key = "account#{account_id}" diff --git a/lib/tasks/search.rake b/lib/tasks/search.rake index c1ec0e28a..df39c3a70 100644 --- a/lib/tasks/search.rake +++ b/lib/tasks/search.rake @@ -2,17 +2,24 @@ namespace :search do desc "Reindex all cards and comments in the search index" task reindex: :environment do puts "Clearing search records..." - Search::Record::Trilogy::SHARD_COUNT.times do |shard_id| - ActiveRecord::Base.connection.execute("DELETE FROM search_records_#{shard_id}") + if ActiveRecord::Base.connection.adapter_name == "SQLite" + ActiveRecord::Base.connection.execute("DELETE FROM search_records") + ActiveRecord::Base.connection.execute("DELETE FROM search_records_fts") + else + Search::Record::Trilogy::SHARD_COUNT.times do |shard_id| + ActiveRecord::Base.connection.execute("DELETE FROM search_records_#{shard_id}") + end end puts "Reindexing cards..." Card.find_each do |card| + Current.account = card.account card.reindex end puts "Reindexing comments..." Comment.find_each do |comment| + Current.account = comment.account comment.reindex end diff --git a/test/models/card/searchable_test.rb b/test/models/card/searchable_test.rb index 18845ab7a..2dc8f13a6 100644 --- a/test/models/card/searchable_test.rb +++ b/test/models/card/searchable_test.rb @@ -31,4 +31,14 @@ class Card::SearchableTest < ActiveSupport::TestCase assert_includes results, card_in_board assert_not_includes results, card_in_other_board end + + test "card requires Current.account to be set" do + Current.account = nil + + error = assert_raises(RuntimeError) do + @board.cards.create!(title: "Test card", creator: @user) + end + + assert_match(/Current.account must be set to save Card/, error.message) + end end diff --git a/test/models/comment/searchable_test.rb b/test/models/comment/searchable_test.rb index a8e52bfc2..3c7dd461f 100644 --- a/test/models/comment/searchable_test.rb +++ b/test/models/comment/searchable_test.rb @@ -10,18 +10,18 @@ class Comment::SearchableTest < ActiveSupport::TestCase test "comment search" do # Comment is indexed on create comment = @card.comments.create!(body: "searchable comment text", creator: @user) - record = Search::Record.for_account(@account.id).find_by(searchable_type: "Comment", searchable_id: comment.id) + record = Search::Record.find_by(searchable_type: "Comment", searchable_id: comment.id) assert_not_nil record # Comment is updated in index comment.update!(body: "updated text") - record = Search::Record.for_account(@account.id).find_by(searchable_type: "Comment", searchable_id: comment.id) + record = Search::Record.find_by(searchable_type: "Comment", searchable_id: comment.id) assert_match /updat/, record.content # Comment is removed from index on destroy comment_id = comment.id comment.destroy - record = Search::Record.for_account(@account.id).find_by(searchable_type: "Comment", searchable_id: comment_id) + record = Search::Record.find_by(searchable_type: "Comment", searchable_id: comment_id) assert_nil record # Finding cards via comment search @@ -34,8 +34,18 @@ class Comment::SearchableTest < ActiveSupport::TestCase # Comment stores parent card_id and board_id new_comment = @card.comments.create!(body: "test comment", creator: @user) - record = Search::Record.for_account(@account.id).find_by(searchable_type: "Comment", searchable_id: new_comment.id) + record = Search::Record.find_by(searchable_type: "Comment", searchable_id: new_comment.id) assert_equal @card.id, record.card_id assert_equal @board.id, record.board_id end + + test "comment requires Current.account to be set" do + Current.account = nil + + error = assert_raises(RuntimeError) do + @card.comments.create!(body: "Test comment", creator: @user) + end + + assert_match(/Current.account must be set to save Comment/, error.message) + end end