From b571303385bd635f6133fad916748aea50ea806d Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Tue, 25 Nov 2025 11:34:41 +0000 Subject: [PATCH] Don't use Current.account for search records It doesn't actually work, and even if we could make it work reliably we are better off if the records always know to go to the right shard. It does make the interface a bit more complicated as we need to select the right shard class with `for(account_id)`. --- app/models/card/searchable.rb | 5 ++--- app/models/concerns/searchable.rb | 19 +++++++------------ app/models/search/record.rb | 16 ++++++++++++++++ app/models/search/record/sqlite.rb | 4 ++++ app/models/search/record/trilogy.rb | 23 +++++++++++++++-------- app/models/user/searcher.rb | 2 +- lib/tasks/search.rake | 10 ++-------- test/models/card/searchable_test.rb | 19 +++++-------------- test/models/comment/searchable_test.rb | 23 +++++++---------------- test/models/search_test.rb | 8 ++++---- 10 files changed, 63 insertions(+), 66 deletions(-) diff --git a/app/models/card/searchable.rb b/app/models/card/searchable.rb index caf9b4fea..8cc108b23 100644 --- a/app/models/card/searchable.rb +++ b/app/models/card/searchable.rb @@ -4,10 +4,9 @@ module Card::Searchable included do include ::Searchable - has_many :card_search_records, class_name: "Search::Record" - scope :mentioning, ->(query, user:) do - joins(:card_search_records).merge(Search::Record.for_query(query, user: user)) + search_record_class = Search::Record.for(user.account_id) + joins(search_record_class.card_join).merge(search_record_class.for_query(query, user: user)) end end diff --git a/app/models/concerns/searchable.rb b/app/models/concerns/searchable.rb index 894c23b94..8b39504ef 100644 --- a/app/models/concerns/searchable.rb +++ b/app/models/concerns/searchable.rb @@ -2,9 +2,6 @@ module Searchable extend ActiveSupport::Concern included do - has_one :search_record, class_name: "Search::Record", as: :searchable - - 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 @@ -15,22 +12,16 @@ 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 - create_search_record!(search_record_attributes) + search_record_class.create!(search_record_attributes) end def update_in_search_index - search_record&.update!(search_record_attributes) || create_in_search_index + search_record_class.upsert!(search_record_attributes) end def remove_from_search_index - search_record&.destroy + search_record_class.find_by(searchable_type: self.class.name, searchable_id: id)&.destroy end def search_record_attributes @@ -46,6 +37,10 @@ module Searchable } end + def search_record_class + Search::Record.for(account_id) + end + # Models must implement these methods: # - account_id: returns the account id # - search_title: returns title string or nil diff --git a/app/models/search/record.rb b/app/models/search/record.rb index 424a40363..2aaff7c24 100644 --- a/app/models/search/record.rb +++ b/app/models/search/record.rb @@ -6,6 +6,22 @@ class Search::Record < ApplicationRecord validates :account_id, :searchable_type, :searchable_id, :card_id, :board_id, :created_at, presence: true + class << self + def upsert!(attributes) + record = find_by(searchable_type: attributes[:searchable_type], searchable_id: attributes[:searchable_id]) + if record + record.update!(attributes) + record + else + create!(attributes) + end + end + + def card_join + "INNER JOIN #{table_name} ON #{table_name}.card_id = cards.id" + end + end + scope :for_query, ->(query, user:) do query = Search::Query.wrap(query) diff --git a/app/models/search/record/sqlite.rb b/app/models/search/record/sqlite.rb index 85aeb59f2..c3da9131a 100644 --- a/app/models/search/record/sqlite.rb +++ b/app/models/search/record/sqlite.rb @@ -25,6 +25,10 @@ module Search::Record::SQLite "snippet(search_records_fts, 1, #{opening_mark}, #{closing_mark}, #{ellipsis}, 20) AS result_content", "#{connection.quote(query.terms)} AS query" ] end + + def for(account_id) + self + end end def card_title diff --git a/app/models/search/record/trilogy.rb b/app/models/search/record/trilogy.rb index dd10334c8..7705f3b71 100644 --- a/app/models/search/record/trilogy.rb +++ b/app/models/search/record/trilogy.rb @@ -4,23 +4,26 @@ module Search::Record::Trilogy SHARD_COUNT = 16 included do + self.abstract_class = true before_save :set_account_key, :stem_content scope :matching, ->(query, account_id) do full_query = "+account#{account_id} +(#{Search::Stemmer.stem(query)})" where("MATCH(#{table_name}.account_key, #{table_name}.content, #{table_name}.title) AGAINST(? IN BOOLEAN MODE)", full_query) end + + SHARD_CLASSES = SHARD_COUNT.times.map do |shard_id| + Class.new(self) do + self.table_name = "search_records_#{shard_id}" + + def self.name + "Search::Record" + end + end + end.freeze end class_methods do - def compute_table_name - if Current.account - "search_records_#{shard_id_for_account(Current.account.id)}" - else - "search_records_0" - end - end - def shard_id_for_account(account_id) Zlib.crc32(account_id.to_s) % SHARD_COUNT end @@ -28,6 +31,10 @@ module Search::Record::Trilogy def search_fields(query) "#{connection.quote(query.terms)} AS query" end + + def for(account_id) + SHARD_CLASSES[shard_id_for_account(account_id)] + end end def card_title diff --git a/app/models/user/searcher.rb b/app/models/user/searcher.rb index 60a901ee8..090adf4b8 100644 --- a/app/models/user/searcher.rb +++ b/app/models/user/searcher.rb @@ -6,7 +6,7 @@ module User::Searcher end def search(terms) - Search::Record.search(terms, user: self) + Search::Record.for(account_id).search(terms, user: self) end def remember_search(terms) diff --git a/lib/tasks/search.rake b/lib/tasks/search.rake index df39c3a70..6b47dbde3 100644 --- a/lib/tasks/search.rake +++ b/lib/tasks/search.rake @@ -12,16 +12,10 @@ namespace :search do end puts "Reindexing cards..." - Card.find_each do |card| - Current.account = card.account - card.reindex - end + Card.includes(:rich_text_description).find_each(&:reindex) puts "Reindexing comments..." - Comment.find_each do |comment| - Current.account = comment.account - comment.reindex - end + Comment.includes(:rich_text_body, :card).find_each(&:reindex) puts "Done! Reindexed #{Card.count} cards and #{Comment.count} comments." end diff --git a/test/models/card/searchable_test.rb b/test/models/card/searchable_test.rb index c0a99ab23..d6dcdf60c 100644 --- a/test/models/card/searchable_test.rb +++ b/test/models/card/searchable_test.rb @@ -32,25 +32,16 @@ class Card::SearchableTest < ActiveSupport::TestCase 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 - test "deleting card removes search record and FTS entry" do + search_record_class = Search::Record.for(@user.account_id) card = @board.cards.create!(title: "Card to delete", creator: @user) # Verify search record exists - search_record = Search::Record.find_by(searchable_type: "Card", searchable_id: card.id) + search_record = search_record_class.find_by(searchable_type: "Card", searchable_id: card.id) assert_not_nil search_record, "Search record should exist after card creation" # For SQLite, verify FTS entry exists - if Search::Record.connection.adapter_name == "SQLite" + if search_record_class.connection.adapter_name == "SQLite" fts_entry = search_record.search_records_fts assert_not_nil fts_entry, "FTS entry should exist" assert_equal card.title, fts_entry.title @@ -60,11 +51,11 @@ class Card::SearchableTest < ActiveSupport::TestCase card.destroy # Verify search record is deleted - search_record = Search::Record.find_by(searchable_type: "Card", searchable_id: card.id) + search_record = search_record_class.find_by(searchable_type: "Card", searchable_id: card.id) assert_nil search_record, "Search record should be deleted after card deletion" # For SQLite, verify FTS entry is deleted - if Search::Record.connection.adapter_name == "SQLite" + if search_record_class.connection.adapter_name == "SQLite" fts_count = Search::Record::SQLite::Fts.where(rowid: card.id).count assert_equal 0, fts_count, "FTS entry should be deleted" end diff --git a/test/models/comment/searchable_test.rb b/test/models/comment/searchable_test.rb index b8b77a8b0..7ba73847c 100644 --- a/test/models/comment/searchable_test.rb +++ b/test/models/comment/searchable_test.rb @@ -8,14 +8,15 @@ class Comment::SearchableTest < ActiveSupport::TestCase end test "comment search" do + search_record_class = Search::Record.for(@user.account_id) # Comment is indexed on create comment = @card.comments.create!(body: "searchable comment text", creator: @user) - record = Search::Record.find_by(searchable_type: "Comment", searchable_id: comment.id) + record = search_record_class.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.find_by(searchable_type: "Comment", searchable_id: comment.id) + record = search_record_class.find_by(searchable_type: "Comment", searchable_id: comment.id) assert_match /updat/, record.content # Comment is removed from index on destroy @@ -23,17 +24,17 @@ class Comment::SearchableTest < ActiveSupport::TestCase search_record_id = record.id # For SQLite, verify FTS entry exists before deletion - if Search::Record.connection.adapter_name == "SQLite" + if search_record_class.connection.adapter_name == "SQLite" fts_entry = record.search_records_fts assert_not_nil fts_entry, "FTS entry should exist before comment deletion" end comment.destroy - record = Search::Record.find_by(searchable_type: "Comment", searchable_id: comment_id) + record = search_record_class.find_by(searchable_type: "Comment", searchable_id: comment_id) assert_nil record # For SQLite, verify FTS entry is also deleted - if Search::Record.connection.adapter_name == "SQLite" + if search_record_class.connection.adapter_name == "SQLite" fts_count = Search::Record::SQLite::Fts.where(rowid: search_record_id).count assert_equal 0, fts_count, "FTS entry should be deleted after comment deletion" end @@ -48,18 +49,8 @@ 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.find_by(searchable_type: "Comment", searchable_id: new_comment.id) + record = search_record_class.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 diff --git a/test/models/search_test.rb b/test/models/search_test.rb index be4174ff8..1f4981090 100644 --- a/test/models/search_test.rb +++ b/test/models/search_test.rb @@ -9,10 +9,10 @@ class SearchTest < ActiveSupport::TestCase comment_card = @board.cards.create!(title: "Some card", creator: @user) comment_card.comments.create!(body: "overflowing text", creator: @user) - results = Search::Record.search("layout", user: @user) + results = Search::Record.for(@user.account_id).search("layout", user: @user) assert results.find { |it| it.card_id == card.id } - results = Search::Record.search("overflowing", user: @user) + results = Search::Record.for(@user.account_id).search("overflowing", user: @user) assert results.find { |it| it.card_id == comment_card.id && it.searchable_type == "Comment" } # Don't include inaccessible boards @@ -21,13 +21,13 @@ class SearchTest < ActiveSupport::TestCase accessible_card = @board.cards.create!(title: "searchable content", creator: @user) inaccessible_card = inaccessible_board.cards.create!(title: "searchable content", creator: other_user) - results = Search::Record.search("searchable", user: @user) + results = Search::Record.for(@user.account_id).search("searchable", user: @user) assert results.find { |it| it.card_id == accessible_card.id } assert_not results.find { |it| it.card_id == inaccessible_card.id } # Empty board_ids returns no results user_without_access = User.create!(name: "No Access User", account: @account) - results = Search::Record.search("anything", user: user_without_access) + results = Search::Record.for(user_without_access.account_id).search("anything", user: user_without_access) assert_empty results end end