Skip to content

Commit cfb2ac1

Browse files
jonabcjonabc
committed
skip dependencies outside of their source directory
1 parent 6463993 commit cfb2ac1

2 files changed

Lines changed: 248 additions & 0 deletions

File tree

‎updater/lib/dependabot/updater/group_update_creation.rb‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,9 @@ def compile_all_dependency_changes_for(group)
177177

178178
sig { params(dependency: Dependabot::Dependency, group: Dependabot::DependencyGroup).returns(T::Boolean) }
179179
def skip_dependency?(dependency, group)
180+
# Skip dependencies that belong to a different directory than the one currently being processed
181+
return true if dependency.directory != job.source.directory
182+
180183
# Check if dependency has already been handled
181184
handled_dependency = dependency_snapshot.handled_dependencies.include?(dependency.name)
182185

‎updater/spec/dependabot/updater/operations/refresh_group_update_pull_request_multi_dir_spec.rb‎

Lines changed: 245 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,245 @@
1+
# typed: false
2+
# frozen_string_literal: true
3+
4+
require "spec_helper"
5+
require "support/dummy_pkg_helpers"
6+
require "dependabot/dependency_change"
7+
require "dependabot/dependency_snapshot"
8+
require "dependabot/service"
9+
require "dependabot/updater/error_handler"
10+
require "dependabot/updater/operations/refresh_group_update_pull_request"
11+
require "dependabot/updater/group_dependency_selector"
12+
require "dependabot/update_checkers"
13+
require "dependabot/update_checkers/base"
14+
require "dependabot/file_parsers"
15+
require "dependabot/file_parsers/base"
16+
require "dependabot/file_updaters"
17+
require "dependabot/file_updaters/base"
18+
19+
# End-to-end test for the multi-directory duplicate dependency bug.
20+
#
21+
# Scenario: A terraform monorepo has 3 directories, each with the same 3 providers.
22+
# On a group refresh job, compile_all_dependency_changes_for is called per directory.
23+
# The bug caused each directory to report N*3 updated deps (where N = number of dirs)
24+
# instead of just 3, because group.dependencies accumulated entries from all directories
25+
# and skip_dependency? didn't filter by directory.
26+
RSpec.describe Dependabot::Updater::Operations::RefreshGroupUpdatePullRequest do
27+
describe "#perform with multi-directory groups" do
28+
subject(:refresh_operation) do
29+
described_class.new(
30+
service: mock_service,
31+
job: job,
32+
dependency_snapshot: dependency_snapshot,
33+
error_handler: mock_error_handler
34+
)
35+
end
36+
37+
let(:directories) { ["/dir1", "/dir2", "/dir3"] }
38+
let(:dep_names) { %w(hashicorp/aws hashicorp/google hashicorp/kubernetes) }
39+
40+
let(:mock_service) do
41+
instance_double(
42+
Dependabot::Service,
43+
increment_metric: nil,
44+
record_update_job_error: nil,
45+
record_update_job_warning: nil,
46+
record_ecosystem_meta: nil,
47+
record_cooldown_meta: nil
48+
)
49+
end
50+
51+
let(:mock_error_handler) do
52+
instance_double(Dependabot::Updater::ErrorHandler, handle_dependency_error: nil)
53+
end
54+
55+
# Build dependency files for each directory
56+
let(:dependency_files) do
57+
directories.map do |dir|
58+
Dependabot::DependencyFile.new(
59+
name: "main.tf",
60+
content: "# terraform config",
61+
directory: dir
62+
)
63+
end
64+
end
65+
66+
let(:job) do
67+
Dependabot::Job.new_update_job(
68+
job_id: "1234",
69+
job_definition: {
70+
"job" => {
71+
"package-manager" => "terraform",
72+
"source" => {
73+
"provider" => "github",
74+
"repo" => "test/terraform-monorepo",
75+
"directories" => directories,
76+
"branch" => nil,
77+
"api-endpoint" => "https://api.github.com/",
78+
"hostname" => "github.com"
79+
},
80+
"dependencies" => dep_names,
81+
"existing-pull-requests" => [],
82+
"existing-group-pull-requests" => [{
83+
"dependency-group-name" => "all-terraform",
84+
"dependencies" => dep_names.flat_map do |name|
85+
directories.map do |dir|
86+
{ "dependency-name" => name, "dependency-version" => "4.0.0", "directory" => dir }
87+
end
88+
end
89+
}],
90+
"updating-a-pull-request" => true,
91+
"lockfile-only" => false,
92+
"update-subdependencies" => false,
93+
"ignore-conditions" => [],
94+
"requirements-update-strategy" => nil,
95+
"allowed-updates" => [{ "dependency-type" => "direct", "update-type" => "all" }],
96+
"credentials-metadata" => [{ "type" => "git_source", "host" => "github.com" }],
97+
"security-advisories" => [],
98+
"max-updater-run-time" => 2700,
99+
"vendor-dependencies" => false,
100+
"experiments" => { "grouped-updates-prototype" => true },
101+
"reject-external-code" => false,
102+
"commit-message-options" => {},
103+
"security-updates-only" => false,
104+
"dependency-groups" => [{
105+
"name" => "all-terraform",
106+
"rules" => { "patterns" => ["*"] }
107+
}],
108+
"dependency-group-to-refresh" => "all-terraform"
109+
}
110+
}
111+
)
112+
end
113+
114+
let(:dependency_snapshot) do
115+
Dependabot::DependencySnapshot.create_from_job_definition(
116+
job: job,
117+
fetched_files: Dependabot::FetchedFiles.new(
118+
base_commit_sha: "mock-sha",
119+
dependency_files: dependency_files
120+
)
121+
)
122+
end
123+
124+
let(:ecosystem) do
125+
Dependabot::Ecosystem.new(
126+
name: "terraform",
127+
package_manager: DummyPkgHelpers::StubPackageManager.new(
128+
name: "terraform", version: "1.5.0", supported_versions: %w(1.5 1.6)
129+
)
130+
)
131+
end
132+
133+
before do
134+
# Register fake implementations BEFORE dependency_snapshot is created.
135+
# DependencySnapshot#initialize calls parse_files! which needs these.
136+
Dependabot::Dependency.register_production_check("terraform", ->(_groups) { true })
137+
138+
Dependabot::FileParsers.register(
139+
"terraform",
140+
Class.new(Dependabot::FileParsers::Base) do
141+
define_method(:parse) do
142+
dir = source&.directory || "/"
143+
%w(hashicorp/aws hashicorp/google hashicorp/kubernetes).map do |name|
144+
Dependabot::Dependency.new(
145+
name: name,
146+
version: "4.0.0",
147+
requirements: [{
148+
file: "main.tf", requirement: "~> 4.0", groups: [],
149+
source: { type: "provider", registry_hostname: "registry.terraform.io",
150+
module_identifier: name }
151+
}],
152+
package_manager: "terraform",
153+
directory: dir
154+
)
155+
end
156+
end
157+
define_method(:ecosystem) { nil }
158+
define_method(:check_required_files) { nil }
159+
end
160+
)
161+
162+
Dependabot::UpdateCheckers.register(
163+
"terraform",
164+
Class.new(Dependabot::UpdateCheckers::Base) do
165+
define_method(:latest_version) { Gem::Version.new("5.0.0") }
166+
define_method(:latest_resolvable_version) { Gem::Version.new("5.0.0") }
167+
define_method(:latest_resolvable_version_with_no_unlock) { Gem::Version.new("5.0.0") }
168+
define_method(:lowest_security_fix_version) { nil }
169+
define_method(:lowest_resolvable_security_fix_version) { nil }
170+
define_method(:updated_requirements) do
171+
dependency.requirements.map { |r| r.merge(requirement: "~> 5.0") }
172+
end
173+
define_method(:up_to_date?) { false }
174+
define_method(:requirements_unlocked_or_can_be?) { true }
175+
define_method(:can_update?) { |**_kwargs| true }
176+
define_method(:updated_dependencies) do |**_kwargs|
177+
[Dependabot::Dependency.new(
178+
name: dependency.name,
179+
version: "5.0.0",
180+
requirements: dependency.requirements.map { |r| r.merge(requirement: "~> 5.0") },
181+
previous_version: "4.0.0",
182+
previous_requirements: dependency.requirements,
183+
package_manager: "terraform",
184+
directory: dependency.directory
185+
)]
186+
end
187+
end
188+
)
189+
190+
Dependabot::FileUpdaters.register(
191+
"terraform",
192+
Class.new(Dependabot::FileUpdaters::Base) do
193+
define_method(:updated_dependency_files) do
194+
dependency_files.map do |f|
195+
Dependabot::DependencyFile.new(name: f.name, content: "# updated", directory: f.directory)
196+
end
197+
end
198+
define_method(:check_required_files) { nil }
199+
end
200+
)
201+
202+
Dependabot::Utils.register_version_class("terraform", Dependabot::Version)
203+
Dependabot::Utils.register_requirement_class("terraform", Dependabot::Requirement)
204+
205+
Dependabot::Experiments.reset!
206+
Dependabot::Experiments.register(:allow_refresh_group_with_all_dependencies, true)
207+
208+
allow(dependency_snapshot).to receive(:ecosystem).and_return(ecosystem)
209+
allow(job).to receive(:package_manager).and_return("terraform")
210+
end
211+
212+
after do
213+
Dependabot::Experiments.reset!
214+
end
215+
216+
it "produces exactly 3 updated dependencies per directory, not 3*N" do
217+
dependency_change = nil
218+
allow(mock_service).to receive(:update_pull_request) { |change| dependency_change = change }
219+
allow(mock_service).to receive(:create_pull_request) { |change| dependency_change = change }
220+
221+
refresh_operation.perform
222+
223+
expect(dependency_change).not_to be_nil
224+
225+
# With 3 directories × 3 deps, correct behavior = 9 total (3 per dir).
226+
# Bug would produce 27 (9 per dir × 3 dirs due to cross-dir contamination).
227+
updated_count = dependency_change.updated_dependencies.length
228+
expect(updated_count).to eq(9), "Expected 9 updated deps but got #{updated_count}"
229+
end
230+
231+
it "has no duplicate dependency name+directory combinations" do
232+
dependency_change = nil
233+
allow(mock_service).to receive(:update_pull_request) { |change| dependency_change = change }
234+
allow(mock_service).to receive(:create_pull_request) { |change| dependency_change = change }
235+
236+
refresh_operation.perform
237+
238+
expect(dependency_change).not_to be_nil
239+
240+
name_dir_pairs = dependency_change.updated_dependencies.map { |d| [d.name, d.directory] }
241+
duplicates = name_dir_pairs.tally.select { |_pair, count| count > 1 }
242+
expect(duplicates).to be_empty, "Found duplicate (name, directory) pairs: #{duplicates.keys.inspect}"
243+
end
244+
end
245+
end

0 commit comments

Comments
 (0)