CORE-753 Make FindOrCreateApplicationGroups and FindOrCreateApplicationUser more resilient to concurrent calls (#1294)

* Attempt to fix some FindOrCreate routines

* Added a bunch of preloading

* Attempt to prevent the id sequence from autoincrementing too much

* Added some early returns for FindOrCreateApplicationGroups

---------

Co-authored-by: Michael Volo <[email protected]>

Author: Dantemss

app/representers/api/v1/application_group_representer.rb

@@ -2,7 +2,7 @@ module Api::V1
   class ApplicationGroupRepresenter < Roar::Decorator
     include Roar::JSON
 
-    property :id, 
+    property :id,
              type: Integer,
              readable: true,
              writeable: false,

app/representers/api/v1/application_groups_representer.rb

@@ -1,9 +1,20 @@
 require 'representable/json/collection'
 
 module Api::V1
-  module ApplicationGroupsRepresenter
+  class ApplicationGroupsRepresenter < Roar::Decorator
     include Representable::JSON::Collection
 
     items class: ApplicationGroup, decorator: ApplicationGroupRepresenter
+
+    def to_hash(options = {})
+      # Avoid N+1 load on application_groups.group
+      ActiveRecord::Associations::Preloader.new.preload represented, group: [
+        { group_members: :user },
+        { group_owners: :user },
+        :member_group_nestings
+      ]
+
+      super(options)
+    end
   end
 end

app/representers/api/v1/application_user_application_representer.rb

@@ -23,12 +23,12 @@ class ApplicationUserApplicationRepresenter < Roar::Decorator
              }
 
     collection :roles,
-      type: String,
-      readable: true,
-      writeable: false,
-      schema_info: {
-        required: true,
-        description: "The User's roles in this application"
-      }
+               type: String,
+               readable: true,
+               writeable: false,
+               schema_info: {
+                 required: true,
+                 description: "The User's roles in this application"
+               }
   end
 end

app/representers/api/v1/application_users_representer.rb

@@ -1,9 +1,18 @@
 require 'representable/json/collection'
 
 module Api::V1
-  module ApplicationUsersRepresenter
+  class ApplicationUsersRepresenter < Roar::Decorator
     include Representable::JSON::Collection
 
     items class: ApplicationUser, decorator: ApplicationUserRepresenter
+
+    def to_hash(options = {})
+      # Avoid N+1 load on application_users.user
+      ActiveRecord::Associations::Preloader.new.preload(
+        represented.to_a, user: { application_users: :application }
+      )
+
+      super(options)
+    end
   end
 end

app/representers/api/v1/group_members_representer.rb

@@ -1,9 +1,22 @@
 require 'representable/json/collection'
 
 module Api::V1
-  module GroupMembersRepresenter
+  class GroupMembersRepresenter < Roar::Decorator
     include Representable::JSON::Collection
 
     items class: GroupMember, decorator: GroupMemberRepresenter
+
+    def to_hash(options = {})
+      # Avoid N+1 load on group_members.group
+      ActiveRecord::Associations::Preloader.new.preload(
+        represented.to_a, group: [
+          { group_members: { user: { application_users: :application } } },
+          { group_owners: { user: { application_users: :application } } },
+          :member_group_nestings
+        ]
+      )
+
+      super(options)
+    end
   end
 end

app/representers/api/v1/group_owners_representer.rb

@@ -1,9 +1,22 @@
 require 'representable/json/collection'
 
 module Api::V1
-  module GroupOwnersRepresenter
+  class GroupOwnersRepresenter < Roar::Decorator
     include Representable::JSON::Collection
 
     items class: GroupOwner, decorator: GroupOwnerRepresenter
+
+    def to_hash(options = {})
+      # Avoid N+1 load on group_members.group
+      ActiveRecord::Associations::Preloader.new.preload(
+        represented.to_a, group: [
+          { group_members: { user: { application_users: :application } } },
+          { group_owners: { user: { application_users: :application } } },
+          :member_group_nestings
+        ]
+      )
+
+      super(options)
+    end
   end
 end

app/representers/api/v1/group_representer.rb

@@ -2,7 +2,7 @@ module Api::V1
   class GroupRepresenter < Roar::Decorator
     include Roar::JSON
 
-    property :id, 
+    property :id,
              type: Integer,
              readable: true,
              writeable: false,
@@ -78,5 +78,17 @@ class GroupRepresenter < Roar::Decorator
                description: "The ID's of all members of groups nested in this group's subtree, including this one; For membership checking purposes"
              }
 
+    def to_hash(options = {})
+      # Avoid N+1 load on group_members.user and group_owners.user
+      ActiveRecord::Associations::Preloader.new.preload(
+        represented.group_members.to_a, user: { application_users: :application }
+      )
+      ActiveRecord::Associations::Preloader.new.preload(
+        represented.group_owners.to_a, user: { application_users: :application }
+      )
+
+      super(options)
+    end
+
   end
 end

