Add a flag to use a subquery for group issues search

We already had a flag to use a CTE, but this broke searching in some
cases where we need to sort by a joined table. Disabling the CTE flag
makes searches much slower.

The new flag, to use a subquery, makes them slightly slower than the
CTE, while maintaining correctness. If both it and the CTE flag are
enabled, the subquery takes precedence.

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)

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,36 @@ class IssuableFinder
   end
   # rubocop: enable CodeReuse/ActiveRecord
+  def use_subquery_for_search?
+    strong_memoize(:use_subquery_for_search) do
+      if attempt_group_search_optimizations?
+        Feature.enabled?(:use_subquery_for_group_issues_search, default_enabled: false)
+      else
+        false
+      end
+    end
+  end
+
+  def use_cte_for_search?
+    strong_memoize(:use_cte_for_search) do
+      if attempt_group_search_optimizations? && !use_subquery_for_search?
+        Feature.enabled?(:use_cte_for_group_issues_search, default_enabled: true)
+      else
+        false
+      end
+    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 +379,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

changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml

+---
+title: Fix error when searching for group issues with priority or popularity sort
+merge_request: 23445
+author:
+type: fixed

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

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