Merge branch 'separate-banzai-references' into 'master'

Separate reference gathering from rendering

This is a required step to allow batch processing when gathering references. This in turn would allow grabbing (for example) all mentioned users of an issue/merge request using a single query.

cc @rspeicher @DouweM 

See merge request !3969

10 files changed, 129 insertions, 74 deletions

diff --git a/app/models/ability.rb b/app/models/ability.rb
index 8c5b255223d..44515550d9e 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -23,6 +23,28 @@ class Ability
       end.concat(global_abilities(user))
     end
 
+    # Given a list of users and a project this method returns the users that can
+    # read the given project.
+    def users_that_can_read_project(users, project)
+      if project.public?
+        users
+      else
+        users.select do |user|
+          if user.admin?
+            true
+          elsif project.internal? && !user.external?
+            true
+          elsif project.owner == user
+            true
+          elsif project.team.members.include?(user)
+            true
+          else
+            false
+          end
+        end
+      end
+    end
+
     # List of possible abilities for anonymous user
     def anonymous_abilities(user, subject)
       if subject.is_a?(PersonalSnippet)
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 562c3ed15b2..f96c7cb34d0 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -8,7 +8,10 @@ class Commit
   include StaticModel
 
   attr_mentionable :safe_message, pipeline: :single_line
-  participant :author, :committer, :notes
+
+  participant :author
+  participant :committer
+  participant :notes_with_associations
 
   attr_accessor :project
 
@@ -194,6 +197,10 @@ class Commit
     project.notes.for_commit_id(self.id)
   end
 
+  def notes_with_associations
+    notes.includes(:author, :project)
+  end
+
   def method_missing(m, *args, &block)
     @raw.send(m, *args, &block)
   end
@@ -219,7 +226,7 @@ class Commit
   def revert_branch_name
     "revert-#{short_id}"
   end
-  
+
   def cherry_pick_branch_name
     project.repository.next_branch("cherry-pick-#{short_id}", mild: true)
   end
@@ -251,11 +258,13 @@ class Commit
   end
 
   def has_been_reverted?(current_user = nil, noteable = self)
-    Gitlab::ReferenceExtractor.lazily do
-      noteable.notes.system.flat_map do |note|
-        note.all_references(current_user).commits
-      end
-    end.any? { |commit_ref| commit_ref.reverts_commit?(self) }
+    ext = all_references(current_user)
+
+    noteable.notes_with_associations.system.each do |note|
+      note.all_references(current_user, extractor: ext)
+    end
+
+    ext.commits.any? { |commit_ref| commit_ref.reverts_commit?(self) }
   end
 
   def change_type_title
diff --git a/app/models/commit_range.rb b/app/models/commit_range.rb
index 51673897d98..4066958f67c 100644
--- a/app/models/commit_range.rb
+++ b/app/models/commit_range.rb
@@ -62,7 +62,7 @@ class CommitRange
   def initialize(range_string, project)
     @project = project
 
-    range_string.strip!
+    range_string = range_string.strip
 
     unless range_string =~ /\A#{PATTERN}\z/
       raise ArgumentError, "invalid CommitRange string format: #{range_string}"
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 9aaaf787372..2326a395cb8 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -59,8 +59,12 @@ module Issuable
              prefix: true
 
     attr_mentionable :title, pipeline: :single_line
-    attr_mentionable :description, cache: true
-    participant :author, :assignee, :notes_with_associations
+    attr_mentionable :description
+
+    participant :author
+    participant :assignee
+    participant :notes_with_associations
+
     strip_attributes :title
 
     acts_as_paranoid
diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index b381d225485..f00b5b8497c 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -23,7 +23,7 @@ module Mentionable
 
   included do
     if self < Participable
-      participant ->(current_user) { mentioned_users(current_user) }
+      participant -> (user, ext) { all_references(user, extractor: ext) }
     end
   end
 
@@ -43,23 +43,22 @@ module Mentionable
     self
   end
 