app/representers/api/v1/groups_representer.rb

@@ -1,9 +1,22 @@
 require 'representable/json/collection'
 
 module Api::V1
-  module GroupsRepresenter
+  class GroupsRepresenter < Roar::Decorator
     include Representable::JSON::Collection
 
     items class: Group, decorator: GroupRepresenter
+
+    def to_hash(options = {})
+      # Avoid N+1 load on groups
+      ActiveRecord::Associations::Preloader.new.preload(
+        represented.to_a, [
+          { group_members: { user: { application_users: :application } } },
+          { group_owners: { user: { application_users: :application } } },
+          :member_group_nestings
+        ]
+      )
+
+      super(options)
+    end
   end
 end

app/representers/api/v1/user_search_representer.rb

@@ -19,5 +19,12 @@ class UserSearchRepresenter < OpenStax::Api::V1::AbstractSearchRepresenter
                  description: "The Users matching the query or a subset thereof when paginating"
                }
 
+    def to_hash(options = {})
+      # Avoid N+1 load on items
+      ActiveRecord::Associations::Preloader.new.preload represented.items.to_a, application_users: :application
+
+      super(options)
+    end
+
   end
 end

app/routines/find_or_create_application_groups.rb

@@ -1,29 +1,36 @@
 # Finds or creates ApplicationGroups for the given application and group_ids.
 class FindOrCreateApplicationGroups
-
-  lev_routine
+  lev_routine transaction: :read_committed
 
   protected
 
   def exec(application_id, group_ids)
-    application_groups = ApplicationGroup.where(application_id: application_id,
-                                                group_id: group_ids)
-                                         .preload(:group).to_a
-    ag_group_ids = application_groups.collect{|ag| ag.group_id}
+    outputs.application_groups = ApplicationGroup.where(
+      application_id: application_id, group_id: group_ids
+    ).to_a
 
-    # There might be a way to make this more efficient
-    group_ids.each do |group_id|
-      unless ag_group_ids.include?(group_id)
-        application_group = ApplicationGroup.create do |app_group|
-          app_group.application_id = application_id
-          app_group.group_id = group_id
-          app_group.save!
-        end
-        application_groups << application_group
-      end
-    end
+    missing_group_ids = group_ids - outputs.application_groups.map(&:group_id)
+    return if missing_group_ids.empty?
 
-    outputs[:application_groups] = application_groups
-  end
+    # Insert missing records
+    # Returns newly-inserted records only
+    # We could run this as the first query, but it would cause the id sequence to autoincrement
+    # by the number of group_ids every time this routine is called
+    # We can still do that if we switch the primary key to a uuid column
+    new_records = ApplicationGroup.import(
+      missing_group_ids.map do |group_id|
+        ApplicationGroup.new(application_id: application_id, group_id: group_id)
+      end, on_duplicate_key_ignore: true
+    ).results
+    outputs.application_groups += new_records
 
+    existing_group_ids = missing_group_ids - new_records.map(&:group_id)
+    return if existing_group_ids.empty?
+
+    # Run the first query again in case another process or thread
+    # inserted the missing records before us
+    outputs.application_groups += ApplicationGroup.where(
+      application_id: application_id, group_id: existing_group_ids
+    ).to_a
+  end
 end

app/routines/find_or_create_application_user.rb

@@ -1,22 +1,23 @@
 # Finds or creates an ApplicationUser for the given application and user.
 class FindOrCreateApplicationUser
-
-  lev_routine
+  lev_routine transaction: :read_committed
 
   protected
 
   def exec(application_id, user_id)
-    application_user = ApplicationUser.where(:application_id => application_id,
-                                             :user_id => user_id).first
-    unless application_user
-      application_user = ApplicationUser.create do |app_user|
-        app_user.application_id = application_id
-        app_user.user_id = user_id
-        app_user.save!
-      end
-    end
-
-    outputs[:application_user] = application_user
+    # First attempt to query the record
+    # If no record found, attempt to insert it record, ignoring conflicts
+    # If no record is inserted, query the existing record again
+    # The first query is necessary to prevent the id sequence from autoincrementing
+    # every time this routine is called
+    # We can remove it if we switch the primary key to a uuid column
+    outputs.application_user = ApplicationUser.find_by(
+      application_id: application_id, user_id: user_id
+    ) || ApplicationUser.import(
+      [ ApplicationUser.new(application_id: application_id, user_id: user_id) ],
+      on_duplicate_key_ignore: true
+    ).results.first || ApplicationUser.find_by(
+      application_id: application_id, user_id: user_id
+    )
   end
-
-end
+end