Merge branch 'tc-refactor-projects-finder-init-collection-ee' into 'master'

CE to EE: refactor projects finder init collection ee

See merge request !2199

app/finders/events_finder.rb

@@ -33,7 +33,8 @@ class EventsFinder
   private
 
   def by_current_user_access(events)
-    events.merge(ProjectsFinder.new(current_user: current_user).execute).references(:project)
+    events.merge(ProjectsFinder.new(current_user: current_user).execute).
+      joins(:project)
   end
 
   def by_action(events)

app/finders/group_projects_finder.rb

@@ -29,35 +29,69 @@ class GroupProjectsFinder < ProjectsFinder
   private
 
   def init_collection
-    only_owned  = options.fetch(:only_owned, false)
-    only_shared = options.fetch(:only_shared, false)
+    projects = if current_user
+                 collection_with_user
+               else
+                 collection_without_user
+               end
 
-    projects = []
+    union(projects)
+  end
 
-    if current_user
-      if group.users.include?(current_user)
-        projects << group.projects unless only_shared
-        projects << group.shared_projects unless only_owned
+  def collection_with_user
+    if group.users.include?(current_user)
+      if only_shared?
+        [shared_projects]
+      elsif only_owned?
+        [owned_projects]
       else
-        unless only_shared
-          projects << group.projects.visible_to_user(current_user)
-          projects << group.projects.public_to_user(current_user)
-        end
-
-        unless only_owned
-          projects << group.shared_projects.visible_to_user(current_user)
-          projects << group.shared_projects.public_to_user(current_user)
-        end
+        [shared_projects, owned_projects]
       end
     else
-      projects << group.projects.public_only unless only_shared
-      projects << group.shared_projects.public_only unless only_owned
+      if only_shared?
+        [shared_projects.public_or_visible_to_user(current_user)]
+      elsif only_owned?
+        [owned_projects.public_or_visible_to_user(current_user)]
+      else
+        [
+          owned_projects.public_or_visible_to_user(current_user),
+          shared_projects.public_or_visible_to_user(current_user)
+        ]
+      end
     end
+  end
 
-    projects
+  def collection_without_user
+    if only_shared?
+      [shared_projects.public_only]
+    elsif only_owned?
+      [owned_projects.public_only]
+    else
+      [shared_projects.public_only, owned_projects.public_only]
+    end
   end
 
   def union(items)
-    find_union(items, Project)
+    if items.one?
+      items.first
+    else
+      find_union(items, Project)
+    end
+  end
+
+  def only_owned?
+    options.fetch(:only_owned, false)
+  end
+
+  def only_shared?
+    options.fetch(:only_shared, false)
+  end
+
+  def owned_projects
+    group.projects
+  end
+
+  def shared_projects
+    group.shared_projects
   end
 end

app/finders/projects_finder.rb

@@ -28,34 +28,56 @@ class ProjectsFinder < UnionFinder
   end
 
   def execute
-    items = init_collection
-    items = items.map do |item|
-      item = by_ids(item)
-      item = by_personal(item)
-      item = by_starred(item)
-      item = by_trending(item)
-      item = by_visibilty_level(item)
-      item = by_tags(item)
-      item = by_search(item)
-      by_archived(item)
-    end
-    items = union(items)
-    sort(items)
+    collection = init_collection
+    collection = by_ids(collection)
+    collection = by_personal(collection)
+    collection = by_starred(collection)
+    collection = by_trending(collection)
+    collection = by_visibilty_level(collection)
+    collection = by_tags(collection)
+    collection = by_search(collection)
+    collection = by_archived(collection)
+
+    sort(collection)
   end
 
   private
 
   def init_collection
-    projects = []
+    if current_user
+      collection_with_user
+    else
+      collection_without_user
+    end
+  end
 
-    if params[:owned].present?
-      projects << current_user.owned_projects if current_user
+  def collection_with_user
+    if owned_projects?
+      current_user.owned_projects
     else
-      projects << current_user.authorized_projects if current_user
-      projects << Project.unscoped.public_to_user(current_user) unless params[:non_public].present?
+      if private_only?
+        current_user.authorized_projects
+      else
+        Project.public_or_visible_to_user(current_user)
+      end
     end
+  end
+
+  # Builds a collection for an anonymous user.
+  def collection_without_user
+    if private_only? || owned_projects?
+      Project.none
+    else
+      Project.public_to_user
+    end
+  end
+
+  def owned_projects?
+    params[:owned].present?
+  end
 
-    projects
+  def private_only?
+    params[:non_public].present?
   end
 
   def by_ids(items)

app/models/ee/user.rb

@@ -56,6 +56,27 @@ module EE
       admin? || auditor?
     end
 
