Fixes #39421 - Speed up host collection membership updates (#11781)

pablomh · cursoragent · web-flow · commit cd3642b1f201 · 2026-06-16T08:39:48.000-04:00
Replace full host collection membership reassignment with delta-based add/remove operations so updates scale with the requested hosts instead of the full collection size. Reuse the same model API from both direct and bulk endpoints, enforce join-row uniqueness, batch the join-table writes for changed hosts, and cover the new behavior with focused regression tests.

Co-authored-by: Cursor &lt;cursoragent@cursor.com&gt;
diff --git a/app/controllers/katello/api/v2/host_collections_controller.rb b/app/controllers/katello/api/v2/host_collections_controller.rb
@@ -93,23 +93,20 @@ def update
     param :id, :number, :desc => N_("Id of the host collection"), :required => true
     param :host_ids, Array, :desc => N_("Array of host ids")
     def add_hosts
-      host_ids = params[:host_ids].map(&:to_i)
+      host_ids, found_host_ids, editable_host_ids = requested_host_membership_ids
 
-      @hosts = ::Host::Managed.where(id: host_ids)
-      @editable_hosts = @hosts.authorized(:edit_hosts)
-
-      already_added_host_ids = @host_collection.host_ids & host_ids
-      unfound_host_ids = host_ids - @hosts.pluck(:id)
-
-      @host_collection.host_ids = (@host_collection.host_ids +
-                                   @editable_hosts.pluck(:id)).uniq
-      @host_collection.save!
+      membership_update = @host_collection.add_host_ids!(
+        :requested_host_ids => found_host_ids,
+        :authorized_host_ids => editable_host_ids
+      )
+      already_added_host_ids = membership_update[:requested_existing_host_ids]
+      unfound_host_ids = host_ids - found_host_ids
 
       messages = format_bulk_action_messages(
           :success    => _("Successfully added %s Host(s)."),
           :error      => _("You were not allowed to add %s"),
-          :models     => @hosts.pluck(:id) - already_added_host_ids,
-          :authorized => @editable_hosts.pluck(:id) - already_added_host_ids
+          :models     => found_host_ids - already_added_host_ids,
+          :authorized => membership_update[:updated_host_ids]
       )
 
       already_added_host_ids.each do |host_id|
@@ -128,22 +125,20 @@ def add_hosts
     param :id, :number, :desc => N_("Id of the host collection"), :required => true
     param :host_ids, Array, :desc => N_("Array of host ids")
     def remove_hosts
-      host_ids = params[:host_ids].map(&:to_i)
-
-      @hosts = ::Host::Managed.where(id: host_ids)
-      @editable_hosts = @hosts.authorized(:edit_hosts)
-
-      already_removed_host_ids = @hosts.pluck(:id) - @host_collection.host_ids
-      unfound_host_ids = host_ids - @hosts.pluck(:id)
+      host_ids, found_host_ids, editable_host_ids = requested_host_membership_ids
 
-      @host_collection.host_ids -= @editable_hosts.pluck(:id)
-      @host_collection.save!
+      membership_update = @host_collection.remove_host_ids!(
+        :requested_host_ids => found_host_ids,
+        :authorized_host_ids => editable_host_ids
+      )
+      already_removed_host_ids = found_host_ids - membership_update[:requested_existing_host_ids]
+      unfound_host_ids = host_ids - found_host_ids
 
       messages = format_bulk_action_messages(
           :success    => _("Successfully removed %s Host(s)."),
           :error      => _("You were not allowed to sync %s"),
-          :models     => @hosts.pluck(:id) - already_removed_host_ids,
-          :authorized => @editable_hosts.pluck(:id) - already_removed_host_ids
+          :models     => found_host_ids - already_removed_host_ids,
+          :authorized => membership_update[:updated_host_ids]
       )
 
       already_removed_host_ids.each do |host_id|
@@ -202,6 +197,13 @@ def host_collection_params_with_host_ids
       result
     end
 
+    def requested_host_membership_ids
+      host_ids = Array(params[:host_ids]).map(&:to_i)
+      @hosts = ::Host::Managed.where(id: host_ids)
+      @editable_hosts = @hosts.authorized(:edit_hosts)
+      [host_ids, @hosts.pluck(:id), @editable_hosts.pluck(:id)]
+    end
+
     def find_editable_host
       @host = resource_finder(::Host::Managed.authorized("edit_hosts"), params[:host_id]) if params[:host_id]
       @organization = @host.organization if @host
