Skip to content

Commit 48c56e6

Browse files
fix(filter): validate package name on full graph (#9102)
### Description Fixes #9096 The issue was that the validation was run against an already filtered set of packages if another selector was provided i.e. `--filter foo[commit]` the name filter `foo` would run against only packages that were changed since `commit`. This meant that `foo` would be incorrectly reported as not existing. This PR changes so the package existence check is run against all packages in the graph instead of an already filtered set. ### Testing Instructions Added red->green unit test Verify that this PR correctly handles the repro provided in #9096 ``` [1 olszewski@chriss-mbp] /tmp/turbo-filter-break $ turbo_dev --skip-infer run lint '--filter=...@repo/eslint-config[HEAD~1]' turbo 2.1.1 • Packages in scope: • Running lint in 0 packages • Remote caching disabled No tasks were executed as part of this run. Tasks: 0 successful, 0 total Cached: 0 cached, 0 total Time: 75ms ```
1 parent 29fe5ef commit 48c56e6

File tree

2 files changed

+39
-26
lines changed

2 files changed

+39
-26
lines changed

crates/turborepo-lib/src/run/scope/filter.rs

+37-26
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,7 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> {
392392

393393
// if we have a filter, use it to filter the entry packages
394394
let filtered_entry_packages = if !selector.name_pattern.is_empty() {
395-
match_package_names(&selector.name_pattern, entry_packages)?
395+
match_package_names(&selector.name_pattern, &self.all_packages(), entry_packages)?
396396
} else {
397397
entry_packages
398398
};
@@ -520,17 +520,12 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> {
520520

521521
if !selector.name_pattern.is_empty() {
522522
if !selector_valid {
523-
entry_packages = self.match_package_names_to_vertices(
524-
&selector.name_pattern,
525-
self.pkg_graph
526-
.packages()
527-
.map(|(name, _)| name.to_owned())
528-
.collect(),
529-
)?;
523+
entry_packages = self.all_packages();
530524
selector_valid = true;
531-
} else {
532-
entry_packages = match_package_names(&selector.name_pattern, entry_packages)?;
533525
}
526+
let all_packages = self.all_packages();
527+
entry_packages =
528+
match_package_names(&selector.name_pattern, &all_packages, entry_packages)?;
534529
}
535530

536531
// if neither a name pattern, parent dir, or from ref is provided, then
@@ -556,15 +551,14 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> {
556551
)
557552
}
558553

559-
fn match_package_names_to_vertices(
560-
&self,
561-
name_pattern: &str,
562-
mut entry_packages: HashSet<PackageName>,
563-
) -> Result<HashSet<PackageName>, ResolutionError> {
564-
// add the root package to the entry packages
565-
entry_packages.insert(PackageName::Root);
566-
567-
match_package_names(name_pattern, entry_packages)
554+
fn all_packages(&self) -> HashSet<PackageName> {
555+
let mut packages = self
556+
.pkg_graph
557+
.packages()
558+
.map(|(name, _)| name.to_owned())
559+
.collect::<HashSet<_>>();
560+
packages.insert(PackageName::Root);
561+
packages
568562
}
569563
}
570564

@@ -574,21 +568,26 @@ impl<'a, T: GitChangeDetector> FilterResolver<'a, T> {
574568
/// the pattern is normalized, replacing `\*` with `.*`
575569
fn match_package_names(
576570
name_pattern: &str,
577-
mut entry_packages: HashSet<PackageName>,
571+
all_packages: &HashSet<PackageName>,
572+
mut packages: HashSet<PackageName>,
578573
) -> Result<HashSet<PackageName>, ResolutionError> {
579574
let matcher = SimpleGlob::new(name_pattern)?;
580-
let matched_packages = entry_packages
581-
.extract_if(|e| matcher.is_match(e.as_ref()))
575+
let matched_packages = all_packages
576+
.iter()
577+
.filter(|e| matcher.is_match(e.as_ref()))
578+
.cloned()
582579
.collect::<HashSet<_>>();
583580

584581
// If the pattern was an exact name and it matched no packages, then error
585582
if matcher.is_exact() && matched_packages.is_empty() {
586-
Err(ResolutionError::NoPackagesMatchedWithName(
583+
return Err(ResolutionError::NoPackagesMatchedWithName(
587584
name_pattern.to_owned(),
588-
))
589-
} else {
590-
Ok(matched_packages)
585+
));
591586
}
587+
588+
packages.retain(|pkg| matched_packages.contains(pkg));
589+
590+
Ok(packages)
592591
}
593592

594593
#[derive(Debug, thiserror::Error)]
@@ -628,6 +627,7 @@ pub enum ResolutionError {
628627
mod test {
629628
use std::collections::{HashMap, HashSet};
630629

630+
use pretty_assertions::assert_eq;
631631
use tempfile::TempDir;
632632
use test_case::test_case;
633633
use turbopath::{AbsoluteSystemPathBuf, AnchoredSystemPathBuf, RelativeUnixPathBuf};
@@ -1206,6 +1206,17 @@ mod test {
12061206
&["package-1", "package-2"] ;
12071207
"match dependency subtree"
12081208
)]
1209+
#[test_case(
1210+
vec![
1211+
TargetSelector {
1212+
name_pattern: "package-3".to_string(),
1213+
git_range: Some(GitRange { from_ref: Some("HEAD~1".to_string()), to_ref: None, ..Default::default() }),
1214+
..Default::default()
1215+
}
1216+
],
1217+
&[] ;
1218+
"gh 9096"
1219+
)]
12091220
fn scm(selectors: Vec<TargetSelector>, expected: &[&str]) {
12101221
let scm_resolver = TestChangeDetector::new(&[
12111222
("HEAD~1", None, &["package-1", "package-2", ROOT_PKG_NAME]),

crates/turborepo-lib/src/run/scope/target_selector.rs

+2
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,7 @@ pub fn is_selector_by_location(
234234
mod test {
235235
use std::str::FromStr;
236236

237+
use pretty_assertions::assert_eq;
237238
use test_case::test_case;
238239
use turbopath::AnchoredSystemPathBuf;
239240

@@ -263,6 +264,7 @@ mod test {
263264
#[test_case("foo...[master]", TargetSelector { raw: "foo...[master]".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), name_pattern: "foo".to_string(), match_dependencies: true, ..Default::default() }; "foo...[master]")]
264265
#[test_case("foo...[master]...", TargetSelector { raw: "foo...[master]...".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), name_pattern: "foo".to_string(), match_dependencies: true, include_dependencies: true, ..Default::default() }; "foo...[master] dot dot dot")]
265266
#[test_case("{foo}...[master]", TargetSelector { raw: "{foo}...[master]".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), parent_dir: Some(AnchoredSystemPathBuf::try_from("foo").unwrap()), match_dependencies: true, ..Default::default() }; " curly brackets foo...[master]")]
267+
#[test_case("...@repo/pkg[master]", TargetSelector { raw: "...@repo/pkg[master]".to_string(), git_range: Some(GitRange { from_ref: Some("master".to_string()), to_ref: None, include_uncommitted: true, ..Default::default() }), name_pattern: "@repo/pkg".to_string(), include_dependents: true, ..Default::default() }; "gh 9096")]
266268
fn parse_target_selector(raw_selector: &str, want: TargetSelector) {
267269
let result = TargetSelector::from_str(raw_selector);
268270

0 commit comments

Comments
 (0)