Merge branch 'rework-authorizations-performance' into 'master'

Rework project authorizations and nested groups for better performance

See merge request !10885

9 files changed, 51 insertions, 157 deletions

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 965ced4d372..ebbcc10dd35 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -64,6 +64,8 @@ class GroupsController < Groups::ApplicationController
   end
 
   def subgroups
+    return not_found unless Group.supports_nested_groups?
+
     @nested_groups = GroupsFinder.new(current_user, parent: group).execute
     @nested_groups = @nested_groups.search(params[:filter_groups]) if params[:filter_groups].present?
   end
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index c4463abdfe6..63d02b76f6b 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -84,89 +84,6 @@ module Routable
         joins(:route).where(wheres.join(' OR '))
       end
     end
-
-    # Builds a relation to find multiple objects that are nested under user membership
-    #
-    # Usage:
-    #
-    #     Klass.member_descendants(1)
-    #
-    # Returns an ActiveRecord::Relation.
-    def member_descendants(user_id)
-      joins(:route).
-        joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(r2.path, '/%')
-               INNER JOIN members ON members.source_id = r2.source_id
-               AND members.source_type = r2.source_type").
-        where('members.user_id = ?', user_id)
-    end
-
-    # Builds a relation to find multiple objects that are nested under user
-    # membership. Includes the parent, as opposed to `#member_descendants`
-    # which only includes the descendants.
-    #
-    # Usage:
-    #
-    #     Klass.member_self_and_descendants(1)
-    #
-    # Returns an ActiveRecord::Relation.
-    def member_self_and_descendants(user_id)
-      joins(:route).
-        joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(r2.path, '/%')
-               OR routes.path = r2.path
-               INNER JOIN members ON members.source_id = r2.source_id
-               AND members.source_type = r2.source_type").
-        where('members.user_id = ?', user_id)
-    end
-
-    # Returns all objects in a hierarchy, where any node in the hierarchy is
-    # under the user membership.
-    #
-    # Usage:
-    #
-    #     Klass.member_hierarchy(1)
-    #
-    # Examples:
-    #
-    #     Given the following group tree...
-    #
-    #            _______group_1_______
-    #           |                     |
-    #           |                     |
-    #     nested_group_1        nested_group_2
-    #           |                     |
-    #           |                     |
-    #     nested_group_1_1      nested_group_2_1
-    #
-    #
-    #     ... the following results are returned:
-    #
-    #     * the user is a member of group 1
-    #       => 'group_1',
-    #          'nested_group_1', nested_group_1_1',
-    #          'nested_group_2', 'nested_group_2_1'
-    #
-    #     * the user is a member of nested_group_2
-    #       => 'group1',
-    #          'nested_group_2', 'nested_group_2_1'
-    #
-    #     * the user is a member of nested_group_2_1
-    #       => 'group1',
-    #          'nested_group_2', 'nested_group_2_1'
-    #
-    # Returns an ActiveRecord::Relation.
-    def member_hierarchy(user_id)
-      paths = member_self_and_descendants(user_id).pluck('routes.path')
-
-      return none if paths.empty?
-
-      wheres = paths.map do |path|
-        "#{connection.quote(path)} = routes.path
-         OR
-         #{connection.quote(path)} LIKE CONCAT(routes.path, '/%')"
-      end
-
-      joins(:route).where(wheres.join(' OR '))
-    end
   end
 
   def full_name
diff --git a/app/models/concerns/select_for_project_authorization.rb b/app/models/concerns/select_for_project_authorization.rb
index 50a1d7fc3e1..58194b0ea13 100644
--- a/app/models/concerns/select_for_project_authorization.rb
+++ b/app/models/concerns/select_for_project_authorization.rb
@@ -3,7 +3,11 @@ module SelectForProjectAuthorization
 
   module ClassMethods
     def select_for_project_authorization
-      select("members.user_id, projects.id AS project_id, members.access_level")
+      select("projects.id AS project_id, members.access_level")
+    end
+
+    def select_as_master_for_project_authorization
+      select(["projects.id AS project_id", "#{Gitlab::Access::MASTER} AS access_level"])
     end
   end
 end
diff --git a/app/models/group.rb b/app/models/group.rb
index cbc10b00cf5..fac5843f75c 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -37,6 +37,10 @@ class Group < Namespace
   after_save :update_two_factor_requirement
 
   class << self
