mirror of
https://github.com/basecamp/once-campfire.git
synced 2026-10-08 07:40:08 +09:00
Keep bot page totals on SQLite-maintained rooms.messages_count
X-Total-Count on GET /rooms/:id/:bot_key/messages was COUNT(*) of the room on every page. Serve it from rooms.messages_count updated by SQLite triggers so Rails, bulk SQL, and foreign writers stay in step — without ActiveRecord counter_cache callbacks those paths skip. Fixes #309. Co-authored-by: Thomas Klemm <github@tklemm.eu>
This commit is contained in:
@@ -37,7 +37,7 @@ class Messages::ByBotsController < MessagesController
|
||||
end
|
||||
|
||||
def set_pagination_headers
|
||||
headers["X-Total-Count"] = @room.messages.count.to_s
|
||||
headers["X-Total-Count"] = @room.messages_count.to_s
|
||||
|
||||
if next_page = next_page_params
|
||||
headers["Link"] = %(<#{room_bot_messages_url(@room, params[:bot_key], **next_page)}>; rel="next")
|
||||
|
||||
@@ -0,0 +1,64 @@
|
||||
# Keeps rooms.messages_count correct for every SQLite writer — Rails, bulk SQL,
|
||||
# and foreign connections — without ActiveRecord counter_cache callbacks that
|
||||
# those paths skip (and that would double-count if combined with triggers).
|
||||
class Room::MessagesCount
|
||||
INSERT_TRIGGER = "messages_ai_rooms_messages_count"
|
||||
DELETE_TRIGGER = "messages_ad_rooms_messages_count"
|
||||
UPDATE_TRIGGER = "messages_au_rooms_messages_count"
|
||||
|
||||
class << self
|
||||
def install!(connection = ActiveRecord::Base.connection)
|
||||
uninstall!(connection)
|
||||
connection.execute <<~SQL
|
||||
CREATE TRIGGER #{INSERT_TRIGGER} AFTER INSERT ON messages
|
||||
BEGIN
|
||||
UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id;
|
||||
END
|
||||
SQL
|
||||
connection.execute <<~SQL
|
||||
CREATE TRIGGER #{DELETE_TRIGGER} AFTER DELETE ON messages
|
||||
BEGIN
|
||||
UPDATE rooms SET messages_count = messages_count - 1 WHERE id = OLD.room_id;
|
||||
END
|
||||
SQL
|
||||
connection.execute <<~SQL
|
||||
CREATE TRIGGER #{UPDATE_TRIGGER} AFTER UPDATE OF room_id ON messages
|
||||
WHEN OLD.room_id IS NOT NEW.room_id
|
||||
BEGIN
|
||||
UPDATE rooms SET messages_count = messages_count - 1 WHERE id = OLD.room_id;
|
||||
UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id;
|
||||
END
|
||||
SQL
|
||||
end
|
||||
|
||||
def uninstall!(connection = ActiveRecord::Base.connection)
|
||||
[ INSERT_TRIGGER, DELETE_TRIGGER, UPDATE_TRIGGER ].each do |name|
|
||||
connection.execute("DROP TRIGGER IF EXISTS #{name}")
|
||||
end
|
||||
end
|
||||
|
||||
def backfill!(connection = ActiveRecord::Base.connection)
|
||||
connection.execute <<~SQL
|
||||
UPDATE rooms SET messages_count = (
|
||||
SELECT COUNT(*) FROM messages WHERE messages.room_id = rooms.id
|
||||
)
|
||||
SQL
|
||||
end
|
||||
|
||||
# schema.rb does not dump SQLite triggers; reinstall after schema:load.
|
||||
def ensure!(connection = ActiveRecord::Base.connection)
|
||||
return unless connection.data_source_exists?(:rooms)
|
||||
return unless connection.column_exists?(:rooms, :messages_count)
|
||||
return if trigger_installed?(connection, INSERT_TRIGGER)
|
||||
|
||||
install!(connection)
|
||||
end
|
||||
|
||||
def trigger_installed?(connection, name)
|
||||
connection.select_value(
|
||||
"SELECT 1 FROM sqlite_master WHERE type = 'trigger' AND name = #{connection.quote(name)}"
|
||||
).present?
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -0,0 +1,10 @@
|
||||
# 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).
|
||||
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
|
||||
rescue ActiveRecord::NoDatabaseError, ActiveRecord::ConnectionNotEstablished
|
||||
end
|
||||
@@ -0,0 +1,17 @@
|
||||
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
|
||||
|
||||
# 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.
|
||||
Room::MessagesCount.backfill!(connection)
|
||||
Room::MessagesCount.install!(connection)
|
||||
end
|
||||
|
||||
def down
|
||||
Room::MessagesCount.uninstall!(connection)
|
||||
remove_column :rooms, :messages_count if column_exists?(:rooms, :messages_count)
|
||||
end
|
||||
end
|
||||
Generated
+4
-1
@@ -10,7 +10,7 @@
|
||||
#
|
||||
# It's strongly recommended that you check this file into your version control system.
|
||||
|
||||
ActiveRecord::Schema[8.2].define(version: 2026_10_05_022000) do
|
||||
ActiveRecord::Schema[8.2].define(version: 2026_10_07_190000) do
|
||||
create_table "accounts", force: :cascade do |t|
|
||||
t.datetime "created_at", null: false
|
||||
t.text "custom_styles"
|
||||
@@ -124,6 +124,7 @@ ActiveRecord::Schema[8.2].define(version: 2026_10_05_022000) do
|
||||
t.string "name"
|
||||
t.string "type", null: false
|
||||
t.datetime "updated_at", null: false
|
||||
t.integer "messages_count", default: 0, null: false
|
||||
end
|
||||
|
||||
create_table "searches", force: :cascade do |t|
|
||||
@@ -182,4 +183,6 @@ ActiveRecord::Schema[8.2].define(version: 2026_10_05_022000) 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
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
# schema.rb cannot dump SQLite triggers. Reinstall after loads, and re-append
|
||||
# Room::MessagesCount.install! after dumps so the call is not lost.
|
||||
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|
|
||||
next unless Rake::Task.task_defined?(task_name)
|
||||
|
||||
Rake::Task[task_name].enhance do
|
||||
Rake::Task[enhancement].invoke
|
||||
end
|
||||
end
|
||||
@@ -1,6 +1,9 @@
|
||||
require "test_helper"
|
||||
require "active_record/testing/query_assertions"
|
||||
|
||||
class Messages::ByBotsControllerTest < ActionDispatch::IntegrationTest
|
||||
include ActiveRecord::Assertions::QueryAssertions
|
||||
|
||||
setup do
|
||||
@room = rooms(:watercooler)
|
||||
end
|
||||
@@ -93,11 +96,14 @@ class Messages::ByBotsControllerTest < ActionDispatch::IntegrationTest
|
||||
@room.messages.create!(body: "Filler #{i}", creator: users(:jason), client_message_id: "filler-#{i}")
|
||||
end
|
||||
|
||||
get room_bot_messages_url(@room, users(:bender).bot_key)
|
||||
assert_no_queries_match(/SELECT COUNT\(\*\) FROM "messages"/i) do
|
||||
get room_bot_messages_url(@room, users(:bender).bot_key)
|
||||
end
|
||||
assert_response :success
|
||||
|
||||
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
|
||||
|
||||
@@ -119,7 +125,10 @@ class Messages::ByBotsControllerTest < ActionDispatch::IntegrationTest
|
||||
end
|
||||
|
||||
test "index in a room with no messages" do
|
||||
get room_bot_messages_url(rooms(:bender_and_kevin), users(:bender).bot_key)
|
||||
room = rooms(:bender_and_kevin)
|
||||
assert_equal 0, room.messages_count
|
||||
|
||||
get room_bot_messages_url(room, users(:bender).bot_key)
|
||||
assert_response :success
|
||||
|
||||
assert_equal [], JSON.parse(response.body)
|
||||
|
||||
@@ -0,0 +1,174 @@
|
||||
require "test_helper"
|
||||
require "sqlite3"
|
||||
|
||||
class Room::MessagesCountTest < ActiveSupport::TestCase
|
||||
setup do
|
||||
@room = rooms(:designers)
|
||||
@other_room = rooms(:pets)
|
||||
synchronize!(@room, @other_room)
|
||||
end
|
||||
|
||||
test "ActiveRecord create and destroy adjust the counter once" do
|
||||
assert_difference -> { @room.reload.messages_count }, +1 do
|
||||
assert_difference -> { @room.messages.count }, +1 do
|
||||
@room.messages.create!(creator: users(:jason), body: "Hello", client_message_id: "count-ar-create")
|
||||
end
|
||||
end
|
||||
|
||||
assert_equal @room.messages.count, @room.reload.messages_count
|
||||
|
||||
assert_difference -> { @room.reload.messages_count }, -1 do
|
||||
assert_difference -> { @room.messages.count }, -1 do
|
||||
@room.messages.order(:id).last.destroy
|
||||
end
|
||||
end
|
||||
|
||||
assert_equal @room.messages.count, @room.reload.messages_count
|
||||
end
|
||||
|
||||
test "bulk insert_all and delete_all keep the counter in step" do
|
||||
rows = Array.new(3) do |i|
|
||||
{
|
||||
room_id: @room.id,
|
||||
creator_id: users(:david).id,
|
||||
client_message_id: "count-bulk-#{i}",
|
||||
created_at: Time.current,
|
||||
updated_at: Time.current
|
||||
}
|
||||
end
|
||||
|
||||
assert_difference -> { @room.reload.messages_count }, +3 do
|
||||
Message.insert_all!(rows)
|
||||
end
|
||||
|
||||
assert_equal @room.messages.count, @room.reload.messages_count
|
||||
|
||||
assert_difference -> { @room.reload.messages_count }, -3 do
|
||||
@room.messages.where(client_message_id: rows.map { it[:client_message_id] }).delete_all
|
||||
end
|
||||
|
||||
assert_equal @room.messages.count, @room.reload.messages_count
|
||||
end
|
||||
|
||||
test "rolled back writes leave the counter unchanged" do
|
||||
before = @room.reload.messages_count
|
||||
|
||||
Message.transaction do
|
||||
@room.messages.create!(creator: users(:jason), body: "Nope", client_message_id: "count-rollback")
|
||||
raise ActiveRecord::Rollback
|
||||
end
|
||||
|
||||
assert_equal before, @room.reload.messages_count
|
||||
assert_nil Message.find_by(client_message_id: "count-rollback")
|
||||
end
|
||||
|
||||
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
|
||||
message.update!(room: @other_room)
|
||||
end
|
||||
end
|
||||
|
||||
assert_equal @room.messages.count, @room.reload.messages_count
|
||||
assert_equal @other_room.messages.count, @other_room.reload.messages_count
|
||||
end
|
||||
|
||||
test "ensure! installs missing triggers without changing existing counts" do
|
||||
before = @room.reload.messages_count
|
||||
Room::MessagesCount.uninstall!
|
||||
|
||||
assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER)
|
||||
|
||||
Room::MessagesCount.ensure!
|
||||
|
||||
assert Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER)
|
||||
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
|
||||
Room::MessagesCount.uninstall!
|
||||
ActiveRecord::Base.connection.execute <<~SQL
|
||||
UPDATE rooms SET messages_count = (
|
||||
SELECT COUNT(*) FROM messages WHERE messages.room_id = rooms.id
|
||||
)
|
||||
SQL
|
||||
Room::MessagesCount.install!
|
||||
|
||||
assert_equal @room.messages.count, @room.reload.messages_count
|
||||
|
||||
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!
|
||||
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
|
||||
|
||||
# Release AR's checkout so the native connection can write.
|
||||
ActiveRecord::Base.connection_pool.release_connection
|
||||
|
||||
SQLite3::Database.new(path) do |db|
|
||||
db.busy_timeout = 5_000
|
||||
db.execute(
|
||||
"INSERT INTO messages (room_id, creator_id, client_message_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?)",
|
||||
[ @room.id, users(:david).id, "count-foreign-insert", now, now ]
|
||||
)
|
||||
end
|
||||
|
||||
assert_equal before + 1, @room.reload.messages_count
|
||||
assert_equal @room.messages.count, @room.messages_count
|
||||
|
||||
foreign_id = Message.find_by!(client_message_id: "count-foreign-insert").id
|
||||
|
||||
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
|
||||
end
|
||||
end
|
||||
@@ -6,10 +6,25 @@ require "mocha/minitest"
|
||||
require "webmock/minitest"
|
||||
require "turbo/broadcastable/test_helper"
|
||||
|
||||
# maintain_test_schema! may reload schema.rb after after_initialize; triggers are
|
||||
# not dumped, so ensure they exist before fixtures insert messages.
|
||||
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.
|
||||
def load_fixtures(config)
|
||||
fixtures = super
|
||||
Room::MessagesCount.backfill!
|
||||
fixtures
|
||||
end
|
||||
end
|
||||
|
||||
class ActiveSupport::TestCase
|
||||
include ActiveJob::TestHelper
|
||||
prepend RoomMessagesCountFixtures
|
||||
|
||||
parallelize(workers: :number_of_processors)
|
||||
|
||||
@@ -19,6 +34,9 @@ class ActiveSupport::TestCase
|
||||
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|
|
||||
@@ -34,3 +52,4 @@ class ActiveSupport::TestCase
|
||||
WebMock.reset!
|
||||
end
|
||||
end
|
||||
|
||||
|
||||
Reference in New Issue
Block a user