Refactor Participable

There are several changes to this module:

1. The use of an explicit stack in Participable#participants
2. Proc behaviour has been changed
3. Batch permissions checking

== Explicit Stack

Participable#participants no longer uses recursion to process "self" and
all child objects, instead it uses an Array and processes objects in
breadth-first order. This allows us to for example create a single
Gitlab::ReferenceExtractor instance and pass this to any Procs. Re-using
a ReferenceExtractor removes the need for running potentially many SQL
queries every time a Proc is called on a new object.

== Proc Behaviour Changed

Previously a Proc in Participable was expected to return an Array of
User instances. This has been changed and instead it's now expected that
a Proc modifies the Gitlab::ReferenceExtractor passed to it. The return
value of the Proc is ignored.

== Permissions Checking

The method Participable#participants uses
Ability.users_that_can_read_project to check if the returned users have
access to the project of "self" _without_ running multiple SQL queries
for every user.

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)
       case true

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

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

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

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?

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

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

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.
     #

spec/models/ability_spec.rb

+require 'spec_helper'
+
+describe Ability, lib: true do
+  describe '.users_that_can_read_project' do
+    context 'using a public project' do
+      it 'returns all the users' do
+        project = create(:project, :public)
+        user = build(:user)
+
+        expect(described_class.users_that_can_read_project([user], project)).
+          to eq([user])
+      end
+    end
+
+    context 'using an internal project' do
+      let(:project) { create(:project, :internal) }
+
+      it 'returns users that are administrators' do
+        user = build(:user, admin: true)
+
+        expect(described_class.users_that_can_read_project([user], project)).
+          to eq([user])
+      end
+
+      it 'returns internal users while skipping external users' do
+        user1 = build(:user)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns external users if they are the project owner' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(project).to receive(:owner).twice.and_return(user1)
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns external users if they are project members' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(project.team).to receive(:members).twice.and_return([user1])
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns an empty Array if all users are external users without access' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([])
+      end
+    end
+
+    context 'using a private project' do
+      let(:project) { create(:project, :private) }
+
+      it 'returns users that are administrators' do
+        user = build(:user, admin: true)
+
+        expect(described_class.users_that_can_read_project([user], project)).
+          to eq([user])
+      end
+
+      it 'returns external users if they are the project owner' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(project).to receive(:owner).twice.and_return(user1)
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns external users if they are project members' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(project.team).to receive(:members).twice.and_return([user1])
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns an empty Array if all users are internal users without access' do
+        user1 = build(:user)
+        user2 = build(:user)
+        users = [user1, user2]
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([])
+      end
+
+      it 'returns an empty Array if all users are external users without access' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([])
+      end
+    end
+  end
+end

spec/models/commit_range_spec.rb

@@ -145,4 +145,27 @@ describe CommitRange, models: true do
       end
     end
   end
+
+  describe '#has_been_reverted?' do
+    it 'returns true if the commit has been reverted' do
+      issue = create(:issue)
+
+      create(:note_on_issue,
+             noteable_id: issue.id,
+             system: true,
+             note: commit1.revert_description)
+
+      expect_any_instance_of(Commit).to receive(:reverts_commit?).
+        with(commit1).
+        and_return(true)
+
+      expect(commit1.has_been_reverted?(nil, issue)).to eq(true)
+    end
+
+    it 'returns false a commit has not been reverted' do
+      issue = create(:issue)
+
+      expect(commit1.has_been_reverted?(nil, issue)).to eq(false)
+    end
+  end
 end

spec/models/commit_spec.rb

 require 'spec_helper'
 
 describe Commit, models: true do
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :public) }
   let(:commit)  { project.commit }
 
   describe 'modules' do
@@ -171,4 +171,40 @@ eos
   describe '#status' do
     # TODO: kamil
   end
+
+  describe '#participants' do
+    let(:user1) { build(:user) }
+    let(:user2) { build(:user) }
+
+    let!(:note1) do
+      create(:note_on_commit,
+             commit_id: commit.id,
+             project: project,
+             note: 'foo')
+    end
+
+    let!(:note2) do
+      create(:note_on_commit,
+             commit_id: commit.id,
+             project: project,
+             note: 'bar')
+    end
+
+    before do
+      allow(commit).to receive(:author).and_return(user1)
+      allow(commit).to receive(:committer).and_return(user2)
+    end
+
+    it 'includes the commit author' do
+      expect(commit.participants).to include(commit.author)
+    end
+
+    it 'includes the committer' do
+      expect(commit.participants).to include(commit.committer)
+    end
+
+    it 'includes the authors of the commit notes' do
+      expect(commit.participants).to include(note1.author, note2.author)
+    end
+  end
 end

spec/models/concerns/participable_spec.rb