diff --git a/app/controllers/katello/api/v2/hosts_bulk_actions_controller.rb b/app/controllers/katello/api/v2/hosts_bulk_actions_controller.rb
@@ -44,13 +44,14 @@ class Api::V2::HostsBulkActionsController < Api::V2::ApiController
     def bulk_add_host_collections
       unless params[:host_collection_ids].blank?
         display_messages = []
+        host_ids = @hosts.map(&:id)
 
         @host_collections.each do |host_collection|
-          pre_host_collection_count = host_collection.host_ids.count
-          host_collection.host_ids =  (host_collection.host_ids + @hosts.map(&:id)).uniq
-          host_collection.save!
-
-          final_count = host_collection.host_ids.count - pre_host_collection_count
+          membership_update = host_collection.add_host_ids!(
+            :requested_host_ids => host_ids,
+            :authorized_host_ids => host_ids
+          )
+          final_count = membership_update[:updated_host_ids].length
           msg = if final_count == 0
                   _("All selected hosts were already members of host collection %{host_collection}.") %
                               {:host_collection => host_collection.name }
@@ -74,12 +75,13 @@ def bulk_remove_host_collections
       display_messages = []
 
       unless params[:host_collection_ids].blank?
+        host_ids = @hosts.map(&:id)
         @host_collections.each do |host_collection|
-          pre_host_collection_count = host_collection.host_ids.count
-          host_collection.host_ids =  (host_collection.host_ids - @hosts.map(&:id)).uniq
-          host_collection.save!
-
-          final_count = pre_host_collection_count - host_collection.host_ids.count
+          membership_update = host_collection.remove_host_ids!(
+            :requested_host_ids => host_ids,
+            :authorized_host_ids => host_ids
+          )
+          final_count = membership_update[:updated_host_ids].length
           display_messages << _("Removed %{count} host(s) from host collection %{host_collection}.") %
               {:count => final_count, :host_collection => host_collection.name }
         end
diff --git a/app/models/katello/host_collection.rb b/app/models/katello/host_collection.rb
@@ -32,15 +32,14 @@ def max_hosts_check
       # NOTE: max_hosts_check and max_hosts_no_exceeded use size() instead of count() because
       # the host list exists as an array rather than a DB query when run as a validation.
       host_count = hosts.size
-      if !unlimited_hosts && (host_count > 0 && (host_count.to_i > max_hosts.to_i)) && max_hosts_changed?
+      if !unlimited_hosts && host_count > 0 && (host_count.to_i > max_hosts.to_i) && max_hosts_changed?
         errors.add :max_host, N_("may not be less than the number of hosts associated with the host collection.")
       end
     end
 
     def max_hosts_not_exceeded
       if !unlimited_hosts && (hosts.size.to_i > max_hosts.to_i)
-        errors.add :base, N_("You cannot have more than #{max_hosts} host(s) associated with host collection #{name}." %
-                              {:max_hosts => max_hosts, :name => name})
+        errors.add :base, max_hosts_exceeded_message
       end
     end
 
@@ -77,6 +76,36 @@ def total_hosts(cached: false)
       end
     end
 
+    def add_host_ids!(requested_host_ids:, authorized_host_ids:)
+      update_host_membership(requested_host_ids, authorized_host_ids) do |existing_host_ids, allowed_host_ids|
+        host_ids_to_add = allowed_host_ids - existing_host_ids
+        ensure_capacity_for!(host_ids_to_add.length)
+        create_host_collection_hosts(host_ids_to_add)
+
+        {
+          :updated_host_ids => host_ids_to_add,
+          :requested_existing_host_ids => existing_host_ids,
+        }
+      end
+    end
+
+    def remove_host_ids!(requested_host_ids:, authorized_host_ids:)
+      update_host_membership(requested_host_ids, authorized_host_ids) do |existing_host_ids, allowed_host_ids|
+        host_ids_to_remove = existing_host_ids & allowed_host_ids
+        destroy_host_collection_hosts(host_ids_to_remove)
+
+        {
+          :updated_host_ids => host_ids_to_remove,
+          :requested_existing_host_ids => existing_host_ids,
+        }
+      end
+    end
+
+    def max_hosts_exceeded_message
+      _("You cannot have more than %{max_hosts} host(s) associated with host collection %{host_collection}.") %
+        { :max_hosts => max_hosts, :host_collection => name }
+    end
+
     # Retrieve the list of accessible host collections in the organization specified, returning
     # them in the following arrays:
     #   critical: those collections that have 1 or more security errata that need to be applied
