diff --git a/crates/sqlbuild-cli/src/argv/command_table.json b/crates/sqlbuild-cli/src/argv/command_table.json index bd81c7e6ef..2a81e8076a 100644 --- a/crates/sqlbuild-cli/src/argv/command_table.json +++ b/crates/sqlbuild-cli/src/argv/command_table.json @@ -4991,7 +4991,7 @@ { "kind": "store", "dest": "refactor_target", - "required": true, + "nargs": "?", "help": "; the model: prefix is optional", "metavar": "TARGET", "group": 0 @@ -4999,11 +4999,30 @@ { "kind": "store", "dest": "refactor_destination", - "required": true, + "nargs": "?", "help": "new .sql path or folder/", "metavar": "DESTINATION", "group": 0 }, + { + "kind": "store", + "dest": "refactor_more_moves", + "nargs": "*", + "default": [], + "help": "further TARGET DESTINATION pairs, moved together with the first", + "metavar": "MORE", + "group": 0 + }, + { + "kind": "store", + "options": [ + "--from-file" + ], + "dest": "refactor_from_file", + "help": "read TARGET DESTINATION 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 e343239742..dd249f7d96 100644 --- a/crates/sqlbuild-cli/src/argv/constants.rs +++ b/crates/sqlbuild-cli/src/argv/constants.rs @@ -63,7 +63,7 @@ pub(crate) const INT_CONVERTER: &str = "int"; pub(crate) const PATH_CONVERTER: &str = "Path"; /// Every `CliNamespace` attribute with a default, in declaration order. -pub(crate) const CLI_NAMESPACE_DEFAULTS: [(&str, NamespaceDefault); 174] = [ +pub(crate) const CLI_NAMESPACE_DEFAULTS: [(&str, NamespaceDefault); 176] = [ ("command", NamespaceDefault::None), ("project_dir", NamespaceDefault::None), ("dbt_project_dir", NamespaceDefault::None), @@ -252,5 +252,7 @@ pub(crate) const CLI_NAMESPACE_DEFAULTS: [(&str, NamespaceDefault); 174] = [ ("refactor_target", NamespaceDefault::None), ("refactor_new_name", NamespaceDefault::None), ("refactor_destination", NamespaceDefault::None), + ("refactor_more_moves", NamespaceDefault::EmptyList), + ("refactor_from_file", NamespaceDefault::None), ("refactor_cascade", NamespaceDefault::Bool(false)), ]; diff --git a/crates/sqlbuild-cli/src/dispatch/constants.rs b/crates/sqlbuild-cli/src/dispatch/constants.rs index b32def4996..678dcdc0cf 100644 --- a/crates/sqlbuild-cli/src/dispatch/constants.rs +++ b/crates/sqlbuild-cli/src/dispatch/constants.rs @@ -504,11 +504,13 @@ pub(crate) const JANITOR_ROUTE: Route = routed( REQUEST_KEYWORD_SHAPE, ); -const REFACTOR_FIELDS: [Field; 9] = [ +const REFACTOR_FIELDS: [Field; 11] = [ field("command", Source::Text("command")), attr("target", "refactor_target"), attr("new_name", "refactor_new_name"), attr("destination", "refactor_destination"), + field("more_moves", Source::TupleOf("refactor_more_moves")), + attr("from_file", "refactor_from_file"), PROJECT_DIR, attr("cascade", "refactor_cascade"), same("dry_run"), diff --git a/crates/sqlbuild-cli/src/dispatch/main/command_dispatch.rs b/crates/sqlbuild-cli/src/dispatch/main/command_dispatch.rs index 2a8444ef27..c51b2ed43d 100644 --- a/crates/sqlbuild-cli/src/dispatch/main/command_dispatch.rs +++ b/crates/sqlbuild-cli/src/dispatch/main/command_dispatch.rs @@ -21,10 +21,11 @@ pub fn command_dispatch(facts: &CommandFacts) -> Dispatch { let checked = |route: &'static Route, failure: Option| Dispatch::Run(route, failure); match command { - "rename" | "mv" => checked( + "rename" => checked( &REFACTOR_ROUTE, (!facts.refactor_target).then(|| unavailable_handler(command, "C950")), ), + "mv" => run(&REFACTOR_ROUTE), "contract" => checked( &CONTRACT_ROUTE, (!facts.contract_ready).then(|| { 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 1369902d70..97bf5c9156 100644 --- a/crates/sqlbuild-cli/src/dispatch/tests/test_command_dispatch.rs +++ b/crates/sqlbuild-cli/src/dispatch/tests/test_command_dispatch.rs @@ -89,16 +89,24 @@ fn given_parsed_command_when_dispatching_then_selects_python_dispatcher_route() expected_dispatch: runs(&SCENARIO_CAPTURE_ROUTE), }, CommandDispatchTestCase { - description: "mv without a target fails", + description: "rename without a target fails", facts: CommandFacts { refactor_target: false, - ..facts("mv", None) + ..facts("rename", None) }, expected_dispatch: Dispatch::Run( &REFACTOR_ROUTE, - Some(failure("mv command handler is unavailable", "C950")), + Some(failure("rename command handler is unavailable", "C950")), ), }, + CommandDispatchTestCase { + description: "mv reads its pairs itself, so it may come only from a file", + facts: CommandFacts { + refactor_target: false, + ..facts("mv", None) + }, + expected_dispatch: runs(&REFACTOR_ROUTE), + }, CommandDispatchTestCase { description: "clean has no handler", facts: facts("clean", None), diff --git a/crates/sqlbuild-python/src/bindings/_helpers/records/constants.rs b/crates/sqlbuild-python/src/bindings/_helpers/records/constants.rs index 6bb05612f8..f29979c521 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/records/constants.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/records/constants.rs @@ -652,9 +652,11 @@ const REFACTOR_COMMAND_REQUEST: RecordSpec = RecordSpec { doc: "CLI inputs for one `sqb rename` or `sqb mv` invocation.", fields: &[ FieldSpec::required("command"), - FieldSpec::required("target"), + FieldSpec::new("target", FieldDefault::None), FieldSpec::new("new_name", FieldDefault::None), FieldSpec::new("destination", FieldDefault::None), + FieldSpec::new("more_moves", FieldDefault::EmptyTuple), + FieldSpec::new("from_file", FieldDefault::None), FieldSpec::new("project_dir", FieldDefault::None), FieldSpec::new("cascade", FieldDefault::False), FieldSpec::new("dry_run", FieldDefault::False), 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 46dad3bdc0..1e640cde14 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_command.rs @@ -1,7 +1,7 @@ //! `sqb rename` and `sqb mv`: plan from compiler facts, verify in a scratch copy, then write. use std::cell::RefCell; -use std::collections::HashMap; +use std::collections::{BTreeSet, HashMap}; use std::path::{Path, PathBuf}; use std::time::Instant; @@ -15,11 +15,12 @@ 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::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::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, RefactorDiagnostic, RefactorStatus, RefactorTargetRequest, + LayerMove, MoveArguments, RefactorDiagnostic, RefactorStatus, RefactorTargetRequest, }; use sqlbuild_refactor::refactoring::errors::{RefactorError, RefactorErrorKind}; use sqlbuild_refactor::refactoring::main::commit_refactor_plan::commit_refactor_plan; @@ -49,6 +50,7 @@ const STAGING_PREFIX: &str = "sqb-refactor-"; const MOVE_COMMAND: &str = "mv"; const EDIT_ERROR_CODE: &str = "C957"; const WRITE_ERROR_CODE: &str = "C958"; +const MOVE_FILE_ERROR_CODE: &str = "C954"; fn imported<'py>(py: Python<'py>, module: &str, name: &str) -> PyResult> { py.import(module)?.getattr(name) @@ -212,28 +214,88 @@ fn target_lookup(all_keys: &Bound<'_, PyAny>) -> PyResult) -> PyResult { - Ok(RefactorTargetRequest { - is_move: native_field(request, "command")? - .str()? - .extract::()? - == MOVE_COMMAND, - target: native_field(request, "target")?.extract()?, - new_name: native_field(request, "new_name")?.extract()?, - destination: native_field(request, "destination")?.extract()?, - cascade: native_field(request, "cascade")?.extract()?, - }) +fn move_command(request: &Bound<'_, PyAny>) -> PyResult { + Ok(native_field(request, "command")? + .str()? + .extract::()? + == MOVE_COMMAND) +} + +/// The target of each refactoring the command asks for: every move pair, or one rename. +fn target_requests( + py: Python<'_>, + request: &Bound<'_, PyAny>, +) -> PyResult> { + 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 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| { + user_error( + py, + ( + format!("cannot read move file {path}: {error}"), + MOVE_FILE_ERROR_CODE, + None, + ), + ) + })?), + }; + let mut words: Vec<&str> = target + .iter() + .chain(&destination) + .map(String::as_str) + .collect(); + words.extend(more.iter().map(String::as_str)); + let arguments: MoveArguments<'_> = MoveArguments { + 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))?; + Ok(pairs + .into_iter() + .map(|(target, destination)| RefactorTargetRequest { + is_move: true, + target, + new_name: None, + destination: Some(destination), + cascade: false, + }) + .collect()) } fn plan( py: Python<'_>, project: &Bound<'_, PyAny>, - request: &RefactorRequest, + requests: &[RefactorRequest], ) -> PyResult { - let facts: RefactorFacts = refactor_facts(project, request)?; + let facts: RefactorFacts = refactor_facts(project, requests)?; + let moving: BTreeSet<&str> = requests + .iter() + .filter(|_| requests.len() > 1) + .map(|request| request.model_name.as_str()) + .collect(); + let vacated: BTreeSet = facts + .models + .iter() + .filter(|model| moving.contains(model.name.as_str())) + .map(|model| model.path.clone()) + .collect(); let host_error: RefCell> = RefCell::new(None); - let host = |model: &str, destination: &str| -> Result { - declaration_placement(project, (model, destination)).map_err(|error| { + let host = |moves: &[(String, String)]| -> Result { + declaration_placement(project, moves, &vacated).map_err(|error| { host_error.replace(Some(error)); RefactorError::internal("the declaration-move host failed") }) @@ -245,7 +307,7 @@ fn plan( }) }; let planned: Result = - plan_refactor(&facts, request, &host, &test_host); + plan_refactor(&facts, requests, &host, &test_host); if let Some(error) = host_error.into_inner() { return Err(error); } @@ -301,6 +363,7 @@ fn verify( } 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)?; @@ -318,11 +381,13 @@ fn run(py: Python<'_>, command: &Command<'_>, status: &Status<'_>) -> PyResult = targets + .iter() + .map(|target| refactor_request_for_target(target, &lookup)) + .collect::>() + .map_err(|error| refactor_error(py, error))?; status.start("Planning edits...")?; - let planned: RefactorPlan = plan(py, &before.project, &request)?; + 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.")?; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs index 743f5b6a6a..e976064a6f 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs @@ -1,24 +1,23 @@ //! Refactoring facts read from a compiled project, and the shared scope placement host. -use std::collections::HashMap; +use std::collections::{BTreeSet, HashMap}; -use pyo3::prelude::{Bound, Py, PyAny, PyAnyMethods, PyResult, Python}; -use pyo3::types::{PyDict, PyDictMethods, PyString, PyTuple}; +use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyResult, Python}; +use pyo3::types::{PyDict, PyDictMethods, PyString, PyTuple, PyTupleMethods}; use sqlbuild_refactor::refactoring::models::{ AuthoredFile, DeclarationPlacement, DependencyFacts, DestinationFacts, DiscoveredFile, ExpansionFacts, ExpansionSpan, LocationFacts, ManualLocation, ModelFacts, PlacedDeclaration, RefactorFacts, RefactorOperation, RefactorRequest, RelocatedDeclaration, ResourceColumnsFacts, SchemaColumnFacts, SqlFileRole, UnitTestFacts, }; +use sqlbuild_scopes::scope_index::models::{ResourceIdentity, ResourceKind}; use crate::bindings::_helpers::model_config::config_values::config_str; -use crate::bindings::_helpers::records::construct::{enum_member, new_record}; use crate::bindings::_helpers::records::fields::field as native_field; -use crate::bindings::_helpers::records::ids::RecordId; use crate::bindings::_helpers::refactor::python_strings::find_python_string_locations; use crate::bindings::_helpers::scopes::scope_lookup::build_scope_lookup; use crate::bindings::_helpers::scopes::scope_reports::{scope_move_preview, scope_rebound_usages}; -use crate::bindings::_helpers::scopes::scope_validation::scope_relocated_declarations; +use crate::bindings::_helpers::scopes::scope_validation::relocated_declarations; const GENERIC_DIALECT: &str = "generic"; const MATERIALIZED_KEY: &str = "materialized"; @@ -306,11 +305,15 @@ fn python_locations( Ok(locations) } -/// The facts one refactoring plan reads. +/// The facts one refactoring plan of `requests` reads. pub(crate) fn refactor_facts( project: &Bound<'_, PyAny>, - request: &RefactorRequest, + requests: &[RefactorRequest], ) -> PyResult { + let mut locations: Vec = Vec::new(); + for request in requests { + locations.extend(python_locations(project, request)?); + } let compiled: Bound<'_, PyAny> = native_field(&native_field(project, "graph")?, "project")?; let discovered: Bound<'_, PyAny> = native_field(project, "discovered")?; let dialect: Option = native_field(&compiled, "sql_analysis_dialect")?.extract()?; @@ -332,7 +335,7 @@ pub(crate) fn refactor_facts( authored_files: authored_files(&discovered)?, yaml_files, unit_tests: unit_tests(&compiled)?, - python_locations: python_locations(project, request)?, + python_locations: locations, }) } @@ -371,10 +374,11 @@ pub(crate) fn unit_test_rebindings( scope_rebound_usages(py, &lookup, paths) } -/// Where the shared scope placement puts declarations when `model` moves to `destination`. +/// Where scope placement puts declarations once every `(model, destination)` moves together. pub(crate) fn declaration_placement( project: &Bound<'_, PyAny>, - (model, destination): (&str, &str), + moves: &[(String, String)], + vacated: &BTreeSet, ) -> PyResult { let py: Python<'_> = project.py(); let index: Bound<'_, PyAny> = native_field( @@ -382,25 +386,43 @@ pub(crate) fn declaration_placement( "scope_index", )?; let lookup: Bound<'_, PyAny> = build_scope_lookup(py, &index)?.into_bound(py); - let (preview, diagnostics) = scope_move_preview( - py, - &lookup, - &PyString::new(py, &format!("{MODEL_KIND_PREFIX}{model}")).into_any(), - &PyString::new(py, destination).into_any(), - )?; - let diagnostics: Bound<'_, PyAny> = diagnostics.into_bound(py); - let relocated: Option> = if preview.is_none() { - None - } else { - let options: Bound<'_, PyDict> = PyDict::new(py); - options.set_item("kind", enum_member(py, RecordId::ResourceKind, "MODEL")?)?; - options.set_item("name", model)?; - let resource: Bound<'_, PyAny> = - new_record(py, RecordId::ResourceIdentity, (), Some(&options))?; - let records: Option> = - scope_relocated_declarations(py, &index, &resource, destination)?; - if let Some(records) = records { - let records: Bound<'_, PyAny> = records.into_bound(py).into_any(); + let mut messages: Vec = Vec::new(); + let mut previewed: bool = true; + for (model, destination) in moves.iter().filter(|(_, path)| !vacated.contains(path)) { + let (preview, diagnostics) = scope_move_preview( + py, + &lookup, + &PyString::new(py, &format!("{MODEL_KIND_PREFIX}{model}")).into_any(), + &PyString::new(py, destination).into_any(), + )?; + for item in diagnostics.into_bound(py).try_iter()? { + messages.push(native_field(&item?, "message")?.extract()?); + } + if preview.is_none() { + previewed = false; + break; + } + } + let relocated: Option> = if previewed { + let resources: Vec<(ResourceIdentity, String)> = moves + .iter() + .map(|(model, destination)| { + let resource: ResourceIdentity = ResourceIdentity { + kind: ResourceKind::Model, + name: model.clone(), + }; + (resource, destination.clone()) + }) + .collect(); + let records = relocated_declarations(&index, &resources)?; + if let Some((records, flags)) = records { + let mut grouping: HashMap = HashMap::new(); + for (record, requires_group) in records.iter().zip(flags) { + grouping.insert( + identity_key(&native_field(&record, "identity")?)?, + requires_group, + ); + } let sort: Bound<'_, PyDict> = PyDict::new(py); sort.set_item( "key", @@ -416,8 +438,10 @@ pub(crate) fn declaration_placement( .try_iter()? { let record: Bound<'_, PyAny> = record?; + let key: String = identity_key(&native_field(&record, "identity")?)?; relocated.push(RelocatedDeclaration { - key: identity_key(&native_field(&record, "identity")?)?, + requires_group: grouping.get(&key).copied().unwrap_or(false), + key, path: native_field(&record, "path")?.extract()?, }); } @@ -425,6 +449,8 @@ pub(crate) fn declaration_placement( } else { None } + } else { + None }; let mut declarations: Vec = Vec::new(); for record in native_field(&index, "declarations")?.try_iter()? { @@ -442,10 +468,7 @@ pub(crate) fn declaration_placement( } Ok(DeclarationPlacement { relocated, - diagnostics: diagnostics - .try_iter()? - .map(|item| native_field(&item?, "message")?.extract()) - .collect::>()?, + diagnostics: messages, declarations, }) } diff --git a/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_validation.rs b/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_validation.rs index 973f36e8b8..c9e7527752 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_validation.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/scopes/scope_validation.rs @@ -8,7 +8,7 @@ use pyo3::{IntoPyObjectExt, pyfunction, wrap_pyfunction}; use sqlbuild_scopes::compile_usages::models::UsageKey; use sqlbuild_scopes::placement::main::diagnostic_order::diagnostic_order; use sqlbuild_scopes::placement::main::placement_diagnostics::placement_diagnostics; -use sqlbuild_scopes::placement::main::relocate_for_resource_move::relocate_for_resource_move; +use sqlbuild_scopes::placement::main::relocate_for_resource_moves::relocate_for_resource_moves; use sqlbuild_scopes::placement::models::{ DiagnosticOrderKey, PlacementCode, PlacementDiagnostic, PlacementIndex, Relocation, }; @@ -285,34 +285,38 @@ pub(crate) fn scope_placement_validated_index( signature = (*, index, resource, destination) )] pub(crate) fn scope_relocated_declarations( - py: Python<'_>, index: &Bound<'_, PyAny>, resource: &Bound<'_, PyAny>, destination: &str, ) -> PyResult>> { + let resource: ResourceIdentity = resource_identity(identity_row(resource)?)?; + let relocated = relocated_declarations(index, &[(resource, destination.to_owned())])?; + Ok(relocated.map(|(records, _)| records.unbind())) +} + +/// Relocated records after every move in `moves`, and per record whether it lacks a needed group. +pub(crate) fn relocated_declarations<'py>( + index: &Bound<'py, PyAny>, + moves: &[(ResourceIdentity, String)], +) -> PyResult, Vec)>> { + let py: Python<'py> = index.py(); if !read_completeness(index)?[RUNTIME_USAGE] { return Ok(None); } let placement: PlacementIndex = read_placement_index(index)?; - let resource: ResourceIdentity = resource_identity(identity_row(resource)?)?; let relocations: Option> = py - .compiler_detach(|| { - Ok(relocate_for_resource_move( - &placement, - &resource, - destination, - )) - }) + .compiler_detach(|| Ok(relocate_for_resource_moves(&placement, moves))) .map_err(compiler_error)?; let Some(relocations) = relocations else { return Ok(None); }; let classes: ScopeClasses<'_> = ScopeClasses::new(py)?; let records: Bound<'_, PyTuple> = tuple_attribute(index, "declarations")?; - let mut moved: Vec> = Vec::with_capacity(relocations.len()); + let mut moved: Vec> = Vec::with_capacity(relocations.len()); + let mut grouping: Vec = Vec::with_capacity(relocations.len()); for item in relocations { let owning_path: Bound<'_, PyAny> = item.owning_path.into_bound_py_any(py)?; - moved.push(replace( + let record: Bound<'py, PyAny> = replace( &records.get_item(item.declaration)?, &[ ("path", item.path.into_bound_py_any(py)?), @@ -326,9 +330,11 @@ pub(crate) fn scope_relocated_declarations( ), ("owning_path", owning_path), ], - )?); + )?; + moved.push(record); + grouping.push(item.requires_group); } - Ok(Some(PyTuple::new(py, moved)?.unbind())) + Ok(Some((PyTuple::new(py, moved)?, grouping))) } fn usage_records<'py>(records: &Bound<'py, PyAny>) -> PyResult>> { diff --git a/crates/sqlbuild-refactor/src/refactor_command/_helpers/json.rs b/crates/sqlbuild-refactor/src/refactor_command/_helpers/json.rs index f55f510774..de323bcaf4 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/_helpers/json.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/_helpers/json.rs @@ -72,6 +72,21 @@ pub(crate) fn location(item: &ManualLocation) -> JsonValue { ]) } +/// Every move of a batch as `{"target": "model:", "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())), + ]) + }) + .collect(), + ) +} + pub(crate) fn operation_value(plan: &RefactorPlan) -> &'static str { match plan.request.operation { RefactorOperation::RenameModel => "rename_model", 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 2af9c4ee12..b6cb78b12a 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/_helpers/output_text.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/_helpers/output_text.rs @@ -16,6 +16,13 @@ use crate::refactoring::models::{ /// `model:` or `column:.`. pub(crate) fn refactor_target(plan: &RefactorPlan) -> String { + if !plan.batch.is_empty() { + return format_count_noun( + i64::try_from(plan.batch.len()).unwrap_or(i64::MAX), + "model", + None, + ); + } let request = &plan.request; if request.operation == RefactorOperation::RenameColumn { return format!( @@ -28,6 +35,9 @@ 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 request = &plan.request; match request.operation { RefactorOperation::RenameColumn => format!( @@ -50,6 +60,29 @@ pub(crate) fn heading(plan: &RefactorPlan) -> String { } } +/// One `model -> destination` line per move of a batch. +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), + request.destination.as_deref().unwrap_or_default() + ) + }), + ) +} + +/// Each item on its own tree branch, the last one closing the tree. +fn tree_items(style: CliStyle, items: impl ExactSizeIterator) -> Vec { + let count: usize = items.len(); + items + .enumerate() + .map(|(index, item)| format!("{} {item}", tree_connector(style, index + 1 == count))) + .collect() +} + pub(crate) fn verb(plan: &RefactorPlan) -> &'static str { if plan.request.operation == RefactorOperation::MoveModel { "move" @@ -104,19 +137,17 @@ pub(crate) fn change_lines(style: CliStyle, changes: &[FileChange]) -> Vec Vec { - migrations - .iter() - .enumerate() - .map(|(index, item)| { + tree_items( + style, + migrations.iter().map(|item| { format!( - "{} {} {} {}", - tree_connector(style, index + 1 == migrations.len()), + "{} {} {}", style.object_name(&item.model_name), item.declaration, style.muted(&item.reason) ) - }) - .collect() + }), + ) } pub(crate) fn position(location: &ManualLocation) -> String { diff --git a/crates/sqlbuild-refactor/src/refactor_command/constants.rs b/crates/sqlbuild-refactor/src/refactor_command/constants.rs index 4d89a600db..c7980c7141 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/constants.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/constants.rs @@ -6,6 +6,9 @@ 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 MOVE_PAIRS_USAGE: &str = + "pass TARGET DESTINATION pairs, or --from-file with one pair per line"; +pub(crate) const MOVE_FILE_COMMENT: char = '#'; pub(crate) const COLUMN_TARGET_KIND: &str = "column"; pub(crate) const COLUMN_TARGET_SEPARATOR: char = '.'; pub(crate) const MODEL_RESOURCE_TYPE: &str = "model"; diff --git a/crates/sqlbuild-refactor/src/refactor_command/main/mod.rs b/crates/sqlbuild-refactor/src/refactor_command/main/mod.rs index 288df1512f..c1128322f3 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/main/mod.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/main/mod.rs @@ -1,4 +1,5 @@ pub mod layer_move_suggestion; +pub mod move_pairs; pub mod refactor_request_for_target; 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 new file mode 100644 index 0000000000..75c8222ff8 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactor_command/main/move_pairs.rs @@ -0,0 +1,58 @@ +//! 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/render_refactor_json.rs b/crates/sqlbuild-refactor/src/refactor_command/main/render_refactor_json.rs index 0ea303cdbd..313b988b76 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,7 +5,7 @@ use sqlbuild_core::json::main::dumps::dumps; use sqlbuild_core::json::models::{JsonDialect, JsonValue, StdlibJsonOptions}; use crate::refactor_command::_helpers::json::{ - count, file, integer, location, object, operation_value, optional_text, text, + batch_moves, count, file, integer, location, object, operation_value, optional_text, text, }; use crate::refactor_command::_helpers::output_text::refactor_target; use crate::refactor_command::models::{RefactorDiagnostic, RefactorStatus}; @@ -23,9 +23,9 @@ pub fn render_refactor_json( } else { 0 }; - let payload: JsonValue = object(vec![ + let mut entries: Vec<(&str, JsonValue)> = vec![ ("operation", text(operation_value(plan))), - ("target", text(&refactor_target(plan))), + ("target", text(&json_target(plan))), ("new_name", text(&request.new_name)), ("destination", optional_text(request.destination.as_deref())), ("cascade", JsonValue::Bool(request.cascade)), @@ -99,7 +99,20 @@ pub fn render_refactor_json( ), ]), ), - ]); + ]; + if !plan.batch.is_empty() { + entries.insert(4, ("moves", batch_moves(plan))); + } + let payload: JsonValue = object(entries); let dialect: JsonDialect = JsonDialect::Stdlib(StdlibJsonOptions::new(Some(2))); Ok(format!("{}\n", dumps(&payload, &dialect)?)) } + +/// The target of the one refactoring, or of the first move of a batch. +fn json_target(plan: &RefactorPlan) -> String { + if plan.batch.is_empty() { + refactor_target(plan) + } else { + format!("model:{}", plan.request.model_name) + } +} 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 a10de1e462..fa8656e69f 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,8 +3,8 @@ use sqlbuild_core::cli_text::models::CliStyle; use crate::refactor_command::_helpers::output_text::{ - change_lines, diagnostic_lines, heading, layer_move_lines, location_lines, migration_lines, - refactor_target, status_line, verb, + batch_lines, change_lines, diagnostic_lines, heading, layer_move_lines, location_lines, + migration_lines, refactor_target, status_line, verb, }; use crate::refactor_command::models::{LayerMove, RefactorDiagnostic, RefactorStatus}; use crate::refactoring::models::{ManualLocation, RefactorPlan}; @@ -18,6 +18,7 @@ pub fn render_refactor_text( style: CliStyle, ) -> String { let mut lines: Vec = vec![style.title(&heading(plan))]; + lines.extend(batch_lines(style, plan)); let blocked: bool = status == RefactorStatus::Refused; if !blocked || (plan.blocking.is_empty() && plan.manual.is_empty()) { lines.extend(change_lines(style, &plan.changes)); diff --git a/crates/sqlbuild-refactor/src/refactor_command/models.rs b/crates/sqlbuild-refactor/src/refactor_command/models.rs index 7defc74f34..974b2473ac 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/models.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/models.rs @@ -50,3 +50,10 @@ pub struct LayerMove { pub layer_folder: Option, pub command: String, } + +/// The command-line words and optional `(path, contents)` move file of one `sqb mv`. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct MoveArguments<'a> { + 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 e8b2db7cab..8481bcf8f5 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/tests.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/tests.rs @@ -2,5 +2,7 @@ mod helpers; #[path = "tests/test_layer_move.rs"] mod layer_move; +#[path = "tests/test_move_pairs.rs"] +mod move_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 708440bb4a..50d0a41111 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/tests/helpers.rs @@ -5,6 +5,7 @@ use crate::refactoring::models::{FileChange, RefactorOperation, RefactorPlan, Re pub(super) fn rename_plan(old_path: &str, new_name: &str) -> RefactorPlan { let (folder, file) = old_path.rsplit_once('/').unwrap_or(("", old_path)); RefactorPlan { + batch: Vec::new(), request: RefactorRequest { operation: RefactorOperation::RenameModel, model_name: file.trim_end_matches(".sql").to_owned(), 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 new file mode 100644 index 0000000000..e78ea0e72d --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactor_command/tests/test_move_pairs.rs @@ -0,0 +1,64 @@ +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_types.rs b/crates/sqlbuild-refactor/src/refactor_command/tests/test_types.rs index 78fa819124..7a5fd95d7b 100644 --- a/crates/sqlbuild-refactor/src/refactor_command/tests/test_types.rs +++ b/crates/sqlbuild-refactor/src/refactor_command/tests/test_types.rs @@ -16,3 +16,11 @@ pub(super) struct RenderTestCase { pub(super) codes: &'static [&'static str], pub(super) expected_text: &'static str, } + +/// Command-line words and a move file, and the pairs or error message they give. +pub(super) struct MovePairsTestCase { + pub(super) description: &'static str, + pub(super) words: &'static [&'static str], + pub(super) file: Option<&'static str>, + pub(super) expected_pairs: Result<&'static [(&'static str, &'static str)], &'static str>, +} diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/text_edits.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/text_edits.rs index 0411ec1229..8fabcaed74 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/text_edits.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/edits/text_edits.rs @@ -184,6 +184,7 @@ pub(crate) fn build_plan( .map(str::to_owned); RefactorPlan { request, + batch: Vec::new(), changes: file_changes(parts.edits, moves), manual: parts.manual, blocking: parts.blocking, diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs new file mode 100644 index 0000000000..3562c4665b --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs @@ -0,0 +1,153 @@ +//! Several model moves planned together against the state after all of them. + +use std::collections::{BTreeMap, BTreeSet}; +use std::path::Path; + +use crate::refactoring::_helpers::edits::text_edits::RefactorParts; +use crate::refactoring::_helpers::files::project_files::project_sql_files; +use crate::refactoring::_helpers::planning::declaration_moves::{ + ModelMove, declaration_moves, shared_destinations, +}; +use crate::refactoring::_helpers::planning::model_planning::{ + FinalState, ModelTarget, own_model_parts, +}; +use crate::refactoring::_helpers::scanning::scan_context::ScanContext; +use crate::refactoring::errors::RefactorError; +use crate::refactoring::models::{ManualLocation, RefactorFacts}; +use crate::refactoring::types::{DeclarationMoveHost, UnitTestScopeHost}; + +/// Edits, blockers and file moves of every target, with declarations placed once for all. +pub(crate) fn batch_parts( + facts: &RefactorFacts, + targets: &[ModelTarget<'_>], + context: &ScanContext, + (host, test_host): (&DeclarationMoveHost<'_>, &UnitTestScopeHost<'_>), +) -> Result { + let files = project_sql_files(facts); + let destinations: Vec = targets.iter().map(ModelTarget::destination).collect(); + let mut state = FinalState { + vacated: targets + .iter() + .map(|target| target.source_path.clone()) + .collect(), + renamed: targets + .iter() + .filter(|target| target.request.new_name != target.model.name) + .map(|target| target.model.name.clone()) + .collect(), + }; + let moves: Vec> = targets + .iter() + .zip(&destinations) + .map(|(target, destination)| ModelMove { + model: &target.model.name, + source: &target.source_path, + destination, + }) + .collect(); + let declaration = + declaration_moves(Path::new(&facts.project_dir), &moves, &state.vacated, host)?; + state + .vacated + .extend(declaration.moves.iter().map(|(source, _)| source.clone())); + let mut parts = RefactorParts { + blocking: batch_conflicts(targets), + ..RefactorParts::default() + }; + let (mut collisions, mut test_blocking, mut pending) = (Vec::new(), Vec::new(), Vec::new()); + let mut test_moves: Vec<(String, String)> = Vec::new(); + for target in targets { + let own = own_model_parts(facts, (&files, context, test_host), target, &state)?; + parts.edits.extend(own.edits); + parts.manual.extend(own.manual); + collisions.extend(own.collisions); + test_blocking.extend(own.test_blocking); + pending.extend(own.pending); + test_moves.extend(own.test_moves); + parts.notes.extend(own.notes); + } + parts.manual.extend(facts.python_locations.iter().cloned()); + parts.blocking.extend(collisions); + parts.blocking.extend(declaration.blocking); + parts.blocking.extend(test_blocking); + parts.blocking.extend(shared_destinations( + test_moves + .iter() + .map(|(source, destination)| (source.as_str(), destination.as_str())), + |first, second, _| format!("the unit tests {first} and {second} would both move here"), + )); + parts.blocking.extend(pending); + parts.moves = declaration.moves; + parts.moves.extend(test_moves); + parts.moves.extend( + targets + .iter() + .zip(destinations) + .filter(|(target, destination)| *destination != target.source_path) + .map(|(target, destination)| (target.source_path.clone(), destination)), + ); + Ok(parts) +} + +/// Moves that contradict each other: a model moved twice, a shared file or name, a reused name. +fn batch_conflicts(targets: &[ModelTarget<'_>]) -> Vec { + let mut conflicts: Vec = Vec::new(); + let mut moved: BTreeSet<&str> = BTreeSet::new(); + let mut reported: BTreeSet<&str> = BTreeSet::new(); + let mut by_destination: BTreeMap = BTreeMap::new(); + let mut by_name: BTreeMap = BTreeMap::new(); + let given_up: BTreeMap = targets + .iter() + .filter(|target| target.request.new_name != target.model.name) + .map(|target| (target.model.name.to_lowercase(), target.model.name.as_str())) + .collect(); + for target in targets { + let model: &str = &target.model.name; + 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"), + )); + } + continue; + } + let destination: String = target.destination(); + if let Some(other) = by_destination.insert(destination.clone(), model) { + conflicts.push(conflict( + &destination, + format!("model:{other} and model:{model} both move to {destination}"), + )); + continue; + } + let name: &str = &target.request.new_name; + if let Some(other) = given_up + .get(&name.to_lowercase()) + .filter(|other| **other != model) + { + conflicts.push(conflict( + &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" + ), + )); + } + if let Some(other) = by_name.insert(name.to_lowercase(), model) { + conflicts.push(conflict( + &destination, + format!("model:{other} and model:{model} would both be named {name}"), + )); + } + } + conflicts +} + +fn conflict(path: &str, reason: String) -> ManualLocation { + ManualLocation { + path: path.to_owned(), + line: None, + column: None, + reason, + } +} diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/declaration_moves.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/declaration_moves.rs index 776e29a261..7c42772c89 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/declaration_moves.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/declaration_moves.rs @@ -12,20 +12,31 @@ use crate::refactoring::types::DeclarationMoveHost; const UNSETTLED_REASON: &str = "declaration placement at the destination could not be worked out"; -/// The declaration file moves of moving `model` from `source` to `destination`, and blockers. +/// One model move: its name and its paths before and after. +pub(crate) struct ModelMove<'a> { + pub(crate) model: &'a str, + pub(crate) source: &'a str, + pub(crate) destination: &'a str, +} + +/// Declaration file moves and blockers of every model move together; `vacated` paths are free. pub(crate) fn declaration_moves( project_dir: &Path, - model: &str, - paths: (&str, &str), + moves: &[ModelMove<'_>], + vacated: &BTreeSet, host: &DeclarationMoveHost<'_>, ) -> Result { - let (source, destination) = paths; - if parent(destination) == parent(source) { + let changed: Vec<(String, String)> = moves + .iter() + .filter(|item| parent(item.destination) != parent(item.source)) + .map(|item| (item.model.to_owned(), item.destination.to_owned())) + .collect(); + let Some((_, first_destination)) = changed.first() else { return Ok(DeclarationMoves::default()); - } - let placement: DeclarationPlacement = host(model, destination)?; + }; + let placement: DeclarationPlacement = host(&changed)?; let Some(relocated) = &placement.relocated else { - return Ok(unsettled(destination, &placement.diagnostics)); + return Ok(unsettled(first_destination, &placement.diagnostics)); }; let mut destinations: BTreeMap = BTreeMap::new(); let mut blocking: Vec = Vec::new(); @@ -47,6 +58,19 @@ pub(crate) fn declaration_moves( )); continue; } + if record.requires_group { + blocking.push(blocker( + original, + format!( + "{} must move to {}/, which keeps its files in group folders, but {} is \ + not in a group; move it into a group folder of its current role first", + original.label, + parent(&record.path), + original.path + ), + )); + continue; + } let planned: &String = destinations .entry(original.path.clone()) .or_insert_with(|| record.path.clone()); @@ -73,11 +97,19 @@ pub(crate) fn declaration_moves( )); } } + blocking.extend(shared_destinations( + destinations + .iter() + .map(|(source, target)| (source.as_str(), target.as_str())), + |first, second, target| { + format!("{first} and {second} would both move to {target}; rename one of them first") + }, + )); let targets: BTreeSet<&String> = destinations.values().collect(); blocking.extend( targets .into_iter() - .filter(|path| join(project_dir, path).exists()) + .filter(|path| !vacated.contains(*path) && join(project_dir, path).exists()) .map(|path| ManualLocation { path: path.clone(), line: None, @@ -91,6 +123,26 @@ pub(crate) fn declaration_moves( }) } +/// A blocker at each path two `(source, destination)` moves share, worded by `reason`. +pub(crate) fn shared_destinations<'a>( + moves: impl Iterator, + reason: impl Fn(&str, &str, &str) -> String, +) -> Vec { + let mut sources: BTreeMap<&str, &str> = BTreeMap::new(); + let mut shared: Vec = Vec::new(); + for (source, destination) in moves { + if let Some(other) = sources.insert(destination, source) { + shared.push(ManualLocation { + path: destination.to_owned(), + line: None, + column: None, + reason: reason(other, source, destination), + }); + } + } + shared +} + fn unsettled(destination: &str, diagnostics: &[String]) -> DeclarationMoves { let reasons: Vec = if diagnostics.is_empty() { vec![UNSETTLED_REASON.to_owned()] diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs index 2cde89557e..9c3eb68681 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/mod.rs @@ -1,7 +1,9 @@ pub(crate) mod audit_arguments; +pub(crate) mod batch_planning; pub(crate) mod column_planning; pub(crate) mod column_references; pub(crate) mod declaration_moves; pub(crate) mod model_planning; pub(crate) mod model_references; +pub(crate) mod single_planning; pub(crate) mod unit_test_moves; diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs index 26370cf44c..58177858f1 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/model_planning.rs @@ -1,5 +1,6 @@ //! Resolve, block and migrate a model rename or move, as `model_planning.py` does. +use std::collections::BTreeSet; use std::path::Path; use crate::refactoring::_helpers::edits::header_edits::{header_tokens, is_value}; @@ -11,7 +12,7 @@ use crate::refactoring::_helpers::files::paths::{ use crate::refactoring::_helpers::files::project_files::{ ProjectSqlFile, project_sql_files, yaml_files, }; -use crate::refactoring::_helpers::planning::declaration_moves::declaration_moves; +use crate::refactoring::_helpers::planning::declaration_moves::{ModelMove, declaration_moves}; use crate::refactoring::_helpers::planning::model_references::{ macro_reference_locations, model_reference_edits, }; @@ -171,23 +172,77 @@ pub(crate) fn model_parts( (host, test_host): (&DeclarationMoveHost<'_>, &UnitTestScopeHost<'_>), ) -> Result { let files = project_sql_files(facts); - let old = target.model.name.as_str(); - let new = target.request.new_name.as_str(); + let destination = target.destination(); let declaration = declaration_moves( Path::new(&facts.project_dir), - &target.model.name, - (&target.source_path, &target.destination()), + &[ModelMove { + model: &target.model.name, + source: &target.source_path, + destination: &destination, + }], + &BTreeSet::new(), host, )?; + let own = own_model_parts( + facts, + (&files, context, test_host), + target, + &FinalState::default(), + )?; + let mut manual = own.manual; + manual.extend(facts.python_locations.iter().cloned()); + let mut blocking = own.collisions; + blocking.extend(declaration.blocking); + blocking.extend(own.test_blocking); + blocking.extend(own.pending); + let mut moves = declaration.moves; + moves.extend(own.test_moves); + Ok(RefactorParts { + edits: own.edits, + manual, + blocking, + moves, + notes: own.notes, + ..RefactorParts::default() + }) +} + +/// What other moves of the same plan change: paths they leave and models they rename. +#[derive(Default)] +pub(crate) struct FinalState { + pub(crate) vacated: BTreeSet, + pub(crate) renamed: BTreeSet, +} + +/// One model's own edits and blockers, apart from the declaration moves planned for all models. +pub(crate) struct OwnModelParts { + pub(crate) edits: Vec<(String, TextEdit)>, + pub(crate) manual: Vec, + pub(crate) collisions: Vec, + pub(crate) test_blocking: Vec, + pub(crate) pending: Vec, + pub(crate) test_moves: Vec<(String, String)>, + pub(crate) notes: Vec, +} + +/// References, unit tests, collisions and pending migrations of one model rename or move. +pub(crate) fn own_model_parts( + facts: &RefactorFacts, + (files, context, test_host): (&[ProjectSqlFile], &ScanContext, &UnitTestScopeHost<'_>), + target: &ModelTarget<'_>, + state: &FinalState, +) -> Result { + let old = target.model.name.as_str(); + let new = target.request.new_name.as_str(); let mut edits: Vec<(String, TextEdit)> = Vec::new(); if new != old { - edits.extend(model_reference_edits(&files, (old, new), context)?); + edits.extend(model_reference_edits(files, (old, new), context)?); edits.extend(yaml_model_edits(&yaml_files(facts), old, new)?); } let destination = target.destination(); let tests = unit_test_parts( Path::new(&facts.project_dir), - (&facts.unit_tests, &files), + (&facts.unit_tests, files), &FollowedModel { names: (old, new), paths: (&target.source_path, &destination), @@ -195,31 +250,30 @@ pub(crate) fn model_parts( test_host, )?; edits.extend(tests.edits); - let mut manual = macro_reference_locations(&facts.models, &files, old, context); - manual.extend(facts.python_locations.iter().cloned()); - let mut blocking = collisions(facts, target); - blocking.extend(declaration.blocking); - blocking.extend(tests.blocking); - blocking.extend(pending_migration(&files, target)?); - let mut moves = declaration.moves; - moves.extend(tests.moves); - Ok(RefactorParts { + Ok(OwnModelParts { edits, - manual, - blocking, - moves, + manual: macro_reference_locations(&facts.models, files, old, context), + collisions: collisions(facts, target, state), + test_blocking: tests.blocking, + pending: pending_migration(files, target)?, + test_moves: tests.moves, notes: tests.notes, - ..RefactorParts::default() }) } -fn collisions(facts: &RefactorFacts, target: &ModelTarget<'_>) -> Vec { +fn collisions( + facts: &RefactorFacts, + target: &ModelTarget<'_>, + state: &FinalState, +) -> Vec { let new = &target.request.new_name; let mut found: Vec = facts .models .iter() .filter(|other| { - other.name.to_lowercase() == new.to_lowercase() && other.name != target.model.name + other.name.to_lowercase() == new.to_lowercase() + && other.name != target.model.name + && !state.renamed.contains(&other.name) }) .map(|other| ManualLocation { path: other.path.clone(), @@ -230,6 +284,7 @@ fn collisions(facts: &RefactorFacts, target: &ModelTarget<'_>) -> Vec, + test_host: &UnitTestScopeHost<'_>, +) -> Result { + let context = ScanContext::for_facts(facts); + if request.operation == RefactorOperation::RenameColumn { + let column = column_rename_context(facts, request, &context)?; + let parts = column_rename_parts(&column)?; + let renamed: Vec<(String, String, String)> = parts + .cascaded + .iter() + .map(|model| (model.clone(), column.old.clone(), column.new.clone())) + .collect(); + return Ok(build_plan( + request.clone(), + parts, + &[], + (renamed, Some(COLUMN_MANUAL_HELP)), + )); + } + let target = model_target(facts, request)?; + let 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)); + } + Ok(build_plan( + target.request, + parts, + &moves, + (Vec::new(), Some(MODEL_MANUAL_HELP)), + )) +} diff --git a/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs b/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs index b82d806464..85112fd5a3 100644 --- a/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs +++ b/crates/sqlbuild-refactor/src/refactoring/main/plan_refactor.rs @@ -1,51 +1,44 @@ -//! Plan a model rename, model move, or column rename from compiler facts. +//! Plan a model rename, column rename, or one or more model moves from compiler facts. 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::batch_planning::batch_parts; +use crate::refactoring::_helpers::planning::model_planning::{ModelTarget, model_target}; +use crate::refactoring::_helpers::planning::single_planning::plan_single; use crate::refactoring::_helpers::scanning::scan_context::ScanContext; -use crate::refactoring::constants::{COLUMN_MANUAL_HELP, MODEL_MANUAL_HELP}; +use crate::refactoring::constants::MODEL_MANUAL_HELP; use crate::refactoring::errors::RefactorError; -use crate::refactoring::models::{RefactorFacts, RefactorOperation, RefactorPlan, RefactorRequest}; - +use crate::refactoring::models::{RefactorFacts, RefactorPlan, RefactorRequest}; use crate::refactoring::types::{DeclarationMoveHost, UnitTestScopeHost}; -/// Plan one refactoring without touching files, asking the hosts about declaration scopes. +/// Plan `requests` together against the state after all of them, without touching files. pub fn plan_refactor( facts: &RefactorFacts, - request: &RefactorRequest, + requests: &[RefactorRequest], host: &DeclarationMoveHost<'_>, test_host: &UnitTestScopeHost<'_>, ) -> Result { - let context = ScanContext::for_facts(facts); - if request.operation == RefactorOperation::RenameColumn { - let column = column_rename_context(facts, request, &context)?; - let parts = column_rename_parts(&column)?; - let renamed: Vec<(String, String, String)> = parts - .cascaded - .iter() - .map(|model| (model.clone(), column.old.clone(), column.new.clone())) - .collect(); - return Ok(build_plan( - request.clone(), - parts, - &[], - (renamed, Some(COLUMN_MANUAL_HELP)), + let [first, rest @ ..] = requests else { + return Err(RefactorError::internal( + "a refactoring needs at least one request", )); + }; + if rest.is_empty() { + return plan_single(facts, first, host, test_host); } - let target = model_target(facts, request)?; - let 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)); - } - Ok(build_plan( - target.request, + let context = ScanContext::for_facts(facts); + let targets: Vec> = requests + .iter() + .map(|request| model_target(facts, request)) + .collect::>()?; + let parts = batch_parts(facts, &targets, &context, (host, test_host))?; + let moves: Vec<(String, String)> = parts.moves.clone(); + let batch: Vec = targets.into_iter().map(|target| target.request).collect(); + let mut plan = build_plan( + batch[0].clone(), parts, &moves, (Vec::new(), Some(MODEL_MANUAL_HELP)), - )) + ); + plan.batch = batch; + Ok(plan) } diff --git a/crates/sqlbuild-refactor/src/refactoring/main/with_model_migration.rs b/crates/sqlbuild-refactor/src/refactoring/main/with_model_migration.rs index 926fec7469..095a70d7ae 100644 --- a/crates/sqlbuild-refactor/src/refactoring/main/with_model_migration.rs +++ b/crates/sqlbuild-refactor/src/refactoring/main/with_model_migration.rs @@ -4,21 +4,36 @@ use crate::refactoring::_helpers::edits::header_edits::insert_header_entry_edit; use crate::refactoring::_helpers::planning::model_planning::decide_model_migration; use crate::refactoring::_helpers::scanning::chars::chars; use crate::refactoring::constants::MIGRATE_FROM_KEY; -use crate::refactoring::models::{ManualLocation, MigrationDeclaration, ModelFacts, RefactorPlan}; +use crate::refactoring::models::{ + ManualLocation, MigrationDeclaration, ModelFacts, RefactorPlan, RefactorRequest, +}; use crate::refactoring::types::Originals; -/// The plan with `migrate_from ` on the model whenever its relation moves. +/// The plan with `migrate_from ` on each moved model whenever its relation moves. pub fn with_model_migration( plan: &RefactorPlan, before: &[ModelFacts], after: Option<&[ModelFacts]>, originals: &Originals, ) -> RefactorPlan { - let old = plan.request.model_name.as_str(); - let new = plan.request.new_name.as_str(); + plan.requests() + .iter() + .fold(plan.clone(), |migrated, request| { + migrate(migrated, request, (before, after), originals) + }) +} + +/// `migrated` with one request's migration, or the reason it cannot keep history. +fn migrate( + mut migrated: RefactorPlan, + request: &RefactorRequest, + (before, after): (&[ModelFacts], Option<&[ModelFacts]>), + originals: &Originals, +) -> RefactorPlan { + let old = request.model_name.as_str(); + let new = request.new_name.as_str(); let decision = decide_model_migration(before, after, (old, new)); - let destination = plan.request.destination.clone(); - let mut migrated = plan.clone(); + let destination = request.destination.clone(); if decision.blocked { migrated.blocking.push(ManualLocation { path: destination.unwrap_or_default(), @@ -28,7 +43,7 @@ pub fn with_model_migration( }); return migrated; } - let Some(index) = plan + let Some(index) = migrated .changes .iter() .position(|change| Some(&change.path) == destination.as_ref()) @@ -39,19 +54,22 @@ pub fn with_model_migration( return migrated; } let declaration = format!("{MIGRATE_FROM_KEY} {old}"); - let change = &plan.changes[index]; + let change = &migrated.changes[index]; let contents = originals .iter() .find(|(path, _)| *path == change.original_path) .map(|(_, text)| text.as_str()) .unwrap_or_default(); match insert_header_entry_edit(contents, &chars(contents), &declaration, &declaration) { - None => migrated.blocking.push(ManualLocation { - path: change.path.clone(), - line: Some(1), - column: Some(1), - reason: format!("add {declaration} to the MODEL header to keep history"), - }), + None => { + let path: String = change.path.clone(); + migrated.blocking.push(ManualLocation { + path, + line: Some(1), + column: Some(1), + reason: format!("add {declaration} to the MODEL header to keep history"), + }); + } Some(edit) => { migrated.changes[index].edits.push(edit); migrated.migrations.push(MigrationDeclaration { diff --git a/crates/sqlbuild-refactor/src/refactoring/models.rs b/crates/sqlbuild-refactor/src/refactoring/models.rs index 08ff608936..04d4d523d7 100644 --- a/crates/sqlbuild-refactor/src/refactoring/models.rs +++ b/crates/sqlbuild-refactor/src/refactoring/models.rs @@ -125,6 +125,9 @@ pub struct MigrationDeclaration { #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] pub struct RefactorPlan { pub request: RefactorRequest, + /// Every resolved move of a batch, in order, the first being `request`; empty otherwise. + #[serde(default)] + pub batch: Vec, pub changes: Vec, pub manual: Vec, pub blocking: Vec, @@ -137,6 +140,15 @@ pub struct RefactorPlan { } impl RefactorPlan { + /// Every request the plan carries out: the batch, or the one request. + pub fn requests(&self) -> &[RefactorRequest] { + if self.batch.is_empty() { + std::slice::from_ref(&self.request) + } else { + &self.batch + } + } + /// The number of text edits the plan writes, over every changed file. pub fn edit_count(&self) -> usize { self.changes.iter().map(|change| change.edits.len()).sum() @@ -270,6 +282,9 @@ pub struct PlacedDeclaration { pub struct RelocatedDeclaration { pub key: String, pub path: String, + /// The destination role keeps its files in groups, but this file is not in one. + #[serde(default)] + pub requires_group: bool, } /// The scope placement answer for a model move across folders. diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs index 5b8a72e3dd..ef108d5753 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs @@ -11,6 +11,7 @@ use crate::refactoring::_helpers::edits::header_edits::{ }; use crate::refactoring::_helpers::edits::text_edits::{apply_text_edits, file_changes, text_edit}; use crate::refactoring::_helpers::edits::yaml_edits::{yaml_column_edits, yaml_model_edits}; +use crate::refactoring::_helpers::files::paths::stem; use crate::refactoring::_helpers::files::workspace::commit_changes; use crate::refactoring::_helpers::planning::column_planning::{ column_rename_context, header_parts, @@ -18,7 +19,7 @@ use crate::refactoring::_helpers::planning::column_planning::{ use crate::refactoring::_helpers::planning::column_references::{ BodyContext, BodyMapping, ColumnQuery, ResourceColumns, analyze_column, consumer_edits, }; -use crate::refactoring::_helpers::planning::declaration_moves::declaration_moves; +use crate::refactoring::_helpers::planning::declaration_moves::{ModelMove, declaration_moves}; use crate::refactoring::_helpers::planning::model_planning::{model_parts, model_target}; use crate::refactoring::_helpers::scanning::chars::chars; use crate::refactoring::_helpers::scanning::scan_context::ScanContext; @@ -322,7 +323,7 @@ pub(super) fn blocking_reasons( let context = ScanContext { backtick_identifiers: false, }; - let host = |_: &str, _: &str| { + let host = |_: &[(String, String)]| { Ok(DeclarationPlacement { relocated: Some(Vec::new()), ..DeclarationPlacement::default() @@ -428,17 +429,26 @@ pub(super) fn relocated(label: &str, path: &str) -> RelocatedDeclaration { RelocatedDeclaration { key: label.to_owned(), path: path.to_owned(), + requires_group: false, } } -/// The moves and `path:line reason` blockers of moving model `orders` with a fixed placement. +/// The moves and `path:line reason` blockers of moving models together with a fixed placement. pub(super) fn planned_declaration_moves( root: &Path, - paths: (&str, &str), + paths: &[(&str, &str)], placement: &DeclarationPlacement, ) -> Result { - let host = |_: &str, _: &str| Ok(placement.clone()); - let moves: DeclarationMoves = declaration_moves(root, "orders", paths, &host)?; + let host = |_: &[(String, String)]| Ok(placement.clone()); + let models: Vec> = paths + .iter() + .map(|(source, destination)| ModelMove { + model: stem(source), + source, + destination, + }) + .collect(); + let moves: DeclarationMoves = declaration_moves(root, &models, &BTreeSet::new(), &host)?; let blocking: Vec = moves .blocking .iter() @@ -674,7 +684,7 @@ pub(super) fn followed_unit_tests( target_model_names: targets.iter().map(|target| (*target).to_owned()).collect(), }); } - let host = |_: &str, _: &str| { + let host = |_: &[(String, String)]| { Ok(DeclarationPlacement { relocated: Some(Vec::new()), ..DeclarationPlacement::default() @@ -687,7 +697,7 @@ pub(super) fn followed_unit_tests( }; let plan = crate::refactoring::main::plan_refactor::plan_refactor( &project_facts, - request, + std::slice::from_ref(request), &host, &test_host, )?; @@ -729,3 +739,42 @@ pub(super) fn followed_test_changes(changes: &[(&str, &[&str])]) -> Vec<(String, expected.sort(); expected } + +/// A move of `model` to `destination`. +pub(super) fn move_request(model: &str, destination: &str) -> RefactorRequest { + RefactorRequest { + operation: RefactorOperation::MoveModel, + model_name: model.to_owned(), + new_name: String::new(), + column_name: None, + destination: Some(destination.to_owned()), + cascade: false, + } +} + +/// The `path: reason` blockers and `original -> path` file moves of planning `requests` together. +pub(super) fn planned_batch( + facts: &RefactorFacts, + requests: &[RefactorRequest], +) -> Result<(Vec, Vec), RefactorError> { + let host = |_: &[(String, String)]| { + Ok(DeclarationPlacement { + relocated: Some(Vec::new()), + ..DeclarationPlacement::default() + }) + }; + let test_host = |_: &str, _: &str| Ok(Some(Vec::new())); + let plan = + crate::refactoring::main::plan_refactor::plan_refactor(facts, requests, &host, &test_host)?; + Ok(( + plan.blocking + .iter() + .map(|item| format!("{}: {}", item.path, item.reason)) + .collect(), + plan.changes + .iter() + .filter(|change| change.moved()) + .map(|change| format!("{} -> {}", change.original_path, change.path)) + .collect(), + )) +} diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs index d2f54be8ab..242a9ea7d7 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs @@ -3,12 +3,13 @@ use crate::refactoring::errors::{RefactorError, RefactorErrorKind}; use crate::refactoring::models::{DeclarationPlacement, RefactorOperation}; use crate::refactoring::tests::helpers::{ blocking_reasons, change, commit_failure, facts, files_on_disk, followed_test_changes, - followed_unit_tests, model, model_in_database, owned_pairs, placed, planned_declaration_moves, - refused_target, relocated, replace_all, request, write_files, + followed_unit_tests, model, model_in_database, move_request, owned_pairs, placed, + planned_batch, planned_declaration_moves, refused_target, relocated, replace_all, request, + write_files, }; use crate::refactoring::tests::test_types::{ - CollisionTestCase, CommitTestCase, DeclarationMovesTestCase, FollowedUnitTestTestCase, - MigrationDecisionTestCase, RefusedTargetTestCase, + BatchPlanTestCase, CollisionTestCase, CommitTestCase, DeclarationMovesTestCase, + FollowedUnitTestTestCase, MigrationDecisionTestCase, RefusedTargetTestCase, }; #[test] @@ -235,7 +236,7 @@ fn given_scope_placement_when_moving_across_folders_then_declaration_files_follo DeclarationMovesTestCase { description: "a move within one folder asks no placement", on_disk: &[], - paths: ("models/orders.sql", "models/orders_v2.sql"), + moves: &[("models/orders.sql", "models/orders_v2.sql")], placement: DeclarationPlacement::default(), expected_moves: &[], expected_blocking: &[], @@ -243,7 +244,7 @@ fn given_scope_placement_when_moving_across_folders_then_declaration_files_follo DeclarationMovesTestCase { description: "an unsettled placement blocks with the preview's reasons", on_disk: &[], - paths: ("models/orders.sql", "models/marts/orders.sql"), + moves: &[("models/orders.sql", "models/marts/orders.sql")], placement: DeclarationPlacement { relocated: None, diagnostics: vec!["Move source model:orders is unknown".to_owned()], @@ -255,7 +256,7 @@ fn given_scope_placement_when_moving_across_folders_then_declaration_files_follo DeclarationMovesTestCase { description: "an unsettled placement without reasons blocks with the default reason", on_disk: &[], - paths: ("models/orders.sql", "models/marts/orders.sql"), + moves: &[("models/orders.sql", "models/marts/orders.sql")], placement: DeclarationPlacement::default(), expected_moves: &[], expected_blocking: &[ @@ -269,7 +270,7 @@ fn given_scope_placement_when_moving_across_folders_then_declaration_files_follo ("models/_sqlbuild/_enums/both.sql", "x"), ("models/marts/_sqlbuild/_enums/both.sql", "x"), ], - paths: ("models/orders.sql", "models/marts/orders.sql"), + moves: &[("models/orders.sql", "models/marts/orders.sql")], placement: DeclarationPlacement { relocated: Some(vec![ relocated("enum:a", "models/marts/_sqlbuild/_enums/both.sql"), @@ -303,13 +304,83 @@ fn given_scope_placement_when_moving_across_folders_then_declaration_files_follo "models/marts/_sqlbuild/_enums/both.sql:0 a declaration must move here, but the file already exists", ], }, + DeclarationMovesTestCase { + description: "two grouped files flattened into one direct file block", + on_disk: &[ + ("models/_sqlbuild/_macros/money/helpers.sql", "x"), + ("models/_sqlbuild/_macros/text/helpers.sql", "x"), + ], + moves: &[("models/orders.sql", "models/marts/orders.sql")], + placement: DeclarationPlacement { + relocated: Some(vec![ + relocated("macro:cents", "macros/helpers.sql"), + relocated("macro:label", "macros/helpers.sql"), + ]), + diagnostics: Vec::new(), + declarations: vec![ + placed("macro:cents", "models/_sqlbuild/_macros/money/helpers.sql"), + placed("macro:label", "models/_sqlbuild/_macros/text/helpers.sql"), + ], + }, + expected_moves: &[ + ( + "models/_sqlbuild/_macros/money/helpers.sql", + "macros/helpers.sql", + ), + ( + "models/_sqlbuild/_macros/text/helpers.sql", + "macros/helpers.sql", + ), + ], + expected_blocking: &[ + "macros/helpers.sql:0 models/_sqlbuild/_macros/money/helpers.sql and \ + models/_sqlbuild/_macros/text/helpers.sql would both move to macros/helpers.sql; \ + rename one of them first", + ], + }, + DeclarationMovesTestCase { + description: "two moved models' same-named local files sharing one target block", + on_disk: &[ + ("models/sales/_sqlbuild/_macros/helpers.sql", "x"), + ("models/crm/_sqlbuild/_macros/helpers.sql", "x"), + ], + moves: &[ + ("models/sales/orders.sql", "models/marts/orders.sql"), + ("models/crm/customers.sql", "models/marts/customers.sql"), + ], + placement: DeclarationPlacement { + relocated: Some(vec![ + relocated("macro:cents", "models/marts/_sqlbuild/_macros/helpers.sql"), + relocated("macro:label", "models/marts/_sqlbuild/_macros/helpers.sql"), + ]), + diagnostics: Vec::new(), + declarations: vec![ + placed("macro:cents", "models/sales/_sqlbuild/_macros/helpers.sql"), + placed("macro:label", "models/crm/_sqlbuild/_macros/helpers.sql"), + ], + }, + expected_moves: &[ + ( + "models/crm/_sqlbuild/_macros/helpers.sql", + "models/marts/_sqlbuild/_macros/helpers.sql", + ), + ( + "models/sales/_sqlbuild/_macros/helpers.sql", + "models/marts/_sqlbuild/_macros/helpers.sql", + ), + ], + expected_blocking: &["models/marts/_sqlbuild/_macros/helpers.sql:0 \ + models/crm/_sqlbuild/_macros/helpers.sql and \ + models/sales/_sqlbuild/_macros/helpers.sql would both move to \ + models/marts/_sqlbuild/_macros/helpers.sql; rename one of them first"], + }, ]; for test_case in test_cases { let project = tempfile::tempdir().map_err(|error| RefactorError::value(error.to_string()))?; write_files(project.path(), test_case.on_disk)?; let (moves, blocking) = - planned_declaration_moves(project.path(), test_case.paths, &test_case.placement)?; + planned_declaration_moves(project.path(), test_case.moves, &test_case.placement)?; assert_eq!( moves, owned_pairs(test_case.expected_moves), @@ -415,3 +486,103 @@ fn given_unit_tests_of_model_when_renaming_or_moving_then_tests_follow() -> Resu } Ok(()) } + +#[test] +fn given_several_moves_when_planning_together_then_chains_pass_and_conflicts_block() +-> Result<(), RefactorError> { + let project = tempfile::tempdir().map_err(|error| RefactorError::value(error.to_string()))?; + write_files( + project.path(), + &[ + ("models/orders.sql", "SELECT 1"), + ("models/customers.sql", "SELECT 1"), + ], + )?; + let project_facts = facts( + &project.path().to_string_lossy(), + vec![ + model("orders", Some("view"), "analytics"), + model("customers", Some("view"), "analytics"), + ], + ); + let test_cases = [ + BatchPlanTestCase { + description: "a model takes the name another move gives up", + requests: vec![ + move_request("orders", "models/customers.sql"), + move_request("customers", "models/clients.sql"), + ], + expected_blocking: &["models/customers.sql: model:orders would take the name \ + model:customers gives up in this batch; the old name must \ + stay free for its history, so move them in separate runs"], + expected_moves: &[ + "models/customers.sql -> models/clients.sql", + "models/orders.sql -> models/customers.sql", + ], + }, + BatchPlanTestCase { + description: "two models moved to the same file", + requests: vec![ + move_request("orders", "models/finance/totals.sql"), + move_request("customers", "models/finance/totals.sql"), + ], + expected_blocking: &[ + "models/finance/totals.sql: model:orders and model:customers both move to \ + models/finance/totals.sql", + ], + expected_moves: &[ + "models/orders.sql -> models/finance/totals.sql", + "models/customers.sql -> models/finance/totals.sql", + ], + }, + BatchPlanTestCase { + description: "one model moved twice", + requests: vec![ + move_request("orders", "models/finance/"), + move_request("orders", "models/sales/"), + ], + expected_blocking: &[ + "models/orders.sql: model:orders is moved more than once in \ + this batch", + ], + expected_moves: &["models/orders.sql -> models/sales/orders.sql"], + }, + BatchPlanTestCase { + description: "two models given one name in different folders", + requests: vec![ + move_request("orders", "models/finance/totals.sql"), + move_request("customers", "models/sales/totals.sql"), + ], + expected_blocking: &[ + "models/sales/totals.sql: model:orders and model:customers would both be named \ + totals", + ], + expected_moves: &[ + "models/orders.sql -> models/finance/totals.sql", + "models/customers.sql -> models/sales/totals.sql", + ], + }, + ]; + for test_case in test_cases { + let (blocking, moves) = planned_batch(&project_facts, &test_case.requests)?; + + assert_eq!( + (blocking, moves), + ( + test_case + .expected_blocking + .iter() + .map(|item| (*item).to_owned()) + .collect::>(), + test_case + .expected_moves + .iter() + .map(|item| (*item).to_owned()) + .collect::>() + ), + "{}", + test_case.description + ); + } + Ok(()) +} diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs index f22c0c54f1..fd757d21e4 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs @@ -160,8 +160,8 @@ pub(super) struct DeclarationMovesTestCase { pub(super) description: &'static str, /// Files on disk below the project. pub(super) on_disk: &'static [(&'static str, &'static str)], - /// `(source, destination)` of the moved model. - pub(super) paths: (&'static str, &'static str), + /// `(source, destination)` of each moved model. + pub(super) moves: &'static [(&'static str, &'static str)], pub(super) placement: DeclarationPlacement, pub(super) expected_moves: &'static [(&'static str, &'static str)], /// `path:line reason` of each blocker. @@ -228,3 +228,11 @@ pub(super) struct FollowedUnitTestTestCase { /// Each `path: reason` note of a test file kept in its folder. pub(super) expected_notes: &'static [&'static str], } + +/// Several moves planned together, and their blockers and file moves. +pub(super) struct BatchPlanTestCase { + pub(super) description: &'static str, + pub(super) requests: Vec, + pub(super) expected_blocking: &'static [&'static str], + pub(super) expected_moves: &'static [&'static str], +} diff --git a/crates/sqlbuild-refactor/src/refactoring/types.rs b/crates/sqlbuild-refactor/src/refactoring/types.rs index 37d5259965..33b02dd129 100644 --- a/crates/sqlbuild-refactor/src/refactoring/types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/types.rs @@ -3,9 +3,9 @@ use crate::refactoring::errors::RefactorError; use crate::refactoring::models::DeclarationPlacement; -/// The shared scope placement for moving a model to a destination: `(model, destination)`. +/// The shared scope placement once every `(model, destination)` move is applied together. pub type DeclarationMoveHost<'a> = - dyn Fn(&str, &str) -> Result + 'a; + dyn Fn(&[(String, String)]) -> Result + 'a; /// Used scoped declarations a `(test path, destination)` move would rebind; `None` if unknown. pub type UnitTestScopeHost<'a> = diff --git a/crates/sqlbuild-scopes/src/placement/_helpers/mod.rs b/crates/sqlbuild-scopes/src/placement/_helpers/mod.rs index 57f1827422..103593545c 100644 --- a/crates/sqlbuild-scopes/src/placement/_helpers/mod.rs +++ b/crates/sqlbuild-scopes/src/placement/_helpers/mod.rs @@ -1,5 +1,7 @@ pub(crate) mod anchors; pub(crate) mod declaration_directory; pub(crate) mod posix_paths; +pub(crate) mod relocation; pub(crate) mod required; +pub(crate) mod role_layout; pub(crate) mod roles; diff --git a/crates/sqlbuild-scopes/src/placement/main/relocate_for_resource_move.rs b/crates/sqlbuild-scopes/src/placement/_helpers/relocation.rs similarity index 56% rename from crates/sqlbuild-scopes/src/placement/main/relocate_for_resource_move.rs rename to crates/sqlbuild-scopes/src/placement/_helpers/relocation.rs index 1327dc38a0..9cd18864ea 100644 --- a/crates/sqlbuild-scopes/src/placement/main/relocate_for_resource_move.rs +++ b/crates/sqlbuild-scopes/src/placement/_helpers/relocation.rs @@ -1,4 +1,4 @@ -//! The declarations whose placement follows a resource to a new path. +//! Phases of relocating declarations after resource moves. use std::collections::{HashMap, HashSet}; @@ -7,61 +7,58 @@ use crate::placement::_helpers::anchors::{ }; use crate::placement::_helpers::declaration_directory::declaration_directory; use crate::placement::_helpers::required::required_placement; -use crate::placement::main::placement_diagnostics::placement_diagnostics; +use crate::placement::_helpers::role_layout::carried_path; use crate::placement::models::{PlacementIndex, Relocation, RequiredPlacement}; use crate::scope_index::models::{ DeclarationIdentity, DeclarationRecord, OwnershipRoot, OwnershipRootKind, ResourceIdentity, ScopeKind, }; -use sqlbuild_core::posix_path::main::posix_name::posix_name; -/// Python's `relocate_for_resource_move`: each moved declaration, or `None` when unsettled. -#[must_use] -pub fn relocate_for_resource_move( +/// The index with every resource in `moves` at its destination. +pub(crate) fn moved_resources( index: &PlacementIndex, - resource: &ResourceIdentity, - destination: &str, -) -> Option> { - let misplaced: HashSet<&DeclarationIdentity> = placement_diagnostics(index) - .into_iter() - .map(|diagnostic| &index.declarations[diagnostic.declaration].identity) - .collect(); + moves: &[(ResourceIdentity, String)], +) -> PlacementIndex { let mut current: PlacementIndex = index.clone(); for record in &mut current.resources { - if &record.identity == resource { - record.path = destination.to_owned(); + if let Some((_, destination)) = moves.iter().find(|(moved, _)| *moved == record.identity) { + record.path.clone_from(destination); } } - for _ in 0..=index.declarations.len() { - let usages: UsagesByDeclaration<'_> = usages_by_declaration(¤t); - let anchors: AnchorSets = anchor_sets(¤t, &usages); - let mut relocated: HashMap = HashMap::new(); - for record in ¤t.declarations { - if record.scope == ScopeKind::Private || misplaced.contains(&record.identity) { - continue; - } - if let Some(moved) = relocated_record(record, &usages, &anchors) { - relocated.insert(record.identity.clone(), moved); - } - } - if relocated.is_empty() { - return Some(changed_records(index, ¤t)); + current +} + +/// The declarations one round moves, and whether each lacks a group its new role needs. +pub(crate) fn relocation_round( + current: &PlacementIndex, + misplaced: &HashSet<&DeclarationIdentity>, +) -> ( + HashMap, + Vec<(DeclarationIdentity, bool)>, +) { + let usages: UsagesByDeclaration<'_> = usages_by_declaration(current); + let anchors: AnchorSets = anchor_sets(current, &usages); + let mut relocated: HashMap = HashMap::new(); + let mut grouping: Vec<(DeclarationIdentity, bool)> = Vec::new(); + for record in ¤t.declarations { + if record.scope == ScopeKind::Private || misplaced.contains(&record.identity) { + continue; } - for record in &mut current.declarations { - if let Some(moved) = relocated.get(&record.identity) { - *record = moved.clone(); - } + if let Some((moved, needed)) = relocated_record(current, record, &usages, &anchors) { + grouping.push((record.identity.clone(), needed)); + relocated.insert(record.identity.clone(), moved); } } - None + (relocated, grouping) } -/// `_relocated_record`: the record at its required placement, when that differs. +/// `_relocated_record`: the record at its required placement, and whether it lacks a needed group. fn relocated_record( + current: &PlacementIndex, record: &DeclarationRecord, usages: &UsagesByDeclaration<'_>, anchors: &AnchorSets, -) -> Option { +) -> Option<(DeclarationRecord, bool)> { let RequiredPlacement { scope, path, .. } = required_placement(record, usages, anchors)?; if record.scope == scope && record.owning_path == path { return None; @@ -85,8 +82,9 @@ fn relocated_record( resource_kind: None, }, }; - Some(DeclarationRecord { - path: format!("{directory}/{}", posix_name(&record.path)), + let (carried, requires_group): (String, bool) = carried_path(current, record, &directory); + let moved = DeclarationRecord { + path: carried, scope, ownership_root, owning_path: if scope == ScopeKind::Global { @@ -95,11 +93,16 @@ fn relocated_record( path }, ..record.clone() - }) + }; + Some((moved, requires_group)) } /// Each declaration whose path differs from the last original record of its identity. -fn changed_records(original: &PlacementIndex, current: &PlacementIndex) -> Vec { +pub(crate) fn changed_records( + original: &PlacementIndex, + current: &PlacementIndex, + requires_group: &HashSet, +) -> Vec { let paths: HashMap<&DeclarationIdentity, &str> = original .declarations .iter() @@ -116,6 +119,7 @@ fn changed_records(original: &PlacementIndex, current: &PlacementIndex) -> Vec (String, bool) { + let name: &str = posix_name(&record.path); + let source: String = declaration_directory( + record.identity.kind, + record.scope, + record.owning_path.as_deref(), + ); + let relative: &str = record + .path + .strip_prefix(&format!("{source}/")) + .unwrap_or(name); + let grouped: bool = relative != name; + match role_layout(index, directory, &record.path) { + RoleLayout::Direct => (format!("{directory}/{name}"), false), + RoleLayout::Grouped if !grouped => (format!("{directory}/{name}"), true), + RoleLayout::Empty | RoleLayout::Grouped | RoleLayout::Mixed => { + (format!("{directory}/{relative}"), false) + } + } +} + +/// The layout of the shared declaration files in `directory`, other than the file at `moving`. +fn role_layout(index: &PlacementIndex, directory: &str, moving: &str) -> RoleLayout { + let prefix: String = format!("{directory}/"); + let mut direct: bool = false; + let mut grouped: bool = false; + for record in &index.declarations { + if record.scope == ScopeKind::Private || record.path == moving { + continue; + } + if let Some(relative) = record.path.strip_prefix(&prefix) { + if relative.contains('/') { + grouped = true; + } else { + direct = true; + } + } + } + match (direct, grouped) { + (false, false) => RoleLayout::Empty, + (true, false) => RoleLayout::Direct, + (false, true) => RoleLayout::Grouped, + (true, true) => RoleLayout::Mixed, + } +} diff --git a/crates/sqlbuild-scopes/src/placement/main/mod.rs b/crates/sqlbuild-scopes/src/placement/main/mod.rs index c605776667..f5288ef9e1 100644 --- a/crates/sqlbuild-scopes/src/placement/main/mod.rs +++ b/crates/sqlbuild-scopes/src/placement/main/mod.rs @@ -1,4 +1,4 @@ pub mod diagnostic_order; pub mod placement_diagnostics; -pub mod relocate_for_resource_move; +pub mod relocate_for_resource_moves; pub mod required_declaration_placements; diff --git a/crates/sqlbuild-scopes/src/placement/main/relocate_for_resource_moves.rs b/crates/sqlbuild-scopes/src/placement/main/relocate_for_resource_moves.rs new file mode 100644 index 0000000000..e39c2e16fa --- /dev/null +++ b/crates/sqlbuild-scopes/src/placement/main/relocate_for_resource_moves.rs @@ -0,0 +1,41 @@ +//! The declarations whose placement follows several resources to new paths. + +use std::collections::HashSet; + +use crate::placement::_helpers::relocation::{changed_records, moved_resources, relocation_round}; +use crate::placement::main::placement_diagnostics::placement_diagnostics; +use crate::placement::models::{PlacementIndex, Relocation}; +use crate::scope_index::models::{DeclarationIdentity, ResourceIdentity}; + +/// Each declaration moved once every resource in `moves` is at its destination, or `None`. +#[must_use] +pub fn relocate_for_resource_moves( + index: &PlacementIndex, + moves: &[(ResourceIdentity, String)], +) -> Option> { + let misplaced: HashSet<&DeclarationIdentity> = placement_diagnostics(index) + .into_iter() + .map(|diagnostic| &index.declarations[diagnostic.declaration].identity) + .collect(); + let mut current: PlacementIndex = moved_resources(index, moves); + let mut requires_group: HashSet = HashSet::new(); + for _ in 0..=index.declarations.len() { + let (relocated, grouping) = relocation_round(¤t, &misplaced); + for (identity, needed) in grouping { + if needed { + requires_group.insert(identity); + } else { + requires_group.remove(&identity); + } + } + if relocated.is_empty() { + return Some(changed_records(index, ¤t, &requires_group)); + } + for record in &mut current.declarations { + if let Some(moved) = relocated.get(&record.identity) { + *record = moved.clone(); + } + } + } + None +} diff --git a/crates/sqlbuild-scopes/src/placement/models.rs b/crates/sqlbuild-scopes/src/placement/models.rs index bab319791d..a3033e9fb9 100644 --- a/crates/sqlbuild-scopes/src/placement/models.rs +++ b/crates/sqlbuild-scopes/src/placement/models.rs @@ -81,6 +81,8 @@ pub struct Relocation { pub scope: ScopeKind, pub ownership_root: OwnershipRoot, pub owning_path: Option, + /// The destination role keeps its files in groups, but this file has none to keep. + pub requires_group: bool, } /// The fields Python orders scope diagnostics by. diff --git a/crates/sqlbuild-scopes/src/placement/tests/helpers.rs b/crates/sqlbuild-scopes/src/placement/tests/helpers.rs index a32c9a15ab..ef127830f5 100644 --- a/crates/sqlbuild-scopes/src/placement/tests/helpers.rs +++ b/crates/sqlbuild-scopes/src/placement/tests/helpers.rs @@ -99,6 +99,66 @@ pub(super) fn chained_macros() -> PlacementIndex { } } +const SALES_GROUPED: (ScopeKind, Option<&str>) = (ScopeKind::Local, Some("models/sales")); +const SALES_SHARED: (ScopeKind, Option<&str>) = (ScopeKind::Inherited, Some("models/sales")); + +/// Two sales models sharing a local `cents`, beside an optional descendant-public `labels`. +fn shared_cents(cents_path: &str, labels_path: Option<&str>) -> PlacementIndex { + let mut declarations = vec![macro_record("cents", cents_path, SALES_GROUPED)]; + declarations.extend(labels_path.map(|path| macro_record("labels", path, SALES_SHARED))); + PlacementIndex { + resources: vec![ + model_record("orders", "models/sales/orders.sql"), + model_record("refunds", "models/sales/refunds.sql"), + model_record("customers", "models/sales/crm/customers.sql"), + ], + declarations, + usages: vec![ + runtime("orders", "cents"), + runtime("refunds", "cents"), + runtime("refunds", "labels"), + runtime("customers", "labels"), + ], + } +} + +/// A grouped `cents` and an empty descendant-public role. +pub(super) fn grouped_cents_empty_role() -> PlacementIndex { + shared_cents("models/sales/_sqlbuild/_macros/money/cents.py", None) +} + +/// A grouped `cents` and a descendant-public role keeping its files in groups. +pub(super) fn grouped_cents_grouped_role() -> PlacementIndex { + shared_cents( + "models/sales/_sqlbuild/_macros/money/cents.py", + Some("models/sales/_sqlbuild/macros/text/labels.py"), + ) +} + +/// A direct `cents` and a descendant-public role keeping its files in groups. +pub(super) fn direct_cents_grouped_role() -> PlacementIndex { + shared_cents( + "models/sales/_sqlbuild/_macros/cents.py", + Some("models/sales/_sqlbuild/macros/text/labels.py"), + ) +} + +/// A grouped `cents` and a descendant-public role keeping its files directly. +pub(super) fn grouped_cents_direct_role() -> PlacementIndex { + shared_cents( + "models/sales/_sqlbuild/_macros/money/cents.py", + Some("models/sales/_sqlbuild/macros/labels.py"), + ) +} + +/// Owned `(model, destination)` moves. +pub(super) fn owned_moves(moves: &[(&str, &str)]) -> Vec<(ResourceIdentity, String)> { + moves + .iter() + .map(|(name, destination)| (model(name), (*destination).to_owned())) + .collect() +} + /// Owned `(declaration, path, owning path)` rows of expected relocations. pub(super) fn owned_relocations( items: &[(usize, &str, Option<&str>)], diff --git a/crates/sqlbuild-scopes/src/placement/tests/test_placement.rs b/crates/sqlbuild-scopes/src/placement/tests/test_placement.rs index 86b9b74014..b5d28dcb12 100644 --- a/crates/sqlbuild-scopes/src/placement/tests/test_placement.rs +++ b/crates/sqlbuild-scopes/src/placement/tests/test_placement.rs @@ -1,14 +1,16 @@ use crate::placement::_helpers::declaration_directory::declaration_directory; use crate::placement::main::diagnostic_order::diagnostic_order; use crate::placement::main::placement_diagnostics::placement_diagnostics; -use crate::placement::main::relocate_for_resource_move::relocate_for_resource_move; +use crate::placement::main::relocate_for_resource_moves::relocate_for_resource_moves; use crate::placement::models::Relocation; use crate::placement::tests::helpers::{ - chained_macros, descendant_models, mixed_diagnostics, model, owned_relocations, + chained_macros, descendant_models, direct_cents_grouped_role, grouped_cents_direct_role, + grouped_cents_empty_role, grouped_cents_grouped_role, mixed_diagnostics, model, owned_moves, + owned_relocations, }; use crate::placement::tests::test_types::{ - DeclarationDirectoryTestCase, DiagnosticOrderTestCase, PlacementDiagnosticsTestCase, - RelocationTestCase, + CarriedLayoutTestCase, DeclarationDirectoryTestCase, DiagnosticOrderTestCase, + PlacementDiagnosticsTestCase, RelocationTestCase, }; use crate::scope_index::models::{DeclarationKind, ScopeKind}; @@ -130,10 +132,9 @@ fn given_resource_move_when_relocating_then_chained_local_declarations_follow() }, ]; for test_case in test_cases { - let relocations: Option> = relocate_for_resource_move( + let relocations: Option> = relocate_for_resource_moves( &(test_case.index)(), - &model("orders"), - test_case.destination, + &[(model("orders"), test_case.destination.to_owned())], ); assert_eq!( @@ -150,6 +151,67 @@ fn given_resource_move_when_relocating_then_chained_local_declarations_follow() } } +#[test] +fn given_destination_role_layout_when_carrying_declaration_then_its_shape_is_matched() { + let test_cases = [ + CarriedLayoutTestCase { + description: "an empty destination role keeps the source group", + index: grouped_cents_empty_role, + moves: &[("orders", "models/sales/eu/orders.sql")], + expected_relocations: &[(0, "models/sales/_sqlbuild/macros/money/cents.py", false)], + }, + CarriedLayoutTestCase { + description: "a grouped destination role keeps the source group", + index: grouped_cents_grouped_role, + moves: &[("orders", "models/sales/eu/orders.sql")], + expected_relocations: &[(0, "models/sales/_sqlbuild/macros/money/cents.py", false)], + }, + CarriedLayoutTestCase { + description: "a direct source in a grouped destination role needs a group", + index: direct_cents_grouped_role, + moves: &[("orders", "models/sales/eu/orders.sql")], + expected_relocations: &[(0, "models/sales/_sqlbuild/macros/cents.py", true)], + }, + CarriedLayoutTestCase { + description: "a direct destination role drops the source group", + index: grouped_cents_direct_role, + moves: &[("orders", "models/sales/eu/orders.sql")], + expected_relocations: &[(0, "models/sales/_sqlbuild/macros/cents.py", false)], + }, + CarriedLayoutTestCase { + description: "moving every user together places the macro once at their new folder", + index: grouped_cents_grouped_role, + moves: &[ + ("orders", "models/sales/eu/orders.sql"), + ("refunds", "models/sales/eu/refunds.sql"), + ], + expected_relocations: &[(0, "models/sales/eu/_sqlbuild/_macros/money/cents.py", false)], + }, + ]; + for test_case in test_cases { + let relocations: Option> = + relocate_for_resource_moves(&(test_case.index)(), &owned_moves(test_case.moves)); + + assert_eq!( + relocations.map(|items| { + items + .into_iter() + .map(|item| (item.declaration, item.path, item.requires_group)) + .collect::>() + }), + Some( + test_case + .expected_relocations + .iter() + .map(|(position, path, group)| (*position, (*path).to_owned(), *group)) + .collect() + ), + "{}", + test_case.description + ); + } +} + #[test] fn given_diagnostics_when_ordering_then_python_sort_key_and_stability_hold() { let test_cases = [DiagnosticOrderTestCase { diff --git a/crates/sqlbuild-scopes/src/placement/tests/test_types.rs b/crates/sqlbuild-scopes/src/placement/tests/test_types.rs index 755b469832..ad3586056c 100644 --- a/crates/sqlbuild-scopes/src/placement/tests/test_types.rs +++ b/crates/sqlbuild-scopes/src/placement/tests/test_types.rs @@ -28,6 +28,14 @@ pub(super) struct RelocationTestCase { pub(super) expected_relocations: Option<&'static [RelocationCase]>, } +/// Resource moves applied together and each relocated `(declaration, path, requires_group)`. +pub(super) struct CarriedLayoutTestCase { + pub(super) description: &'static str, + pub(super) index: fn() -> PlacementIndex, + pub(super) moves: &'static [(&'static str, &'static str)], + pub(super) expected_relocations: &'static [(usize, &'static str, bool)], +} + /// Diagnostic order keys and the stable order Python sorts them into. pub(super) struct DiagnosticOrderTestCase { pub(super) description: &'static str, 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 8a817edcba..d627319255 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md @@ -10,6 +10,7 @@ Online: https://sqlbuild.com/docs/cli/rename/ - Usage - Rename or move +- Move several models at once - What changes - History - Refusals @@ -26,7 +27,8 @@ Nothing is written until the edited project compiles. The command applies its ed ```bash sqb rename [flags] -sqb mv [flags] +sqb mv [ ...] [flags] +sqb mv --from-file [flags] sqb rename . [--cascade] [flags] ``` @@ -55,6 +57,25 @@ sqb mv orders__int_clean__order_lines models/orders/intermediate/enriched/orders sqb mv orders__int_clean__order_lines models/orders/intermediate/clean/web/ ``` +## Move several models at once + +`sqb mv` takes several ` ` pairs, or `--from-file` with one pair per line, separated by a tab or spaces. Blank lines and lines starting with `#` are ignored. + +```bash +sqb mv stg_order_cents models/finance/ stg_order_refunds models/finance/ +sqb mv --from-file moves.tsv --dry-run +``` + +```text +# moves.tsv +stg_order_cents models/finance/ +stg_order_refunds models/finance/ +``` + +The moves are planned and verified together: the project compiles once before planning and the edited project once after, whatever the number of moves. As with a single move, adding `migrate_from` to models whose relation moves takes one more compile of the edited project. Declarations and unit tests are placed for the state after every move, so a macro whose users all move goes straight to their new folder instead of up to a shared folder and back. States between the moves are never checked, which lets a batch make a change that no order of single moves could make. As with one move, nothing is written unless the whole batch compiles. + +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. + ## What changes Renaming or moving a model rewrites: @@ -79,6 +100,8 @@ 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. + Declarations are moved, never copied. Folders the move leaves empty are removed. Renaming a column rewrites the model that produces it and every query that reads it: @@ -106,6 +129,7 @@ The command refuses a rename while the model, or the column, still declares `mig The command refuses, and writes nothing, when: - 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; - a move would leave no model in the old schema, so the old relation could not be found; - a reference is produced by a macro, or a column is read inside SQL that a macro generates; @@ -134,6 +158,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. | | `--json` | Print the result as JSON on stdout. Progress goes to stderr. | ## Output @@ -151,7 +176,7 @@ 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`. +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. ## Exit codes diff --git a/src/sqlbuild/_native.pyi b/src/sqlbuild/_native.pyi index 81d32c72aa..5060fad237 100644 --- a/src/sqlbuild/_native.pyi +++ b/src/sqlbuild/_native.pyi @@ -8403,9 +8403,11 @@ class RefactorCommandRequest(NativeRecord): """CLI inputs for one `sqb rename` or `sqb mv` invocation.""" command: str - target: str + target: str | None new_name: str | None destination: str | None + more_moves: tuple[str, ...] + from_file: str | None project_dir: Path | None cascade: bool dry_run: bool @@ -8414,9 +8416,11 @@ class RefactorCommandRequest(NativeRecord): def __init__( self, command: str, - target: str, + target: str | None = None, new_name: str | None = None, destination: str | None = None, + more_moves: tuple[str, ...] = (), + from_file: str | None = None, project_dir: Path | None = None, cascade: bool = False, dry_run: bool = False, 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 2c802d8726..b1b0c4960f 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 @@ -92,6 +92,15 @@ class DeclarationMoveE2ETestCase: expected_moved: dict[str, str] +@dataclass(frozen=True) +class DeclarationMoveRefusalE2ETestCase: + description: str + model: str + extra_files: dict[str, str] + destination: str + expected_message: str + + @dataclass(frozen=True) class HiddenOriginE2ETestCase: description: str @@ -157,3 +166,34 @@ class ScopedUnitTestMoveE2ETestCase: amount_sql: str expected_path: str expected_notes: tuple[str, ...] + + +@dataclass(frozen=True) +class BatchMoveE2ETestCase: + description: str + arguments: tuple[str, ...] + move_file: str + expected_moved: dict[str, str] + expected_compiles: int + + +@dataclass(frozen=True) +class SingleMoveOfBatchE2ETestCase: + description: str + move: tuple[str, str] + expected_reason: str + + +@dataclass(frozen=True) +class BatchMoveConflictE2ETestCase: + description: str + files: dict[str, str] + arguments: tuple[str, ...] + expected_reason: str + + +@dataclass(frozen=True) +class BatchMoveDryRunE2ETestCase: + description: 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 ca0ac74105..b231c94ec4 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py @@ -117,6 +117,75 @@ def project_toml(*, target_settings: str = "") -> str: } +_LAYOUT_RULES: str = '\n[rules]\nselect = ["SQBRDECLARATION302"]\n' +_LABEL_MACRO: str = 'def upper_label(expression: str) -> str:\n return f"UPPER({expression})"\n' + + +def grouped_role_files(*, cents_path: str, labels_path: str) -> dict[str, str]: + """Return the cents users with ``cents_path`` and a shared label macro at ``labels_path``. + + Both cents users sit in ``models/staging``; the label macro is read there and from + ``models/staging/eu``, so it is descendant-public at ``models/staging``. + """ + + return { + "sqlbuild_project.toml": project_toml() + _LAYOUT_RULES, + cents_path: CENTS_MACRO["models/staging/_sqlbuild/_macros/cents.py"], + "models/staging/stg_order_cents.sql": CENTS_MACRO["models/staging/stg_order_cents.sql"], + "models/staging/stg_order_refunds.sql": ORDER_REFUNDS[ + "models/staging/stg_order_refunds.sql" + ].replace("AS refund_cents\n", "AS refund_cents,\n @upper_label(\"'refund'\") AS label\n"), + labels_path: _LABEL_MACRO, + "models/staging/eu/stg_order_labels.sql": _label_model("stg_order_labels"), + } + + +def shared_cents_with_grouped_global_role() -> dict[str, str]: + """Return two staging users of a direct local cents macro beside a grouped ``macros/`` role. + + Moving only one user to ``models/finance`` would carry cents to the grouped top-level role, + which a direct file cannot join; moving both leaves it local to ``models/finance``. + """ + + return { + "sqlbuild_project.toml": project_toml() + _LAYOUT_RULES, + **CENTS_MACRO, + **ORDER_REFUNDS, + "macros/text/labels.py": _LABEL_MACRO, + "models/staging/stg_order_labels.sql": _label_model("stg_order_labels"), + "models/marts/order_labels.sql": _label_model("order_labels"), + } + + +def same_named_local_macros() -> dict[str, str]: + """Return two models in different folders, each using its own local ``helpers.py`` macro.""" + + return { + "models/staging/_sqlbuild/_macros/helpers.py": CENTS_MACRO[ + "models/staging/_sqlbuild/_macros/cents.py" + ], + "models/staging/stg_order_cents.sql": CENTS_MACRO["models/staging/stg_order_cents.sql"], + "models/marts/_sqlbuild/_macros/helpers.py": _LABEL_MACRO, + "models/marts/order_labels.sql": _label_model("order_labels"), + } + + +def _label_model(name: str) -> str: + return ( + f"MODEL (description 'Test model {name}.',\n materialized view,\n);\n\n" + 'SELECT\n order_id,\n @upper_label("\'order\'") AS label\nFROM __ref("stg_orders")\n' + ) + + +MOVE_FILE_PLACEHOLDER: str = "{move_file}" + + +def with_move_file(arguments: tuple[str, ...], move_file: Path) -> tuple[str, ...]: + """Return ``arguments`` with the move-file placeholder replaced by ``move_file``.""" + + return tuple(argument.replace(MOVE_FILE_PLACEHOLDER, str(move_file)) for argument in arguments) + + def order_amount_union(*, first: str, second: str) -> dict[str, str]: """Return a view unioning amount from two models, and a view reading its output.""" diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_batch_moves.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_batch_moves.py new file mode 100644 index 0000000000..9951b85b25 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_batch_moves.py @@ -0,0 +1,234 @@ +"""CLI e2e coverage for `sqb mv` moving 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 ( + BatchMoveConflictE2ETestCase, + BatchMoveDryRunE2ETestCase, + BatchMoveE2ETestCase, + SingleMoveOfBatchE2ETestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.refactor.helpers import ( + MOVE_FILE_PLACEHOLDER, + existing_files, + project_text, + relation_type, + same_named_local_macros, + shared_cents_with_grouped_global_role, + sqb, + with_move_file, + write_orders_project, +) + +_CENTS: str = "models/staging/_sqlbuild/_macros/cents.py" +_COMPILE_STARTS: tuple[str, ...] = ( + "Compiling project...", + "Verifying the edited project compiles...", +) +_BOTH_USERS: tuple[str, ...] = ( + "model:stg_order_cents", + "models/finance/", + "stg_order_refunds", + "models/finance/", +) +_EXPECTED_MOVED: dict[str, str] = { + "models/staging/stg_order_cents.sql": "models/finance/stg_order_cents.sql", + "models/staging/stg_order_refunds.sql": "models/finance/stg_order_refunds.sql", + _CENTS: "models/finance/_sqlbuild/_macros/cents.py", +} + + +@pytest.mark.parametrize( + "test_case", + [ + BatchMoveE2ETestCase( + description="pairs on the command line", + arguments=_BOTH_USERS, + move_file="", + expected_moved=_EXPECTED_MOVED, + expected_compiles=2, + ), + BatchMoveE2ETestCase( + description="pairs from a file with comments and blank lines", + arguments=("--from-file", MOVE_FILE_PLACEHOLDER), + move_file=( + "# move every cents user together\n\n" + "model:stg_order_cents\tmodels/finance/\n" + " stg_order_refunds models/finance/ \n" + ), + expected_moved=_EXPECTED_MOVED, + expected_compiles=2, + ), + ], + ids=lambda case: case.description, +) +def test_given_moves_invalid_one_at_a_time_when_moved_together_then_one_verification_applies_all( + tmp_path: Path, test_case: BatchMoveE2ETestCase +) -> None: + """Every user of a local macro moves at once, so the macro moves once to their new folder.""" + + project_dir: Path = write_orders_project( + tmp_path=tmp_path, files=shared_cents_with_grouped_global_role() + ) + move_file: Path = tmp_path / "moves.tsv" + _ = move_file.write_text(test_case.move_file, encoding="utf-8") + + moved: subprocess.CompletedProcess[str] = sqb( + project_dir, "mv", *with_move_file(test_case.arguments, move_file), "--json" + ) + payload: dict[str, Any] = json.loads(moved.stdout) + built: subprocess.CompletedProcess[str] = sqb(project_dir, "build") + + assert moved.returncode == 0, moved.stdout + moved.stderr + assert payload["status"] == "applied" + assert sum(moved.stderr.count(start) for start in _COMPILE_STARTS) == ( + test_case.expected_compiles + ), moved.stderr + assert existing_files( + project_dir=project_dir, + paths=(*test_case.expected_moved, *test_case.expected_moved.values()), + ) == tuple(sorted(test_case.expected_moved.values())), moved.stdout + assert built.returncode == 0, built.stdout + built.stderr + assert relation_type(project_dir=project_dir, name="stg_order_refunds") is not None + + +@pytest.mark.parametrize( + "test_case", + [ + SingleMoveOfBatchE2ETestCase( + description="moving one user alone needs a group in the top-level role", + move=("stg_order_cents", "models/finance/"), + expected_reason="macro:to_cents must move to macros/, which keeps its files in group", + ), + ], + ids=lambda case: case.description, +) +def test_given_one_move_of_the_batch_alone_when_moving_then_it_is_refused( + tmp_path: Path, test_case: SingleMoveOfBatchE2ETestCase +) -> None: + """The batch above is needed: its moves are not valid one at a time.""" + + project_dir: Path = write_orders_project( + tmp_path=tmp_path, files=shared_cents_with_grouped_global_role() + ) + before: dict[str, str] = project_text(project_dir) + + moved: subprocess.CompletedProcess[str] = sqb(project_dir, "mv", *test_case.move) + + assert moved.returncode == 1, moved.stdout + moved.stderr + assert test_case.expected_reason in " ".join(moved.stdout.split()) + assert project_text(project_dir) == before + + +@pytest.mark.parametrize( + "test_case", + [ + BatchMoveConflictE2ETestCase( + files=shared_cents_with_grouped_global_role(), + description="two models moved to the same file", + arguments=( + "stg_order_cents", + "models/finance/order_amounts.sql", + "stg_order_refunds", + "models/finance/order_amounts.sql", + ), + expected_reason=( + "model:stg_order_cents and model:stg_order_refunds both move to " + "models/finance/order_amounts.sql" + ), + ), + BatchMoveConflictE2ETestCase( + files=shared_cents_with_grouped_global_role(), + description="one model moved twice", + arguments=( + "stg_order_cents", + "models/finance/", + "stg_order_cents", + "models/marts/", + ), + expected_reason="model:stg_order_cents is moved more than once", + ), + BatchMoveConflictE2ETestCase( + files=shared_cents_with_grouped_global_role(), + description="a model taking the name another model gives up", + arguments=( + "stg_order_refunds", + "models/staging/stg_order_returns.sql", + "stg_order_cents", + "models/staging/stg_order_refunds.sql", + ), + expected_reason=( + "model:stg_order_cents would take the name model:stg_order_refunds gives up" + ), + ), + BatchMoveConflictE2ETestCase( + description="two moved models' same-named local macro files sharing one target", + files=same_named_local_macros(), + arguments=( + "stg_order_cents", + "models/finance/", + "order_labels", + "models/finance/", + ), + expected_reason=( + "models/marts/_sqlbuild/_macros/helpers.py and " + "models/staging/_sqlbuild/_macros/helpers.py would both move to " + "models/finance/_sqlbuild/_macros/helpers.py; rename one of them first" + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_conflicting_moves_when_moved_together_then_batch_is_refused_and_nothing_changes( + tmp_path: Path, test_case: BatchMoveConflictE2ETestCase +) -> 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) + + assert moved.returncode == 1, moved.stdout + moved.stderr + assert test_case.expected_reason in " ".join(moved.stdout.split()), moved.stdout + assert "Refused; no files changed" in moved.stdout + assert project_text(project_dir) == before + + +@pytest.mark.parametrize( + "test_case", + [ + BatchMoveDryRunE2ETestCase( + description="both cents users and the macro they share", + arguments=_BOTH_USERS, + expected_lines=( + "Move 2 models", + "├── stg_order_cents -> models/finance/stg_order_cents.sql", + "└── stg_order_refunds -> models/finance/stg_order_refunds.sql", + f"├── models/finance/_sqlbuild/_macros/cents.py (moved from {_CENTS})", + "Dry run: compiles, 3 files would change; nothing written", + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_batch_dry_run_when_moving_then_combined_plan_is_printed_and_nothing_written( + tmp_path: Path, test_case: BatchMoveDryRunE2ETestCase +) -> None: + project_dir: Path = write_orders_project( + tmp_path=tmp_path, files=shared_cents_with_grouped_global_role() + ) + 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 set(test_case.expected_lines) <= set(moved.stdout.splitlines()), moved.stdout + assert project_text(project_dir) == before diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_move_declarations.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_move_declarations.py index 1305866406..ef8065ca18 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_move_declarations.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_move_declarations.py @@ -9,18 +9,22 @@ from tests.e2e.src.sqlbuild.cli.commands.main.refactor._test_types import ( DeclarationMoveE2ETestCase, + DeclarationMoveRefusalE2ETestCase, ) from tests.e2e.src.sqlbuild.cli.commands.main.refactor.helpers import ( CENTS_MACRO, ORDER_REFUNDS, ORDER_SHAPE, existing_files, + grouped_role_files, + project_text, relation_type, sqb, write_orders_project, ) _CENTS: str = "models/staging/_sqlbuild/_macros/cents.py" +_GROUPED_CENTS: str = "models/staging/_sqlbuild/_macros/money/cents.py" _PRIORITY_ENUM: dict[str, str] = { "models/marts/_sqlbuild/_enums/priority.sql": ( "ENUM (\n name priority,\n members (LOW 1, HIGH 3),\n);\n" @@ -62,6 +66,26 @@ destination="models/marts/", expected_moved={_CENTS: "macros/cents.py"}, ), + DeclarationMoveE2ETestCase( + description="a grouped macro keeps its group in a grouped destination role", + model="stg_order_cents", + extra_files=grouped_role_files( + cents_path=_GROUPED_CENTS, + labels_path="models/staging/_sqlbuild/macros/text/labels.py", + ), + destination="models/staging/eu/", + expected_moved={_GROUPED_CENTS: "models/staging/_sqlbuild/macros/money/cents.py"}, + ), + DeclarationMoveE2ETestCase( + description="a grouped macro drops its group in a direct destination role", + model="stg_order_cents", + extra_files=grouped_role_files( + cents_path=_GROUPED_CENTS, + labels_path="models/staging/_sqlbuild/macros/labels.py", + ), + destination="models/staging/eu/", + expected_moved={_GROUPED_CENTS: "models/staging/_sqlbuild/macros/cents.py"}, + ), ], ids=lambda case: case.description, ) @@ -86,3 +110,37 @@ def test_given_scoped_declarations_when_moving_model_then_they_follow_placement_ assert "error[S0" not in compiled.stdout + compiled.stderr assert built.returncode == 0, built.stdout + built.stderr assert relation_type(project_dir=project_dir, name=model) is not None + + +@pytest.mark.parametrize( + "test_case", + [ + DeclarationMoveRefusalE2ETestCase( + description="a direct macro cannot join a grouped destination role", + model="stg_order_cents", + extra_files=grouped_role_files( + cents_path=_CENTS, labels_path="models/staging/_sqlbuild/macros/text/labels.py" + ), + destination="models/staging/eu/", + expected_message=( + "macro:to_cents must move to models/staging/_sqlbuild/macros/, which keeps its " + f"files in group folders, but {_CENTS} is not in a group; move it into a group " + "folder of its current role first" + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_direct_declaration_and_grouped_destination_role_when_moving_then_move_is_refused( + tmp_path: Path, test_case: DeclarationMoveRefusalE2ETestCase +) -> None: + project_dir: Path = write_orders_project(tmp_path=tmp_path, files=test_case.extra_files) + before: dict[str, str] = project_text(project_dir) + + moved: subprocess.CompletedProcess[str] = sqb( + project_dir, "mv", f"model:{test_case.model}", test_case.destination + ) + + assert moved.returncode == 1, moved.stdout + moved.stderr + assert test_case.expected_message in " ".join((moved.stdout + moved.stderr).split()) + assert project_text(project_dir) == before diff --git a/website/src/content/docs/docs/cli/rename.mdx b/website/src/content/docs/docs/cli/rename.mdx index fa5226d2b2..2488c0b617 100644 --- a/website/src/content/docs/docs/cli/rename.mdx +++ b/website/src/content/docs/docs/cli/rename.mdx @@ -11,7 +11,8 @@ Nothing is written until the edited project compiles. The command applies its ed ```bash sqb rename [flags] -sqb mv [flags] +sqb mv [ ...] [flags] +sqb mv --from-file [flags] sqb rename . [--cascade] [flags] ``` @@ -40,6 +41,25 @@ sqb mv orders__int_clean__order_lines models/orders/intermediate/enriched/orders sqb mv orders__int_clean__order_lines models/orders/intermediate/clean/web/ ``` +## Move several models at once + +`sqb mv` takes several ` ` pairs, or `--from-file` with one pair per line, separated by a tab or spaces. Blank lines and lines starting with `#` are ignored. + +```bash +sqb mv stg_order_cents models/finance/ stg_order_refunds models/finance/ +sqb mv --from-file moves.tsv --dry-run +``` + +```text +# moves.tsv +stg_order_cents models/finance/ +stg_order_refunds models/finance/ +``` + +The moves are planned and verified together: the project compiles once before planning and the edited project once after, whatever the number of moves. As with a single move, adding `migrate_from` to models whose relation moves takes one more compile of the edited project. Declarations and unit tests are placed for the state after every move, so a macro whose users all move goes straight to their new folder instead of up to a shared folder and back. States between the moves are never checked, which lets a batch make a change that no order of single moves could make. As with one move, nothing is written unless the whole batch compiles. + +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. + ## What changes Renaming or moving a model rewrites: @@ -64,6 +84,8 @@ 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. + Declarations are moved, never copied. Folders the move leaves empty are removed. Renaming a column rewrites the model that produces it and every query that reads it: @@ -91,6 +113,7 @@ The command refuses a rename while the model, or the column, still declares `mig The command refuses, and writes nothing, when: - 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; - a move would leave no model in the old schema, so the old relation could not be found; - a reference is produced by a macro, or a column is read inside SQL that a macro generates; @@ -119,6 +142,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. | | `--json` | Print the result as JSON on stdout. Progress goes to stderr. | ## Output @@ -136,7 +160,7 @@ 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`. +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. ## Exit codes