+    def access_level
+      if auditor?
+        :auditor
+      else
+        super
+      end
+    end
+
+    def access_level=(new_level)
+      new_level = new_level.to_s
+      return unless %w(admin auditor regular).include?(new_level)
+
+      self.admin = (new_level == 'admin')
+      self.auditor = (new_level == 'auditor')
+    end
+
+    # Does the user have access to all private groups & projects?
+    def has_full_private_access?
+      admin_or_auditor?
+    end
+
     def remember_me!
       return if ::Gitlab::Geo.secondary?
       super

app/models/project.rb

@@ -271,20 +271,49 @@ class Project < ActiveRecord::Base
 
   enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 }
 
+  # Returns a collection of projects that is either public or visible to the
+  # logged in user.
+  def self.public_or_visible_to_user(user = nil)
+    if user
+      authorized = user.
+        project_authorizations.
+        select(1).
+        where('project_authorizations.project_id = projects.id')
+
+      levels = Gitlab::VisibilityLevel.levels_for_user(user)
+
+      where('EXISTS (?) OR projects.visibility_level IN (?)', authorized, levels)
+    else
+      public_to_user
+    end
+  end
+
   # project features may be "disabled", "internal" or "enabled". If "internal",
   # they are only available to team members. This scope returns projects where
   # the feature is either enabled, or internal with permission for the user.
+  #
+  # This method uses an optimised version of `with_feature_access_level` for
+  # logged in users to more efficiently get private projects with the given
+  # feature.
   def self.with_feature_available_for_user(feature, user)
-    return with_feature_enabled(feature) if user.try(:admin?)
+    visible = [nil, ProjectFeature::ENABLED]
 
-    unconditional = with_feature_access_level(feature, [nil, ProjectFeature::ENABLED])
-    return unconditional if user.nil?
+    if user&.admin?
+      with_feature_enabled(feature)
+    elsif user
+      column = ProjectFeature.quoted_access_level_column(feature)
 
-    conditional = with_feature_access_level(feature, ProjectFeature::PRIVATE)
-    authorized = user.authorized_projects.merge(conditional.reorder(nil))
+      authorized = user.project_authorizations.select(1).
+        where('project_authorizations.project_id = projects.id')
 
-    union = Gitlab::SQL::Union.new([unconditional.select(:id), authorized.select(:id)])
-    where(arel_table[:id].in(Arel::Nodes::SqlLiteral.new(union.to_sql)))
+      with_project_feature.
+        where("#{column} IN (?) OR (#{column} = ? AND EXISTS (?))",
+              visible,
+              ProjectFeature::PRIVATE,
+              authorized)
+    else
+      with_feature_access_level(feature, visible)
+    end
   end
 
   scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }

app/models/project_feature.rb

@@ -27,6 +27,13 @@ class ProjectFeature < ActiveRecord::Base
 
       "#{feature}_access_level".to_sym
     end
+
+    def quoted_access_level_column(feature)
+      attribute = connection.quote_column_name(access_level_attribute(feature))
+      table = connection.quote_table_name(table_name)
+
+      "#{table}.#{attribute}"
+    end
   end
 
   # Default scopes force us to unscope here since a service may need to check

app/models/user.rb

@@ -1003,8 +1003,6 @@ class User < ActiveRecord::Base
   def access_level
     if admin?
       :admin
-    elsif auditor?
-      :auditor
     else
       :regular
     end
@@ -1012,10 +1010,14 @@ class User < ActiveRecord::Base
 
   def access_level=(new_level)
     new_level = new_level.to_s
-    return unless %w(admin auditor regular).include?(new_level)
+    return unless %w(admin regular).include?(new_level)
 
     self.admin = (new_level == 'admin')
-    self.auditor = (new_level == 'auditor')
+  end
+
+  # Does the user have access to all private groups & projects?
+  def has_full_private_access?
+    admin?
   end
 
   def update_two_factor_requirement

changelogs/unreleased/refactor-projects-finder-init-collection.yml

+---
+title: Refactor ProjectsFinder#init_collection to produce more efficient queries for
+  retrieving projects
+merge_request:
+author:

lib/gitlab/visibility_level.rb

@@ -13,18 +13,8 @@ module Gitlab
       scope :public_and_internal_only,  -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
       scope :non_public_only,           -> { where.not(visibility_level: PUBLIC) }
 
-      scope :public_to_user, -> (user) do
-        if user
-          if user.admin_or_auditor?
-            all
-          elsif !user.external?
-            public_and_internal_only
-          else
-            public_only
-          end
-        else
-          public_only
-        end
+      scope :public_to_user, -> (user = nil) do
+        where(visibility_level: VisibilityLevel.levels_for_user(user))
       end
     end
 
