Merge branch 'move-group-issues-search-cte-up-the-chain' into 'master'

Add a flag to use a subquery for group issues search

See merge request gitlab-org/gitlab-ce!23445

5 files changed, 168 insertions, 11 deletions

diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index 9aa98e2ca1f..a597996a362 100644
--- a/app/controllers/concerns/issuable_collections.rb
+++ b/app/controllers/concerns/issuable_collections.rb
@@ -102,7 +102,7 @@ module IssuableCollections
     elsif @group
       options[:group_id] = @group.id
       options[:include_subgroups] = true
-      options[:use_cte_for_search] = true
+      options[:attempt_group_search_optimizations] = true
     end
 
     params.permit(finder_type.valid_params).merge(options)
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index e04e3a2a7e0..b73a3fa6e01 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -27,12 +27,13 @@
 #     created_before: datetime
 #     updated_after: datetime
 #     updated_before: datetime
-#     use_cte_for_search: boolean
+#     attempt_group_search_optimizations: boolean
 #
 class IssuableFinder
   prepend FinderWithCrossProjectAccess
   include FinderMethods
   include CreatedAtFilter
+  include Gitlab::Utils::StrongMemoize
 
   requires_cross_project_access unless: -> { project? }
 
@@ -75,8 +76,9 @@ class IssuableFinder
     items = init_collection
     items = filter_items(items)
 
-    # This has to be last as we may use a CTE as an optimization fence by
-    # passing the use_cte_for_search param
+    # This has to be last as we may use a CTE as an optimization fence
+    # by passing the attempt_group_search_optimizations param and
+    # enabling the use_cte_for_group_issues_search feature flag
     # https://www.postgresql.org/docs/current/static/queries-with.html
     items = by_search(items)
 
@@ -85,6 +87,8 @@ class IssuableFinder
 
   def filter_items(items)
     items = by_project(items)
+    items = by_group(items)
+    items = by_subquery(items)
     items = by_scope(items)
     items = by_created_at(items)
     items = by_updated_at(items)
@@ -282,12 +286,31 @@ class IssuableFinder
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
+  def use_subquery_for_search?
+    strong_memoize(:use_subquery_for_search) do
+      attempt_group_search_optimizations? &&
+        Feature.enabled?(:use_subquery_for_group_issues_search, default_enabled: false)
+    end
+  end
+
+  def use_cte_for_search?
+    strong_memoize(:use_cte_for_search) do
+      attempt_group_search_optimizations? &&
+        !use_subquery_for_search? &&
+        Feature.enabled?(:use_cte_for_group_issues_search, default_enabled: true)
+    end
+  end
+
   private
 
   def init_collection
     klass.all
   end
 
+  def attempt_group_search_optimizations?
+    search && Gitlab::Database.postgresql? && params[:attempt_group_search_optimizations]
+  end
+
   def count_key(value)
     Array(value).last.to_sym
   end
@@ -351,12 +374,13 @@ class IssuableFinder
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
-  def use_cte_for_search?
-    return false unless search
-    return false unless Gitlab::Database.postgresql?
-    return false unless Feature.enabled?(:use_cte_for_group_issues_search, default_enabled: true)
-
-    params[:use_cte_for_search]
+  # Wrap projects and groups in a subquery if the conditions are met.
+  def by_subquery(items)
+    if use_subquery_for_search?
+      klass.where(id: items.select(:id)) # rubocop: disable CodeReuse/ActiveRecord
+    else
+      items
+    end
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
diff --git a/changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml b/changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml
new file mode 100644
index 00000000000..0269e7b6196
--- /dev/null
+++ b/changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml
@@ -0,0 +1,5 @@
+---
+title: Fix error when searching for group issues with priority or popularity sort
+merge_request: 23445
+author:
+type: fixed
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index f6c85102830..4b0dc4c9b69 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -226,9 +226,10 @@ describe GroupsController do
     end
 
     context 'searching' do
-      # Remove as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/52271
       before do
+        # Remove in https://gitlab.com/gitlab-org/gitlab-ce/issues/54643
         stub_feature_flags(use_cte_for_group_issues_search: false)
+        stub_feature_flags(use_subquery_for_group_issues_search: true)
       end
 
       it 'works with popularity sort' do
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index 515f6f70b99..80f7232f282 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/spec/finders/issues_finder_spec.rb
@@ -640,4 +640,131 @@ describe IssuesFinder do
       end
     end
   end
+
+  describe '#use_subquery_for_search?' do
+    let(:finder) { described_class.new(nil, params) }
+
+    before do
+      allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
+      stub_feature_flags(use_subquery_for_group_issues_search: true)
+    end
+
+    context 'when there is no search param' do
+      let(:params) { { attempt_group_search_optimizations: true } }
+
+      it 'returns false' do
+        expect(finder.use_subquery_for_search?).to be_falsey
+      end
+    end
+
+    context 'when the database is not Postgres' do
+      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+
+      before do
+        allow(Gitlab::Database).to receive(:postgresql?).and_return(false)
+      end
+
+      it 'returns false' do
+        expect(finder.use_subquery_for_search?).to be_falsey
+      end
+    end
+
+    context 'when the attempt_group_search_optimizations param is falsey' do
+      let(:params) { { search: 'foo' } }
+
+      it 'returns false' do
+        expect(finder.use_subquery_for_search?).to be_falsey
+      end
+    end
+
+    context 'when the use_subquery_for_group_issues_search flag is disabled' do
+      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+
+      before do
+        stub_feature_flags(use_subquery_for_group_issues_search: false)
+      end
+
+      it 'returns false' do
+        expect(finder.use_subquery_for_search?).to be_falsey
+      end
+    end
+
+    context 'when all conditions are met' do
+      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+
+      it 'returns true' do
+        expect(finder.use_subquery_for_search?).to be_truthy
+      end
+    end
+  end
+
+  describe '#use_cte_for_search?' do
+    let(:finder) { described_class.new(nil, params) }
+
+    before do
+      allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
+      stub_feature_flags(use_cte_for_group_issues_search: true)
+      stub_feature_flags(use_subquery_for_group_issues_search: false)
+    end
+
+    context 'when there is no search param' do
+      let(:params) { { attempt_group_search_optimizations: true } }
+
+      it 'returns false' do
+        expect(finder.use_cte_for_search?).to be_falsey
+      end
+    end
+
+    context 'when the database is not Postgres' do
+      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+
+      before do
+        allow(Gitlab::Database).to receive(:postgresql?).and_return(false)
+      end
+
+      it 'returns false' do
+        expect(finder.use_cte_for_search?).to be_falsey
+      end
+    end
+
+    context 'when the attempt_group_search_optimizations param is falsey' do
+      let(:params) { { search: 'foo' } }
+
+      it 'returns false' do
+        expect(finder.use_cte_for_search?).to be_falsey
+      end
+    end
+
+    context 'when the use_cte_for_group_issues_search flag is disabled' do
+      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+
+      before do
+        stub_feature_flags(use_cte_for_group_issues_search: false)
+      end
+
+      it 'returns false' do
+        expect(finder.use_cte_for_search?).to be_falsey
+      end
+    end
+
+    context 'when use_subquery_for_search? is true' do
+      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+
+      before do
+        stub_feature_flags(use_subquery_for_group_issues_search: true)
+      end
+
+      it 'returns false' do
+        expect(finder.use_cte_for_search?).to be_falsey
+      end
+    end
+
+    context 'when all conditions are met' do
+      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+
+      it 'returns true' do
+        expect(finder.use_cte_for_search?).to be_truthy
+      end
+    end
+  end
 end