Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/boards_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
48 changes: 48 additions & 0 deletions test/controllers/boards_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
46 changes: 46 additions & 0 deletions test/controllers/users_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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