Merge branch '333965-remove-optimized-issuable-label-filter-feature-flag' into 'master'

Remove optimized_issuable_label_filter flag

See merge request gitlab-org/gitlab!70289

app/finders/issuable_finder.rb

@@ -41,7 +41,6 @@ class IssuableFinder
   include FinderMethods
   include CreatedAtFilter
   include Gitlab::Utils::StrongMemoize
-  prepend OptimizedIssuableLabelFilter
 
   requires_cross_project_access unless: -> { params.project? }
 
@@ -149,7 +148,6 @@ class IssuableFinder
 
   # Negates all params found in `negatable_params`
   def filter_negated_items(items)
-    items = by_negated_label(items)
     items = by_negated_milestone(items)
     items = by_negated_release(items)
     items = by_negated_my_reaction_emoji(items)
@@ -172,29 +170,19 @@ class IssuableFinder
     count_params = params.merge(state: nil, sort: nil, force_cte: true)
     finder = self.class.new(current_user, count_params)
 
-    counts = Hash.new(0)
+    state_counts = finder
+      .execute
+      .reorder(nil)
+      .group(:state_id)
+      .count
 
-    # Searching by label includes a GROUP BY in the query, but ours will be last
-    # because it is added last. Searching by multiple labels also includes a row
-    # per issuable, so we have to count those in Ruby - which is bad, but still
-    # better than performing multiple queries.
-    #
-    # This does not apply when we are using a CTE for the search, as the labels
-    # GROUP BY is inside the subquery in that case, so we set labels_count to 1.
-    #
-    # Groups and projects have separate feature flags to suggest the use
-    # of a CTE. The CTE will not be used if the sort doesn't support it,
-    # but will always be used for the counts here as we ignore sorting
-    # anyway.
-    labels_count = params.label_names.any? ? params.label_names.count : 1
-    labels_count = 1 if use_cte_for_search?
+    counts = Hash.new(0)
 
-    finder.execute.reorder(nil).group(:state_id).count.each do |key, value|
-      counts[count_key(key)] += value / labels_count
+    state_counts.each do |key, value|
+      counts[count_key(key)] += value
     end
 
     counts[:all] = counts.values.sum
-
     counts.with_indifferent_access
   end
   # rubocop: enable CodeReuse/ActiveRecord
@@ -360,7 +348,7 @@ class IssuableFinder
   def sort(items)
     # Ensure we always have an explicit sort order (instead of inheriting
     # multiple orders when combining ActiveRecord::Relation objects).
-    params[:sort] ? items.sort_by_attribute(params[:sort], excluded_labels: params.label_names) : items.reorder(id: :desc)
+    params[:sort] ? items.sort_by_attribute(params[:sort], excluded_labels: label_filter.label_names_excluded_from_priority_sort) : items.reorder(id: :desc)
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
@@ -384,6 +372,20 @@ class IssuableFinder
     end
   end
 
+  def by_label(items)
+    label_filter.filter(items)
+  end
+
+  def label_filter
+    strong_memoize(:label_filter) do
+      Issuables::LabelFilter.new(
+        params: original_params,
+        project: params.project,
+        group: params.group
+      )
+    end
+  end
+
   # rubocop: disable CodeReuse/ActiveRecord
   def by_milestone(items)
     return items unless params.milestones?
@@ -436,24 +438,6 @@ class IssuableFinder
     items.without_particular_release(not_params[:release_tag], not_params[:project_id])
   end
 
-  def by_label(items)
-    return items unless params.labels?
-
-    if params.filter_by_no_label?
-      items.without_label
-    elsif params.filter_by_any_label?
-      items.any_label(params[:sort])
-    else
-      items.with_label(params.label_names, params[:sort])
-    end
-  end
-
-  def by_negated_label(items)
-    return items unless not_params.labels?
-
-    items.without_particular_labels(not_params.label_names)
-  end
-
   def by_my_reaction_emoji(items)
     return items unless params[:my_reaction_emoji] && current_user
 

app/finders/issuable_finder/params.rb

@@ -29,20 +29,6 @@ class IssuableFinder
       params.present?
     end
 
-    def filter_by_no_label?
-      downcased = label_names.map(&:downcase)
-
-      downcased.include?(FILTER_NONE)
-    end
-
-    def filter_by_any_label?
-      label_names.map(&:downcase).include?(FILTER_ANY)
-    end
-
-    def labels?
-      params[:label_name].present?
-    end
-
     def milestones?
       params[:milestone_title].present? || params[:milestone_wildcard_id].present?
     end