+    def supports_nested_groups?
+      Gitlab::Database.postgresql?
+    end
+
     # Searches for groups matching the given query.
     #
     # This method uses ILIKE on PostgreSQL and LIKE on MySQL.
@@ -77,7 +81,7 @@ class Group < Namespace
       if current_scope.joins_values.include?(:shared_projects)
         joins('INNER JOIN namespaces project_namespace ON project_namespace.id = projects.namespace_id')
           .where('project_namespace.share_with_group_lock = ?',  false)
-          .select("members.user_id, projects.id AS project_id, LEAST(project_group_links.group_access, members.access_level) AS access_level")
+          .select("projects.id AS project_id, LEAST(project_group_links.group_access, members.access_level) AS access_level")
       else
         super
       end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index d6b0ab0e52c..9bdfab9a066 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -176,26 +176,20 @@ class Namespace < ActiveRecord::Base
     projects.with_shared_runners.any?
   end
 
-  # Scopes the model on ancestors of the record
+  # Returns all the ancestors of the current namespaces.
   def ancestors
-    if parent_id
-      path = route ? route.path : full_path
-      paths = []
+    return self.class.none unless parent_id
 
-      until path.blank?
-        path = path.rpartition('/').first
-        paths << path
-      end
-
-      self.class.joins(:route).where('routes.path IN (?)', paths).reorder('routes.path ASC')
-    else
-      self.class.none
-    end
+    Gitlab::GroupHierarchy.
+      new(self.class.where(id: parent_id)).
+      base_and_ancestors
   end
 
-  # Scopes the model on direct and indirect children of the record
+  # Returns all the descendants of the current namespace.
   def descendants
-    self.class.joins(:route).merge(Route.inside_path(route.path)).reorder('routes.path ASC')
+    Gitlab::GroupHierarchy.
+      new(self.class.where(parent_id: id)).
+      base_and_descendants
   end
 
   def user_ids_for_project_authorizations
diff --git a/app/models/project_authorization.rb b/app/models/project_authorization.rb
index 4c7f4f5a429..def09675253 100644
--- a/app/models/project_authorization.rb
+++ b/app/models/project_authorization.rb
@@ -6,6 +6,12 @@ class ProjectAuthorization < ActiveRecord::Base
   validates :access_level, inclusion: { in: Gitlab::Access.all_values }, presence: true
   validates :user, uniqueness: { scope: [:project, :access_level] }, presence: true
 
+  def self.select_from_union(union)
+    select(['project_id', 'MAX(access_level) AS access_level']).
+      from("(#{union.to_sql}) #{ProjectAuthorization.table_name}").
+      group(:project_id)
+  end
+
   def self.insert_authorizations(rows, per_batch = 1000)
     rows.each_slice(per_batch) do |slice|
       tuples = slice.map do |tuple|
diff --git a/app/models/user.rb b/app/models/user.rb
index ad24ec0ec08..efd53810ec3 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -9,9 +9,12 @@ class User < ActiveRecord::Base
   include Sortable
   include CaseSensitivity
   include TokenAuthenticatable
+  include IgnorableColumn
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
+  ignore_column :authorized_projects_populated
+
   add_authentication_token_field :authentication_token
   add_authentication_token_field :incoming_email_token
 
@@ -200,7 +203,6 @@ class User < ActiveRecord::Base
   scope :blocked, -> { with_states(:blocked, :ldap_blocked) }
   scope :external, -> { where(external: true) }
   scope :active, -> { with_state(:active).non_internal }
-  scope :not_in_project, ->(project) { project.users.present? ? where("id not in (:ids)", ids: project.users.map(&:id) ) : all }
   scope :without_projects, -> { where('id NOT IN (SELECT DISTINCT(user_id) FROM members WHERE user_id IS NOT NULL AND requested_at IS NULL)') }
   scope :todo_authors, ->(user_id, state) { where(id: Todo.where(user_id: user_id, state: state).select(:author_id)) }
   scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('last_sign_in_at', 'DESC')) }
@@ -492,23 +494,16 @@ class User < ActiveRecord::Base
     Group.where("namespaces.id IN (#{union.to_sql})")
   end
 
-  def nested_groups
-    Group.member_descendants(id)
-  end
-
+  # Returns a relation of groups the user has access to, including their parent
+  # and child groups (recursively).
   def all_expanded_groups
-    Group.member_hierarchy(id)
+    Gitlab::GroupHierarchy.new(groups).all_groups
   end
 
   def expanded_groups_requiring_two_factor_authentication
     all_expanded_groups.where(require_two_factor_authentication: true)
   end
 
