Merge pull request #7452 from TomNaessens/add-sanitization-to-dep-gro…

…up-strat-branch-namer

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) { "_" }