Add sanitization to BranchNamer::DependencyGroupStrategy

common/lib/dependabot/pull_request_creator/branch_namer/base.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+module Dependabot
+  class PullRequestCreator
+    class BranchNamer
+      class Base
+        attr_reader :dependencies, :files, :target_branch, :separator, :prefix, :max_length
+
+        def initialize(dependencies:, files:, target_branch:, separator: "/",
+                       prefix: "dependabot", max_length: nil)
+          @dependencies  = dependencies
+          @files         = files
+          @target_branch = target_branch
+          @separator     = separator
+          @prefix        = prefix
+          @max_length    = max_length
+        end
+
+        private
+
+        def sanitize_branch_name(ref_name)
+          # General git ref validation
+          sanitized_name = sanitize_ref(ref_name)
+
+          # Some users need branch names without slashes
+          sanitized_name = sanitized_name.gsub("/", separator)
+
+          # Shorten the ref in case users refs have length limits
+          if max_length && (sanitized_name.length > max_length)
+            sha = Digest::SHA1.hexdigest(sanitized_name)[0, max_length]
+            sanitized_name[[max_length - sha.size, 0].max..] = sha
+          end
+
+          sanitized_name
+        end
+
+        def sanitize_ref(ref)
+          # This isn't a complete implementation of git's ref validation, but it
+          # covers most cases that crop up. Its list of allowed characters is a
+          # bit stricter than git's, but that's for cosmetic reasons.
+          ref.
+            # Remove forbidden characters (those not already replaced elsewhere)
+            gsub(%r{[^A-Za-z0-9/\-_.(){}]}, "").
+            # Slashes can't be followed by periods
+            gsub(%r{/\.}, "/dot-").squeeze(".").squeeze("/").
+            # Trailing periods are forbidden
+            sub(/\.$/, "")
+        end
+      end
+    end
+  end
+end

common/lib/dependabot/pull_request_creator/branch_namer/dependency_group_strategy.rb

@@ -1,32 +1,32 @@
 # frozen_string_literal: true
 
+require "dependabot/pull_request_creator/branch_namer/base"
+
 module Dependabot
   class PullRequestCreator
     class BranchNamer
-      class DependencyGroupStrategy
+      class DependencyGroupStrategy < Base
         def initialize(dependencies:, files:, target_branch:, dependency_group:,
                        separator: "/", prefix: "dependabot", max_length: nil)
-          @dependencies     = dependencies
-          @files            = files
-          @target_branch    = target_branch
+          super(
+            dependencies: dependencies,
+            files: files,
+            target_branch: target_branch,
+            separator: separator,
+            prefix: prefix,
+            max_length: max_length
+          )
+
           @dependency_group = dependency_group
-          @separator        = separator
-          @prefix           = prefix
-          @max_length       = max_length
         end
 
-        # FIXME: Incorporate max_length truncation once we allow user config
-        #
-        # For now, we are using a placeholder DependencyGroup with a
-        # fixed-length name, so we can punt on handling truncation until
-        # we determine the strict validation rules for names
         def new_branch_name
-          File.join(prefixes, group_name_with_dependency_digest).gsub("/", separator)
+          sanitize_branch_name(File.join(prefixes, group_name_with_dependency_digest))
         end
 
         private
 
