diff --git a/app/controllers/boards_controller.rb b/app/controllers/boards_controller.rb index 5b8763b71..9399c50e7 100644 --- a/app/controllers/boards_controller.rb +++ b/app/controllers/boards_controller.rb @@ -5,7 +5,7 @@ class BoardsController < ApplicationController before_action :ensure_permission_to_admin_board, only: %i[ update destroy ] def index - set_page_and_extract_portion_from Current.user.boards + set_page_and_extract_portion_from Current.user.boards.includes(creator: :identity) end def show diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 54c18704c..7062b85e5 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -3,7 +3,7 @@ class UsersController < ApplicationController before_action :ensure_permission_to_change_user, only: %i[ update destroy ] def index - set_page_and_extract_portion_from Current.account.users.active.alphabetically + set_page_and_extract_portion_from Current.account.users.active.alphabetically.includes(:identity) end def show diff --git a/test/controllers/boards_controller_test.rb b/test/controllers/boards_controller_test.rb index 094cca56b..fd1180b4b 100644 --- a/test/controllers/boards_controller_test.rb +++ b/test/controllers/boards_controller_test.rb @@ -240,4 +240,52 @@ class BoardsControllerTest < ActionDispatch::IntegrationTest assert_response :no_content end + + test "index as JSON avoids N+1 queries on creator and identity" do + # Warm up the cache and establish baseline + get boards_path, as: :json + assert_response :success + + # Count queries for the actual test + queries = track_sql_queries do + get boards_path, as: :json + end + + # Filter relevant queries for the index action + # Exclude current user lookup (happens during authentication) + board_queries = queries.select { |q| q =~ /SELECT.*FROM [`"]boards[`"]/ && q !~ /COUNT/ } + # The includes(creator: :identity) generates a query with IN clause for users + creator_queries = queries.select { |q| + q =~ /SELECT.*FROM [`"]users[`"]/ && + q =~ /WHERE.*IN \(/ # The includes query uses IN clause + } + # The includes query for identities should also use IN clause + identity_queries = queries.select { |q| + q =~ /SELECT.*FROM [`"]identities[`"]/ && + q =~ /WHERE.*IN \(/ + } + + # Should have exactly 1 query for boards, 1 for users (creators), and 1 for identities + assert_equal 1, board_queries.size, "Expected 1 board query, got #{board_queries.size}: #{board_queries.inspect}" + assert_equal 1, creator_queries.size, "Expected 1 creator query via includes, got #{creator_queries.size}: #{creator_queries.inspect}" + assert_equal 1, identity_queries.size, "Expected 1 identity query via includes, got #{identity_queries.size}: #{identity_queries.inspect}" + + # Verify response contains creator with email address + json = @response.parsed_body + first_board = json.first + assert first_board["creator"].present?, "Creator should be present in response" + assert first_board["creator"]["email_address"].present?, "Creator email address should be present in response" + end + + private + def track_sql_queries(&block) + queries = [] + subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload| + queries << payload[:sql] unless payload[:name] == "SCHEMA" + end + yield + queries + ensure + ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber + end end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index ab91a4ab6..0f0bc129c 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -144,4 +144,50 @@ class UsersControllerTest < ActionDispatch::IntegrationTest assert_response :no_content end + + test "index as JSON avoids N+1 queries on identity" do + sign_in_as :kevin + + # Warm up the cache and establish baseline + get users_path, as: :json + assert_response :success + + # Count queries for the actual test + queries = track_sql_queries do + get users_path, as: :json + end + + # Filter relevant queries (SELECT on users and identities for the index action) + # Exclude the current user lookup query (happens during authentication) + index_user_queries = queries.select { |q| + q =~ /SELECT.*FROM [`"]users[`"]/ && + q =~ /ORDER BY lower\(name\)/ && # The index query has ORDER BY lower(name) + q !~ /COUNT/ + } + # The includes(:identity) generates a query with IN clause for multiple identities + index_identity_queries = queries.select { |q| + q =~ /SELECT.*FROM [`"]identities[`"]/ && + q =~ /WHERE.*IN \(/ # The includes query uses IN clause + } + + # Should have exactly 1 query for users and 1 query for identities (via includes) + assert_equal 1, index_user_queries.size, "Expected 1 user index query, got #{index_user_queries.size}: #{index_user_queries.inspect}" + assert_equal 1, index_identity_queries.size, "Expected 1 identity query via includes, got #{index_identity_queries.size}: #{index_identity_queries.inspect}" + + # Verify response contains email addresses (which require identity association) + json = @response.parsed_body + assert json.first["email_address"].present?, "Email address should be present in response" + end + + private + def track_sql_queries(&block) + queries = [] + subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload| + queries << payload[:sql] unless payload[:name] == "SCHEMA" + end + yield + queries + ensure + ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber + end end