Merge pull request #5873 from dependabot/mctofu/optimize-transitive-update

[npm] Flag indirect transitive updates to be ignored by the FileUpdater

Author: mctofu

common/lib/dependabot/dependency.rb

@@ -115,6 +115,13 @@ def all_versions
       all_versions.filter_map(&:version)
     end
 
+    # This dependency is being indirectly updated by an update to another
+    # dependency. We don't need to try and update it ourselves but want to
+    # surface it to the user in the PR.
+    def informational_only?
+      metadata[:information_only]
+    end
+
     def ==(other)
       other.instance_of?(self.class) && to_h == other.to_h
     end

hex/spec/dependabot/hex/update_checker_spec.rb

@@ -228,7 +228,7 @@
         [{ file: "mix.exs", requirement: "~> 1.2.1", groups: [], source: nil }]
       end
 
-      it { is_expected.to eq(Gem::Version.new("1.3.4")) }
+      it { is_expected.to eq(Gem::Version.new("1.3.5")) }
     end
 
     context "when the user is ignoring the latest version" do

npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb

@@ -190,16 +190,18 @@ def conflicting_updated_dependencies
         end
         # rubocop:enable Metrics/AbcSize
 
-        # We don't need to update this but need to include it so it's described
-        # in the PR and we'll pass validation that this dependency is at a
-        # non-vulnerable version.
+        # We don't need to directly update the target dependency if it will
+        # be updated as a side effect of updating the parent. However, we need
+        # to include it so it's described in the PR and we'll pass validation
+        # that this dependency is at a non-vulnerable version.
         if updated_deps.none? { |dep| dep.name == dependency.name }
           target_version = vulnerability_audit["target_version"]
           updated_deps << build_updated_dependency(
             dependency: dependency,
             version: target_version,
             previous_version: dependency.version,
-            removed: target_version.nil?
+            removed: target_version.nil?,
+            metadata: { information_only: true } # Instruct updater to not directly update this dependency
           )
         end
 
@@ -223,6 +225,7 @@ def build_updated_dependency(update_details)
         removed = update_details.fetch(:removed, false)
         version = update_details.fetch(:version).to_s unless removed
         previous_version = update_details.fetch(:previous_version)&.to_s
+        metadata = update_details.fetch(:metadata, {})
 
         Dependency.new(
           name: original_dep.name,
@@ -236,7 +239,8 @@ def build_updated_dependency(update_details)
           previous_version: previous_version,
           previous_requirements: original_dep.requirements,
           package_manager: original_dep.package_manager,
-          removed: removed
+          removed: removed,
+          metadata: metadata
         )
       end
 

npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker_spec.rb

@@ -1361,6 +1361,22 @@
         )
     end
 
+    # Dependency doesn't consider metadata as part of equality checks
+    # so this allows us to check that the metadata is updated in tests.
+    RSpec::Matchers.define :including_metadata do |expected|
+      match do |actual|
+        actual == expected && actual.metadata == expected.metadata
+      end
+    end
+
+    def contain_exactly_including_metadata(*expected)
+      contain_exactly(*expected.map { |e| including_metadata(e) })
+    end
+
+    def eq_including_metadata(expected_array)
+      eq(expected_array).and contain_exactly_including_metadata(*expected_array)
+    end
+
     context "for a security update for a locked transitive dependency" do
       let(:dependency_files) { project_dependency_files("npm8/locked_transitive_dependency") }
       let(:registry_listing_url) { "https://registry.npmjs.org/locked-transitive-dependency" }
@@ -1385,14 +1401,15 @@
 
       it "correctly updates the transitive dependency" do
         expect(checker.send(:updated_dependencies_after_full_unlock)).