-        attr_reader :dependencies, :dependency_group, :files, :target_branch, :separator, :prefix, :max_length
+        attr_reader :dependency_group
 
         def prefixes
           [

common/lib/dependabot/pull_request_creator/branch_namer/solo_strategy.rb

@@ -3,23 +3,12 @@
 require "digest"
 
 require "dependabot/metadata_finders"
+require "dependabot/pull_request_creator/branch_namer/base"
 
 module Dependabot
   class PullRequestCreator
     class BranchNamer
-      class SoloStrategy
-        attr_reader :dependencies, :files, :target_branch, :separator, :prefix, :max_length
-
-        def initialize(dependencies:, files:, target_branch:, separator: "/",
-                       prefix: "dependabot", max_length: nil)
-          @dependencies  = dependencies
-          @files         = files
-          @target_branch = target_branch
-          @separator     = separator
-          @prefix        = prefix
-          @max_length    = max_length
-        end
-
+      class SoloStrategy < Base
         def new_branch_name
           @name ||=
             begin
@@ -39,16 +28,7 @@ def new_branch_name
               "#{dependency_name_part}-#{branch_version_suffix}"
             end
 
-          # Some users need branch names without slashes
-          sanitized_name = sanitize_ref(File.join(prefixes, @name).gsub("/", separator))
-
-          # Shorten the ref in case users refs have length limits
-          if @max_length && (sanitized_name.length > @max_length)
-            sha = Digest::SHA1.hexdigest(sanitized_name)[0, @max_length]
-            sanitized_name[[@max_length - sha.size, 0].max..] = sha
-          end
-
-          sanitized_name
+          sanitize_branch_name(File.join(prefixes, @name))
         end
 
         private
@@ -189,19 +169,6 @@ def library?
         def requirements_changed?(dependency)
           (dependency.requirements - dependency.previous_requirements).any?
         end
-
-        def sanitize_ref(ref)
-          # This isn't a complete implementation of git's ref validation, but it
-          # covers most cases that crop up. Its list of allowed characters is a
-          # bit stricter than git's, but that's for cosmetic reasons.
-          ref.
-            # Remove forbidden characters (those not already replaced elsewhere)
-            gsub(%r{[^A-Za-z0-9/\-_.(){}]}, "").
-            # Slashes can't be followed by periods
-            gsub(%r{/\.}, "/dot-").squeeze(".").squeeze("/").
-            # Trailing periods are forbidden
-            sub(/\.$/, "")
-        end
       end
     end
   end

common/spec/dependabot/pull_request_creator/branch_namer/dependency_group_strategy_spec.rb

@@ -2,6 +2,7 @@
 
 require "digest"
 
+require "spec_helper"
 require "dependabot/dependency"
 require "dependabot/dependency_file"
 require "dependabot/dependency_group"
@@ -14,7 +15,8 @@
       files: [gemfile],
       target_branch: target_branch,
       separator: separator,
-      dependency_group: dependency_group
+      dependency_group: dependency_group,
+      max_length: max_length
     )
   end
 
@@ -40,8 +42,11 @@
   let(:dependency_group) do
     Dependabot::DependencyGroup.new(name: "my-dependency-group", rules: anything)
   end
+  let(:max_length) { nil }
 
   describe "#new_branch_name" do
+    subject(:new_branch_name) { namer.new_branch_name }
+
     context "with defaults for separator, target branch and files in the root directory" do
       let(:directory) { "/" }
       let(:target_branch) { nil }
@@ -126,6 +131,48 @@
       end
     end
 
+    context "with a maximum length" do
+      let(:directory) { "/" }
+      let(:target_branch) { nil }
+      let(:separator) { "/" }
+
+      context "with a maximum length longer than branch name" do
+        let(:max_length) { 50 }
+
+        it { is_expected.to eq("dependabot/bundler/my-dependency-group-b8d660191d") }
+        its(:length) { is_expected.to eq(49) }
+      end
+
+      context "with a maximum length shorter than branch name" do
+        let(:dependency_group) do
+          Dependabot::DependencyGroup.new(name: "business-and-work-and-desks-and-tables-and-chairs", rules: anything)
+        end
+
+        let(:sha1_digest) { Digest::SHA1.hexdigest("dependabot/bundler/#{dependency_group.name}-b8d660191d") }
+
+        context "with a maximum length longer than sha1 length" do
+          let(:max_length) { 50 }
+
+          it { is_expected.to eq("dependabot#{sha1_digest}") }
+          its(:length) { is_expected.to eq(50) }
+        end
+
+        context "with a maximum length equal than sha1 length" do
+          let(:max_length) { 40 }
+
+          it { is_expected.to eq(sha1_digest) }
+          its(:length) { is_expected.to eq(40) }
+        end
+
+        context "with a maximum length shorter than sha1 length" do
+          let(:max_length) { 20 }
+
+          it { is_expected.to eq(sha1_digest[0...20]) }
+          its(:length) { is_expected.to eq(20) }
+        end
+      end
+    end
+
     context "for files in a non-root directory" do
       let(:directory) { "rails app/" } # let's make sure we deal with spaces too
       let(:target_branch) { nil }
@@ -146,7 +193,7 @@
       end
     end
 
-    context "for files in a non-root directory targetting a branch" do
+    context "for files in a non-root directory targeting a branch" do
       let(:directory) { "rails-app/" }
       let(:target_branch) { "develop" }
       let(:separator) { "_" }