Add option to control PR description max length + set reasonable defa…

…ults for each platform (#7487) Add option to set the max character length of the PR description. Any descriptions longer than the max will get truncated with a clear message to the user that the remaining description was truncated. This is mostly a refactor / moving of existing code that had been copy/pasted across several PR creators so that we have a clean generic version that can re-used everywhere. In addition to being configurable, this also sets reasonable defaults for some of the platforms we support. --------- Co-authored-by: Dennis Carey <[email protected]> Co-authored-by: Jeff Widman <[email protected]>
dependabot · Jul 25, 2023 · ce32701 · ce32701
1 parent e4afdd4
commit ce32701
Show file tree

Hide file tree

Showing 9 changed files with 139 additions and 84 deletions.
diff --git a/common/lib/dependabot/clients/azure.rb b/common/lib/dependabot/clients/azure.rb
@@ -22,8 +22,6 @@ class TagsCreationForbidden < StandardError; end
 
       RETRYABLE_ERRORS = [InternalServerError, BadGateway, ServiceNotAvailable].freeze
 
-      MAX_PR_DESCRIPTION_LENGTH = 3999
-
       #######################
       # Constructor methods #
       #######################
@@ -174,7 +172,6 @@ def create_commit(branch_name, base_commit, commit_message, files,
       def create_pull_request(pr_name, source_branch, target_branch,
                               pr_description, labels,
                               reviewers = nil, assignees = nil, work_item = nil)
-        pr_description = truncate_pr_description(pr_description)
 
         content = {
           sourceRefName: "refs/heads/" + source_branch,
@@ -375,19 +372,6 @@ def auth_header_for(token)
         end
       end
 
-      def truncate_pr_description(pr_description)
-        # Azure DevOps only support descriptions up to 4000 characters in UTF-16
-        # encoding.
-        # https://developercommunity.visualstudio.com/content/problem/608770/remove-4000-character-limit-on-pull-request-descri.html
-        pr_description = pr_description.dup.force_encoding(Encoding::UTF_16)
-        if pr_description.length > MAX_PR_DESCRIPTION_LENGTH
-          truncated_msg = (+"...\n\n_Description has been truncated_").force_encoding(Encoding::UTF_16)
-          truncate_length = MAX_PR_DESCRIPTION_LENGTH - truncated_msg.length
-          pr_description = (pr_description[0..truncate_length] + truncated_msg)
-        end
-        pr_description.force_encoding(Encoding::UTF_8)
-      end
-
       def tags_creation_forbidden?(response)
         return if response.body.empty?
 

diff --git a/common/lib/dependabot/pull_request_creator.rb b/common/lib/dependabot/pull_request_creator.rb
@@ -49,7 +49,8 @@ def initialize(cause, pull_request)
                 :commit_message_options, :vulnerabilities_fixed,
                 :reviewers, :assignees, :milestone, :branch_name_separator,
                 :branch_name_prefix, :branch_name_max_length, :github_redirection_service,
-                :custom_headers, :provider_metadata, :dependency_group
+                :custom_headers, :provider_metadata, :dependency_group, :pr_message_max_length,
+                :pr_message_encoding
 
     def initialize(source:, base_commit:, dependencies:, files:, credentials:,
                    pr_message_header: nil, pr_message_footer: nil,
@@ -61,7 +62,8 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:,
                    automerge_candidate: false,
                    github_redirection_service: DEFAULT_GITHUB_REDIRECTION_SERVICE,
                    custom_headers: nil, require_up_to_date_base: false,
-                   provider_metadata: {}, message: nil, dependency_group: nil)
+                   provider_metadata: {}, message: nil, dependency_group: nil, pr_message_max_length: nil,
+                   pr_message_encoding: nil)
       @dependencies               = dependencies
       @source                     = source
       @base_commit                = base_commit
@@ -88,6 +90,8 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:,
       @provider_metadata          = provider_metadata
       @message                    = message
       @dependency_group           = dependency_group
+      @pr_message_max_length      = pr_message_max_length
+      @pr_message_encoding        = pr_message_encoding
 
       check_dependencies_have_previous_version
     end
@@ -216,19 +220,32 @@ def codecommit_creator
     end
 
     def message
-      @message ||=
-        MessageBuilder.new(
-          source: source,
-          dependencies: dependencies,
-          files: files,
-          credentials: credentials,
-          commit_message_options: commit_message_options,
-          pr_message_header: pr_message_header,
-          pr_message_footer: pr_message_footer,
-          vulnerabilities_fixed: vulnerabilities_fixed,
-          github_redirection_service: github_redirection_service,
-          dependency_group: dependency_group
-        )
+      return @message unless @message.nil?
+
+      case source.provider
+      when "github"
+        @pr_message_max_length = Github::PR_DESCRIPTION_MAX_LENGTH if @pr_message_max_length.nil?
+      when "azure"
+        @pr_message_max_length = Azure::PR_DESCRIPTION_MAX_LENGTH if @pr_message_max_length.nil?
+        @pr_message_encoding = Azure::PR_DESCRIPTION_ENCODING if @pr_message_encoding.nil?
+      when "codecommit"
+        @pr_message_max_length = Codecommit::PR_DESCRIPTION_MAX_LENGTH if @pr_message_max_length.nil?
+      end
+
+      @message = MessageBuilder.new(
+        source: source,
+        dependencies: dependencies,
+        files: files,
+        credentials: credentials,
+        commit_message_options: commit_message_options,
+        pr_message_header: pr_message_header,
+        pr_message_footer: pr_message_footer,
+        vulnerabilities_fixed: vulnerabilities_fixed,
+        github_redirection_service: github_redirection_service,
+        dependency_group: dependency_group,
+        pr_message_max_length: pr_message_max_length,
+        pr_message_encoding: pr_message_encoding
+      )
     end
 
     def branch_namer

diff --git a/common/lib/dependabot/pull_request_creator/azure.rb b/common/lib/dependabot/pull_request_creator/azure.rb
@@ -10,6 +10,11 @@ class Azure
                   :files, :commit_message, :pr_description, :pr_name,
                   :author_details, :labeler, :reviewers, :assignees, :work_item
 
+      # Azure DevOps limits PR descriptions to a max of 4,000 characters in UTF-16 encoding:
+      # https://developercommunity.visualstudio.com/content/problem/608770/remove-4000-character-limit-on-pull-request-descri.html
+      PR_DESCRIPTION_MAX_LENGTH = 3_999 # 0 based count
+      PR_DESCRIPTION_ENCODING = Encoding::UTF_16
+
       def initialize(source:, branch_name:, base_commit:, credentials:,
                      files:, commit_message:, pr_description:, pr_name:,
                      author_details:, labeler:, reviewers: nil, assignees: nil, work_item: nil)

diff --git a/common/lib/dependabot/pull_request_creator/codecommit.rb b/common/lib/dependabot/pull_request_creator/codecommit.rb
@@ -10,6 +10,10 @@ class Codecommit
                   :files, :commit_message, :pr_description, :pr_name,
                   :author_details, :labeler
 
+      # CodeCommit limits PR descriptions to a max length of 10,240 characters:
+      # https://docs.aws.amazon.com/codecommit/latest/APIReference/API_PullRequest.html
+      PR_DESCRIPTION_MAX_LENGTH = 10_239 # 0 based count
+
       def initialize(source:, branch_name:, base_commit:, credentials:,
                      files:, commit_message:, pr_description:, pr_name:,
                      author_details:, labeler:, require_up_to_date_base:)

diff --git a/common/lib/dependabot/pull_request_creator/github.rb b/common/lib/dependabot/pull_request_creator/github.rb
@@ -9,7 +9,9 @@ module Dependabot
   class PullRequestCreator
     # rubocop:disable Metrics/ClassLength
     class Github
-      MAX_PR_DESCRIPTION_LENGTH = 65_536 # characters (see #create_pull_request)
+      # GitHub limits PR descriptions to a max of 65,536 characters:
+      # https://github.com/orgs/community/discussions/27190#discussioncomment-3726017
+      PR_DESCRIPTION_MAX_LENGTH = 65_535 # 0 based count
 
       attr_reader :source, :branch_name, :base_commit, :credentials,
                   :files, :pr_description, :pr_name, :commit_message,
@@ -349,18 +351,6 @@ def add_milestone_to_pull_request(pull_request)
       end
 
       def create_pull_request
-        # Limit PR description to MAX_PR_DESCRIPTION_LENGTH (65,536) characters
-        # and truncate with message if over. The API limit is 262,144 bytes
-        # (https://github.community/t/maximum-length-for-the-comment-body-in-issues-and-pr/148867/2).
-        # As Ruby strings are UTF-8 encoded, this is a pessimistic limit: it
-        # presumes the case where all characters are 4 bytes.
-        pr_description = @pr_description.dup
-        if pr_description && pr_description.length > MAX_PR_DESCRIPTION_LENGTH
-          truncated_msg = "...\n\n_Description has been truncated_"
-          truncate_length = MAX_PR_DESCRIPTION_LENGTH - truncated_msg.length
-          pr_description = (pr_description[0, truncate_length] + truncated_msg)
-        end
-
         github_client_for_source.create_pull_request(
           source.repo,
           target_branch,

diff --git a/common/lib/dependabot/pull_request_creator/message_builder.rb b/common/lib/dependabot/pull_request_creator/message_builder.rb
@@ -22,13 +22,16 @@ class MessageBuilder
       attr_reader :source, :dependencies, :files, :credentials,
                   :pr_message_header, :pr_message_footer,
                   :commit_message_options, :vulnerabilities_fixed,
-                  :github_redirection_service, :dependency_group
+                  :github_redirection_service, :dependency_group, :pr_message_max_length,
+                  :pr_message_encoding
+
+      TRUNCATED_MSG = "...\n\n_Description has been truncated_"
 
       def initialize(source:, dependencies:, files:, credentials:,
                      pr_message_header: nil, pr_message_footer: nil,
                      commit_message_options: {}, vulnerabilities_fixed: {},
                      github_redirection_service: DEFAULT_GITHUB_REDIRECTION_SERVICE,
-                     dependency_group: nil)
+                     dependency_group: nil, pr_message_max_length: nil, pr_message_encoding: nil)
         @dependencies               = dependencies
         @files                      = files
         @source                     = source
@@ -39,22 +42,46 @@ def initialize(source:, dependencies:, files:, credentials:,
         @vulnerabilities_fixed      = vulnerabilities_fixed
         @github_redirection_service = github_redirection_service
         @dependency_group           = dependency_group
+        @pr_message_max_length      = pr_message_max_length
+        @pr_message_encoding        = pr_message_encoding
       end
 
+      attr_writer :pr_message_max_length
+
+      attr_writer :pr_message_encoding
+
       def pr_name
         name = dependency_group ? group_pr_name : solo_pr_name
         name[0] = name[0].capitalize if pr_name_prefixer.capitalize_first_word?
         "#{pr_name_prefix}#{name}"
       end
 
       def pr_message
-        suffixed_pr_message_header + commit_message_intro +
-          metadata_cascades + prefixed_pr_message_footer
+        msg = "#{suffixed_pr_message_header}#{commit_message_intro}#{metadata_cascades}#{prefixed_pr_message_footer}"
+        truncate_pr_message(msg)
       rescue StandardError => e
         Dependabot.logger.error("Error while generating PR message: #{e.message}")
         suffixed_pr_message_header + prefixed_pr_message_footer
       end
 
+      # Truncate PR message as determined by the pr_message_max_length and pr_message_encoding instance variables
+      # The encoding is used when calculating length, all messages are returned as ruby UTF_8 encoded string
+      def truncate_pr_message(msg)
+        return msg if pr_message_max_length.nil?
+
+        msg = msg.dup
+        msg = msg.force_encoding(pr_message_encoding) unless pr_message_encoding.nil?
+
+        if msg.length > pr_message_max_length
+          tr_msg = pr_message_encoding.nil? ? TRUNCATED_MSG : (+TRUNCATED_MSG).dup.force_encoding(pr_message_encoding)
+          trunc_length = pr_message_max_length - tr_msg.length
+          msg = (msg[0..trunc_length] + tr_msg)
+        end
+        # if we used a custom encoding for calculating length, then we need to force back to UTF-8
+        msg.force_encoding(Encoding::UTF_8) unless pr_message_encoding.nil?
+        msg
+      end
+
       def commit_message
         message = commit_subject + "\n\n"
         message += commit_message_intro

diff --git a/common/spec/dependabot/pull_request_creator/azure_spec.rb b/common/spec/dependabot/pull_request_creator/azure_spec.rb
@@ -168,23 +168,6 @@
       end
     end
 
-    context "with e very long pr description" do
-      let(:pr_description) { ("a" * 3997) + "💣 kaboom" }
-      it "truncates the description respecting azures encoding" do
-        creator.create
-
-        expect(WebMock).
-          to(
-            have_requested(:post, "#{repo_api_url}/pullrequests?api-version=5.0").
-            with do |req|
-              description = JSON.parse(req.body).fetch("description")
-              expect(description.length).to eq 4000
-              expect(description).to end_with("\n\n_Description has been truncated_")
-            end
-          )
-      end
-    end
-
     context "with author details provided" do
       let(:author_details) do
         { email: "[email protected]", name: "dependabot" }

diff --git a/common/spec/dependabot/pull_request_creator/github_spec.rb b/common/spec/dependabot/pull_request_creator/github_spec.rb
@@ -1088,25 +1088,6 @@
           )
         end
       end
-
-      context "the PR description is too long" do
-        let(:pr_description) { "a" * (described_class::MAX_PR_DESCRIPTION_LENGTH + 1) }
-
-        it "truncates the description" do
-          creator.create
-
-          expect(WebMock).
-            to have_requested(:post, "#{repo_api_url}/pulls").
-            with(
-              body: {
-                base: "master",
-                head: "dependabot/bundler/business-1.5.0",
-                title: "PR name",
-                body: ->(body) { expect(body.length).to be <= described_class::MAX_PR_DESCRIPTION_LENGTH }
-              }
-            )
-        end
-      end
     end
   end
 end
diff --git a/common/spec/dependabot/pull_request_creator/message_builder_spec.rb b/common/spec/dependabot/pull_request_creator/message_builder_spec.rb
@@ -2572,4 +2572,68 @@ def commits_details(base:, head:)
     its(:pr_message) { should eq(pr_message) }
     its(:commit_message) { should eq(commit_message) }
   end
+
+  subject(:message_builder) { builder }
+  describe "#truncate_pr_message" do
+    context "when pr_message_max_length is not provided" do
+      let(:message) { "This is a normal length PR description and it should not be truncated." }
+
+      it "returns the original message" do
+        expect(message_builder.truncate_pr_message(message)).to eq(message)
+      end
+
+      let(:message) { "This is a test message with special characters: © ®" }
+
+      it "returns the original encoding of the message" do
+        message_builder.pr_message_encoding = Encoding::UTF_16
+        expect(message_builder.truncate_pr_message(message)).to eq(message)
+      end
+    end
+
+    context "when pr_message_max_length is provided" do
+      let(:message) { "A" * 10_250 } # Exceeds the maximum length of 10,239
+      let(:pr_message_max_length) { 10_239 }
+
+      it "truncates the message to the specified length" do
+        truncated_msg = "...\n\n_Description has been truncated_"
+        truncate_length = pr_message_max_length - truncated_msg.length
+        expected_truncated_description = "#{message[0..truncate_length]}#{truncated_msg}"
+
+        message_builder.pr_message_max_length = pr_message_max_length
+        expect(message_builder.truncate_pr_message(message)).to eq(expected_truncated_description)
+      end
+
+      let(:message) { "© ®" * 100 } # Exceeds the maximum length of 100
+      let(:pr_message_max_length) { 100 }
+
+      it "truncates and maintains the specified encoding" do
+        encode_utf16 = Encoding::UTF_16
+        msg = message.dup.force_encoding(encode_utf16)
+        trunc_msg = (+"...\n\n_Description has been truncated_").force_encoding(encode_utf16)
+        trunc_length = pr_message_max_length - trunc_msg.length
+        msg = "#{msg[0..trunc_length]}#{trunc_msg}"
+        msg = msg.force_encoding(Encoding::UTF_8)
+
+        message_builder.pr_message_max_length = pr_message_max_length
+        message_builder.pr_message_encoding = encode_utf16
+        expect(message_builder.truncate_pr_message(message)).to eq(msg)
+      end
+    end
+
+    context "when the pull request description is an empty string" do
+      let(:message) { "" }
+      let(:pr_message_max_length) { 100 }
+
+      it "returns an empty string" do
+        message_builder.pr_message_max_length = pr_message_max_length
+        expect(message_builder.truncate_pr_message(message)).to eq("")
+      end
+
+      it "returns an empty string when encoded" do
+        message_builder.pr_message_max_length = pr_message_max_length
+        message_builder.pr_message_encoding = Encoding::UTF_16
+        expect(message_builder.truncate_pr_message(message)).to eq("")
+      end
+    end
+  end
 end