+require 'spec_helper'
+
+describe Participable, models: true do
+  let(:model) do
+    Class.new do
+      include Participable
+    end
+  end
+
+  describe '.participant' do
+    it 'adds the participant attributes to the existing list' do
+      model.participant(:foo)
+      model.participant(:bar)
+
+      expect(model.participant_attrs).to eq([:foo, :bar])
+    end
+  end
+
+  describe '#participants' do
+    it 'returns the list of participants' do
+      model.participant(:foo)
+      model.participant(:bar)
+
+      user1 = build(:user)
+      user2 = build(:user)
+      user3 = build(:user)
+      project = build(:project, :public)
+      instance = model.new
+
+      expect(instance).to receive(:foo).and_return(user2)
+      expect(instance).to receive(:bar).and_return(user3)
+      expect(instance).to receive(:project).twice.and_return(project)
+
+      participants = instance.participants(user1)
+
+      expect(participants).to include(user2)
+      expect(participants).to include(user3)
+    end
+
+    it 'supports attributes returning another Participable' do
+      other_model = Class.new { include Participable }
+
+      other_model.participant(:bar)
+      model.participant(:foo)
+
+      instance = model.new
+      other = other_model.new
+      user1 = build(:user)
+      user2 = build(:user)
+      project = build(:project, :public)
+
+      expect(instance).to receive(:foo).and_return(other)
+      expect(other).to receive(:bar).and_return(user2)
+      expect(instance).to receive(:project).twice.and_return(project)
+
+      expect(instance.participants(user1)).to eq([user2])
+    end
+
+    context 'when using a Proc as an attribute' do
+      it 'calls the supplied Proc' do
+        user1 = build(:user)
+        project = build(:project, :public)
+
+        user_arg = nil
+        ext_arg = nil
+
+        model.participant -> (user, ext) do
+          user_arg = user
+          ext_arg = ext
+        end
+
+        instance = model.new
+
+        expect(instance).to receive(:project).twice.and_return(project)
+
+        instance.participants(user1)
+
+        expect(user_arg).to eq(user1)
+        expect(ext_arg).to be_an_instance_of(Gitlab::ReferenceExtractor)
+      end
+    end
+  end
+end

spec/models/issue_spec.rb

@@ -231,4 +231,42 @@ describe Issue, models: true do
       expect(issue.to_branch_name).to match /confidential-issue\z/
     end
   end
+
+  describe '#participants' do
+    context 'using a public project' do
+      let(:project) { create(:project, :public) }
+      let(:issue) { create(:issue, project: project) }
+
+      let!(:note1) do
+        create(:note_on_issue, noteable: issue, project: project, note: 'a')
+      end
+
+      let!(:note2) do
+        create(:note_on_issue, noteable: issue, project: project, note: 'b')
+      end
+
+      it 'includes the issue author' do
+        expect(issue.participants).to include(issue.author)
+      end
+
+      it 'includes the authors of the notes' do
+        expect(issue.participants).to include(note1.author, note2.author)
+      end
+    end
+
+    context 'using a private project' do
+      it 'does not include mentioned users that do not have access to the project' do
+        project = create(:project)
+        user = create(:user)
+        issue = create(:issue, project: project)
+
+        create(:note_on_issue,
+               noteable: issue,
+               project: project,
+               note: user.to_reference)
+
+        expect(issue.participants).not_to include(user)
+      end
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -411,4 +411,28 @@ describe MergeRequest, models: true do
       end
     end
   end
+
+  describe '#participants' do
+    let(:project) { create(:project, :public) }
+
+    let(:mr) do
+      create(:merge_request, source_project: project, target_project: project)
+    end
+
+    let!(:note1) do
+      create(:note_on_merge_request, noteable: mr, project: project, note: 'a')
+    end
+
+    let!(:note2) do
+      create(:note_on_merge_request, noteable: mr, project: project, note: 'b')
+    end
+
+    it 'includes the merge request author' do
+      expect(mr.participants).to include(mr.author)
+    end
+
+    it 'includes the authors of the notes' do
+      expect(mr.participants).to include(note1.author, note2.author)
+    end
+  end
 end

spec/models/note_spec.rb

@@ -93,8 +93,19 @@ describe Note, models: true do
     let!(:note2) { create(:note) }
 
     it "reads the rendered note body from the cache" do
-      expect(Banzai::Renderer).to receive(:render).with(note1.note, pipeline: :note, cache_key: [note1, "note"], project: note1.project)
-      expect(Banzai::Renderer).to receive(:render).with(note2.note, pipeline: :note, cache_key: [note2, "note"], project: note2.project)
+      expect(Banzai::Renderer).to receive(:render).
+        with(note1.note,
+             pipeline: :note,
+             cache_key: [note1, "note"],
+             project: note1.project,
+             author: note1.author)
+
+      expect(Banzai::Renderer).to receive(:render).
+        with(note2.note,
+             pipeline: :note,
+             cache_key: [note2, "note"],
+             project: note2.project,
+             author: note2.author)
 
       note1.all_references
       note2.all_references
@@ -195,4 +206,14 @@ describe Note, models: true do
       expect { note.valid? }.to change(note, :line_code).to(nil)
     end
   end
+
+  describe '#participants' do
+    it 'includes the note author' do
+      project = create(:project, :public)
+      issue = create(:issue, project: project)
+      note = create(:note_on_issue, noteable: issue, project: project)
+
+      expect(note.participants).to include(note.author)
+    end
+  end
 end

spec/models/snippet_spec.rb

@@ -87,4 +87,31 @@ describe Snippet, models: true do
       expect(described_class.search_code('FOO')).to eq([snippet])
     end
   end
+
+  describe '#participants' do
+    let(:project) { create(:project, :public) }
+    let(:snippet) { create(:snippet, content: 'foo', project: project) }
+
+    let!(:note1) do
+      create(:note_on_project_snippet,
+             noteable: snippet,
+             project: project,
+             note: 'a')
+    end
+
+    let!(:note2) do
+      create(:note_on_project_snippet,
+             noteable: snippet,
+             project: project,
+             note: 'b')
+    end
+
+    it 'includes the snippet author' do
+      expect(snippet.participants).to include(snippet.author)
+    end
+
+    it 'includes the note authors' do
+      expect(snippet.participants).to include(note1.author, note2.author)
+    end
+  end
 end