Optimize SQL queries used in Groups::GroupMembersController#create27374-groups-groupmemberscontroller-create-is-slow-due-to-sql

The following optimizations were performed:

- Add new association to GroupMember and ProjectMember

  This new association will allow us to check if a user is a member of a
  Project or Group through a single query instead of two.

- Optimize retrieving of Members when adding multiple Users

3 files changed, 47 insertions, 17 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index 190b27cf66b..e746e4a12c9 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -16,6 +16,7 @@ class Group < Namespace
     source: :user
 
   has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent
+  has_many :members_and_requesters, as: :source, class_name: 'GroupMember'
 
   has_many :milestones
   has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
diff --git a/app/models/member.rb b/app/models/member.rb
index ee2cb13697b..cbbd58f2eaf 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -126,20 +126,11 @@ class Member < ActiveRecord::Base
       find_by(invite_token: invite_token)
     end
 
-    def add_user(source, user, access_level, current_user: nil, expires_at: nil)
-      user = retrieve_user(user)
+    def add_user(source, user, access_level, existing_members: nil, current_user: nil, expires_at: nil)
+      # `user` can be either a User object, User ID or an email to be invited
+      member = retrieve_member(source, user, existing_members)
       access_level = retrieve_access_level(access_level)
 
-      # `user` can be either a User object or an email to be invited
-      member =
-        if user.is_a?(User)
-          source.members.find_by(user_id: user.id) ||
-            source.requesters.find_by(user_id: user.id) ||
-            source.members.build(user_id: user.id)
-        else
-          source.members.build(invite_email: user)
-        end
-
       return member unless can_update_member?(current_user, member)
 
       member.attributes = {
@@ -165,17 +156,15 @@ class Member < ActiveRecord::Base
     def add_users(source, users, access_level, current_user: nil, expires_at: nil)
       return [] unless users.present?
 
-      # Collect all user ids into separate array
-      # so we can use single sql query to get user objects
-      user_ids = users.select { |user| user =~ /\A\d+\Z/ }
-      users = users - user_ids + User.where(id: user_ids)
+      emails, users, existing_members = parse_users_list(source, users)
 
       self.transaction do
-        users.map do |user|
+        (emails + users).map! do |user|
           add_user(
             source,
             user,
             access_level,
+            existing_members: existing_members,
             current_user: current_user,
             expires_at: expires_at
           )
@@ -189,6 +178,31 @@ class Member < ActiveRecord::Base
 
     private
 
+    def parse_users_list(source, list)
+      emails, user_ids, users = [], [], []
+      existing_members = {}
+
+      list.each do |item|
+        case item
+        when User
+          users << item
+        when Integer
+          user_ids << item
+        when /\A\d+\Z/
+          user_ids << item.to_i
+        when Devise.email_regexp
+          emails << item
+        end
+      end
+
+      if user_ids.present?
+        users.concat(User.where(id: user_ids))
+        existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id)
+      end
+
+      [emails, users, existing_members]
+    end
+
     # This method is used to find users that have been entered into the "Add members" field.
     # These can be the User objects directly, their IDs, their emails, or new emails to be invited.
     def retrieve_user(user)
@@ -197,6 +211,20 @@ class Member < ActiveRecord::Base
       User.find_by(id: user) || User.find_by(email: user) || user
     end
 
+    def retrieve_member(source, user, existing_members)
+      user = retrieve_user(user)
+
+      if user.is_a?(User)
+        if existing_members
+          existing_members[user.id] || source.members.build(user_id: user.id)
+        else
+          source.members_and_requesters.find_or_initialize_by(user_id: user.id)
+        end
+      else
+        source.members.build(invite_email: user)
+      end
+    end
+
     def retrieve_access_level(access_level)
       access_levels.fetch(access_level) { access_level.to_i }
     end
diff --git a/app/models/project.rb b/app/models/project.rb
index 051c4c8e2ec..3d89dabd96f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -144,6 +144,7 @@ class Project < ActiveRecord::Base
 
   has_many :requesters, -> { where.not(requested_at: nil) },
     as: :source, class_name: 'ProjectMember', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+  has_many :members_and_requesters, as: :source, class_name: 'ProjectMember'
 
   has_many :deploy_keys_projects
   has_many :deploy_keys, through: :deploy_keys_projects