Merge branch '351703-performance-issues-with-releasescontroller-and-subgroups-2' into 'master'

Resolve ReleasesFinder IN Query Performance Issues

See merge request gitlab-org/gitlab!80093

app/controllers/groups/releases_controller.rb

@@ -15,11 +15,17 @@ module Groups
     private
 
     def releases
-      ReleasesFinder
-        .new(@group, current_user, { include_subgroups: true })
-        .execute(preload: false)
-        .page(params[:page])
-        .per(30)
+      if Feature.enabled?(:group_releases_finder_inoperator)
+        Releases::GroupReleasesFinder
+          .new(@group, current_user, { include_subgroups: true, page: params[:page], per: 30 })
+          .execute(preload: false)
+      else
+        ReleasesFinder
+          .new(@group, current_user, { include_subgroups: true })
+          .execute(preload: false)
+          .page(params[:page])
+          .per(30)
+      end
     end
   end
 end

app/finders/releases/group_releases_finder.rb

+# frozen_string_literal: true
+module Releases
+  ##
+  # The GroupReleasesFinder does not support all the options of ReleasesFinder
+  # due to use of InOperatorOptimization for finding subprojects/subgroups
+  #
+  # order_by - only ordering by released_at is supported
+  # filter by tag - currently not supported
+  class GroupReleasesFinder
+    include Gitlab::Utils::StrongMemoize
+
+    attr_reader :parent, :current_user, :params
+
+    def initialize(parent, current_user = nil, params = {})
+      @parent = parent
+      @current_user = current_user
+      @params = params
+
+      params[:order_by] ||= 'released_at'
+      params[:sort] ||= 'desc'
+      params[:page] ||= 0
+      params[:per] ||= 30
+    end
+
+    def execute(preload: true)
+      return Release.none unless Ability.allowed?(current_user, :read_release, parent)
+
+      releases = get_releases(preload: preload)
+
+      paginate_releases(releases)
+    end
+
+    private
+
+    def include_subgroups?
+      params.fetch(:include_subgroups, false)
+    end
+
+    def accessible_projects_scope
+      if include_subgroups?
+        Project.for_group_and_its_subgroups(parent)
+      else
+        parent.projects
+      end
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def get_releases(preload: true)
+      Gitlab::Pagination::Keyset::InOperatorOptimization::QueryBuilder.new(
+        scope: releases_scope(preload: preload),
+        array_scope: accessible_projects_scope.select(:id),
+        array_mapping_scope: -> (project_id_expression) { Release.where(Release.arel_table[:project_id].eq(project_id_expression)) },
+        finder_query: -> (order_by, id_expression) { Release.where(Release.arel_table[:id].eq(id_expression)) }
+      )
+      .execute
+    end
+
+    def releases_scope(preload: true)
+      scope = Release.all
+      scope = order_releases(scope)
+      scope = scope.preloaded if preload
+      scope
+    end
+
+    def order_releases(scope)
+      scope.sort_by_attribute("released_at_#{params[:sort]}").order(id: params[:sort])
+    end
+
+    def paginate_releases(releases)
+      releases.page(params[:page].to_i).per(params[:per])
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+  end
+end

app/models/release.rb

@@ -6,6 +6,7 @@ class Release < ApplicationRecord
   include Importable
   include Gitlab::Utils::StrongMemoize
   include EachBatch
+  include FromUnion
 
   cache_markdown_field :description
 

config/feature_flags/development/group_releases_finder_inoperator.yml

+---
+name: group_releases_finder_inoperator
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80093
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/355463
+milestone: '14.9'
+type: development
+group: group::release
+default_enabled: false

spec/controllers/groups/releases_controller_spec.rb

@@ -20,11 +20,11 @@ RSpec.describe Groups::ReleasesController do
     context 'as json' do
       let(:format) { :json }
 
-      subject { get :index, params: { group_id: group }, format: format }
+      subject(:index) { get :index, params: { group_id: group }, format: format }
 
       context 'json_response' do
         before do