@@ -160,24 +146,6 @@ class IssuableFinder
       end
     end
 
-    def label_names
-      if labels?
-        params[:label_name].is_a?(String) ? params[:label_name].split(',') : params[:label_name]
-      else
-        []
-      end
-    end
-
-    def labels
-      strong_memoize(:labels) do
-        if labels? && !filter_by_no_label?
-          LabelsFinder.new(current_user, project_ids: projects, title: label_names).execute(skip_authorization: true) # rubocop: disable CodeReuse/Finder
-        else
-          Label.none
-        end
-      end
-    end
-
     def milestones
       strong_memoize(:milestones) do
         if milestones?

app/finders/issuables/label_filter.rb

+# frozen_string_literal: true
+
+module Issuables
+  class LabelFilter < BaseFilter
+    include Gitlab::Utils::StrongMemoize
+    extend Gitlab::Cache::RequestCache
+
+    def initialize(project:, group:, **kwargs)
+      @project = project
+      @group = group
+
+      super(**kwargs)
+    end
+
+    def filter(issuables)
+      filtered = by_label(issuables)
+      by_negated_label(filtered)
+    end
+
+    def label_names_excluded_from_priority_sort
+      label_names_from_params
+    end
+
+    private
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def by_label(issuables)
+      return issuables unless label_names_from_params.present?
+
+      target_model = issuables.model
+
+      if filter_by_no_label?
+        issuables.where(label_link_query(target_model).arel.exists.not)
+      elsif filter_by_any_label?
+        issuables.where(label_link_query(target_model).arel.exists)
+      else
+        issuables_with_selected_labels(issuables, label_names_from_params)
+      end
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    def by_negated_label(issuables)
+      return issuables unless label_names_from_not_params.present?
+
+      issuables_without_selected_labels(issuables, label_names_from_not_params)
+    end
+
+    def filter_by_no_label?
+      label_names_from_params.map(&:downcase).include?(FILTER_NONE)
+    end
+
+    def filter_by_any_label?
+      label_names_from_params.map(&:downcase).include?(FILTER_ANY)
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def issuables_with_selected_labels(issuables, label_names)
+      target_model = issuables.model
+
+      if root_namespace
+        all_label_ids = find_label_ids(label_names)
+        # Found less labels in the DB than we were searching for. Return nothing.
+        return issuables.none if all_label_ids.size != label_names.size
+
+        all_label_ids.each do |label_ids|
+          issuables = issuables.where(label_link_query(target_model, label_ids: label_ids).arel.exists)
+        end
+      else
+        label_names.each do |label_name|
+          issuables = issuables.where(label_link_query(target_model, label_names: label_name).arel.exists)
+        end
+      end
+
+      issuables
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def issuables_without_selected_labels(issuables, label_names)
+      target_model = issuables.model
+
+      if root_namespace
+        label_ids = find_label_ids(label_names).flatten(1)
+
+        issuables.where(label_link_query(target_model, label_ids: label_ids).arel.exists.not)
+      else
+        issuables.where(label_link_query(target_model, label_names: label_names).arel.exists.not)
+      end
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def find_label_ids(label_names)
+      group_labels = Label
+        .where(project_id: nil)
+        .where(title: label_names)
+        .where(group_id: root_namespace.self_and_descendant_ids)
+
+      project_labels = Label
+        .where(group_id: nil)
+        .where(title: label_names)
+        .where(project_id: Project.select(:id).where(namespace_id: root_namespace.self_and_descendant_ids))
+
+      Label
+        .from_union([group_labels, project_labels], remove_duplicates: false)
+        .reorder(nil)
+        .pluck(:title, :id)
+        .group_by(&:first)
+        .values
+        .map { |labels| labels.map(&:last) }
+    end
+    # Avoid repeating label queries times when the finder is instantiated multiple times during the request.
+    request_cache(:find_label_ids) { root_namespace.id }
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def label_link_query(target_model, label_ids: nil, label_names: nil)
+      relation = LabelLink
+        .where(target_type: target_model.name)
+        .where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
+
+      relation = relation.where(label_id: label_ids) if label_ids
+      relation = relation.joins(:label).where(labels: { name: label_names }) if label_names
+
+      relation
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    def label_names_from_params
+      return if params[:label_name].blank?
+
+      strong_memoize(:label_names_from_params) do
+        split_label_names(params[:label_name])
+      end
+    end
+
+    def label_names_from_not_params
+      return if not_params.blank? || not_params[:label_name].blank?
+
+      strong_memoize(:label_names_from_not_params) do
+        split_label_names(not_params[:label_name])
+      end
+    end
+
+    def split_label_names(label_name_param)
+      label_name_param.is_a?(String) ? label_name_param.split(',') : label_name_param
+    end
+
+    def root_namespace
+      strong_memoize(:root_namespace) do
+        (@project || @group)&.root_ancestor
+      end
+    end
+  end
+end

