Merge branch '64213-not_filtering' into 'master'

Allow negating terms in IssuableFinder

See merge request gitlab-org/gitlab!16748

app/finders/issuable_finder.rb

@@ -46,10 +46,13 @@ class IssuableFinder
   # This is used in unassigning users
   NONE = '0'
 
+  NEGATABLE_PARAMS_HELPER_KEYS = %i[include_subgroups in].freeze
+
   attr_accessor :current_user, :params
 
-  def self.scalar_params
-    @scalar_params ||= %i[
+  class << self
+    def scalar_params
+      @scalar_params ||= %i[
       assignee_id
       assignee_username
       author_id
@@ -60,14 +63,30 @@ class IssuableFinder
       search
       in
     ]
-  end
+    end
 
-  def self.array_params
-    @array_params ||= { label_name: [], assignee_username: [] }
-  end
+    def array_params
+      @array_params ||= { label_name: [], assignee_username: [] }
+    end
+
+    # This should not be used in controller strong params!
+    def negatable_scalar_params
+      @negatable_scalar_params ||= scalar_params + %i[project_id group_id]
+    end
+
+    # This should not be used in controller strong params!
+    def negatable_array_params
+      @negatable_array_params ||= array_params.keys.append(:iids)
+    end
 
-  def self.valid_params
-    @valid_params ||= scalar_params + [array_params]
+    # This should not be used in controller strong params!
+    def negatable_params
+      @negatable_params ||= negatable_scalar_params + negatable_array_params
+    end
+
+    def valid_params
+      @valid_params ||= scalar_params + [array_params] + [{ not: [] }]
+    end
   end
 
   def initialize(current_user, params = {})
@@ -79,6 +98,9 @@ class IssuableFinder
     items = init_collection
     items = filter_items(items)
 
+    # Let's see if we have to negate anything
+    items = by_negation(items)
+
     # This has to be last as we use a CTE as an optimization fence
     # for counts by passing the force_cte param and enabling the
     # attempt_group_search_optimizations feature flag
@@ -366,6 +388,33 @@ class IssuableFinder
     Array(value).last.to_sym
   end
 
+  # Negates all params found in `negatable_params`
+  # rubocop: disable CodeReuse/ActiveRecord
+  def by_negation(items)
+    not_params = params[:not].dup
+    # API endpoints send in `nil` values so we test if there are any non-nil
+    return items unless not_params.present? && not_params.values.any?
+
+    not_params.keep_if { |_k, v| v.present? }.each do |(key, value)|
+      # These aren't negatable params themselves, but rather help other searches, so we skip them.
+      # They will be added into all the NOT searches.
+      next if NEGATABLE_PARAMS_HELPER_KEYS.include?(key.to_sym)
+      next unless self.class.negatable_params.include?(key.to_sym)
+
+      # These are "helper" params that are required inside the NOT to get the right results. They usually come in
+      # at the top-level params, but if they do come in inside the `:not` params, they should take precedence.
+      not_helpers = params.slice(*NEGATABLE_PARAMS_HELPER_KEYS).merge(params[:not].slice(*NEGATABLE_PARAMS_HELPER_KEYS))
+      not_param = { key => value }.with_indifferent_access.merge(not_helpers)
+
+      items_to_negate = self.class.new(current_user, not_param).execute
+
+      items = items.where.not(id: items_to_negate)
+    end
+
+    items
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+
   # rubocop: disable CodeReuse/ActiveRecord
   def by_scope(items)
     return items.none if current_user_related? && !current_user

changelogs/unreleased/64213-not_filtering.yml

+---
+title: Add not param to Issues API endpoint
+merge_request: 16748
+author:
+type: added

doc/api/issues.md

@@ -57,7 +57,8 @@ GET /issues?confidential=true
 | `created_before`    | datetime         | no         | Return issues created on or before the given time                                                                                                   |
 | `updated_after`     | datetime         | no         | Return issues updated on or after the given time                                                                                                    |
 | `updated_before`    | datetime         | no         | Return issues updated on or before the given time                                                                                                   |
-| `confidential`     | Boolean          | no         | Filter confidential or public issues.                                                                                                               |
+| `confidential`      | Boolean          | no         | Filter confidential or public issues.                                                                                                               |
+| `not`               | Hash             | no         | Return issues that do not match the parameters supplied. Accepts: `labels`, `milestone`, `author_id`, `author_username`, `assignee_id`, `assignee_username`, `my_reaction_emoji`, `search`, `in` |  
 
 ```bash
 curl --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/issues
@@ -206,6 +207,7 @@ GET /groups/:id/issues?confidential=true
 | `updated_after`     | datetime         | no         | Return issues updated on or after the given time                                                                              |
 | `updated_before`    | datetime         | no         | Return issues updated on or before the given time                                                                             |
 | `confidential`     | Boolean          | no         | Filter confidential or public issues.                                                                                         |
+| `not`               | Hash             | no         | Return issues that do not match the parameters supplied. Accepts: `labels`, `milestone`, `author_id`, `author_username`, `assignee_id`, `assignee_username`, `my_reaction_emoji`, `search`, `in` |  
 
 ```bash
 curl --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/groups/4/issues
@@ -354,6 +356,7 @@ GET /projects/:id/issues?confidential=true
 | `updated_after`     | datetime         | no         | Return issues updated on or after the given time                                                                              |
 | `updated_before`    | datetime         | no         | Return issues updated on or before the given time                                                                             |
 | `confidential`     | Boolean          | no         | Filter confidential or public issues.                                                                                         |
+| `not`               | Hash             | no         | Return issues that do not match the parameters supplied. Accepts: `labels`, `milestone`, `author_id`, `author_username`, `assignee_id`, `assignee_username`, `my_reaction_emoji`, `search`, `in` |
 
 ```bash
 curl --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/4/issues

lib/api/helpers/issues_helpers.rb

@@ -11,6 +11,9 @@ module API
       params :optional_issues_params_ee do
       end
 
+      params :optional_issue_not_params_ee do
+      end
+
       def self.update_params_at_least_one_of
         [
           :assignee_id,
@@ -35,8 +38,11 @@ module API
         args = declared_params.merge(args)
 
         args.delete(:id)
+        args[:not] ||= {}
         args[:milestone_title] ||= args.delete(:milestone)
+        args[:not][:milestone_title] ||= args[:not]&.delete(:milestone)
         args[:label_name] ||= args.delete(:labels)
+        args[:not][:label_name] ||= args[:not]&.delete(:labels)
         args[:scope] = args[:scope].underscore if args[:scope]
         args[:sort] = "#{args[:order_by]}_#{args[:sort]}"
 

lib/api/issues.rb

@@ -9,28 +9,35 @@ module API
     before { authenticate_non_get! }
 
     helpers do
-      params :issues_stats_params do
+      params :negatable_issue_filter_params do
         optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names'
         optional :milestone, type: String, desc: 'Milestone title'
-        optional :milestone, type: String, desc: 'Return issues for a specific milestone'
         optional :iids, type: Array[Integer], desc: 'The IID array of issues'
         optional :search, type: String, desc: 'Search issues for text present in the title, description, or any combination of these'
         optional :in, type: String, desc: '`title`, `description`, or a string joining them with comma'
-        optional :created_after, type: DateTime, desc: 'Return issues created after the specified time'
-        optional :created_before, type: DateTime, desc: 'Return issues created before the specified time'
-        optional :updated_after, type: DateTime, desc: 'Return issues updated after the specified time'
-        optional :updated_before, type: DateTime, desc: 'Return issues updated before the specified time'
 
         optional :author_id, type: Integer, desc: 'Return issues which are authored by the user with the given ID'
         optional :author_username, type: String, desc: 'Return issues which are authored by the user with the given username'
         mutually_exclusive :author_id, :author_username
 
         optional :assignee_id, types: [Integer, String], integer_none_any: true,
-                               desc: 'Return issues which are assigned to the user with the given ID'
+                 desc: 'Return issues which are assigned to the user with the given ID'
         optional :assignee_username, type: Array[String], check_assignees_count: true,
-                                     coerce_with: Validations::CheckAssigneesCount.coerce,
-                                     desc: 'Return issues which are assigned to the user with the given username'
+                 coerce_with: Validations::CheckAssigneesCount.coerce,
+                 desc: 'Return issues which are assigned to the user with the given username'
         mutually_exclusive :assignee_id, :assignee_username
+      end
+
+      params :issues_stats_params do
+        use :negatable_issue_filter_params
+        optional :created_after, type: DateTime, desc: 'Return issues created after the specified time'
+        optional :created_before, type: DateTime, desc: 'Return issues created before the specified time'
+        optional :updated_after, type: DateTime, desc: 'Return issues updated after the specified time'
+        optional :updated_before, type: DateTime, desc: 'Return issues updated before the specified time'
+
+        optional :not, type: Hash do
+          use :negatable_issue_filter_params
+        end
 
         optional :scope, type: String, values: %w[created-by-me assigned-to-me created_by_me assigned_to_me all],
                          desc: 'Return issues for the given scope: `created_by_me`, `assigned_to_me` or `all`'

spec/requests/api/issues/get_group_issues_spec.rb

@@ -437,17 +437,21 @@ describe API::Issues do
       end
 
       context 'with labeled issues' do
+        let(:group_issue2) { create :issue, project: group_project }
         let(:label_b) { create(:label, title: 'foo', project: group_project) }
         let(:label_c) { create(:label, title: 'bar', project: group_project) }
 
         before do
+          create(:label_link, label: group_label, target: group_issue2)
           create(:label_link, label: label_b, target: group_issue)
+          create(:label_link, label: label_b, target: group_issue2)
           create(:label_link, label: label_c, target: group_issue)
 
           get api(base_url, user), params: params
         end
 
         let(:issue) { group_issue }
+        let(:issue2) { group_issue2 }
         let(:label) { group_label }
 
         it_behaves_like 'labeled issues with labels and label_name params'

spec/requests/api/issues/get_project_issues_spec.rb

@@ -283,11 +283,14 @@ describe API::Issues do
     end
 
     context 'with labeled issues' do
+      let(:issue2) { create :issue, project: project }
       let(:label_b) { create(:label, title: 'foo', project: project) }
       let(:label_c) { create(:label, title: 'bar', project: project) }
 
       before do
+        create(:label_link, label: label, target: issue2)
         create(:label_link, label: label_b, target: issue)
+        create(:label_link, label: label_b, target: issue2)
         create(:label_link, label: label_c, target: issue)
 
         get api('/issues', user), params: params

spec/requests/api/issues/issues_spec.rb

@@ -427,9 +427,12 @@ describe API::Issues do
         context 'with labeled issues' do
           let(:label_b) { create(:label, title: 'foo', project: project) }
           let(:label_c) { create(:label, title: 'bar', project: project) }
+          let(:issue2) { create(:issue, author: user, project: project) }
 
           before do
+            create(:label_link, label: label, target: issue2)
             create(:label_link, label: label_b, target: issue)
+            create(:label_link, label: label_b, target: issue2)
             create(:label_link, label: label_c, target: issue)
 
             get api('/issues', user), params: params
@@ -497,46 +500,74 @@ describe API::Issues do
         end
       end
 
-      it 'returns an empty array if no issue matches milestone' do
-        get api("/issues?milestone=#{empty_milestone.title}", user)
+      context 'filter by milestone' do
+        it 'returns an empty array if no issue matches milestone' do
+          get api("/issues?milestone=#{empty_milestone.title}", user)
 
-        expect_paginated_array_response([])
-      end
+          expect_paginated_array_response([])
+        end
 
-      it 'returns an empty array if milestone does not exist' do
-        get api('/issues?milestone=foo', user)
+        it 'returns an empty array if milestone does not exist' do
+          get api('/issues?milestone=foo', user)
 
-        expect_paginated_array_response([])
-      end
+          expect_paginated_array_response([])
+        end
 
-      it 'returns an array of issues in given milestone' do
-        get api("/issues?milestone=#{milestone.title}", user)
+        it 'returns an array of issues in given milestone' do
+          get api("/issues?milestone=#{milestone.title}", user)
 
-        expect_paginated_array_response([issue.id, closed_issue.id])
-      end
+          expect_paginated_array_response([issue.id, closed_issue.id])
+        end
 
-      it 'returns an array of issues in given milestone_title param' do
-        get api("/issues?milestone_title=#{milestone.title}", user)
+        it 'returns an array of issues in given milestone_title param' do
+          get api("/issues?milestone_title=#{milestone.title}", user)
 
-        expect_paginated_array_response([issue.id, closed_issue.id])
-      end
+          expect_paginated_array_response([issue.id, closed_issue.id])
+        end
 
-      it 'returns an array of issues matching state in milestone' do
-        get api("/issues?milestone=#{milestone.title}&state=closed", user)
+        it 'returns an array of issues matching state in milestone' do
+          get api("/issues?milestone=#{milestone.title}&state=closed", user)
 
-        expect_paginated_array_response(closed_issue.id)
-      end
+          expect_paginated_array_response(closed_issue.id)
+        end
 
-      it 'returns an array of issues with no milestone' do
-        get api("/issues?milestone=#{no_milestone_title}", author)
+        it 'returns an array of issues with no milestone' do
+          get api("/issues?milestone=#{no_milestone_title}", author)
 
-        expect_paginated_array_response(confidential_issue.id)
-      end
+          expect_paginated_array_response(confidential_issue.id)
+        end
 
-      it 'returns an array of issues with no milestone using milestone_title param' do
-        get api("/issues?milestone_title=#{no_milestone_title}", author)
+        it 'returns an array of issues with no milestone using milestone_title param' do
+          get api("/issues?milestone_title=#{no_milestone_title}", author)
 
-        expect_paginated_array_response(confidential_issue.id)
+          expect_paginated_array_response(confidential_issue.id)
+        end
+
+        context 'negated' do
+          it 'returns all issues if milestone does not exist' do
+            get api('/issues?not[milestone]=foo', user)
+
+            expect_paginated_array_response([issue.id, closed_issue.id])
+          end
+
+          it 'returns all issues that do not belong to a milestone but have a milestone' do
+            get api("/issues?not[milestone]=#{empty_milestone.title}", user)
+
+            expect_paginated_array_response([issue.id, closed_issue.id])
+          end
+
+          it 'returns an array of issues with any milestone' do
+            get api("/issues?not[milestone]=#{no_milestone_title}", user)
+
+            expect_paginated_array_response([issue.id, closed_issue.id])
+          end
+
+          it 'returns an array of issues matching state not in milestone' do
+            get api("/issues?not[milestone]=#{empty_milestone.title}&state=closed", user)
+
+            expect_paginated_array_response(closed_issue.id)
+          end
+        end
       end
 
       it 'returns an array of issues found by iids' do

spec/support/shared_contexts/finders/issues_finder_shared_contexts.rb

@@ -12,6 +12,7 @@ RSpec.shared_context 'IssuesFinder context' do
   set(:project3) { create(:project, group: subgroup) }
   set(:milestone) { create(:milestone, project: project1) }
   set(:label) { create(:label, project: project2) }
+  set(:label2) { create(:label, project: project2) }
   set(:issue1) { create(:issue, author: user, assignees: [user], project: project1, milestone: milestone, title: 'gitlab', created_at: 1.week.ago, updated_at: 1.week.ago) }
   set(:issue2) { create(:issue, author: user, assignees: [user], project: project2, description: 'gitlab', created_at: 1.week.from_now, updated_at: 1.week.from_now) }
   set(:issue3) { create(:issue, author: user2, assignees: [user2], project: project2, title: 'tanuki', description: 'tanuki', created_at: 2.weeks.from_now, updated_at: 2.weeks.from_now) }
@@ -24,6 +25,7 @@ end
 RSpec.shared_context 'IssuesFinder#execute context' do
   let!(:closed_issue) { create(:issue, author: user2, assignees: [user2], project: project2, state: 'closed') }
   let!(:label_link) { create(:label_link, label: label, target: issue2) }
+  let!(:label_link2) { create(:label_link, label: label2, target: issue3) }
   let(:search_user) { user }
   let(:params) { {} }
   let(:issues) { described_class.new(search_user, params.reverse_merge(scope: scope, state: 'opened')).execute }

spec/support/shared_examples/finders/assignees_filter_shared_examples.rb

@@ -6,12 +6,24 @@ shared_examples 'assignee ID filter' do
   end
 end
 
+shared_examples 'assignee NOT ID filter' do
+  it 'returns issuables not assigned to that user' do
+    expect(issuables).to contain_exactly(*expected_issuables)
+  end
+end
+
 shared_examples 'assignee username filter' do
   it 'returns issuables assigned to those users' do
     expect(issuables).to contain_exactly(*expected_issuables)
   end
 end
 
+shared_examples 'assignee NOT username filter' do
+  it 'returns issuables not assigned to those users' do
+    expect(issuables).to contain_exactly(*expected_issuables)
+  end
+end
+
 shared_examples 'no assignee filter' do
   let(:params) { { assignee_id: 'None' } }
 

spec/support/shared_examples/requests/api/issues_shared_examples.rb

@@ -8,6 +8,13 @@ shared_examples 'labeled issues with labels and label_name params' do
     end
   end
 
+  shared_examples 'returns negated label names' do
+    it 'returns label names' do
+      expect_paginated_array_response(issue2.id)
+      expect(json_response.first['labels']).to eq([label_b.title, label.title])
+    end
+  end
+
   shared_examples 'returns basic label entity' do
     it 'returns basic label entity' do
       expect_paginated_array_response(issue.id)
@@ -28,6 +35,20 @@ shared_examples 'labeled issues with labels and label_name params' do
     it_behaves_like 'returns label names'
   end
 
+  context 'negation' do
+    context 'array of labeled issues when all labels match with negation' do
+      let(:params) { { labels: "#{label.title},#{label_b.title}", not: { labels: "#{label_c.title}" } } }
+
+      it_behaves_like 'returns negated label names'
+    end
+
+    context 'array of labeled issues when all labels match with negation with label params as array' do
+      let(:params) { { labels: [label.title, label_b.title], not: { labels: [label_c.title] } } }
+
+      it_behaves_like 'returns negated label names'
+    end
+  end
+
   context 'when with_labels_details provided' do
     context 'array of labeled issues when all labels match' do
       let(:params) { { labels: "#{label.title},#{label_b.title},#{label_c.title}", with_labels_details: true } }