From ce327011d81c0b10a25a0d943513b09d0d43d8ee Mon Sep 17 00:00:00 2001 From: dwc0011 <53784917+dwc0011@users.noreply.github.com> Date: Tue, 25 Jul 2023 14:24:20 -0400 Subject: [PATCH] Add option to control PR description max length + set reasonable defaults 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 Co-authored-by: Jeff Widman --- common/lib/dependabot/clients/azure.rb | 16 ----- common/lib/dependabot/pull_request_creator.rb | 47 +++++++++----- .../dependabot/pull_request_creator/azure.rb | 5 ++ .../pull_request_creator/codecommit.rb | 4 ++ .../dependabot/pull_request_creator/github.rb | 16 +---- .../pull_request_creator/message_builder.rb | 35 ++++++++-- .../pull_request_creator/azure_spec.rb | 17 ----- .../pull_request_creator/github_spec.rb | 19 ------ .../message_builder_spec.rb | 64 +++++++++++++++++++ 9 files changed, 139 insertions(+), 84 deletions(-) diff --git a/common/lib/dependabot/clients/azure.rb b/common/lib/dependabot/clients/azure.rb index c43df865b19..27bc596b8d9 100644 --- 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 index f5e49a5da60..df5c2cd8790 100644 --- 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 index 1a139bf3631..0c605faa45e 100644 --- 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 index e7bb305ea33..1ea54f3191e 100644 --- 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 index 0ea6e2546e6..e460aa6e590 100644 --- 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 index edd04d0521a..4c81ad31f44 100644 --- 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,8 +42,14 @@ 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? @@ -48,13 +57,31 @@ def pr_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 index 1996ad7a38d..d6d9fed1c21 100644 --- 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: "support@dependabot.com", name: "dependabot" } diff --git a/common/spec/dependabot/pull_request_creator/github_spec.rb b/common/spec/dependabot/pull_request_creator/github_spec.rb index ba472aa6b09..93bb17bad26 100644 --- 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 index e9197229784..e903ffbc7d1 100644 --- 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