-  def all_references(current_user = nil, text = nil)
-    ext = Gitlab::ReferenceExtractor.new(self.project, current_user || self.author, self.author)
+  def all_references(current_user = nil, text = nil, extractor: nil)
+    extractor ||= Gitlab::ReferenceExtractor.
+      new(project, current_user || author)
 
     if text
-      ext.analyze(text)
+      extractor.analyze(text, author: author)
     else
       self.class.mentionable_attrs.each do |attr, options|
-        text = send(attr)
+        text = __send__(attr)
+        options = options.merge(cache_key: [self, attr], author: author)
 
-        context = options.dup
-        context[:cache_key] = [self, attr] if context.delete(:cache) && self.persisted?
-
-        ext.analyze(text, context)
+        extractor.analyze(text, options)
       end
     end
 
-    ext
+    extractor
   end
 
   def mentioned_users(current_user = nil)
diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb
index fc6f83b918b..9056722f45e 100644
--- a/app/models/concerns/participable.rb
+++ b/app/models/concerns/participable.rb
@@ -3,8 +3,6 @@
 # Contains functionality related to objects that can have participants, such as
 # an author, an assignee and people mentioned in its description or comments.
 #
-# Used by Issue, Note, MergeRequest, Snippet and Commit.
-#
 # Usage:
 #
 #     class Issue < ActiveRecord::Base
@@ -12,22 +10,36 @@
 #
 #       # ...
 #
-#       participant :author, :assignee, :notes, ->(current_user) { mentioned_users(current_user) }
+#       participant :author
+#       participant :assignee
+#       participant :notes
+#
+#       participant -> (current_user, ext) do
+#         ext.analyze('...')
+#       end
 #     end
 #
 #     issue = Issue.last
 #     users = issue.participants
-#     # `users` will contain the issue's author, its assignee,
-#     # all users returned by its #mentioned_users method,
-#     # as well as all participants to all of the issue's notes,
-#     # since Note implements Participable as well.
-#
 module Participable
   extend ActiveSupport::Concern
 
   module ClassMethods
-    def participant(*attrs)
-      participant_attrs.concat(attrs)
+    # Adds a list of participant attributes. Attributes can either be symbols or
+    # Procs.
+    #
+    # When using a Proc instead of a Symbol the Proc will be given two
+    # arguments:
+    #
+    # 1. The current user (as an instance of User)
+    # 2. An instance of `Gitlab::ReferenceExtractor`
+    #
+    # It is expected that a Proc populates the given reference extractor
+    # instance with data. The return value of the Proc is ignored.
+    #
+    # attr - The name of the attribute or a Proc
+    def participant(attr)
+      participant_attrs << attr
     end
 
     def participant_attrs
@@ -35,42 +47,42 @@ module Participable
     end
   end
 
-  # Be aware that this method makes a lot of sql queries.
-  # Save result into variable if you are going to reuse it inside same request
-  def participants(current_user = self.author)
-    participants =
-      Gitlab::ReferenceExtractor.lazily do
-        self.class.participant_attrs.flat_map do |attr|
-          value =
-            if attr.respond_to?(:call)
-              instance_exec(current_user, &attr)
-            else
-              send(attr)
-            end
+  # Returns the users participating in a discussion.
+  #
+  # This method processes attributes of objects in breadth-first order.
+  #
+  # Returns an Array of User instances.
+  def participants(current_user = nil)
+    current_user ||= author
+    ext = Gitlab::ReferenceExtractor.new(project, current_user)
+    participants = Set.new
+    process = [self]
 
-          participants_for(value, current_user)
-        end.compact.uniq
-      end
+    until process.empty?
+      source = process.pop
 
