From fd4ce3a0ffa221fc87736233abd7fcdafa363610 Mon Sep 17 00:00:00 2001 From: Jianjun Chen Date: Wed, 15 Jul 2026 07:23:19 +0800 Subject: [PATCH] fix(cli): resolve baseline paths and EXPLAIN per statement - Join git_repo when mapping baseline-discovered relative paths so audit works when CWD is the config directory (#24) - Run file_diff under repo_path for consistent --diff-aware behavior - Split multi-statement SQL via ogsql-parser and EXPLAIN each DML statement independently; failures no longer abort the whole file (#25) - clippy: use slice::contains in cr-core filter Closes #24 Closes #25 --- Cargo.lock | 1 + crates/cr-cli/Cargo.toml | 1 + crates/cr-cli/src/main.rs | 116 +++++++++++++++++++++++++++++------ crates/cr-core/src/filter.rs | 2 +- crates/cr-git/src/lib.rs | 7 ++- 5 files changed, 106 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5c79128..f9f4dc4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -831,6 +831,7 @@ dependencies = [ "gaussdb", "globset", "indicatif", + "ogsql-parser", "serde_json", "tokio", "toml", diff --git a/crates/cr-cli/Cargo.toml b/crates/cr-cli/Cargo.toml index 5ad59ba..dd22fb5 100644 --- a/crates/cr-cli/Cargo.toml +++ b/crates/cr-cli/Cargo.toml @@ -21,6 +21,7 @@ cr-config = { workspace = true } cr-db = { workspace = true } cr-git = { workspace = true } gaussdb = { workspace = true } +ogsql-parser = { workspace = true } clap = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } diff --git a/crates/cr-cli/src/main.rs b/crates/cr-cli/src/main.rs index 9985d0f..48ee549 100644 --- a/crates/cr-cli/src/main.rs +++ b/crates/cr-cli/src/main.rs @@ -360,8 +360,11 @@ fn discover_audit_files( tracing::warn!(baseline = %baseline, "相对于 baseline 未发现变更文件"); } let total = f.len(); - let filtered: Vec = - f.into_iter().filter(|cf| file_matches_project_type(cf, project_type)).map(|cf| cf.path).collect(); + let filtered: Vec = f + .into_iter() + .filter(|cf| file_matches_project_type(cf, project_type)) + .map(|cf| resolve_repo_relative_path(cf.path, repo_path)) + .collect(); let type_skipped = total - filtered.len(); let (filtered, exclude_skipped) = apply_exclude_filter(filtered, exclude_patterns, repo_path); @@ -809,7 +812,7 @@ fn run_all_projects( fn apply_diff_aware_filter( findings: Vec, baseline: Option<&str>, - _repo_path: &Path, + repo_path: &Path, ) -> Vec { use cr_audit_static::diff_aware::{filter_findings_to_diff, parse_hunks}; @@ -821,7 +824,9 @@ fn apply_diff_aware_filter( let mut file_hunks: std::collections::HashMap> = std::collections::HashMap::new(); let unique_files: std::collections::HashSet<&String> = findings.iter().map(|f| &f.file_path).collect(); for file in unique_files { - match cr_git::file_diff(baseline, file) { + let rel = Path::new(file).strip_prefix(repo_path).unwrap_or(Path::new(file)); + let rel_str = rel.to_string_lossy(); + match cr_git::file_diff(baseline, &rel_str, repo_path) { Ok(diff_text) => { let hunks = parse_hunks(&diff_text); if hunks.is_empty() { @@ -843,6 +848,15 @@ fn apply_diff_aware_filter( filtered } +#[must_use] +fn resolve_repo_relative_path(path: PathBuf, repo_path: &Path) -> PathBuf { + if path.is_absolute() { + path + } else { + repo_path.join(path) + } +} + #[allow(clippy::too_many_arguments)] pub(crate) async fn audit_files_async( files: &[PathBuf], @@ -1019,28 +1033,64 @@ async fn audit_sql_file( )); if let Some(conn) = db_conn { - if !is_explainable_dml(sql) { - tracing::debug!("跳过 EXPLAIN（非 DML 语句：CREATE/ALTER/DROP/SET/TRIGGER/...）"); - } else { - tracing::debug!("EXPLAIN 审核中"); - let timeout = db_config.map(|d| d.explain.timeout_seconds).unwrap_or(30); - match cr_db::execute_explain(conn.client(), sql, timeout).await { - Ok(explain_text) => match cr_audit_explain::analyze_explain_text(&explain_text, file_path) { - Ok(explain_findings) => findings.extend(explain_findings), - Err(e) => { - tracing::warn!(error = %e, "EXPLAIN 解析失败，跳过执行计划审核"); - } - }, + let timeout = db_config.map(|d| d.explain.timeout_seconds).unwrap_or(30); + findings.extend(explain_sql_statements(conn, sql, file_path, timeout).await); + } + + findings +} + +async fn explain_sql_statements( + conn: &cr_db::GaussDbConnection, + sql: &str, + file_path: &str, + timeout: u64, +) -> Vec { + let mut findings = Vec::new(); + let statements = explainable_statement_texts(sql); + if statements.is_empty() { + tracing::debug!("跳过 EXPLAIN（无可 EXPLAIN 的 DML 语句）"); + return findings; + } + + tracing::debug!(count = statements.len(), "EXPLAIN 审核中（按语句）"); + for (idx, stmt_sql) in statements.iter().enumerate() { + match cr_db::execute_explain(conn.client(), stmt_sql, timeout).await { + Ok(explain_text) => match cr_audit_explain::analyze_explain_text(&explain_text, file_path) { + Ok(explain_findings) => findings.extend(explain_findings), Err(e) => { - tracing::warn!(error = %e, "EXPLAIN 执行失败，该 SQL 仅静态审核"); + tracing::warn!( + error = %e, + stmt = idx + 1, + "EXPLAIN 解析失败，跳过该语句" + ); } + }, + Err(e) => { + tracing::warn!( + error = %e, + stmt = idx + 1, + "EXPLAIN 执行失败，跳过该语句" + ); } } } - findings } +#[must_use] +fn explainable_statement_texts(sql: &str) -> Vec { + let (stmt_infos, _errors) = ogsql_parser::Parser::parse_sql(sql); + if !stmt_infos.is_empty() { + return stmt_infos.into_iter().map(|si| si.sql_text).filter(|t| is_explainable_dml(t)).collect(); + } + if is_explainable_dml(sql) { + vec![sql.to_string()] + } else { + Vec::new() + } +} + fn is_explainable_dml(sql: &str) -> bool { for raw_line in sql.lines() { let trimmed = raw_line.trim(); @@ -1092,6 +1142,36 @@ mod tests { toml::from_str::(&s).unwrap().projects.get("test").unwrap().codeweb.clone().unwrap() } + #[test] + fn resolve_repo_relative_joins_relative_paths() { + let repo = Path::new("c2j/ogagila"); + let joined = resolve_repo_relative_path(PathBuf::from("sqls/a.sql"), repo); + assert_eq!(joined, PathBuf::from("c2j/ogagila/sqls/a.sql")); + } + + #[test] + fn resolve_repo_relative_keeps_absolute_paths() { + let abs = PathBuf::from("/tmp/repo/sqls/a.sql"); + let out = resolve_repo_relative_path(abs.clone(), Path::new("c2j/ogagila")); + assert_eq!(out, abs); + } + + #[test] + fn explainable_statement_texts_splits_multi_select() { + let sql = "SELECT 1 FROM t;\nSELECT * FROM rental LIMIT 1;\nCREATE TABLE x (id int);\n"; + let stmts = explainable_statement_texts(sql); + assert!(stmts.len() >= 2, "expected >=2 DML statements, got {stmts:?}"); + assert!(stmts.iter().all(|s| is_explainable_dml(s))); + assert!(stmts.iter().any(|s| s.to_uppercase().contains("RENTAL"))); + } + + #[test] + fn explainable_statement_texts_skips_ddl_only() { + let sql = "CREATE TABLE t (id int);\nSET search_path TO public;\n"; + let stmts = explainable_statement_texts(sql); + assert!(stmts.is_empty(), "DDL/SET only should yield no explainable stmts: {stmts:?}"); + } + #[test] fn test_augment_with_impact_no_config() { let cfg = cr_config::CodewebConfig::default(); diff --git a/crates/cr-core/src/filter.rs b/crates/cr-core/src/filter.rs index 89b9940..1fd314a 100644 --- a/crates/cr-core/src/filter.rs +++ b/crates/cr-core/src/filter.rs @@ -103,7 +103,7 @@ fn passes_filter( if let Some(expr) = category_filter { if let Some((mode, values)) = parse_filter_expr(expr) { let cat_str = finding.category.as_kebab_str(); - let matched = values.iter().any(|v| *v == cat_str); + let matched = values.contains(&cat_str); if mode == "exclude" && matched { return false; } diff --git a/crates/cr-git/src/lib.rs b/crates/cr-git/src/lib.rs index e59fed2..0b92932 100644 --- a/crates/cr-git/src/lib.rs +++ b/crates/cr-git/src/lib.rs @@ -177,11 +177,14 @@ pub fn changed_files(baseline: &str, repo_path: &Path) -> Result Result { - let output = Command::new("git").args(["diff", baseline, "--", file_path]).output()?; +pub fn file_diff(baseline: &str, file_path: &str, repo_path: &Path) -> Result { + let output = Command::new("git").args(["diff", baseline, "--", file_path]).current_dir(repo_path).output()?; if !output.status.success() { return Err(std::io::Error::other(format!("git diff failed: {}", String::from_utf8_lossy(&output.stderr))));