Skip to content

Commit 749f207

Browse files
refactor(rust-guard): add MinIntegrity::from_policy_str to unify duplicate parsers
Add a bidirectional MinIntegrity type by introducing from_policy_str(), the natural complement to as_str(). Refactor integrity_level_rank and integrity_for_level to use the new method, eliminating two identical 4-way case-insensitive if/else chains. Also add 14 new tests: 6 covering from_policy_str (known values, case-insensitivity, whitespace trimming, unknown inputs, roundtrip) and 8 covering extract_repo_from_item (all 5 lookup strategies + empty case + priority check), which previously had zero direct tests. Closes #7369 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 08bc2c4 commit 749f207

1 file changed

Lines changed: 130 additions & 28 deletions

File tree

  • guards/github-guard/rust-guard/src/labels

guards/github-guard/rust-guard/src/labels/helpers.rs

Lines changed: 130 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,24 @@ impl MinIntegrity {
109109
MinIntegrity::Merged => policy_integrity::MERGED,
110110
}
111111
}
112+
113+
/// Parse a policy integrity level string, case-insensitively.
114+
/// Returns `None` for unrecognised values.
115+
pub(crate) fn from_policy_str(level: &str) -> Option<Self> {
116+
use super::constants::policy_integrity as pi;
117+
let level = level.trim();
118+
if level.eq_ignore_ascii_case(pi::NONE) {
119+
Some(Self::None)
120+
} else if level.eq_ignore_ascii_case(pi::UNAPPROVED) {
121+
Some(Self::Unapproved)
122+
} else if level.eq_ignore_ascii_case(pi::APPROVED) {
123+
Some(Self::Approved)
124+
} else if level.eq_ignore_ascii_case(pi::MERGED) {
125+
Some(Self::Merged)
126+
} else {
127+
None
128+
}
129+
}
112130
}
113131