@@ -35,6 +25,18 @@ module Gitlab
     class << self
       delegate :values, to: :options
 
+      def levels_for_user(user = nil)
+        return [PUBLIC] unless user
+
+        if user.has_full_private_access?
+          [PRIVATE, INTERNAL, PUBLIC]
+        elsif user.external?
+          [PUBLIC]
+        else
+          [INTERNAL, PUBLIC]
+        end
+      end
+
       def string_values
         string_options.keys
       end

spec/lib/gitlab/ee/visibility_level_spec.rb

+require 'spec_helper'
+
+describe Gitlab::VisibilityLevel, lib: true do  # rubocop:disable RSpec/FilePath
+  describe '.levels_for_user' do
+    it 'returns all levels for an auditor' do
+      user = build(:user, :auditor)
+
+      expect(described_class.levels_for_user(user)).
+        to eq([Gitlab::VisibilityLevel::PRIVATE,
+               Gitlab::VisibilityLevel::INTERNAL,
+               Gitlab::VisibilityLevel::PUBLIC])
+    end
+  end
+end

spec/lib/gitlab/visibility_level_spec.rb

@@ -18,4 +18,35 @@ describe Gitlab::VisibilityLevel, lib: true do
       expect(described_class.level_value(100)).to eq(Gitlab::VisibilityLevel::PRIVATE)
     end
   end
+
+  describe '.levels_for_user' do
+    it 'returns all levels for an admin' do
+      user = build(:user, :admin)
+
+      expect(described_class.levels_for_user(user)).
+        to eq([Gitlab::VisibilityLevel::PRIVATE,
+               Gitlab::VisibilityLevel::INTERNAL,
+               Gitlab::VisibilityLevel::PUBLIC])
+    end
+
+    it 'returns INTERNAL and PUBLIC for internal users' do
+      user = build(:user)
+
+      expect(described_class.levels_for_user(user)).
+        to eq([Gitlab::VisibilityLevel::INTERNAL,
+               Gitlab::VisibilityLevel::PUBLIC])
+    end
+
+    it 'returns PUBLIC for external users' do
+      user = build(:user, :external)
+
+      expect(described_class.levels_for_user(user)).
+        to eq([Gitlab::VisibilityLevel::PUBLIC])
+    end
+
+    it 'returns PUBLIC when no user is given' do
+      expect(described_class.levels_for_user).
+        to eq([Gitlab::VisibilityLevel::PUBLIC])
+    end
+  end
 end

spec/models/ee/user_spec.rb

+require 'spec_helper'
+
+describe EE::User, models: true do
+  describe '#access_level=' do
+    let(:user) { build(:user) }
+
+    before do
+      # `auditor?` returns true only when the user is an auditor _and_ the auditor license
+      # add-on is present. We aren't testing this here, so we can assume that the add-on exists.
+      allow_any_instance_of(License).to receive(:feature_available?).with(:auditor_user) { true }
+    end
+
+    it "does not set 'auditor' for an invalid access level" do
+      user.access_level = :invalid_access_level
+
+      expect(user.auditor).to be false
+    end
+
+    it "does not set 'auditor' for admin level" do
+      user.access_level = :admin
+
+      expect(user.auditor).to be false
+    end
+
+    it "assigns the 'auditor' access level" do
+      user.access_level = :auditor
+
+      expect(user.access_level).to eq(:auditor)
+      expect(user.admin).to be false
+      expect(user.auditor).to be true
+    end
+
+    it "assigns the 'auditor' access level" do
+      user.access_level = :regular
+
+      expect(user.access_level).to eq(:regular)
+      expect(user.admin).to be false
+      expect(user.auditor).to be false
+    end
+
+    it "clears the 'admin' access level when a user is made an auditor" do
+      user.access_level = :admin
+      user.access_level = :auditor
+
+      expect(user.access_level).to eq(:auditor)
+      expect(user.admin).to be false
+      expect(user.auditor).to be true
+    end
+
+    it "clears the 'auditor' access level when a user is made an admin" do
+      user.access_level = :auditor
+      user.access_level = :admin
+
+      expect(user.access_level).to eq(:admin)
+      expect(user.admin).to be true
+      expect(user.auditor).to be false
+    end
+
+    it "doesn't clear existing 'auditor' access levels when an invalid access level is passed in" do
+      user.access_level = :auditor
+      user.access_level = :invalid_access_level
+
+      expect(user.access_level).to eq(:auditor)
+      expect(user.admin).to be false
+      expect(user.auditor).to be true
+    end
+  end
+
+  describe '#has_full_private_access?' do
+    it 'returns true for auditor user' do
+      user = build(:user, :auditor)
+
+      expect(user.has_full_private_access?).to be_truthy
+    end
+  end
+end

