Merge branch '259003-document-why-cached-queries-are-bad' into 'master'

Document why N+1 Cached queries are considered bad

See merge request gitlab-org/gitlab!46367

GITALY_SERVER_VERSION

-020b5f709d58277c360ba409b8f8a9e81cee2781
+a77f28ee61f3238c10769ddbd6e428debadfc933

config/sidekiq_queues.yml

@@ -144,6 +144,8 @@
   - 1
 - - group_import
   - 1
+- - group_saml_group_sync
+  - 1
 - - hashed_storage
   - 1
 - - import_issues_csv

ee/app/models/ee/group.rb

@@ -229,6 +229,11 @@ module EE
       saml_provider.persisted? && saml_provider.enabled?
     end
 
+    def saml_group_sync_available?
+      ::Feature.enabled?(:saml_group_links, self) &&
+        feature_available?(:group_saml_group_sync) && root_ancestor.saml_enabled?
+    end
+
     override :multiple_issue_boards_available?
     def multiple_issue_boards_available?
       feature_available?(:multiple_group_issue_boards)

ee/app/models/saml_group_link.rb

@@ -7,4 +7,6 @@ class SamlGroupLink < ApplicationRecord
 
   validates :group, :access_level, presence: true
   validates :saml_group_name, presence: true, uniqueness: { scope: [:group_id] }, length: { maximum: 255 }
+
+  scope :by_id_and_group_id, ->(id, group_id) { where(id: id, group_id: group_id) }
 end

ee/app/services/groups/sync_service.rb

+# frozen_string_literal: true
+
+#
+# Usage example:
+#
+# Groups::SyncService.new(nil, user, group_links: array_of_group_links).execute
+#
+# Given group links must respond to `group_id` and `access_level`.
+#
+# This is a generic group sync service, reusable by many IdP-specific
+# implementations. The worker (caller) is responsible for providing the
+# specific group links, which this service then iterates over
+# and adds users to respective groups. See `SamlGroupSyncWorker` for an
+# example.
+#
+module Groups
+  class SyncService < Groups::BaseService
+    def execute
+      group_links_by_group.each do |group_id, group_links|
+        access_level = max_access_level(group_links)
+        Group.find_by_id(group_id)&.add_user(current_user, access_level)
+      end
+    end
+
+    private
+
+    def group_links_by_group
+      params[:group_links].group_by(&:group_id)
+    end
+
+    def max_access_level(group_links)
+      human_access_level = group_links.map(&:access_level)
+      human_access_level.map { |level| integer_access_level(level) }.max
+    end
+
+    def integer_access_level(human_access_level)
+      ::Gitlab::Access.options_with_owner[human_access_level]
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -661,6 +661,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: group_saml_group_sync
+  :feature_category: :authentication_and_authorization
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: incident_management_apply_incident_sla_exceeded_label
   :feature_category: :incident_management
   :has_external_dependencies:

ee/app/workers/group_saml_group_sync_worker.rb

+# frozen_string_literal: true
+
+class GroupSamlGroupSyncWorker
+  include ApplicationWorker
+
+  feature_category :authentication_and_authorization
+  idempotent!
+
+  def perform(user_id, top_level_group_id, group_link_ids)
+    top_level_group = Group.find_by_id(top_level_group_id)
+    user = User.find_by_id(user_id)
+
+    return unless user && feature_available?(top_level_group)
+
+    group_links = find_group_links(group_link_ids, top_level_group)
+
+    Groups::SyncService.new(nil, user, group_links: group_links).execute
+  end
+
+  private
+
+  def feature_available?(group)
+    group && group.saml_group_sync_available?
+  end
+
+  def find_group_links(group_link_ids, top_level_group)
+    SamlGroupLink.by_id_and_group_id(group_link_ids, top_level_group.self_and_descendants.select(:id))
+  end
+end

ee/spec/models/group_spec.rb

@@ -792,6 +792,50 @@ RSpec.describe Group do
     end
   end
 
+  describe '#saml_group_sync_available?' do
+    subject { group.saml_group_sync_available? }
+
+    context 'when saml_group_links is not enabled' do
+      before do
+        stub_feature_flags(saml_group_links: false)
+      end
+
+      it { is_expected.to eq(false) }
+    end
+
+    context 'when saml_group_links is enabled' do
+      before do
+        stub_feature_flags(saml_group_links: true)
+      end
+
+      it { is_expected.to eq(false) }
+
+      context 'with group_saml_group_sync feature licensed' do
+        before do
+          stub_licensed_features(group_saml_group_sync: true)
+        end
+
+        it { is_expected.to eq(false) }
+
+        context 'with saml enabled' do
+          before do
+            create(:saml_provider, group: group, enabled: true)
+          end
+
+          it { is_expected.to eq(true) }
+
+          context 'when the group is a subgroup' do
+            let(:subgroup) { create(:group, :private, parent: group) }
+
+            subject { subgroup.saml_group_sync_available? }
+
+            it { is_expected.to eq(true) }
+          end
+        end
+      end
+    end
+  end
+
   describe "#insights_config" do
     context 'when group has no Insights project configured' do
       it 'returns the default config' do

ee/spec/models/saml_group_link_spec.rb

@@ -22,4 +22,32 @@ RSpec.describe SamlGroupLink do
       it { is_expected.to validate_uniqueness_of(:saml_group_name).scoped_to([:group_id]) }
     end
   end