-          subject
+          index
         end
 
         it 'returns an application/json content_type' do
@@ -38,7 +38,7 @@ RSpec.describe Groups::ReleasesController do
 
       context 'the user is not authorized' do
         before do
-          subject
+          index
         end
 
         it 'does not return any releases' do
@@ -54,12 +54,38 @@ RSpec.describe Groups::ReleasesController do
         it "returns all group's public and private project's releases as JSON, ordered by released_at" do
           sign_in(guest)
 
-          subject
+          index
 
           expect(json_response.map {|r| r['tag'] } ).to match_array(%w(p2 p1 v2 v1))
         end
       end
 
+      context 'group_releases_finder_inoperator feature flag' do
+        before do
+          sign_in(guest)
+        end
+
+        it 'calls old code when disabled' do
+          stub_feature_flags(group_releases_finder_inoperator: false)
+
+          allow(ReleasesFinder).to receive(:new).and_call_original
+
+          index
+
+          expect(ReleasesFinder).to have_received(:new)
+        end
+
+        it 'calls new code when enabled' do
+          stub_feature_flags(group_releases_finder_inoperator: true)
+
+          allow(Releases::GroupReleasesFinder).to receive(:new).and_call_original
+
+          index
+
+          expect(Releases::GroupReleasesFinder).to have_received(:new)
+        end
+      end
+
       context 'N+1 queries' do
         it 'avoids N+1 database queries' do
           control_count = ActiveRecord::QueryRecorder.new { subject }.count

