diff --git a/Cargo.lock b/Cargo.lock index 78ca90c265..a5da2600dc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1395,6 +1395,7 @@ dependencies = [ "sqlbuild-config", "sqlbuild-core", "sqlbuild-model-config", + "sqlbuild-pysource", "sqlbuild-sqltext", "tempfile", ] diff --git a/crates/sqlbuild-cli/src/argv/_helpers/normalization.rs b/crates/sqlbuild-cli/src/argv/_helpers/normalization.rs index f0b830b251..311d9ef353 100644 --- a/crates/sqlbuild-cli/src/argv/_helpers/normalization.rs +++ b/crates/sqlbuild-cli/src/argv/_helpers/normalization.rs @@ -5,8 +5,8 @@ use crate::argv::constants::{ COMMAND_DEST, CONCURRENCY_COMMANDS, CONCURRENCY_ENV_VAR, CURSOR_COMMANDS, DBT_COMMAND, DBT_DEBUG_COMMAND, DBT_PASSTHROUGH_COMMANDS, DBT_PASSTHROUGH_SEPARATOR, DBT_VERBOSE_OPTION, DBT_VERBOSE_OPTIONS, DEBUG_OPTION, INT_CONVERTER, JANITOR_COMMAND, NO_COLOR_OPTION, - POSITIONAL_SELECT_COMMANDS, SCOPE_COMMAND, SCOPE_DEFAULT_PAGE_SIZE, SCOPE_GLOBAL_SUMMARY, - SCOPE_KIND_DEST, TEST_COMMAND, + POSITIONAL_SELECT_COMMANDS, RENAME_COMMAND, SCOPE_COMMAND, SCOPE_DEFAULT_PAGE_SIZE, + SCOPE_GLOBAL_SUMMARY, SCOPE_KIND_DEST, TEST_COMMAND, }; use crate::argv::models::{Namespace, ParserSpec, Value}; @@ -96,6 +96,12 @@ impl Run<'_> { )); } } + if command == RENAME_COMMAND + && let Some(message) = rename_error(&namespace) + { + let rename: &ParserSpec = subcommand_parser(parser, RENAME_COMMAND).unwrap_or(parser); + return Err(self.error(rename, &message)); + } if command == SCOPE_COMMAND && let Some(message) = scope_error(parser, &namespace) { @@ -263,3 +269,28 @@ fn integer(value: &Value) -> i128 { i128::MAX }) } + +/// The usage error of a `sqb rename` without `--from-file` and without one full pair. +fn rename_error(namespace: &Namespace) -> Option { + if namespace.value("refactor_from_file") != Value::None + || namespace.value("refactor_new_name") != Value::None + { + return None; + } + let missing: &str = if namespace.value("refactor_target") == Value::None { + "TARGET, NEW_NAME" + } else { + "NEW_NAME" + }; + Some(format!("the following arguments are required: {missing}")) +} + +/// The parser of sub-command `name` below `parser`. +fn subcommand_parser<'a>(parser: &'a ParserSpec, name: &str) -> Option<&'a ParserSpec> { + parser + .actions + .iter() + .flat_map(|action| &action.parsers) + .find(|(command, _)| command == name) + .map(|(_, spec)| spec) +} diff --git a/crates/sqlbuild-cli/src/argv/command_table.json b/crates/sqlbuild-cli/src/argv/command_table.json index e47f29cad0..66c16460bf 100644 --- a/crates/sqlbuild-cli/src/argv/command_table.json +++ b/crates/sqlbuild-cli/src/argv/command_table.json @@ -4891,7 +4891,7 @@ { "kind": "store", "dest": "refactor_target", - "required": true, + "nargs": "?", "help": " or .; the model: and column: prefixes are optional", "metavar": "TARGET", "group": 0 @@ -4899,10 +4899,29 @@ { "kind": "store", "dest": "refactor_new_name", - "required": true, + "nargs": "?", "metavar": "NEW_NAME", "group": 0 }, + { + "kind": "store", + "dest": "refactor_more_moves", + "nargs": "*", + "default": [], + "help": "further model TARGET NEW_NAME pairs, renamed together with the first", + "metavar": "MORE", + "group": 0 + }, + { + "kind": "store", + "options": [ + "--from-file" + ], + "dest": "refactor_from_file", + "help": "read model TARGET NEW_NAME pairs from FILE, one per line; # starts a comment", + "metavar": "FILE", + "group": 1 + }, { "kind": "store_true", "options": [ diff --git a/crates/sqlbuild-cli/src/argv/constants.rs b/crates/sqlbuild-cli/src/argv/constants.rs index 5f8f0d494b..912372680d 100644 --- a/crates/sqlbuild-cli/src/argv/constants.rs +++ b/crates/sqlbuild-cli/src/argv/constants.rs @@ -51,6 +51,7 @@ pub(crate) const POSITIONAL_SELECT_COMMANDS: [&str; 8] = [ ]; pub(crate) const CONCURRENCY_COMMANDS: [&str; 5] = ["audit", "build", "load", "seed", "test"]; pub(crate) const CURSOR_COMMANDS: [&str; 3] = ["build", "load", "plan"]; +pub(crate) const RENAME_COMMAND: &str = "rename"; pub(crate) const SCOPE_COMMAND: &str = "scope"; pub(crate) const TEST_COMMAND: &str = "test"; pub(crate) const JANITOR_COMMAND: &str = "janitor"; diff --git a/crates/sqlbuild-cli/src/argv/tests/test_parse_cli_arguments.rs b/crates/sqlbuild-cli/src/argv/tests/test_parse_cli_arguments.rs index 6358fbeb11..4fa7bed300 100644 --- a/crates/sqlbuild-cli/src/argv/tests/test_parse_cli_arguments.rs +++ b/crates/sqlbuild-cli/src/argv/tests/test_parse_cli_arguments.rs @@ -88,6 +88,18 @@ fn given_help_version_or_rejected_arguments_when_parsing_then_exits_after_writin expected_exit: 2, expected_last_write: "sqb rules: error[C900]: the following arguments are required: rules_command\n\n", }, + ExitTestCase { + description: "a rename target without a new name stays a usage error", + argv: &["rename", "stg_orders"], + expected_exit: 2, + expected_last_write: "sqb rename: error[C900]: the following arguments are required: NEW_NAME\n\n", + }, + ExitTestCase { + description: "a rename without a target or a file stays a usage error", + argv: &["rename"], + expected_exit: 2, + expected_last_write: "sqb rename: error[C900]: the following arguments are required: TARGET, NEW_NAME\n\n", + }, ]; for test_case in test_cases { diff --git a/crates/sqlbuild-cli/src/dispatch/main/command_dispatch.rs b/crates/sqlbuild-cli/src/dispatch/main/command_dispatch.rs index c51b2ed43d..dcad84311c 100644 --- a/crates/sqlbuild-cli/src/dispatch/main/command_dispatch.rs +++ b/crates/sqlbuild-cli/src/dispatch/main/command_dispatch.rs @@ -9,7 +9,6 @@ use crate::dispatch::constants::{ LINEAGE_ROUTE, LOAD_ROUTE, PLAN_ROUTE, PLAYGROUND_ROUTE, QUERY_ROUTE, RECALL_COMMAND_NAME, RECALL_ROUTE, REFACTOR_ROUTE, RULES_ROUTE, SCOPE_ROUTE, SEED_ROUTE, SKILLS_ROUTE, TEST_ROUTE, }; -use crate::dispatch::main::unavailable_handler::unavailable_handler; use crate::dispatch::models::{CommandFacts, Dispatch, DispatchFailure, Route}; /// The route `facts` select, checked in the order the Python dispatcher checked them. @@ -21,11 +20,7 @@ pub fn command_dispatch(facts: &CommandFacts) -> Dispatch { let checked = |route: &'static Route, failure: Option| Dispatch::Run(route, failure); match command { - "rename" => checked( - &REFACTOR_ROUTE, - (!facts.refactor_target).then(|| unavailable_handler(command, "C950")), - ), - "mv" => run(&REFACTOR_ROUTE), + "rename" | "mv" => run(&REFACTOR_ROUTE), "contract" => checked( &CONTRACT_ROUTE, (!facts.contract_ready).then(|| { diff --git a/crates/sqlbuild-cli/src/dispatch/models.rs b/crates/sqlbuild-cli/src/dispatch/models.rs index 1eb5c45b67..5067a0f548 100644 --- a/crates/sqlbuild-cli/src/dispatch/models.rs +++ b/crates/sqlbuild-cli/src/dispatch/models.rs @@ -97,7 +97,6 @@ pub struct CommandFacts { pub command: Option, pub subcommand: Option, pub json: bool, - pub refactor_target: bool, pub from_target: bool, pub contract_ready: bool, pub format_diff: bool, diff --git a/crates/sqlbuild-cli/src/dispatch/tests/helpers.rs b/crates/sqlbuild-cli/src/dispatch/tests/helpers.rs index dddc39e942..6a923ff94c 100644 --- a/crates/sqlbuild-cli/src/dispatch/tests/helpers.rs +++ b/crates/sqlbuild-cli/src/dispatch/tests/helpers.rs @@ -7,7 +7,6 @@ pub(super) fn facts(command: &str, subcommand: Option<&str>) -> CommandFacts { CommandFacts { command: Some(command.to_owned()), subcommand: subcommand.map(str::to_owned), - refactor_target: true, from_target: true, contract_ready: true, ..CommandFacts::default() diff --git a/crates/sqlbuild-cli/src/dispatch/tests/test_command_dispatch.rs b/crates/sqlbuild-cli/src/dispatch/tests/test_command_dispatch.rs index 97bf5c9156..59b64f4eb7 100644 --- a/crates/sqlbuild-cli/src/dispatch/tests/test_command_dispatch.rs +++ b/crates/sqlbuild-cli/src/dispatch/tests/test_command_dispatch.rs @@ -89,22 +89,13 @@ fn given_parsed_command_when_dispatching_then_selects_python_dispatcher_route() expected_dispatch: runs(&SCENARIO_CAPTURE_ROUTE), }, CommandDispatchTestCase { - description: "rename without a target fails", - facts: CommandFacts { - refactor_target: false, - ..facts("rename", None) - }, - expected_dispatch: Dispatch::Run( - &REFACTOR_ROUTE, - Some(failure("rename command handler is unavailable", "C950")), - ), + description: "rename reads its pairs itself, so they may come only from a file", + facts: facts("rename", None), + expected_dispatch: runs(&REFACTOR_ROUTE), }, CommandDispatchTestCase { - description: "mv reads its pairs itself, so it may come only from a file", - facts: CommandFacts { - refactor_target: false, - ..facts("mv", None) - }, + description: "mv reads its pairs itself, so they may come only from a file", + facts: facts("mv", None), expected_dispatch: runs(&REFACTOR_ROUTE), }, CommandDispatchTestCase { diff --git a/crates/sqlbuild-pysource/src/import_rewrites/_helpers/import_statements.rs b/crates/sqlbuild-pysource/src/import_rewrites/_helpers/import_statements.rs new file mode 100644 index 0000000000..27b7b6d876 --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/_helpers/import_statements.rs @@ -0,0 +1,373 @@ +//! The import edits and refusals of one Python file. + +use ruff_python_ast::visitor::{Visitor, walk_expr, walk_stmt}; +use ruff_python_ast::{Alias, Expr, ExprCall, ModModule, Stmt, StmtImport, StmtImportFrom}; +use ruff_text_size::Ranged; + +use crate::import_rewrites::_helpers::module_paths::{ + dotted, dotted_parts, package_parts, resolve_relative, spell_from_target, +}; +use crate::import_rewrites::constants::{ + DYNAMIC_IMPORT_FUNCTIONS, FROM_KEYWORD, MODULE_SEPARATOR, STAR_IMPORT_NAME, +}; +use crate::import_rewrites::models::{ImportEdit, ImportRefusal, PythonSource}; +use crate::import_rewrites::types::{ModuleMoves, PackageBindings}; +use crate::module_parse::main::parse_python_module::parse_python_module; + +/// One file's text, path, packages before and after any move of the file, and module moves. +pub(crate) struct ImportFile<'a> { + pub(crate) path: &'a str, + pub(crate) contents: &'a str, + /// The package relative imports start from, where the file is now. + pub(crate) package: Vec, + /// The package relative imports start from, where the file will be. + pub(crate) new_package: Vec, + /// Each moved module's dotted parts, before and after. + pub(crate) moves: &'a ModuleMoves, + /// The names each project package's `__init__.py` binds, by package parts. + pub(crate) package_bindings: &'a PackageBindings, +} + +/// The edits and refusals of every import in `source`, which moves to `destination` if any. +pub(crate) fn source_rewrites( + source: &PythonSource, + destination: Option<&str>, + (moves, bindings): (&ModuleMoves, &PackageBindings), +) -> (Vec, Vec) { + let Ok(module) = parse_python_module(&source.contents) else { + let refusals: Vec = moves + .keys() + .find(|old| source.contents.contains(&dotted(old))) + .map(|old| ImportRefusal { + path: source.path.clone(), + line: 1, + column: 1, + reason: format!( + "names {} but is not valid Python, so its imports cannot be rewritten; fix \ + the file first", + dotted(old) + ), + }) + .into_iter() + .collect(); + return (Vec::new(), refusals); + }; + let file = ImportFile { + path: &source.path, + contents: &source.contents, + package: package_parts(&source.path), + new_package: package_parts(destination.unwrap_or(&source.path)), + moves, + package_bindings: bindings, + }; + file_rewrites(&file, &module) +} + +/// The edits and refusals of every import in `module`, in source order. +fn file_rewrites( + file: &ImportFile<'_>, + module: &ModModule, +) -> (Vec, Vec) { + let mut collector = Collector { + file, + edits: Vec::new(), + refusals: Vec::new(), + }; + for statement in &module.body { + collector.visit_stmt(statement); + } + (collector.edits, collector.refusals) +} + +struct Collector<'a, 'b> { + file: &'b ImportFile<'a>, + edits: Vec, + refusals: Vec, +} + +impl<'c> Visitor<'c> for Collector<'_, '_> { + fn visit_stmt(&mut self, statement: &'c Stmt) { + match statement { + Stmt::Import(import) => self.import(import), + Stmt::ImportFrom(import) => self.import_from(import), + _ => {} + } + walk_stmt(self, statement); + } + + fn visit_expr(&mut self, expression: &'c Expr) { + if let Expr::Call(call) = expression { + self.dynamic_import(call); + } + walk_expr(self, expression); + } +} + +impl Collector<'_, '_> { + fn refuse(&mut self, offset: usize, reason: String) { + let before: &str = self.file.contents.get(..offset).unwrap_or_default(); + let line_start: usize = before.rfind('\n').map_or(0, |index| index + 1); + self.refusals.push(ImportRefusal { + path: self.file.path.to_owned(), + line: before.matches('\n').count() + 1, + column: before[line_start..].chars().count() + 1, + reason, + }); + } + + fn edit(&mut self, (start, end): (usize, usize), replacement: String) { + self.edits.push(ImportEdit { + path: self.file.path.to_owned(), + start, + end, + replacement, + }); + } + + /// `import a.b.c as name` follows a moved module; without `as` the dotted name is refused. + fn import(&mut self, import: &StmtImport) { + for alias in &import.names { + let parts: Vec = dotted_parts(alias.name.as_str()); + let Some(moved) = self.file.moves.get(&parts) else { + continue; + }; + let start: usize = alias.name.range().start().to_usize(); + if alias.asname.is_none() { + self.refuse( + start, + format!( + "imports {} without an alias, and code that reads the dotted name \ + cannot be rewritten; import it with `as` or `from` first", + dotted(&parts) + ), + ); + continue; + } + if !self.matches_source(alias, &parts) { + self.refuse(start, unusual_spelling(&parts)); + continue; + } + self.edit((start, alias.name.range().end().to_usize()), dotted(moved)); + } + } + + fn matches_source(&self, alias: &Alias, parts: &[String]) -> bool { + let range = alias.name.range(); + self.file + .contents + .get(range.start().to_usize()..range.end().to_usize()) + == Some(dotted(parts).as_str()) + } + + /// `from import names` reads from the moved module, or its new package, after moves. + fn import_from(&mut self, import: &StmtImportFrom) { + let level: usize = import.level as usize; + let module: Vec = import + .module + .as_ref() + .map_or_else(Vec::new, |module| dotted_parts(module.as_str())); + let Some(base) = resolve_relative(&self.file.package, level, &module) else { + return; + }; + let start: usize = import.range().start().to_usize(); + let Some(new_base) = self.new_base(import, &base, start) else { + return; + }; + let moved_file: bool = self.file.package != self.file.new_package; + if new_base == base && (level == 0 || !moved_file) { + return; + } + let spelled: String = spell_from_target(&self.file.new_package, &new_base, level); + let original: String = format!( + "{}{}", + MODULE_SEPARATOR.to_string().repeat(level), + dotted(&module) + ); + if spelled == original { + return; + } + let Some(span) = self.module_span(import) else { + self.refuse(start, unusual_spelling(&base)); + return; + }; + self.edit(span, spelled); + } + + /// The module the statement must read from after the moves, or `None` when it is refused. + fn new_base( + &mut self, + import: &StmtImportFrom, + base: &[String], + start: usize, + ) -> Option> { + if let Some(moved) = self.file.moves.get(base) { + return Some(moved.clone()); + } + let star: bool = import + .names + .iter() + .any(|alias| alias.name.as_str() == STAR_IMPORT_NAME); + if star { + if let Some((old, _)) = self + .file + .moves + .iter() + .find(|(old, _)| old.split_last().is_some_and(|(_, parent)| parent == base)) + { + self.refuse( + start, + format!( + "star-imports the package of {}, which moves; import the names it \ + needs explicitly first", + dotted(old) + ), + ); + return None; + } + return Some(base.to_vec()); + } + let mut moved: Vec<(&Alias, &Vec)> = Vec::new(); + for alias in &import.names { + let mut parts: Vec = base.to_vec(); + parts.push(alias.name.as_str().to_owned()); + if let Some(new) = self.file.moves.get(&parts) { + moved.push((alias, new)); + } + } + let Some((_, first_moved)) = moved.first() else { + return Some(base.to_vec()); + }; + let package: &[String] = first_moved.split_last().map_or(&[], |(_, parent)| parent); + match submodule_refusal(self.file, base, (&moved, import.names.len()), package) { + None => Some(package.to_vec()), + Some(reason) => { + self.refuse(start, reason); + None + } + } + } + + /// The byte span of `` after `from`, when the source spells it plainly. + fn module_span(&self, import: &StmtImportFrom) -> Option<(usize, usize)> { + let contents: &str = self.file.contents; + let keyword_end: usize = import.range().start().to_usize() + FROM_KEYWORD.len(); + let rest: &str = contents.get(keyword_end..)?; + let start: usize = keyword_end + (rest.len() - rest.trim_start_matches([' ', '\t']).len()); + let end: usize = match &import.module { + Some(module) => module.range().end().to_usize(), + None => { + let dots: usize = contents + .get(start..)? + .chars() + .take_while(|character| *character == MODULE_SEPARATOR) + .count(); + start + dots + } + }; + let written: &str = contents.get(start..end)?; + let expected: String = format!( + "{}{}", + MODULE_SEPARATOR.to_string().repeat(import.level as usize), + import.module.as_ref().map_or("", |module| module.as_str()) + ); + (written == expected).then_some((start, end)) + } + + /// `importlib.import_module("...")` or `__import__("...")` naming a moved module. + fn dynamic_import(&mut self, call: &ExprCall) { + let function: Option<&str> = match call.func.as_ref() { + Expr::Name(name) => Some(name.id.as_str()), + Expr::Attribute(attribute) => Some(attribute.attr.as_str()), + _ => None, + }; + if !function.is_some_and(|function| DYNAMIC_IMPORT_FUNCTIONS.contains(&function)) { + return; + } + let Some(Expr::StringLiteral(literal)) = call.arguments.args.first() else { + return; + }; + let name: &str = literal.value.to_str(); + let relative: &str = name.trim_start_matches(MODULE_SEPARATOR); + let named: Option> = self + .file + .moves + .keys() + .find(|old| { + let dotted_old: String = dotted(old); + name == dotted_old + || name.starts_with(&format!("{dotted_old}{MODULE_SEPARATOR}")) + || (relative.len() < name.len() + && old.last().is_some_and(|stem| { + relative == stem + || relative.ends_with(&format!("{MODULE_SEPARATOR}{stem}")) + })) + }) + .cloned(); + if let Some(old) = named { + self.refuse( + call.range().start().to_usize(), + format!( + "imports {} dynamically by name, which cannot be rewritten safely; \ + change it to an import statement or update it by hand first", + dotted(&old) + ), + ); + } + } +} + +/// Why moved modules imported by name from `base` cannot follow to `package`, if they cannot. +fn submodule_refusal( + file: &ImportFile<'_>, + base: &[String], + (moved, names): (&[(&Alias, &Vec)], usize), + package: &[String], +) -> Option { + for (alias, new) in moved { + let name: &str = alias.name.as_str(); + let mut old: Vec = base.to_vec(); + old.push(name.to_owned()); + if file + .package_bindings + .get(base) + .is_some_and(|bound| bound.contains(name)) + { + return Some(format!( + "imports {name} from {}, whose __init__.py also binds {name}, so the import \ + would read the moved module {} instead after a rewrite; rewrite it by hand first", + dotted(base), + dotted(&old) + )); + } + if new.last().map(String::as_str) != Some(name) { + return Some(format!( + "imports the moved module {} by its name, which becomes {}; rewrite the import \ + by hand or import the module with an alias first", + dotted(&old), + dotted(new) + )); + } + if new.split_last().map(|(_, parent)| parent) != Some(package) { + return Some(format!( + "imports modules from {} that move to different packages; split the import \ + first", + dotted(base) + )); + } + } + (moved.len() != names).then(|| { + format!( + "imports a moved module from {} together with names that do not move with it; split \ + the import first", + dotted(base) + ) + }) +} + +fn unusual_spelling(parts: &[String]) -> String { + format!( + "imports {} with spacing or line breaks inside the module name; write it on one line \ + first", + dotted(parts) + ) +} diff --git a/crates/sqlbuild-pysource/src/import_rewrites/_helpers/mod.rs b/crates/sqlbuild-pysource/src/import_rewrites/_helpers/mod.rs new file mode 100644 index 0000000000..d1bced387f --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/_helpers/mod.rs @@ -0,0 +1,3 @@ +pub(crate) mod import_statements; +pub(crate) mod module_paths; +pub(crate) mod package_bindings; diff --git a/crates/sqlbuild-pysource/src/import_rewrites/_helpers/module_paths.rs b/crates/sqlbuild-pysource/src/import_rewrites/_helpers/module_paths.rs new file mode 100644 index 0000000000..33db0cf062 --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/_helpers/module_paths.rs @@ -0,0 +1,78 @@ +//! Dotted module names of project files, relative import resolution, and import spelling. + +use crate::import_rewrites::constants::{ + MODULE_SEPARATOR, PACKAGE_INIT_STEM, PATH_SEPARATOR, PYTHON_SUFFIX, SHALLOW_RELATIVE_LEVEL, +}; + +/// The dotted module a project-relative `.py` path imports as, or `None` for other files. +pub(crate) fn module_parts(path: &str) -> Option> { + let stem: &str = path.strip_suffix(PYTHON_SUFFIX)?; + let mut parts: Vec = stem.split(PATH_SEPARATOR).map(str::to_owned).collect(); + if parts.last().is_some_and(|last| last == PACKAGE_INIT_STEM) { + parts.pop(); + } + (!parts.is_empty() && parts.iter().all(|part| !part.is_empty())).then_some(parts) +} + +/// The package a file's relative imports start from: the folder parts of its path. +pub(crate) fn package_parts(path: &str) -> Vec { + let mut parts: Vec = path.split(PATH_SEPARATOR).map(str::to_owned).collect(); + parts.pop(); + parts +} + +/// Dotted parts of a dotted name. +pub(crate) fn dotted_parts(name: &str) -> Vec { + if name.is_empty() { + return Vec::new(); + } + name.split(MODULE_SEPARATOR).map(str::to_owned).collect() +} + +/// A dotted name from its parts. +pub(crate) fn dotted(parts: &[String]) -> String { + parts.join(&MODULE_SEPARATOR.to_string()) +} + +/// The module `from ` reads from `package`; `None` above the top package. +pub(crate) fn resolve_relative( + package: &[String], + level: usize, + module: &[String], +) -> Option> { + if level == 0 { + return Some(module.to_vec()); + } + let climb: usize = level - 1; + if climb >= package.len() { + return None; + } + let mut parts: Vec = package[..package.len() - climb].to_vec(); + parts.extend(module.iter().cloned()); + Some(parts) +} + +/// `target` as a `from` import in `package` spells it: relative when close, else absolute. +pub(crate) fn spell_from_target( + package: &[String], + target: &[String], + original_level: usize, +) -> String { + if original_level == 0 { + return dotted(target); + } + let common: usize = package + .iter() + .zip(target) + .take_while(|(left, right)| left == right) + .count(); + let level: usize = package.len() - common + 1; + if common == 0 || level > original_level.max(SHALLOW_RELATIVE_LEVEL) { + return dotted(target); + } + format!( + "{}{}", + MODULE_SEPARATOR.to_string().repeat(level), + dotted(&target[common..]) + ) +} diff --git a/crates/sqlbuild-pysource/src/import_rewrites/_helpers/package_bindings.rs b/crates/sqlbuild-pysource/src/import_rewrites/_helpers/package_bindings.rs new file mode 100644 index 0000000000..1ba5fca3df --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/_helpers/package_bindings.rs @@ -0,0 +1,98 @@ +//! The names project packages bind in their `__init__.py`. + +use ruff_python_ast::{Alias, Expr, Stmt}; + +use crate::import_rewrites::_helpers::module_paths::module_parts; +use crate::import_rewrites::constants::{MODULE_SEPARATOR, PACKAGE_INIT_FILE}; +use crate::import_rewrites::models::PythonSource; +use crate::import_rewrites::types::PackageBindings; +use crate::module_parse::main::parse_python_module::parse_python_module; + +/// Names each parseable `__init__.py` among `sources` binds at module level, by package parts. +pub(crate) fn package_bindings(sources: &[PythonSource]) -> PackageBindings { + let mut bindings: PackageBindings = PackageBindings::new(); + for source in sources { + let is_init: bool = source + .path + .rsplit('/') + .next() + .is_some_and(|name| name == PACKAGE_INIT_FILE); + let (true, Some(package), Ok(module)) = ( + is_init, + module_parts(&source.path), + parse_python_module(&source.contents), + ) else { + continue; + }; + bindings.insert(package, module.body.iter().flat_map(bound_names).collect()); + } + bindings +} + +/// The names `statement` binds at module level, through `if`, `try` and `with` blocks. +fn bound_names(statement: &Stmt) -> Vec { + match statement { + Stmt::FunctionDef(function) => vec![function.name.as_str().to_owned()], + Stmt::ClassDef(class) => vec![class.name.as_str().to_owned()], + Stmt::Assign(assign) => assign.targets.iter().flat_map(target_names).collect(), + Stmt::AnnAssign(assign) => target_names(&assign.target), + Stmt::AugAssign(assign) => target_names(&assign.target), + Stmt::Import(import) => import.names.iter().map(imported_module_binding).collect(), + Stmt::ImportFrom(import) => import + .names + .iter() + .map(|alias| { + alias + .asname + .as_ref() + .unwrap_or(&alias.name) + .as_str() + .to_owned() + }) + .collect(), + Stmt::If(branch) => branch + .body + .iter() + .chain( + branch + .elif_else_clauses + .iter() + .flat_map(|clause| &clause.body), + ) + .flat_map(bound_names) + .collect(), + Stmt::Try(block) => block + .body + .iter() + .chain(&block.orelse) + .chain(&block.finalbody) + .flat_map(bound_names) + .collect(), + Stmt::With(block) => block.body.iter().flat_map(bound_names).collect(), + _ => Vec::new(), + } +} + +/// The name `import a.b [as c]` binds: `c`, or `a`. +fn imported_module_binding(alias: &Alias) -> String { + match &alias.asname { + Some(asname) => asname.as_str().to_owned(), + None => alias + .name + .as_str() + .split(MODULE_SEPARATOR) + .next() + .unwrap_or_default() + .to_owned(), + } +} + +/// The plain names an assignment target binds. +fn target_names(target: &Expr) -> Vec { + match target { + Expr::Name(name) => vec![name.id.as_str().to_owned()], + Expr::Tuple(tuple) => tuple.elts.iter().flat_map(target_names).collect(), + Expr::List(list) => list.elts.iter().flat_map(target_names).collect(), + _ => Vec::new(), + } +} diff --git a/crates/sqlbuild-pysource/src/import_rewrites/constants.rs b/crates/sqlbuild-pysource/src/import_rewrites/constants.rs new file mode 100644 index 0000000000..926577e3dc --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/constants.rs @@ -0,0 +1,20 @@ +//! Names, suffixes and limits of Python import rewriting. + +/// Separator of dotted module names. +pub(crate) const MODULE_SEPARATOR: char = '.'; +/// Separator of project-relative POSIX paths. +pub(crate) const PATH_SEPARATOR: char = '/'; +/// The suffix of Python module files. +pub(crate) const PYTHON_SUFFIX: &str = ".py"; +/// The stem of a package's own module file. +pub(crate) const PACKAGE_INIT_STEM: &str = "__init__"; +/// The file name of a package's own module. +pub(crate) const PACKAGE_INIT_FILE: &str = "__init__.py"; +/// The alias name of a star import. +pub(crate) const STAR_IMPORT_NAME: &str = "*"; +/// The keyword that starts a `from ... import` statement. +pub(crate) const FROM_KEYWORD: &str = "from"; +/// The deepest relative import a rewrite writes when the original was shallower. +pub(crate) const SHALLOW_RELATIVE_LEVEL: usize = 2; +/// Built-in and `importlib` functions that import a module named by a string. +pub(crate) const DYNAMIC_IMPORT_FUNCTIONS: [&str; 2] = ["__import__", "import_module"]; diff --git a/crates/sqlbuild-pysource/src/import_rewrites/main/mod.rs b/crates/sqlbuild-pysource/src/import_rewrites/main/mod.rs new file mode 100644 index 0000000000..fefdff24fa --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/main/mod.rs @@ -0,0 +1 @@ +pub mod python_import_rewrites; diff --git a/crates/sqlbuild-pysource/src/import_rewrites/main/python_import_rewrites.rs b/crates/sqlbuild-pysource/src/import_rewrites/main/python_import_rewrites.rs new file mode 100644 index 0000000000..5e01b3164d --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/main/python_import_rewrites.rs @@ -0,0 +1,44 @@ +//! Rewrite the Python imports of modules that move, or say why one cannot be rewritten. + +use crate::import_rewrites::_helpers::import_statements::source_rewrites; +use crate::import_rewrites::_helpers::module_paths::module_parts; +use crate::import_rewrites::_helpers::package_bindings::package_bindings; +use crate::import_rewrites::constants::STAR_IMPORT_NAME; +use crate::import_rewrites::models::{ImportRewrites, ModuleMove, PythonSource}; +use crate::import_rewrites::types::ModuleMoves; + +/// Byte-offset import edits `moves` need in every source, and the imports they cannot rewrite. +#[must_use] +pub fn python_import_rewrites(sources: &[PythonSource], moves: &[ModuleMove]) -> ImportRewrites { + let modules: ModuleMoves = moves + .iter() + .filter_map(|item| Some((module_parts(&item.from)?, module_parts(&item.to)?))) + .filter(|(old, new)| old != new) + .collect(); + let mut rewrites = ImportRewrites::default(); + if modules.is_empty() { + return rewrites; + } + let bindings = package_bindings(sources); + let names: Vec<&str> = modules + .keys() + .filter_map(|old| old.last().map(String::as_str)) + .collect(); + let mut ordered: Vec<&PythonSource> = sources.iter().collect(); + ordered.sort_by(|left, right| left.path.cmp(&right.path)); + for source in ordered { + let destination: Option<&str> = moves + .iter() + .find(|item| item.from == source.path) + .map(|item| item.to.as_str()); + let mentions: bool = names.iter().any(|name| source.contents.contains(name)) + || source.contents.contains(STAR_IMPORT_NAME); + if destination.is_none() && !mentions { + continue; + } + let (edits, refusals) = source_rewrites(source, destination, (&modules, &bindings)); + rewrites.edits.extend(edits); + rewrites.refusals.extend(refusals); + } + rewrites +} diff --git a/crates/sqlbuild-pysource/src/import_rewrites/mod.rs b/crates/sqlbuild-pysource/src/import_rewrites/mod.rs new file mode 100644 index 0000000000..e68542ff38 --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/mod.rs @@ -0,0 +1,8 @@ +pub(crate) mod _helpers; +pub(crate) mod constants; +pub mod main; +pub mod models; +pub(crate) mod types; + +#[cfg(test)] +mod tests; diff --git a/crates/sqlbuild-pysource/src/import_rewrites/models.rs b/crates/sqlbuild-pysource/src/import_rewrites/models.rs new file mode 100644 index 0000000000..54832ae52e --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/models.rs @@ -0,0 +1,42 @@ +//! Python sources, module moves, and the import edits and refusals a move needs. + +/// One project Python file: its project-relative POSIX path and its text. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct PythonSource { + pub path: String, + pub contents: String, +} + +/// One Python module file moving from one project-relative POSIX path to another. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ModuleMove { + pub from: String, + pub to: String, +} + +/// One replacement in a Python file, in byte offsets of the file before any edit. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ImportEdit { + pub path: String, + pub start: usize, + pub end: usize, + pub replacement: String, +} + +/// An import of a moved module that cannot be rewritten safely, and why. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ImportRefusal { + pub path: String, + /// The 1-based line. + pub line: usize, + /// The 1-based column in characters. + pub column: usize, + pub reason: String, +} + +/// Every import edit and refusal module moves need, in file then source order. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct ImportRewrites { + pub edits: Vec, + pub refusals: Vec, +} diff --git a/crates/sqlbuild-pysource/src/import_rewrites/tests.rs b/crates/sqlbuild-pysource/src/import_rewrites/tests.rs new file mode 100644 index 0000000000..8c10f152de --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/tests.rs @@ -0,0 +1,6 @@ +#[path = "tests/helpers.rs"] +mod helpers; +#[path = "tests/test_python_import_rewrites.rs"] +mod python_import_rewrites; +#[path = "tests/test_types.rs"] +mod test_types; diff --git a/crates/sqlbuild-pysource/src/import_rewrites/tests/helpers.rs b/crates/sqlbuild-pysource/src/import_rewrites/tests/helpers.rs new file mode 100644 index 0000000000..c48da490b3 --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/tests/helpers.rs @@ -0,0 +1,12 @@ +use crate::import_rewrites::models::ImportEdit; + +/// `contents` with `edits` applied back to front. +pub(crate) fn applied(contents: &str, edits: &[&ImportEdit]) -> String { + let mut text: String = contents.to_owned(); + let mut ordered: Vec<&&ImportEdit> = edits.iter().collect(); + ordered.sort_by_key(|edit| std::cmp::Reverse(edit.start)); + for edit in ordered { + text.replace_range(edit.start..edit.end, &edit.replacement); + } + text +} diff --git a/crates/sqlbuild-pysource/src/import_rewrites/tests/test_python_import_rewrites.rs b/crates/sqlbuild-pysource/src/import_rewrites/tests/test_python_import_rewrites.rs new file mode 100644 index 0000000000..3b37334203 --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/tests/test_python_import_rewrites.rs @@ -0,0 +1,285 @@ +use crate::import_rewrites::main::python_import_rewrites::python_import_rewrites; +use crate::import_rewrites::models::{ImportEdit, ImportRewrites, ModuleMove, PythonSource}; +use crate::import_rewrites::tests::helpers::applied; +use crate::import_rewrites::tests::test_types::ImportRewriteTestCase; + +const CENTS: &str = "models/staging/_sqlbuild/_macros/cents.py"; +const MOVED_CENTS: &str = "models/finance/_sqlbuild/_macros/cents.py"; +const CENTS_MOVE: &[(&str, &str)] = &[(CENTS, MOVED_CENTS)]; + +#[test] +fn given_moved_modules_when_rewriting_imports_then_each_import_follows_or_is_refused() { + let test_cases = [ + ImportRewriteTestCase { + description: "a direct from-import follows the module", + sources: &[ + (CENTS, "def to_cents(x):\n return x\n"), + ( + "python/checks/amounts.py", + "from models.staging._sqlbuild._macros.cents import to_cents\n", + ), + ], + moves: CENTS_MOVE, + expected_sources: &[( + "python/checks/amounts.py", + "from models.finance._sqlbuild._macros.cents import to_cents\n", + )], + expected_refusals: &[], + }, + ImportRewriteTestCase { + description: "an aliased module import and a grouped import follow the module", + sources: &[( + "python/checks/amounts.py", + "import models.staging._sqlbuild._macros.cents as cents_module\n\ + from models.staging._sqlbuild._macros.cents import (\n to_cents,\n \ + to_dollars,\n)\n\n\ndef check():\n import models.staging._sqlbuild._macros.cents \ + as inner\n return inner\n", + )], + moves: CENTS_MOVE, + expected_sources: &[( + "python/checks/amounts.py", + "import models.finance._sqlbuild._macros.cents as cents_module\n\ + from models.finance._sqlbuild._macros.cents import (\n to_cents,\n \ + to_dollars,\n)\n\n\ndef check():\n import models.finance._sqlbuild._macros.cents \ + as inner\n return inner\n", + )], + expected_refusals: &[], + }, + ImportRewriteTestCase { + description: "a module imported by name from its package moves the package", + sources: &[( + "python/checks/amounts.py", + "from models.staging._sqlbuild._macros import cents\n", + )], + moves: CENTS_MOVE, + expected_sources: &[( + "python/checks/amounts.py", + "from models.finance._sqlbuild._macros import cents\n", + )], + expected_refusals: &[], + }, + ImportRewriteTestCase { + description: "relative imports that would climb far become absolute", + sources: &[ + ( + CENTS, + "from .rates import RATE\nfrom . import rounding\n\n\n\ + def to_cents(x):\n return x * RATE\n", + ), + ("models/staging/_sqlbuild/_macros/rates.py", "RATE = 100\n"), + ( + "models/staging/_sqlbuild/_macros/totals.py", + "from .cents import to_cents\n", + ), + ], + moves: CENTS_MOVE, + expected_sources: &[ + ( + CENTS, + "from models.staging._sqlbuild._macros.rates import RATE\n\ + from models.staging._sqlbuild._macros import rounding\n\n\n\ + def to_cents(x):\n return x * RATE\n", + ), + ( + "models/staging/_sqlbuild/_macros/totals.py", + "from models.finance._sqlbuild._macros.cents import to_cents\n", + ), + ], + expected_refusals: &[], + }, + ImportRewriteTestCase { + description: "a sibling move keeps a short relative import", + sources: &[( + "macros/money/totals.py", + "from ..money.cents import to_cents\nfrom .cents import to_dollars\n", + )], + moves: &[("macros/money/cents.py", "macros/text/cents.py")], + expected_sources: &[( + "macros/money/totals.py", + "from ..text.cents import to_cents\nfrom ..text.cents import to_dollars\n", + )], + expected_refusals: &[], + }, + ImportRewriteTestCase { + description: "imports of modules that stay are left alone", + sources: &[( + "python/checks/amounts.py", + "import os\nfrom models.staging._sqlbuild._macros.rates import RATE\n", + )], + moves: CENTS_MOVE, + expected_sources: &[], + expected_refusals: &[], + }, + ImportRewriteTestCase { + description: "an import without an alias is refused", + sources: &[( + "python/checks/amounts.py", + "import os\nimport models.staging._sqlbuild._macros.cents\n", + )], + moves: CENTS_MOVE, + expected_sources: &[], + expected_refusals: &[( + "python/checks/amounts.py:2:8", + "imports models.staging._sqlbuild._macros.cents without an alias", + )], + }, + ImportRewriteTestCase { + description: "a dynamic import naming the module is refused", + sources: &[( + "python/checks/amounts.py", + "import importlib\n\nCENTS = importlib.import_module(\ + \"models.staging._sqlbuild._macros.cents\")\n", + )], + moves: CENTS_MOVE, + expected_sources: &[], + expected_refusals: &[( + "python/checks/amounts.py:3:9", + "imports models.staging._sqlbuild._macros.cents dynamically by name", + )], + }, + ImportRewriteTestCase { + description: "a star import from the moved module's package is refused", + sources: &[( + "python/checks/amounts.py", + "from models.staging._sqlbuild._macros import *\n", + )], + moves: CENTS_MOVE, + expected_sources: &[], + expected_refusals: &[( + "python/checks/amounts.py:1:1", + "star-imports the package of models.staging._sqlbuild._macros.cents", + )], + }, + ImportRewriteTestCase { + description: "a star import from the moved module itself follows it", + sources: &[( + "python/checks/amounts.py", + "from models.staging._sqlbuild._macros.cents import *\n", + )], + moves: CENTS_MOVE, + expected_sources: &[( + "python/checks/amounts.py", + "from models.finance._sqlbuild._macros.cents import *\n", + )], + expected_refusals: &[], + }, + ImportRewriteTestCase { + description: "a moved module imported beside a name that stays is refused", + sources: &[( + "python/checks/amounts.py", + "from models.staging._sqlbuild._macros import cents, rates\n", + )], + moves: CENTS_MOVE, + expected_sources: &[], + expected_refusals: &[( + "python/checks/amounts.py:1:1", + "imports a moved module from models.staging._sqlbuild._macros together with names", + )], + }, + ImportRewriteTestCase { + description: "a moved module whose package binds its name is refused", + sources: &[ + ( + "models/staging/_sqlbuild/_macros/__init__.py", + "from .cents import cents\n", + ), + ( + "python/checks/amounts.py", + "import os\nfrom models.staging._sqlbuild._macros import cents\n", + ), + ], + moves: CENTS_MOVE, + expected_sources: &[( + "models/staging/_sqlbuild/_macros/__init__.py", + "from models.finance._sqlbuild._macros.cents import cents\n", + )], + expected_refusals: &[( + "python/checks/amounts.py:2:1", + "imports cents from models.staging._sqlbuild._macros, whose __init__.py also \ + binds cents", + )], + }, + ImportRewriteTestCase { + description: "a module imported by a name the move changes is refused", + sources: &[( + "python/checks/amounts.py", + "from models.staging._sqlbuild._macros import cents\n", + )], + moves: &[(CENTS, "models/finance/_sqlbuild/_macros/money.py")], + expected_sources: &[], + expected_refusals: &[( + "python/checks/amounts.py:1:1", + "imports the moved module models.staging._sqlbuild._macros.cents by its name, \ + which becomes models.finance._sqlbuild._macros.money", + )], + }, + ]; + for test_case in test_cases { + let sources: Vec = test_case + .sources + .iter() + .map(|(path, contents)| PythonSource { + path: (*path).to_owned(), + contents: (*contents).to_owned(), + }) + .collect(); + let moves: Vec = test_case + .moves + .iter() + .map(|(from, to)| ModuleMove { + from: (*from).to_owned(), + to: (*to).to_owned(), + }) + .collect(); + + let rewrites: ImportRewrites = python_import_rewrites(&sources, &moves); + + let rewritten: Vec<(String, String)> = sources + .iter() + .filter_map(|source| { + let edits: Vec<&ImportEdit> = rewrites + .edits + .iter() + .filter(|edit| edit.path == source.path) + .collect(); + (!edits.is_empty()) + .then(|| (source.path.clone(), applied(&source.contents, &edits))) + }) + .collect(); + let refusals: Vec<(String, bool)> = rewrites + .refusals + .iter() + .zip(test_case.expected_refusals) + .map(|(refusal, (_, reason))| { + ( + format!("{}:{}:{}", refusal.path, refusal.line, refusal.column), + refusal.reason.starts_with(reason), + ) + }) + .collect(); + assert_eq!( + rewritten, + test_case + .expected_sources + .iter() + .map(|(path, contents)| ((*path).to_owned(), (*contents).to_owned())) + .collect::>(), + "{}", + test_case.description + ); + assert_eq!( + (rewrites.refusals.len(), refusals), + ( + test_case.expected_refusals.len(), + test_case + .expected_refusals + .iter() + .map(|(location, _)| ((*location).to_owned(), true)) + .collect::>() + ), + "{}: {:?}", + test_case.description, + rewrites.refusals + ); + } +} diff --git a/crates/sqlbuild-pysource/src/import_rewrites/tests/test_types.rs b/crates/sqlbuild-pysource/src/import_rewrites/tests/test_types.rs new file mode 100644 index 0000000000..19496a8e5f --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/tests/test_types.rs @@ -0,0 +1,12 @@ +/// Python sources and module moves, and the rewritten sources or refusals they give. +pub(crate) struct ImportRewriteTestCase { + pub(crate) description: &'static str, + /// `(path, contents)` of each project Python file. + pub(crate) sources: &'static [(&'static str, &'static str)], + /// `(from, to)` of each moved module file. + pub(crate) moves: &'static [(&'static str, &'static str)], + /// `(path, contents)` of each source an edit changes, after the edits. + pub(crate) expected_sources: &'static [(&'static str, &'static str)], + /// `path:line:column` and the start of the reason of each refusal. + pub(crate) expected_refusals: &'static [(&'static str, &'static str)], +} diff --git a/crates/sqlbuild-pysource/src/import_rewrites/types.rs b/crates/sqlbuild-pysource/src/import_rewrites/types.rs new file mode 100644 index 0000000000..00d703e107 --- /dev/null +++ b/crates/sqlbuild-pysource/src/import_rewrites/types.rs @@ -0,0 +1,8 @@ +//! Lookup tables of Python import rewriting. + +use std::collections::{BTreeMap, BTreeSet}; + +/// Each moved module's dotted parts, before and after. +pub(crate) type ModuleMoves = BTreeMap, Vec>; +/// The names each project package's `__init__.py` binds, by package parts. +pub(crate) type PackageBindings = BTreeMap, BTreeSet>; diff --git a/crates/sqlbuild-pysource/src/lib.rs b/crates/sqlbuild-pysource/src/lib.rs index 2faf6d791c..56220f8303 100644 --- a/crates/sqlbuild-pysource/src/lib.rs +++ b/crates/sqlbuild-pysource/src/lib.rs @@ -4,6 +4,7 @@ pub(crate) mod cpython_tree; pub mod definition_locations; +pub mod import_rewrites; pub mod literal_sql; pub mod macro_modules; pub mod module_parse; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs b/crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs index 8a2589e5e1..fe254c55e8 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs @@ -519,7 +519,6 @@ pub(crate) fn command_facts(args: &Bound<'_, PyAny>) -> PyResult { subcommand: optional_text(&attr(subcommand_attribute)?)?, command, json: flag("json")?, - refactor_target: present("refactor_target")?, from_target: present("from_target")?, contract_ready: present("contract_command")? && present("contract_from")?, format_diff: flag("format_diff")?, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs b/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs index 73f7aced98..c2496e31fd 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/cli/entry.rs @@ -208,7 +208,7 @@ fn write_compile_machine_error( print_object(&document, None, None) } -/// With `sqb recall … --json`, `sqb plan --json` or `sqb janitor --json` (before it printed its own document), the failure as `{"error": {"code", "message", "help"}}` on stdout, so callers parsing the output read the error instead of failing to parse it. +/// With `sqb recall … --json`, `--json` on `sqb plan`, `sqb rename` or `sqb mv`, or `sqb janitor --json` (before it printed its own document), the failure as `{"error": {"code", "message", "help"}}` on stdout, so callers parsing the output read the error instead of failing to parse it. fn write_machine_error( invocation: &Invocation<'_>, error: &Bound<'_, PyAny>, @@ -224,7 +224,10 @@ fn write_machine_error( } else if invocation.command_is("janitor") { invocation.flag("json")? && !take_machine_output_written() } else { - invocation.command_is("plan") && invocation.flag("json")? + (invocation.command_is("plan") + || invocation.command_is("rename") + || invocation.command_is("mv")) + && invocation.flag("json")? }; if !json_output { return Ok(()); diff --git a/crates/sqlbuild-python/src/bindings/_helpers/refactor/offline_compile.rs b/crates/sqlbuild-python/src/bindings/_helpers/refactor/offline_compile.rs index 81a1fc8644..0ff7c2451d 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/refactor/offline_compile.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/refactor/offline_compile.rs @@ -32,18 +32,16 @@ fn matches_any_class(py: Python<'_>, error: &PyErr, classes: &[ErrorId]) -> PyRe Ok(false) } -/// A compile failure as the one error diagnostic it reports. +/// A compile failure, an exception rather than a native record, as its one error diagnostic. fn compile_error_diagnostic<'py>(py: Python<'py>, error: &PyErr) -> PyResult> { let value: Bound<'py, PyAny> = error.value(py).clone().into_any(); - let code: Bound<'py, PyAny> = if value.hasattr("code")? { - native_field(&value, "code")?.str()?.into_any() - } else { - PyString::new(py, DEFAULT_ERROR_CODE).into_any() + let code: Bound<'py, PyAny> = match value.getattr_opt("code")? { + Some(code) if !code.is_none() => code.str()?.into_any(), + _ => PyString::new(py, DEFAULT_ERROR_CODE).into_any(), }; - let message: Bound<'py, PyAny> = if value.hasattr("message")? { - native_field(&value, "message")?.str()?.into_any() - } else { - value.str()?.into_any() + let message: Bound<'py, PyAny> = match value.getattr_opt("message")? { + Some(message) if !message.is_none() => message.str()?.into_any(), + _ => value.str()?.into_any(), }; new_record_with( py, @@ -63,12 +61,21 @@ fn compile_error_diagnostic<'py>(py: Python<'py>, error: &PyErr) -> PyResult(value: &Bound<'py, PyAny>, name: &str) -> PyResult> { + Ok(value + .getattr_opt(name)? + .unwrap_or_else(|| value.py().None().into_bound(value.py()))) +} + /// A SQL-test planning failure as the test-phase error diagnostic it reports. fn planning_error_diagnostic<'py>(py: Python<'py>, error: &PyErr) -> PyResult> { let value: Bound<'py, PyAny> = error.value(py).clone().into_any(); let location: Bound<'py, PyAny> = if value.is_instance(&error_class(py, ErrorId::SqlTestReference)?)? { - native_field(&value, "location")? + value + .getattr_opt("location")? + .unwrap_or_else(|| py.None().into_bound(py)) } else { py.None().into_bound(py) }; @@ -81,9 +88,9 @@ fn planning_error_diagnostic<'py>(py: Python<'py>, error: &PyErr) -> PyResult( }; let mut errors: Vec> = error_diagnostics(native_field(&analysis, "diagnostics")?)?; - if errors.is_empty() { + let rule_phase: Bound<'py, PyAny> = enum_member(py, RecordId::DiagnosticPhase, "RULE")?; + let mut only_rule_failures: bool = true; + for error in &errors { + only_rule_failures &= native_field(error, "phase")?.is(&rule_phase); + } + if only_rule_failures { let planned: PyResult> = static_sql_test_planning_diagnostics( py, Some(keywords( diff --git a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs index 1e640cde14..ed138bf233 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs @@ -14,13 +14,16 @@ use pyo3::{pyfunction, wrap_pyfunction}; use sqlbuild_analysis::lineage_command::models::LineageKey; use sqlbuild_core::cli_text::main::format_count_noun::format_count_noun; use sqlbuild_core::cli_text::models::CliStyle; +use sqlbuild_refactor::refactor_command::main::compare_with_baseline::compare_with_baseline; use sqlbuild_refactor::refactor_command::main::layer_move_suggestion::layer_move_suggestion; -use sqlbuild_refactor::refactor_command::main::move_pairs::move_pairs; -use sqlbuild_refactor::refactor_command::main::refactor_request_for_target::refactor_request_for_target; +use sqlbuild_refactor::refactor_command::main::refactor_pairs::refactor_pairs; +use sqlbuild_refactor::refactor_command::main::refactor_requests::refactor_requests; +use sqlbuild_refactor::refactor_command::main::render_project_failure_json::render_project_failure_json; use sqlbuild_refactor::refactor_command::main::render_refactor_json::render_refactor_json; use sqlbuild_refactor::refactor_command::main::render_refactor_text::render_refactor_text; use sqlbuild_refactor::refactor_command::models::{ - LayerMove, MoveArguments, RefactorDiagnostic, RefactorStatus, RefactorTargetRequest, + BaselineComparison, LayerMove, PairArguments, RefactorDiagnostic, RefactorStatus, + RefactorTargetRequest, }; use sqlbuild_refactor::refactoring::errors::{RefactorError, RefactorErrorKind}; use sqlbuild_refactor::refactoring::main::commit_refactor_plan::commit_refactor_plan; @@ -51,6 +54,8 @@ const MOVE_COMMAND: &str = "mv"; const EDIT_ERROR_CODE: &str = "C957"; const WRITE_ERROR_CODE: &str = "C958"; const MOVE_FILE_ERROR_CODE: &str = "C954"; +const RULE_PHASE: &str = "rule"; +const PROJECT_FAILED_HELP: &str = "sqb compile"; fn imported<'py>(py: Python<'py>, module: &str, name: &str) -> PyResult> { py.import(module)?.getattr(name) @@ -109,25 +114,29 @@ fn optional_posix(path: &Bound<'_, PyAny>) -> PyResult> { Ok(Some(path.call_method0("as_posix")?.extract()?)) } -/// The edited project's errors, paths inside the scratch copy made relative to it. +/// A compiled project's errors, paths inside `root` made relative to it. fn relative_diagnostics( errors: &[Bound<'_, PyAny>], - staging_dir: &Path, + root: &Path, ) -> PyResult> { let mut diagnostics: Vec = Vec::new(); for error in errors { let path: Option = optional_posix(&native_field(error, "path")?)?.map(|path| { let candidate: &Path = Path::new(&path); - match candidate.strip_prefix(staging_dir) { - Ok(relative) if candidate.is_absolute() => relative.to_string_lossy().into_owned(), + match candidate.strip_prefix(root) { + Ok(relative) if candidate.is_absolute() => { + relative.to_string_lossy().replace('\\', "/") + } _ => path, } }); + let phase: String = native_field(error, "phase")?.str()?.extract()?; diagnostics.push(RefactorDiagnostic { code: native_field(error, "code")?.extract()?, message: native_field(error, "message")?.extract()?, path, line: native_field(error, "line")?.extract()?, + rule: phase == RULE_PHASE, }); } Ok(diagnostics) @@ -191,11 +200,18 @@ struct Command<'py> { json_output: bool, } -/// Where a refactoring stopped: its plan, terminal status and compile errors. +/// Where a refactoring stopped: plan, status, blocking errors and the rule-failure baseline. struct Outcome { plan: RefactorPlan, status: RefactorStatus, diagnostics: Vec, + baseline: BaselineComparison, +} + +/// How the command ended: with a plan, or refused because the project does not compile. +enum Finished { + Planned(Box), + ProjectFailed(Vec), } /// A plan verified in the scratch copy, with its originals and the edited project's errors. @@ -221,57 +237,56 @@ fn move_command(request: &Bound<'_, PyAny>) -> PyResult { == MOVE_COMMAND) } -/// The target of each refactoring the command asks for: every move pair, or one rename. +/// The target of each refactoring the command asks for: every move or rename pair. fn target_requests( py: Python<'_>, request: &Bound<'_, PyAny>, ) -> PyResult> { + let is_move: bool = move_command(request)?; let target: Option = native_field(request, "target")?.extract()?; - let destination: Option = native_field(request, "destination")?.extract()?; - if !move_command(request)? { - return Ok(vec![RefactorTargetRequest { - is_move: false, - target: target.unwrap_or_default(), - new_name: native_field(request, "new_name")?.extract()?, - destination, - cascade: native_field(request, "cascade")?.extract()?, - }]); - } + let second: Option = + native_field(request, if is_move { "destination" } else { "new_name" })?.extract()?; + let cascade: bool = native_field(request, "cascade")?.extract()?; let more: Vec = native_field(request, "more_moves")?.extract()?; let from_file: Option = native_field(request, "from_file")?.extract()?; let contents: Option = match &from_file { None => None, Some(path) => Some(std::fs::read_to_string(path).map_err(|error| { + let noun: &str = if is_move { "move" } else { "rename" }; user_error( py, ( - format!("cannot read move file {path}: {error}"), + format!("cannot read {noun} file {path}: {error}"), MOVE_FILE_ERROR_CODE, None, ), ) })?), }; - let mut words: Vec<&str> = target - .iter() - .chain(&destination) - .map(String::as_str) - .collect(); + let mut words: Vec<&str> = target.iter().chain(&second).map(String::as_str).collect(); words.extend(more.iter().map(String::as_str)); - let arguments: MoveArguments<'_> = MoveArguments { + let arguments: PairArguments<'_> = PairArguments { + is_move, words, file: from_file.as_deref().zip(contents.as_deref()), }; let pairs: Vec<(String, String)> = - move_pairs(&arguments).map_err(|error| refactor_error(py, error))?; + refactor_pairs(&arguments).map_err(|error| refactor_error(py, error))?; Ok(pairs .into_iter() - .map(|(target, destination)| RefactorTargetRequest { - is_move: true, - target, - new_name: None, - destination: Some(destination), - cascade: false, + .map(|(target, second)| { + let (new_name, destination) = if is_move { + (None, Some(second)) + } else { + (Some(second), None) + }; + RefactorTargetRequest { + is_move, + target, + new_name, + destination, + cascade, + } }) .collect()) } @@ -330,7 +345,7 @@ fn verify( command: &Command<'_>, (before, mut plan): (&Bound<'_, PyAny>, RefactorPlan), (staging_dir, status): (&Path, &Status<'_>), -) -> PyResult> { +) -> PyResult> { let project_dir: &Path = &command.project_dir; status.start("Verifying the edited project compiles...")?; let mut originals: Originals = stage(py, (project_dir, staging_dir), &plan, true)?; @@ -342,11 +357,7 @@ fn verify( with_model_migration(&plan, &before_facts, after_facts.as_deref(), &originals); if !migrated.blocking.is_empty() { status.error("Refused: the renamed model's history cannot be kept.")?; - return Ok(Err(Outcome { - plan: migrated, - status: RefactorStatus::Refused, - diagnostics: Vec::new(), - })); + return Ok(Err(migrated)); } if migrated.migrations != plan.migrations { plan = migrated; @@ -362,40 +373,58 @@ fn verify( })) } -fn run(py: Python<'_>, command: &Command<'_>, status: &Status<'_>) -> PyResult> { +fn finished(outcome: Outcome) -> Finished { + Finished::Planned(Box::new(outcome)) +} + +fn refused(plan: RefactorPlan, baseline: BaselineComparison) -> Finished { + finished(Outcome { + plan, + status: RefactorStatus::Refused, + diagnostics: Vec::new(), + baseline, + }) +} + +fn run(py: Python<'_>, command: &Command<'_>, status: &Status<'_>) -> PyResult { let targets: Vec = target_requests(py, &command.request)?; let started: Instant = Instant::now(); status.start("Compiling project...")?; let before: Compiled<'_> = compile(py, &command.project_dir, false)?; - if before.project.is_none() || !before.errors.is_empty() { - status.error("Project does not compile; fix it before renaming (sqb compile)")?; + let existing: Vec = + relative_diagnostics(&before.errors, &command.project_dir)?; + if before.project.is_none() || existing.iter().any(|diagnostic| !diagnostic.rule) { + status.error(&project_failed_message(command)?)?; write_errors(py, &before.errors)?; - return Ok(None); + return Ok(Finished::ProjectFailed(existing)); } - status.complete(&format!( - "Compiled project. ({:.2}s)", - started.elapsed().as_secs_f64() - ))?; + let elapsed: f64 = started.elapsed().as_secs_f64(); + status.complete(&if existing.is_empty() { + format!("Compiled project. ({elapsed:.2}s)") + } else { + format!( + "Compiled project with {}; the change may not add or worsen any. ({elapsed:.2}s)", + format_count_noun( + i64::try_from(existing.len()).unwrap_or(i64::MAX), + "existing rule failure", + None, + ) + ) + })?; + let unplanned: BaselineComparison = compare_with_baseline(&existing, &existing, &[]); let names: HashMap = target_lookup(&native_field( &native_field(&before.project, "graph")?, "all_keys", )?)?; let lookup = |name: &str| names.get(name).cloned(); - let requests: Vec = targets - .iter() - .map(|target| refactor_request_for_target(target, &lookup)) - .collect::>() - .map_err(|error| refactor_error(py, error))?; + let requests: Vec = + refactor_requests(&targets, &lookup).map_err(|error| refactor_error(py, error))?; status.start("Planning edits...")?; let planned: RefactorPlan = plan(py, &before.project, &requests)?; status.complete(&format!("Planned edits to {}.", files(&planned)))?; if !planned.blocking.is_empty() || !planned.manual.is_empty() { status.error("Refused: some references cannot be rewritten safely.")?; - return Ok(Some(Outcome { - plan: planned, - status: RefactorStatus::Refused, - diagnostics: Vec::new(), - })); + return Ok(refused(planned, unplanned)); } let options: Bound<'_, PyDict> = PyDict::new(py); options.set_item("prefix", STAGING_PREFIX)?; @@ -415,39 +444,91 @@ fn run(py: Python<'_>, command: &Command<'_>, status: &Status<'_>) -> PyResult verified, - Err(outcome) => return Ok(Some(outcome)), + Err(migrated) => return Ok(refused(migrated, unplanned)), }; - if !diagnostics.is_empty() { - status.error("Verification failed: the edited project does not compile.")?; - return Ok(Some(Outcome { + let baseline: BaselineComparison = + compare_with_baseline(&existing, &diagnostics, &plan.changes); + if !baseline.blocking.is_empty() { + status.error( + if baseline.blocking.iter().all(|diagnostic| diagnostic.rule) { + "Verification failed: the change adds or worsens rule failures." + } else { + "Verification failed: the edited project does not compile." + }, + )?; + return Ok(finished(Outcome { plan, status: RefactorStatus::CompileFailed, - diagnostics, + diagnostics: baseline.blocking.clone(), + baseline, })); } - status.complete("Verified: the edited project compiles.")?; + status.complete(if baseline.before.is_empty() { + "Verified: the edited project compiles." + } else { + "Verified: the edited project compiles and adds or worsens no rule failure." + })?; if native_field(&command.request, "dry_run")?.extract::()? { - return Ok(Some(Outcome { + return Ok(finished(Outcome { plan, status: RefactorStatus::DryRun, diagnostics: Vec::new(), + baseline, })); } status.start(&format!("Writing {}...", files(&plan)))?; commit_refactor_plan(&command.project_dir, &originals, &plan) .map_err(|error| refactor_error(py, error))?; status.complete(&format!("Wrote {}.", files(&plan)))?; - Ok(Some(Outcome { + Ok(finished(Outcome { plan, status: RefactorStatus::Applied, diagnostics: Vec::new(), + baseline, })) } +/// The refusal for a project that does not compile before the change. +fn project_failed_message(command: &Command<'_>) -> PyResult { + let verb: &str = if move_command(&command.request)? { + "moving" + } else { + "renaming" + }; + Ok(format!( + "Project does not compile; fix it before {verb} ({PROJECT_FAILED_HELP})" + )) +} + +fn write_stdout(py: Python<'_>, output: String) -> PyResult<()> { + py.import("sys")? + .getattr("stdout")? + .call_method1("write", (output,))?; + Ok(()) +} + +fn finish_project_failed( + py: Python<'_>, + command: &Command<'_>, + diagnostics: &[RefactorDiagnostic], +) -> PyResult { + if command.json_output { + let message: String = project_failed_message(command)?; + let output: String = + render_project_failure_json((&message, PROJECT_FAILED_HELP), diagnostics) + .map_err(|error| PyValueError::new_err(format!("{error:?}")))?; + write_stdout(py, output)?; + } + Ok(1) +} + fn finish(py: Python<'_>, command: &Command<'_>, outcome: &Outcome) -> PyResult { let output: String = if command.json_output { - render_refactor_json(&outcome.plan, outcome.status, &outcome.diagnostics) - .map_err(|error| PyValueError::new_err(format!("{error:?}")))? + render_refactor_json( + &outcome.plan, + (outcome.status, &outcome.diagnostics, &outcome.baseline), + ) + .map_err(|error| PyValueError::new_err(format!("{error:?}")))? } else { let project_dir: Bound<'_, PyAny> = native_field(&command.request, "project_dir")?; let project_dir: Option = if project_dir.is_none() { @@ -462,16 +543,14 @@ fn finish(py: Python<'_>, command: &Command<'_>, outcome: &Outcome) -> PyResult< ); render_refactor_text( &outcome.plan, - (outcome.status, &outcome.diagnostics), + (outcome.status, &outcome.diagnostics, &outcome.baseline), layer_move.as_ref(), CliStyle { use_color: command.use_color, }, ) }; - py.import("sys")? - .getattr("stdout")? - .call_method1("write", (output,))?; + write_stdout(py, output)?; Ok(i32::from(!matches!( outcome.status, RefactorStatus::Applied | RefactorStatus::DryRun @@ -501,11 +580,11 @@ pub(crate) fn run_refactor_command(py: Python<'_>, request: &Bound<'_, PyAny>) - let status: Status<'_> = Status { reporter: status_reporter(&stream, use_color)?, }; - let result: PyResult> = compiler_guard(|| run(py, &command, &status)); + let result: PyResult = compiler_guard(|| run(py, &command, &status)); status.close()?; match result? { - None => Ok(1), - Some(outcome) => finish(py, &command, &outcome), + Finished::ProjectFailed(diagnostics) => finish_project_failed(py, &command, &diagnostics), + Finished::Planned(outcome) => finish(py, &command, &outcome), } } diff --git a/crates/sqlbuild-refactor/Cargo.toml b/crates/sqlbuild-refactor/Cargo.toml index 497a17b354..8d992e24b4 100644 --- a/crates/sqlbuild-refactor/Cargo.toml +++ b/crates/sqlbuild-refactor/Cargo.toml @@ -17,5 +17,6 @@ sqlbuild-analysis.workspace = true sqlbuild-config.workspace = true sqlbuild-core.workspace = true sqlbuild-model-config.workspace = true +sqlbuild-pysource.workspace = true sqlbuild-sqltext.workspace = true tempfile.workspace = true diff --git a/crates/sqlbuild-refactor/src/refactor_command/_helpers/baseline.rs b/crates/sqlbuild-refactor/src/refactor_command/_helpers/baseline.rs new file mode 100644 index 0000000000..2c7c544877 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactor_command/_helpers/baseline.rs @@ -0,0 +1,71 @@ +//! Matching of rule failures before and after a refactoring. + +use crate::refactor_command::constants::NUMBER_PLACEHOLDER; +use crate::refactor_command::models::RefactorDiagnostic; +use crate::refactoring::models::FileChange; + +/// The index of the first unused earlier failure `matches` accepts. +pub(crate) fn first_unused( + before: &[RefactorDiagnostic], + used: &[bool], + matches: impl Fn(&RefactorDiagnostic) -> bool, +) -> Option { + before + .iter() + .zip(used) + .position(|(earlier, used)| !*used && matches(earlier)) +} + +/// The path a moved file had before the change, or `path` itself. +pub(crate) fn original_path(changes: &[FileChange], path: Option<&str>) -> Option { + let path: &str = path?; + let moved: Option<&FileChange> = changes + .iter() + .find(|change| change.moved() && change.path == path); + Some(moved.map_or_else(|| path.to_owned(), |change| change.original_path.clone())) +} + +/// Whether `after` is `before` with every number kept or lowered and nothing else changed. +pub(crate) fn reduced(before: &str, after: &str) -> bool { + let (before_text, before_numbers) = numbers_apart(before); + let (after_text, after_numbers) = numbers_apart(after); + before_text == after_text + && before_numbers.len() == after_numbers.len() + && before_numbers + .iter() + .zip(&after_numbers) + .all(|(earlier, later)| later <= earlier) +} + +/// `message` with each run of ASCII digits replaced by a placeholder, and those numbers. +fn numbers_apart(message: &str) -> (String, Vec<(usize, String)>) { + let mut text: String = String::new(); + let mut numbers: Vec<(usize, String)> = Vec::new(); + let mut digits: String = String::new(); + for character in message.chars() { + if character.is_ascii_digit() { + digits.push(character); + continue; + } + if let Some(number) = number_of(&digits) { + numbers.push(number); + text.push(NUMBER_PLACEHOLDER); + } + digits.clear(); + text.push(character); + } + if let Some(number) = number_of(&digits) { + numbers.push(number); + text.push(NUMBER_PLACEHOLDER); + } + (text, numbers) +} + +/// A run of digits as `(significant length, digits)`, which orders by value; none when empty. +fn number_of(digits: &str) -> Option<(usize, String)> { + if digits.is_empty() { + return None; + } + let significant: &str = digits.trim_start_matches('0'); + Some((significant.len(), significant.to_owned())) +} diff --git a/crates/sqlbuild-refactor/src/refactor_command/_helpers/json.rs b/crates/sqlbuild-refactor/src/refactor_command/_helpers/json.rs index de323bcaf4..4553b91262 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/_helpers/json.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/_helpers/json.rs @@ -3,6 +3,7 @@ use sqlbuild_core::json::models::{JsonInteger, JsonValue}; use crate::refactor_command::_helpers::output_text::sorted_edits; +use crate::refactor_command::models::RefactorDiagnostic; use crate::refactoring::models::{FileChange, ManualLocation, RefactorOperation, RefactorPlan}; pub(crate) fn text(value: &str) -> JsonValue { @@ -72,16 +73,36 @@ pub(crate) fn location(item: &ManualLocation) -> JsonValue { ]) } -/// Every move of a batch as `{"target": "model:", "destination": }`. +/// Each diagnostic as `{code, message, path, line}`. +pub(crate) fn diagnostic_array(diagnostics: &[RefactorDiagnostic]) -> JsonValue { + JsonValue::Array( + diagnostics + .iter() + .map(|diagnostic| { + object(vec![ + ("code", text(&diagnostic.code)), + ("message", text(&diagnostic.message)), + ("path", optional_text(diagnostic.path.as_deref())), + ("line", integer(diagnostic.line)), + ]) + }) + .collect(), + ) +} + +/// Every request of a batch as `{"target", "new_name" for renames, "destination"}`. pub(crate) fn batch_moves(plan: &RefactorPlan) -> JsonValue { JsonValue::Array( plan.batch .iter() .map(|request| { - object(vec![ - ("target", text(&format!("model:{}", request.model_name))), - ("destination", optional_text(request.destination.as_deref())), - ]) + let mut entries: Vec<(&str, JsonValue)> = + vec![("target", text(&format!("model:{}", request.model_name)))]; + if request.operation != RefactorOperation::MoveModel { + entries.push(("new_name", text(&request.new_name))); + } + entries.push(("destination", optional_text(request.destination.as_deref()))); + object(entries) }) .collect(), ) diff --git a/crates/sqlbuild-refactor/src/refactor_command/_helpers/mod.rs b/crates/sqlbuild-refactor/src/refactor_command/_helpers/mod.rs index 51d2f42cf2..a095cef3e6 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/_helpers/mod.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/_helpers/mod.rs @@ -1,5 +1,7 @@ +pub(crate) mod baseline; pub(crate) mod json; pub(crate) mod layer_folders; pub(crate) mod output_text; pub(crate) mod quoting; +pub(crate) mod request_for_target; pub(crate) mod text; diff --git a/crates/sqlbuild-refactor/src/refactor_command/_helpers/output_text.rs b/crates/sqlbuild-refactor/src/refactor_command/_helpers/output_text.rs index b6cb78b12a..7f7e3ab33f 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/_helpers/output_text.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/_helpers/output_text.rs @@ -9,7 +9,9 @@ use crate::refactor_command::_helpers::text::{ use crate::refactor_command::constants::{ BLANK_PREFIX, CONTINUE_PREFIX, FOLDER_PLACEHOLDER, KIND_WIDTH, POSITION_WIDTH, }; -use crate::refactor_command::models::{LayerMove, RefactorDiagnostic, RefactorStatus}; +use crate::refactor_command::models::{ + BaselineComparison, LayerMove, RefactorDiagnostic, RefactorStatus, +}; use crate::refactoring::models::{ FileChange, ManualLocation, MigrationDeclaration, RefactorOperation, RefactorPlan, TextEdit, }; @@ -36,7 +38,12 @@ pub(crate) fn refactor_target(plan: &RefactorPlan) -> String { pub(crate) fn heading(plan: &RefactorPlan) -> String { if !plan.batch.is_empty() { - return format!("Move {}", refactor_target(plan)); + let verb: &str = if plan.request.operation == RefactorOperation::MoveModel { + "Move" + } else { + "Rename" + }; + return format!("{verb} {}", refactor_target(plan)); } let request = &plan.request; match request.operation { @@ -60,16 +67,17 @@ pub(crate) fn heading(plan: &RefactorPlan) -> String { } } -/// One `model -> destination` line per move of a batch. +/// One `model -> destination` line per move of a batch, or `model -> new name` per rename. pub(crate) fn batch_lines(style: CliStyle, plan: &RefactorPlan) -> Vec { tree_items( style, plan.batch.iter().map(|request| { - format!( - "{} -> {}", - style.object_name(&request.model_name), + let after: &str = if request.operation == RefactorOperation::MoveModel { request.destination.as_deref().unwrap_or_default() - ) + } else { + &request.new_name + }; + format!("{} -> {after}", style.object_name(&request.model_name)) }), ) } @@ -197,6 +205,42 @@ pub(crate) fn diagnostic_lines(style: CliStyle, diagnostics: &[RefactorDiagnosti lines } +/// The project's rule failures before the change and how many remain, are resolved or block. +pub(crate) fn baseline_lines(style: CliStyle, baseline: &BaselineComparison) -> Vec { + let failures = |count: usize| -> String { + format_count_noun( + i64::try_from(count).unwrap_or(i64::MAX), + "rule failure", + None, + ) + }; + let blocking: usize = baseline + .blocking + .iter() + .filter(|diagnostic| diagnostic.rule) + .count(); + let mut lines: Vec = vec![ + style.section("Existing rule failures"), + format!( + "The project had {} before this change; {} resolved, {} still failing.", + failures(baseline.before.len()), + baseline.resolved.len(), + baseline.remaining.len() + ), + ]; + lines.extend(diagnostic_lines(style, &baseline.remaining)); + lines.push(style.muted(&if blocking == 0 { + "Existing rule failures do not block the change; it adds or worsens none.".to_owned() + } else { + format!( + "Existing rule failures do not block the change, but the {} below {} new or worse.", + failures(blocking), + if blocking == 1 { "is" } else { "are" } + ) + })); + lines +} + pub(crate) fn layer_move_lines(style: CliStyle, layer_move: &LayerMove) -> Vec { let folder: String = match (&layer_move.folder, &layer_move.layer_folder) { (Some(folder), _) => format!("under {folder}/"), diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/refactor_request_for_target.rs b/crates/sqlbuild-refactor/src/refactor_command/_helpers/request_for_target.rs similarity index 98% rename from crates/sqlbuild-refactor/src/refactor_command/main/refactor_request_for_target.rs rename to crates/sqlbuild-refactor/src/refactor_command/_helpers/request_for_target.rs index 23451b30db..d2e6dfc204 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/main/refactor_request_for_target.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/_helpers/request_for_target.rs @@ -12,7 +12,7 @@ use crate::refactoring::errors::RefactorError; use crate::refactoring::models::{RefactorOperation, RefactorRequest}; /// The planner request for a `[kind:]` or `[kind:].` target. -pub fn refactor_request_for_target( +pub(crate) fn refactor_request_for_target( request: &RefactorTargetRequest, lookup: &dyn Fn(&str) -> Option, ) -> Result { diff --git a/crates/sqlbuild-refactor/src/refactor_command/constants.rs b/crates/sqlbuild-refactor/src/refactor_command/constants.rs index c7980c7141..e906c01cde 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/constants.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/constants.rs @@ -6,8 +6,16 @@ pub(crate) const MOVE_USAGE: &str = "write the target as ; the model: pre pub(crate) const RENAME_SCOPE: &str = "sqb rename renames models and model columns"; pub(crate) const MOVE_SCOPE: &str = "sqb mv moves models"; pub(crate) const TARGET_ERROR_CODE: &str = "C954"; +pub(crate) const PROJECT_FAILED_CODE: &str = "C959"; +pub(crate) const PROJECT_FAILED_STATUS: &str = "project_failed"; pub(crate) const MOVE_PAIRS_USAGE: &str = "pass TARGET DESTINATION pairs, or --from-file with one pair per line"; +pub(crate) const RENAME_PAIRS_USAGE: &str = + "pass model TARGET NEW_NAME pairs, or --from-file with one pair per line"; +pub(crate) const BATCH_RENAME_USAGE: &str = + "rename columns, or use --cascade, with one TARGET NEW_NAME per sqb rename"; +/// What stands for each number when rule failure messages are compared. +pub(crate) const NUMBER_PLACEHOLDER: char = '#'; pub(crate) const MOVE_FILE_COMMENT: char = '#'; pub(crate) const COLUMN_TARGET_KIND: &str = "column"; pub(crate) const COLUMN_TARGET_SEPARATOR: char = '.'; diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/compare_with_baseline.rs b/crates/sqlbuild-refactor/src/refactor_command/main/compare_with_baseline.rs new file mode 100644 index 0000000000..28e216c3c2 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactor_command/main/compare_with_baseline.rs @@ -0,0 +1,83 @@ +//! Compare the edited project's errors with the rule failures the project already had. + +use std::collections::BTreeSet; + +use crate::refactor_command::_helpers::baseline::{first_unused, original_path, reduced}; +use crate::refactor_command::models::{BaselineComparison, RefactorDiagnostic}; +use crate::refactoring::models::FileChange; + +/// How the edited project's `result` errors compare with the project's `baseline` errors. +#[must_use] +pub fn compare_with_baseline( + baseline: &[RefactorDiagnostic], + result: &[RefactorDiagnostic], + changes: &[FileChange], +) -> BaselineComparison { + let before: Vec = baseline + .iter() + .filter(|diagnostic| diagnostic.rule) + .cloned() + .collect(); + let original = |path: &Option| original_path(changes, path.as_deref()); + let mut used: Vec = vec![false; before.len()]; + let mut matched: Vec> = vec![None; result.len()]; + for (index, diagnostic) in result.iter().enumerate() { + if !diagnostic.rule { + continue; + } + let path: Option = original(&diagnostic.path); + matched[index] = first_unused(&before, &used, |earlier| { + earlier.code == diagnostic.code + && earlier.path == path + && earlier.message == diagnostic.message + }); + if let Some(found) = matched[index] { + used[found] = true; + } + } + for (index, diagnostic) in result.iter().enumerate() { + if !diagnostic.rule || matched[index].is_some() { + continue; + } + let path: Option = original(&diagnostic.path); + matched[index] = first_unused(&before, &used, |earlier| { + earlier.code == diagnostic.code + && earlier.path == path + && reduced(&earlier.message, &diagnostic.message) + }); + if let Some(found) = matched[index] { + used[found] = true; + } + } + let (mut remaining, mut blocking) = (Vec::new(), Vec::new()); + for (diagnostic, found) in result.iter().zip(&matched) { + if found.is_some() { + remaining.push(diagnostic.clone()); + } else { + blocking.push(diagnostic.clone()); + } + } + let worsened: BTreeSet<(&str, Option)> = blocking + .iter() + .map(|diagnostic: &RefactorDiagnostic| { + (diagnostic.code.as_str(), original(&diagnostic.path)) + }) + .collect(); + let rules_unchecked: bool = blocking.iter().any(|diagnostic| !diagnostic.rule); + let resolved: Vec = before + .iter() + .zip(&used) + .filter(|(diagnostic, used)| { + !rules_unchecked + && !**used + && !worsened.contains(&(diagnostic.code.as_str(), diagnostic.path.clone())) + }) + .map(|(diagnostic, _)| diagnostic.clone()) + .collect(); + BaselineComparison { + before, + remaining, + resolved, + blocking, + } +} diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/layer_move_suggestion.rs b/crates/sqlbuild-refactor/src/refactor_command/main/layer_move_suggestion.rs index ea4e0841c4..fb5c6b6ea3 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/main/layer_move_suggestion.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/main/layer_move_suggestion.rs @@ -19,6 +19,7 @@ pub fn layer_move_suggestion( ) -> Option { let renamed: &FileChange = plan.changes.iter().find(|change| change.moved())?; if plan.request.operation != RefactorOperation::RenameModel + || !plan.batch.is_empty() || diagnostics.is_empty() || diagnostics.iter().any(|diagnostic| { !LAYER_FOLDER_RULE_CODES.contains(&diagnostic.code.as_str()) diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/mod.rs b/crates/sqlbuild-refactor/src/refactor_command/main/mod.rs index c1128322f3..3197485a7a 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/main/mod.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/main/mod.rs @@ -1,5 +1,7 @@ +pub mod compare_with_baseline; pub mod layer_move_suggestion; -pub mod move_pairs; -pub mod refactor_request_for_target; +pub mod refactor_pairs; +pub mod refactor_requests; +pub mod render_project_failure_json; pub mod render_refactor_json; pub mod render_refactor_text; diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/move_pairs.rs b/crates/sqlbuild-refactor/src/refactor_command/main/move_pairs.rs deleted file mode 100644 index 75c8222ff8..0000000000 --- a/crates/sqlbuild-refactor/src/refactor_command/main/move_pairs.rs +++ /dev/null @@ -1,58 +0,0 @@ -//! The `TARGET DESTINATION` pairs of one `sqb mv`, from the command line and `--from-file`. - -use crate::refactor_command::constants::{MOVE_FILE_COMMENT, MOVE_PAIRS_USAGE, TARGET_ERROR_CODE}; -use crate::refactor_command::models::MoveArguments; -use crate::refactoring::errors::RefactorError; - -/// Every `(target, destination)` pair in command-line order, then in file order. -pub fn move_pairs(arguments: &MoveArguments<'_>) -> Result, RefactorError> { - let words: &[&str] = &arguments.words; - if words.len() % 2 == 1 { - return Err(RefactorError::input( - TARGET_ERROR_CODE, - format!("'{}' has no DESTINATION", words[words.len() - 1]), - Some(MOVE_PAIRS_USAGE), - )); - } - let mut pairs: Vec<(String, String)> = words - .chunks(2) - .map(|pair| (pair[0].to_owned(), pair[1].to_owned())) - .collect(); - if let Some((path, contents)) = arguments.file { - pairs.extend(file_pairs(path, contents)?); - } - if pairs.is_empty() { - return Err(RefactorError::input( - TARGET_ERROR_CODE, - "sqb mv needs a TARGET and a DESTINATION", - Some(MOVE_PAIRS_USAGE), - )); - } - Ok(pairs) -} - -/// The pairs of a move file: one per line, two whitespace-separated words, `#` comments. -fn file_pairs(path: &str, contents: &str) -> Result, RefactorError> { - let mut pairs: Vec<(String, String)> = Vec::new(); - for (number, line) in contents.lines().enumerate() { - let text: &str = line.trim(); - if text.is_empty() || text.starts_with(MOVE_FILE_COMMENT) { - continue; - } - let words: Vec<&str> = text.split_whitespace().collect(); - let [target, destination] = words.as_slice() else { - return Err(RefactorError::input( - TARGET_ERROR_CODE, - format!( - "{path}:{}: expected TARGET and DESTINATION separated by a tab or spaces, \ - found {} words", - number + 1, - words.len() - ), - Some(MOVE_PAIRS_USAGE), - )); - }; - pairs.push(((*target).to_owned(), (*destination).to_owned())); - } - Ok(pairs) -} diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/refactor_pairs.rs b/crates/sqlbuild-refactor/src/refactor_command/main/refactor_pairs.rs new file mode 100644 index 0000000000..976cbdf869 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactor_command/main/refactor_pairs.rs @@ -0,0 +1,71 @@ +//! The `TARGET DESTINATION` or `TARGET NEW_NAME` pairs of one `sqb mv` or `sqb rename`. + +use crate::refactor_command::constants::{ + MOVE_FILE_COMMENT, MOVE_PAIRS_USAGE, RENAME_PAIRS_USAGE, TARGET_ERROR_CODE, +}; +use crate::refactor_command::models::PairArguments; +use crate::refactoring::errors::RefactorError; + +/// Every `(target, second)` pair in command-line order, then in file order. +pub fn refactor_pairs( + arguments: &PairArguments<'_>, +) -> Result, RefactorError> { + let (command, second, usage) = if arguments.is_move { + ("sqb mv", "DESTINATION", MOVE_PAIRS_USAGE) + } else { + ("sqb rename", "NEW_NAME", RENAME_PAIRS_USAGE) + }; + let words: &[&str] = &arguments.words; + if words.len() % 2 == 1 { + return Err(RefactorError::input( + TARGET_ERROR_CODE, + format!("'{}' has no {second}", words[words.len() - 1]), + Some(usage), + )); + } + let mut pairs: Vec<(String, String)> = words + .chunks(2) + .map(|pair| (pair[0].to_owned(), pair[1].to_owned())) + .collect(); + if let Some((path, contents)) = arguments.file { + pairs.extend(file_pairs((path, contents), second, usage)?); + } + if pairs.is_empty() { + return Err(RefactorError::input( + TARGET_ERROR_CODE, + format!("{command} needs a TARGET and a {second}"), + Some(usage), + )); + } + Ok(pairs) +} + +/// The pairs of a pair file: one per line, two whitespace-separated words, `#` comments. +fn file_pairs( + (path, contents): (&str, &str), + second: &str, + usage: &str, +) -> Result, RefactorError> { + let mut pairs: Vec<(String, String)> = Vec::new(); + for (number, line) in contents.lines().enumerate() { + let text: &str = line.trim(); + if text.is_empty() || text.starts_with(MOVE_FILE_COMMENT) { + continue; + } + let words: Vec<&str> = text.split_whitespace().collect(); + let [target, other] = words.as_slice() else { + return Err(RefactorError::input( + TARGET_ERROR_CODE, + format!( + "{path}:{}: expected TARGET and {second} separated by a tab or spaces, \ + found {} words", + number + 1, + words.len() + ), + Some(usage), + )); + }; + pairs.push(((*target).to_owned(), (*other).to_owned())); + } + Ok(pairs) +} diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/refactor_requests.rs b/crates/sqlbuild-refactor/src/refactor_command/main/refactor_requests.rs new file mode 100644 index 0000000000..4c6d1c90c3 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactor_command/main/refactor_requests.rs @@ -0,0 +1,45 @@ +//! The planner requests of every target of one `sqb rename` or `sqb mv`. + +use sqlbuild_analysis::lineage_command::models::LineageKey; + +use crate::refactor_command::_helpers::request_for_target::refactor_request_for_target; +use crate::refactor_command::constants::{BATCH_RENAME_USAGE, TARGET_ERROR_CODE}; +use crate::refactor_command::models::RefactorTargetRequest; +use crate::refactoring::errors::RefactorError; +use crate::refactoring::models::{RefactorOperation, RefactorRequest}; + +/// Each target's request; several renames must all be model renames without `--cascade`. +pub fn refactor_requests( + targets: &[RefactorTargetRequest], + lookup: &dyn Fn(&str) -> Option, +) -> Result, RefactorError> { + if targets.len() > 1 && targets.iter().any(|target| target.cascade) { + return Err(RefactorError::input( + TARGET_ERROR_CODE, + "--cascade applies to one column rename, not to several renames", + Some(BATCH_RENAME_USAGE), + )); + } + let requests: Vec = targets + .iter() + .map(|target| refactor_request_for_target(target, lookup)) + .collect::>()?; + if requests.len() <= 1 { + return Ok(requests); + } + if let Some((target, _)) = targets + .iter() + .zip(&requests) + .find(|(_, request)| request.operation == RefactorOperation::RenameColumn) + { + return Err(RefactorError::input( + TARGET_ERROR_CODE, + format!( + "'{}' names a column; several renames in one sqb rename must all be models", + target.target.trim() + ), + Some(BATCH_RENAME_USAGE), + )); + } + Ok(requests) +} diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/render_project_failure_json.rs b/crates/sqlbuild-refactor/src/refactor_command/main/render_project_failure_json.rs new file mode 100644 index 0000000000..f6d35396f3 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactor_command/main/render_project_failure_json.rs @@ -0,0 +1,36 @@ +//! The JSON error document of `sqb rename` and `sqb mv` when the project does not compile. + +use sqlbuild_core::json::errors::JsonEmitError; +use sqlbuild_core::json::main::dumps::dumps; +use sqlbuild_core::json::models::{JsonDialect, JsonValue, StdlibJsonOptions}; + +use crate::refactor_command::_helpers::json::{diagnostic_array, object, text}; +use crate::refactor_command::constants::{PROJECT_FAILED_CODE, PROJECT_FAILED_STATUS}; +use crate::refactor_command::models::RefactorDiagnostic; + +/// `{"status", "error": {code, message, help}, "compile": {ok, errors}}` with a trailing newline. +pub fn render_project_failure_json( + (message, help): (&str, &str), + diagnostics: &[RefactorDiagnostic], +) -> Result { + let payload: JsonValue = object(vec![ + ("status", text(PROJECT_FAILED_STATUS)), + ( + "error", + object(vec![ + ("code", text(PROJECT_FAILED_CODE)), + ("message", text(message)), + ("help", text(help)), + ]), + ), + ( + "compile", + object(vec![ + ("ok", JsonValue::Bool(false)), + ("errors", diagnostic_array(diagnostics)), + ]), + ), + ]); + let dialect: JsonDialect = JsonDialect::Stdlib(StdlibJsonOptions::new(Some(2))); + Ok(format!("{}\n", dumps(&payload, &dialect)?)) +} diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_json.rs b/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_json.rs index 313b988b76..3bc87e2c38 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_json.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_json.rs @@ -5,17 +5,17 @@ use sqlbuild_core::json::main::dumps::dumps; use sqlbuild_core::json::models::{JsonDialect, JsonValue, StdlibJsonOptions}; use crate::refactor_command::_helpers::json::{ - batch_moves, count, file, integer, location, object, operation_value, optional_text, text, + batch_moves, count, diagnostic_array, file, location, object, operation_value, optional_text, + text, }; use crate::refactor_command::_helpers::output_text::refactor_target; -use crate::refactor_command::models::{RefactorDiagnostic, RefactorStatus}; -use crate::refactoring::models::RefactorPlan; +use crate::refactor_command::models::{BaselineComparison, RefactorDiagnostic, RefactorStatus}; +use crate::refactoring::models::{RefactorOperation, RefactorPlan}; /// The same facts as the text output, as stable JSON with a trailing newline. pub fn render_refactor_json( plan: &RefactorPlan, - status: RefactorStatus, - diagnostics: &[RefactorDiagnostic], + (status, diagnostics, baseline): (RefactorStatus, &[RefactorDiagnostic], &BaselineComparison), ) -> Result { let request = &plan.request; let changed: usize = if status == RefactorStatus::Applied { @@ -81,27 +81,25 @@ pub fn render_refactor_json( "compile", object(vec![ ("ok", JsonValue::Bool(diagnostics.is_empty())), - ( - "errors", - JsonValue::Array( - diagnostics - .iter() - .map(|diagnostic| { - object(vec![ - ("code", text(&diagnostic.code)), - ("message", text(&diagnostic.message)), - ("path", optional_text(diagnostic.path.as_deref())), - ("line", integer(diagnostic.line)), - ]) - }) - .collect(), - ), - ), + ("errors", diagnostic_array(diagnostics)), + ]), + ), + ( + "rule_baseline", + object(vec![ + ("before", diagnostic_array(&baseline.before)), + ("remaining", diagnostic_array(&baseline.remaining)), + ("resolved", diagnostic_array(&baseline.resolved)), ]), ), ]; if !plan.batch.is_empty() { - entries.insert(4, ("moves", batch_moves(plan))); + let key: &str = if request.operation == RefactorOperation::MoveModel { + "moves" + } else { + "renames" + }; + entries.insert(4, (key, batch_moves(plan))); } let payload: JsonValue = object(entries); let dialect: JsonDialect = JsonDialect::Stdlib(StdlibJsonOptions::new(Some(2))); diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_text.rs b/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_text.rs index fa8656e69f..7b9096cead 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_text.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_text.rs @@ -3,17 +3,19 @@ use sqlbuild_core::cli_text::models::CliStyle; use crate::refactor_command::_helpers::output_text::{ - batch_lines, change_lines, diagnostic_lines, heading, layer_move_lines, location_lines, - migration_lines, refactor_target, status_line, verb, + baseline_lines, batch_lines, change_lines, diagnostic_lines, heading, layer_move_lines, + location_lines, migration_lines, refactor_target, status_line, verb, +}; +use crate::refactor_command::models::{ + BaselineComparison, LayerMove, RefactorDiagnostic, RefactorStatus, }; -use crate::refactor_command::models::{LayerMove, RefactorDiagnostic, RefactorStatus}; use crate::refactoring::models::{ManualLocation, RefactorPlan}; -/// The edit tree, manual locations, compile errors, and one terminal status line. +/// The edit tree, locations, rule-failure baseline, compile errors, and one status line. #[must_use] pub fn render_refactor_text( plan: &RefactorPlan, - (status, diagnostics): (RefactorStatus, &[RefactorDiagnostic]), + (status, diagnostics, baseline): (RefactorStatus, &[RefactorDiagnostic], &BaselineComparison), layer_move: Option<&LayerMove>, style: CliStyle, ) -> String { @@ -46,6 +48,9 @@ pub fn render_refactor_text( lines.push(help.clone()); } } + if !baseline.before.is_empty() && !blocked { + lines.extend(baseline_lines(style, baseline)); + } if !diagnostics.is_empty() { lines.push(style.error_strong("Compile errors in the edited project")); lines.extend(diagnostic_lines(style, diagnostics)); diff --git a/crates/sqlbuild-refactor/src/refactor_command/models.rs b/crates/sqlbuild-refactor/src/refactor_command/models.rs index 974b2473ac..f477ee13f7 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/models.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/models.rs @@ -40,6 +40,21 @@ pub struct RefactorDiagnostic { /// The path in POSIX form. pub path: Option, pub line: Option, + /// Whether a project rule reported it, rather than syntax, binding or reference checks. + pub rule: bool, +} + +/// The project's rule failures before a refactoring, and how the edited project compares. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct BaselineComparison { + /// Rule failures of the project before any edit. + pub before: Vec, + /// Rule failures of the edited project that were already there, unchanged or reduced. + pub remaining: Vec, + /// Rule failures of the project before that the edited project no longer has. + pub resolved: Vec, + /// Edited-project errors that block the write: non-rule errors and new or worse rule failures. + pub blocking: Vec, } /// The one-step `sqb mv` for a rename refused only by the layer-folder rules. @@ -51,9 +66,11 @@ pub struct LayerMove { pub command: String, } -/// The command-line words and optional `(path, contents)` move file of one `sqb mv`. +/// The command-line words and optional `(path, contents)` pair file of `sqb mv` or `sqb rename`. #[derive(Clone, Debug, Default, PartialEq, Eq)] -pub struct MoveArguments<'a> { +pub struct PairArguments<'a> { + /// Whether the pairs are `sqb mv` moves rather than `sqb rename` renames. + pub is_move: bool, pub words: Vec<&'a str>, pub file: Option<(&'a str, &'a str)>, } diff --git a/crates/sqlbuild-refactor/src/refactor_command/tests.rs b/crates/sqlbuild-refactor/src/refactor_command/tests.rs index 8481bcf8f5..8cd0b5bf12 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/tests.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/tests.rs @@ -1,8 +1,10 @@ +#[path = "tests/test_compare_with_baseline.rs"] +mod compare_with_baseline; #[path = "tests/helpers.rs"] mod helpers; #[path = "tests/test_layer_move.rs"] mod layer_move; -#[path = "tests/test_move_pairs.rs"] -mod move_pairs; +#[path = "tests/test_refactor_pairs.rs"] +mod refactor_pairs; #[path = "tests/test_types.rs"] mod test_types; diff --git a/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs index 50d0a41111..9ec4f3ef67 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs @@ -1,4 +1,5 @@ use crate::refactor_command::models::RefactorDiagnostic; +use crate::refactor_command::tests::test_types::DiagnosticRow; use crate::refactoring::models::{FileChange, RefactorOperation, RefactorPlan, RefactorRequest}; /// A model rename plan whose file keeps its folder. @@ -37,6 +38,33 @@ pub(super) fn rule_diagnostics(codes: &[&str], path: &str) -> Vec Vec { + rows.iter() + .map(|(rule, code, path, message)| RefactorDiagnostic { + code: (*code).to_owned(), + message: (*message).to_owned(), + path: (!path.is_empty()).then(|| (*path).to_owned()), + line: None, + rule: *rule, + }) + .collect() +} + +pub(super) fn labels(diagnostics: &[RefactorDiagnostic]) -> Vec { + diagnostics + .iter() + .map(|diagnostic| { + format!( + "{} {}", + diagnostic.code, + diagnostic.path.as_deref().unwrap_or_default() + ) }) .collect() } diff --git a/crates/sqlbuild-refactor/src/refactor_command/tests/test_compare_with_baseline.rs b/crates/sqlbuild-refactor/src/refactor_command/tests/test_compare_with_baseline.rs new file mode 100644 index 0000000000..a0c4c012d7 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactor_command/tests/test_compare_with_baseline.rs @@ -0,0 +1,245 @@ +use crate::refactor_command::main::compare_with_baseline::compare_with_baseline; +use crate::refactor_command::models::BaselineComparison; +use crate::refactor_command::tests::helpers::{diagnostics, labels}; +use crate::refactor_command::tests::test_types::{BaselineComparisonTestCase, DiagnosticRow}; +use crate::refactoring::models::FileChange; + +const OVER_FULL: DiagnosticRow = ( + true, + "XSQBRLAYOUT001", + "models/staging", + "models/staging holds 3 models; the maximum is 2", +); + +#[test] +fn given_rule_failures_before_and_after_when_comparing_then_only_new_or_worse_ones_block() { + let test_cases = [ + BaselineComparisonTestCase { + description: "a folder over its limit that loses a model keeps an existing failure", + before: &[OVER_FULL], + after: &[( + true, + "XSQBRLAYOUT001", + "models/staging", + "models/staging holds 2 models; the maximum is 2", + )], + moves: &[("models/staging/a.sql", "models/finance/a.sql")], + expected_remaining: &["XSQBRLAYOUT001 models/staging"], + expected_resolved: &[], + expected_blocking: &[], + }, + BaselineComparisonTestCase { + description: "a failure the moves repair is resolved", + before: &[OVER_FULL], + after: &[], + moves: &[("models/staging/a.sql", "models/finance/a.sql")], + expected_remaining: &[], + expected_resolved: &["XSQBRLAYOUT001 models/staging"], + expected_blocking: &[], + }, + BaselineComparisonTestCase { + description: "a folder over its limit that gains a model is worse and blocks", + before: &[OVER_FULL], + after: &[( + true, + "XSQBRLAYOUT001", + "models/staging", + "models/staging holds 4 models; the maximum is 2", + )], + moves: &[("models/finance/a.sql", "models/staging/a.sql")], + expected_remaining: &[], + expected_resolved: &[], + expected_blocking: &["XSQBRLAYOUT001 models/staging"], + }, + BaselineComparisonTestCase { + description: "a failure in a folder that had none is new and blocks", + before: &[OVER_FULL], + after: &[ + OVER_FULL, + ( + true, + "XSQBRLAYOUT001", + "models/finance", + "models/finance holds 3 models; the maximum is 2", + ), + ], + moves: &[("models/marts/a.sql", "models/finance/a.sql")], + expected_remaining: &["XSQBRLAYOUT001 models/staging"], + expected_resolved: &[], + expected_blocking: &["XSQBRLAYOUT001 models/finance"], + }, + BaselineComparisonTestCase { + description: "an unchanged failure on a moved file follows the file", + before: &[( + true, + "SQBRMODEL101", + "models/staging/a.sql", + "a has no tests", + )], + after: &[( + true, + "SQBRMODEL101", + "models/finance/a.sql", + "a has no tests", + )], + moves: &[("models/staging/a.sql", "models/finance/a.sql")], + expected_remaining: &["SQBRMODEL101 models/finance/a.sql"], + expected_resolved: &[], + expected_blocking: &[], + }, + BaselineComparisonTestCase { + description: "a changed failure on an edited file blocks", + before: &[( + true, + "XSQBRSIZE001", + "models/marts/totals.sql", + "totals reads 2 models", + )], + after: &[( + true, + "XSQBRSIZE001", + "models/marts/totals.sql", + "totals reads 3 models", + )], + moves: &[("models/marts/totals.sql", "models/marts/totals.sql")], + expected_remaining: &[], + expected_resolved: &[], + expected_blocking: &["XSQBRSIZE001 models/marts/totals.sql"], + }, + BaselineComparisonTestCase { + description: "a changed failure without a place blocks", + before: &[(true, "XSQBRMART001", "", "the project has 2 marts")], + after: &[(true, "XSQBRMART001", "", "the project has 3 marts")], + moves: &[("models/staging/a.sql", "models/marts/a.sql")], + expected_remaining: &[], + expected_resolved: &[], + expected_blocking: &["XSQBRMART001 "], + }, + BaselineComparisonTestCase { + description: "a reduced failure without a place is existing", + before: &[(true, "XSQBRMART001", "", "the project has 3 marts")], + after: &[(true, "XSQBRMART001", "", "the project has 2 marts")], + moves: &[("models/marts/a.sql", "models/staging/a.sql")], + expected_remaining: &["XSQBRMART001 "], + expected_resolved: &[], + expected_blocking: &[], + }, + BaselineComparisonTestCase { + description: "a failure whose words change blocks even with smaller numbers", + before: &[( + true, + "SQBRPROJECT204", + "models/o/a.sql", + "owner o has fin_a and fin_b", + )], + after: &[( + true, + "SQBRPROJECT204", + "models/o/a.sql", + "owner o has fin_a and fin_c", + )], + moves: &[("models/o/c/c.sql", "models/o/fin_c/c.sql")], + expected_remaining: &[], + expected_resolved: &[], + expected_blocking: &["SQBRPROJECT204 models/o/a.sql"], + }, + BaselineComparisonTestCase { + description: "a count that grows at an untouched place blocks", + before: &[( + true, + "SQBRPROJECT204", + "models/o/a.sql", + "2 sibling owner directories", + )], + after: &[( + true, + "SQBRPROJECT204", + "models/o/a.sql", + "3 sibling owner directories", + )], + moves: &[("models/o/c/c.sql", "models/o/fin_c/c.sql")], + expected_remaining: &[], + expected_resolved: &[], + expected_blocking: &["SQBRPROJECT204 models/o/a.sql"], + }, + BaselineComparisonTestCase { + description: "nothing counts as resolved when another error may have hidden rules", + before: &[OVER_FULL], + after: &[( + false, + "D016", + "models/staging/a.sql", + "invalid model identity", + )], + moves: &[], + expected_remaining: &[], + expected_resolved: &[], + expected_blocking: &["D016 models/staging/a.sql"], + }, + BaselineComparisonTestCase { + description: "errors that are not rule failures always block", + before: &[OVER_FULL], + after: &[ + OVER_FULL, + (false, "S006", "models/staging/a.sql", "not visible"), + ], + moves: &[], + expected_remaining: &["XSQBRLAYOUT001 models/staging"], + expected_resolved: &[], + expected_blocking: &["S006 models/staging/a.sql"], + }, + BaselineComparisonTestCase { + description: "two failures where the project had one: the second is new", + before: &[OVER_FULL], + after: &[OVER_FULL, OVER_FULL], + moves: &[], + expected_remaining: &["XSQBRLAYOUT001 models/staging"], + expected_resolved: &[], + expected_blocking: &["XSQBRLAYOUT001 models/staging"], + }, + ]; + for test_case in test_cases { + let changes: Vec = test_case + .moves + .iter() + .map(|(original_path, path)| FileChange { + path: (*path).to_owned(), + original_path: (*original_path).to_owned(), + edits: Vec::new(), + }) + .collect(); + + let comparison: BaselineComparison = compare_with_baseline( + &diagnostics(test_case.before), + &diagnostics(test_case.after), + &changes, + ); + + assert_eq!( + ( + labels(&comparison.remaining), + labels(&comparison.resolved), + labels(&comparison.blocking), + ), + ( + test_case + .expected_remaining + .iter() + .map(|label| (*label).to_owned()) + .collect(), + test_case + .expected_resolved + .iter() + .map(|label| (*label).to_owned()) + .collect(), + test_case + .expected_blocking + .iter() + .map(|label| (*label).to_owned()) + .collect(), + ), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-refactor/src/refactor_command/tests/test_layer_move.rs b/crates/sqlbuild-refactor/src/refactor_command/tests/test_layer_move.rs index 4b3a867a8b..3785868d76 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/tests/test_layer_move.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/tests/test_layer_move.rs @@ -2,7 +2,9 @@ use sqlbuild_core::cli_text::models::CliStyle; use crate::refactor_command::main::layer_move_suggestion::layer_move_suggestion; use crate::refactor_command::main::render_refactor_text::render_refactor_text; -use crate::refactor_command::models::{LayerMove, RefactorDiagnostic, RefactorStatus}; +use crate::refactor_command::models::{ + BaselineComparison, LayerMove, RefactorDiagnostic, RefactorStatus, +}; use crate::refactor_command::tests::helpers::{rename_plan, rule_diagnostics}; use crate::refactor_command::tests::test_types::{LayerMoveTestCase, RenderTestCase}; use crate::refactoring::models::RefactorPlan; @@ -165,7 +167,11 @@ fn given_layer_failure_when_rendering_text_then_suggestion_follows_errors() { assert_eq!( render_refactor_text( &plan, - (RefactorStatus::CompileFailed, &diagnostics), + ( + RefactorStatus::CompileFailed, + &diagnostics, + &BaselineComparison::default(), + ), suggestion.as_ref(), CliStyle { use_color: false }, ), diff --git a/crates/sqlbuild-refactor/src/refactor_command/tests/test_move_pairs.rs b/crates/sqlbuild-refactor/src/refactor_command/tests/test_move_pairs.rs deleted file mode 100644 index e78ea0e72d..0000000000 --- a/crates/sqlbuild-refactor/src/refactor_command/tests/test_move_pairs.rs +++ /dev/null @@ -1,64 +0,0 @@ -use crate::refactor_command::main::move_pairs::move_pairs; -use crate::refactor_command::models::MoveArguments; -use crate::refactor_command::tests::test_types::MovePairsTestCase; - -#[test] -fn given_words_and_move_file_when_reading_pairs_then_every_pair_is_read_in_order() { - let test_cases = [ - MovePairsTestCase { - description: "pairs on the command line, then from the file", - words: &["orders", "models/finance/", "customers", "models/crm/"], - file: Some("# moves\n\nrefunds\tmodels/finance/\n totals models/marts/t.sql \n"), - expected_pairs: Ok(&[ - ("orders", "models/finance/"), - ("customers", "models/crm/"), - ("refunds", "models/finance/"), - ("totals", "models/marts/t.sql"), - ]), - }, - MovePairsTestCase { - description: "a target without a destination", - words: &["orders", "models/finance/", "customers"], - file: None, - expected_pairs: Err("'customers' has no DESTINATION"), - }, - MovePairsTestCase { - description: "a file line with three words", - words: &[], - file: Some("orders models/finance/ extra\n"), - expected_pairs: Err( - "moves.tsv:1: expected TARGET and DESTINATION separated by a tab or spaces, \ - found 3 words", - ), - }, - MovePairsTestCase { - description: "nothing to move", - words: &[], - file: Some("# only a comment\n"), - expected_pairs: Err("sqb mv needs a TARGET and a DESTINATION"), - }, - ]; - for test_case in test_cases { - let pairs = move_pairs(&MoveArguments { - words: test_case.words.to_vec(), - file: test_case.file.map(|contents| ("moves.tsv", contents)), - }); - - assert_eq!( - pairs.map_err(|error| error.message), - test_case - .expected_pairs - .map(|pairs| { - pairs - .iter() - .map(|(target, destination)| { - ((*target).to_owned(), (*destination).to_owned()) - }) - .collect::>() - }) - .map_err(str::to_owned), - "{}", - test_case.description - ); - } -} diff --git a/crates/sqlbuild-refactor/src/refactor_command/tests/test_refactor_pairs.rs b/crates/sqlbuild-refactor/src/refactor_command/tests/test_refactor_pairs.rs new file mode 100644 index 0000000000..d4ea6b989a --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactor_command/tests/test_refactor_pairs.rs @@ -0,0 +1,98 @@ +use crate::refactor_command::main::refactor_pairs::refactor_pairs; +use crate::refactor_command::models::PairArguments; +use crate::refactor_command::tests::test_types::RefactorPairsTestCase; + +#[test] +fn given_words_and_pair_file_when_reading_pairs_then_every_pair_is_read_in_order() { + let test_cases = [ + RefactorPairsTestCase { + description: "pairs on the command line, then from the file", + is_move: true, + words: &["orders", "models/finance/", "customers", "models/crm/"], + file: Some("# moves\n\nrefunds\tmodels/finance/\n totals models/marts/t.sql \n"), + expected_pairs: Ok(&[ + ("orders", "models/finance/"), + ("customers", "models/crm/"), + ("refunds", "models/finance/"), + ("totals", "models/marts/t.sql"), + ]), + }, + RefactorPairsTestCase { + description: "a target without a destination", + is_move: true, + words: &["orders", "models/finance/", "customers"], + file: None, + expected_pairs: Err("'customers' has no DESTINATION"), + }, + RefactorPairsTestCase { + description: "a file line with three words", + is_move: true, + words: &[], + file: Some("orders models/finance/ extra\n"), + expected_pairs: Err( + "pairs.tsv:1: expected TARGET and DESTINATION separated by a tab or spaces, \ + found 3 words", + ), + }, + RefactorPairsTestCase { + description: "nothing to move", + is_move: true, + words: &[], + file: Some("# only a comment\n"), + expected_pairs: Err("sqb mv needs a TARGET and a DESTINATION"), + }, + RefactorPairsTestCase { + description: "renames on the command line, then from the file", + is_move: false, + words: &["orders", "orders_v2"], + file: Some("customers\tclients\n"), + expected_pairs: Ok(&[("orders", "orders_v2"), ("customers", "clients")]), + }, + RefactorPairsTestCase { + description: "a rename target without a new name", + is_move: false, + words: &["orders", "orders_v2", "customers"], + file: None, + expected_pairs: Err("'customers' has no NEW_NAME"), + }, + RefactorPairsTestCase { + description: "a rename file line with one word", + is_move: false, + words: &[], + file: Some("\norders\n"), + expected_pairs: Err( + "pairs.tsv:2: expected TARGET and NEW_NAME separated by a tab or spaces, \ + found 1 words", + ), + }, + RefactorPairsTestCase { + description: "nothing to rename", + is_move: false, + words: &[], + file: None, + expected_pairs: Err("sqb rename needs a TARGET and a NEW_NAME"), + }, + ]; + for test_case in test_cases { + let pairs = refactor_pairs(&PairArguments { + is_move: test_case.is_move, + words: test_case.words.to_vec(), + file: test_case.file.map(|contents| ("pairs.tsv", contents)), + }); + + assert_eq!( + pairs.map_err(|error| error.message), + test_case + .expected_pairs + .map(|pairs| { + pairs + .iter() + .map(|(target, second)| ((*target).to_owned(), (*second).to_owned())) + .collect::>() + }) + .map_err(str::to_owned), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-refactor/src/refactor_command/tests/test_types.rs b/crates/sqlbuild-refactor/src/refactor_command/tests/test_types.rs index 7a5fd95d7b..c8858bc7bc 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/tests/test_types.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/tests/test_types.rs @@ -18,9 +18,25 @@ pub(super) struct RenderTestCase { } /// Command-line words and a move file, and the pairs or error message they give. -pub(super) struct MovePairsTestCase { +pub(super) struct RefactorPairsTestCase { pub(super) description: &'static str, + pub(super) is_move: bool, pub(super) words: &'static [&'static str], pub(super) file: Option<&'static str>, pub(super) expected_pairs: Result<&'static [(&'static str, &'static str)], &'static str>, } + +/// One diagnostic as `(rule, code, path, message)`. +pub(super) type DiagnosticRow = (bool, &'static str, &'static str, &'static str); + +/// The project's errors before and after file moves, and how they compare. +pub(super) struct BaselineComparisonTestCase { + pub(super) description: &'static str, + pub(super) before: &'static [DiagnosticRow], + pub(super) after: &'static [DiagnosticRow], + /// `(original path, path)` of each moved file. + pub(super) moves: &'static [(&'static str, &'static str)], + pub(super) expected_remaining: &'static [&'static str], + pub(super) expected_resolved: &'static [&'static str], + pub(super) expected_blocking: &'static [&'static str], +} diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/files/workspace.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/files/workspace.rs index d47cfeee1c..a69e0f37d7 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/files/workspace.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/files/workspace.rs @@ -1,9 +1,11 @@ //! Stage edits in a scratch copy, then commit with rollback, as `workspace.py` does. +use std::collections::BTreeSet; use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; +use sqlbuild_core::text::errors::TextDecodeError; use sqlbuild_core::text::main::decode_text::decode_text; use crate::refactoring::_helpers::edits::text_edits::apply_text_edits; @@ -15,7 +17,7 @@ use crate::refactoring::constants::{ }; use crate::refactoring::errors::{RefactorError, RefactorErrorKind}; use crate::refactoring::models::FileChange; -use crate::refactoring::types::Originals; +use crate::refactoring::types::{Originals, ProjectText}; fn io_error(path: &Path, error: impl std::fmt::Display) -> RefactorError { RefactorError { @@ -74,6 +76,52 @@ fn copy_directory(root: &Path, current: &Path, staging_dir: &Path) -> Result Result, RefactorError> { + let mut found: Vec = Vec::new(); + let mut pending: Vec = vec![project_dir.to_path_buf()]; + while let Some(current) = pending.pop() { + let Ok(entries) = fs::read_dir(¤t) else { + continue; + }; + for entry in entries.filter_map(Result::ok) { + let path = entry.path(); + let name = entry.file_name().to_string_lossy().into_owned(); + let is_link = entry.file_type().is_ok_and(|kind| kind.is_symlink()); + if name.starts_with(HIDDEN_PREFIX) { + continue; + } + if path.is_dir() { + let ignored = (current == project_dir + && IGNORED_PROJECT_DIRECTORIES.contains(&name.as_str())) + || name == PYCACHE_DIRECTORY; + if !ignored && !is_link { + pending.push(path); + } + continue; + } + if suffix(&name) != wanted { + continue; + } + let Ok(relative) = path.strip_prefix(project_dir) else { + continue; + }; + let bytes: Vec = fs::read(&path).map_err(|error| io_error(&path, error))?; + let text: Result = decode_text(&bytes); + let relative: Vec = relative + .components() + .map(|part| part.as_os_str().to_string_lossy().into_owned()) + .collect(); + found.push((relative.join("/"), text)); + } + } + found.sort_by(|left, right| left.0.cmp(&right.0)); + Ok(found) +} + /// `Path.read_text(encoding="utf-8")`: strict UTF-8 with universal newlines. fn read_text(path: &Path) -> Result { let bytes = fs::read(path).map_err(|error| io_error(path, error))?; @@ -143,16 +191,24 @@ fn write_text(path: &Path, contents: &str) -> Result<(), RefactorError> { fs::write(path, translated).map_err(|error| io_error(path, error)) } -/// Apply a plan to the scratch copy. +/// Moved sources no change writes to; staging and commit both remove them after every write. +pub(crate) fn vacated_paths(changes: &[FileChange]) -> Vec<&str> { + let written: BTreeSet<&str> = changes.iter().map(|change| change.path.as_str()).collect(); + changes + .iter() + .filter(|change| change.moved()) + .map(|change| change.original_path.as_str()) + .filter(|source| !written.contains(source)) + .collect() +} + +/// Apply a plan to the scratch copy, in the same order as the real write. pub(crate) fn write_staged_changes( staging_dir: &Path, originals: &Originals, changes: &[FileChange], ) -> Result<(), RefactorError> { let contents = edited_contents(originals, changes)?; - for change in changes.iter().filter(|change| change.moved()) { - let _ = fs::remove_file(join(staging_dir, &change.original_path)); - } for change in changes { let target = join(staging_dir, &change.path); if let Some(parent) = target.parent() { @@ -160,6 +216,9 @@ pub(crate) fn write_staged_changes( } write_text(&target, contents_for(&contents, &change.path))?; } + for source in vacated_paths(changes) { + let _ = fs::remove_file(join(staging_dir, source)); + } prune_emptied_directories(staging_dir, changes); Ok(()) } @@ -245,8 +304,8 @@ fn write_all( } write_atomically(&target, contents_for(contents, &change.path))?; } - for change in changes.iter().filter(|change| change.moved()) { - let source = join(project_dir, &change.original_path); + for source in vacated_paths(changes) { + let source = join(project_dir, source); fs::remove_file(&source).map_err(|error| io_error(&source, error))?; } Ok(()) diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs index 3562c4665b..0e75d1d5ed 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs @@ -13,7 +13,7 @@ use crate::refactoring::_helpers::planning::model_planning::{ }; use crate::refactoring::_helpers::scanning::scan_context::ScanContext; use crate::refactoring::errors::RefactorError; -use crate::refactoring::models::{ManualLocation, RefactorFacts}; +use crate::refactoring::models::{ManualLocation, RefactorFacts, RefactorOperation}; use crate::refactoring::types::{DeclarationMoveHost, UnitTestScopeHost}; /// Edits, blockers and file moves of every target, with declarations placed once for all. @@ -103,17 +103,25 @@ fn batch_conflicts(targets: &[ModelTarget<'_>]) -> Vec { .collect(); for target in targets { let model: &str = &target.model.name; + let is_move: bool = target.request.operation == RefactorOperation::MoveModel; + let (verb, participle) = if is_move { + ("move", "moved") + } else { + ("rename", "renamed") + }; if !moved.insert(model) { if reported.insert(model) { conflicts.push(conflict( &target.source_path, - format!("model:{model} is moved more than once in this batch"), + format!("model:{model} is {participle} more than once in this batch"), )); } continue; } let destination: String = target.destination(); - if let Some(other) = by_destination.insert(destination.clone(), model) { + if let Some(other) = by_destination.insert(destination.clone(), model) + && is_move + { conflicts.push(conflict( &destination, format!("model:{other} and model:{model} both move to {destination}"), @@ -129,7 +137,7 @@ fn batch_conflicts(targets: &[ModelTarget<'_>]) -> Vec { &destination, format!( "model:{model} would take the name model:{other} gives up in this batch; \ - the old name must stay free for its history, so move them in separate runs" + the old name must stay free for its history, so {verb} them in separate runs" ), )); } diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs index 9c3eb68681..c363fc9d2a 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs @@ -5,5 +5,6 @@ pub(crate) mod column_references; pub(crate) mod declaration_moves; pub(crate) mod model_planning; pub(crate) mod model_references; +pub(crate) mod python_imports; pub(crate) mod single_planning; pub(crate) mod unit_test_moves; diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/python_imports.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/python_imports.rs new file mode 100644 index 0000000000..4d252d4e5c --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/python_imports.rs @@ -0,0 +1,83 @@ +//! Python imports of the Python modules a refactoring moves. + +use std::path::Path; + +use sqlbuild_core::text::errors::TextDecodeError; +use sqlbuild_pysource::import_rewrites::main::python_import_rewrites::python_import_rewrites; +use sqlbuild_pysource::import_rewrites::models::{ImportRewrites, ModuleMove, PythonSource}; + +use crate::refactoring::_helpers::edits::text_edits::text_edit; +use crate::refactoring::_helpers::files::paths::{name, suffix}; +use crate::refactoring::_helpers::files::workspace::project_files_with_suffix; +use crate::refactoring::_helpers::scanning::chars::chars; +use crate::refactoring::constants::{PYTHON_SUFFIX, UNREADABLE_PYTHON_REASON}; +use crate::refactoring::errors::RefactorError; +use crate::refactoring::models::{EditKind, ManualLocation, TextEdit}; + +/// Import edits by path, and the imports that cannot be rewritten. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub(crate) struct ImportParts { + pub(crate) edits: Vec<(String, TextEdit)>, + pub(crate) blocking: Vec, +} + +/// Import edits for the Python files `moves` moves, and a blocker for each unsafe import. +pub(crate) fn python_import_parts( + project_dir: &Path, + moves: &[(String, String)], +) -> Result { + let modules: Vec = moves + .iter() + .filter(|(source, destination)| { + source != destination + && suffix(name(source)) == PYTHON_SUFFIX + && suffix(name(destination)) == PYTHON_SUFFIX + }) + .map(|(source, destination)| ModuleMove { + from: source.clone(), + to: destination.clone(), + }) + .collect(); + let mut parts = ImportParts::default(); + if modules.is_empty() { + return Ok(parts); + } + let mut sources: Vec = Vec::new(); + for (path, text) in project_files_with_suffix(project_dir, PYTHON_SUFFIX)? { + match text { + Ok(contents) => sources.push(PythonSource { path, contents }), + Err(TextDecodeError { .. }) => parts.blocking.push(ManualLocation { + path, + line: None, + column: None, + reason: UNREADABLE_PYTHON_REASON.to_owned(), + }), + } + } + let rewrites: ImportRewrites = python_import_rewrites(&sources, &modules); + for edit in rewrites.edits { + let Some(source) = sources.iter().find(|source| source.path == edit.path) else { + continue; + }; + let start: usize = source.contents[..edit.start].chars().count(); + let end: usize = start + source.contents[edit.start..edit.end].chars().count(); + parts.edits.push(( + edit.path, + text_edit( + &chars(&source.contents), + (start, end), + edit.replacement, + EditKind::Import, + ), + )); + } + parts + .blocking + .extend(rewrites.refusals.into_iter().map(|refusal| ManualLocation { + path: refusal.path, + line: Some(refusal.line), + column: Some(refusal.column), + reason: refusal.reason, + })); + Ok(parts) +} diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/single_planning.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/single_planning.rs index 1aefb30ae7..7bd5d73743 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/single_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/single_planning.rs @@ -1,10 +1,13 @@ //! One model rename, model move, or column rename planned from compiler facts. +use std::path::Path; + use crate::refactoring::_helpers::edits::text_edits::build_plan; use crate::refactoring::_helpers::planning::column_planning::{ column_rename_context, column_rename_parts, }; use crate::refactoring::_helpers::planning::model_planning::{model_parts, model_target}; +use crate::refactoring::_helpers::planning::python_imports::python_import_parts; use crate::refactoring::_helpers::scanning::scan_context::ScanContext; use crate::refactoring::constants::{COLUMN_MANUAL_HELP, MODEL_MANUAL_HELP}; use crate::refactoring::errors::RefactorError; @@ -36,12 +39,15 @@ pub(crate) fn plan_single( )); } let target = model_target(facts, request)?; - let parts = model_parts(facts, &target, &context, (host, test_host))?; + let mut parts = model_parts(facts, &target, &context, (host, test_host))?; let mut moves = parts.moves.clone(); let destination = target.destination(); if destination != target.source_path { moves.push((target.source_path.clone(), destination)); } + let imports = python_import_parts(Path::new(&facts.project_dir), &moves)?; + parts.edits.extend(imports.edits); + parts.blocking.extend(imports.blocking); Ok(build_plan( target.request, parts, diff --git a/crates/sqlbuild-refactor/src/refactoring/constants.rs b/crates/sqlbuild-refactor/src/refactoring/constants.rs index 85ed832b7d..6a317bd16f 100644 --- a/crates/sqlbuild-refactor/src/refactoring/constants.rs +++ b/crates/sqlbuild-refactor/src/refactoring/constants.rs @@ -2,6 +2,8 @@ pub const MODEL_KIND_PREFIX: &str = "model:"; pub const SQL_SUFFIX: &str = ".sql"; +pub const PYTHON_SUFFIX: &str = ".py"; +pub const UNREADABLE_PYTHON_REASON: &str = "this Python file cannot be read as UTF-8, so its imports of the moved Python files cannot be checked; fix or remove it first"; pub const CURRENT_DIRECTORY: &str = "."; pub const REF_FUNCTION: &str = "__ref"; pub const REF_FIXTURE_PREFIX: &str = "__ref__"; diff --git a/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs b/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs index 85112fd5a3..b9e0b49716 100644 --- a/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs +++ b/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs @@ -1,8 +1,11 @@ //! Plan a model rename, column rename, or one or more model moves from compiler facts. +use std::path::Path; + use crate::refactoring::_helpers::edits::text_edits::build_plan; use crate::refactoring::_helpers::planning::batch_planning::batch_parts; use crate::refactoring::_helpers::planning::model_planning::{ModelTarget, model_target}; +use crate::refactoring::_helpers::planning::python_imports::python_import_parts; use crate::refactoring::_helpers::planning::single_planning::plan_single; use crate::refactoring::_helpers::scanning::scan_context::ScanContext; use crate::refactoring::constants::MODEL_MANUAL_HELP; @@ -30,8 +33,11 @@ pub fn plan_refactor( .iter() .map(|request| model_target(facts, request)) .collect::>()?; - let parts = batch_parts(facts, &targets, &context, (host, test_host))?; + let mut parts = batch_parts(facts, &targets, &context, (host, test_host))?; let moves: Vec<(String, String)> = parts.moves.clone(); + let imports = python_import_parts(Path::new(&facts.project_dir), &moves)?; + parts.edits.extend(imports.edits); + parts.blocking.extend(imports.blocking); let batch: Vec = targets.into_iter().map(|target| target.request).collect(); let mut plan = build_plan( batch[0].clone(), diff --git a/crates/sqlbuild-refactor/src/refactoring/models.rs b/crates/sqlbuild-refactor/src/refactoring/models.rs index dd7f71d15e..45325ff10c 100644 --- a/crates/sqlbuild-refactor/src/refactoring/models.rs +++ b/crates/sqlbuild-refactor/src/refactoring/models.rs @@ -22,6 +22,8 @@ pub enum EditKind { Header, Column, Migration, + /// A Python import of a moved module. + Import, } impl EditKind { @@ -33,6 +35,7 @@ impl EditKind { Self::Header => "header", Self::Column => "column", Self::Migration => "migration", + Self::Import => "import", } } } diff --git a/crates/sqlbuild-refactor/src/refactoring/tests.rs b/crates/sqlbuild-refactor/src/refactoring/tests.rs index b2ab52ea1a..3c5fadaa10 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests.rs @@ -16,5 +16,7 @@ mod sql_sites; mod test_types; #[path = "tests/test_text_edits.rs"] mod text_edits; +#[path = "tests/test_workspace.rs"] +mod workspace; #[path = "tests/test_yaml_edits.rs"] mod yaml_edits; diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs index ef108d5753..1d29494bf6 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs @@ -778,3 +778,29 @@ pub(super) fn planned_batch( .collect(), )) } + +/// A change that moves `original_path` to `path` without edits. +pub(super) fn moved(original_path: &str, path: &str) -> FileChange { + FileChange { + path: path.to_owned(), + original_path: original_path.to_owned(), + edits: Vec::new(), + } +} + +/// Each of `paths` that is a file below `root`, with its contents, sorted by path. +pub(super) fn tree(root: &Path, paths: &[&str]) -> Vec<(String, String)> { + let mut files: Vec<(String, String)> = paths + .iter() + .filter(|path| root.join(path).is_file()) + .map(|path| { + ( + (*path).to_owned(), + fs::read_to_string(root.join(path)).unwrap_or_default(), + ) + }) + .collect(); + files.sort(); + files.dedup(); + files +} diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs index 84ffc994fb..3a74e7187f 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs @@ -248,3 +248,12 @@ pub(super) struct BatchPlanTestCase { pub(super) expected_blocking: &'static [&'static str], pub(super) expected_moves: &'static [&'static str], } + +/// Files on disk, file moves applied to them, and the files left afterwards. +pub(super) struct StagedCommitOrderTestCase { + pub(super) description: &'static str, + pub(super) files: &'static [(&'static str, &'static str)], + /// `(original path, path)` of each moved file, in plan order. + pub(super) changes: &'static [(&'static str, &'static str)], + pub(super) expected_files: &'static [(&'static str, &'static str)], +} diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_workspace.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_workspace.rs new file mode 100644 index 0000000000..b30cae192d --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_workspace.rs @@ -0,0 +1,82 @@ +use crate::refactoring::_helpers::files::workspace::{commit_changes, write_staged_changes}; +use crate::refactoring::models::FileChange; +use crate::refactoring::tests::helpers::{moved, owned_pairs, tree, write_files}; +use crate::refactoring::tests::test_types::StagedCommitOrderTestCase; +use crate::refactoring::types::Originals; + +#[test] +fn given_changes_when_staging_and_committing_then_both_leave_the_same_files() { + let test_cases = [ + StagedCommitOrderTestCase { + description: "a plain move leaves only the destination", + files: &[("a/one.sql", "one"), ("a/two.sql", "two")], + changes: &[("a/one.sql", "b/one.sql")], + expected_files: &[("a/two.sql", "two"), ("b/one.sql", "one")], + }, + StagedCommitOrderTestCase { + description: "a file one change leaves and another moves onto is kept", + files: &[("a.sql", "first"), ("b.sql", "second")], + changes: &[("a.sql", "b.sql"), ("b.sql", "c.sql")], + expected_files: &[("b.sql", "first"), ("c.sql", "second")], + }, + StagedCommitOrderTestCase { + description: "the order of chained changes does not matter", + files: &[("a.sql", "first"), ("b.sql", "second")], + changes: &[("b.sql", "c.sql"), ("a.sql", "b.sql")], + expected_files: &[("b.sql", "first"), ("c.sql", "second")], + }, + ]; + for test_case in test_cases { + let changes: Vec = test_case + .changes + .iter() + .map(|(source, destination)| moved(source, destination)) + .collect(); + let originals: Originals = test_case + .files + .iter() + .filter(|(path, _)| test_case.changes.iter().any(|(source, _)| source == path)) + .map(|(path, text)| ((*path).to_owned(), (*text).to_owned())) + .collect(); + let paths: Vec<&str> = test_case + .files + .iter() + .map(|(path, _)| *path) + .chain( + test_case + .changes + .iter() + .map(|(_, destination)| *destination), + ) + .collect(); + let (staged, committed) = (tempfile::tempdir(), tempfile::tempdir()); + let (Ok(staged), Ok(committed)) = (staged, committed) else { + panic!( + "{}: cannot create temporary directories", + test_case.description + ); + }; + for root in [staged.path(), committed.path()] { + assert!( + write_files(root, test_case.files).is_ok(), + "{}", + test_case.description + ); + } + + let staging = write_staged_changes(staged.path(), &originals, &changes); + let commit = commit_changes(committed.path(), &originals, &changes); + + assert!(staging.is_ok(), "{}: {staging:?}", test_case.description); + assert!(commit.is_ok(), "{}: {commit:?}", test_case.description); + assert_eq!( + (tree(staged.path(), &paths), tree(committed.path(), &paths)), + ( + owned_pairs(test_case.expected_files), + owned_pairs(test_case.expected_files) + ), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-refactor/src/refactoring/types.rs b/crates/sqlbuild-refactor/src/refactoring/types.rs index 33b02dd129..c31e1ae715 100644 --- a/crates/sqlbuild-refactor/src/refactoring/types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/types.rs @@ -1,5 +1,7 @@ //! Callback and alias types shared by refactoring planning and its bindings. +use sqlbuild_core::text::errors::TextDecodeError; + use crate::refactoring::errors::RefactorError; use crate::refactoring::models::DeclarationPlacement; @@ -14,6 +16,9 @@ pub type UnitTestScopeHost<'a> = /// Original texts keyed by original path, in plan order. pub type Originals = Vec<(String, String)>; +/// One project file's POSIX path and its text, or why it is not UTF-8. +pub type ProjectText = (String, Result); + /// One physical column: `(name, type)`. pub type PhysicalColumn = (String, String); diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md index d627319255..5680d39da6 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md @@ -11,8 +11,10 @@ Online: https://sqlbuild.com/docs/cli/rename/ - Usage - Rename or move - Move several models at once +- Rename several models at once - What changes - History +- Existing rule failures - Refusals - Flags - Output @@ -26,7 +28,8 @@ Nothing is written until the edited project compiles. The command applies its ed ## Usage ```bash -sqb rename [flags] +sqb rename [ ...] [flags] +sqb rename --from-file [flags] sqb mv [ ...] [flags] sqb mv --from-file [flags] sqb rename . [--cascade] [flags] @@ -76,6 +79,17 @@ The moves are planned and verified together: the project compiles once before pl A batch is refused when it moves the same model twice, moves two models to the same file, gives two models the same new name, or gives a model the name another model of the batch gives up: the old name keeps working through a compatibility view after the rename, so it must stay free. With `--dry-run`, the output lists the combined edits of every move. +## Rename several models at once + +`sqb rename` takes several ` ` pairs the same way, on the command line or with `--from-file`: + +```bash +sqb rename stg_orders stg_order_lines fact_orders fact_order_lines +sqb rename --from-file renames.tsv --dry-run +``` + +The renames are planned and verified together, with one compile before and one of the edited project after, and each renamed model gets its own `migrate_from`, so every old name keeps working through its compatibility view after the next build. The same refusals as for batch moves apply: a model renamed twice, two models given one name, or a model taking a name another model of the batch gives up. Column renames and `--cascade` take one `. ` per command; a batch that names a column is refused. + ## What changes Renaming or moving a model rewrites: @@ -100,7 +114,16 @@ Moving a model to another folder also moves the macros, enums, constants, reusab - a declaration other resources still use moves to the lowest folder they share, for example `models/staging/_sqlbuild/macros/`; - when that folder would be an authored root such as `models/`, or the users span resource trees such as models and tests, it moves to the top-level role (`macros/`, `enums/`, `constants/`, `schemas/`, ...). -A moved declaration file takes the shape of the role folder it lands in. In an empty role it keeps its group folder, such as `money/` in `_macros/money/cents.py`, or stays direct if it had none. In a role whose files all sit in group folders it keeps its group, and a file without a group is refused: move it into a group folder of its current role first. In a role whose files sit directly in it, the group folder is dropped. Python imports of a moved Python macro file are not rewritten; verification refuses the move when one breaks. +A moved declaration file takes the shape of the role folder it lands in. In an empty role it keeps its group folder, such as `money/` in `_macros/money/cents.py`, or stays direct if it had none. In a role whose files all sit in group folders it keeps its group, and a file without a group is refused: move it into a group folder of its current role first. In a role whose files sit directly in it, the group folder is dropped. + +When a Python file moves, such as a carried macro module, every project Python file that imports it is updated: macros, Python nodes, custom rules, and helpers. `from import ...`, grouped and parenthesized imports, `import as `, and `from import ` point at the new module path. Relative imports keep their style when the new target is close, such as `from ..text.cents import to_cents`, and become absolute when they would climb further. The move is refused, naming the file and line, when an import cannot be rewritten safely: + +- `import ` without `as`, because code reads the dotted name itself; +- `from import *` from the moved module's package; +- `importlib.import_module("")` or `__import__("")` naming the moved module; +- `from import , ` where the other name does not move with it; +- `from import ` when the package's `__init__.py` also binds that name, or when the move renames the module; +- a project Python file that cannot be read as UTF-8, because its imports cannot be checked. Declarations are moved, never copied. Folders the move leaves empty are removed. @@ -124,10 +147,19 @@ A renamed column of an incremental or snapshot model gets ` (migrate_from < The command refuses a rename while the model, or the column, still declares `migrate_from` from an earlier rename. Build it on every target and remove the declaration first. +## Existing rule failures + +A project that already fails some project rules, such as a folder holding more models than a layout rule allows, can still be refactored, so that a move can repair it. Only rule failures are tolerated: a project with syntax, binding, or reference errors is refused before planning, as is any project `sqb compile` cannot load. + +The command compiles the project as `sqb compile` does, including SQL test planning, records its rule failures before planning, and compares the edited project with them. A rule failure of the edited project is existing when the project already had one with the same code, at the same place (a moved file keeps its identity), and with the same message. It is also existing when only its numbers changed and none grew, such as a folder that now holds 41 models instead of 44. Any other change, new numbers, larger numbers, or different words, makes it new or worse, and it blocks the write, as do all errors that are not rule failures. Rules stay enabled throughout; nothing is ignored. + +The output, including `--dry-run`, lists the existing failures under `Existing rule failures`, with how many the change resolved and how many remain. Some rules report a failure at one sample file of a group, so moving part of the group can change the reported place or message and block the move even when it helps. Move the whole group in one batch instead: moving the direct models of a folder into a sub-folder at once, so that it turns from a leaf into a branch, is `sqb mv a models/entry/records/ b models/entry/records/ c models/entry/records/`. + ## Refusals The command refuses, and writes nothing, when: +- the project does not compile before the change, other than existing rule failures; - the new name already belongs to another model or column, or the destination file exists; - a moved declaration file is not in a group folder, but the role folder it must move to keeps its files in group folders; - a declaration a moved model uses must change folder, but its file also holds declarations that must stay, or it is not an authored project file, or a file with its name already exists at the new folder; @@ -135,7 +167,8 @@ The command refuses, and writes nothing, when: - a reference is produced by a macro, or a column is read inside SQL that a macro generates; - a column reaches a model through `SELECT *` in a subquery, a `USING` or `NATURAL` join, or an unqualified name that could belong to several tables; - a downstream model's `SELECT *` would silently rename its output, without `--cascade`; -- Python code or Python node SQL strings name the model or column. +- Python code or Python node SQL strings name the model or column; +- a Python import of a moved Python file cannot be rewritten safely. A rename whose new name belongs to another layer's folder, under the layer-folder rules `SQBRPROJECT102` and `SQBRPROJECT105`, fails verification because `sqb rename` keeps the file where it is. When those rules are the only failures, the output says where the model belongs and prints the one-step [`sqb mv`](#rename-or-move) command, with that layer's folder and the current sub-folders filled in: @@ -158,7 +191,7 @@ References produced by a macro are only possible when a project turns off explic |------|-------------| | `--cascade` | For column renames, rename the output of every downstream model that passes the column through. | | `--dry-run` | Print every edit and verify the edited project compiles, without writing files. | -| `--from-file ` | For `sqb mv`, read ` ` pairs, one per line, and move them together. | +| `--from-file ` | Read ` ` pairs for `sqb mv`, or ` ` pairs for `sqb rename`, one per line, and apply them together. | | `--json` | Print the result as JSON on stdout. Progress goes to stderr. | ## Output @@ -176,7 +209,9 @@ Migrations Compiled: ok, 3 files changed ``` -With `--json`, the result has `operation`, `target`, `new_name`, `destination`, `status` (`applied`, `dry_run`, `refused`, or `compile_failed`), `files` with every edit, `migrations`, `renamed_columns`, `manual`, `blocking`, `notes` (files deliberately kept in place, with the reason), and `compile.errors`. A batch move also has `moves`, every `target` and `destination` in order; `target`, `new_name`, and `destination` then describe its first move. +With `--json`, the result has `operation`, `target`, `new_name`, `destination`, `status` (`applied`, `dry_run`, `refused`, or `compile_failed`), `files` with every edit, `migrations`, `renamed_columns`, `manual`, `blocking`, `notes` (files deliberately kept in place, with the reason), `compile.errors`, and `rule_baseline` with the project's rule failures `before` the change, those `remaining`, and those `resolved`. A batch move also has `moves`, every `target` and `destination` in order, and a batch rename has `renames`, every `target`, `new_name`, and `destination`; `target`, `new_name`, and `destination` then describe the first one. + +When the request is invalid or the project does not compile, `--json` prints an error document instead: `{"error": {"code", "message", "help"}}`, and for a project that does not compile also `"status": "project_failed"` and `compile.errors`. ## Exit codes diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py index ee1dd6d081..d017b36f43 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/_test_types.py @@ -206,3 +206,86 @@ class BatchMoveDryRunE2ETestCase: description: str arguments: tuple[str, ...] expected_lines: tuple[str, ...] + + +@dataclass(frozen=True) +class BatchRenameE2ETestCase: + description: str + arguments: tuple[str, ...] + rename_file: str + expected_files: tuple[str, ...] + expected_fragments: dict[str, tuple[str, ...]] + expected_old_name_views: tuple[str, ...] + + +@dataclass(frozen=True) +class BatchRenameRefusalE2ETestCase: + description: str + arguments: tuple[str, ...] + expected_message: str + + +@dataclass(frozen=True) +class PythonImportMoveE2ETestCase: + description: str + files: dict[str, str] + expected_fragments: dict[str, tuple[str, ...]] + + +@dataclass(frozen=True) +class PythonImportRefusalE2ETestCase: + description: str + files: dict[str, str] + expected_location: str + expected_reason: str + binary_files: dict[str, bytes] = field(default_factory=dict) + + +@dataclass(frozen=True) +class RenameUsageErrorE2ETestCase: + description: str + arguments: tuple[str, ...] + expected_returncode: int + expected_message: str + + +@dataclass(frozen=True) +class ProjectFailureE2ETestCase: + description: str + command: tuple[str, ...] + expected_codes: tuple[str, ...] + + +@dataclass(frozen=True) +class InvalidRequestJsonE2ETestCase: + description: str + command: tuple[str, ...] + expected_code: str + expected_message: str + + +@dataclass(frozen=True) +class LayoutRepairE2ETestCase: + description: str + files: dict[str, str] + arguments: tuple[str, ...] + expected_status: str + expected_baseline: dict[str, int] + expected_blocking: tuple[str, ...] + command: str = "mv" + + +@dataclass(frozen=True) +class RefusedBaselineE2ETestCase: + description: str + files: dict[str, str] + command: tuple[str, ...] + expected_codes: tuple[str, ...] + + +@dataclass(frozen=True) +class LayoutRepairDryRunE2ETestCase: + description: str + files: dict[str, str] + arguments: tuple[str, ...] + expected_lines: tuple[str, ...] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py index b231c94ec4..be42950291 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py @@ -602,3 +602,97 @@ def run_printed_command(command: str) -> subprocess.CompletedProcess[str]: text=True, check=False, ) + + +CENTS_CHECK: str = "python/checks/order_cents.py" + + +def cents_check(imports: str, body: str = ' assert to_cents("1")\n') -> dict[str, str]: + """Return the cents macro and its model plus a Python check importing the macro module.""" + + return { + **CENTS_MACRO, + CENTS_CHECK: ( + "from sqlbuild.checks import check\nfrom sqlbuild.refs import model\n\n" + f"{imports}\n\n\n" + '@check(depends_on=model("stg_orders"))\n' + "def order_cents_present(ctx):\n" + ' """Cents come from the shared macro."""\n' + f"{body}" + " return ctx.pass_()\n" + ), + } + + +_FOLDER_LIMIT_RULE: str = """from collections import Counter + +from sqlbuild.rules import Finding, Project, RuleContext, rule + +_MAXIMUM: int = 3 + + +@rule(code="XSQBRLAYOUT001", message="Folders hold few models", remediation="Split the folder.") +def folder_size(*, project: Project, ctx: RuleContext) -> list[Finding]: + del project + counts: Counter[str] = Counter(model.path.parent.as_posix() for model in ctx.project.models) + return [ + ctx.finding( + subject=folder, message=f"{folder} holds {count} models; the maximum is {_MAXIMUM}" + ) + for folder, count in sorted(counts.items()) + if count > _MAXIMUM + ] +""" + + +def layout_view(name: str, source: str = '__ref("stg_orders")') -> str: + """Return a view model selecting order_id from `source`.""" + + return ( + f"MODEL (description 'Test model {name}.',\n materialized view,\n);\n\n" + f"SELECT order_id FROM {source}\n" + ) + + +OVER_FULL_STAGING: dict[str, str] = { + "sqlbuild_project.toml": project_toml() + + '\n[rules]\nselect = ["XSQBRLAYOUT001"]\n\n' + + "[rules.thresholds]\nmin_custom_rule_test_cases = 0\n", + "rules/layout.py": _FOLDER_LIMIT_RULE, + **{ + f"models/staging/stg_order_{name}.sql": layout_view(f"stg_order_{name}") + for name in ("lines", "refunds", "returns", "notes") + }, +} +_DOMAIN_RULES: str = '\n[rules]\nselect = ["SQBRPROJECT202"]\ndomains = ["orders"]\n' +MIXED_OWNER: dict[str, str] = { + "sqlbuild_project.toml": project_toml() + _DOMAIN_RULES, + "models/orders/staging/orders__stg__lines.sql": layout_view( + "orders__stg__lines", '__source("raw_orders")' + ), + "models/orders/staging/orders__stg__refunds.sql": layout_view( + "orders__stg__refunds", '__source("raw_orders")' + ), + "models/orders/staging/web/orders__stg__web_lines.sql": layout_view( + "orders__stg__web_lines", '__source("raw_orders")' + ), +} +MIXED_OWNER_CHILD: str = "models/orders/staging/core/" + +RESERVED_CTE_FACT_ORDERS: str = ( + "MODEL (description 'Test model.',\n materialized table,\n);\n\n" + 'WITH __helper__tmp AS (SELECT * FROM __ref("stg_orders"))\n' + "SELECT\n o.order_id,\n o.customer_id,\n o.amount,\n o.order_date\n" + "FROM __helper__tmp AS o\n" +) +SIBLING_OWNERS: dict[str, str] = { + "sqlbuild_project.toml": project_toml() + + '\n[rules]\nselect = ["SQBRPROJECT204"]\ndomains = ["orders"]\n\n' + + "[rules.thresholds]\nmin_shared_owner_prefix_directories = 2\n", + **{ + f"models/orders/staging/{folder}/orders__stg__{name}.sql": layout_view( + f"orders__stg__{name}", '__source("raw_orders")' + ) + for folder, name in (("fin_a", "a"), ("fin_b", "b"), ("core", "c")) + }, +} diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_batch_renames.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_batch_renames.py new file mode 100644 index 0000000000..fea7cd4493 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_batch_renames.py @@ -0,0 +1,197 @@ +"""CLI e2e coverage for `sqb rename` renaming several models in one verified step.""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from typing import Any + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.refactor._test_types import ( + BatchRenameE2ETestCase, + BatchRenameRefusalE2ETestCase, + RenameUsageErrorE2ETestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.refactor.helpers import ( + MOVE_FILE_PLACEHOLDER, + existing_files, + fragments_present, + project_text, + relation_type, + sqb, + with_move_file, + write_orders_project, +) + +_COMPILE_STARTS: tuple[str, ...] = ( + "Compiling project...", + "Verifying the edited project compiles...", +) +_RENAMED_FILES: tuple[str, ...] = ( + "models/marts/fact_order_lines.sql", + "models/staging/stg_order_lines.sql", +) +_RENAMED_FRAGMENTS: dict[str, tuple[str, ...]] = { + "models/marts/fact_order_lines.sql": ( + '__ref("stg_order_lines")', + "migrate_from fact_orders", + ), + "models/staging/stg_order_lines.sql": ("migrate_from stg_orders",), + "models/marts/customer_totals.sql": ('__ref("fact_order_lines")',), + "tests/unit/test_fact_order_lines.sql": ( + "__ref__stg_order_lines AS (", + "__expected__fact_order_lines AS (", + ), +} + + +@pytest.mark.parametrize( + "test_case", + [ + BatchRenameE2ETestCase( + description="pairs on the command line", + arguments=("stg_orders", "stg_order_lines", "model:fact_orders", "fact_order_lines"), + rename_file="", + expected_files=_RENAMED_FILES, + expected_fragments=_RENAMED_FRAGMENTS, + expected_old_name_views=("stg_orders", "fact_orders"), + ), + BatchRenameE2ETestCase( + description="pairs from a file with comments and blank lines", + arguments=("--from-file", MOVE_FILE_PLACEHOLDER), + rename_file=( + "# rename the order models together\n\n" + "stg_orders\tstg_order_lines\n fact_orders fact_order_lines \n" + ), + expected_files=_RENAMED_FILES, + expected_fragments=_RENAMED_FRAGMENTS, + expected_old_name_views=("stg_orders", "fact_orders"), + ), + ], + ids=lambda case: case.description, +) +def test_given_several_renames_when_renamed_together_then_one_verification_applies_all( + tmp_path: Path, test_case: BatchRenameE2ETestCase +) -> None: + """Both models rename at once, keep their history, and leave old-name views after a build.""" + + project_dir: Path = write_orders_project(tmp_path=tmp_path) + rename_file: Path = tmp_path / "renames.tsv" + _ = rename_file.write_text(test_case.rename_file, encoding="utf-8") + first_build: subprocess.CompletedProcess[str] = sqb(project_dir, "build") + + renamed: subprocess.CompletedProcess[str] = sqb( + project_dir, "rename", *with_move_file(test_case.arguments, rename_file), "--json" + ) + payload: dict[str, Any] = json.loads(renamed.stdout) + built: subprocess.CompletedProcess[str] = sqb(project_dir, "build") + + assert first_build.returncode == 0, first_build.stdout + first_build.stderr + assert renamed.returncode == 0, renamed.stdout + renamed.stderr + assert payload["status"] == "applied" + assert [item["new_name"] for item in payload["renames"]] == [ + "stg_order_lines", + "fact_order_lines", + ] + assert sum(renamed.stderr.count(start) for start in _COMPILE_STARTS) == 2, renamed.stderr + assert existing_files( + project_dir=project_dir, + paths=( + "models/staging/stg_orders.sql", + "models/marts/fact_orders.sql", + *test_case.expected_files, + ), + ) == tuple(sorted(test_case.expected_files)) + assert fragments_present( + files=project_text(project_dir), expected=test_case.expected_fragments + ), project_text(project_dir) + assert built.returncode == 0, built.stdout + built.stderr + assert { + name: relation_type(project_dir=project_dir, name=name) + for name in test_case.expected_old_name_views + } == dict.fromkeys(test_case.expected_old_name_views, "VIEW") + + +@pytest.mark.parametrize( + "test_case", + [ + BatchRenameRefusalE2ETestCase( + description="a chain that gives a model the name another one gives up", + arguments=("stg_orders", "stg_order_lines", "fact_orders", "stg_orders"), + expected_message=( + "model:fact_orders would take the name model:stg_orders gives up in this batch" + ), + ), + BatchRenameRefusalE2ETestCase( + description="two models given one name", + arguments=("stg_orders", "order_lines", "fact_orders", "order_lines"), + expected_message="would both be named order_lines", + ), + BatchRenameRefusalE2ETestCase( + description="a model renamed twice", + arguments=("stg_orders", "order_lines", "stg_orders", "order_items"), + expected_message="model:stg_orders is renamed more than once in this batch", + ), + BatchRenameRefusalE2ETestCase( + description="a column rename in a batch", + arguments=("stg_orders", "stg_order_lines", "fact_orders.amount", "revenue"), + expected_message=( + "'fact_orders.amount' names a column; several renames in one sqb rename " + "must all be models" + ), + ), + BatchRenameRefusalE2ETestCase( + description="--cascade with several renames", + arguments=("stg_orders.amount", "revenue", "fact_orders", "x", "--cascade"), + expected_message="--cascade applies to one column rename, not to several renames", + ), + BatchRenameRefusalE2ETestCase( + description="a target without a new name", + arguments=("stg_orders", "stg_order_lines", "fact_orders"), + expected_message="'fact_orders' has no NEW_NAME", + ), + ], + ids=lambda case: case.description, +) +def test_given_invalid_batch_rename_when_renaming_then_it_is_refused_and_nothing_changes( + tmp_path: Path, test_case: BatchRenameRefusalE2ETestCase +) -> None: + project_dir: Path = write_orders_project(tmp_path=tmp_path) + before: dict[str, str] = project_text(project_dir) + + renamed: subprocess.CompletedProcess[str] = sqb(project_dir, "rename", *test_case.arguments) + + assert renamed.returncode == 1, renamed.stdout + renamed.stderr + assert test_case.expected_message in " ".join((renamed.stdout + renamed.stderr).split()) + assert project_text(project_dir) == before + + +@pytest.mark.parametrize( + "test_case", + [ + RenameUsageErrorE2ETestCase( + description="a target without a new name", + arguments=("stg_orders",), + expected_returncode=2, + expected_message="the following arguments are required: NEW_NAME", + ), + RenameUsageErrorE2ETestCase( + description="no target and no file", + arguments=(), + expected_returncode=2, + expected_message="the following arguments are required: TARGET, NEW_NAME", + ), + ], + ids=lambda case: case.description, +) +def test_given_rename_without_a_pair_when_parsing_then_it_is_a_usage_error( + tmp_path: Path, test_case: RenameUsageErrorE2ETestCase +) -> None: + project_dir: Path = write_orders_project(tmp_path=tmp_path) + + renamed: subprocess.CompletedProcess[str] = sqb(project_dir, "rename", *test_case.arguments) + + assert renamed.returncode == test_case.expected_returncode, renamed.stdout + renamed.stderr + assert test_case.expected_message in renamed.stderr, renamed.stderr diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_layout_repairs.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_layout_repairs.py new file mode 100644 index 0000000000..9727d6ee6f --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_layout_repairs.py @@ -0,0 +1,214 @@ +"""CLI e2e coverage for `sqb mv` repairing a project that already fails layout rules.""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from typing import Any + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.refactor._test_types import ( + LayoutRepairDryRunE2ETestCase, + LayoutRepairE2ETestCase, + RefusedBaselineE2ETestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.refactor.helpers import ( + MIXED_OWNER, + MIXED_OWNER_CHILD, + OVER_FULL_STAGING, + RESERVED_CTE_FACT_ORDERS, + SIBLING_OWNERS, + layout_view, + project_text, + sqb, + write_orders_project, +) + + +@pytest.mark.parametrize( + "test_case", + [ + LayoutRepairE2ETestCase( + description="moving a model out of an over-full folder keeps the reduced failure", + files=OVER_FULL_STAGING, + arguments=("stg_order_notes", "models/support/"), + expected_status="applied", + expected_baseline={"before": 1, "remaining": 1, "resolved": 0}, + expected_blocking=(), + ), + LayoutRepairE2ETestCase( + description="moving two models out repairs the folder", + files=OVER_FULL_STAGING, + arguments=( + "stg_order_notes", + "models/support/", + "stg_order_returns", + "models/support/", + ), + expected_status="applied", + expected_baseline={"before": 1, "remaining": 0, "resolved": 1}, + expected_blocking=(), + ), + LayoutRepairE2ETestCase( + description="moving a model into the over-full folder worsens it", + files=OVER_FULL_STAGING, + arguments=("fact_orders", "models/staging/"), + expected_status="compile_failed", + expected_baseline={"before": 1, "remaining": 0, "resolved": 0}, + expected_blocking=("XSQBRLAYOUT001 models/staging holds 6 models; the maximum is 3",), + ), + LayoutRepairE2ETestCase( + description="filling another folder past the limit is a new failure", + files=OVER_FULL_STAGING, + arguments=( + "stg_order_notes", + "models/marts/", + "stg_order_returns", + "models/marts/", + ), + expected_status="compile_failed", + expected_baseline={"before": 1, "remaining": 0, "resolved": 1}, + expected_blocking=("XSQBRLAYOUT001 models/marts holds 4 models; the maximum is 3",), + ), + LayoutRepairE2ETestCase( + description="moving every direct model of a mixed owner into a child repairs it", + files=MIXED_OWNER, + arguments=( + "orders__stg__lines", + MIXED_OWNER_CHILD, + "orders__stg__refunds", + MIXED_OWNER_CHILD, + ), + expected_status="applied", + expected_baseline={"before": 1, "remaining": 0, "resolved": 1}, + expected_blocking=(), + ), + LayoutRepairE2ETestCase( + description="a count that grows at a file nothing moves into blocks", + files=SIBLING_OWNERS, + arguments=("orders__stg__c", "models/orders/staging/fin_c/"), + expected_status="compile_failed", + expected_baseline={"before": 1, "remaining": 0, "resolved": 0}, + expected_blocking=("SQBRPROJECT204 3 sibling owner directories",), + ), + LayoutRepairE2ETestCase( + description="a rename that adds another error is blocked despite the baseline", + files=OVER_FULL_STAGING, + command="rename", + arguments=("stg_order_notes", "__expected__notes"), + expected_status="compile_failed", + expected_baseline={"before": 1, "remaining": 0, "resolved": 0}, + expected_blocking=("D016",), + ), + LayoutRepairE2ETestCase( + description="moving only one direct model of a mixed owner still mixes it", + files=MIXED_OWNER, + arguments=("orders__stg__lines", MIXED_OWNER_CHILD), + expected_status="compile_failed", + expected_baseline={"before": 1, "remaining": 0, "resolved": 1}, + expected_blocking=("SQBRPROJECT202",), + ), + ], + ids=lambda case: case.description, +) +def test_given_existing_rule_failures_when_moving_then_only_new_or_worse_failures_block( + tmp_path: Path, test_case: LayoutRepairE2ETestCase +) -> None: + project_dir: Path = write_orders_project(tmp_path=tmp_path, files=test_case.files) + before: dict[str, str] = project_text(project_dir) + + moved: subprocess.CompletedProcess[str] = sqb( + project_dir, test_case.command, *test_case.arguments, "--json" + ) + payload: dict[str, Any] = json.loads(moved.stdout) + + baseline: dict[str, list[dict[str, Any]]] = payload["rule_baseline"] + blocking: tuple[str, ...] = tuple( + f"{error['code']} {error['message']}".strip() for error in payload["compile"]["errors"] + ) + assert payload["status"] == test_case.expected_status, moved.stdout + moved.stderr + assert {key: len(items) for key, items in baseline.items()} == test_case.expected_baseline + assert len(blocking) == len(test_case.expected_blocking), blocking + assert all( + found.startswith(expected) + for found, expected in zip(blocking, test_case.expected_blocking, strict=True) + ), blocking + assert moved.returncode == int(test_case.expected_status != "applied") + assert (project_text(project_dir) == before) == (test_case.expected_status != "applied") + + +@pytest.mark.parametrize( + "test_case", + [ + LayoutRepairDryRunE2ETestCase( + description="an over-full folder losing a model", + files=OVER_FULL_STAGING, + arguments=("stg_order_notes", "models/support/"), + expected_lines=( + "Existing rule failures", + "The project had 1 rule failure before this change; 0 resolved, 1 still failing.", + "└── XSQBRLAYOUT001 models/staging:1 models/staging holds 4 models; " + "the maximum is 3", + "Existing rule failures do not block the change; it adds or worsens none.", + "Dry run: compiles, 1 file would change; nothing written", + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_existing_rule_failures_when_dry_run_then_baseline_and_result_are_explained( + tmp_path: Path, test_case: LayoutRepairDryRunE2ETestCase +) -> None: + project_dir: Path = write_orders_project(tmp_path=tmp_path, files=test_case.files) + before: dict[str, str] = project_text(project_dir) + + moved: subprocess.CompletedProcess[str] = sqb( + project_dir, "mv", *test_case.arguments, "--dry-run" + ) + + assert moved.returncode == 0, moved.stdout + moved.stderr + assert "Compiled project with 1 existing rule failure" in moved.stdout, moved.stdout + assert set(test_case.expected_lines) <= set(moved.stdout.splitlines()), moved.stdout + assert project_text(project_dir) == before + + +@pytest.mark.parametrize( + "test_case", + [ + RefusedBaselineE2ETestCase( + description="an over-full folder and a reference to a missing model", + files={ + **OVER_FULL_STAGING, + "models/marts/order_gaps.sql": layout_view("order_gaps", '__ref("missing_orders")'), + }, + command=("mv", "stg_order_notes", "models/support/"), + expected_codes=("P001",), + ), + RefusedBaselineE2ETestCase( + description="an over-full folder and a unit test that cannot be planned", + files={**OVER_FULL_STAGING, "models/marts/fact_orders.sql": RESERVED_CTE_FACT_ORDERS}, + command=("mv", "customer_totals", "models/support/"), + expected_codes=("XSQBRLAYOUT001", "P001"), + ), + ], + ids=lambda case: case.description, +) +def test_given_rule_failure_and_other_error_when_moving_then_project_is_refused( + tmp_path: Path, test_case: RefusedBaselineE2ETestCase +) -> None: + """Existing rule failures are tolerated, but any other error still stops the move.""" + + project_dir: Path = write_orders_project(tmp_path=tmp_path, files=test_case.files) + before: dict[str, str] = project_text(project_dir) + + moved: subprocess.CompletedProcess[str] = sqb(project_dir, *test_case.command, "--json") + payload: dict[str, Any] = json.loads(moved.stdout) + + assert moved.returncode == 1, moved.stdout + moved.stderr + assert payload["status"] == "project_failed" + assert tuple(error["code"] for error in payload["compile"]["errors"]) == ( + test_case.expected_codes + ), payload + assert project_text(project_dir) == before diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_project_failures.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_project_failures.py new file mode 100644 index 0000000000..20e133c11c --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_project_failures.py @@ -0,0 +1,142 @@ +"""CLI e2e coverage for `sqb rename` and `sqb mv` on projects that do not compile.""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from typing import Any + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.refactor._test_types import ( + InvalidRequestJsonE2ETestCase, + ProjectFailureE2ETestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.refactor.helpers import ( + project_text, + sqb, + write_orders_project, +) + +_DUPLICATE_MACROS: dict[str, str] = { + "macros/cents.py": 'def to_cents(expression: str) -> str:\n return f"({expression}) * 100"\n', + "macros/money.py": 'def to_cents(expression: str) -> str:\n return f"({expression}) * 100"\n', +} +_UNKNOWN_REFERENCE: dict[str, str] = { + "models/marts/order_gaps.sql": ( + "MODEL (description 'Test model order_gaps.',\n materialized view,\n);\n\n" + 'SELECT order_id FROM __ref("missing_orders")\n' + ), +} + + +@pytest.mark.parametrize( + "test_case", + [ + ProjectFailureE2ETestCase( + description="mv on a project with a duplicate macro name", + command=("mv", "fact_orders", "models/reporting/"), + expected_codes=("P001",), + ), + ProjectFailureE2ETestCase( + description="rename on a project with a duplicate macro name", + command=("rename", "fact_orders", "order_facts"), + expected_codes=("P001",), + ), + ], + ids=lambda case: case.description, +) +def test_given_input_stage_failure_when_refactoring_then_compile_error_is_reported_cleanly( + tmp_path: Path, test_case: ProjectFailureE2ETestCase +) -> None: + """A project failing before analysis is refused with its error, in text and as JSON.""" + + project_dir: Path = write_orders_project(tmp_path=tmp_path, files=_DUPLICATE_MACROS) + before: dict[str, str] = project_text(project_dir) + + text: subprocess.CompletedProcess[str] = sqb(project_dir, *test_case.command) + machine: subprocess.CompletedProcess[str] = sqb(project_dir, *test_case.command, "--json") + payload: dict[str, Any] = json.loads(machine.stdout) + + output: str = text.stdout + text.stderr + assert text.returncode == 1, output + assert "Traceback" not in output and "native field" not in output, output + assert "Project does not compile; fix it before" in text.stdout, output + assert all(f"error[{code}]" in text.stderr for code in test_case.expected_codes), output + assert machine.returncode == 1, machine.stdout + machine.stderr + assert (payload["status"], payload["error"]["code"], payload["compile"]["ok"]) == ( + "project_failed", + "C959", + False, + ) + assert tuple(error["code"] for error in payload["compile"]["errors"]) == ( + test_case.expected_codes + ) + assert project_text(project_dir) == before + + +@pytest.mark.parametrize( + "test_case", + [ + ProjectFailureE2ETestCase( + description="rename on a project with an unknown reference", + command=("rename", "fact_orders", "order_facts"), + expected_codes=("P001",), + ), + ProjectFailureE2ETestCase( + description="mv on a project with an unknown reference", + command=("mv", "fact_orders", "models/reporting/"), + expected_codes=("P001",), + ), + ], + ids=lambda case: case.description, +) +def test_given_reference_error_when_refactoring_with_json_then_stdout_is_an_error_document( + tmp_path: Path, test_case: ProjectFailureE2ETestCase +) -> None: + project_dir: Path = write_orders_project(tmp_path=tmp_path, files=_UNKNOWN_REFERENCE) + + machine: subprocess.CompletedProcess[str] = sqb(project_dir, *test_case.command, "--json") + payload: dict[str, Any] = json.loads(machine.stdout) + + assert machine.returncode == 1, machine.stdout + machine.stderr + assert payload["status"] == "project_failed" + assert payload["error"]["message"].startswith("Project does not compile") + assert tuple(error["code"] for error in payload["compile"]["errors"]) == ( + test_case.expected_codes + ), payload + assert "missing_orders" in payload["compile"]["errors"][0]["message"], payload + + +@pytest.mark.parametrize( + "test_case", + [ + InvalidRequestJsonE2ETestCase( + description="rename of a model that does not exist", + command=("rename", "missing_orders", "order_facts"), + expected_code="C951", + expected_message="no model named 'missing_orders' in this project", + ), + InvalidRequestJsonE2ETestCase( + description="mv without a destination", + command=("mv", "fact_orders"), + expected_code="C954", + expected_message="'fact_orders' has no DESTINATION", + ), + ], + ids=lambda case: case.description, +) +def test_given_invalid_request_when_refactoring_with_json_then_stdout_is_an_error_document( + tmp_path: Path, test_case: InvalidRequestJsonE2ETestCase +) -> None: + project_dir: Path = write_orders_project(tmp_path=tmp_path) + + machine: subprocess.CompletedProcess[str] = sqb(project_dir, *test_case.command, "--json") + payload: dict[str, Any] = json.loads(machine.stdout) + + assert machine.returncode == 1, machine.stdout + machine.stderr + assert (payload["error"]["code"], payload["error"]["message"]) == ( + test_case.expected_code, + test_case.expected_message, + ) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_python_import_moves.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_python_import_moves.py new file mode 100644 index 0000000000..d3e8631f7a --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_python_import_moves.py @@ -0,0 +1,163 @@ +"""CLI e2e coverage for `sqb mv` rewriting Python imports of the Python files it moves.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.refactor._test_types import ( + PythonImportMoveE2ETestCase, + PythonImportRefusalE2ETestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.refactor.helpers import ( + CENTS_CHECK, + cents_check, + fragments_present, + project_text, + sqb, + write_orders_project, +) + +_MOVED: str = "models.finance._sqlbuild._macros.cents" + + +@pytest.mark.parametrize( + "test_case", + [ + PythonImportMoveE2ETestCase( + description="direct, aliased and grouped imports of a carried macro module", + files={ + **cents_check( + "from models.staging._sqlbuild._macros.cents import to_cents\n" + "import models.staging._sqlbuild._macros.cents as cents_module\n" + "from models.staging._sqlbuild._macros.cents import (\n to_cents as cents,\n)", + body=' assert to_cents("1") == cents_module.to_cents("1") == cents("1")\n', + ), + }, + expected_fragments={ + CENTS_CHECK: ( + f"from {_MOVED} import to_cents\n", + f"import {_MOVED} as cents_module\n", + f"from {_MOVED} import (\n to_cents as cents,\n)", + ), + }, + ), + PythonImportMoveE2ETestCase( + description="a relative import from a macro module that stays", + files={ + **cents_check( + "from models.staging._sqlbuild._macros import cents as cents_module", + body=' assert cents_module.to_cents("1")\n', + ), + "models/staging/_sqlbuild/_macros/totals.py": ( + "from .cents import to_cents\n\n\n" + "def order_total(expression: str) -> str:\n" + ' return f"SUM({to_cents(expression)})"\n' + ), + "models/staging/stg_order_totals.sql": ( + "MODEL (description 'Test model stg_order_totals.',\n materialized view,\n);\n\n" + 'SELECT\n @order_total("amount") AS total_cents\nFROM __ref("stg_orders")\n' + ), + }, + expected_fragments={ + CENTS_CHECK: ("from macros import cents as cents_module",), + "models/staging/_sqlbuild/_macros/totals.py": ( + "from macros.cents import to_cents\n", + ), + }, + ), + ], + ids=lambda case: case.description, +) +def test_given_python_imports_of_a_moved_macro_when_moving_then_imports_follow_and_project_builds( + tmp_path: Path, test_case: PythonImportMoveE2ETestCase +) -> None: + project_dir: Path = write_orders_project(tmp_path=tmp_path, files=test_case.files) + + moved: subprocess.CompletedProcess[str] = sqb( + project_dir, "mv", "stg_order_cents", "models/finance/" + ) + compiled: subprocess.CompletedProcess[str] = sqb(project_dir, "compile") + checked: subprocess.CompletedProcess[str] = sqb(project_dir, "build") + + assert moved.returncode == 0, moved.stdout + moved.stderr + assert "import " in moved.stdout, moved.stdout + assert fragments_present( + files={ + **project_text(project_dir), + CENTS_CHECK: (project_dir / CENTS_CHECK).read_text(encoding="utf-8"), + }, + expected=test_case.expected_fragments, + ), moved.stdout + assert compiled.returncode == 0, compiled.stdout + compiled.stderr + assert checked.returncode == 0, checked.stdout + checked.stderr + + +@pytest.mark.parametrize( + "test_case", + [ + PythonImportRefusalE2ETestCase( + description="a dynamic import naming the macro module", + files=cents_check( + "import importlib\n\n" + 'CENTS = importlib.import_module("models.staging._sqlbuild._macros.cents")', + body=' assert CENTS.to_cents("1")\n', + ), + expected_location=f"{CENTS_CHECK}:6:9", + expected_reason=( + "imports models.staging._sqlbuild._macros.cents dynamically by name, which " + "cannot be rewritten safely" + ), + ), + PythonImportRefusalE2ETestCase( + description="a star import from the macro module's package", + files=cents_check( + "from models.staging._sqlbuild._macros.cents import *\n" + "from models.staging._sqlbuild._macros import *" + ), + expected_location=f"{CENTS_CHECK}:5:1", + expected_reason=( + "star-imports the package of models.staging._sqlbuild._macros.cents, which moves" + ), + ), + PythonImportRefusalE2ETestCase( + description="an import without an alias", + files=cents_check( + "import models.staging._sqlbuild._macros.cents", + body=' assert models.staging._sqlbuild._macros.cents.to_cents("1")\n', + ), + expected_location=f"{CENTS_CHECK}:4:8", + expected_reason="imports models.staging._sqlbuild._macros.cents without an alias", + ), + PythonImportRefusalE2ETestCase( + description="a project Python file that is not UTF-8", + files=cents_check("from models.staging._sqlbuild._macros.cents import to_cents"), + binary_files={"rules/legacy.py": b"LABEL = '\xe9'\n"}, + expected_location="rules/legacy.py", + expected_reason="this Python file cannot be read as UTF-8", + ), + ], + ids=lambda case: case.description, +) +def test_given_unrewritable_python_import_when_moving_then_move_is_refused_naming_the_line( + tmp_path: Path, test_case: PythonImportRefusalE2ETestCase +) -> None: + project_dir: Path = write_orders_project(tmp_path=tmp_path, files=test_case.files) + for path, contents in test_case.binary_files.items(): + (project_dir / path).parent.mkdir(parents=True, exist_ok=True) + _ = (project_dir / path).write_bytes(contents) + before: dict[str, str] = project_text(project_dir) + check_before: str = (project_dir / CENTS_CHECK).read_text(encoding="utf-8") + + moved: subprocess.CompletedProcess[str] = sqb( + project_dir, "mv", "stg_order_cents", "models/finance/" + ) + + output: str = " ".join(moved.stdout.split()) + assert moved.returncode == 1, moved.stdout + moved.stderr + assert f"{test_case.expected_location} {test_case.expected_reason}" in output, moved.stdout + assert "Refused; no files changed" in moved.stdout + assert project_text(project_dir) == before + assert (project_dir / CENTS_CHECK).read_text(encoding="utf-8") == check_before diff --git a/tests/goldens/compiler/failure/refactor-mv-model.json b/tests/goldens/compiler/failure/refactor-mv-model.json index 6bf7d16bc7..5e1d48b577 100644 --- a/tests/goldens/compiler/failure/refactor-mv-model.json +++ b/tests/goldens/compiler/failure/refactor-mv-model.json @@ -27,6 +27,11 @@ "compile": { "ok": true, "errors": [] + }, + "rule_baseline": { + "before": [], + "remaining": [], + "resolved": [] } } }, diff --git a/tests/goldens/compiler/failure/refactor-rename-column-cascade.json b/tests/goldens/compiler/failure/refactor-rename-column-cascade.json index 50248b4ecf..d166fda85f 100644 --- a/tests/goldens/compiler/failure/refactor-rename-column-cascade.json +++ b/tests/goldens/compiler/failure/refactor-rename-column-cascade.json @@ -46,6 +46,11 @@ "compile": { "ok": true, "errors": [] + }, + "rule_baseline": { + "before": [], + "remaining": [], + "resolved": [] } } }, diff --git a/tests/goldens/compiler/failure/refactor-rename-column-star-refused.json b/tests/goldens/compiler/failure/refactor-rename-column-star-refused.json index fa264ffbf2..985ffd5ca8 100644 --- a/tests/goldens/compiler/failure/refactor-rename-column-star-refused.json +++ b/tests/goldens/compiler/failure/refactor-rename-column-star-refused.json @@ -48,6 +48,11 @@ "compile": { "ok": true, "errors": [] + }, + "rule_baseline": { + "before": [], + "remaining": [], + "resolved": [] } } }, diff --git a/tests/goldens/compiler/failure/refactor-rename-column.json b/tests/goldens/compiler/failure/refactor-rename-column.json index ee6aba0795..1972938bdb 100644 --- a/tests/goldens/compiler/failure/refactor-rename-column.json +++ b/tests/goldens/compiler/failure/refactor-rename-column.json @@ -54,6 +54,11 @@ "compile": { "ok": true, "errors": [] + }, + "rule_baseline": { + "before": [], + "remaining": [], + "resolved": [] } } }, diff --git a/tests/goldens/compiler/failure/refactor-rename-model-collision-refused.json b/tests/goldens/compiler/failure/refactor-rename-model-collision-refused.json index 028a84732f..217eb87ba6 100644 --- a/tests/goldens/compiler/failure/refactor-rename-model-collision-refused.json +++ b/tests/goldens/compiler/failure/refactor-rename-model-collision-refused.json @@ -47,6 +47,11 @@ "compile": { "ok": true, "errors": [] + }, + "rule_baseline": { + "before": [], + "remaining": [], + "resolved": [] } } }, diff --git a/tests/goldens/compiler/failure/refactor-rename-model-dry-run.json b/tests/goldens/compiler/failure/refactor-rename-model-dry-run.json index 1be2e24417..cb5d073d1d 100644 --- a/tests/goldens/compiler/failure/refactor-rename-model-dry-run.json +++ b/tests/goldens/compiler/failure/refactor-rename-model-dry-run.json @@ -40,6 +40,11 @@ "compile": { "ok": true, "errors": [] + }, + "rule_baseline": { + "before": [], + "remaining": [], + "resolved": [] } } }, diff --git a/tests/goldens/compiler/failure/refactor-rename-model-yaml-anchors.json b/tests/goldens/compiler/failure/refactor-rename-model-yaml-anchors.json index d2bdae8b76..0ee7341015 100644 --- a/tests/goldens/compiler/failure/refactor-rename-model-yaml-anchors.json +++ b/tests/goldens/compiler/failure/refactor-rename-model-yaml-anchors.json @@ -60,6 +60,11 @@ "compile": { "ok": true, "errors": [] + }, + "rule_baseline": { + "before": [], + "remaining": [], + "resolved": [] } } }, diff --git a/tests/goldens/compiler/failure/refactor-rename-model.json b/tests/goldens/compiler/failure/refactor-rename-model.json index c12a99df9c..c585797596 100644 --- a/tests/goldens/compiler/failure/refactor-rename-model.json +++ b/tests/goldens/compiler/failure/refactor-rename-model.json @@ -61,6 +61,11 @@ "compile": { "ok": true, "errors": [] + }, + "rule_baseline": { + "before": [], + "remaining": [], + "resolved": [] } } }, diff --git a/tests/native_field_fallbacks.txt b/tests/native_field_fallbacks.txt index 73b4e6c43b..b9818f784f 100644 --- a/tests/native_field_fallbacks.txt +++ b/tests/native_field_fallbacks.txt @@ -62,7 +62,6 @@ crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs CliNamespace.json crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs CliNamespace.left_query crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs CliNamespace.left_query_file crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs CliNamespace.project_dir -crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs CliNamespace.refactor_target crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs CliNamespace.right_query crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs CliNamespace.right_query_file crates/sqlbuild-python/src/bindings/_helpers/cli/dispatch.rs CliNamespace.scenario_command diff --git a/website/src/content/docs/docs/cli/rename.mdx b/website/src/content/docs/docs/cli/rename.mdx index 2488c0b617..84f956f65d 100644 --- a/website/src/content/docs/docs/cli/rename.mdx +++ b/website/src/content/docs/docs/cli/rename.mdx @@ -10,7 +10,8 @@ Nothing is written until the edited project compiles. The command applies its ed ## Usage ```bash -sqb rename [flags] +sqb rename [ ...] [flags] +sqb rename --from-file [flags] sqb mv [ ...] [flags] sqb mv --from-file [flags] sqb rename . [--cascade] [flags] @@ -60,6 +61,17 @@ The moves are planned and verified together: the project compiles once before pl A batch is refused when it moves the same model twice, moves two models to the same file, gives two models the same new name, or gives a model the name another model of the batch gives up: the old name keeps working through a compatibility view after the rename, so it must stay free. With `--dry-run`, the output lists the combined edits of every move. +## Rename several models at once + +`sqb rename` takes several ` ` pairs the same way, on the command line or with `--from-file`: + +```bash +sqb rename stg_orders stg_order_lines fact_orders fact_order_lines +sqb rename --from-file renames.tsv --dry-run +``` + +The renames are planned and verified together, with one compile before and one of the edited project after, and each renamed model gets its own `migrate_from`, so every old name keeps working through its compatibility view after the next build. The same refusals as for batch moves apply: a model renamed twice, two models given one name, or a model taking a name another model of the batch gives up. Column renames and `--cascade` take one `. ` per command; a batch that names a column is refused. + ## What changes Renaming or moving a model rewrites: @@ -84,7 +96,16 @@ Moving a model to another folder also moves the macros, enums, constants, reusab - a declaration other resources still use moves to the lowest folder they share, for example `models/staging/_sqlbuild/macros/`; - when that folder would be an authored root such as `models/`, or the users span resource trees such as models and tests, it moves to the top-level role (`macros/`, `enums/`, `constants/`, `schemas/`, ...). -A moved declaration file takes the shape of the role folder it lands in. In an empty role it keeps its group folder, such as `money/` in `_macros/money/cents.py`, or stays direct if it had none. In a role whose files all sit in group folders it keeps its group, and a file without a group is refused: move it into a group folder of its current role first. In a role whose files sit directly in it, the group folder is dropped. Python imports of a moved Python macro file are not rewritten; verification refuses the move when one breaks. +A moved declaration file takes the shape of the role folder it lands in. In an empty role it keeps its group folder, such as `money/` in `_macros/money/cents.py`, or stays direct if it had none. In a role whose files all sit in group folders it keeps its group, and a file without a group is refused: move it into a group folder of its current role first. In a role whose files sit directly in it, the group folder is dropped. + +When a Python file moves, such as a carried macro module, every project Python file that imports it is updated: macros, Python nodes, custom rules, and helpers. `from import ...`, grouped and parenthesized imports, `import as `, and `from import ` point at the new module path. Relative imports keep their style when the new target is close, such as `from ..text.cents import to_cents`, and become absolute when they would climb further. The move is refused, naming the file and line, when an import cannot be rewritten safely: + +- `import ` without `as`, because code reads the dotted name itself; +- `from import *` from the moved module's package; +- `importlib.import_module("")` or `__import__("")` naming the moved module; +- `from import , ` where the other name does not move with it; +- `from import ` when the package's `__init__.py` also binds that name, or when the move renames the module; +- a project Python file that cannot be read as UTF-8, because its imports cannot be checked. Declarations are moved, never copied. Folders the move leaves empty are removed. @@ -108,10 +129,19 @@ A renamed column of an incremental or snapshot model gets ` (migrate_from < The command refuses a rename while the model, or the column, still declares `migrate_from` from an earlier rename. Build it on every target and remove the declaration first. +## Existing rule failures + +A project that already fails some project rules, such as a folder holding more models than a layout rule allows, can still be refactored, so that a move can repair it. Only rule failures are tolerated: a project with syntax, binding, or reference errors is refused before planning, as is any project `sqb compile` cannot load. + +The command compiles the project as `sqb compile` does, including SQL test planning, records its rule failures before planning, and compares the edited project with them. A rule failure of the edited project is existing when the project already had one with the same code, at the same place (a moved file keeps its identity), and with the same message. It is also existing when only its numbers changed and none grew, such as a folder that now holds 41 models instead of 44. Any other change, new numbers, larger numbers, or different words, makes it new or worse, and it blocks the write, as do all errors that are not rule failures. Rules stay enabled throughout; nothing is ignored. + +The output, including `--dry-run`, lists the existing failures under `Existing rule failures`, with how many the change resolved and how many remain. Some rules report a failure at one sample file of a group, so moving part of the group can change the reported place or message and block the move even when it helps. Move the whole group in one batch instead: moving the direct models of a folder into a sub-folder at once, so that it turns from a leaf into a branch, is `sqb mv a models/entry/records/ b models/entry/records/ c models/entry/records/`. + ## Refusals The command refuses, and writes nothing, when: +- the project does not compile before the change, other than existing rule failures; - the new name already belongs to another model or column, or the destination file exists; - a moved declaration file is not in a group folder, but the role folder it must move to keeps its files in group folders; - a declaration a moved model uses must change folder, but its file also holds declarations that must stay, or it is not an authored project file, or a file with its name already exists at the new folder; @@ -119,7 +149,8 @@ The command refuses, and writes nothing, when: - a reference is produced by a macro, or a column is read inside SQL that a macro generates; - a column reaches a model through `SELECT *` in a subquery, a `USING` or `NATURAL` join, or an unqualified name that could belong to several tables; - a downstream model's `SELECT *` would silently rename its output, without `--cascade`; -- Python code or Python node SQL strings name the model or column. +- Python code or Python node SQL strings name the model or column; +- a Python import of a moved Python file cannot be rewritten safely. A rename whose new name belongs to another layer's folder, under the layer-folder rules `SQBRPROJECT102` and `SQBRPROJECT105`, fails verification because `sqb rename` keeps the file where it is. When those rules are the only failures, the output says where the model belongs and prints the one-step [`sqb mv`](#rename-or-move) command, with that layer's folder and the current sub-folders filled in: @@ -142,7 +173,7 @@ References produced by a macro are only possible when a project turns off explic |------|-------------| | `--cascade` | For column renames, rename the output of every downstream model that passes the column through. | | `--dry-run` | Print every edit and verify the edited project compiles, without writing files. | -| `--from-file ` | For `sqb mv`, read ` ` pairs, one per line, and move them together. | +| `--from-file ` | Read ` ` pairs for `sqb mv`, or ` ` pairs for `sqb rename`, one per line, and apply them together. | | `--json` | Print the result as JSON on stdout. Progress goes to stderr. | ## Output @@ -160,7 +191,9 @@ Migrations Compiled: ok, 3 files changed ``` -With `--json`, the result has `operation`, `target`, `new_name`, `destination`, `status` (`applied`, `dry_run`, `refused`, or `compile_failed`), `files` with every edit, `migrations`, `renamed_columns`, `manual`, `blocking`, `notes` (files deliberately kept in place, with the reason), and `compile.errors`. A batch move also has `moves`, every `target` and `destination` in order; `target`, `new_name`, and `destination` then describe its first move. +With `--json`, the result has `operation`, `target`, `new_name`, `destination`, `status` (`applied`, `dry_run`, `refused`, or `compile_failed`), `files` with every edit, `migrations`, `renamed_columns`, `manual`, `blocking`, `notes` (files deliberately kept in place, with the reason), `compile.errors`, and `rule_baseline` with the project's rule failures `before` the change, those `remaining`, and those `resolved`. A batch move also has `moves`, every `target` and `destination` in order, and a batch rename has `renames`, every `target`, `new_name`, and `destination`; `target`, `new_name`, and `destination` then describe the first one. + +When the request is invalid or the project does not compile, `--json` prints an error document instead: `{"error": {"code", "message", "help"}}`, and for a project that does not compile also `"status": "project_failed"` and `compile.errors`. ## Exit codes