diff --git a/updater/lib/dependabot/dependency_snapshot.rb b/updater/lib/dependabot/dependency_snapshot.rb index 9320eb14978..28e72019e49 100644 --- a/updater/lib/dependabot/dependency_snapshot.rb +++ b/updater/lib/dependabot/dependency_snapshot.rb @@ -119,18 +119,23 @@ def job_group @dependency_group_engine.find_group(name: T.must(job.dependency_group_to_refresh)) end - sig { params(group: Dependabot::DependencyGroup).void } - def mark_group_handled(group) + sig { params(group: Dependabot::DependencyGroup, excluding_dependencies: T::Set[String]).void } + def mark_group_handled(group, excluding_dependencies = Set.new) Dependabot.logger.info("Marking group '#{group.name}' as handled.") directories.each do |directory| @current_directory = directory # add the existing dependencies in the group so individual updates don't try to update them - add_handled_dependencies(dependencies_in_existing_pr_for_group(group).filter_map { |d| d["dependency-name"] }) + dependencies_in_existing_prs = dependencies_in_existing_pr_for_group(group) + # also add dependencies that might be in the group, as a rebase would add them; - # this avoids individual PR creation that immediately is superseded by a group PR supersede - add_handled_dependencies(group.dependencies.map(&:name)) + # this avoids individual PR creation that immediately is superseded by a group PR supersede. + current_dependencies = group.dependencies.map(&:name).reject do |dep| + excluding_dependencies.include?(dep) + end + + add_handled_dependencies(current_dependencies.concat(dependencies_in_existing_prs)) end end @@ -169,6 +174,17 @@ def ungrouped_dependencies allowed_dependencies.reject { |dep| handled_dependencies.include?(dep.name) } end + sig { params(group: Dependabot::DependencyGroup).returns(T::Array[String]) } + def dependencies_in_existing_pr_for_group(group) + existing = job.existing_group_pull_requests.find do |pr| + pr["dependency-group-name"] == group.name + end&.fetch("dependencies", []) || [] + + existing.filter_map do |dep| + dep["dependency-name"] + end + end + private sig do @@ -277,13 +293,6 @@ def dependency_file_parser parser end - sig { params(group: Dependabot::DependencyGroup).returns(T::Array[T::Hash[String, String]]) } - def dependencies_in_existing_pr_for_group(group) - job.existing_group_pull_requests.find do |pr| - pr["dependency-group-name"] == group.name - end&.fetch("dependencies", []) || [] - end - sig { void } def assert_current_directory_set! if @current_directory == "" && directories.count == 1 diff --git a/updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb b/updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb index 4f813232748..2339ba11867 100644 --- a/updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb +++ b/updater/lib/dependabot/updater/operations/refresh_group_update_pull_request.rb @@ -72,7 +72,7 @@ def initialize(service:, job:, dependency_snapshot:, error_handler:) end sig { void } - def perform # rubocop:disable Metrics/AbcSize + def perform # This guards against any jobs being performed where the data is malformed, this should not happen unless # there was is defect in the service and we emitted a payload where the job and configuration data objects # were out of sync. @@ -100,24 +100,35 @@ def perform # rubocop:disable Metrics/AbcSize # so users are informed this group is no longer actionable by Dependabot. warn_group_is_empty(job_group) close_pull_request(reason: :dependency_group_empty, group: job_group) - else - Dependabot.logger.info("Updating the '#{job_group.name}' group") + return + end + + process_group_dependencies(job_group) + end - # Preprocess to discover existing group PRs and add their dependencies to the handled list before processing - # the refresh. This prevents multiple PRs from being created for the same dependency during the refresh. - dependency_snapshot.groups.each do |group| - next unless group.name != job_group.name && pr_exists_for_dependency_group?(group) + sig { params(job_group: Dependabot::DependencyGroup).void } + def process_group_dependencies(job_group) + Dependabot.logger.info("Updating the '#{job_group.name}' group") - dependency_snapshot.mark_group_handled(group) - end + existing_pr_dependencies = Set.new - if dependency_change.nil? - Dependabot.logger.info("Nothing could update for Dependency Group: '#{job_group.name}'") - return - end + # Preprocess to discover existing group PRs and add their dependencies to the handled list before processing + # the refresh. This prevents multiple PRs from being created for the same dependency during the refresh. + dependency_snapshot.groups.each do |group| + group_pr_deps = dependency_snapshot.dependencies_in_existing_pr_for_group(group) + existing_pr_dependencies.merge(group_pr_deps) - upsert_pull_request_with_error_handling(T.must(dependency_change), job_group) + next unless group.name != job_group.name && pr_exists_for_dependency_group?(group) + + dependency_snapshot.mark_group_handled(group, existing_pr_dependencies) end + + if dependency_change.nil? + Dependabot.logger.info("Nothing could update for Dependency Group: '#{job_group.name}'") + return + end + + upsert_pull_request_with_error_handling(T.must(dependency_change), job_group) end private diff --git a/updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb b/updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb index a68397ce6ca..17e83fac4e7 100644 --- a/updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb +++ b/updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_spec.rb @@ -268,6 +268,31 @@ refresh_group.perform end end + + context "when there is an existing PR for the same group it has a minor version in another group" do + let(:job_definition) do + job_definition_fixture("bundler/version_updates/group_update_refresh_multiple_groups_unchaged") + end + + let(:dependency_files) do + original_bundler_files(fixture: "bundler_multiple_groups") + end + + before do + stub_rubygems_calls + end + + it "updates the existing pull request without errors" do + expect(mock_service).not_to receive(:close_pull_request) + expect(mock_service).to receive(:update_pull_request) do |dependency_change| + expect(dependency_change.dependency_group.name).to eql("major") + expect(dependency_change.updated_dependency_files_hash) + .to eql(updated_bundler_files_hash(fixture: "bundler_multiple_groups")) + end + + refresh_group.perform + end + end end describe "#deduce_updated_dependency" do diff --git a/updater/spec/fixtures/bundler_multiple_groups/original/Gemfile b/updater/spec/fixtures/bundler_multiple_groups/original/Gemfile new file mode 100644 index 00000000000..9ff0158015b --- /dev/null +++ b/updater/spec/fixtures/bundler_multiple_groups/original/Gemfile @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +source "https://rubygems.org" + +gem "dummy-pkg-a", "~> 1.1.0" +gem "dummy-pkg-b", "~> 1.1.0" diff --git a/updater/spec/fixtures/bundler_multiple_groups/original/Gemfile.lock b/updater/spec/fixtures/bundler_multiple_groups/original/Gemfile.lock new file mode 100644 index 00000000000..290d981559f --- /dev/null +++ b/updater/spec/fixtures/bundler_multiple_groups/original/Gemfile.lock @@ -0,0 +1,15 @@ +GEM + remote: https://rubygems.org/ + specs: + dummy-pkg-a (1.1.0) + dummy-pkg-b (1.1.0) + +PLATFORMS + ruby + +DEPENDENCIES + dummy-pkg-a (~> 1.1.0) + dummy-pkg-b (~> 1.1.0) + +BUNDLED WITH + 2.4.13 diff --git a/updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile b/updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile new file mode 100644 index 00000000000..ecc7a043dfa --- /dev/null +++ b/updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +source "https://rubygems.org" + +gem "dummy-pkg-a", "~> 2.0.0" +gem "dummy-pkg-b", "~> 1.1.0" diff --git a/updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile.lock b/updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile.lock new file mode 100644 index 00000000000..dba54d174b4 --- /dev/null +++ b/updater/spec/fixtures/bundler_multiple_groups/updated/Gemfile.lock @@ -0,0 +1,15 @@ +GEM + remote: https://rubygems.org/ + specs: + dummy-pkg-a (2.0.0) + dummy-pkg-b (1.1.0) + +PLATFORMS + ruby + +DEPENDENCIES + dummy-pkg-a (~> 2.0.0) + dummy-pkg-b (~> 1.1.0) + +BUNDLED WITH + 2.4.13 diff --git a/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh_multiple_groups_unchaged.yaml b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh_multiple_groups_unchaged.yaml new file mode 100644 index 00000000000..72250a2ce6a --- /dev/null +++ b/updater/spec/fixtures/job_definitions/bundler/version_updates/group_update_refresh_multiple_groups_unchaged.yaml @@ -0,0 +1,77 @@ +job: + package-manager: bundler + source: + provider: github + repo: dependabot/dependabot-bug-version-types + branch: + api-endpoint: https://api.github.com/ + hostname: github.com + directories: + - "/" + dependencies: + - "dummy-pkg-a" + existing-pull-requests: [] + existing-group-pull-requests: + - dependency-group-name: major + dependencies: + - dependency-name: dummy-pkg-a + dependency-version: 2.0.0 + directory: "/" + - dependency-group-name: minor + dependencies: + - dependency-name: dummy-pkg-b + dependency-version: 1.2.0 + directory: "/" + updating-a-pull-request: true + lockfile-only: false + update-subdependencies: false + ignore-conditions: [] + requirements-update-strategy: null + allowed-updates: + - dependency-type: direct + update-type: all + credentials-metadata: + - type: git_source + host: github.com + security-advisories: [] + security-updates-only: false + max-updater-run-time: 2700 + vendor-dependencies: false + repo-private: false + proxy-log-response-body-on-auth-failure: true + reject-external-code: false + commit-message-options: + prefix: + prefix-development: + include-scope: + dependency-group-to-refresh: major + dependency-groups: + - name: major + rules: + patterns: + - "dummy-pkg-a" + update-types: + - "major" + - name: minor + rules: + patterns: + - "*" + update-types: + - "patch" + - "minor" + experiments: + dependency-change-validation: true + enable-file-parser-python-local: true + enable-fix-for-pnpm-no-change-error: true + enable-record-ecosystem-meta: true + enable-shared-helpers-command-timeout: true + lead-security-dependency: true + move-job-token: true + npm-v6-deprecation-warning: true + npm-v6-unsupported-error: true + nuget-native-analysis: true + nuget-use-direct-discovery: true + proxy-cached: true + python-3-8-deprecation-warning: true + record-ecosystem-versions: true + record-update-job-unknown-error: true diff --git a/updater/spec/fixtures/rubygems-versions-a.json b/updater/spec/fixtures/rubygems-versions-a.json index 4740cd92c47..45ecb4b1bf5 100644 --- a/updater/spec/fixtures/rubygems-versions-a.json +++ b/updater/spec/fixtures/rubygems-versions-a.json @@ -1,4 +1,21 @@ [ + { + "authors": "Dependabot", + "built_at": "2017-06-08T00:00:00.000Z", + "created_at": "2017-06-08T11:17:36.474Z", + "description": "", + "downloads_count": 301, + "metadata": {}, + "number": "2.0.0", + "summary": "A dummy package for testing Dependabot", + "platform": "ruby", + "rubygems_version": ">= 0", + "ruby_version": ">= 0", + "prerelease": false, + "licenses": ["MIT"], + "requirements": [], + "sha": "8c79aa73922d7f7b38cd96b7af6d489f7c354c6a9fa25bd72772e75d018fbd96" + }, { "authors": "Dependabot", "built_at": "2017-06-08T00:00:00.000Z", @@ -12,9 +29,7 @@ "rubygems_version": ">= 0", "ruby_version": ">= 0", "prerelease": false, - "licenses": [ - "MIT" - ], + "licenses": ["MIT"], "requirements": [], "sha": "51b99c7db0d39924d690e19282f63d1fba9cc002ef55a139d9b6a4b0469399a1" }, @@ -31,9 +46,7 @@ "rubygems_version": ">= 0", "ruby_version": ">= 0", "prerelease": false, - "licenses": [ - "MIT" - ], + "licenses": ["MIT"], "requirements": [], "sha": "c8725691239b43d5f4c343b64d30afae6dd25ff1a79cca4d80534804c638b113" }, @@ -50,9 +63,7 @@ "rubygems_version": ">= 0", "ruby_version": ">= 0", "prerelease": false, - "licenses": [ - "MIT" - ], + "licenses": ["MIT"], "requirements": [], "sha": "0147d64042d5ab109d185f54957fcfb88f1ff9158651944ff75c6c82d47ab444" }