diff --git a/app/models/room/messages_count.rb b/app/models/room/messages_count.rb index 5790108..53254ac 100644 --- a/app/models/room/messages_count.rb +++ b/app/models/room/messages_count.rb @@ -5,6 +5,7 @@ class Room::MessagesCount INSERT_TRIGGER = "messages_ai_rooms_messages_count" DELETE_TRIGGER = "messages_ad_rooms_messages_count" UPDATE_TRIGGER = "messages_au_rooms_messages_count" + TRIGGERS = [ INSERT_TRIGGER, DELETE_TRIGGER, UPDATE_TRIGGER ].freeze class << self def install!(connection = ActiveRecord::Base.connection) @@ -32,7 +33,7 @@ class Room::MessagesCount end def uninstall!(connection = ActiveRecord::Base.connection) - [ INSERT_TRIGGER, DELETE_TRIGGER, UPDATE_TRIGGER ].each do |name| + TRIGGERS.each do |name| connection.execute("DROP TRIGGER IF EXISTS #{name}") end end @@ -47,9 +48,10 @@ class Room::MessagesCount # schema.rb does not dump SQLite triggers; reinstall after schema:load. def ensure!(connection = ActiveRecord::Base.connection) + return unless connection.adapter_name.match?(/sqlite/i) return unless connection.data_source_exists?(:rooms) return unless connection.column_exists?(:rooms, :messages_count) - return if trigger_installed?(connection, INSERT_TRIGGER) + return if TRIGGERS.all? { |name| trigger_installed?(connection, name) } install!(connection) end @@ -61,4 +63,3 @@ class Room::MessagesCount end end end - diff --git a/config/initializers/room_messages_count.rb b/config/initializers/room_messages_count.rb index 44af41b..6c1c057 100644 --- a/config/initializers/room_messages_count.rb +++ b/config/initializers/room_messages_count.rb @@ -1,10 +1,5 @@ -# schema.rb cannot dump SQLite triggers. Reinstall after boot when an existing DB -# is missing them (db:schema:load / test schema load also call ensure! via rake). +# schema.rb cannot dump SQLite triggers. Cover existing DBs that lost them. Rails.application.config.after_initialize do - ActiveRecord::Base.connection_pool.with_connection do |connection| - next unless connection.adapter_name.match?(/sqlite/i) - - Room::MessagesCount.ensure!(connection) - end + Room::MessagesCount.ensure! rescue ActiveRecord::NoDatabaseError, ActiveRecord::ConnectionNotEstablished end diff --git a/db/migrate/20261007190000_add_messages_count_to_rooms.rb b/db/migrate/20261007190000_add_messages_count_to_rooms.rb index afc58c9..55286fa 100644 --- a/db/migrate/20261007190000_add_messages_count_to_rooms.rb +++ b/db/migrate/20261007190000_add_messages_count_to_rooms.rb @@ -1,8 +1,6 @@ class AddMessagesCountToRooms < ActiveRecord::Migration[8.2] def up - unless column_exists?(:rooms, :messages_count) - add_column :rooms, :messages_count, :integer, null: false, default: 0 - end + add_column :rooms, :messages_count, :integer, null: false, default: 0 # Backfill before installing triggers so the COUNT rewrite does not race with # concurrent inserts, and so we never rely on Rails callbacks for the tally. @@ -12,6 +10,6 @@ class AddMessagesCountToRooms < ActiveRecord::Migration[8.2] def down Room::MessagesCount.uninstall!(connection) - remove_column :rooms, :messages_count if column_exists?(:rooms, :messages_count) + remove_column :rooms, :messages_count end end diff --git a/db/schema.rb b/db/schema.rb index d5b3a37..ccb79c9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -183,6 +183,5 @@ ActiveRecord::Schema[8.2].define(version: 2026_10_07_190000) do # Virtual tables defined in this database. # Note that virtual tables may not work with other database engines. Be careful if changing database. create_virtual_table "message_search_index", "fts5", ["body", "tokenize=porter"] - # SQLite triggers are not dumped by schema.rb; keep rooms.messages_count honest after schema:load. - Room::MessagesCount.install! end + diff --git a/lib/tasks/room_messages_count.rake b/lib/tasks/room_messages_count.rake index dcb2d08..e042a14 100644 --- a/lib/tasks/room_messages_count.rake +++ b/lib/tasks/room_messages_count.rake @@ -1,38 +1,16 @@ -# schema.rb cannot dump SQLite triggers. Reinstall after loads, and re-append -# Room::MessagesCount.install! after dumps so the call is not lost. +# schema.rb cannot dump SQLite triggers — reinstall after schema loads. namespace :room_messages_count do task ensure: :environment do ActiveRecord::Base.connection_pool.with_connection do |connection| - next unless connection.adapter_name.match?(/sqlite/i) - Room::MessagesCount.ensure!(connection) end end - - task append_schema_install: :environment do - schema = Rails.root.join("db/schema.rb") - contents = schema.read - marker = "Room::MessagesCount.install!" - next if contents.include?(marker) - - contents.sub!(/\nend\n?\z/, <<~RUBY) - - # SQLite triggers are not dumped by schema.rb; keep rooms.messages_count honest after schema:load. - #{marker} - end - RUBY - schema.write(contents) - end end -{ - "db:schema:load" => "room_messages_count:ensure", - "db:test:load_schema" => "room_messages_count:ensure", - "db:schema:dump" => "room_messages_count:append_schema_install" -}.each do |task_name, enhancement| +%w[db:schema:load db:test:load_schema].each do |task_name| next unless Rake::Task.task_defined?(task_name) Rake::Task[task_name].enhance do - Rake::Task[enhancement].invoke + Rake::Task["room_messages_count:ensure"].invoke end end diff --git a/test/controllers/messages/by_bots_controller_test.rb b/test/controllers/messages/by_bots_controller_test.rb index a7fc835..4345dca 100644 --- a/test/controllers/messages/by_bots_controller_test.rb +++ b/test/controllers/messages/by_bots_controller_test.rb @@ -103,7 +103,6 @@ class Messages::ByBotsControllerTest < ActionDispatch::IntegrationTest json = JSON.parse(response.body) assert_equal Message::PAGE_SIZE, json.size - assert_equal @room.reload.messages_count.to_s, response.headers["X-Total-Count"] assert_equal "41", response.headers["X-Total-Count"] assert_not_includes json.map { it["id"] }, messages(:fourth).id diff --git a/test/models/room/messages_count_test.rb b/test/models/room/messages_count_test.rb index 53deac1..911b21e 100644 --- a/test/models/room/messages_count_test.rb +++ b/test/models/room/messages_count_test.rb @@ -5,7 +5,12 @@ class Room::MessagesCountTest < ActiveSupport::TestCase setup do @room = rooms(:designers) @other_room = rooms(:pets) - synchronize!(@room, @other_room) + end + + test "fixture rooms start with an accurate messages_count" do + [ rooms(:watercooler), rooms(:designers), rooms(:bender_and_kevin) ].each do |room| + assert_equal room.messages.count, room.messages_count, "#{room.name || room.id} fixture count" + end end test "ActiveRecord create and destroy adjust the counter once" do @@ -64,7 +69,6 @@ class Room::MessagesCountTest < ActiveSupport::TestCase test "moving a message between rooms moves the counter" do message = @room.messages.create!(creator: users(:jason), body: "Move me", client_message_id: "count-move") - synchronize!(@room, @other_room) assert_difference -> { @room.reload.messages_count }, -1 do assert_difference -> { @other_room.reload.messages_count }, +1 do @@ -76,6 +80,31 @@ class Room::MessagesCountTest < ActiveSupport::TestCase assert_equal @other_room.messages.count, @other_room.reload.messages_count end + test "Message does not declare an ActiveRecord counter_cache" do + reflection = Message.reflect_on_association(:room) + assert_not reflection.counter_cache_column + end +end + +# Destructive trigger DDL and foreign connections need a committed DB. +class Room::MessagesCountLifecycleTest < ActiveSupport::TestCase + self.use_transactional_tests = false + + setup do + Room::MessagesCount.ensure! + @room = rooms(:designers) + @other_room = rooms(:pets) + Room::MessagesCount.backfill! + @room.reload + @other_room.reload + end + + teardown do + Message.where("client_message_id LIKE ?", "count-%").delete_all + Room::MessagesCount.ensure! + Room::MessagesCount.backfill! + end + test "ensure! installs missing triggers without changing existing counts" do before = @room.reload.messages_count Room::MessagesCount.uninstall! @@ -84,68 +113,41 @@ class Room::MessagesCountTest < ActiveSupport::TestCase Room::MessagesCount.ensure! - assert Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) + assert Room::MessagesCount::TRIGGERS.all? { |name| + Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) + } assert_equal before, @room.reload.messages_count assert_difference -> { @room.reload.messages_count }, +1 do @room.messages.create!(creator: users(:jason), body: "After ensure", client_message_id: "count-ensure") end - ensure - Room::MessagesCount.ensure! end - test "backfill plus triggers do not double-count ActiveRecord writes" do + test "ensure! repairs a partial trigger install" do Room::MessagesCount.uninstall! ActiveRecord::Base.connection.execute <<~SQL - UPDATE rooms SET messages_count = ( - SELECT COUNT(*) FROM messages WHERE messages.room_id = rooms.id - ) + CREATE TRIGGER #{Room::MessagesCount::INSERT_TRIGGER} AFTER INSERT ON messages + BEGIN + UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; + END SQL - Room::MessagesCount.install! - assert_equal @room.messages.count, @room.reload.messages_count + assert Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) + assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::DELETE_TRIGGER) - assert_difference -> { @room.reload.messages_count }, +1 do - assert_difference -> { @room.messages.count }, +1 do - @room.messages.create!(creator: users(:jason), body: "Once", client_message_id: "count-no-double") - end - end - - assert_equal @room.messages.count, @room.reload.messages_count - ensure Room::MessagesCount.ensure! - end - private - def synchronize!(*rooms) - Room::MessagesCount.ensure! - Room::MessagesCount.backfill! - rooms.each(&:reload) - end -end - -# Foreign connections cannot join the transactional fixture lock; run outside it. -class Room::MessagesCountForeignConnectionTest < ActiveSupport::TestCase - self.use_transactional_tests = false - - setup do - Room::MessagesCount.ensure! - @room = rooms(:designers) - Room::MessagesCount.backfill! - @room.reload - end - - teardown do - Message.where(client_message_id: "count-foreign-insert").delete_all - Room::MessagesCount.backfill! + assert Room::MessagesCount::TRIGGERS.all? { |name| + Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) + } end test "foreign SQLite connections keep the counter in step" do path = File.expand_path(ActiveRecord::Base.connection_db_config.database) now = Time.current.utc.strftime("%Y-%m-%d %H:%M:%S.%6N") before = @room.reload.messages_count + other_before = @other_room.reload.messages_count - # Release AR's checkout so the native connection can write. ActiveRecord::Base.connection_pool.release_connection SQLite3::Database.new(path) do |db| @@ -163,12 +165,22 @@ class Room::MessagesCountForeignConnectionTest < ActiveSupport::TestCase ActiveRecord::Base.connection_pool.release_connection + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + db.execute("UPDATE messages SET room_id = ? WHERE id = ?", [ @other_room.id, foreign_id ]) + end + + assert_equal before, @room.reload.messages_count + assert_equal other_before + 1, @other_room.reload.messages_count + + ActiveRecord::Base.connection_pool.release_connection + SQLite3::Database.new(path) do |db| db.busy_timeout = 5_000 db.execute("DELETE FROM messages WHERE id = ?", [ foreign_id ]) end assert_equal before, @room.reload.messages_count - assert_equal @room.messages.count, @room.messages_count + assert_equal other_before, @other_room.reload.messages_count end end diff --git a/test/test_helper.rb b/test/test_helper.rb index cf47a46..6e3fa8b 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -13,8 +13,8 @@ Room::MessagesCount.ensure! WebMock.enable! module RoomMessagesCountFixtures - # Fixture YAML loads alphabetically, so messages are inserted before rooms. - # INSERT triggers then update zero room rows; reconcile once after load. + # Fixture YAML still inserts messages before rooms via some load paths, so + # INSERT triggers can update zero room rows. Reconcile once after load. def load_fixtures(config) fixtures = super Room::MessagesCount.backfill! @@ -28,15 +28,13 @@ class ActiveSupport::TestCase parallelize(workers: :number_of_processors) - # Setup all fixtures in test/fixtures/*.yml for all tests in alphabetical order. - fixtures :all + # Prefer rooms before messages when the loader honors declaration order. + fixtures :accounts, :users, :rooms, :memberships, :messages, "action_text/rich_texts", + :boosts, :searches, :sessions, :webhooks, "push/subscriptions" include SessionTestHelper, MentionTestHelper, TurboTestHelper, DnsTestHelper setup do - # DROP TRIGGER commits outside transactional fixtures; put triggers back each test. - Room::MessagesCount.ensure! - ActionCable.server.pubsub.clear Rails.configuration.tap do |config| @@ -52,4 +50,3 @@ class ActiveSupport::TestCase WebMock.reset! end end -