+
+  describe '.by_id_and_group_id' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:group_link) { create(:saml_group_link, group: group) }
+
+    it 'finds the group link' do
+      results = described_class.by_id_and_group_id(group_link.id, group.id)
+
+      expect(results).to match_array([group_link])
+    end
+
+    context 'with multiple groups and group links' do
+      let_it_be(:group2) { create(:group) }
+      let_it_be(:group_link2) { create(:saml_group_link, group: group2) }
+
+      it 'finds group links within the given groups' do
+        results = described_class.by_id_and_group_id([group_link, group_link2], [group, group2])
+
+        expect(results).to match_array([group_link, group_link2])
+      end
+
+      it 'does not find group links outside the given groups' do
+        results = described_class.by_id_and_group_id([group_link, group_link2], [group])
+
+        expect(results).to match_array([group_link])
+      end
+    end
+  end
 end

ee/spec/services/groups/sync_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::SyncService, '#execute' do
+  let(:user) { create(:user) }
+
+  describe '#execute' do
+    subject(:sync) { described_class.new(nil, user, group_links: group_links).execute }
+
+    let_it_be(:top_level_group) { create(:group) }
+    let_it_be(:group1) { create(:group, parent: top_level_group) }
+
+    let_it_be(:group_links) do
+      [
+        create(:saml_group_link, group: top_level_group, access_level: 'Guest'),
+        create(:saml_group_link, group: group1, access_level: 'Reporter'),
+        create(:saml_group_link, group: group1, access_level: 'Developer')
+      ]
+    end
+
+    it 'adds two new group member records' do
+      expect { sync }.to change { GroupMember.count }.by(2)
+    end
+
+    it 'adds the user to top_level_group as Guest' do
+      sync
+
+      expect(top_level_group.members.find_by(user_id: user.id).access_level)
+        .to eq(::Gitlab::Access::GUEST)
+    end
+
+    it 'adds the user to group1 as Developer' do
+      sync
+
+      expect(group1.members.find_by(user_id: user.id).access_level)
+        .to eq(::Gitlab::Access::DEVELOPER)
+    end
+
+    context 'when the user is already a member' do
+      context 'with the correct access level' do
+        before do
+          group1.add_user(user, ::Gitlab::Access::DEVELOPER)
+        end
+
+        it 'does not change group member count' do
+          expect { sync }.not_to change { group1.members.count }
+        end
+
+        it 'retains the correct access level' do
+          sync
+
+          expect(group1.members.find_by(user_id: user.id).access_level)
+            .to eq(::Gitlab::Access::DEVELOPER)
+        end
+      end
+
+      context 'with a different access level' do
+        before do
+          top_level_group.add_user(user, ::Gitlab::Access::MAINTAINER)
+        end
+
+        it 'does not change the group member count' do
+          expect { sync }.not_to change { top_level_group.members.count }
+        end
+
+        it 'updates the access_level' do
+          sync
+
+          expect(top_level_group.members.find_by(user_id: user.id).access_level)
+            .to eq(::Gitlab::Access::GUEST)
+        end
+      end
+    end
+  end
+end

ee/spec/workers/group_saml_group_sync_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GroupSamlGroupSyncWorker do
+  describe '#perform' do
+    let_it_be(:user) { create(:user) }
+
+    let_it_be(:top_level_group) { create(:group) }
+    let_it_be(:top_level_group_link) { create(:saml_group_link, group: top_level_group) }
+
+    let_it_be(:group) { create(:group, parent: top_level_group) }
+    let_it_be(:group_link) { create(:saml_group_link, group: group) }
+
+    context 'when the group does not have group_saml_group_sync feature licensed' do
+      before do
+        create(:saml_provider, group: top_level_group, enabled: true)
+      end
+
+      it 'does not call the sync service' do
+        expect(Groups::SyncService).not_to receive(:new)
+
+        perform([top_level_group_link.id])
+      end
+    end
+
+    context 'when the group has group_saml_group_sync feature licensed' do
+      before do
+        stub_licensed_features(group_saml_group_sync: true)
+        stub_feature_flags(saml_group_links: true)
+      end
+
+      context 'when SAML is not enabled' do
+        it 'does not call the sync service' do
+          expect(Groups::SyncService).not_to receive(:new)
+
+          perform([top_level_group_link.id])
+        end
+      end
+
+      context 'when SAML is enabled' do
+        before do
+          create(:saml_provider, group: top_level_group, enabled: true)
+        end
+
+        it 'calls the sync service with the group links' do
+          stub_sync_service_expectation([top_level_group_link, group_link])
+
+          perform([top_level_group_link.id, group_link.id])
+        end
+
+        it 'does not call the sync service when the user does not exist' do
+          expect(Groups::SyncService).not_to receive(:new)
+
+          described_class.new.perform(non_existing_record_id, top_level_group.id, [group_link])
+        end
+
+        context 'when a group link falls outside the top-level group' do
+          let(:outside_group_link) { create(:saml_group_link, group: create(:group)) }
+
+          it 'drops group links outside the top level group' do
+            stub_sync_service_expectation([group_link])
+
+            perform([outside_group_link.id, group_link])
+          end
+        end
+      end
+    end
+
+    def stub_sync_service_expectation(group_links)
+      expect(Groups::SyncService).to receive(:new).with(nil, user, group_links: group_links).and_call_original
+    end
+
+    def perform(group_links)
+      described_class.new.perform(user.id, top_level_group.id, group_links)
+    end
+  end
+end