-    unless Gitlab::ReferenceExtractor.lazy?
-      participants.select! do |user|
-        user.can?(:read_project, project)
+      case source
+      when User
+        participants << source
+      when Participable
+        source.class.participant_attrs.each do |attr|
+          if attr.respond_to?(:call)
+            source.instance_exec(current_user, ext, &attr)
+          else
+            process << source.__send__(attr)
+          end
+        end
+      when Enumerable, ActiveRecord::Relation
+        # This uses reverse_each so we can use "pop" to get the next value to
+        # process (in order). Using unshift instead of pop would require
+        # moving all Array values one index to the left (which can be
+        # expensive).
+        source.reverse_each { |obj| process << obj }
       end
     end
 
-    participants
-  end
-
-  private
+    participants.merge(ext.users)
 
-  def participants_for(value, current_user = nil)
-    case value
-    when User, Banzai::LazyReference
-      [value]
-    when Enumerable, ActiveRecord::Relation
-      value.flat_map { |v| participants_for(v, current_user) }
-    when Participable
-      value.participants(current_user)
-    end
+    Ability.users_that_can_read_project(participants.to_a, project)
   end
 end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 2d4a9b9f19a..bd0fbc96d18 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -95,14 +95,13 @@ class Issue < ActiveRecord::Base
   end
 
   def referenced_merge_requests(current_user = nil)
-    @referenced_merge_requests ||= {}
-    @referenced_merge_requests[current_user] ||= begin
-      Gitlab::ReferenceExtractor.lazily do
-        [self, *notes].flat_map do |note|
-          note.all_references(current_user).merge_requests
-        end
-      end.sort_by(&:iid).uniq
+    ext = all_references(current_user)
+
+    notes_with_associations.each do |object|
+      object.all_references(current_user, extractor: ext)
     end
+
+    ext.merge_requests.sort_by(&:iid)
   end
 
   # All branches containing the current issue's ID, except for
@@ -139,9 +138,13 @@ class Issue < ActiveRecord::Base
   def closed_by_merge_requests(current_user = nil)
     return [] unless open?
 
-    notes.system.flat_map do |note|
-      note.all_references(current_user).merge_requests
-    end.uniq.select { |mr| mr.open? && mr.closes_issue?(self) }
+    ext = all_references(current_user)
+
+    notes.system.each do |note|
+      note.all_references(current_user, extractor: ext)
+    end
+
+    ext.merge_requests.select { |mr| mr.open? && mr.closes_issue?(self) }
   end
 
   def moved?
diff --git a/app/models/note.rb b/app/models/note.rb
index 052239af43c..c21981ead84 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -6,7 +6,7 @@ class Note < ActiveRecord::Base
 
   default_value_for :system, false
 
-  attr_mentionable :note, cache: true, pipeline: :note
+  attr_mentionable :note, pipeline: :note
   participant :author
 
   belongs_to :project
diff --git a/app/models/project_snippet.rb b/app/models/project_snippet.rb
index 5fba6baa204..25b5d777641 100644
--- a/app/models/project_snippet.rb
+++ b/app/models/project_snippet.rb
@@ -7,5 +7,6 @@ class ProjectSnippet < Snippet
   # Scopes
   scope :fresh, -> { order("created_at DESC") }
 
-  participant :author, :notes
+  participant :author
+  participant :notes_with_associations
 end
diff --git a/app/models/snippet.rb b/app/models/snippet.rb
index 0a3c3b57669..407697b745c 100644
--- a/app/models/snippet.rb
+++ b/app/models/snippet.rb
@@ -30,7 +30,8 @@ class Snippet < ActiveRecord::Base
   scope :public_and_internal, -> { where(visibility_level: [Snippet::PUBLIC, Snippet::INTERNAL]) }
   scope :fresh,   -> { order("created_at DESC") }
 
-  participant :author, :notes
+  participant :author
+  participant :notes_with_associations
 
   def self.reference_prefix
     '$'
@@ -100,6 +101,10 @@ class Snippet < ActiveRecord::Base
     content.lines.count > 1000
   end
 
+  def notes_with_associations
+    notes.includes(:author, :project)
+  end
+
   class << self
     # Searches for snippets with a matching title or file name.
     #