Commit e1b22cc5 authored by Tan Le's avatar Tan Le Committed by Douglas Barbosa Alexandre

Avoid N+1 queries when fetching author name

For audit event view, we need to fetch author and the associated
name, and this results in unnecessary extra SQL calls. This change
introduces `BatchLoader` to lazily load associated authors and speeds up
audit events queries via both web application and API.
parent 2af53741
......@@ -30,12 +30,26 @@ class AuditEvent < ApplicationRecord
end
def author_name
self.user.name
lazy_author.name
end
def formatted_details
details.merge(details.slice(:from, :to).transform_values(&:to_s))
end
def lazy_author
BatchLoader.for(author_id).batch(default_value: default_author_value) do |author_ids, loader|
User.where(id: author_ids).find_each do |user|
loader.call(user.id, user)
end
end
end
private
def default_author_value
::Gitlab::Audit::NullAuthor.for(author_id, details[:author_name])
end
end
AuditEvent.prepend_if_ee('EE::AuditEvent')
......@@ -13,7 +13,7 @@ class AuditEventService
#
# @return [AuditEventService]
def initialize(author, entity, details = {})
@author = author
@author = build_author(author)
@entity = entity
@details = details
end
......@@ -49,6 +49,14 @@ class AuditEventService
private
def build_author(author)
if author.is_a?(User)
author
else
Gitlab::Audit::UnauthenticatedAuthor.new(name: author)
end
end
def base_payload
{
author_id: @author.id,
......
......@@ -10,7 +10,7 @@ class Admin::AuditLogsController < Admin::ApplicationController
PER_PAGE = 25
def index
@events = audit_log_events.page(params[:page]).per(PER_PAGE).without_count
@events = audit_log_events
@entity = case audit_logs_params[:entity_type]
when 'User'
......@@ -27,7 +27,10 @@ class Admin::AuditLogsController < Admin::ApplicationController
private
def audit_log_events
AuditLogFinder.new(audit_logs_params).execute
events = AuditLogFinder.new(audit_logs_params).execute
events = events.page(params[:page]).per(PER_PAGE).without_count
Gitlab::Audit::Events::Preloader.preload!(events)
end
def check_license_admin_audit_log_available!
......
......@@ -11,7 +11,9 @@ class Groups::AuditEventsController < Groups::ApplicationController
layout 'group_settings'
def index
@events = AuditLogFinder.new(audit_logs_params).execute.page(params[:page])
events = AuditLogFinder.new(audit_logs_params).execute.page(params[:page])
@events = Gitlab::Audit::Events::Preloader.preload!(events)
end
private
......
......@@ -12,7 +12,9 @@ class Projects::AuditEventsController < Projects::ApplicationController
layout 'project_settings'
def index
@events = AuditLogFinder.new(audit_logs_params).execute.page(params[:page])
events = AuditLogFinder.new(audit_logs_params).execute.page(params[:page])
@events = Gitlab::Audit::Events::Preloader.preload!(events)
end
private
......
......@@ -5,36 +5,22 @@ module EE
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
# While tracking events that could take place even when
# a user is not logged in, (eg: downloading repo of a public project),
# we set the author_id of such events as -1
UNAUTH_USER_AUTHOR_ID = -1
# Events that are authored by unathenticated users, should be
# shown as authored by `An unauthenticated user` in the UI.
UNAUTH_USER_AUTHOR_NAME = 'An unauthenticated user'.freeze
override :author_name
def author_name
if (author_name = details[:author_name].presence || user&.name)
author_name
elsif authored_by_unauth_user?
UNAUTH_USER_AUTHOR_NAME
end
end
def entity
return unless entity_type && entity_id
# Avoiding exception if the record doesn't exist
@entity ||= entity_type.constantize.find_by_id(entity_id) # rubocop:disable Gitlab/ModuleWithInstanceVariables
lazy_entity
end
def present
AuditEventPresenter.new(self)
end
def authored_by_unauth_user?
author_id == UNAUTH_USER_AUTHOR_ID
def lazy_entity
BatchLoader.for(entity_id)
.batch(
key: entity_type, default_value: ::Gitlab::Audit::NullEntity.new
) do |ids, loader, args|
model = Object.const_get(args[:key], false)
model.where(id: ids).find_each { |record| loader.call(record.id, record) }
end
end
end
end
......@@ -4,13 +4,11 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
presents :audit_event
def author_name
user = audit_event.user
author = audit_event.lazy_author
if user
link_to(user.name, user_path(user))
else
audit_event.author_name
end
return author.name if author.is_a?(Gitlab::Audit::NullAuthor)
link_to(author.name, user_path(author))
end
def target
......@@ -26,9 +24,9 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
end
def object
entity = audit_event.entity
entity = audit_event.lazy_entity
return unless entity
return if entity.is_a?(Gitlab::Audit::NullEntity)
link_to(details[:entity_path] || entity.name, entity).html_safe
end
......
......@@ -89,7 +89,7 @@ module EE
@details = {
failed_login: auth.upcase,
author_name: @author,
author_name: @author.name,
target_details: @author,
ip_address: ip
}
......@@ -141,7 +141,7 @@ module EE
@details[:entity_path] = @entity&.full_path if admin_audit_log_enabled?
SecurityEvent.create(
author_id: @author.respond_to?(:id) ? @author.id : AuditEvent::UNAUTH_USER_AUTHOR_ID,
author_id: @author.id,
entity_id: @entity.respond_to?(:id) ? @entity.id : -1,
entity_type: 'User',
details: @details
......@@ -213,7 +213,7 @@ module EE
override :base_payload
def base_payload
{
author_id: @author.respond_to?(:id) ? @author.id : AuditEvent::UNAUTH_USER_AUTHOR_ID,
author_id: @author.id,
# `@author.respond_to?(:id)` is to support cases where we need to log events
# that could take place even when a user is unathenticated, Eg: downloading a public repo.
# For such events, it is not mandatory that an author is always present.
......
---
title: Fix N+1 queries in Audit Events controllers
merge_request: 26245
author:
type: performance
# frozen_string_literal: true
module Gitlab
module Audit
class DeletedAuthor < Gitlab::Audit::NullAuthor
end
end
end
# frozen_string_literal: true
module Gitlab
module Audit
module Events
class Preloader
def self.preload!(audit_events)
audit_events.tap do |audit_events|
audit_events.each do |audit_event|
audit_event.lazy_author
audit_event.lazy_entity
end
end
end
end
end
end
end
# frozen_string_literal: true
module Gitlab
module Audit
class NullAuthor
attr_reader :id, :name
# Creates an Author
#
# While tracking events that could take place even when
# a user is not logged in, (eg: downloading repo of a public project),
# we set the author_id of such events as -1
#
# @param [Integer] id
# @param [String] name
#
# @return [Gitlab::Audit::UnauthenticatedAuthor, Gitlab::Audit::DeletedAuthor]
def self.for(id, name)
if id == -1
Gitlab::Audit::UnauthenticatedAuthor.new(name: name)
else
Gitlab::Audit::DeletedAuthor.new(id: id, name: name)
end
end
def initialize(id:, name:)
@id = id
@name = name
end
end
end
end
# frozen_string_literal: true
module Gitlab
module Audit
class NullEntity
end
end
end
# frozen_string_literal: true
module Gitlab
module Audit
class UnauthenticatedAuthor < Gitlab::Audit::NullAuthor
# Events that are authored by unathenticated users, should be
# shown as authored by `An unauthenticated user` in the UI.
def initialize(name: nil)
super(id: -1, name: (name || 'An unauthenticated user'))
end
end
end
end
......@@ -4,33 +4,38 @@ FactoryBot.define do
factory :audit_event, class: 'SecurityEvent', aliases: [:user_audit_event] do
user
transient { target_user { create(:user) } }
entity_type { 'User' }
entity_id { user.id }
entity_id { target_user.id }
details do
{
change: 'email address',
from: 'admin@gitlab.com',
to: 'maintainer@gitlab.com',
author_name: user.name,
target_id: user.id,
target_id: target_user.id,
target_type: 'User',
target_details: user.name,
target_details: target_user.name,
ip_address: '127.0.0.1',
entity_path: user.username
entity_path: target_user.username
}
end
trait :project_event do
transient { target_project { create(:project) } }
entity_type { 'Project' }
entity_id { create(:project).id }
entity_id { target_project.id }
details do
{
add: 'user_access',
as: 'Developer',
change: 'packges_enabled',
from: true,
to: false,
author_name: user.name,
target_id: user.id,
target_type: 'User',
target_details: user.name,
target_id: target_project.id,
target_type: 'Project',
target_details: target_project.name,
ip_address: '127.0.0.1',
entity_path: 'gitlab.org/gitlab-ce'
}
......@@ -38,17 +43,19 @@ FactoryBot.define do
end
trait :group_event do
transient { target_group { create(:group) } }
entity_type { 'Group' }
entity_id { create(:group).id }
entity_id { target_group.id }
details do
{
change: 'project_creation_level',
from: nil,
to: 'Developers + Maintainers',
author_name: 'Administrator',
target_id: 1,
author_name: user.name,
target_id: target_group.id,
target_type: 'Group',
target_details: "gitlab-org",
target_details: target_group.name,
ip_address: '127.0.0.1',
entity_path: "gitlab-org"
}
......
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Audit::Events::Preloader do
describe '.preload!' do
let_it_be(:audit_events) { create_list(:audit_event, 2) }
let(:audit_events_relation) { AuditEvent.where(id: audit_events.map(&:id)) }
subject { described_class.preload!(audit_events_relation) }
it 'returns an ActiveRecord::Relation' do
expect(subject).to be_an(ActiveRecord::Relation)
end
it 'preloads associated records' do
log = ActiveRecord::QueryRecorder.new do
subject.map do |event|
[event.author_name, event.lazy_entity.name]
end
end
# Expected queries when requesting for AuditEvent with associated records
#
# 1. On the audit_events table
# SELECT "audit_events".* FROM "audit_events"
# 2. On the users table for author_name
# SELECT "users".* FROM "users" WHERE "users"."id" IN (1, 3)
# 3. On the users table for entity name
# SELECT "users".* FROM "users" WHERE "users"."id" IN (2, 4)
#
expect(log.count).to eq(3)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Audit::NullAuthor do
describe '.for' do
it 'returns an DeletedAuthor' do
expect(described_class.for(666, 'Old Hat')).to be_a(Gitlab::Audit::DeletedAuthor)
end
it 'returns an UnauthenticatedAuthor when id equals -1', :aggregate_failures do
expect(described_class.for(-1, 'Frank')).to be_a(Gitlab::Audit::UnauthenticatedAuthor)
expect(described_class.for(-1, 'Frank')).to have_attributes(id: -1, name: 'Frank')
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Audit::UnauthenticatedAuthor do
describe '#initialize' do
it 'sets correct attributes' do
expect(described_class.new(name: 'Peppa Pig'))
.to have_attributes(id: -1, name: 'Peppa Pig')
end
it 'sets default name when it is not provided' do
expect(described_class.new)
.to have_attributes(id: -1, name: 'An unauthenticated user')
end
end
end
......@@ -91,8 +91,8 @@ RSpec.describe AuditEvent, type: :model do
context 'when entity does not exist' do
subject(:event) { described_class.new(entity_id: 99999, entity_type: 'User') }
it 'returns nil' do
expect(event.entity).to be_blank
it 'returns a NullEntity' do
expect(event.entity).to be_a(Gitlab::Audit::NullEntity)
end
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe 'view audit events' do
describe 'GET /audit_events' do
let_it_be(:admin) { create(:admin) }
let_it_be(:audit_event) { create(:user_audit_event) }
before do
stub_licensed_features(admin_audit_log: true)
login_as(admin)
end
it 'returns 200' do
send_request
expect(response.status).to eq(200)
end
it 'avoids N+1 DB queries', :request_store do
# warm up cache so these initial queries would not leak in our QueryRecorder
send_request
control = ActiveRecord::QueryRecorder.new(skip_cached: false) { send_request }
create_list(:user_audit_event, 2)
expect do
send_request
end.not_to exceed_all_query_limit(control)
end
def send_request
get admin_audit_logs_path
end
end
end
......@@ -107,7 +107,7 @@ describe API::AuditEvents do
expect(response["id"]).to eq(user_audit_event.id)
expect(response["author_id"]).to eq(user_audit_event.user.id)
expect(response["entity_id"]).to eq(user_audit_event.user.id)
expect(response["entity_id"]).to eq(user_audit_event.entity_id)
expect(response["entity_type"]).to eq('User')
expect(Time.parse(response["created_at"])).to be_like_time(user_audit_event.created_at)
expect(details).to eq user_audit_event.formatted_details.with_indifferent_access
......@@ -157,7 +157,7 @@ describe API::AuditEvents do
expect(json_response["id"]).to eq(user_audit_event.id)
expect(json_response["author_id"]).to eq(user_audit_event.user.id)
expect(json_response["entity_id"]).to eq(user_audit_event.user.id)
expect(json_response["entity_id"]).to eq(user_audit_event.entity_id)
expect(json_response["entity_type"]).to eq('User')
expect(Time.parse(json_response["created_at"])).to be_like_time(user_audit_event.created_at)
expect(details).to eq user_audit_event.formatted_details.with_indifferent_access
......
# frozen_string_literal: true
require 'spec_helper'
describe 'view audit events' do
describe 'GET /groups/:group/-/audit_events' do
let_it_be(:group) { create(:group) }
let_it_be(:user) { create(:user) }
let_it_be(:audit_event) { create(:group_audit_event, entity_id: group.id) }
before do
stub_licensed_features(audit_events: true)
group.add_owner(user)
login_as(user)
end
it 'returns 200' do
send_request
expect(response.status).to eq(200)
end
it 'avoids N+1 DB queries', :request_store do
# warm up cache so these initial queries would not leak in our QueryRecorder
send_request
control = ActiveRecord::QueryRecorder.new(skip_cached: false) { send_request }
create_list(:group_audit_event, 2, entity_id: group.id)
expect do
send_request
end.not_to exceed_all_query_limit(control)
end
def send_request
get group_audit_events_path(group)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe 'view audit events' do
describe 'GET /:namespace/:project/-/audit_events' do
let_it_be(:project) { create(:project, :repository) }
let_it_be(:user) { project.owner }
let_it_be(:audit_event) { create(:project_audit_event, entity_id: project.id) }
before do
stub_licensed_features(audit_events: true)
login_as(user)
end
it 'returns 200' do
send_request
expect(response.status).to eq(200)
end
it 'avoids N+1 DB queries', :request_store do
control = ActiveRecord::QueryRecorder.new(skip_cached: false) { send_request }
create_list(:project_audit_event, 2, entity_id: project.id)
expect do
send_request
end.not_to exceed_all_query_limit(control)
end
def send_request
get namespace_project_audit_events_path(
namespace_id: project.namespace,
project_id: project
)
end
end
end
......@@ -216,7 +216,7 @@ describe AuditEventService do
describe '#for_user' do
let(:author_name) { 'Administrator' }
let(:current_user) { instance_spy(User, name: author_name) }
let(:current_user) { User.new(name: author_name) }
let(:target_user_full_path) { 'ejohn' }
let(:user) { instance_spy(User, full_path: target_user_full_path) }
let(:custom_message) { 'Some strange event has occurred' }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment