Skip to content

Commit

Permalink
fix(config check): 🐛 pattern locality (#890)
Browse files Browse the repository at this point in the history
This improves the handling of the location of the patterns when passed
from configuration files or from the command line.
  • Loading branch information
xaf authored Jan 15, 2025
1 parent 196d6b4 commit 368f16a
Show file tree
Hide file tree
Showing 4 changed files with 277 additions and 9 deletions.
2 changes: 1 addition & 1 deletion src/internal/commands/builtin/config/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ impl ConfigCheckCommand {
let cliarg_patterns: Vec<String> = args
.patterns
.iter()
.map(|value| path_pattern_from_str(value, None))
.map(|value| path_pattern_from_str(value, None, true))
.collect();

// Filter and sort the errors
Expand Down
128 changes: 120 additions & 8 deletions src/internal/config/parser/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::internal::commands::utils::abs_path_from_path;
use crate::internal::config::parser::errors::ConfigErrorHandler;
use crate::internal::config::parser::errors::ConfigErrorKind;
use crate::internal::config::parser::github::StringFilter;
use crate::internal::config::ConfigScope;
use crate::internal::config::ConfigValue;

#[derive(Debug, Serialize, Deserialize, Clone, Default)]
Expand Down Expand Up @@ -159,27 +160,34 @@ fn path_pattern_from_config_value(value: &ConfigValue) -> String {
let parent = as_path.parent().unwrap_or(&as_path);
let as_str = parent.to_string_lossy();

path_pattern_from_str(&pattern, Some(&as_str))
path_pattern_from_str(
&pattern,
Some(&as_str),
!matches!(value.get_scope(), ConfigScope::Workdir),
)
}
None => pattern.to_string(),
}
}

pub fn path_pattern_from_str(pattern: &str, location: Option<&str>) -> String {
pub fn path_pattern_from_str(pattern: &str, location: Option<&str>, global: bool) -> String {
let (negative, pattern) = if let Some(pattern) = pattern.strip_prefix('!') {
(true, pattern)
} else {
(false, pattern)
};

// If global pattern, we allow to specify absolute paths, otherwise
// absolute paths are from the provided location
let pattern = if global {
pattern
} else {
pattern.trim_start_matches("/")
};

// If the pattern starts with '/' or '*', it's an absolute path
// or a glob pattern, so we don't need to prepend the location.
if pattern.starts_with('/')
|| pattern.starts_with("*/")
|| pattern.starts_with("**/")
|| pattern == "**"
|| pattern == "*"
{
if pattern.starts_with('/') || pattern.starts_with("**/") || pattern == "**" {
return format!("{}{}", if negative { "!" } else { "" }, pattern);
}

Expand All @@ -193,3 +201,107 @@ pub fn path_pattern_from_str(pattern: &str, location: Option<&str>) -> String {
abs_pattern.to_string_lossy()
)
}

#[cfg(test)]
mod tests {
use super::*;

mod path_pattern_from_str {
use super::*;

#[test]
fn test_global_patterns() {
// Test absolute paths with global flag
assert_eq!(
path_pattern_from_str("/some/path", None, true),
"/some/path"
);

// Test relative paths with global flag
let current_dir = std::env::current_dir().expect("failed to get current dir");
assert_eq!(
path_pattern_from_str("some/path", None, true),
current_dir.join("some/path").to_string_lossy().to_string(),
);
}

#[test]
fn test_negative_patterns() {
// Test negative absolute path
assert_eq!(
path_pattern_from_str("!/some/path", None, true),
"!/some/path"
);

// Test negative relative path with location
assert_eq!(
path_pattern_from_str("!relative/path", Some("/base/dir"), false),
"!/base/dir/relative/path"
);
}

#[test]
fn test_glob_patterns() {
// Test basic glob pattern
assert_eq!(
path_pattern_from_str("**/file.txt", None, false),
"**/file.txt"
);

// Test double-star pattern
assert_eq!(path_pattern_from_str("**", None, false), "**");

// Test negative glob pattern
assert_eq!(
path_pattern_from_str("!**/file.txt", None, false),
"!**/file.txt"
);
}

#[test]
fn test_relative_paths() {
// Test relative path with location
assert_eq!(
path_pattern_from_str("relative/path", Some("/base/dir"), false),
"/base/dir/relative/path"
);

// Test relative path without location (should use current dir)
let current_dir = std::env::current_dir().expect("failed to get current dir");
assert_eq!(
path_pattern_from_str("relative/path", None, false),
current_dir
.join("relative/path")
.to_string_lossy()
.to_string(),
);
}

#[test]
fn test_trim_leading_slash() {
// Test that leading slashes are trimmed for non-global patterns
assert_eq!(
path_pattern_from_str("/path/to/file", Some("/base/dir"), false),
"/base/dir/path/to/file"
);
}

#[test]
fn test_edge_cases() {
// Test empty pattern
assert_eq!(
path_pattern_from_str("", Some("/base/dir"), false),
"/base/dir"
);

// Test single slash
assert_eq!(path_pattern_from_str("/", None, true), "/");

// Test negative empty pattern
assert_eq!(
path_pattern_from_str("!", Some("/base/dir"), false),
"!/base/dir"
);
}
}
}
147 changes: 147 additions & 0 deletions src/internal/config/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,156 @@ pub fn check_allowed(value: &str, patterns: &[String]) -> bool {
if matches {
return !is_deny; // Early return on match
}

if pattern_str.ends_with('*') {
continue;
}

// If the pattern does not end with a wildcard, try checking
// for the pattern as a directory that should prefix the file
let dir_pattern = format!("{}/**", pattern_str.trim_end_matches('/'));
let matches = glob::Pattern::new(&dir_pattern).is_ok_and(|pat| pat.matches(value));
if matches {
return !is_deny; // Early return on match
}
}

// Get the last pattern's deny status (if any) for the default case
let default = patterns.last().map_or(true, |p| p.starts_with('!'));
default
}

#[cfg(test)]
mod tests {
use super::*;

mod check_allowed {
use super::*;

#[test]
fn test_empty_patterns() {
assert!(check_allowed("any/path", &[]));
assert!(check_allowed("", &[]));
}

#[test]
fn test_exact_matches() {
// Test exact path matching
assert!(check_allowed("src/main.rs", &["src/main.rs".to_string()]));

// Test exact path with deny
assert!(!check_allowed("src/main.rs", &["!src/main.rs".to_string()]));
}

#[test]
fn test_wildcard_patterns() {
// Test with * wildcard
assert!(check_allowed("src/test.rs", &["src/*.rs".to_string()]));

// Test with multiple patterns including wildcard
assert!(check_allowed(
"src/lib.rs",
&["src/*.rs".to_string(), "!src/test.rs".to_string()]
));

// Test with ** wildcard
assert!(check_allowed(
"src/subfolder/test.rs",
&["src/**/*.rs".to_string()]
));
}

#[test]
fn test_directory_prefix_matching() {
// Test directory prefix without trailing slash
assert!(check_allowed(
"src/subfolder/file.rs",
&["src/subfolder".to_string()]
));

// Test directory prefix with trailing slash
assert!(check_allowed(
"src/subfolder/file.rs",
&["src/subfolder/".to_string()]
));

// Test nested directory matching
assert!(check_allowed(
"src/deep/nested/file.rs",
&["src/deep".to_string()]
));
}

#[test]
fn test_multiple_patterns() {
let patterns = vec![
"src/secret/public.rs".to_string(),
"!src/secret/**".to_string(),
"src/**".to_string(),
];

// Should match general src pattern
assert!(check_allowed("src/main.rs", &patterns));

// Should be denied by secret pattern
assert!(!check_allowed("src/secret/private.rs", &patterns));

// Should be explicitly allowed despite being in secret dir
assert!(check_allowed("src/secret/public.rs", &patterns));
}

#[test]
fn test_default_behavior() {
// Test default allow (last pattern is negative)
assert!(check_allowed(
"random.txt",
&["src/**".to_string(), "!tests/**".to_string()]
));

// Test default deny (last pattern is positive)
assert!(!check_allowed(
"random.txt",
&["!src/**".to_string(), "src/allowed.rs".to_string()]
));
}

#[test]
fn test_pattern_priority() {
let patterns = vec![
"docs/internal/public/**".to_string(),
"!docs/internal/**".to_string(),
"docs/**".to_string(),
];

// Should match third pattern
assert!(check_allowed("docs/api.md", &patterns));

// Should be denied by second pattern
assert!(!check_allowed("docs/internal/secret.md", &patterns));

// Should be allowed by first pattern
assert!(check_allowed("docs/internal/public/readme.md", &patterns));
}

#[test]
fn test_directory_prefix() {
let patterns = vec!["src".to_string()];

assert!(check_allowed("src", &patterns));
assert!(check_allowed("src/test", &patterns));
assert!(check_allowed("src/another/test", &patterns));
}

#[test]
fn test_edge_cases() {
// Test root pattern
assert!(check_allowed("any/path", &["**".to_string()]));

// Test single negative pattern
assert!(!check_allowed("any/path", &["!**".to_string()]));

// Test pattern with just trailing wildcard
assert!(check_allowed("src/anything", &["src/*".to_string()]));
}
}
}
9 changes: 9 additions & 0 deletions website/contents/reference/configuration/parameters/check.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ This is expected to be a list of objects containing the following parameters:
| `patterns` | list of strings | Pattern of files to include (or exclude, if starting by `!`) in the check. Allows for glob patterns to be used. |
| `ignore` | list of strings | [Error codes](/reference/builtin-commands/config/check#error-codes) to ignore. |
| `select` | list of strings | [Error codes](/reference/builtin-commands/config/check#error-codes) to select. |
| `tags` | list of strings or objects | Tags to include in the check, and how to validate them. The elements of the list can be a string, in which case it is assumed to be a tag name to require, or a key-value pair where the value is a [Filter](github#filter-object) object. |

## Example

Expand All @@ -39,4 +40,12 @@ check:
- "C001" # Select all errors of type C001
- "M" # Select all errors of type M
- "P0" # Select all errors of type P

tags:
- should exist
- should match: 'val*'
- should exactly match:
exact: 'value'
- should be a number:
regex: '^\d+$'
```

0 comments on commit 368f16a

Please sign in to comment.