-          to eq([
+          to eq_including_metadata([
             Dependabot::Dependency.new(
               name: "@dependabot-fixtures/npm-transitive-dependency",
               version: "1.0.1",
               package_manager: "npm_and_yarn",
               previous_version: "1.0.0",
               requirements: [],
-              previous_requirements: []
+              previous_requirements: [],
+              metadata: { information_only: true }
             ),
             Dependabot::Dependency.new(
               name: "@dependabot-fixtures/npm-parent-dependency",
@@ -1426,14 +1443,15 @@
         let(:registry_listing_url) { "https://registry.npmjs.org/transitive-dependency-locked-by-intermediate" }
 
         it "correctly updates the transitive dependency" do
-          expect(checker.send(:updated_dependencies_after_full_unlock)).to eq([
+          expect(checker.send(:updated_dependencies_after_full_unlock)).to eq_including_metadata([
             Dependabot::Dependency.new(
               name: "@dependabot-fixtures/npm-transitive-dependency",
               package_manager: "npm_and_yarn",
               previous_requirements: [],
               previous_version: "1.0.0",
               requirements: [],
-              version: "1.0.1"
+              version: "1.0.1",
+              metadata: { information_only: true }
             ),
             Dependabot::Dependency.new(
               name: "@dependabot-fixtures/npm-intermediate-dependency",
@@ -1452,7 +1470,7 @@
         let(:registry_listing_url) { "https://registry.npmjs.org/transitive-dependency-locked-by-multiple" }
 
         it "correctly updates the transitive dependency" do
-          expect(checker.send(:updated_dependencies_after_full_unlock)).to contain_exactly(
+          expect(checker.send(:updated_dependencies_after_full_unlock)).to contain_exactly_including_metadata(
             Dependabot::Dependency.new(
               name: "@dependabot-fixtures/npm-parent-dependency",
               package_manager: "npm_and_yarn",
@@ -1531,7 +1549,8 @@
               previous_requirements: [],
               previous_version: "1.0.0",
               requirements: [],
-              version: "1.0.1"
+              version: "1.0.1",
+              metadata: { information_only: true }
             )
           )
         end
@@ -1547,14 +1566,15 @@
         end
 
         it "correctly updates the parent dependency and removes the transitive because removal is enabled" do
-          expect(checker.send(:updated_dependencies_after_full_unlock)).to contain_exactly(
+          expect(checker.send(:updated_dependencies_after_full_unlock)).to contain_exactly_including_metadata(
             Dependabot::Dependency.new(
               name: "@dependabot-fixtures/npm-transitive-dependency",
               package_manager: "npm_and_yarn",
               previous_requirements: [],
               previous_version: "1.0.0",
               requirements: [],
-              removed: true
+              removed: true,
+              metadata: { information_only: true }
             ),
             Dependabot::Dependency.new(
               name: "@dependabot-fixtures/npm-remove-dependency",
@@ -1608,14 +1628,15 @@
         end
 
         it "correctly updates the transitive dependency by unlocking the parent" do
-          expect(checker.send(:updated_dependencies_after_full_unlock)).to eq([
+          expect(checker.send(:updated_dependencies_after_full_unlock)).to eq_including_metadata([
             Dependabot::Dependency.new(
               name: "@dependabot-fixtures/npm-transitive-dependency-with-more-versions",
               package_manager: "npm_and_yarn",
               previous_requirements: [],
               previous_version: "1.0.0",
               requirements: [],
-              version: "2.0.0"
+              version: "2.0.0",
+              metadata: { information_only: true }
             ),
             Dependabot::Dependency.new(
               name: "@dependabot-fixtures/npm-parent-dependency-with-more-versions",

updater/lib/dependabot/updater.rb

@@ -733,9 +733,10 @@ def generate_dependency_files_for(updated_dependencies)
         logger_info("Updating #{dependency_names.join(', ')}")
       end
 
-      # Removal is only supported for transitive dependencies which are removed as a
-      # side effect of the parent update
-      deps_to_update = updated_dependencies.reject(&:removed?)
+      # Ignore dependencies that are tagged as information_only. These will be
+      # updated indirectly as a result of a parent dependency update and are
+      # only included here to be included in the PR info.
+      deps_to_update = updated_dependencies.reject(&:informational_only?)
       updater = file_updater_for(deps_to_update)
       updater.updated_dependency_files
     end