diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 6d6e0cc6c7f65334faf1d04413ef2aad39ac4c81..c51041513b56ee6437eb7c2c2cca3d877a94f9a4 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -41,7 +41,7 @@ module IssuableCollections return if pagination_disabled? @issuables = @issuables.page(params[:page]) - @issuable_meta_data = issuable_meta_data(@issuables, collection_type) + @issuable_meta_data = issuable_meta_data(@issuables, collection_type, current_user) @total_pages = issuable_page_count end # rubocop:enable Gitlab/ModuleWithInstanceVariables diff --git a/app/controllers/concerns/issuable_collections_action.rb b/app/controllers/concerns/issuable_collections_action.rb index 18ed4027eac88e986b921148f826fe3405a843c0..4ad287c4a13c748bc10c502676bdec00a4e6e47f 100644 --- a/app/controllers/concerns/issuable_collections_action.rb +++ b/app/controllers/concerns/issuable_collections_action.rb @@ -11,7 +11,7 @@ module IssuableCollectionsAction .non_archived .page(params[:page]) - @issuable_meta_data = issuable_meta_data(@issues, collection_type) + @issuable_meta_data = issuable_meta_data(@issues, collection_type, current_user) respond_to do |format| format.html @@ -22,7 +22,7 @@ module IssuableCollectionsAction def merge_requests @merge_requests = issuables_collection.page(params[:page]) - @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type) + @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type, current_user) end # rubocop:enable Gitlab/ModuleWithInstanceVariables diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 62b97fc25902f5181acf6b39c19ee5f891e88568..4ab23a0b937bccd9fffd0989431baf43b78fbacb 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -297,7 +297,7 @@ class ProjectsController < Projects::ApplicationController elsif @project.feature_available?(:issues, current_user) @issues = issuables_collection.page(params[:page]) @collection_type = 'Issue' - @issuable_meta_data = issuable_meta_data(@issues, @collection_type) + @issuable_meta_data = issuable_meta_data(@issues, @collection_type, current_user) end render :show diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index fa9dda2ab31a69b22a2425e3a1eb7187d2cc062f..21ae580fa048d72dcdca11e85cef22f3cceb9047 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -29,6 +29,7 @@ # updated_after: datetime # updated_before: datetime # attempt_group_search_optimizations: boolean +# attempt_project_search_optimizations: boolean # class IssuableFinder prepend FinderWithCrossProjectAccess @@ -184,7 +185,6 @@ class IssuableFinder @project = project end - # rubocop: disable CodeReuse/ActiveRecord def projects return @projects if defined?(@projects) @@ -192,17 +192,25 @@ class IssuableFinder projects = if current_user && params[:authorized_only].presence && !current_user_related? - current_user.authorized_projects + current_user.authorized_projects(min_access_level) elsif group - finder_options = { include_subgroups: params[:include_subgroups], only_owned: true } - GroupProjectsFinder.new(group: group, current_user: current_user, options: finder_options).execute # rubocop: disable CodeReuse/Finder + find_group_projects else - ProjectsFinder.new(current_user: current_user).execute # rubocop: disable CodeReuse/Finder + Project.public_or_visible_to_user(current_user, min_access_level) end - @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil) + @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil) # rubocop: disable CodeReuse/ActiveRecord + end + + def find_group_projects + return Project.none unless group + + if params[:include_subgroups] + Project.where(namespace_id: group.self_and_descendants) # rubocop: disable CodeReuse/ActiveRecord + else + group.projects + end.public_or_visible_to_user(current_user, min_access_level) end - # rubocop: enable CodeReuse/ActiveRecord def search params[:search].presence @@ -572,4 +580,8 @@ class IssuableFinder scope = params[:scope] scope == 'created_by_me' || scope == 'authored' || scope == 'assigned_to_me' end + + def min_access_level + ProjectFeature.required_minimum_access_level(klass) + end end diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index cb44575d6f196c280ac0ad2ea561dff5a9d32212..c827059643053f165e7f1123e2bc95119588ed19 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -48,9 +48,9 @@ class IssuesFinder < IssuableFinder OR (issues.confidential = TRUE AND (issues.author_id = :user_id OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id) - OR issues.project_id IN(:project_ids)))', + OR EXISTS (:authorizations)))', user_id: current_user.id, - project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id)) + authorizations: current_user.authorizations_for_projects(min_access_level: CONFIDENTIAL_ACCESS_LEVEL, related_project_column: "issues.project_id")) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 93d3c991846463c6b9f2960ced89a4415c1c44be..23b731b1aede66329c44953b8232b7f0c8ab4025 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -62,7 +62,7 @@ class ProjectsFinder < UnionFinder collection = by_personal(collection) collection = by_starred(collection) collection = by_trending(collection) - collection = by_visibilty_level(collection) + collection = by_visibility_level(collection) collection = by_tags(collection) collection = by_search(collection) collection = by_archived(collection) @@ -71,12 +71,11 @@ class ProjectsFinder < UnionFinder collection end - # rubocop: disable CodeReuse/ActiveRecord def collection_with_user if owned_projects? current_user.owned_projects elsif min_access_level? - current_user.authorized_projects.where('project_authorizations.access_level >= ?', params[:min_access_level]) + current_user.authorized_projects(params[:min_access_level]) else if private_only? current_user.authorized_projects @@ -85,7 +84,6 @@ class ProjectsFinder < UnionFinder end end end - # rubocop: enable CodeReuse/ActiveRecord # Builds a collection for an anonymous user. def collection_without_user @@ -131,7 +129,7 @@ class ProjectsFinder < UnionFinder end # rubocop: disable CodeReuse/ActiveRecord - def by_visibilty_level(items) + def by_visibility_level(items) params[:visibility_level].present? ? items.where(visibility_level: params[:visibility_level]) : items end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 52c49498e9b8b98086bb7acb12952e45effba744..c61c43c9090a58f8589ddc307caa0deb1db55dfa 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -277,7 +277,7 @@ module IssuablesHelper initialTaskStatus: issuable.task_status } - data[:hasClosingMergeRequest] = issuable.merge_requests_count != 0 if issuable.is_a?(Issue) + data[:hasClosingMergeRequest] = issuable.merge_requests_count(current_user) != 0 if issuable.is_a?(Issue) if parent.is_a?(Group) data[:groupPath] = parent.path diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 17f94b4bd9b120fd8373a581b020dd7ba10bfa89..2523d0070a6f69a76ab28c1ec3beba62c9fb6a89 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -29,7 +29,11 @@ module Issuable # This object is used to gather issuable meta data for displaying # upvotes, downvotes, notes and closing merge requests count for issues and merge requests # lists avoiding n+1 queries and improving performance. - IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :merge_requests_count) + IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :mrs_count) do + def merge_requests_count(user = nil) + mrs_count + end + end included do cache_markdown_field :title, pipeline: :single_line diff --git a/app/models/issue.rb b/app/models/issue.rb index eb4c87e05d590f5583a7eae2407e3961c05472f4..44e7119239da95c92c9748511c479f13f57ef3f9 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -270,8 +270,8 @@ class Issue < ApplicationRecord end # rubocop: enable CodeReuse/ServiceClass - def merge_requests_count - merge_requests_closing_issues.count + def merge_requests_count(user = nil) + ::MergeRequestsClosingIssues.count_for_issue(self.id, user) end private diff --git a/app/models/merge_requests_closing_issues.rb b/app/models/merge_requests_closing_issues.rb index 61af50841ee2801494f6db9874b2a1a54cc8e38e..22cedf57b86e20f7411415d4b48b0ea5b4d98273 100644 --- a/app/models/merge_requests_closing_issues.rb +++ b/app/models/merge_requests_closing_issues.rb @@ -7,11 +7,38 @@ class MergeRequestsClosingIssues < ApplicationRecord validates :merge_request_id, uniqueness: { scope: :issue_id }, presence: true validates :issue_id, presence: true + scope :with_issues, ->(ids) { where(issue_id: ids) } + scope :with_merge_requests_enabled, -> do + joins(:merge_request) + .joins('INNER JOIN project_features ON merge_requests.target_project_id = project_features.project_id') + .where('project_features.merge_requests_access_level >= :access', access: ProjectFeature::ENABLED) + end + + scope :accessible_by, ->(user) do + joins(:merge_request) + .joins('INNER JOIN project_features ON merge_requests.target_project_id = project_features.project_id') + .where('project_features.merge_requests_access_level >= :access OR EXISTS(:authorizations)', + access: ProjectFeature::ENABLED, + authorizations: user.authorizations_for_projects(min_access_level: Gitlab::Access::REPORTER, related_project_column: "merge_requests.target_project_id") + ) + end + class << self - def count_for_collection(ids) - group(:issue_id) - .where(issue_id: ids) - .pluck('issue_id', 'COUNT(*) as count') + def count_for_collection(ids, current_user) + closing_merge_requests(ids, current_user).group(:issue_id).pluck('issue_id', 'COUNT(*) as count') + end + + def count_for_issue(id, current_user) + closing_merge_requests(id, current_user).count + end + + private + + def closing_merge_requests(ids, current_user) + return with_issues(ids) if current_user&.admin? + return with_issues(ids).with_merge_requests_enabled if current_user.blank? + + with_issues(ids).accessible_by(current_user) end end end diff --git a/app/models/project.rb b/app/models/project.rb index 873f93a3b0561f4737ad2902ac556e525df18460..d0b4c85c2af1e7be41eb3136127e72162ce9f8c1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -461,10 +461,12 @@ class Project < ApplicationRecord # 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) + def self.public_or_visible_to_user(user = nil, min_access_level = nil) + min_access_level = nil if user&.admin? + if user where('EXISTS (?) OR projects.visibility_level IN (?)', - user.authorizations_for_projects, + user.authorizations_for_projects(min_access_level: min_access_level), Gitlab::VisibilityLevel.levels_for_user(user)) else public_to_user @@ -474,30 +476,32 @@ class Project < ApplicationRecord # project features may be "disabled", "internal", "enabled" or "public". If "internal", # they are only available to team members. This scope returns projects where # the feature is either public, enabled, or internal with permission for the user. + # Note: this scope doesn't enforce that the user has access to the projects, it just checks + # that the user has access to the feature. It's important to use this scope with others + # that checks project authorizations first. # # 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) visible = [ProjectFeature::ENABLED, ProjectFeature::PUBLIC] - min_access_level = ProjectFeature.required_minimum_access_level(feature) if user&.admin? with_feature_enabled(feature) elsif user + min_access_level = ProjectFeature.required_minimum_access_level(feature) column = ProjectFeature.quoted_access_level_column(feature) with_project_feature - .where( - "(projects.visibility_level > :private AND (#{column} IS NULL OR #{column} >= (:public_visible) OR (#{column} = :private_visible AND EXISTS(:authorizations))))"\ - " OR (projects.visibility_level = :private AND (#{column} IS NULL OR #{column} >= :private_visible) AND EXISTS(:authorizations))", - { - private: Gitlab::VisibilityLevel::PRIVATE, - public_visible: ProjectFeature::ENABLED, - private_visible: ProjectFeature::PRIVATE, - authorizations: user.authorizations_for_projects(min_access_level: min_access_level) - }) + .where("#{column} IS NULL OR #{column} IN (:public_visible) OR (#{column} = :private_visible AND EXISTS (:authorizations))", + { + public_visible: visible, + private_visible: ProjectFeature::PRIVATE, + authorizations: user.authorizations_for_projects(min_access_level: min_access_level) + }) else + # This has to be added to include features whose value is nil in the db + visible << nil with_feature_access_level(feature, visible) end end diff --git a/app/models/user.rb b/app/models/user.rb index d3524bfd6ae13364d73e6d9b11d5af296e6e4755..541ec3098e255f9a7ee29fe04a1c24942ccc11f4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -761,11 +761,15 @@ class User < ApplicationRecord # Typically used in conjunction with projects table to get projects # a user has been given access to. + # The param `related_project_column` is the column to compare to the + # project_authorizations. By default is projects.id # # Example use: # `Project.where('EXISTS(?)', user.authorizations_for_projects)` - def authorizations_for_projects(min_access_level: nil) - authorizations = project_authorizations.select(1).where('project_authorizations.project_id = projects.id') + def authorizations_for_projects(min_access_level: nil, related_project_column: 'projects.id') + authorizations = project_authorizations + .select(1) + .where("project_authorizations.project_id = #{related_project_column}") return authorizations unless min_access_level.present? diff --git a/app/views/shared/_issuable_meta_data.html.haml b/app/views/shared/_issuable_meta_data.html.haml index 31a5370a5f8d66f74f5711f754a24614fb763fe6..71b13a5d741667280da243831bb7c7d9983ffe8f 100644 --- a/app/views/shared/_issuable_meta_data.html.haml +++ b/app/views/shared/_issuable_meta_data.html.haml @@ -2,7 +2,7 @@ - issue_votes = @issuable_meta_data[issuable.id] - upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes - issuable_url = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes') -- issuable_mr = @issuable_meta_data[issuable.id].merge_requests_count +- issuable_mr = @issuable_meta_data[issuable.id].merge_requests_count(current_user) - if issuable_mr > 0 %li.issuable-mr.d-none.d-sm-block.has-tooltip{ title: _('Related merge requests') } diff --git a/changelogs/unreleased/fj-59522-improve-search-controller-performance.yml b/changelogs/unreleased/fj-59522-improve-search-controller-performance.yml new file mode 100644 index 0000000000000000000000000000000000000000..c513f3c3aebdf372e84aac014f9de97b89b5d00f --- /dev/null +++ b/changelogs/unreleased/fj-59522-improve-search-controller-performance.yml @@ -0,0 +1,5 @@ +--- +title: Add improvements to global search of issues and merge requests +merge_request: 27817 +author: +type: performance diff --git a/changelogs/unreleased/security-59581-related-merge-requests-count.yml b/changelogs/unreleased/security-59581-related-merge-requests-count.yml new file mode 100644 index 0000000000000000000000000000000000000000..83faa2f7c1375cd55d9518d257784ec49c5311dd --- /dev/null +++ b/changelogs/unreleased/security-59581-related-merge-requests-count.yml @@ -0,0 +1,5 @@ +--- +title: Expose merge requests count based on user access +merge_request: +author: +type: security diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 079ee7f5cccd1c0530e78bcdef260e403a1c08cb..44996b0c737543e84747930f7ffde7c4910646be 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -493,9 +493,9 @@ module API expose :state, :created_at, :updated_at # Avoids an N+1 query when metadata is included - def issuable_metadata(subject, options, method) + def issuable_metadata(subject, options, method, args = nil) cached_subject = options.dig(:issuable_metadata, subject.id) - (cached_subject || subject).public_send(method) # rubocop: disable GitlabSecurity/PublicSend + (cached_subject || subject).public_send(method, *args) # rubocop: disable GitlabSecurity/PublicSend end end @@ -554,7 +554,7 @@ module API end expose(:user_notes_count) { |issue, options| issuable_metadata(issue, options, :user_notes_count) } - expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count) } + expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count, options[:current_user]) } expose(:upvotes) { |issue, options| issuable_metadata(issue, options, :upvotes) } expose(:downvotes) { |issue, options| issuable_metadata(issue, options, :downvotes) } expose :due_date diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 999a9cb5a825e86adbea2c0921c1dbce0b3c8a4c..54ee62e3e8a96f9cd2be8d15d3f52cf606f09e16 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -93,7 +93,7 @@ module API options = { with: Entities::IssueBasic, current_user: current_user, - issuable_metadata: issuable_meta_data(issues, 'Issue') + issuable_metadata: issuable_meta_data(issues, 'Issue', current_user) } present issues, options @@ -120,7 +120,7 @@ module API options = { with: Entities::IssueBasic, current_user: current_user, - issuable_metadata: issuable_meta_data(issues, 'Issue') + issuable_metadata: issuable_meta_data(issues, 'Issue', current_user) } present issues, options @@ -150,7 +150,7 @@ module API with: Entities::IssueBasic, current_user: current_user, project: user_project, - issuable_metadata: issuable_meta_data(issues, 'Issue') + issuable_metadata: issuable_meta_data(issues, 'Issue', current_user) } present issues, options diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index f164e1c1eeb37d80983b9c8d7cf594bc12cdd1f9..460fb2ed6b8594dedd6e812e521c462b59ad16dc 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -71,7 +71,7 @@ module API if params[:view] == 'simple' options[:with] = Entities::MergeRequestSimple else - options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest') + options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest', current_user) end options diff --git a/lib/api/todos.rb b/lib/api/todos.rb index d2196f05173e197705791ee307587e7ea5697d55..f332a554c41a719378baeebf5e68b2736cb7fba9 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -65,7 +65,7 @@ module API next unless collection targets = collection.map(&:target) - options[type] = { issuable_metadata: issuable_meta_data(targets, type) } + options[type] = { issuable_metadata: issuable_meta_data(targets, type, current_user) } end end end diff --git a/lib/gitlab/group_search_results.rb b/lib/gitlab/group_search_results.rb index 7255293b19448543261df91d7173b414cbb1e24c..334642f252e55689ab1478c1bb6413dfdff6f593 100644 --- a/lib/gitlab/group_search_results.rb +++ b/lib/gitlab/group_search_results.rb @@ -2,6 +2,8 @@ module Gitlab class GroupSearchResults < SearchResults + attr_reader :group + def initialize(current_user, limit_projects, group, query, default_project_filter: false, per_page: 20) super(current_user, limit_projects, query, default_project_filter: default_project_filter, per_page: per_page) @@ -26,5 +28,9 @@ module Gitlab .where(id: groups.select('members.user_id')) end # rubocop:enable CodeReuse/ActiveRecord + + def issuable_params + super.merge(group_id: group.id) + end end end diff --git a/lib/gitlab/issuable_metadata.rb b/lib/gitlab/issuable_metadata.rb index 351d15605e0cff8119dbe503de85f60b0ff54a58..be73bcd5506ba690ca732711b8287d5099f81a8d 100644 --- a/lib/gitlab/issuable_metadata.rb +++ b/lib/gitlab/issuable_metadata.rb @@ -2,7 +2,7 @@ module Gitlab module IssuableMetadata - def issuable_meta_data(issuable_collection, collection_type) + def issuable_meta_data(issuable_collection, collection_type, user = nil) # ActiveRecord uses Object#extend for null relations. if !(issuable_collection.singleton_class < ActiveRecord::NullRelation) && issuable_collection.respond_to?(:limit_value) && @@ -23,7 +23,7 @@ module Gitlab issuable_votes_count = ::AwardEmoji.votes_for_collection(issuable_ids, collection_type) issuable_merge_requests_count = if collection_type == 'Issue' - ::MergeRequestsClosingIssues.count_for_collection(issuable_ids) + ::MergeRequestsClosingIssues.count_for_collection(issuable_ids, user) else [] end diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb index 9dc162b7b4fdc70a3c056f2abed85b747322a62d..0f3b97e23176ba5b1e28a24513c0fb56a0ac7f11 100644 --- a/lib/gitlab/project_search_results.rb +++ b/lib/gitlab/project_search_results.rb @@ -151,5 +151,9 @@ module Gitlab def repository_wiki_ref @repository_wiki_ref ||= repository_ref || project.wiki.default_branch end + + def issuable_params + super.merge(project_id: project.id) + end end end diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index e52c82868710b7bca0b02c976241fc0dc9f6222b..7c1e6b1baff36ea799df7fa5a2c2fe9ca48b23a1 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -2,6 +2,8 @@ module Gitlab class SearchResults + COUNT_LIMIT = 1001 + attr_reader :current_user, :query, :per_page # Limit search results by passed projects @@ -25,29 +27,26 @@ module Gitlab def objects(scope, page = nil, without_count = true) collection = case scope when 'projects' - projects.page(page).per(per_page) + projects when 'issues' - issues.page(page).per(per_page) + issues when 'merge_requests' - merge_requests.page(page).per(per_page) + merge_requests when 'milestones' - milestones.page(page).per(per_page) + milestones when 'users' - users.page(page).per(per_page) + users else - Kaminari.paginate_array([]).page(page).per(per_page) - end + Kaminari.paginate_array([]) + end.page(page).per(per_page) without_count ? collection.without_count : collection end - # rubocop: disable CodeReuse/ActiveRecord def limited_projects_count - @limited_projects_count ||= projects.limit(count_limit).count + @limited_projects_count ||= limited_count(projects) end - # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord def limited_issues_count return @limited_issues_count if @limited_issues_count @@ -56,35 +55,28 @@ module Gitlab # and confidential issues user has access to, is too complex. # It's faster to try to fetch all public issues first, then only # if necessary try to fetch all issues. - sum = issues(public_only: true).limit(count_limit).count - @limited_issues_count = sum < count_limit ? issues.limit(count_limit).count : sum + sum = limited_count(issues(public_only: true)) + @limited_issues_count = sum < count_limit ? limited_count(issues) : sum end - # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord def limited_merge_requests_count - @limited_merge_requests_count ||= merge_requests.limit(count_limit).count + @limited_merge_requests_count ||= limited_count(merge_requests) end - # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord def limited_milestones_count - @limited_milestones_count ||= milestones.limit(count_limit).count + @limited_milestones_count ||= limited_count(milestones) end - # rubocop: enable CodeReuse/ActiveRecord - # rubocop:disable CodeReuse/ActiveRecord def limited_users_count - @limited_users_count ||= users.limit(count_limit).count + @limited_users_count ||= limited_count(users) end - # rubocop:enable CodeReuse/ActiveRecord def single_commit_result? false end def count_limit - 1001 + COUNT_LIMIT end def users @@ -99,23 +91,15 @@ module Gitlab limit_projects.search(query) end - # rubocop: disable CodeReuse/ActiveRecord def issues(finder_params = {}) - issues = IssuesFinder.new(current_user, finder_params).execute + issues = IssuesFinder.new(current_user, issuable_params.merge(finder_params)).execute + unless default_project_filter - issues = issues.where(project_id: project_ids_relation) + issues = issues.where(project_id: project_ids_relation) # rubocop: disable CodeReuse/ActiveRecord end - issues = - if query =~ /#(\d+)\z/ - issues.where(iid: $1) - else - issues.full_search(query) - end - - issues.reorder('updated_at DESC') + issues end - # rubocop: enable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord def milestones @@ -127,23 +111,15 @@ module Gitlab end # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord def merge_requests - merge_requests = MergeRequestsFinder.new(current_user).execute + merge_requests = MergeRequestsFinder.new(current_user, issuable_params).execute + unless default_project_filter merge_requests = merge_requests.in_projects(project_ids_relation) end - merge_requests = - if query =~ /[#!](\d+)\z/ - merge_requests.where(iid: $1) - else - merge_requests.full_search(query) - end - - merge_requests.reorder('updated_at DESC') + merge_requests end - # rubocop: enable CodeReuse/ActiveRecord def default_scope 'projects' @@ -174,5 +150,23 @@ module Gitlab limit_projects.select(:id).reorder(nil) end # rubocop: enable CodeReuse/ActiveRecord + + def issuable_params + {}.tap do |params| + params[:sort] = 'updated_desc' + + if query =~ /#(\d+)\z/ + params[:iids] = $1 + else + params[:search] = query + end + end + end + + # rubocop: disable CodeReuse/ActiveRecord + def limited_count(relation) + relation.reorder(nil).limit(count_limit).size + end + # rubocop: enable CodeReuse/ActiveRecord end end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index ab185ab3972269eff07a48e550aec39f7ed54c95..743ec32288506abb80ab1fab0ffa0339aab79a33 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -260,6 +260,7 @@ FactoryBot.define do trait(:merge_requests_enabled) { merge_requests_access_level ProjectFeature::ENABLED } trait(:merge_requests_disabled) { merge_requests_access_level ProjectFeature::DISABLED } trait(:merge_requests_private) { merge_requests_access_level ProjectFeature::PRIVATE } + trait(:merge_requests_public) { merge_requests_access_level ProjectFeature::PUBLIC } trait(:repository_enabled) { repository_access_level ProjectFeature::ENABLED } trait(:repository_disabled) { repository_access_level ProjectFeature::DISABLED } trait(:repository_private) { repository_access_level ProjectFeature::PRIVATE } diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 4133987a07ed8bcacb6f2b3d8d1fa783320300d3..f66395c48b7f206b360bf00f7c7564c149f98e62 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -669,9 +669,7 @@ describe IssuesFinder do end it 'filters by confidentiality' do - expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything) - - subject + expect(subject.to_sql).to match("issues.confidential") end end @@ -688,9 +686,7 @@ describe IssuesFinder do end it 'filters by confidentiality' do - expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything) - - subject + expect(subject.to_sql).to match("issues.confidential") end end diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 9d4b9af3ec3855da074f0371ec839d4d4930658f..a2417f8df1b335443f6277220636c684d5b8cac0 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -31,7 +31,7 @@ describe MergeRequestsFinder do end context 'filtering by group' do - it 'includes all merge requests when user has access exceluding merge requests from projects the user does not have access to' do + it 'includes all merge requests when user has access excluding merge requests from projects the user does not have access to' do private_project = allow_gitaly_n_plus_1 { create(:project, :private, group: group) } private_project.add_guest(user) create(:merge_request, :simple, author: user, source_project: private_project, target_project: private_project) diff --git a/spec/lib/gitlab/issuable_metadata_spec.rb b/spec/lib/gitlab/issuable_metadata_spec.rb index 6ec861632336e5845a5d56c4e5214044a2d6e8fd..a0a6353bb5ca3d0da186a00554bc7c320bed4279 100644 --- a/spec/lib/gitlab/issuable_metadata_spec.rb +++ b/spec/lib/gitlab/issuable_metadata_spec.rb @@ -7,11 +7,11 @@ describe Gitlab::IssuableMetadata do subject { Class.new { include Gitlab::IssuableMetadata }.new } it 'returns an empty Hash if an empty collection is provided' do - expect(subject.issuable_meta_data(Issue.none, 'Issue')).to eq({}) + expect(subject.issuable_meta_data(Issue.none, 'Issue', user)).to eq({}) end it 'raises an error when given a collection with no limit' do - expect { subject.issuable_meta_data(Issue.all, 'Issue') }.to raise_error(/must have a limit/) + expect { subject.issuable_meta_data(Issue.all, 'Issue', user) }.to raise_error(/must have a limit/) end context 'issues' do @@ -23,7 +23,7 @@ describe Gitlab::IssuableMetadata do let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) } it 'aggregates stats on issues' do - data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue') + data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue', user) expect(data.count).to eq(2) expect(data[issue.id].upvotes).to eq(1) @@ -46,7 +46,7 @@ describe Gitlab::IssuableMetadata do let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") } it 'aggregates stats on merge requests' do - data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest') + data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest', user) expect(data.count).to eq(2) expect(data[merge_request.id].upvotes).to eq(1) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 07de913516c302fb810a2bdb650dcc5531431c22..7013616314ea2a05a93902ca68b58624070aa806 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3188,61 +3188,105 @@ describe Project do end describe '.with_feature_available_for_user' do - let!(:user) { create(:user) } - let!(:feature) { MergeRequest } - let!(:project) { create(:project, :public, :merge_requests_enabled) } + let(:user) { create(:user) } + let(:feature) { MergeRequest } subject { described_class.with_feature_available_for_user(feature, user) } - context 'when user has access to project' do - subject { described_class.with_feature_available_for_user(feature, user) } + shared_examples 'feature disabled' do + let(:project) { create(:project, :public, :merge_requests_disabled) } + + it 'does not return projects with the project feature disabled' do + is_expected.not_to include(project) + end + end + + shared_examples 'feature public' do + let(:project) { create(:project, :public, :merge_requests_public) } + + it 'returns projects with the project feature public' do + is_expected.to include(project) + end + end + + shared_examples 'feature enabled' do + let(:project) { create(:project, :public, :merge_requests_enabled) } + + it 'returns projects with the project feature enabled' do + is_expected.to include(project) + end + end + + shared_examples 'feature access level is nil' do + let(:project) { create(:project, :public) } + + it 'returns projects with the project feature access level nil' do + project.project_feature.update(merge_requests_access_level: nil) + + is_expected.to include(project) + end + end + context 'with user' do before do project.add_guest(user) end - context 'when public project' do - context 'when feature is public' do - it 'returns project' do - is_expected.to include(project) + it_behaves_like 'feature disabled' + it_behaves_like 'feature public' + it_behaves_like 'feature enabled' + it_behaves_like 'feature access level is nil' + + context 'when feature is private' do + let(:project) { create(:project, :public, :merge_requests_private) } + + context 'when user does not has access to the feature' do + it 'does not return projects with the project feature private' do + is_expected.not_to include(project) end end - context 'when feature is private' do - let!(:project) { create(:project, :public, :merge_requests_private) } - - it 'returns project when user has access to the feature' do - project.add_maintainer(user) + context 'when user has access to the feature' do + it 'returns projects with the project feature private' do + project.add_reporter(user) is_expected.to include(project) end - - it 'does not return project when user does not have the minimum access level required' do - is_expected.not_to include(project) - end end end + end - context 'when private project' do - let!(:project) { create(:project) } + context 'user is an admin' do + let(:user) { create(:user, :admin) } - it 'returns project when user has access to the feature' do - project.add_maintainer(user) + it_behaves_like 'feature disabled' + it_behaves_like 'feature public' + it_behaves_like 'feature enabled' + it_behaves_like 'feature access level is nil' - is_expected.to include(project) - end + context 'when feature is private' do + let(:project) { create(:project, :public, :merge_requests_private) } - it 'does not return project when user does not have the minimum access level required' do - is_expected.not_to include(project) + it 'returns projects with the project feature private' do + is_expected.to include(project) end end end - context 'when user does not have access to project' do - let!(:project) { create(:project) } + context 'without user' do + let(:user) { nil } - it 'does not return project when user cant access project' do - is_expected.not_to include(project) + it_behaves_like 'feature disabled' + it_behaves_like 'feature public' + it_behaves_like 'feature enabled' + it_behaves_like 'feature access level is nil' + + context 'when feature is private' do + let(:project) { create(:project, :public, :merge_requests_private) } + + it 'does not return projects with the project feature private' do + is_expected.not_to include(project) + end end end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 352ea448c00610b3efa0465da306cea7d4101de4..1b6261aad2bdd498955e467fa9c6a6e8a9029f36 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -504,8 +504,9 @@ describe API::Projects do project4.add_reporter(user2) end - it 'returns an array of groups the user has at least developer access' do + it 'returns an array of projects the user has at least developer access' do get api('/projects', user2), params: { min_access_level: 30 } + expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array diff --git a/spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb b/spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..5f4e178f2e5d95be9cd0ba11f23caeb7f94fa910 --- /dev/null +++ b/spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb @@ -0,0 +1,37 @@ +def get_issue + json_response.is_a?(Array) ? json_response.detect {|issue| issue['id'] == target_issue.id} : json_response +end + +shared_examples 'accessible merge requests count' do + it 'returns anonymous accessible merge requests count' do + get api(api_url), params: { scope: 'all' } + + issue = get_issue + expect(issue).not_to be_nil + expect(issue['merge_requests_count']).to eq(1) + end + + it 'returns guest accessible merge requests count' do + get api(api_url, guest), params: { scope: 'all' } + + issue = get_issue + expect(issue).not_to be_nil + expect(issue['merge_requests_count']).to eq(1) + end + + it 'returns reporter accessible merge requests count' do + get api(api_url, user), params: { scope: 'all' } + + issue = get_issue + expect(issue).not_to be_nil + expect(issue['merge_requests_count']).to eq(2) + end + + it 'returns admin accessible merge requests count' do + get api(api_url, admin), params: { scope: 'all' } + + issue = get_issue + expect(issue).not_to be_nil + expect(issue['merge_requests_count']).to eq(2) + end +end