Merge branch...

Merge branch '21033-controller-groups-groupmemberscontroller-index-executes-more-than-100-sql-queries-p80-108-3' into 'master'

Resolve group_member policy n+1

See merge request gitlab-org/gitlab!58668

app/models/group.rb

@@ -346,6 +346,10 @@ class Group < Namespace
     members_with_parents.owners.exists?(user_id: user)
   end
 
+  def blocked_owners
+    members.blocked.where(access_level: Gitlab::Access::OWNER)
+  end
+
   def has_maintainer?(user)
     return false unless user
 
@@ -358,14 +362,29 @@ class Group < Namespace
 
   # Check if user is a last owner of the group.
   def last_owner?(user)
-    has_owner?(user) && members_with_parents.owners.size == 1
+    has_owner?(user) && single_owner?
+  end
+
+  def member_last_owner?(member)
+    return member.last_owner unless member.last_owner.nil?
+
+    last_owner?(member.user)
   end
 
-  def last_blocked_owner?(user)
+  def single_owner?
+    members_with_parents.owners.size == 1
+  end
+
+  def single_blocked_owner?
+    blocked_owners.size == 1
+  end
+
+  def member_last_blocked_owner?(member)
+    return member.last_blocked_owner unless member.last_blocked_owner.nil?
+
     return false if members_with_parents.owners.any?
 
-    blocked_owners = members.blocked.where(access_level: Gitlab::Access::OWNER)
-    blocked_owners.size == 1 && blocked_owners.exists?(user_id: user)
+    single_blocked_owner? && blocked_owners.exists?(user_id: member.user)
   end
 
   def ldap_synced?

app/models/members/group_member.rb

@@ -26,6 +26,8 @@ class GroupMember < Member
   after_create :update_two_factor_requirement, unless: :invite?
   after_destroy :update_two_factor_requirement, unless: :invite?
 
+  attr_accessor :last_owner, :last_blocked_owner
+
   def self.access_level_roles
     Gitlab::Access.options_with_owner
   end

app/models/members/last_group_owner_assigner.rb

+# frozen_string_literal: true
+
+module Members
+  class LastGroupOwnerAssigner
+    def initialize(group, members)
+      @group = group
+      @members = members
+    end
+
+    def execute
+      @last_blocked_owner = no_owners_in_heirarchy? && group.single_blocked_owner?
+      @group_single_owner = owners.size == 1
+
+      members.each { |member| set_last_owner(member) }
+    end
+
+    private
+
+    attr_reader :group, :members, :last_blocked_owner, :group_single_owner
+
+    def no_owners_in_heirarchy?
+      owners.empty?
+    end
+
+    def set_last_owner(member)
+      member.last_owner = member.id.in?(owner_ids) && group_single_owner
+      member.last_blocked_owner = member.id.in?(blocked_owner_ids) && last_blocked_owner
+    end
+
+    def owner_ids
+      @owner_ids ||= owners.where(id: member_ids).ids
+    end
+
+    def blocked_owner_ids
+      @blocked_owner_ids ||= group.blocked_owners.where(id: member_ids).ids
+    end
+
+    def member_ids
+      @members_ids ||= members.pluck(:id)
+    end
+
+    def owners
+      @owners ||= group.members_with_parents.owners.load
+    end
+  end
+end

app/policies/group_member_policy.rb

@@ -4,7 +4,7 @@ class GroupMemberPolicy < BasePolicy
   delegate :group
 
   with_scope :subject
-  condition(:last_owner) { @subject.group.last_owner?(@subject.user) || @subject.group.last_blocked_owner?(@subject.user) }
+  condition(:last_owner) { @subject.group.member_last_owner?(@subject) || @subject.group.member_last_blocked_owner?(@subject) }
 
   desc "Membership is users' own"
   with_score 0

app/serializers/member_serializer.rb

@@ -2,4 +2,10 @@
 
 class MemberSerializer < BaseSerializer
   entity MemberEntity
+
+  def represent(members, opts = {})
+    Members::LastGroupOwnerAssigner.new(opts[:group], members).execute unless opts[:source].is_a?(Project)
+
+    super(members, opts)
+  end
 end

changelogs/unreleased/21033-controller-groups-groupmemberscontroller-index-executes-more-than-1.yml

+---
+title: Resolve group_member policy n+1
+merge_request: 58668
+author:
+type: performance

spec/models/group_spec.rb

@@ -681,39 +681,178 @@ RSpec.describe Group do
     end
   end
 
-  describe '#last_blocked_owner?' do
-    let(:blocked_user) { create(:user, :blocked) }
+  describe '#member_last_blocked_owner?' do
+    let_it_be(:blocked_user) { create(:user, :blocked) }
+
+    let(:member) { blocked_user.group_members.last }
 
     before do
       group.add_user(blocked_user, GroupMember::OWNER)
     end
 
-    it { expect(group.last_blocked_owner?(blocked_user)).to be_truthy }
+    context 'when last_blocked_owner is set' do
+      before do
+        expect(group).not_to receive(:members_with_parents)
+      end
+
+      it 'returns true' do
+        member.last_blocked_owner = true
+
+        expect(group.member_last_blocked_owner?(member)).to be(true)
+      end
+
+      it 'returns false' do
+        member.last_blocked_owner = false
+
+        expect(group.member_last_blocked_owner?(member)).to be(false)
+      end
+    end
+
+    context 'when last_blocked_owner is not set' do
+      it { expect(group.member_last_blocked_owner?(member)).to be(true) }
+
+      context 'with another active owner' do
+        before do
+          group.add_user(create(:user), GroupMember::OWNER)
+        end
+
+        it { expect(group.member_last_blocked_owner?(member)).to be(false) }
+      end
+
+      context 'with 2 blocked owners' do
+        before do
+          group.add_user(create(:user, :blocked), GroupMember::OWNER)
+        end
+
+        it { expect(group.member_last_blocked_owner?(member)).to be(false) }
+      end
+
+      context 'with owners from a parent' do
+        before do
+          parent_group = create(:group)
+          create(:group_member, :owner, group: parent_group)
+          group.update(parent: parent_group)
+        end
+
+        it { expect(group.member_last_blocked_owner?(member)).to be(false) }
+      end
+    end
+  end
+
+  context 'when analyzing blocked owners' do
+    let_it_be(:blocked_user) { create(:user, :blocked) }
+
+    describe '#single_blocked_owner?' do
+      context 'when there is only one blocked owner' do
+        before do
+          group.add_user(blocked_user, GroupMember::OWNER)
+        end
+
+        it 'returns true' do
+          expect(group.single_blocked_owner?).to eq(true)
+        end
+      end
+
+      context 'when there are multiple blocked owners' do
+        let_it_be(:blocked_user_2) { create(:user, :blocked) }
+
+        before do
+          group.add_user(blocked_user, GroupMember::OWNER)
+          group.add_user(blocked_user_2, GroupMember::OWNER)
+        end
+
+        it 'returns true' do
+          expect(group.single_blocked_owner?).to eq(false)
+        end
+      end
+
+      context 'when there are no blocked owners' do
+        it 'returns false' do
+          expect(group.single_blocked_owner?).to eq(false)
+        end
+      end
+    end
+
+    describe '#blocked_owners' do
+      let_it_be(:user) { create(:user) }
 
-    context 'with another active owner' do
       before do
-        group.add_user(create(:user), GroupMember::OWNER)
+        group.add_user(blocked_user, GroupMember::OWNER)
+        group.add_user(user, GroupMember::OWNER)
       end
 
-      it { expect(group.last_blocked_owner?(blocked_user)).to be_falsy }
+      it 'has only blocked owners' do
+        expect(group.blocked_owners.map(&:user)).to match([blocked_user])
+      end
     end
+  end
 
-    context 'with 2 blocked owners' do
+  describe '#single_owner?' do
+    let_it_be(:user) { create(:user) }
+
+    context 'when there is only one owner' do
       before do
-        group.add_user(create(:user, :blocked), GroupMember::OWNER)
+        group.add_user(user, GroupMember::OWNER)
       end
 
-      it { expect(group.last_blocked_owner?(blocked_user)).to be_falsy }
+      it 'returns true' do
+        expect(group.single_owner?).to eq(true)
+      end
     end
 
-    context 'with owners from a parent' do
+    context 'when there are multiple owners' do
+      let_it_be(:user_2) { create(:user) }
+
       before do
-        parent_group = create(:group)
-        create(:group_member, :owner, group: parent_group)
-        group.update(parent: parent_group)
+        group.add_user(user, GroupMember::OWNER)
+        group.add_user(user_2, GroupMember::OWNER)
+      end
+
+      it 'returns true' do
+        expect(group.single_owner?).to eq(false)
+      end
+    end
+
+    context 'when there are no owners' do
+      it 'returns false' do
+        expect(group.single_owner?).to eq(false)
       end
+    end
+  end
+
+  describe '#member_last_owner?' do
+    let_it_be(:user) { create(:user) }
+
+    let(:member) { group.members.last }
 
-      it { expect(group.last_blocked_owner?(blocked_user)).to be_falsy }
+    before do
+      group.add_user(user, GroupMember::OWNER)
+    end
+
+    context 'when last_owner is set' do
+      before do
+        expect(group).not_to receive(:last_owner?)
+      end
+
+      it 'returns true' do
+        member.last_owner = true
+
+        expect(group.member_last_owner?(member)).to be(true)
+      end
+
+      it 'returns false' do
+        member.last_owner = false
+
+        expect(group.member_last_owner?(member)).to be(false)
+      end
+    end
+
+    context 'when last_owner is not set' do
+      it 'returns true' do
+        expect(group).to receive(:last_owner?).and_call_original
+
+        expect(group.member_last_owner?(member)).to be(true)
+      end
     end
   end
 

spec/models/members/last_group_owner_assigner_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::LastGroupOwnerAssigner do
+  describe "#execute" do
+    let_it_be(:user, reload: true) { create(:user) }
+    let_it_be(:group) { create(:group) }
+
+    let(:group_member) { user.members.last }
+
+    subject(:assigner) { described_class.new(group, [group_member]) }
+
+    before do
+      group.add_owner(user)
+    end
+
+    it "avoids extra database queries utilizing memoization", :aggregate_failures do
+      control = ActiveRecord::QueryRecorder.new { assigner.execute }
+      count_queries = control.occurrences_by_line_method.first[1][:occurrences].find_all { |i| i.include?('SELECT COUNT') }
+
+      expect(control.count).to be <= 5
+      expect(count_queries.count).to eq(0)
+    end
+
+    context "when there are unblocked owners" do
+      context "with one unblocked owner" do
+        specify do
+          expect { assigner.execute }.to change(group_member, :last_owner)
+                                           .from(nil).to(true)
+                                           .and change(group_member, :last_blocked_owner)
+                                                  .from(nil).to(false)
+        end
+      end
+
+      context "with multiple unblocked owners" do
+        let_it_be(:unblocked_owner_member) { create(:group_member, :owner, source: group) }
+
+        specify do
+          expect { assigner.execute }.to change(group_member, :last_owner)
+                                           .from(nil).to(false)
+                                           .and change(group_member, :last_blocked_owner)
+                                                  .from(nil).to(false)
+        end
+
+        it "has many members passed" do
+          assigner = described_class.new(group, [unblocked_owner_member, group_member])
+
+          expect { assigner.execute }.to change(group_member, :last_owner)
+                                           .from(nil).to(false)
+                                           .and change(group_member, :last_blocked_owner)
+                                                  .from(nil).to(false)
+                                                  .and change(unblocked_owner_member, :last_owner)
+                                                         .from(nil).to(false)
+                                                         .and change(unblocked_owner_member, :last_blocked_owner)
+                                                                .from(nil).to(false)
+        end
+      end
+    end
+
+    context "when there are blocked owners" do
+      before do
+        user.block!
+      end
+
+      context "with one blocked owner" do
+        specify do
+          expect { assigner.execute }.to change(group_member, :last_owner)
+                                           .from(nil).to(false)
+                                           .and change(group_member, :last_blocked_owner)
+                                                  .from(nil).to(true)
+        end
+      end
+
+      context "with multiple unblocked owners" do
+        specify do
+          create_list(:group_member, 2, :owner, source: group)
+
+          expect { assigner.execute }.to change(group_member, :last_owner)
+                                           .from(nil).to(false)
+                                           .and change(group_member, :last_blocked_owner)
+                                                  .from(nil).to(false)
+        end
+      end
+
+      context "with multiple blocked owners" do
+        specify do
+          create(:group_member, :owner, :blocked, source: group)
+
+          expect { assigner.execute }.to change(group_member, :last_owner)
+                                           .from(nil).to(false)
+                                           .and change(group_member, :last_blocked_owner)
+                                                  .from(nil).to(false)
+        end
+      end
+    end
+  end
+end

