Merge branch 'revert-06fee04e' into 'master'

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

See merge request gitlab-org/gitlab!28333

app/models/audit_event.rb

@@ -30,26 +30,12 @@ class AuditEvent < ApplicationRecord
   end
 
   def author_name
-    lazy_author.name
+    self.user.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 = build_author(author)
+    @author = author
     @entity = entity
     @details = details
   end
@@ -49,14 +49,6 @@ 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
+    @events = audit_log_events.page(params[:page]).per(PER_PAGE).without_count
 
     @entity = case audit_logs_params[:entity_type]
               when 'User'
@@ -27,10 +27,7 @@ class Admin::AuditLogsController < Admin::ApplicationController
   private
 
   def audit_log_events
-    events = AuditLogFinder.new(audit_logs_params).execute
-    events = events.page(params[:page]).per(PER_PAGE).without_count
-
-    Gitlab::Audit::Events::Preloader.preload!(events)
+    AuditLogFinder.new(audit_logs_params).execute
   end
 
   def check_license_admin_audit_log_available!

ee/app/controllers/groups/audit_events_controller.rb

@@ -11,9 +11,7 @@ class Groups::AuditEventsController < Groups::ApplicationController
   layout 'group_settings'
 
   def index
-    events = AuditLogFinder.new(audit_logs_params).execute.page(params[:page])
-
-    @events = Gitlab::Audit::Events::Preloader.preload!(events)
+    @events = AuditLogFinder.new(audit_logs_params).execute.page(params[:page])
   end
 
   private

ee/app/controllers/projects/audit_events_controller.rb

@@ -12,9 +12,7 @@ class Projects::AuditEventsController < Projects::ApplicationController
   layout 'project_settings'
 
   def index
-    events = AuditLogFinder.new(audit_logs_params).execute.page(params[:page])
-
-    @events = Gitlab::Audit::Events::Preloader.preload!(events)
+    @events = AuditLogFinder.new(audit_logs_params).execute.page(params[:page])
   end
 
   private

ee/app/models/ee/audit_event.rb

@@ -5,22 +5,36 @@ 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
-      lazy_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
     end
 
     def present
       AuditEventPresenter.new(self)
     end
 
-    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
+    def authored_by_unauth_user?
+      author_id == UNAUTH_USER_AUTHOR_ID
     end
   end
 end

ee/app/presenters/audit_event_presenter.rb

@@ -4,11 +4,13 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
   presents :audit_event
 
   def author_name
-    author = audit_event.lazy_author
+    user = audit_event.user
 
-    return author.name if author.is_a?(Gitlab::Audit::NullAuthor)
-
-    link_to(author.name, user_path(author))
+    if user
+      link_to(user.name, user_path(user))
+    else
+      audit_event.author_name
+    end
   end
 
   def target
@@ -24,9 +26,9 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
   end
 
   def object
-    entity = audit_event.lazy_entity
+    entity = audit_event.entity
 
-    return if entity.is_a?(Gitlab::Audit::NullEntity)
+    return unless entity
 
     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.name,
+        author_name: @author,
         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.id,
+        author_id: @author.respond_to?(:id) ? @author.id : AuditEvent::UNAUTH_USER_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.id,
+        author_id: @author.respond_to?(:id) ? @author.id : AuditEvent::UNAUTH_USER_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,38 +4,33 @@ FactoryBot.define do
   factory :audit_event, class: 'SecurityEvent', aliases: [:user_audit_event] do
     user
 
-    transient { target_user { create(:user) } }
-
     entity_type { 'User' }
-    entity_id   { target_user.id }
+    entity_id   { user.id }
     details do
       {
         change: 'email address',
         from: 'admin@gitlab.com',
         to: 'maintainer@gitlab.com',
         author_name: user.name,
-        target_id: target_user.id,
+        target_id: user.id,
         target_type: 'User',
-        target_details: target_user.name,
+        target_details: user.name,
         ip_address: '127.0.0.1',
-        entity_path: target_user.username
+        entity_path: user.username
       }
     end
 
     trait :project_event do
-      transient { target_project { create(:project) } }
-
       entity_type { 'Project' }
-      entity_id   { target_project.id }
+      entity_id   { create(:project).id }
       details do
         {
-          change: 'packges_enabled',
-          from: true,
-          to: false,
+          add: 'user_access',
+          as: 'Developer',
           author_name: user.name,
-          target_id: target_project.id,
-          target_type: 'Project',
-          target_details: target_project.name,
+          target_id: user.id,
+          target_type: 'User',
+          target_details: user.name,
           ip_address: '127.0.0.1',
           entity_path: 'gitlab.org/gitlab-ce'
         }
@@ -43,19 +38,17 @@ FactoryBot.define do
     end
 
     trait :group_event do
-      transient { target_group { create(:group) } }
-
       entity_type { 'Group' }
-      entity_id   { target_group.id }
+      entity_id   { create(:group).id }
       details do
         {
           change: 'project_creation_level',
           from: nil,
           to: 'Developers + Maintainers',
-          author_name: user.name,
-          target_id: target_group.id,
+          author_name: 'Administrator',
+          target_id: 1,
           target_type: 'Group',
-          target_details: target_group.name,
+          target_details: "gitlab-org",
           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 a NullEntity' do
-        expect(event.entity).to be_a(Gitlab::Audit::NullEntity)
+      it 'returns nil' do
+        expect(event.entity).to be_blank
       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.entity_id)
+            expect(response["entity_id"]).to eq(user_audit_event.user.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.entity_id)
+              expect(json_response["entity_id"]).to eq(user_audit_event.user.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) { User.new(name: author_name) }
+    let(:current_user) { instance_spy(User, 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' }