app/models/concerns/issuable.rb

@@ -117,20 +117,6 @@ module Issuable
     end
     # rubocop:enable GitlabSecurity/SqlInjection
 
-    scope :without_particular_labels, ->(label_names) do
-      labels_table = Label.arel_table
-      label_links_table = LabelLink.arel_table
-      issuables_table = klass.arel_table
-      inner_query = label_links_table.project('true')
-                        .join(labels_table, Arel::Nodes::InnerJoin).on(labels_table[:id].eq(label_links_table[:label_id]))
-                        .where(label_links_table[:target_type].eq(name)
-                                   .and(label_links_table[:target_id].eq(issuables_table[:id]))
-                                   .and(labels_table[:title].in(label_names)))
-                        .exists.not
-
-      where(inner_query)
-    end
-
     scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) }
     scope :with_label_ids, ->(label_ids) { joins(:label_links).where(label_links: { label_id: label_ids }) }
     scope :join_project, -> { joins(:project) }

app/models/concerns/optimized_issuable_label_filter.rb

-# frozen_string_literal: true
-
-module OptimizedIssuableLabelFilter
-  extend ActiveSupport::Concern
-
-  prepended do
-    extend Gitlab::Cache::RequestCache
-
-    # Avoid repeating label queries times when the finder is instantiated multiple times during the request.
-    request_cache(:find_label_ids) { [root_namespace.id, params.label_names] }
-  end
-
-  def by_label(items)
-    return items unless params.labels?
-
-    return super if Feature.disabled?(:optimized_issuable_label_filter, default_enabled: :yaml)
-
-    target_model = items.model
-
-    if params.filter_by_no_label?
-      items.where('NOT EXISTS (?)', optimized_any_label_query(target_model))
-    elsif params.filter_by_any_label?
-      items.where('EXISTS (?)', optimized_any_label_query(target_model))
-    else
-      issuables_with_selected_labels(items, target_model)
-    end
-  end
-
-  # Taken from IssuableFinder
-  def count_by_state
-    return super if Feature.disabled?(:optimized_issuable_label_filter, default_enabled: :yaml)
-
-    count_params = params.merge(state: nil, sort: nil, force_cte: true)
-    finder = self.class.new(current_user, count_params)
-
-    state_counts = finder
-      .execute
-      .reorder(nil)
-      .group(:state_id)
-      .count
-
-    counts = Hash.new(0)
-
-    state_counts.each do |key, value|
-      counts[count_key(key)] += value
-    end
-
-    counts[:all] = counts.values.sum
-    counts.with_indifferent_access
-  end
-
-  private
-
-  def issuables_with_selected_labels(items, target_model)
-    if root_namespace
-      all_label_ids = find_label_ids
-      # Found less labels in the DB than we were searching for. Return nothing.
-      return items.none if all_label_ids.size != params.label_names.size
-
-      all_label_ids.each do |label_ids|
-        items = items.where('EXISTS (?)', optimized_label_query_by_label_ids(target_model, label_ids))
-      end
-    else
-      params.label_names.each do |label_name|
-        items = items.where('EXISTS (?)', optimized_label_query_by_label_name(target_model, label_name))
-      end
-    end
-
-    items
-  end
-
-  def find_label_ids
-    group_labels = Label
-      .where(project_id: nil)
-      .where(title: params.label_names)
-      .where(group_id: root_namespace.self_and_descendants.select(:id))
-
-    project_labels = Label
-      .where(group_id: nil)
-      .where(title: params.label_names)
-      .where(project_id: Project.select(:id).where(namespace_id: root_namespace.self_and_descendants.select(:id)))
-
-    Label
-      .from_union([group_labels, project_labels], remove_duplicates: false)
-      .reorder(nil)
-      .pluck(:title, :id)
-      .group_by(&:first)
-      .values
-      .map { |labels| labels.map(&:last) }
-  end
-
-  def root_namespace
-    strong_memoize(:root_namespace) do
-      (params.project || params.group)&.root_ancestor
-    end
-  end
-
-  def optimized_any_label_query(target_model)
-    LabelLink
-      .where(target_type: target_model.name)
-      .where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
-      .limit(1)
-  end
-
-  def optimized_label_query_by_label_ids(target_model, label_ids)
-    LabelLink
-      .where(target_type: target_model.name)
-      .where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
-      .where(label_id: label_ids)
-      .limit(1)
-  end
-
-  def optimized_label_query_by_label_name(target_model, label_name)
-    LabelLink
-      .joins(:label)
-      .where(target_type: target_model.name)
-      .where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
-      .where(labels: { name: label_name })
-      .limit(1)
-  end
-end

