Merge branch 'index-milestone-title-label' into 'master'

Improve performance when filtering on milestone on labels

Also some minor refactoring in the seeding.

See merge request !1587

6 files changed, 77 insertions, 48 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 0a9b51ed6c1..2eefe0137ee 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -74,6 +74,7 @@ v 8.1.0 (unreleased)
   - Only render 404 page from /public
   - Hide passwords from services API (Alex Lossent)
   - Fix: Images cannot show when projects' path was changed
+  - Optimize query when filtering on issuables (Zeger-Jan van de Weg)
   - Fix padding of outdated discussion item.
 
 v 8.0.4
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 97c7e74c294..c407dfc163a 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -53,15 +53,36 @@ class IssuableFinder
       end
   end
 
+  def project?
+    params[:project_id].present?
+  end
+
   def project
     return @project if defined?(@project)
 
-    @project =
-      if params[:project_id].present?
-        Project.find(params[:project_id])
-      else
-        nil
-      end
+    if project?
+      @project = Project.find(params[:project_id])
+      
+      unless Ability.abilities.allowed?(current_user, :read_project, @project)
+        @project = nil
+      end 
+    else
+      @project = nil
+    end
+
+    @project
+  end
+
+  def projects
+    return @projects if defined?(@projects)
+
+    if project?
+      project
+    elsif current_user && params[:authorized_only].presence && !current_user_related?
+      current_user.authorized_projects
+    else
+      ProjectsFinder.new.execute(current_user)
+    end
   end
 
   def search
@@ -72,7 +93,7 @@ class IssuableFinder
     params[:milestone_title].present?
   end
 
-  def no_milestones?
+  def filter_by_no_milestone?
     milestones? && params[:milestone_title] == Milestone::None.title
   end
 
@@ -81,12 +102,22 @@ class IssuableFinder
 
     @milestones =
       if milestones?
-        Milestone.where(title: params[:milestone_title])
+        scope = Milestone.where(project_id: projects)
+
+        scope.where(title: params[:milestone_title])
       else
         nil
       end
   end
 
+  def labels?
+    params[:label_name].present?
+  end
+
+  def filter_by_no_label?
+    labels? && params[:label_name] == Label::None.title
+  end
+
   def assignee?
     params[:assignee_id].present?
   end
@@ -120,19 +151,7 @@ class IssuableFinder
   private
 
   def init_collection
-    table_name = klass.table_name
-
-    if project
-      if Ability.abilities.allowed?(current_user, :read_project, project)
-        project.send(table_name)
-      else
-        []
-      end
-    elsif current_user && params[:authorized_only].presence && !current_user_related?
-      klass.of_projects(current_user.authorized_projects).references(:project)
-    else
-      klass.of_projects(ProjectsFinder.new.execute(current_user)).references(:project)
-    end
+    klass.all
   end
 
   def by_scope(items)
@@ -170,7 +189,12 @@ class IssuableFinder
   end
 
   def by_project(items)
-    items = items.of_projects(project.id) if project
+    items =
+      if projects
+        items.of_projects(projects).references(:project)
+      else
+        items.none
+      end
 
     items
   end
@@ -185,18 +209,6 @@ class IssuableFinder
     items.sort(params[:sort])
   end
 
-  def by_milestone(items)
-    if milestones?
-      if no_milestones?
-        items = items.where(milestone_id: [-1, nil])
-      else
-        items = items.where(milestone_id: milestones.try(:pluck, :id))
-      end
-    end
-
-    items
-  end
-
   def by_assignee(items)
     if assignee?
       items = items.where(assignee_id: assignee.try(:id))
@@ -213,20 +225,36 @@ class IssuableFinder
     items
   end
 
-  def by_label(items)
-    if params[:label_name].present?
-      if params[:label_name] == Label::None.title
-        item_ids = LabelLink.where(target_type: klass.name).pluck(:target_id)
+  def by_milestone(items)
+    if milestones?
+      if filter_by_no_milestone?
+        items = items.where(milestone_id: [-1, nil])
+      else
+        items = items.joins(:milestone).where(milestones: { title: params[:milestone_title] })
+
+        if projects
+          items = items.where(milestones: { project_id: projects })
+        end
+      end
+    end
+
+    items
+  end
 
-        items = items.where('id NOT IN (?)', item_ids)
+  def by_label(items)
+    if labels?
+      if filter_by_no_label?
+        items = items.
+          joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{klass.name}' AND label_links.target_id = #{klass.table_name}.id").
+          where(label_links: { id: nil })
       else
         label_names = params[:label_name].split(",")
 
-        item_ids = LabelLink.joins(:label).
-          where('labels.title in (?)', label_names).
-          where(target_type: klass.name).pluck(:target_id)
+        items = items.joins(:labels).where(labels: { title: label_names })
 
-        items = items.where(id: item_ids)
+        if projects
+          items = items.where(labels: { project_id: projects })
+        end
       end
     end
 
diff --git a/db/fixtures/development/05_users.rb b/db/fixtures/development/05_users.rb
index 378354efd5a..03da29c4c68 100644
--- a/db/fixtures/development/05_users.rb
+++ b/db/fixtures/development/05_users.rb
@@ -1,5 +1,5 @@
 Gitlab::Seeder.quiet do
-  (2..20).each  do |i|
+  20.times do |i|
     begin
       User.create!(
         username: FFaker::Internet.user_name,
@@ -15,7 +15,7 @@ Gitlab::Seeder.quiet do
     end
   end
 
-  (1..5).each do |i|
+  5.times do |i|
     begin
       User.create!(
         username: "user#{i}",
diff --git a/db/fixtures/development/07_milestones.rb b/db/fixtures/development/07_milestones.rb
index a43116829d9..e028ac82ba3 100644
--- a/db/fixtures/development/07_milestones.rb
+++ b/db/fixtures/development/07_milestones.rb
@@ -1,6 +1,6 @@
 Gitlab::Seeder.quiet do
   Project.all.each do |project|
-    (1..5).each  do |i|
+    5.times do |i|
       milestone_params = {
         title: "v#{i}.0",
         description: FFaker::Lorem.sentence,
diff --git a/db/fixtures/development/09_issues.rb b/db/fixtures/development/09_issues.rb
index c636e96381c..4fa572fca9b 100644
--- a/db/fixtures/development/09_issues.rb
+++ b/db/fixtures/development/09_issues.rb
@@ -1,6 +1,6 @@
 Gitlab::Seeder.quiet do
   Project.all.each do |project|
-    (1..10).each  do |i|
+    10.times do
       issue_params = {
         title: FFaker::Lorem.sentence(6),
         description: FFaker::Lorem.sentence,
diff --git a/db/fixtures/development/12_snippets.rb b/db/fixtures/development/12_snippets.rb
index 3bd4b442ade..74898544a69 100644
--- a/db/fixtures/development/12_snippets.rb
+++ b/db/fixtures/development/12_snippets.rb
@@ -22,7 +22,7 @@ class Member < ActiveRecord::Base
 end
 eos
 
-  (1..50).each  do |i|
+  50.times do |i|
     user = User.all.sample
 
     PersonalSnippet.seed(:id, [{