spec/models/project_feature_spec.rb

@@ -4,6 +4,18 @@ describe ProjectFeature do
   let(:project) { create(:empty_project) }
   let(:user) { create(:user) }
 
+  describe '.quoted_access_level_column' do
+    it 'returns the table name and quoted column name for a feature' do
+      expected = if Gitlab::Database.postgresql?
+                   '"project_features"."issues_access_level"'
+                 else
+                   '`project_features`.`issues_access_level`'
+                 end
+
+      expect(described_class.quoted_access_level_column(:issues)).to eq(expected)
+    end
+  end
+
   describe '#feature_available?' do
     let(:features) { %w(issues wiki builds merge_requests snippets repository) }
 

spec/models/project_spec.rb

@@ -2552,4 +2552,36 @@ describe Project, models: true do
       expect(project.last_repository_updated_at.to_i).to eq(project.created_at.to_i)
     end
   end
+
+  describe '.public_or_visible_to_user' do
+    let!(:user) { create(:user) }
+
+    let!(:private_project) do
+      create(:empty_project, :private, creator: user, namespace: user.namespace)
+    end
+
+    let!(:public_project) { create(:empty_project, :public) }
+
+    context 'with a user' do
+      let(:projects) do
+        Project.all.public_or_visible_to_user(user)
+      end
+
+      it 'includes projects the user has access to' do
+        expect(projects).to include(private_project)
+      end
+
+      it 'includes projects the user can see' do
+        expect(projects).to include(public_project)
+      end
+    end
+
+    context 'without a user' do
+      it 'only includes public projects' do
+        projects = Project.all.public_or_visible_to_user
+
+        expect(projects).to eq([public_project])
+      end
+    end
+  end
 end

spec/models/user_spec.rb

@@ -1737,18 +1737,11 @@ describe User, models: true do
   describe '#access_level=' do
     let(:user) { build(:user) }
 
-    before do
-      # `auditor?` returns true only when the user is an auditor _and_ the auditor license
-      # add-on is present. We aren't testing this here, so we can assume that the add-on exists.
-      allow_any_instance_of(License).to receive(:feature_available?).with(:auditor_user) { true }
-    end
-
     it 'does nothing for an invalid access level' do
       user.access_level = :invalid_access_level
 
       expect(user.access_level).to eq(:regular)
       expect(user.admin).to be false
-      expect(user.auditor).to be false
     end
 
     it "assigns the 'admin' access level" do
@@ -1756,41 +1749,6 @@ describe User, models: true do
 
       expect(user.access_level).to eq(:admin)
       expect(user.admin).to be true
-      expect(user.auditor).to be false
-    end
-
-    it "assigns the 'auditor' access level" do
-      user.access_level = :auditor
-
-      expect(user.access_level).to eq(:auditor)
-      expect(user.admin).to be false
-      expect(user.auditor).to be true
-    end
-
-    it "assigns the 'auditor' access level" do
-      user.access_level = :regular
-
-      expect(user.access_level).to eq(:regular)
-      expect(user.admin).to be false
-      expect(user.auditor).to be false
-    end
-
-    it "clears the 'admin' access level when a user is made an auditor" do
-      user.access_level = :admin
-      user.access_level = :auditor
-
-      expect(user.access_level).to eq(:auditor)
-      expect(user.admin).to be false
-      expect(user.auditor).to be true
-    end
-
-    it "clears the 'auditor' access level when a user is made an admin" do
-      user.access_level = :auditor
-      user.access_level = :admin
-
-      expect(user.access_level).to eq(:admin)
-      expect(user.admin).to be true
-      expect(user.auditor).to be false
     end
 
     it "doesn't clear existing access levels when an invalid access level is passed in" do
@@ -1799,7 +1757,6 @@ describe User, models: true do
 
       expect(user.access_level).to eq(:admin)
       expect(user.admin).to be true
-      expect(user.auditor).to be false
     end
 
     it "accepts string values in addition to symbols" do
@@ -1807,7 +1764,20 @@ describe User, models: true do
 
       expect(user.access_level).to eq(:admin)
       expect(user.admin).to be true
-      expect(user.auditor).to be false
+    end
+  end
+
+  describe '#has_full_private_access?' do
+    it 'returns false for regular user' do
+      user = build(:user)
+
+      expect(user.has_full_private_access?).to be_falsy
+    end
+
+    it 'returns true for admin user' do
+      user = build(:user, :admin)
+
+      expect(user.has_full_private_access?).to be_truthy
     end
   end