diff --git a/app/finders/events_finder.rb b/app/finders/events_finder.rb index b0450ddc1fd..29beb6cb224 100644 --- a/app/finders/events_finder.rb +++ b/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) diff --git a/app/finders/group_projects_finder.rb b/app/finders/group_projects_finder.rb index f043c38c6f9..f2d3b90b8e2 100644 --- a/app/finders/group_projects_finder.rb +++ b/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 diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 5bf722d1ec6..8bfbe37c543 100644 --- a/app/finders/projects_finder.rb +++ b/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 params[:owned].present? - projects << current_user.owned_projects if current_user + if current_user + collection_with_user else - projects << current_user.authorized_projects if current_user - projects << Project.unscoped.public_to_user(current_user) unless params[:non_public].present? + collection_without_user end + end - projects + def collection_with_user + if owned_projects? + current_user.owned_projects + else + 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 + + def private_only? + params[:non_public].present? end def by_ids(items) diff --git a/app/models/project.rb b/app/models/project.rb index 4c394646787..36ec4f398ca 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -266,20 +266,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') } diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index e3ef4919b28..dde2a11440d 100644 --- a/app/models/project_feature.rb +++ b/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 diff --git a/changelogs/unreleased/refactor-projects-finder-init-collection.yml b/changelogs/unreleased/refactor-projects-finder-init-collection.yml new file mode 100644 index 00000000000..c8113419f21 --- /dev/null +++ b/changelogs/unreleased/refactor-projects-finder-init-collection.yml @@ -0,0 +1,5 @@ +--- +title: Refactor ProjectsFinder#init_collection to produce more efficient queries for + retrieving projects +merge_request: +author: diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index 2b53798e70f..36e5b5041a6 100644 --- a/lib/gitlab/visibility_level.rb +++ b/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? - 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.admin? + [PRIVATE, INTERNAL, PUBLIC] + elsif user.external? + [PUBLIC] + else + [INTERNAL, PUBLIC] + end + end + def string_values string_options.keys end diff --git a/spec/lib/gitlab/visibility_level_spec.rb b/spec/lib/gitlab/visibility_level_spec.rb index 3255c6f1ef7..a8f21803ec7 100644 --- a/spec/lib/gitlab/visibility_level_spec.rb +++ b/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 = double(:user, admin?: true) + + 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 = double(:user, admin?: false, external?: false) + + expect(described_class.levels_for_user(user)). + to eq([Gitlab::VisibilityLevel::INTERNAL, + Gitlab::VisibilityLevel::PUBLIC]) + end + + it 'returns PUBLIC for external users' do + user = double(:user, admin?: false, external?: true) + + 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 diff --git a/spec/models/project_feature_spec.rb b/spec/models/project_feature_spec.rb index 09a4448d387..580c83c12c0 100644 --- a/spec/models/project_feature_spec.rb +++ b/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) } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 63333b7af1f..c7ba3ae903d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2060,4 +2060,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