@@ -115,6 +144,73 @@ def self.humanize_class_name(_name = nil)
       _("Host Collections")
     end
 
+    private
+
+    def update_host_membership(requested_host_ids, authorized_host_ids)
+      normalized_requested_host_ids = normalize_host_ids(requested_host_ids)
+      normalized_authorized_host_ids = normalize_host_ids(authorized_host_ids)
+      allowed_host_ids = normalized_requested_host_ids & normalized_authorized_host_ids
+
+      return empty_membership_result if normalized_requested_host_ids.empty?
+
+      with_lock do
+        existing_host_ids = host_collection_hosts.where(:host_id => normalized_requested_host_ids).pluck(:host_id)
+        result = yield(existing_host_ids, allowed_host_ids)
+
+        clear_membership_cache if result[:updated_host_ids].any?
+
+        result
+      end
+    end
+
+    def empty_membership_result
+      {
+        :updated_host_ids => [],
+        :requested_existing_host_ids => [],
+      }
+    end
+
+    def normalize_host_ids(host_ids)
+      Array(host_ids).map(&:to_i).uniq
+    end
+
+    def ensure_capacity_for!(additional_host_count)
+      return if additional_host_count.zero? || unlimited_hosts
+
+      if host_collection_hosts.count + additional_host_count > max_hosts
+        errors.add(:base, max_hosts_exceeded_message)
+        fail ActiveRecord::RecordInvalid, self
+      end
+    end
+
+    def create_host_collection_hosts(host_ids)
+      return if host_ids.empty?
+
+      timestamp = Time.current
+      HostCollectionHosts.insert_all(
+        host_ids.map do |host_id|
+          {
+            :host_collection_id => id,
+            :host_id => host_id,
+            :created_at => timestamp,
+            :updated_at => timestamp,
+          }
+        end
+      )
+    end
+
+    def destroy_host_collection_hosts(host_ids)
+      return if host_ids.empty?
+
+      host_collection_hosts.where(:host_id => host_ids).delete_all
+    end
+
+    def clear_membership_cache
+      association(:host_collection_hosts).reset
+      association(:hosts).reset
+      Rails.cache.delete("#{cache_key}/total_hosts")
+    end
+
     apipie :class, desc: "A class representing #{model_name.human} object" do
       name 'Host Collection'
       refs 'HostCollection'
diff --git a/app/models/katello/host_collection_hosts.rb b/app/models/katello/host_collection_hosts.rb
@@ -9,9 +9,7 @@ def validate_max_hosts_not_exceeded
       if new_record? && self.host_collection_id
         host_collection = HostCollection.find(self.host_collection_id)
         if host_collection && !host_collection.unlimited_hosts && (host_collection.hosts.size >= host_collection.max_hosts)
-          errors.add :base,
-                     _("You cannot have more than %{max_hosts} host(s) associated with host collection '%{host_collection}'.") %
-                         { :max_hosts => host_collection.max_hosts, :host_collection => host_collection.name }
+          errors.add(:base, host_collection.max_hosts_exceeded_message)
         end
       end
     end
