Use efficient queries in Tag#delete_old_manifests

Replace object loading and individual destroy calls with pluck and
delete_all to match the Repository version. The old implementation
loaded all manifest objects into memory and called destroy on each
one, which is wasteful for tags with many manifests.

Author: andrew

app/models/tag.rb

@@ -179,12 +179,13 @@ def sync_manifest(m)
   end
 
   def delete_old_manifests(new_manifests)
-    existing_manifests = manifests.map{|m| [m.ecosystem, m.filepath] }
-    to_be_removed = existing_manifests - new_manifests.map{|m| [(m[:platform] || m[:ecosystem]), m[:path]] }
-    to_be_removed.each do |m|
-      manifests.where(ecosystem: m[0], filepath: m[1]).each(&:destroy)
-    end
-    manifests.where.not(id: manifests.latest.map(&:id)).each(&:destroy)
+    new_keys = new_manifests.map { |m| [m[:platform] || m[:ecosystem], m[:path]] }
+    keep_ids = manifests.latest.select { |m| new_keys.include?([m.ecosystem, m.filepath]) }.map(&:id)
+    delete_ids = manifests.where.not(id: keep_ids).pluck(:id)
+    return if delete_ids.empty?
+
+    Dependency.where(manifest_id: delete_ids).delete_all
+    Manifest.where(id: delete_ids).delete_all
   end
 
   def purl

test/models/tag_test.rb

@@ -29,6 +29,48 @@ class TagTest < ActiveSupport::TestCase
     end
   end
 
+  context 'delete_old_manifests' do
+    setup do
+      @host = FactoryBot.create(:github_host)
+      @repository = FactoryBot.create(:repository, host: @host, full_name: 'test/delmanifests', owner: 'test')
+      @tag = FactoryBot.create(:tag, repository: @repository, name: 'v2.0.0', sha: 'def456')
+    end
+
+    should 'delete manifests not present in new list' do
+      old_manifest = Manifest.create!(tag: @tag, ecosystem: 'npm', filepath: 'package.json', kind: 'manifest')
+      keep_manifest = Manifest.create!(tag: @tag, ecosystem: 'rubygems', filepath: 'Gemfile', kind: 'manifest')
+
+      new_manifests = [{ platform: 'rubygems', path: 'Gemfile' }]
+
+      @tag.delete_old_manifests(new_manifests)
+
+      assert_not Manifest.exists?(old_manifest.id)
+      assert Manifest.exists?(keep_manifest.id)
+    end
+
+    should 'delete dependencies belonging to removed manifests' do
+      old_manifest = Manifest.create!(tag: @tag, ecosystem: 'npm', filepath: 'package.json', kind: 'manifest')
+      dep = Dependency.create!(manifest: old_manifest, repository: @repository, ecosystem: 'npm', package_name: 'lodash', requirements: '^4.0', kind: 'runtime')
+
+      new_manifests = []
+
+      @tag.delete_old_manifests(new_manifests)
+
+      assert_not Manifest.exists?(old_manifest.id)
+      assert_not Dependency.exists?(dep.id)
+    end
+
+    should 'do nothing when all manifests match' do
+      keep = Manifest.create!(tag: @tag, ecosystem: 'npm', filepath: 'package.json', kind: 'manifest')
+
+      new_manifests = [{ platform: 'npm', path: 'package.json' }]
+
+      @tag.delete_old_manifests(new_manifests)
+
+      assert Manifest.exists?(keep.id)
+    end
+  end
+
   context 'parse_dependencies method' do
     setup do
       @host = FactoryBot.create(:github_host)