Merge branch 'tle-view-audit-events-query-opt' into 'master'

Avoid N+1 queries on Audit Events controllers

See merge request gitlab-org/gitlab!26245

app/models/audit_event.rb

@@ -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')

app/services/audit_event_service.rb

@@ -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,

ee/app/controllers/admin/audit_logs_controller.rb

@@ -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!

ee/app/controllers/groups/audit_events_controller.rb

@@ -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

ee/app/controllers/projects/audit_events_controller.rb

@@ -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

ee/app/models/ee/audit_event.rb

@@ -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

ee/app/presenters/audit_event_presenter.rb

@@ -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

ee/app/services/ee/audit_event_service.rb

@@ -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.

ee/changelogs/unreleased/fix-audit-events-controllers-n-plus-1.yml

+---
+title: Fix N+1 queries in Audit Events controllers
+merge_request: 26245
+author:
+type: performance

ee/lib/gitlab/audit/deleted_author.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Audit
+    class DeletedAuthor < Gitlab::Audit::NullAuthor
+    end
+  end
+end

ee/lib/gitlab/audit/events/preloader.rb

+# 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

ee/lib/gitlab/audit/null_author.rb

+# 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

ee/lib/gitlab/audit/null_entity.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Audit
+    class NullEntity
+    end
+  end
+end

ee/lib/gitlab/audit/unauthenticated_author.rb

+# 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

ee/spec/factories/audit_events.rb

@@ -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"
         }

ee/spec/lib/gitlab/audit/events/preloader_spec.rb

+# 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

ee/spec/lib/gitlab/audit/null_author_spec.rb

+# 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

ee/spec/lib/gitlab/audit/unauthenticated_author_spec.rb

+# 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

ee/spec/models/audit_event_spec.rb

@@ -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

ee/spec/requests/admin/audit_events_spec.rb

+# 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

ee/spec/requests/api/audit_events_spec.rb

@@ -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

ee/spec/requests/groups/audit_events_spec.rb

+# 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

ee/spec/requests/projects/audit_events_spec.rb

+# 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

ee/spec/services/audit_event_service_spec.rb

@@ -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' }