114132
#[derive(Debug, Clone, Default)]
@@ -462,22 +480,18 @@ fn effective_endorser_min_integrity<'a>(ctx: &'a PolicyContext) -> &'a str {
462480
/// Returns: 1 = none, 2 = unapproved, 3 = approved, 4 = merged.
463481
/// Unrecognised levels default to rank 3 (approved) with a warning log.
464482
fn integrity_level_rank(level: &str) -> u8 {
465-
use super::constants::policy_integrity as pi;
466-
let level = level.trim();
467-
if level.eq_ignore_ascii_case(pi::NONE) {
468-
1
469-
} else if level.eq_ignore_ascii_case(pi::UNAPPROVED) {
470-
2
471-
} else if level.eq_ignore_ascii_case(pi::APPROVED) {
472-
3
473-
} else if level.eq_ignore_ascii_case(pi::MERGED) {
474-
4
475-
} else {
476-
crate::log_warn(&format!(
477-
"integrity_level_rank: unrecognised level {:?}, defaulting to 'approved'",
478-
level
479-
));
480-
3 // unrecognised → safe default is "approved" (matches endorser_min_integrity default)
483+
match MinIntegrity::from_policy_str(level) {
484+
Some(MinIntegrity::None) => 1,
485+
Some(MinIntegrity::Unapproved) => 2,
486+
Some(MinIntegrity::Approved) => 3,
487+
Some(MinIntegrity::Merged) => 4,
488+
None => {
489+
crate::log_warn(&format!(
490+
"integrity_level_rank: unrecognised level {:?}, defaulting to 'approved'",
491+
level.trim()
492+
));
493+
3 // unrecognised → safe default is "approved" (matches endorser_min_integrity default)
494+
}
481495
}
482496
}
483497

@@ -496,18 +510,12 @@ fn cap_integrity(
496510

497511
/// Build the integrity `Vec<String>` for a given level name over a scope.
498512
fn integrity_for_level(level: &str, scope: &str, ctx: &PolicyContext) -> Vec<String> {
499-
use super::constants::policy_integrity as pi;
500-
let level = level.trim();
501-
if level.eq_ignore_ascii_case(pi::NONE) {
502-
none_integrity(scope, ctx)
503-
} else if level.eq_ignore_ascii_case(pi::UNAPPROVED) {
504-
reader_integrity(scope, ctx)
505-
} else if level.eq_ignore_ascii_case(pi::APPROVED) {
506-
writer_integrity(scope, ctx)
507-
} else if level.eq_ignore_ascii_case(pi::MERGED) {
508-
merged_integrity(scope, ctx)
509-
} else {
510-
none_integrity(scope, ctx) // safe default
513+
match MinIntegrity::from_policy_str(level) {
514+
Some(MinIntegrity::None) => none_integrity(scope, ctx),
515+
Some(MinIntegrity::Unapproved) => reader_integrity(scope, ctx),
516+
Some(MinIntegrity::Approved) => writer_integrity(scope, ctx),
517+
Some(MinIntegrity::Merged) => merged_integrity(scope, ctx),
518+
None => none_integrity(scope, ctx), // safe default
511519
}
512520
}
513521

@@ -2124,7 +2132,101 @@ mod tests {
21242132
assert_eq!(MinIntegrity::Merged.as_str(), policy_integrity::MERGED);
21252133
}
21262134

2135+
#[test]
2136+
fn test_min_integrity_from_policy_str_known_values() {
2137+
assert_eq!(MinIntegrity::from_policy_str("none"), Some(MinIntegrity::None));
2138+
assert_eq!(MinIntegrity::from_policy_str("unapproved"), Some(MinIntegrity::Unapproved));
2139+
assert_eq!(MinIntegrity::from_policy_str("approved"), Some(MinIntegrity::Approved));
2140+
assert_eq!(MinIntegrity::from_policy_str("merged"), Some(MinIntegrity::Merged));
2141+
}
2142+
2143+
#[test]
2144+
fn test_min_integrity_from_policy_str_case_insensitive() {
2145+
assert_eq!(MinIntegrity::from_policy_str("NONE"), Some(MinIntegrity::None));
2146+
assert_eq!(MinIntegrity::from_policy_str("None"), Some(MinIntegrity::None));
2147+
assert_eq!(MinIntegrity::from_policy_str("APPROVED"), Some(MinIntegrity::Approved));
2148+
assert_eq!(MinIntegrity::from_policy_str("Merged"), Some(MinIntegrity::Merged));
2149+
}
2150+
2151+
#[test]
2152+
fn test_min_integrity_from_policy_str_whitespace_trimmed() {
2153+
assert_eq!(MinIntegrity::from_policy_str(" none "), Some(MinIntegrity::None));
2154+
assert_eq!(MinIntegrity::from_policy_str(" MERGED "), Some(MinIntegrity::Merged));
2155+
}
2156+
2157+
#[test]
2158+
fn test_min_integrity_from_policy_str_unknown_returns_none() {
2159+
assert_eq!(MinIntegrity::from_policy_str("unknown"), None);
2160+
assert_eq!(MinIntegrity::from_policy_str(""), None);
2161+
assert_eq!(MinIntegrity::from_policy_str(" "), None);
2162+
}
2163+
2164+
#[test]
2165+
fn test_min_integrity_roundtrip() {
2166+
// from_policy_str(as_str()) should be identity
2167+
for level in [MinIntegrity::None, MinIntegrity::Unapproved, MinIntegrity::Approved, MinIntegrity::Merged] {
2168+
let s = level.as_str();
2169+
assert_eq!(MinIntegrity::from_policy_str(s), Some(level));
2170+
}
2171+
}
2172+
2173+
// =========================================================================
2174+
// Tests for extract_repo_from_item
21272175
// =========================================================================
2176+
2177+
#[test]
2178+
fn test_extract_repo_from_item_direct_full_name() {
2179+
let item = serde_json::json!({ "full_name": "owner/repo" });
2180+
assert_eq!(extract_repo_from_item(&item), "owner/repo");
2181+
}
2182+
2183+
#[test]
2184+
fn test_extract_repo_from_item_repository_full_name() {
2185+
let item = serde_json::json!({ "repository": { "full_name": "owner/repo" } });
2186+
assert_eq!(extract_repo_from_item(&item), "owner/repo");
2187+
}
2188+
2189+
#[test]
2190+
fn test_extract_repo_from_item_base_repo_for_prs() {
2191+
let item = serde_json::json!({ "base": { "repo": { "full_name": "owner/repo" } } });
2192+
assert_eq!(extract_repo_from_item(&item), "owner/repo");
2193+
}
2194+
2195+
#[test]
2196+
fn test_extract_repo_from_item_head_repo_for_fork_prs() {
2197+
let item = serde_json::json!({ "head": { "repo": { "full_name": "fork/repo" } } });
2198+
assert_eq!(extract_repo_from_item(&item), "fork/repo");
2199+
}
2200+
2201+
#[test]
2202+
fn test_extract_repo_from_item_repository_url_fallback() {
2203+
let item = serde_json::json!({ "repository_url": "https://api.github.com/repos/owner/repo" });
2204+
assert_eq!(extract_repo_from_item(&item), "owner/repo");
2205+
}
2206+
2207+
#[test]
2208+
fn test_extract_repo_from_item_html_url_fallback() {
2209+
let item = serde_json::json!({ "html_url": "https://github.com/owner/repo/issues/1" });
2210+
assert_eq!(extract_repo_from_item(&item), "owner/repo");
2211+
}
2212+
2213+
#[test]
2214+
fn test_extract_repo_from_item_returns_empty_when_no_repo_info() {
2215+
let item = serde_json::json!({ "id": 42, "title": "something" });
2216+
assert_eq!(extract_repo_from_item(&item), "");
2217+
}
2218+
2219+
#[test]
2220+
fn test_extract_repo_from_item_full_name_priority_over_nested() {
2221+
// full_name should win even when repository.full_name is also present
2222+
let item = serde_json::json!({
2223+
"full_name": "winner/repo",
2224+
"repository": { "full_name": "loser/repo" }
2225+
});
2226+
assert_eq!(extract_repo_from_item(&item), "winner/repo");
2227+
}
2228+
2229+
21282230
// Tests for reaction-based endorsement / disapproval helpers
21292231
// =========================================================================
21302232

0 commit comments

Comments
 (0)