spec/finders/releases/group_releases_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Releases::GroupReleasesFinder do
+  let(:user)  { create(:user) }
+  let(:group) { create(:group) }
+  let(:project) { create(:project, :repository, group: group) }
+  let(:params) { {} }
+  let(:args) { {} }
+  let(:repository) { project.repository }
+  let(:v1_0_0) { create(:release, project: project, tag: 'v1.0.0') }
+  let(:v1_1_0) { create(:release, project: project, tag: 'v1.1.0') }
+  let(:v1_1_1) { create(:release, project: project, tag: 'v1.1.1') }
+
+  before do
+    v1_0_0.update_attribute(:released_at, 2.days.ago)
+    v1_1_0.update_attribute(:released_at, 1.day.ago)
+    v1_1_1.update_attribute(:released_at, 0.5.days.ago)
+  end
+
+  shared_examples_for 'when the user is not part of the project' do
+    it 'returns no releases' do
+      is_expected.to be_empty
+    end
+  end
+
+  shared_examples_for 'when the user is not part of the group' do
+    before do
+      allow(Ability).to receive(:allowed?).with(user, :read_release, group).and_return(false)
+    end
+
+    it 'returns no releases' do
+      is_expected.to be_empty
+    end
+  end
+
+  shared_examples_for 'preload' do
+    before do
+      allow(Ability).to receive(:allowed?).with(user, :read_release, group).and_return(true)
+    end
+
+    it 'preloads associations' do
+      expect(Release).to receive(:preloaded).once.and_call_original
+
+      releases
+    end
+
+    context 'when preload is false' do
+      let(:args) { { preload: false } }
+
+      it 'does not preload associations' do
+        expect(Release).not_to receive(:preloaded)
+
+        releases
+      end
+    end
+  end
+
+  describe 'when parent is a group' do
+    context 'without subgroups' do
+      let(:project2) { create(:project, :repository, namespace: group) }
+      let!(:v6) { create(:release, project: project2, tag: 'v6') }
+
+      subject(:releases) { described_class.new(group, user, params).execute(**args) }
+
+      it_behaves_like 'preload'
+      it_behaves_like 'when the user is not part of the group'
+
+      context 'when the user is a project guest on one sibling project' do
+        before do
+          project.add_guest(user)
+        end
+
+        it 'does not return any releases' do
+          expect(releases.size).to eq(0)
+          expect(releases).to eq([])
+        end
+      end
+
+      context 'when the user is a guest on the group' do
+        before do
+          group.add_guest(user)
+          v1_0_0.update_attribute(:released_at, 3.days.ago)
+          v6.update_attribute(:released_at, 2.days.ago)
+          v1_1_0.update_attribute(:released_at, 1.day.ago)
+          v1_1_1.update_attribute(:released_at, v1_1_0.released_at)
+        end
+
+        it 'sorts by release date and id' do
+          expect(releases.size).to eq(4)
+          expect(releases).to eq([v1_1_1, v1_1_0, v6, v1_0_0])
+        end
+      end
+    end
+
+    describe 'with subgroups' do
+      let(:params) { { include_subgroups: true } }
+
+      subject(:releases) { described_class.new(group, user, params).execute(**args) }
+
+      context 'with a single-level subgroup' do
+        let(:subgroup) { create(:group, parent: group) }
+        let(:project2) { create(:project, :repository, namespace: subgroup) }
+        let!(:v6)      { create(:release, project: project2, tag: 'v6') }
+
+        it_behaves_like 'when the user is not part of the group'
+
+        context 'when the user a project guest in the subgroup project' do
+          before do
+            project2.add_guest(user)
+          end
+
+          it 'does not return any releases' do
+            expect(releases).to match_array([])
+          end
+        end
+
+        context 'when the user is a guest on the group' do
+          before do
+            group.add_guest(user)
+            v6.update_attribute(:released_at, 2.days.ago)
+          end
+
+          it 'returns all releases' do
+            expect(releases).to match_array([v1_1_1, v1_1_0, v1_0_0, v6])
+          end
+        end
+      end
+
+      context 'with a multi-level subgroup' do
+        let(:subgroup) { create(:group, parent: group) }
+        let(:subsubgroup) { create(:group, parent: subgroup) }
+        let(:project2) { create(:project, :repository, namespace: subgroup) }
+        let(:project3) { create(:project, :repository, namespace: subsubgroup) }
+        let!(:v6) { create(:release, project: project2, tag: 'v6') }
+        let!(:p3) { create(:release, project: project3, tag: 'p3') }
+
+        before do
+          v6.update_attribute(:released_at, 2.days.ago)
+          p3.update_attribute(:released_at, 3.days.ago)
+        end
+
+        it_behaves_like 'when the user is not part of the group'
+
+        context 'when the user a project guest in the subgroup and subsubgroup project' do
+          before do
+            project2.add_guest(user)
+            project3.add_guest(user)
+          end
+
+          it 'does not return any releases' do
+            expect(releases).to match_array([])
+          end
+        end
+
+        context 'when the user a project guest in the subsubgroup project' do
+          before do
+            project3.add_guest(user)
+          end
+
+          it 'does not return any releases' do
+            expect(releases).to match_array([])
+          end
+        end
+
+        context 'when the user a guest on the group' do
+          before do
+            group.add_guest(user)
+          end
+
+          it 'returns all releases' do
+            expect(releases).to match_array([v1_1_1, v1_1_0, v6, v1_0_0, p3])
+          end
+        end
+
+        context 'performance testing' do
+          shared_examples 'avoids N+1 queries' do |query_params = {}|
+            context 'with subgroups' do
+              let(:params) { query_params }
+
+              it 'include_subgroups avoids N+1 queries' do
+                control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+                  releases
+                end.count
+
+                subgroups = create_list(:group, 10, parent: group)
+                projects = create_list(:project, 10, namespace: subgroups[0])
+                create_list(:release, 10, project: projects[0], author: user)
+
+                expect do
+                  releases
+                end.not_to exceed_all_query_limit(control_count)
+              end
+            end
+          end
+
+          it_behaves_like 'avoids N+1 queries'
+          it_behaves_like 'avoids N+1 queries', { simple: true }
+        end
+      end
+    end
+  end
+end