Skip to content

Commit 70452de

Browse files
authored
refactor: Use Set-based parent guard and deduplicate subgroups in DependencyGroupEngine
- Use @group_by_name_parent_names Set in matches_group_by_parent_group? for consistency with the recursion guard in create_dynamic_subgroups_for_dependency_name_groups - Reuse existing subgroups instead of creating duplicates when assign_to_groups! is called multiple times (once per directory)
1 parent 675bc54 commit 70452de

File tree

2 files changed

+26
-16
lines changed

2 files changed

+26
-16
lines changed

updater/lib/dependabot/dependency_group_engine.rb

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ def mark_ungrouped_if_no_matches(dependency, matched_groups)
145145
sig { params(dependency: Dependabot::Dependency).returns(T::Boolean) }
146146
def matches_group_by_parent_group?(dependency)
147147
@dependency_groups.any? do |group|
148-
group.group_by_dependency_name? && group.contains?(dependency)
148+
@group_by_name_parent_names.include?(group.name) && group.contains?(dependency)
149149
end
150150
end
151151

@@ -226,16 +226,23 @@ def create_dynamic_subgroups_for_dependency_name_groups(dependencies)
226226
matching_deps = dependencies.select { |dep| parent_group.contains?(dep) }
227227

228228
matching_deps.group_by(&:name).each do |dep_name, deps|
229-
# Preserve "group-by" in subgroup rules so group_by_dependency_name? returns true,
230-
# enabling correct handled-dependency tracking when semver rules reject an update.
231-
subgroup_rules = parent_group.rules.merge("patterns" => [dep_name])
232-
subgroup = Dependabot::DependencyGroup.new(
233-
name: "#{parent_group.name}/#{dep_name}",
234-
rules: subgroup_rules,
235-
applies_to: parent_group.applies_to
236-
)
237-
subgroup.dependencies.concat(deps)
238-
@dependency_groups << subgroup
229+
subgroup_name = "#{parent_group.name}/#{dep_name}"
230+
existing_subgroup = @dependency_groups.find { |g| g.name == subgroup_name }
231+
232+
if existing_subgroup
233+
existing_subgroup.dependencies.concat(deps)
234+
else
235+
# Preserve "group-by" in subgroup rules so group_by_dependency_name? returns true,
236+
# enabling correct handled-dependency tracking when semver rules reject an update.
237+
subgroup_rules = parent_group.rules.merge("patterns" => [dep_name])
238+
subgroup = Dependabot::DependencyGroup.new(
239+
name: subgroup_name,
240+
rules: subgroup_rules,
241+
applies_to: parent_group.applies_to
242+
)
243+
subgroup.dependencies.concat(deps)
244+
@dependency_groups << subgroup
245+
end
239246
end
240247

241248
parent_group.dependencies.clear

updater/spec/dependabot/dependency_group_engine_spec.rb

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -562,12 +562,12 @@
562562
)
563563
dependency_group_engine.assign_to_groups!(dependencies: [new_pkg_a])
564564

565-
# Should have exactly 2 subgroups for dummy-pkg-a (one per directory call),
566-
# NOT 3 (which would happen if the first subgroup was treated as a parent)
565+
# Should reuse the existing subgroup rather than creating a duplicate
567566
subgroups_a = dependency_group_engine.dependency_groups.select do |g|
568567
g.name == "per-dependency/dummy-pkg-a"
569568
end
570-
expect(subgroups_a.length).to be(2)
569+
expect(subgroups_a.length).to be(1)
570+
expect(subgroups_a.first.dependencies).to contain_exactly(dummy_pkg_a, new_pkg_a)
571571

572572
# Should have no sub-sub-groups
573573
nested = dependency_group_engine.dependency_groups.select do |g|
@@ -895,11 +895,14 @@
895895
# Simulate a second directory by calling assign_to_groups! again.
896896
dependency_group_engine.assign_to_groups!(dependencies: [dummy_pkg_a_dir2])
897897

898-
# Should have subgroups from both calls, but no sub-subgroups
898+
# Should reuse the existing subgroup rather than creating a duplicate
899899
subgroups_a = dependency_group_engine.dependency_groups.select do |g|
900900
g.name == "monorepo-deps/dummy-pkg-a"
901901
end
902-
expect(subgroups_a.length).to eq(2)
902+
expect(subgroups_a.length).to eq(1)
903+
904+
# The existing subgroup should now contain deps from both calls
905+
expect(subgroups_a.first.dependencies.count { |d| d.name == "dummy-pkg-a" }).to eq(3)
903906

904907
nested = dependency_group_engine.dependency_groups.select do |g|
905908
g.name.start_with?("monorepo-deps/dummy-pkg-a/")

0 commit comments

Comments
 (0)