spec/serializers/member_serializer_spec.rb

@@ -7,28 +7,77 @@ RSpec.describe MemberSerializer do
 
   let_it_be(:current_user) { create(:user) }
 
-  subject { described_class.new.represent(members, { current_user: current_user, group: group, source: source }) }
+  subject(:representation) do
+    described_class.new.represent(members, { current_user: current_user, group: group, source: source }).to_json
+  end
 
   shared_examples 'members.json' do
-    it 'matches json schema' do
-      expect(subject.to_json).to match_schema('members')
-    end
+    it { is_expected.to match_schema('members') }
   end
 
   context 'group member' do
-    let(:group) { create(:group) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:members) { present_members(create_list(:group_member, 1, group: group)) }
+
     let(:source) { group }
-    let(:members) { present_members(create_list(:group_member, 1, group: group)) }
 
     it_behaves_like 'members.json'
+
+    it 'handles last group owner assignment' do
+      group_member = members.last
+
+      expect { representation }.to change(group_member, :last_owner)
+                                     .from(nil).to(true)
+                                     .and change(group_member, :last_blocked_owner).from(nil).to(false)
+    end
+
+    context "with LastGroupOwnerAssigner query improvements" do
+      it "avoids N+1 database queries for last group owner assignment in MembersPresenter" do
+        group_member = create(:group_member, group: group)
+        control_count = ActiveRecord::QueryRecorder.new { member_last_owner_with_preload([group_member]) }.count
+        group_members = create_list(:group_member, 3, group: group)
+
+        expect { member_last_owner_with_preload(group_members) }.not_to exceed_query_limit(control_count)
+      end
+
+      it "avoids N+1 database queries for last blocked owner assignment in MembersPresenter" do
+        group_member = create(:group_member, group: group)
+        control_count = ActiveRecord::QueryRecorder.new { member_last_blocked_owner_with_preload([group_member]) }.count
+        group_members = create_list(:group_member, 3, group: group)
+
+        expect { member_last_blocked_owner_with_preload(group_members) }.not_to exceed_query_limit(control_count)
+      end
+
+      def member_last_owner_with_preload(members)
+        assigner_with_preload(members)
+        members.map { |m| group.member_last_owner?(m) }
+      end
+
+      def member_last_blocked_owner_with_preload(members)
+        assigner_with_preload(members)
+        members.map { |m| group.member_last_blocked_owner?(m) }
+      end
+
+      def assigner_with_preload(members)
+        MembersPreloader.new(members).preload_all
+        Members::LastGroupOwnerAssigner.new(group, members).execute
+      end
+    end
   end
 
   context 'project member' do
-    let(:project) { create(:project) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:members) { present_members(create_list(:project_member, 1, project: project)) }
+
     let(:source) { project }
     let(:group) { project.group }
-    let(:members) { present_members(create_list(:project_member, 1, project: project)) }
 
     it_behaves_like 'members.json'
+
+    it 'does not invoke group owner assignment' do
+      expect(Members::LastGroupOwnerAssigner).not_to receive(:new)
+
+      representation
+    end
   end
 end