Merge pull request #7548 from dependabot/brrygrdn/refactor-dependency…

…-group-engine

[Grouped Updates] Refactor the DependencyGroupEngine into an object, Improved logging for empty groups

common/lib/dependabot/dependency_group.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "wildcard_matcher"
+require "yaml"
 
 module Dependabot
   class DependencyGroup
@@ -13,7 +14,8 @@ def initialize(name:, rules:)
     end
 
     def contains?(dependency)
-      @dependencies.include?(dependency) if @dependencies.any?
+      return true if @dependencies.include?(dependency)
+
       positive_match = rules["patterns"].any? { |rule| WildcardMatcher.match?(rule, dependency.name) }
       negative_match =  rules["exclude-patterns"]&.any? { |rule| WildcardMatcher.match?(rule, dependency.name) }
 
@@ -23,5 +25,12 @@ def contains?(dependency)
     def to_h
       { "name" => name }
     end
+
+    # Provides a debug utility to view the group as it appears in the config file.
+    def to_config_yaml
+      {
+        "groups" => { name => rules }
+      }.to_yaml.delete_prefix("---\n")
+    end
   end
 end

common/spec/dependabot/dependency_group_spec.rb

@@ -27,6 +27,22 @@
   end
 
   let(:test_dependency2) do
+    Dependabot::Dependency.new(
+      name: "test-dependency-2",
+      package_manager: "bundler",
+      version: "1.1.0",
+      requirements: [
+        {
+          file: "Gemfile",
+          requirement: "~> 1.1.0",
+          groups: [],
+          source: nil
+        }
+      ]
+    )
+  end
+
+  let(:another_test_dependency) do
     Dependabot::Dependency.new(
       name: "another-test-dependency",
       package_manager: "bundler",
@@ -74,32 +90,72 @@
   end
 
   describe "#contains?" do
-    context "before dependencies are assigned to the group" do
-      it "returns true if the dependency matches a rule" do
-        expect(dependency_group.dependencies).to eq([])
-        expect(dependency_group.contains?(test_dependency1)).to be_truthy
+    context "when the rules include patterns" do
+      let(:rules) do
+        {
+          "patterns" => ["test-*", "nothing-matches-this"],
+          "exclude-patterns" => ["*-2"]
+        }
       end
 
-      it "returns false if the dependency does not match a rule" do
-        expect(dependency_group.dependencies).to eq([])
-        expect(dependency_group.contains?(test_dependency2)).to be_falsey
-      end
-    end
+      context "before dependencies are assigned to the group" do
+        it "returns true if the dependency matches a pattern" do
+          expect(dependency_group.dependencies).to eq([])
+          expect(dependency_group.contains?(test_dependency1)).to be_truthy
+        end
 
-    context "after dependencies are assigned to the group" do
-      before do
-        dependency_group.dependencies << test_dependency1
-      end
+        it "returns false if the dependency is specifically excluded" do
+          expect(dependency_group.dependencies).to eq([])
+          expect(dependency_group.contains?(test_dependency2)).to be_falsey
+        end
 
-      it "returns true if the dependency is in the dependency list" do
-        expect(dependency_group.dependencies).to include(test_dependency1)
-        expect(dependency_group.contains?(test_dependency1)).to be_truthy
+        it "returns false if the dependency does not match any patterns" do
+          expect(dependency_group.dependencies).to eq([])
+          expect(dependency_group.contains?(another_test_dependency)).to be_falsey
+        end
       end
 
-      it "returns false if the dependency is not in the dependency list and does not match a rule" do
-        expect(dependency_group.dependencies).to include(test_dependency1)
-        expect(dependency_group.contains?(test_dependency2)).to be_falsey
+      context "after dependencies are assigned to the group" do
+        before do
+          dependency_group.dependencies << test_dependency1
+        end
+
+        it "returns true if the dependency is in the dependency list" do
+          expect(dependency_group.dependencies).to include(test_dependency1)
+          expect(dependency_group.contains?(test_dependency1)).to be_truthy
+        end
+
+        it "returns false if the dependency is specifically excluded" do
+          expect(dependency_group.dependencies).to include(test_dependency1)
+          expect(dependency_group.contains?(test_dependency2)).to be_falsey
+        end
+
+        it "returns false if the dependency is not in the dependency list and does not match a pattern" do
+          expect(dependency_group.dependencies).to include(test_dependency1)
+          expect(dependency_group.contains?(another_test_dependency)).to be_falsey
+        end
       end
     end
   end
+
+  describe "#to_config_yaml" do
+    let(:rules) do
+      {
+        "patterns" => ["test-*", "nothing-matches-this"],
+        "exclude-patterns" => ["*-2"]
+      }
+    end
+
+    it "renders the group to match our configuration file" do
+      expect(dependency_group.to_config_yaml).to eql(<<~YAML)
+        groups:
+          test_group:
+            patterns:
+            - test-*
+            - nothing-matches-this
+            exclude-patterns:
+            - "*-2"
+      YAML
+    end
+  end
 end

updater/lib/dependabot/dependency_group_engine.rb

@@ -5,81 +5,80 @@
 # This class implements our strategy for keeping track of and matching dependency
 # groups that are defined by users in their dependabot config file.
 #
-# This is a static class tied to the lifecycle of a Job
-# - Each UpdateJob registers its own DependencyGroupEngine which calculates
-#    the grouped and ungrouped dependencies for a DependencySnapshot
-# - Groups are only calculated once after the Job has registered its dependencies
-# - All allowed dependencies should be passed in to the calculate_dependency_groups! method
+# We instantiate the DependencyGroupEngine after parsing dependencies, configuring
+# any groups from the job's configuration before assigning the dependency list to
+# the groups.
+#
+# We permit dependencies to be in more than one group and also track those which
+# have zero matches so they may be updated individuall.
 #
 # **Note:** This is currently an experimental feature which is not supported
 #           in the service or as an integration point.
 #
 module Dependabot
-  module DependencyGroupEngine
-    @groups_calculated = false
-    @registered_groups = []
-
-    @dependency_groups = {}
-    @ungrouped_dependencies = []
+  class DependencyGroupEngine
+    class ConfigurationError < StandardError; end
 
-    def self.reset!
-      @groups_calculated = false
-      @registered_groups = []
-
-      @dependency_groups = {}
-      @ungrouped_dependencies = []
-    end
+    def self.from_job_config(job:)
+      groups = job.dependency_groups.map do |group|
+        Dependabot::DependencyGroup.new(name: group["name"], rules: group["rules"])
+      end
 
-    # Eventually the key for a dependency group should be a hash since names _can_ conflict within jobs
-    def self.register(name, rules)
-      @registered_groups.push Dependabot::DependencyGroup.new(name: name, rules: rules)
+      new(dependency_groups: groups)
     end
 
-    def self.groups_for(dependency)
-      return [] if dependency.nil?
-      return [] unless dependency.instance_of?(Dependabot::Dependency)
+    attr_reader :dependency_groups, :groups_calculated, :ungrouped_dependencies
 
-      @registered_groups.select do |group|
-        group.contains?(dependency)
-      end
+    def find_group(name:)
+      dependency_groups.find { |group| group.name == name }
     end
 
-    # { group_name => [DependencyGroup], ... }
-    def self.dependency_groups(dependencies)
-      return @dependency_groups if @groups_calculated
-
-      @groups_calculated = calculate_dependency_groups!(dependencies)
+    def assign_to_groups!(dependencies:)
+      raise ConfigurationError, "dependency groups have already been configured!" if @groups_calculated
 
-      @dependency_groups
-    end
+      if dependency_groups.any?
+        dependencies.each do |dependency|
+          matched_groups = @dependency_groups.each_with_object([]) do |group, matches|
+            next unless group.contains?(dependency)
 
-    # Returns a list of dependencies that do not belong to any of the groups
-    def self.ungrouped_dependencies(dependencies)
-      return @ungrouped_dependencies if @groups_calculated
+            group.dependencies.push(dependency)
+            matches << group
+          end
 
-      @groups_calculated = calculate_dependency_groups!(dependencies)
+          # If we had no matches, collect the dependency as ungrouped
+          @ungrouped_dependencies << dependency if matched_groups.empty?
+        end
+      else
+        @ungrouped_dependencies = dependencies
+      end
 
-      @ungrouped_dependencies
+      validate_groups
+      @groups_calculated = true
     end
 
-    def self.calculate_dependency_groups!(dependencies)
-      # If we try to calculate dependency groups when there are no groups registered
-      # then all of the dependencies end up in the ungrouped list which can break
-      # an UpdateAllVersions#dependencies check
-      return false unless @registered_groups.any?
+    private
 
-      dependencies.each do |dependency|
-        groups = groups_for(dependency)
+    def initialize(dependency_groups:)
+      @dependency_groups = dependency_groups
+      @ungrouped_dependencies = []
+      @groups_calculated = false
+    end
 
-        @ungrouped_dependencies << dependency if groups.empty?
+    def validate_groups
+      empty_groups = dependency_groups.select { |group| group.dependencies.empty? }
+      warn_misconfigured_groups(empty_groups) if empty_groups.any?
+    end
 
-        groups.each do |group|
-          group.dependencies.push(dependency)
-          @dependency_groups[group.name.to_sym] = group
-        end
-      end
+    def warn_misconfigured_groups(groups)
+      Dependabot.logger.warn <<~WARN
+        Please check your configuration as there are groups no dependencies match:
+        #{groups.map { |g| "- #{g.name}" }.join("\n")}
 
-      true
+        This can happen if:
+        - the group's 'pattern' rules are mispelled
+        - your configuration's 'allow' rules do not permit any of the dependencies that match the group
+        - the dependencies that match the group rules have been removed from your project
+      WARN
     end
   end
 end

updater/lib/dependabot/dependency_snapshot.rb

@@ -59,22 +59,18 @@ def job_group
       return nil unless job.dependency_group_to_refresh
       return @job_group if defined?(@job_group)
 
-      @job_group = groups.fetch(job.dependency_group_to_refresh.to_sym, nil)
+      @job_group = @dependency_group_engine.find_group(name: job.dependency_group_to_refresh)
     end
 
-    # A dependency snapshot will always have the same set of dependencies since it only depends
-    # on the Job and dependency groups, which are static for a given commit.
     def groups
-      # The DependencyGroupEngine registers dependencies when the Job is created
-      # and it will memoize the dependency groups
-      Dependabot::DependencyGroupEngine.dependency_groups(allowed_dependencies)
+      @dependency_group_engine.dependency_groups
     end
 
     def ungrouped_dependencies
       # If no groups are defined, all dependencies are ungrouped by default.
       return allowed_dependencies unless groups.any?
 
-      Dependabot::DependencyGroupEngine.ungrouped_dependencies(allowed_dependencies)
+      @dependency_group_engine.ungrouped_dependencies
     end
 
     private
@@ -85,6 +81,8 @@ def initialize(job:, base_commit_sha:, dependency_files:)
       @dependency_files = dependency_files
 
       @dependencies = parse_files!
+      @dependency_group_engine = DependencyGroupEngine.from_job_config(job: job)
+      @dependency_group_engine.assign_to_groups!(dependencies: allowed_dependencies)
     end
 
     attr_reader :job

updater/lib/dependabot/job.rb

@@ -81,7 +81,7 @@ def self.standardise_keys(hash)
 
     # NOTE: "attributes" are fetched and injected at run time from
     # dependabot-api using the UpdateJobPrivateSerializer
-    def initialize(attributes) # rubocop:disable Metrics/AbcSize
+    def initialize(attributes)
       @id                             = attributes.fetch(:id)
       @allowed_updates                = attributes.fetch(:allowed_updates)
       @commit_message_options         = attributes.fetch(:commit_message_options, {})
@@ -111,12 +111,16 @@ def initialize(attributes) # rubocop:disable Metrics/AbcSize
       @update_subdependencies         = attributes.fetch(:update_subdependencies)
       @updating_a_pull_request        = attributes.fetch(:updating_a_pull_request)
       @vendor_dependencies            = attributes.fetch(:vendor_dependencies, false)
-      @dependency_groups              = attributes.fetch(:dependency_groups, [])
+      # TODO: Make this hash required
+      #
+      # We will need to do a pass updating the CLI and smoke tests before this is possible,
+      # so let's consider it optional for now. If we get a nil value, let's force it to be
+      # an array.
+      @dependency_groups              = attributes.fetch(:dependency_groups, []) || []
       @dependency_group_to_refresh    = attributes.fetch(:dependency_group_to_refresh, nil)
       @repo_private                   = attributes.fetch(:repo_private, nil)
 
       register_experiments
-      register_dependency_groups
     end
 
     def clone?
@@ -254,14 +258,6 @@ def security_advisories_for(dependency)
       end
     end
 
-    def register_dependency_groups
-      return if dependency_groups.nil?
-
-      dependency_groups.each do |group|
-        Dependabot::DependencyGroupEngine.register(group["name"], group["rules"])
-      end
-    end
-
     def ignore_conditions_for(dependency)
       update_config.ignored_versions_for(
         dependency,

updater/lib/dependabot/updater/group_update_creation.rb

@@ -239,6 +239,20 @@ def peer_dependency_should_update_instead?(dependency_name, dependency_files, up
           end
       end
 
+      def warn_group_is_empty(group)
+        Dependabot.logger.warn(
+          "Skipping update group for '#{group.name}' as it does not match any allowed dependencies."
+        )
+
+        return unless Dependabot.logger.debug?
+
+        Dependabot.logger.debug(<<~DEBUG.chomp)
+          The configuration for this group is:
+
+          #{group.to_config_yaml}
+        DEBUG
+      end
+
       def prepare_workspace
         return unless job.clone? && job.repo_contents_path
 

updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb

@@ -36,6 +36,8 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:, group:)
         end
 
         def perform
+          return warn_group_is_empty(group) if group.dependencies.empty?
+
           Dependabot.logger.info("Starting update group for '#{group.name}'")
 
           dependency_change = compile_all_dependency_changes_for(group)

updater/lib/dependabot/updater/operations/group_update_all_versions.rb

@@ -70,7 +70,7 @@ def run_grouped_dependency_updates
           Dependabot.logger.info("Starting grouped update job for #{job.source.repo}")
           Dependabot.logger.info("Found #{dependency_snapshot.groups.count} group(s).")
 
-          dependency_snapshot.groups.each do |_group_hash, group|
+          dependency_snapshot.groups.each do |group|
             if pr_exists_for_dependency_group?(group)
               Dependabot.logger.info("Detected existing pull request for '#{group.name}'.")
               Dependabot.logger.info(