-  def nested_groups_projects
-    Project.joins(:namespace).where('namespaces.parent_id IS NOT NULL').
-      member_descendants(id)
-  end
-
   def refresh_authorized_projects
     Users::RefreshAuthorizedProjectsService.new(self).execute
   end
@@ -517,18 +512,15 @@ class User < ActiveRecord::Base
     project_authorizations.where(project_id: project_ids).delete_all
   end
 
-  def set_authorized_projects_column
-    unless authorized_projects_populated
-      update_column(:authorized_projects_populated, true)
-    end
-  end
-
   def authorized_projects(min_access_level = nil)
-    refresh_authorized_projects unless authorized_projects_populated
-
-    # We're overriding an association, so explicitly call super with no arguments or it would be passed as `force_reload` to the association
+    # We're overriding an association, so explicitly call super with no
+    # arguments or it would be passed as `force_reload` to the association
     projects = super()
-    projects = projects.where('project_authorizations.access_level >= ?', min_access_level) if min_access_level
+
+    if min_access_level
+      projects = projects.
+        where('project_authorizations.access_level >= ?', min_access_level)
+    end
 
     projects
   end
diff --git a/app/services/users/refresh_authorized_projects_service.rb b/app/services/users/refresh_authorized_projects_service.rb
index 8f6f5b937c4..3e07b811027 100644
--- a/app/services/users/refresh_authorized_projects_service.rb
+++ b/app/services/users/refresh_authorized_projects_service.rb
@@ -73,12 +73,11 @@ module Users
     # remove - The IDs of the authorization rows to remove.
     # add - Rows to insert in the form `[user id, project id, access level]`
     def update_authorizations(remove = [], add = [])
-      return if remove.empty? && add.empty? && user.authorized_projects_populated
+      return if remove.empty? && add.empty?
 
       User.transaction do
         user.remove_project_authorizations(remove) unless remove.empty?
         ProjectAuthorization.insert_authorizations(add) unless add.empty?
-        user.set_authorized_projects_column
       end
 
       # Since we batch insert authorization rows, Rails' associations may get
@@ -101,38 +100,13 @@ module Users
     end
 
     def fresh_authorizations
-      ProjectAuthorization.
-        unscoped.
-        select('project_id, MAX(access_level) AS access_level').
-        from("(#{project_authorizations_union.to_sql}) #{ProjectAuthorization.table_name}").
-        group(:project_id)
-    end
-
-    private
-
-    # Returns a union query of projects that the user is authorized to access
-    def project_authorizations_union
-      relations = [
-        # Personal projects
-        user.personal_projects.select("#{user.id} AS user_id, projects.id AS project_id, #{Gitlab::Access::MASTER} AS access_level"),
-
-        # Projects the user is a member of
-        user.projects.select_for_project_authorization,
-
-        # Projects of groups the user is a member of
-        user.groups_projects.select_for_project_authorization,
-
-        # Projects of subgroups of groups the user is a member of
-        user.nested_groups_projects.select_for_project_authorization,
-
-        # Projects shared with groups the user is a member of
-        user.groups.joins(:shared_projects).select_for_project_authorization,
-
-        # Projects shared with subgroups of groups the user is a member of
-        user.nested_groups.joins(:shared_projects).select_for_project_authorization
-      ]
+      klass = if Group.supports_nested_groups?
+                Gitlab::ProjectAuthorizations::WithNestedGroups
+              else
+                Gitlab::ProjectAuthorizations::WithoutNestedGroups
+              end
 
-      Gitlab::SQL::Union.new(relations)
+      klass.new(user).calculate
     end
   end
 end
diff --git a/app/views/groups/_show_nav.html.haml b/app/views/groups/_show_nav.html.haml
index b2097e88741..35b75bc0923 100644
--- a/app/views/groups/_show_nav.html.haml
+++ b/app/views/groups/_show_nav.html.haml
@@ -2,6 +2,7 @@
   = nav_link(page: group_path(@group)) do
     = link_to group_path(@group) do
       Projects
-  = nav_link(page: subgroups_group_path(@group)) do
-    = link_to subgroups_group_path(@group) do
-      Subgroups
+  - if Group.supports_nested_groups?
+    = nav_link(page: subgroups_group_path(@group)) do
+      = link_to subgroups_group_path(@group) do
+        Subgroups