diff --git a/db/migrate/20260612120500_add_unique_index_to_katello_host_collection_hosts.rb b/db/migrate/20260612120500_add_unique_index_to_katello_host_collection_hosts.rb
@@ -0,0 +1,32 @@
+class AddUniqueIndexToKatelloHostCollectionHosts < ActiveRecord::Migration[6.1]
+  def up
+    remove_duplicate_host_collection_hosts
+
+    add_index :katello_host_collection_hosts,
+              [:host_collection_id, :host_id],
+              :unique => true,
+              :name => :index_katello_host_collection_hosts_on_collection_and_host
+  end
+
+  def down
+    remove_index :katello_host_collection_hosts,
+                 :name => :index_katello_host_collection_hosts_on_collection_and_host
+  end
+
+  private
+
+  def remove_duplicate_host_collection_hosts
+    execute(<<~SQL.squish)
+      DELETE FROM katello_host_collection_hosts
+      WHERE id IN (
+        SELECT id
+        FROM (
+          SELECT id,
+                 ROW_NUMBER() OVER (PARTITION BY host_collection_id, host_id ORDER BY id) AS duplicate_rank
+          FROM katello_host_collection_hosts
+        ) duplicate_rows
+        WHERE duplicate_rank > 1
+      )
+    SQL
+  end
+end
diff --git a/test/controllers/api/v2/hosts_bulk_actions_controller_test.rb b/test/controllers/api/v2/hosts_bulk_actions_controller_test.rb
@@ -53,15 +53,21 @@ def test_add_host_collection
       put :bulk_add_host_collections, params: { :included => {:ids => @host_ids}, :organization_id => @org.id, :host_collection_ids => [@host_collection1.id, @host_collection2.id] }
 
       assert_response :success
+      results = JSON.parse(response.body)
       assert_equal 2, @host1.host_collections.count
+      assert_includes results['displayMessages'], "Added 1 host(s) to host collection #{@host_collection1.name}."
+      assert_includes results['displayMessages'], "Added 2 host(s) to host collection #{@host_collection2.name}."
     end
 
     def test_remove_host_collection
       assert_equal 1, @host1.host_collections.count # system initially has simple_host_collection
       put :bulk_remove_host_collections, params: { :included => {:ids => @host_ids}, :organization_id => @org.id, :host_collection_ids => [@host_collection1.id, @host_collection2.id] }
 
       assert_response :success
+      results = JSON.parse(response.body)
       assert_equal 0, @host1.host_collections.count
+      assert_includes results['displayMessages'], "Removed 1 host(s) from host collection #{@host_collection1.name}."
+      assert_includes results['displayMessages'], "Removed 0 host(s) from host collection #{@host_collection2.name}."
     end
 
     def test_destroy_hosts
diff --git a/test/models/host_collection_test.rb b/test/models/host_collection_test.rb
@@ -147,6 +147,56 @@ def test_negative_create_with_invalid_name
       end
     end
 
+    def test_add_host_ids_adds_only_missing_hosts
+      result = @simple_collection.add_host_ids!(
+        :requested_host_ids => [@host_one.id, @host_two.id],
+        :authorized_host_ids => [@host_one.id, @host_two.id]
+      )
+
+      assert_equal [@host_one.id], result[:requested_existing_host_ids]
+      assert_equal [@host_two.id], result[:updated_host_ids]
+      assert_equal [@host_one.id, @host_two.id].sort, @simple_collection.reload.host_ids.sort
+    end
+
+    def test_remove_host_ids_removes_only_existing_hosts
+      result = @simple_collection.remove_host_ids!(
+        :requested_host_ids => [@host_one.id, @host_two.id],
+        :authorized_host_ids => [@host_one.id, @host_two.id]
+      )
+
+      assert_equal [@host_one.id], result[:requested_existing_host_ids]
+      assert_equal [@host_one.id], result[:updated_host_ids]
+      assert_empty @simple_collection.reload.host_ids
+    end
+
+    def test_add_host_ids_only_updates_requested_authorized_hosts
+      result = @simple_collection.add_host_ids!(
+        :requested_host_ids => [@host_two.id],
+        :authorized_host_ids => [@host_one.id]
+      )
+
+      assert_empty result[:updated_host_ids]
+      assert_equal [@host_one.id], @simple_collection.reload.host_ids
+    end
+
+    def test_add_host_ids_honors_max_hosts
+      limited_collection = katello_host_collections(:one_host_limit_host_collection)
+      limited_collection.add_host_ids!(
+        :requested_host_ids => [@host_one.id],
+        :authorized_host_ids => [@host_one.id]
+      )
+
+      error = assert_raises(ActiveRecord::RecordInvalid) do
+        limited_collection.add_host_ids!(
+          :requested_host_ids => [@host_two.id],
+          :authorized_host_ids => [@host_two.id]
+        )
+      end
+
+      assert_includes error.record.errors[:base], limited_collection.max_hosts_exceeded_message
+      assert_equal [@host_one.id], limited_collection.reload.host_ids
+    end
+
     def test_audit_on_host_collection_creation
       new_host_collection = HostCollection.new(
         :name => "Test Audit Host Collection ",