config/feature_flags/development/optimized_issuable_label_filter.yml

----
-name: optimized_issuable_label_filter
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34503
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/259719
-milestone: '13.4'
-type: development
-group: group::optimize
-default_enabled: true

ee/app/finders/epics_finder.rb

@@ -65,7 +65,7 @@ class EpicsFinder < IssuableFinder
     @skip_visibility_check = skip_visibility_check
 
     raise ArgumentError, 'group_id argument is missing' unless params[:group_id]
-    return Epic.none unless Ability.allowed?(current_user, :read_epic, group)
+    return Epic.none unless Ability.allowed?(current_user, :read_epic, params.group)
 
     items = init_collection
     items = filter_items(items)
@@ -83,8 +83,8 @@ class EpicsFinder < IssuableFinder
     groups = if params[:iids].present?
                # If we are querying for specific iids, then we should only be looking at
                # those in the group, not any sub-groups (which can have identical iids).
-               # The `group` method takes care of checking permissions
-               [group]
+               # The `params.group` method takes care of checking permissions
+               [params.group]
              else
                permissioned_related_groups
              end
@@ -141,21 +141,7 @@ class EpicsFinder < IssuableFinder
     # API endpoints send in `nil` values so we test if there are any non-nil
     return items unless not_params&.values&.any?
 
-    items = by_negated_my_reaction_emoji(items)
-
-    by_negated_label(items)
-  end
-
-  def group
-    strong_memoize(:group) do
-      next unless params[:group_id]
-
-      if params[:group_id].is_a?(Group)
-        params[:group_id]
-      else
-        Group.find(params[:group_id])
-      end
-    end
+    by_negated_my_reaction_emoji(items)
   end
 
   def starts_with_iid(items)
@@ -172,13 +158,13 @@ class EpicsFinder < IssuableFinder
     include_descendants = params.fetch(:include_descendant_groups, true)
 
     if include_ancestors && include_descendants
-      group.self_and_hierarchy
+      params.group.self_and_hierarchy
     elsif include_ancestors
-      group.self_and_ancestors
+      params.group.self_and_ancestors
     elsif include_descendants
-      group.self_and_descendants
+      params.group.self_and_descendants
     else
-      Group.id_in(group.id)
+      Group.id_in(params.group.id)
     end
   end
 
@@ -251,7 +237,7 @@ class EpicsFinder < IssuableFinder
     # `groups` is a list of groups in the same group hierarchy, group is
     # highest in the group hierarchy except if we fetch ancestors - in that
     # case top-level group is group's root parent
-    parent = params.fetch(:include_ancestor_groups, false) ? group.root_ancestor : group
+    parent = params.fetch(:include_ancestor_groups, false) ? params.group.root_ancestor : params.group
 
     # If they can view confidential epics in this parent group they can
     # definitely view confidential epics in subgroups.
@@ -290,6 +276,6 @@ class EpicsFinder < IssuableFinder
 
   override :feature_flag_scope
   def feature_flag_scope
-    group
+    params.group
   end
 end

ee/spec/graphql/resolvers/epics_resolver_spec.rb

@@ -325,7 +325,7 @@ RSpec.describe Resolvers::EpicsResolver do
     context 'with negated filters' do
       let_it_be(:group) { create(:group) }
       let_it_be(:author) { create(:user) }
