From dd755c2b0bf3ba84e677beff856764bd7112eab9 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 15:10:14 +0100 Subject: [PATCH 1/3] fix: give a carried declaration the shape of its destination role When sqb mv carries a scoped declaration to a wider owner, the file now matches the layout of the role folder it lands in instead of always being written directly in it. An empty role keeps the source's group folder, a grouped role keeps the source's group, and a direct role drops it. A file without a group that must join a grouped role is refused with a message to move it into a group first, rather than writing a layout that verification rejects for mixing direct and grouped files. Placement relocation now accepts several resource moves at once. --- .../_helpers/refactor/refactor_facts.rs | 35 ++++---- .../_helpers/scopes/scope_validation.rs | 34 ++++---- .../_helpers/planning/declaration_moves.rs | 13 +++ .../src/refactoring/models.rs | 3 + .../src/refactoring/tests/helpers.rs | 1 + .../src/placement/_helpers/mod.rs | 2 + .../relocation.rs} | 84 ++++++++++--------- .../src/placement/_helpers/role_layout.rs | 66 +++++++++++++++ .../sqlbuild-scopes/src/placement/main/mod.rs | 2 +- .../main/relocate_for_resource_moves.rs | 41 +++++++++ .../sqlbuild-scopes/src/placement/models.rs | 2 + .../src/placement/tests/helpers.rs | 60 +++++++++++++ .../src/placement/tests/test_placement.rs | 76 +++++++++++++++-- .../src/placement/tests/test_types.rs | 8 ++ .../sqlbuild/references/docs/cli/rename.md | 3 + .../cli/commands/main/refactor/_test_types.py | 32 +++++++ .../cli/commands/main/refactor/helpers.py | 56 +++++++++++++ .../main/refactor/test_move_declarations.py | 58 +++++++++++++ website/src/content/docs/docs/cli/rename.mdx | 3 + 19 files changed, 502 insertions(+), 77 deletions(-) rename crates/sqlbuild-scopes/src/placement/{main/relocate_for_resource_move.rs => _helpers/relocation.rs} (56%) create mode 100644 crates/sqlbuild-scopes/src/placement/_helpers/role_layout.rs create mode 100644 crates/sqlbuild-scopes/src/placement/main/relocate_for_resource_moves.rs 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..2a97e4eddb 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/refactor/refactor_facts.rs @@ -2,23 +2,22 @@ use std::collections::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"; @@ -392,15 +391,19 @@ pub(crate) fn declaration_placement( 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 resource: ResourceIdentity = ResourceIdentity { + kind: ResourceKind::Model, + name: model.to_owned(), + }; + let records = relocated_declarations(&index, &[(resource, destination.to_owned())])?; + 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 +419,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()?, }); } 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/refactoring/_helpers/planning/declaration_moves.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/declaration_moves.rs index 776e29a261..51d3fc8946 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/declaration_moves.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/declaration_moves.rs @@ -47,6 +47,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()); diff --git a/crates/sqlbuild-refactor/src/refactoring/models.rs b/crates/sqlbuild-refactor/src/refactoring/models.rs index 08ff608936..4721907953 100644 --- a/crates/sqlbuild-refactor/src/refactoring/models.rs +++ b/crates/sqlbuild-refactor/src/refactoring/models.rs @@ -270,6 +270,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..58afc9d7d3 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs @@ -428,6 +428,7 @@ pub(super) fn relocated(label: &str, path: &str) -> RelocatedDeclaration { RelocatedDeclaration { key: label.to_owned(), path: path.to_owned(), + requires_group: false, } } 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..4fdf9085a8 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md @@ -79,6 +79,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 +108,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; 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..9ca4498f34 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,26 @@ 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 + arguments: tuple[str, ...] + expected_reason: 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..184ce824d1 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,62 @@ 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 _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_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..52c3222d00 100644 --- a/website/src/content/docs/docs/cli/rename.mdx +++ b/website/src/content/docs/docs/cli/rename.mdx @@ -64,6 +64,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 +93,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; From 93c2be66a48ee382e726df5fba16e71bb754e19b Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 18:45:07 +0100 Subject: [PATCH 2/3] feat: move several models in one verified sqb mv sqb mv now takes several TARGET DESTINATION pairs, or --from-file with one pair per line (tab or space separated, # comments and blank lines allowed). The project compiles once before planning and the edited project once after. Every move is planned together: declarations and unit tests are placed for the state after all moves, a model may take a file or name another move leaves, and a batch that moves a model twice, sends two models to one file or gives them one name is refused. Nothing is written unless the whole batch verifies; --dry-run prints the combined plan, and JSON output lists the moves. A single move plans and prints as before. --- .../sqlbuild-cli/src/argv/command_table.json | 23 +- crates/sqlbuild-cli/src/argv/constants.rs | 4 +- crates/sqlbuild-cli/src/dispatch/constants.rs | 4 +- .../src/dispatch/main/command_dispatch.rs | 3 +- .../dispatch/tests/test_command_dispatch.rs | 14 +- .../bindings/_helpers/records/constants.rs | 4 +- .../_helpers/refactor/refactor_command.rs | 109 +++++++-- .../_helpers/refactor/refactor_facts.rs | 68 ++++-- .../src/refactor_command/_helpers/json.rs | 15 ++ .../refactor_command/_helpers/output_text.rs | 47 +++- .../src/refactor_command/constants.rs | 3 + .../src/refactor_command/main/mod.rs | 1 + .../src/refactor_command/main/move_pairs.rs | 58 +++++ .../main/render_refactor_json.rs | 21 +- .../main/render_refactor_text.rs | 5 +- .../src/refactor_command/models.rs | 7 + .../src/refactor_command/tests.rs | 2 + .../src/refactor_command/tests/helpers.rs | 1 + .../refactor_command/tests/test_move_pairs.rs | 64 ++++++ .../src/refactor_command/tests/test_types.rs | 8 + .../refactoring/_helpers/edits/text_edits.rs | 1 + .../_helpers/planning/batch_planning.rs | 161 +++++++++++++ .../_helpers/planning/declaration_moves.rs | 29 ++- .../src/refactoring/_helpers/planning/mod.rs | 2 + .../_helpers/planning/model_planning.rs | 99 ++++++-- .../_helpers/planning/single_planning.rs | 51 ++++ .../src/refactoring/main/plan_refactor.rs | 61 +++-- .../refactoring/main/with_model_migration.rs | 46 ++-- .../src/refactoring/models.rs | 12 + .../src/refactoring/tests/helpers.rs | 60 ++++- .../src/refactoring/tests/test_planning.rs | 109 ++++++++- .../src/refactoring/tests/test_types.rs | 8 + .../src/refactoring/types.rs | 4 +- .../sqlbuild/references/docs/cli/rename.md | 26 ++- src/sqlbuild/_native.pyi | 8 +- .../cli/commands/main/refactor/_test_types.py | 7 + .../main/refactor/test_batch_moves.py | 217 ++++++++++++++++++ website/src/content/docs/docs/cli/rename.mdx | 25 +- 38 files changed, 1220 insertions(+), 167 deletions(-) create mode 100644 crates/sqlbuild-refactor/src/refactor_command/main/move_pairs.rs create mode 100644 crates/sqlbuild-refactor/src/refactor_command/tests/test_move_pairs.rs create mode 100644 crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs create mode 100644 crates/sqlbuild-refactor/src/refactoring/_helpers/planning/single_planning.rs create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_batch_moves.py 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 2a97e4eddb..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,6 +1,6 @@ //! 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, PyAny, PyAnyMethods, PyResult, Python}; use pyo3::types::{PyDict, PyDictMethods, PyString, PyTuple, PyTupleMethods}; @@ -305,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()?; @@ -331,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, }) } @@ -370,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( @@ -381,21 +386,35 @@ 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 resource: ResourceIdentity = ResourceIdentity { - kind: ResourceKind::Model, - name: model.to_owned(), - }; - let records = relocated_declarations(&index, &[(resource, destination.to_owned())])?; + 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) { @@ -430,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()? { @@ -447,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-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..93d37f0c65 --- /dev/null +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs @@ -0,0 +1,161 @@ +//! 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}; +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_test_destinations(&test_moves)); + 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 +} + +/// Unit test files of different models that would move to the same path. +fn shared_test_destinations(moves: &[(String, 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(conflict( + destination, + format!("the unit tests {other} and {source} would both move here"), + )); + } + } + shared +} + +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 51d3fc8946..10669fb7b5 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(); @@ -90,7 +101,7 @@ pub(crate) fn declaration_moves( 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, 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 4721907953..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() diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs index 58afc9d7d3..7ada1ae1b5 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs @@ -18,7 +18,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 +322,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() @@ -438,8 +438,17 @@ pub(super) fn planned_declaration_moves( 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 moves: DeclarationMoves = declaration_moves( + root, + &[ModelMove { + model: "orders", + source: paths.0, + destination: paths.1, + }], + &BTreeSet::new(), + &host, + )?; let blocking: Vec = moves .blocking .iter() @@ -675,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() @@ -688,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, )?; @@ -730,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..a95490de9d 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] @@ -415,3 +416,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..bc40c44065 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs @@ -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/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/rename.md index 4fdf9085a8..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: @@ -137,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 @@ -154,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 9ca4498f34..c31ed81cce 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 @@ -189,3 +189,10 @@ class BatchMoveConflictE2ETestCase: description: 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/test_batch_moves.py b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_batch_moves.py new file mode 100644 index 0000000000..888eb208ae --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/test_batch_moves.py @@ -0,0 +1,217 @@ +"""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, + 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( + 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( + 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( + 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" + ), + ), + ], + 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=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) + + assert moved.returncode == 1, moved.stdout + moved.stderr + assert test_case.expected_reason in moved.stdout, moved.stdout + moved.stderr + 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/website/src/content/docs/docs/cli/rename.mdx b/website/src/content/docs/docs/cli/rename.mdx index 52c3222d00..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: @@ -122,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 @@ -139,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 From eef0d545c5d3c942fe30e04afb211fe7d86eba2f Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 21 Oct 2026 19:17:58 +0100 Subject: [PATCH 3/3] fix: refuse a move that sends two declaration files to one path Two carried declaration files could share one target, either from two moved models' same-named local files or from grouped files flattened into a direct role. Planning now refuses this up front, naming both files, instead of leaving it to a confusing verification compile error. --- .../_helpers/planning/batch_planning.rs | 26 +++--- .../_helpers/planning/declaration_moves.rs | 28 +++++++ .../src/refactoring/tests/helpers.rs | 24 +++--- .../src/refactoring/tests/test_planning.rs | 80 +++++++++++++++++-- .../src/refactoring/tests/test_types.rs | 4 +- .../cli/commands/main/refactor/_test_types.py | 1 + .../cli/commands/main/refactor/helpers.py | 13 +++ .../main/refactor/test_batch_moves.py | 25 +++++- 8 files changed, 161 insertions(+), 40 deletions(-) diff --git a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs index 93d37f0c65..3562c4665b 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/batch_planning.rs @@ -5,7 +5,9 @@ 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}; +use crate::refactoring::_helpers::planning::declaration_moves::{ + ModelMove, declaration_moves, shared_destinations, +}; use crate::refactoring::_helpers::planning::model_planning::{ FinalState, ModelTarget, own_model_parts, }; @@ -68,7 +70,12 @@ pub(crate) fn batch_parts( parts.blocking.extend(collisions); parts.blocking.extend(declaration.blocking); parts.blocking.extend(test_blocking); - parts.blocking.extend(shared_test_destinations(&test_moves)); + 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); @@ -136,21 +143,6 @@ fn batch_conflicts(targets: &[ModelTarget<'_>]) -> Vec { conflicts } -/// Unit test files of different models that would move to the same path. -fn shared_test_destinations(moves: &[(String, 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(conflict( - destination, - format!("the unit tests {other} and {source} would both move here"), - )); - } - } - shared -} - fn conflict(path: &str, reason: String) -> ManualLocation { ManualLocation { path: path.to_owned(), 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 10669fb7b5..7c42772c89 100644 --- a/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/declaration_moves.rs +++ b/crates/sqlbuild-refactor/src/refactoring/_helpers/planning/declaration_moves.rs @@ -97,6 +97,14 @@ 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 @@ -115,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/tests/helpers.rs b/crates/sqlbuild-refactor/src/refactoring/tests/helpers.rs index 7ada1ae1b5..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, @@ -432,23 +433,22 @@ pub(super) fn relocated(label: &str, path: &str) -> RelocatedDeclaration { } } -/// 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 = |_: &[(String, String)]| Ok(placement.clone()); - let moves: DeclarationMoves = declaration_moves( - root, - &[ModelMove { - model: "orders", - source: paths.0, - destination: paths.1, - }], - &BTreeSet::new(), - &host, - )?; + 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() diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs index a95490de9d..242a9ea7d7 100644 --- a/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs +++ b/crates/sqlbuild-refactor/src/refactoring/tests/test_planning.rs @@ -236,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: &[], @@ -244,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()], @@ -256,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: &[ @@ -270,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"), @@ -304,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), diff --git a/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs b/crates/sqlbuild-refactor/src/refactoring/tests/test_types.rs index bc40c44065..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. 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 c31ed81cce..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 @@ -187,6 +187,7 @@ class SingleMoveOfBatchE2ETestCase: @dataclass(frozen=True) class BatchMoveConflictE2ETestCase: description: str + files: dict[str, str] arguments: tuple[str, ...] expected_reason: 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 184ce824d1..b231c94ec4 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/refactor/helpers.py @@ -157,6 +157,19 @@ def shared_cents_with_grouped_global_role() -> dict[str, str]: } +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" 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 index 888eb208ae..9951b85b25 100644 --- 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 @@ -20,6 +20,7 @@ existing_files, project_text, relation_type, + same_named_local_macros, shared_cents_with_grouped_global_role, sqb, with_move_file, @@ -130,6 +131,7 @@ def test_given_one_move_of_the_batch_alone_when_moving_then_it_is_refused( "test_case", [ BatchMoveConflictE2ETestCase( + files=shared_cents_with_grouped_global_role(), description="two models moved to the same file", arguments=( "stg_order_cents", @@ -143,6 +145,7 @@ def test_given_one_move_of_the_batch_alone_when_moving_then_it_is_refused( ), ), BatchMoveConflictE2ETestCase( + files=shared_cents_with_grouped_global_role(), description="one model moved twice", arguments=( "stg_order_cents", @@ -153,6 +156,7 @@ def test_given_one_move_of_the_batch_alone_when_moving_then_it_is_refused( 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", @@ -164,21 +168,34 @@ def test_given_one_move_of_the_batch_alone_when_moving_then_it_is_refused( "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=shared_cents_with_grouped_global_role() - ) + 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 moved.stdout, 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