-      let_it_be(:label) { create(:label) }
+      let_it_be(:label) { create(:group_label, group: group) }
       let_it_be(:epic_1) { create(:labeled_epic, group: group, labels: [label]) }
       let_it_be(:epic_2) { create(:epic, group: group, author: author) }
       let_it_be(:epic_3) { create(:epic, group: group) }

spec/finders/issues_finder_spec.rb

@@ -426,7 +426,6 @@ RSpec.describe IssuesFinder do
         end
       end
 
-      shared_examples ':label_name parameter' do
       context 'filtering by label' do
         let(:params) { { label_name: label.title } }
 
@@ -543,23 +542,6 @@ RSpec.describe IssuesFinder do
           expect(issues).to contain_exactly(issue1, issue2)
         end
       end
-      end
-
-      context 'when `optimized_issuable_label_filter` feature flag is off' do
-        before do
-          stub_feature_flags(optimized_issuable_label_filter: false)
-        end
-
-        it_behaves_like ':label_name parameter'
-      end
-
-      context 'when `optimized_issuable_label_filter` feature flag is on' do
-        before do
-          stub_feature_flags(optimized_issuable_label_filter: true)
-        end
-
-        it_behaves_like ':label_name parameter'
-      end
 
       context 'filtering by issue term' do
         let(:params) { { search: 'git' } }

spec/finders/merge_requests_finder_spec.rb

@@ -227,7 +227,6 @@ RSpec.describe MergeRequestsFinder do
         end
       end
 
-      shared_examples ':label_name parameter' do
       describe ':label_name parameter' do
         let(:common_labels) { create_list(:label, 3) }
         let(:distinct_labels) { create_list(:label, 3) }
@@ -261,23 +260,6 @@ RSpec.describe MergeRequestsFinder do
           expect(all_common).to match_array(merge_requests)
         end
       end
-      end
-
-      context 'when `optimized_issuable_label_filter` feature flag is off' do
-        before do
-          stub_feature_flags(optimized_issuable_label_filter: false)
-        end
-
-        it_behaves_like ':label_name parameter'
-      end
-
-      context 'when `optimized_issuable_label_filter` feature flag is on' do
-        before do
-          stub_feature_flags(optimized_issuable_label_filter: true)
-        end
-
-        it_behaves_like ':label_name parameter'
-      end
 
       it 'filters by source project id' do
         params = { source_project_id: merge_request2.source_project_id }

spec/graphql/resolvers/merge_requests_resolver_spec.rb

@@ -294,16 +294,6 @@ RSpec.describe Resolvers::MergeRequestsResolver do
           nils_last(mr.metrics.merged_at)
         end
 
-        context 'when label filter is given and the optimized_issuable_label_filter feature flag is off' do
-          before do
-            stub_feature_flags(optimized_issuable_label_filter: false)
-          end
-
-          it 'does not raise PG::GroupingError' do
-            expect { resolve_mr(project, sort: :merged_at_desc, labels: %w[a b]) }.not_to raise_error
-          end
-        end
-
         context 'when sorting by closed at' do
           before do
             merge_request_1.metrics.update!(latest_closed_at: 10.days.ago)

spec/requests/api/issues/get_group_issues_spec.rb

@@ -402,14 +402,7 @@ RSpec.describe API::Issues do
         expect_paginated_array_response([group_closed_issue.id, group_issue.id])
       end
 
-      shared_examples 'labels parameter' do
-        it 'returns an array of labeled group issues' do
-          get api(base_url, user), params: { labels: group_label.title }
-
-          expect_paginated_array_response(group_issue.id)
-          expect(json_response.first['labels']).to eq([group_label.title])
-        end
-
+      context 'labels parameter' do
         it 'returns an array of labeled group issues' do
           get api(base_url, user), params: { labels: group_label.title }
 
@@ -458,22 +451,6 @@ RSpec.describe API::Issues do
         end
       end
 
-      context 'when `optimized_issuable_label_filter` feature flag is off' do
-        before do
-          stub_feature_flags(optimized_issuable_label_filter: false)
-        end
-
-        it_behaves_like 'labels parameter'
-      end
-
-      context 'when `optimized_issuable_label_filter` feature flag is on' do
-        before do
-          stub_feature_flags(optimized_issuable_label_filter: true)
-        end
-
-        it_behaves_like 'labels parameter'
-      end
-
       it 'returns issues matching given search string for title' do
         get api(base_url, user), params: { search: group_issue.title }