diff --git a/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/constants.py b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/constants.py new file mode 100644 index 000000000..8465d8768 --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/constants.py @@ -0,0 +1,3 @@ +"""Carrier pickup constants.""" + +MINIMUM_CHARGE_CENTS: int = 250 diff --git a/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py new file mode 100644 index 000000000..86672d2b3 --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/conforming/providers/carrier_api/provider.py @@ -0,0 +1,17 @@ +"""Carrier pickup client.""" + +from providers.carrier_api.constants import MINIMUM_CHARGE_CENTS + +from sqlbuild.providers import Provider + +_CENTS_PER_UNIT: int = 100 + + +class CarrierApi(Provider): + """Client for the carrier pickup API.""" + + endpoint: str = "https://carrier.example.com" + + def minimum_charge(self) -> float: + """Return the minimum pickup charge in currency units.""" + return MINIMUM_CHARGE_CENTS / _CENTS_PER_UNIT diff --git a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json index c72b412c7..f7ae9dfa5 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json +++ b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json @@ -2,6 +2,9 @@ "select": ["SQBRPYTHON", "SQBRPYTHON306"], "findings": [ ["SQBRPYTHON101", "hooks/python/alert.py", 4, 11], + ["SQBRPYTHON015", "providers/carrier_api/provider.py", 3, 8], + ["SQBRPYTHON014", "providers/carrier_api/provider.py", 8, 5], + ["SQBRPYTHON013", "providers/stock_feed.py", 1, 1], ["SQBRPYTHON613", "python/inventory/__init__.py", 1, 1], ["SQBRPYTHON002", "python/inventory/common/__init__.py", 1, 1], ["SQBRPYTHON306", "python/inventory/common/stock.py", 4, 1], diff --git a/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py new file mode 100644 index 000000000..4a3dedd1d --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/carrier_api/provider.py @@ -0,0 +1,16 @@ +"""Carrier pickup client.""" + +import python.report + +from sqlbuild.providers import Provider + + +def pickup_label(sku: str) -> str: + """Return the pickup label of one product.""" + return f"{sku} {python.report.__name__}" + + +class CarrierApi(Provider): + """Client for the carrier pickup API.""" + + endpoint: str = "https://carrier.example.com" diff --git a/benchmarks/duckshop/edge_cases/python_rules/violations/providers/stock_feed.py b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/stock_feed.py new file mode 100644 index 000000000..f12b5b36c --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/violations/providers/stock_feed.py @@ -0,0 +1 @@ +"""A provider module directly under providers/.""" diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs index 934d7ac58..7d82ac38c 100644 --- a/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs @@ -173,7 +173,7 @@ fn union_names(annotation: &Expr) -> Vec { } /// The last name of a name, an attribute chain, or the subscripted base of either. -fn last_name(expression: &Expr) -> Option { +pub(crate) fn last_name(expression: &Expr) -> Option { match expression { Expr::Name(name) => Some(name.id.to_string()), Expr::Attribute(attribute) => Some(attribute.attr.to_string()), diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs index 0d018f6f9..da1b2da49 100644 --- a/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs @@ -1,3 +1,4 @@ pub(crate) mod functions; pub(crate) mod imports; pub(crate) mod positions; +pub(crate) mod targets; diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/targets.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/targets.rs new file mode 100644 index 000000000..b008ce826 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/targets.rs @@ -0,0 +1,49 @@ +//! The names assignments bind and the dotted names of class bases. + +use ruff_python_ast::Expr; + +/// The names an assignment's targets bind; empty when any target is an attribute or subscript. +pub(crate) fn assignment_names(targets: &[Expr]) -> Vec { + elements_names(targets).unwrap_or_default() +} + +/// Each class base as dotted text, such as `paged.PagedApi`; empty for any other expression. +pub(crate) fn class_bases(bases: &[Expr]) -> Vec { + bases.iter().map(base_name).collect() +} + +/// One class base as dotted text, or empty. +fn base_name(base: &Expr) -> String { + dotted_name(base).unwrap_or_default() +} + +/// The names one target binds through tuples, lists and starred targets; `None` for an attribute or subscript. +fn target_names(target: &Expr) -> Option> { + match target { + Expr::Name(name) => Some(vec![name.id.to_string()]), + Expr::Tuple(tuple) => elements_names(&tuple.elts), + Expr::List(list) => elements_names(&list.elts), + Expr::Starred(starred) => target_names(&starred.value), + _ => None, + } +} + +/// The names several targets bind; `None` when any of them binds no plain name. +fn elements_names(elements: &[Expr]) -> Option> { + let names: Vec> = elements.iter().map(target_names).collect::>()?; + Some(names.concat()) +} + +/// A name or attribute chain as dotted text, seen through a subscript such as `Generic[T]`. +fn dotted_name(expression: &Expr) -> Option { + match expression { + Expr::Name(name) => Some(name.id.to_string()), + Expr::Attribute(attribute) => Some(format!( + "{}.{}", + dotted_name(&attribute.value)?, + attribute.attr + )), + Expr::Subscript(subscript) => dotted_name(&subscript.value), + _ => None, + } +} diff --git a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs index 1ad582ceb..3197611d3 100644 --- a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs @@ -8,7 +8,8 @@ use crate::cpython_tree::models::ParsedSource; use crate::node_units::_helpers::functions::{leading_docstring, unit_function}; use crate::node_units::_helpers::imports::{is_type_checking_imports, module_imports}; use crate::node_units::_helpers::positions::{UnitText, unit_position}; -use crate::node_units::models::{UnitModule, UnitStatement, UnitStatementKind}; +use crate::node_units::_helpers::targets::{assignment_names, class_bases}; +use crate::node_units::models::{UnitClass, UnitModule, UnitStatement, UnitStatementKind}; /// The top-level statements and imports of `contents`, or `None` when it does not parse. #[must_use] @@ -32,6 +33,17 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option { UnitStatementKind::Function(unit_function(function, &source)) } + Stmt::ClassDef(class) => UnitStatementKind::Class(UnitClass { + name: class.name.to_string(), + position: unit_position(&source, class.name.range().start()), + bases: class_bases(class.bases()), + }), + Stmt::Assign(assign) => { + UnitStatementKind::Assignment(assignment_names(&assign.targets)) + } + Stmt::AnnAssign(assign) => UnitStatementKind::Assignment(assignment_names( + std::slice::from_ref(&assign.target), + )), _ => UnitStatementKind::Other, }, position: unit_position(&source, statement.range().start()), diff --git a/crates/sqlbuild-pysource/src/node_units/models.rs b/crates/sqlbuild-pysource/src/node_units/models.rs index fb2ddaa36..e1587cd6d 100644 --- a/crates/sqlbuild-pysource/src/node_units/models.rs +++ b/crates/sqlbuild-pysource/src/node_units/models.rs @@ -29,10 +29,23 @@ pub enum UnitStatementKind { /// An import, or an `if TYPE_CHECKING:` block that only imports. Import, Function(UnitFunction), - /// Any other statement, such as a class, an assignment or a loop. + Class(UnitClass), + /// An assignment, annotated or not, by the names it binds; empty when any target is an attribute or subscript. + Assignment(Vec), + /// Any other statement, such as a loop or a call. Other, } +/// A class and its bases. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct UnitClass { + pub name: String, + /// Where the class's name is written. + pub position: UnitPosition, + /// Each base as dotted text, such as `Provider` or `paged.PagedApi`; empty when it is another expression. + pub bases: Vec, +} + /// One imported name of an `import` or `from ... import` at module level. #[derive(Clone, Debug, PartialEq, Eq)] pub struct UnitImport { diff --git a/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs index 18c4892f5..cc9f74360 100644 --- a/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs @@ -14,17 +14,22 @@ const FACTORY_MODULE: &str = "@factory\ndef tables():\n for table in TABLES:\ fn given_module_when_reading_then_each_top_level_statement_has_its_kind() { let test_cases = [ StatementKindsTestCase { - description: "a CRLF node module with a docstring, imports, a value and a node", + description: "a CRLF node module with a docstring, imports, an assignment and a node", contents: CRLF_NODE_MODULE, expected_kinds: Some(vec![ "Docstring", "Import", "Import", "Import", - "Other", + "Assignment", "Function", ]), }, + StatementKindsTestCase { + description: "a class and a loop", + contents: "class Store(Provider):\n pass\n\n\nfor row in rows:\n pass\n", + expected_kinds: Some(vec!["Class", "Other"]), + }, StatementKindsTestCase { description: "a module that does not parse", contents: "def broken(:\n", diff --git a/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs b/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs index 1ddcca0d0..e6eee93ec 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs @@ -59,6 +59,9 @@ const REVIEWED_SCOPES: &[(&str, RuleScope)] = &[ ("SQBRPYTHON010", RuleScope::Python), ("SQBRPYTHON011", RuleScope::Python), ("SQBRPYTHON012", RuleScope::Python), + ("SQBRPYTHON013", RuleScope::Python), + ("SQBRPYTHON014", RuleScope::Python), + ("SQBRPYTHON015", RuleScope::Python), ("SQBRPYTHON101", RuleScope::Python), ("SQBRPYTHON102", RuleScope::Python), ("SQBRPYTHON103", RuleScope::Python), diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs index 86a8cf301..4be254067 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs @@ -3,3 +3,4 @@ pub(crate) mod layout; pub(crate) mod modules; pub(crate) mod nodes; pub(crate) mod project; +pub(crate) mod providers; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs index 07a5d825b..2f9f4de65 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs @@ -53,7 +53,9 @@ fn node_module_faults(file: &UnitFile<'_>) -> Vec { function.position, ), }, - UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), + UnitStatementKind::Class(_) + | UnitStatementKind::Assignment(_) + | UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), }; faults.extend(unit_fault( NODE_MODULE_SHAPE_CODE, @@ -95,7 +97,9 @@ fn main_module_faults(file: &UnitFile<'_>, project: &UnitProject<'_>) -> Vec (OTHER_STATEMENT.to_owned(), statement.position), + UnitStatementKind::Class(_) + | UnitStatementKind::Assignment(_) + | UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), }; faults.extend(unit_fault( MAIN_MODULE_SHAPE_CODE, @@ -208,7 +212,9 @@ fn checks_module_faults(file: &UnitFile<'_>, project: &UnitProject<'_>) -> Vec { + UnitStatementKind::Class(_) + | UnitStatementKind::Assignment(_) + | UnitStatementKind::Other => { vec![(OTHER_STATEMENT.to_owned(), statement.position)] } }; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs index ff3c6791b..8299d9621 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs @@ -107,7 +107,7 @@ fn nearest_unit(parts: &[&str], units: &BTreeSet) -> Option { } /// The dotted module of a Python path and of every package above it. -fn dotted_names(path: &str) -> Vec { +pub(crate) fn dotted_names(path: &str) -> Vec { let stem: &str = path.strip_suffix(".py").unwrap_or(path); let parts: Vec<&str> = stem .split('/') @@ -207,11 +207,21 @@ pub(crate) fn imported_module( import: &UnitImport, file: &UnitFile<'_>, project: &UnitProject<'_>, +) -> Option { + let mut folder: Vec<&str> = vec![PACKAGE_ROOT]; + folder.extend(&file.parts[..file.parts.len() - 1]); + absolute_module(import, &folder, &project.modules) +} + +/// The absolute dotted module `import` reads from a file in the project folder `folder`, given every project module. +pub(crate) fn absolute_module( + import: &UnitImport, + folder: &[&str], + modules: &BTreeSet, ) -> Option { let mut base: Vec<&str> = Vec::new(); if import.level > 0 { - base.push(PACKAGE_ROOT); - base.extend(&file.parts[..file.parts.len() - 1]); + base.extend(folder); for _ in 1..import.level { base.pop()?; } @@ -222,7 +232,7 @@ pub(crate) fn imported_module( return Some(module); }; let submodule: String = format!("{module}.{name}"); - Some(if project.modules.contains(&submodule) { + Some(if modules.contains(&submodule) { submodule } else { module diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs new file mode 100644 index 000000000..971166a87 --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/providers.rs @@ -0,0 +1,400 @@ +//! Provider capability folders: what they hold, what provider.py declares, and who imports their private code. + +use std::collections::{BTreeMap, BTreeSet}; + +use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; + +use sqlbuild_pysource::node_units::main::node_unit_module::node_unit_module; +use sqlbuild_pysource::node_units::models::{ + UnitClass, UnitModule, UnitPosition, UnitStatementKind, +}; + +use crate::models::{Fault, PythonSource}; +use crate::python_rules::_helpers::engine::faults::catalogue_fault; +use crate::python_rules::_helpers::engine::placement::package_anchor; +use crate::python_rules::_helpers::units::project::{absolute_module, dotted_names, unit_fault}; +use crate::python_rules::constants::{ + INIT_FILE, ORCHESTRATION_ROOT, PACKAGE_ROOT, PRIVATE_PREFIX, PROVIDER_BASE, PROVIDER_FILE, + PROVIDER_IMPORT_BOUNDARIES_CODE, PROVIDER_LAYOUT_CODE, PROVIDER_MODULE_SHAPE_CODE, + PROVIDER_PRIVATE_FOLDERS, PROVIDER_ROLE_FILES, PROVIDER_RULE_CODES, PROVIDERS_ROOT, + SHARED_UTILITIES_PACKAGE, STAR_IMPORT, +}; + +const FILE_START: UnitPosition = UnitPosition { line: 1, column: 1 }; +const OTHER_STATEMENT: &str = "a statement that is not an import, a constant or the provider class"; +/// The package Provider is imported from. +const SQLBUILD_PACKAGE: &str = "sqlbuild"; + +/// One parsed project file and its path parts. +#[derive(Debug)] +struct ParsedFile<'a> { + path: &'a str, + parts: Vec<&'a str>, + module: UnitModule, +} + +/// Faults of the selected provider rules among `codes`. +pub(crate) fn provider_faults( + sources: &[PythonSource], + codes: &BTreeSet<&str>, + python_version: (u8, u8), +) -> Vec { + if !PROVIDER_RULE_CODES.iter().any(|code| codes.contains(code)) { + return Vec::new(); + } + let mut faults: Vec = provider_layout_faults(sources); + let files: Vec> = sources + .par_iter() + .filter(|source| { + source.relative_path.split('/').next() == Some(PROVIDERS_ROOT) + || source.text.contains(PROVIDERS_ROOT) + }) + .filter_map(|source| { + Some(ParsedFile { + path: source.relative_path.as_str(), + parts: source.relative_path.split('/').collect(), + module: node_unit_module(&source.text, python_version)?, + }) + }) + .collect(); + let modules: BTreeSet = sources + .iter() + .flat_map(|source| dotted_names(&source.relative_path)) + .collect(); + let classes: ProviderClasses = provider_classes(&files, &modules); + for file in &files { + if matches!(file.parts.as_slice(), [root, _, file_name] if *root == PROVIDERS_ROOT && *file_name == PROVIDER_FILE) + { + faults.extend(provider_module_faults(file, &classes)); + } + faults.extend(import_faults(file, &modules)); + } + faults.retain(|fault| codes.contains(fault.code.as_str())); + faults +} + +/// Modules directly in providers/, capability folders without provider.py, and modules or folders they may not hold. +fn provider_layout_faults(sources: &[PythonSource]) -> Vec { + let mut faults: Vec = Vec::new(); + let mut capabilities: BTreeSet<&str> = BTreeSet::new(); + let mut folders: BTreeSet = BTreeSet::new(); + for source in sources { + let path: &str = source.relative_path.as_str(); + let Some(rest) = path.strip_prefix(&format!("{PROVIDERS_ROOT}/")) else { + continue; + }; + let parts: Vec<&str> = rest.split('/').collect(); + let detail: Option = match parts.as_slice() { + [file] if *file == INIT_FILE => None, + [_] => Some(format!("{path} sits directly in {PROVIDERS_ROOT}/")), + [capability, file] => { + capabilities.insert(capability); + (*file != PROVIDER_FILE && !PROVIDER_ROLE_FILES.contains(file)) + .then(|| format!("{path} is not provider.py or a provider role file")) + } + [capability, folder, ..] => { + capabilities.insert(capability); + if !PROVIDER_PRIVATE_FOLDERS.contains(folder) { + folders.insert(format!("{PROVIDERS_ROOT}/{capability}/{folder}")); + } + None + } + [] => None, + }; + if let Some(detail) = detail { + faults.extend(catalogue_fault(PROVIDER_LAYOUT_CODE, path, (1, 1), &detail)); + } + } + let paths: BTreeSet<&str> = sources + .iter() + .map(|source| source.relative_path.as_str()) + .collect(); + faults.extend( + capabilities + .iter() + .map(|capability| format!("{PROVIDERS_ROOT}/{capability}")) + .filter(|folder| !paths.contains(format!("{folder}/{PROVIDER_FILE}").as_str())) + .filter_map(|folder| { + catalogue_fault( + PROVIDER_LAYOUT_CODE, + package_anchor(sources, &folder)?, + (1, 1), + &format!("{folder}/ has no {PROVIDER_FILE}"), + ) + }), + ); + faults.extend(folders.iter().filter_map(|folder| { + catalogue_fault( + PROVIDER_LAYOUT_CODE, + package_anchor(sources, folder)?, + (1, 1), + &format!("{folder}/ is not _helpers/ or _classes/"), + ) + })); + faults +} + +/// provider.py: imports, private constants and exactly one public Provider subclass. +fn provider_module_faults(file: &ParsedFile<'_>, classes: &ProviderClasses) -> Vec { + let module: String = file_module(file.path); + let mut faults: Vec = Vec::new(); + let mut providers: usize = 0; + let mut undecided: usize = 0; + for statement in &file.module.statements { + let (detail, position): (String, UnitPosition) = match &statement.kind { + UnitStatementKind::Docstring | UnitStatementKind::Import => continue, + UnitStatementKind::Assignment(names) if names.is_empty() => { + (OTHER_STATEMENT.to_owned(), statement.position) + } + UnitStatementKind::Assignment(names) => { + match names.iter().find(|name| !name.starts_with(PRIVATE_PREFIX)) { + Some(name) => ( + format!("public constant '{name}' belongs in constants.py"), + statement.position, + ), + None => continue, + } + } + UnitStatementKind::Class(class) => { + let verdict: Verdict = classes.verdict(&module, &class.name); + let detail: Option = class_detail(class, verdict, providers); + if !class.name.starts_with(PRIVATE_PREFIX) { + providers += usize::from(verdict == Verdict::Provider); + undecided += usize::from(verdict == Verdict::Unknown); + } + match detail { + Some(detail) => (detail, class.position), + None => continue, + } + } + UnitStatementKind::Function(function) => ( + format!("function '{}' belongs in _helpers/", function.name), + function.position, + ), + UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), + }; + faults.extend(unit_fault( + PROVIDER_MODULE_SHAPE_CODE, + file.path, + position, + &detail, + )); + } + if providers == 0 && undecided == 0 { + faults.extend(unit_fault( + PROVIDER_MODULE_SHAPE_CODE, + file.path, + FILE_START, + &format!("{} declares no public {PROVIDER_BASE} subclass", file.path), + )); + } + faults +} + +/// Why `class` does not belong in provider.py, after `earlier` public Provider subclasses. +fn class_detail(class: &UnitClass, verdict: Verdict, earlier: usize) -> Option { + let name: &str = &class.name; + if name.starts_with(PRIVATE_PREFIX) { + return Some(format!("collaborator class '{name}' belongs in _classes/")); + } + match verdict { + Verdict::NotProvider => Some(format!("class '{name}' does not subclass {PROVIDER_BASE}")), + Verdict::Provider => { + (earlier > 0).then(|| format!("'{name}' is a second {PROVIDER_BASE} subclass")) + } + Verdict::Unknown => None, + } +} + +/// Whether a class subclasses Provider, as far as the parsed project sources show. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum Verdict { + Provider, + NotProvider, + /// A base leads outside what was parsed, such as another package, so the rule stays silent. + Unknown, +} + +/// What a class's base names: Provider, a project class, a builtin, or something unresolved. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +enum BaseTarget { + Provider, + Class(String, String), + Builtin, + Unknown, +} + +/// The bases of every class in the parsed files, by module and class name. +struct ProviderClasses { + bases: BTreeMap<(String, String), Vec>, +} + +impl ProviderClasses { + /// Whether class `name` of `module` reaches Provider; `Unknown` when a base chain leaves the parsed classes. + fn verdict(&self, module: &str, name: &str) -> Verdict { + let mut seen: BTreeSet<(String, String)> = BTreeSet::new(); + let mut pending: Vec<(String, String)> = vec![(module.to_owned(), name.to_owned())]; + let mut unknown: bool = false; + while let Some(key) = pending.pop() { + if !seen.insert(key.clone()) { + continue; + } + let Some(bases) = self.bases.get(&key) else { + unknown = true; + continue; + }; + for base in bases { + match base { + BaseTarget::Provider => return Verdict::Provider, + BaseTarget::Class(module, name) => pending.push((module.clone(), name.clone())), + BaseTarget::Builtin => {} + BaseTarget::Unknown => unknown = true, + } + } + } + if unknown { + Verdict::Unknown + } else { + Verdict::NotProvider + } + } +} + +/// Every class of `files` with what each of its bases names. +fn provider_classes(files: &[ParsedFile<'_>], modules: &BTreeSet) -> ProviderClasses { + let mut bases: BTreeMap<(String, String), Vec> = BTreeMap::new(); + for file in files { + let module: String = file_module(file.path); + for statement in &file.module.statements { + if let UnitStatementKind::Class(class) = &statement.kind { + let targets: Vec = class + .bases + .iter() + .map(|base| base_target(file, base, modules)) + .collect(); + bases.insert((module.clone(), class.name.clone()), targets); + } + } + } + ProviderClasses { bases } +} + +/// What `base` names in `file`, resolved through its imports and its own classes. +fn base_target(file: &ParsedFile<'_>, base: &str, modules: &BTreeSet) -> BaseTarget { + let (first, attributes): (&str, Option<&str>) = match base.split_once('.') { + Some((first, attributes)) => (first, Some(attributes)), + None => (base, None), + }; + if first.is_empty() { + return BaseTarget::Unknown; + } + let folder: &[&str] = &file.parts[..file.parts.len() - 1]; + if let Some(import) = file + .module + .imports + .iter() + .find(|import| import.local == first) + { + let Some(source) = absolute_module(import, folder, modules) else { + return BaseTarget::Unknown; + }; + let (module, name): (String, &str) = match (&import.name, attributes) { + (Some(name), None) if source.ends_with(&format!(".{name}")) => { + return BaseTarget::Unknown; + } + (Some(name), None) => (source, name.as_str()), + (bound, Some(attributes)) => { + let bound_module: String = match bound { + Some(name) if source.ends_with(&format!(".{name}")) => source, + Some(_) => return BaseTarget::Unknown, + None if import.module.split('.').next() == Some(import.local.as_str()) => { + import.local.clone() + } + None => source, + }; + match attributes.rsplit_once('.') { + Some((path, name)) => (format!("{bound_module}.{path}"), name), + None => (bound_module, attributes), + } + } + (None, None) => return BaseTarget::Unknown, + }; + return if module.split('.').next() == Some(SQLBUILD_PACKAGE) && name == PROVIDER_BASE { + BaseTarget::Provider + } else { + BaseTarget::Class(module, name.to_owned()) + }; + } + if attributes.is_some() + || file + .module + .imports + .iter() + .any(|import| import.local == STAR_IMPORT) + { + return BaseTarget::Unknown; + } + let local_class: bool = file.module.statements.iter().any(|statement| { + matches!(&statement.kind, UnitStatementKind::Class(class) if class.name == base) + }); + if local_class { + BaseTarget::Class(file_module(file.path), base.to_owned()) + } else if base == PROVIDER_BASE { + BaseTarget::Provider + } else { + BaseTarget::Builtin + } +} + +/// The dotted module of a project file, such as `providers.storefront_api.provider`. +fn file_module(path: &str) -> String { + dotted_names(path).pop().unwrap_or_default() +} + +/// Imports of another provider's private code, and a provider's imports of python/ outside utils/ or of orchestration/. +fn import_faults(file: &ParsedFile<'_>, modules: &BTreeSet) -> Vec { + let capability: Option<&str> = match file.parts.as_slice() { + [root, capability, _, ..] if *root == PROVIDERS_ROOT => Some(capability), + _ => None, + }; + let folder: &[&str] = &file.parts[..file.parts.len() - 1]; + file.module + .imports + .iter() + .filter_map(|import| { + let module: String = absolute_module(import, folder, modules)?; + if !modules.contains(&module) { + return None; + } + let detail: String = boundary_detail(&module, capability)?; + unit_fault( + PROVIDER_IMPORT_BOUNDARIES_CODE, + file.path, + import.position, + &format!("{} imports {module}, {detail}", file.path), + ) + }) + .collect() +} + +/// Why a file in provider `capability` (or outside providers/ when `None`) may not import `module`. +fn boundary_detail(module: &str, capability: Option<&str>) -> Option { + let parts: Vec<&str> = module.split('.').collect(); + if let [root, owner, folder, ..] = parts.as_slice() { + let private: bool = *root == PROVIDERS_ROOT && PROVIDER_PRIVATE_FOLDERS.contains(folder); + if private && capability != Some(*owner) { + return Some(format!("which is private to {PROVIDERS_ROOT}/{owner}/")); + } + } + capability?; + match parts.as_slice() { + [root, utils, ..] if *root == PACKAGE_ROOT && *utils == SHARED_UTILITIES_PACKAGE => None, + [root, ..] if *root == PACKAGE_ROOT => Some(format!( + "but a provider imports only {PACKAGE_ROOT}/{SHARED_UTILITIES_PACKAGE}/ from {PACKAGE_ROOT}/" + )), + [root, ..] if *root == ORCHESTRATION_ROOT => Some(format!( + "but a provider never imports {ORCHESTRATION_ROOT}/" + )), + _ => None, + } +} diff --git a/crates/sqlbuild-rules/src/python_rules/constants.rs b/crates/sqlbuild-rules/src/python_rules/constants.rs index 114a17029..77649efd2 100644 --- a/crates/sqlbuild-rules/src/python_rules/constants.rs +++ b/crates/sqlbuild-rules/src/python_rules/constants.rs @@ -5,7 +5,7 @@ use fensu_native::rules::constants as fensu; use crate::python_rules::models::{PythonFiles, PythonRuleSpec}; /// Every `SQBRPYTHON` rule; numbers are never reused, and new rules append to their band. -pub(crate) const PYTHON_RULES: [PythonRuleSpec; 74] = [ +pub(crate) const PYTHON_RULES: [PythonRuleSpec; 77] = [ PythonRuleSpec { code: "SQBRPYTHON001", fensu_code: None, @@ -150,6 +150,42 @@ pub(crate) const PYTHON_RULES: [PythonRuleSpec; 74] = [ message: "nodes are declared in a unit's node.py, and checks in a unit's checks.py", remediation: "Move the declaration into its own unit folder under its domain, with the implementation in that folder's main.py.", }, + PythonRuleSpec { + code: "SQBRPYTHON013", + fensu_code: None, + slug: "provider-layout", + family: "providers", + owner: "file", + applies: PythonFiles::All, + enabled_by_default: true, + options: &[], + message: "providers/ holds capability folders, each with provider.py, optional role files, _helpers/ and _classes/ only", + remediation: "Move a module directly under providers/ into providers//provider.py, add provider.py to each capability folder, and move other modules into _helpers/, _classes/ or a role file: models.py, types.py, constants.py or exceptions.py.", + }, + PythonRuleSpec { + code: "SQBRPYTHON014", + fensu_code: None, + slug: "provider-module-shape", + family: "providers", + owner: "file", + applies: PythonFiles::All, + enabled_by_default: true, + options: &[], + message: "provider.py holds imports, private constants and exactly one public Provider subclass", + remediation: "Keep one public class that subclasses Provider in provider.py; move functions into _helpers/, collaborator classes into _classes/ and public constants into constants.py.", + }, + PythonRuleSpec { + code: "SQBRPYTHON015", + fensu_code: None, + slug: "provider-import-boundaries", + family: "providers", + owner: "file", + applies: PythonFiles::All, + enabled_by_default: true, + options: &[], + message: "a provider's _helpers/ and _classes/ are private to it, and providers import only python/utils/ from the runtime package and nothing from orchestration/", + remediation: "Import a provider through provider.py or its role files; move code a provider shares with nodes into python/utils//.", + }, PythonRuleSpec { code: "SQBRPYTHON101", fensu_code: Some(fensu::PARAMETER_ANNOTATION_CODE), @@ -1036,6 +1072,35 @@ pub(crate) const NODE_DESCRIPTION_CODE: &str = "SQBRPYTHON011"; pub(crate) const NODE_DECLARED_IN_UNIT_CODE: &str = "SQBRPYTHON012"; /// The fewest folders below `python/` a unit sits in: its domain and its own. pub(crate) const MIN_UNIT_DEPTH: usize = 2; +pub(crate) const PROVIDER_LAYOUT_CODE: &str = "SQBRPYTHON013"; +pub(crate) const PROVIDER_MODULE_SHAPE_CODE: &str = "SQBRPYTHON014"; +pub(crate) const PROVIDER_IMPORT_BOUNDARIES_CODE: &str = "SQBRPYTHON015"; +/// The provider rules, which read parsed modules across the project. +pub(crate) const PROVIDER_RULE_CODES: [&str; 3] = [ + PROVIDER_LAYOUT_CODE, + PROVIDER_MODULE_SHAPE_CODE, + PROVIDER_IMPORT_BOUNDARIES_CODE, +]; +/// The file declaring a provider capability's Provider subclass. +pub(crate) const PROVIDER_FILE: &str = "provider.py"; +/// The files a provider capability folder may hold beside provider.py. +pub(crate) const PROVIDER_ROLE_FILES: [&str; 5] = [ + "models.py", + "types.py", + "constants.py", + "exceptions.py", + INIT_FILE, +]; +/// The private folders a provider capability folder may hold. +pub(crate) const PROVIDER_PRIVATE_FOLDERS: [&str; 2] = ["_helpers", "_classes"]; +/// The base class a provider subclasses. +pub(crate) const PROVIDER_BASE: &str = "Provider"; +/// The folder holding orchestration declarations, imported as `orchestration`. +pub(crate) const ORCHESTRATION_ROOT: &str = "orchestration"; +/// The prefix of a private name. +pub(crate) const PRIVATE_PREFIX: &str = "_"; +/// The name a `from module import *` binds. +pub(crate) const STAR_IMPORT: &str = "*"; /// The file that declares a unit's node and makes its folder a unit. pub(crate) const UNIT_NODE_FILE: &str = "node.py"; /// The file holding a unit's checks. diff --git a/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs b/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs index 0ebea79d5..103cc7fe3 100644 --- a/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs +++ b/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs @@ -13,6 +13,7 @@ use crate::python_rules::_helpers::engine::placement::python_targets; use crate::python_rules::_helpers::engine::programs::Programs; use crate::python_rules::_helpers::engine::project_plane::loaded_project; use crate::python_rules::_helpers::units::project::unit_faults; +use crate::python_rules::_helpers::units::providers::provider_faults; use crate::python_rules::models::{ CallInputs, PythonCacheEntry, PythonEvaluation, PythonRuleSpec, PythonTarget, }; @@ -49,6 +50,11 @@ pub(crate) fn evaluate_python_rules( &codes, request.python_version, )); + evaluation.faults.extend(provider_faults( + &request.python_sources, + &codes, + request.python_version, + )); evaluation.faults.sort_by(|left, right| { (&left.path, left.line, left.column, &left.code).cmp(&( &right.path, diff --git a/crates/sqlbuild-rules/src/python_rules/tests.rs b/crates/sqlbuild-rules/src/python_rules/tests.rs index 0506832c2..ae23fb200 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests.rs @@ -4,6 +4,8 @@ mod evaluate_python_rules; mod helpers; #[path = "tests/test_node_units.rs"] mod node_units; +#[path = "tests/test_provider_rules.rs"] +mod provider_rules; #[path = "tests/test_python_cache.rs"] mod python_cache; #[path = "tests/test_python_catalogue.rs"] diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs new file mode 100644 index 000000000..1dbe18cb4 --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_provider_rules.rs @@ -0,0 +1,331 @@ +use serde_json::json; + +use crate::python_rules::tests::helpers::{owned_faults, python_rule_faults}; +use crate::python_rules::tests::test_types::PythonRunTestCase; + +const PROVIDER_RULES: [&str; 3] = ["SQBRPYTHON013", "SQBRPYTHON014", "SQBRPYTHON015"]; + +#[test] +fn given_providers_when_evaluating_provider_rules_then_reports_each_broken_convention() { + let test_cases = [ + PythonRunTestCase { + description: "capability folders with provider.py, role files and private folders report nothing", + files: &[ + ("providers/__init__.py", ""), + ( + "providers/storefront_api/provider.py", + r#""""Storefront client.""" +from sqlbuild.providers import Provider + +from python.utils.retries import ATTEMPTS + +from ._classes.page_cursor import PageCursor + +_PAGE_SIZE: int = 100 + + +class StorefrontApi(Provider): + """Read-only client for the storefront order API.""" + + base_url: str +"#, + ), + ("providers/storefront_api/models.py", ""), + ("providers/storefront_api/_classes/page_cursor.py", ""), + ("providers/storefront_api/_helpers/paging.py", ""), + ("python/utils/retries.py", ""), + ( + "python/orders/daily/main.py", + "from providers.storefront_api.provider import StorefrontApi\n", + ), + ], + config: json!({ "select": PROVIDER_RULES }), + expected_faults: vec![], + }, + PythonRunTestCase { + description: "layout: a module directly in providers/, a stray module, a missing provider.py and a stray folder", + files: &[ + ("providers/storefront_api.py", ""), + ("providers/carrier_api/client.py", ""), + ("providers/partner_sftp/provider.py", ""), + ("providers/partner_sftp/sftp/connection.py", ""), + ], + config: json!({ "select": ["SQBRPYTHON013"] }), + expected_faults: vec![ + ("SQBRPYTHON013", "providers/carrier_api/client.py", 1, 1), + ("SQBRPYTHON013", "providers/carrier_api/client.py", 1, 1), + ( + "SQBRPYTHON013", + "providers/partner_sftp/sftp/connection.py", + 1, + 1, + ), + ("SQBRPYTHON013", "providers/storefront_api.py", 1, 1), + ], + }, + PythonRunTestCase { + description: "provider.py: a public constant, a function, a private and a plain class, a second provider, and none at all", + files: &[ + ( + "providers/storefront_api/provider.py", + r#""""Doc.""" +from sqlbuild.providers import Provider + +_TIMEOUT: int = 3 +RETRIES = 2 + + +def build() -> None: + pass + + +class _Cursor: + pass + + +class Helper: + pass + + +class StorefrontApi(Provider): + """Client.""" + + +class CarrierApi(Provider): + """Second.""" +"#, + ), + ("providers/empty/provider.py", "import os\n"), + ], + config: json!({ "select": ["SQBRPYTHON014"] }), + expected_faults: vec![ + ("SQBRPYTHON014", "providers/empty/provider.py", 1, 1), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 5, + 1, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 8, + 5, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 12, + 7, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 16, + 7, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 24, + 7, + ), + ], + }, + PythonRunTestCase { + description: "provider.py: Provider through an alias, a module attribute or _classes/ bases reached by relative imports; bases outside the parsed sources stay silent", + files: &[ + ( + "providers/storefront_api/provider.py", + r#"from ._classes.paged import PagedApi + + +class StorefrontApi(PagedApi): + """Client.""" +"#, + ), + ( + "providers/storefront_api/_classes/paged.py", + "from .base import BaseApi\n\n\nclass PagedApi(BaseApi):\n pass\n", + ), + ( + "providers/storefront_api/_classes/base.py", + "from sqlbuild.providers import Provider\n\n\nclass BaseApi(Provider):\n pass\n", + ), + ( + "providers/carrier_api/provider.py", + r#"from sqlbuild.providers import Provider as SqbProvider + + +class CarrierApi(SqbProvider): + """Client.""" +"#, + ), + ( + "providers/billing_api/provider.py", + "from ._classes import paged\n\n\nclass BillingApi(paged.PagedApi):\n \"\"\"Client.\"\"\"\n", + ), + ( + "providers/billing_api/_classes/paged.py", + "from sqlbuild.providers import Provider\n\n\nclass PagedApi(Provider):\n pass\n", + ), + ( + "providers/ledger_api/provider.py", + "from ._classes import LedgerBase\n\n\nclass LedgerApi(LedgerBase):\n \"\"\"Client.\"\"\"\n", + ), + ( + "providers/ledger_api/_classes/__init__.py", + "from .base import LedgerBase\n", + ), + ( + "providers/ledger_api/_classes/base.py", + "from sqlbuild.providers import Provider\n\n\nclass LedgerBase(Provider):\n pass\n", + ), + ( + "providers/catalog_api/provider.py", + "from python.utils.clients import PagedClient\n\n\nclass CatalogApi(PagedClient):\n \"\"\"Client.\"\"\"\n", + ), + ( + "python/utils/clients.py", + "from python.utils.base import BaseClient\n\n\nclass PagedClient(BaseClient):\n pass\n", + ), + ( + "python/utils/base.py", + "from sqlbuild.providers import Provider\n\n\nclass BaseClient(Provider):\n pass\n", + ), + ( + "providers/partner_api/provider.py", + "from vendor.clients import VendorProvider\n\n\nclass PartnerApi(VendorProvider):\n \"\"\"Client.\"\"\"\n", + ), + ], + config: json!({ "select": ["SQBRPYTHON014"] }), + expected_faults: vec![], + }, + PythonRunTestCase { + description: "provider.py: tuple, subscript, mixed and chained assignments, a builtin base and a module attribute naming a plain class", + files: &[ + ( + "providers/storefront_api/provider.py", + r#"import os + +from sqlbuild.providers import Provider + +_HOST, _PORT = "h", 1 +HOST, _TIMEOUT = "h", 3 +os.environ["TZ"] = "UTC" +_ZONE, os.environ["TZ"] = "h", "UTC" +_TZ = os.environ["TZ"] = "UTC" + + +class StorefrontApi(Provider): + """Client.""" +"#, + ), + ( + "providers/carrier_api/provider.py", + "from ._classes import shapes\n\n\nclass CarrierApi(shapes.Parcel):\n \"\"\"Client.\"\"\"\n", + ), + ( + "providers/carrier_api/_classes/shapes.py", + "class Parcel:\n pass\n", + ), + ( + "providers/returns_api/provider.py", + "class ReturnsApi(Exception):\n \"\"\"Client.\"\"\"\n", + ), + ], + config: json!({ "select": ["SQBRPYTHON014"] }), + expected_faults: vec![ + ("SQBRPYTHON014", "providers/carrier_api/provider.py", 1, 1), + ("SQBRPYTHON014", "providers/carrier_api/provider.py", 4, 7), + ("SQBRPYTHON014", "providers/returns_api/provider.py", 1, 1), + ("SQBRPYTHON014", "providers/returns_api/provider.py", 1, 7), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 6, + 1, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 7, + 1, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 8, + 1, + ), + ( + "SQBRPYTHON014", + "providers/storefront_api/provider.py", + 9, + 1, + ), + ], + }, + PythonRunTestCase { + description: "imports: another provider's private code from anywhere, and python/ outside utils/ or orchestration/ from a provider", + files: &[ + ( + "providers/storefront_api/provider.py", + r#"from python.utils.retries import ATTEMPTS +from python.orders.daily.main import main +from orchestration.sensors.orders import on_orders +from providers.carrier_api._helpers.auth import token +from ._classes.cursor import Cursor +from providers.carrier_api.provider import CarrierApi +"#, + ), + ("providers/storefront_api/_classes/cursor.py", ""), + ("providers/carrier_api/provider.py", ""), + ("providers/carrier_api/_helpers/auth.py", ""), + ("python/utils/retries.py", ""), + ( + "python/orders/daily/main.py", + "from providers.storefront_api._classes.cursor import Cursor\n", + ), + ("orchestration/sensors/orders.py", ""), + ( + "hooks/python/notify.py", + "import providers.carrier_api._helpers.auth\n", + ), + ], + config: json!({ "select": ["SQBRPYTHON015"] }), + expected_faults: vec![ + ("SQBRPYTHON015", "hooks/python/notify.py", 1, 8), + ( + "SQBRPYTHON015", + "providers/storefront_api/provider.py", + 2, + 38, + ), + ( + "SQBRPYTHON015", + "providers/storefront_api/provider.py", + 3, + 42, + ), + ( + "SQBRPYTHON015", + "providers/storefront_api/provider.py", + 4, + 49, + ), + ("SQBRPYTHON015", "python/orders/daily/main.py", 1, 54), + ], + }, + ]; + for test_case in test_cases { + let faults = python_rule_faults(test_case.files, test_case.config.clone()); + + assert_eq!( + faults, + Ok(owned_faults(&test_case.expected_faults)), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs index f8be29887..95ce1c8d8 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs @@ -14,14 +14,14 @@ use crate::rules::models::BuiltinOptionKind; fn given_the_catalogue_when_listing_python_rules_then_each_has_its_own_project_wide_code() { let test_cases = [ CatalogueTestCase { - description: "12 SQLBuild layout and unit rules and 62 Fensu core rules have distinct codes", + description: "15 SQLBuild layout, unit and provider rules and 62 Fensu core rules have distinct codes", count: distinct_python_codes, - expected_count: 74, + expected_count: 77, }, CatalogueTestCase { description: "every Python rule reports against the project in the python family", count: project_wide_python_rules, - expected_count: 74, + expected_count: 77, }, ]; for test_case in test_cases { diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md index d1b9d7117..c9be71b78 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/providers.md @@ -24,10 +24,10 @@ Providers are shared runtime services that SQLBuild discovers, configures, and i ## Defining a provider -Create a Python file under `providers/` in your project. A provider is a class that subclasses `Provider` from `sqlbuild.providers`: +Create a `provider.py` in a capability folder under `providers/` (see [Project layout](#project-layout)). A provider is a class that subclasses `Provider` from `sqlbuild.providers`: ```python -# providers/warehouse_client.py +# providers/warehouse_client/provider.py from sqlbuild.providers import Provider class WarehouseClient(Provider): @@ -83,7 +83,7 @@ You can also type-annotate the parameter for IDE support and compile-time valida ```python from sqlbuild.tasks import task -from providers.warehouse_client import WarehouseClient +from providers.warehouse_client.provider import WarehouseClient @task def export_orders(ctx, warehouse_client: WarehouseClient): @@ -217,19 +217,30 @@ Setting values, defaults included, are never published. ## Project layout +Give each provider its own folder under `providers/`, named for the capability it provides: + ``` my-project/ providers/ - warehouse_client.py - slack_notifier.py + warehouse_client/ + provider.py # the Provider subclass + constants.py + _helpers/ # private to warehouse_client + slack_notifier/ + provider.py hooks/ sql/ record_notification.sql python/ notify.py python/ - loaders/ - load_orders.py - tasks/ - export_orders.py + orders/ + load_orders/ + node.py + main.py ``` + +Discovery also accepts `.py` files directly in `providers/`. A project that selects the +[Python Rules](../rules/python-rules.md#laying-out-providers) reports them, along with +anything else in a provider folder besides `provider.py`, its role files, `_helpers/` and +`_classes/`. diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md index 60562d761..78938aa8f 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md @@ -12,6 +12,7 @@ Online: https://sqlbuild.com/docs/concepts/rules/python-rules/ - Which files are checked - Laying out `python/` - Writing nodes as units +- Laying out providers - Main modules and entry modules - Unreachable definitions - Rule options @@ -159,6 +160,35 @@ A unit may import another unit's `node.py` only to name that node in `depends_on A factory's `node.py` holds one `@factory` function, and a check generated by a factory may stay in `node.py`. `@audit_factory` functions are not units and may sit anywhere under `python/`. +## Laying out providers + +`SQBRPYTHON013` to `SQBRPYTHON015` keep each provider in its own capability folder, with its +private code beside it: + +```text +providers/ + storefront_api/ + provider.py # the one public Provider subclass + models.py + _classes/ + page_cursor.py # a private collaborator class + carrier_api/ + provider.py +``` + +A capability folder may also hold `types.py`, `constants.py`, `exceptions.py`, `__init__.py` and +`_helpers/`. These are reported: + +| Code | Finding | Fix | +|---|---|---| +| `SQBRPYTHON013` | A module directly in `providers/`, a capability folder without `provider.py`, or another module or folder in it | Move it to `providers//provider.py`, `_helpers/`, `_classes/` or a role file | +| `SQBRPYTHON014` | `provider.py` holding a function, a public constant, another class or a second provider | Keep one public `Provider` subclass and private constants; move the rest to `_helpers/`, `_classes/` or `constants.py` | +| `SQBRPYTHON015` | An import of a provider's `_helpers/` or `_classes/` from outside its folder, or a provider importing `python/` outside `utils/` or anything in `orchestration/` | Import the provider through `provider.py` or its role files; share code through `python/utils/` | + +`SQBRPYTHON014` follows a class's bases through the project's modules, so a provider may subclass +`Provider` through an alias or a base class in `_classes/`. A class whose base comes from another +package is never reported as not being a provider. + ## Main modules and entry modules Some Rules apply only to a domain's main modules: files under a `main/` folder, and a unit's @@ -240,6 +270,14 @@ remediation. | `SQBRPYTHON011` | node-description-not-docstring | `python/` | yes | Tasks, assets and checks set `description=` without a docstring; loaders set neither | | `SQBRPYTHON012` | node-declared-in-unit | `python/` | yes | Nodes are declared in a unit's `node.py`, and checks in its `checks.py` | +### Providers + +| Code | Name | Files | Default | Checks | +|---|---|---|---|---| +| `SQBRPYTHON013` | provider-layout | All | yes | `providers/` holds capability folders with `provider.py`, role files, `_helpers/` and `_classes/` only | +| `SQBRPYTHON014` | provider-module-shape | All | yes | `provider.py` holds imports, private constants and one public `Provider` subclass | +| `SQBRPYTHON015` | provider-import-boundaries | All | yes | A provider's `_helpers/` and `_classes/` are private to it; providers import only `python/utils/` from `python/` and nothing from `orchestration/` | + ### Type annotations | Code | Name | Files | Default | Checks | diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py index e13cb556e..d86e8149c 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py @@ -3520,7 +3520,7 @@ def test_given_project_python_when_running_python_rules_then_findings_match_comp _PYTHON_ANNOTATED_ALERT, ), ), - expected_hits_and_misses=((0, 1), (0, 12), (11, 1), (0, 12), (13, 0)), + expected_hits_and_misses=((0, 1), (0, 14), (13, 1), (0, 14), (15, 0)), expected_python_cache=(False, True, True, True, True), ) ], diff --git a/website/src/content/docs/docs/concepts/python-nodes/providers.mdx b/website/src/content/docs/docs/concepts/python-nodes/providers.mdx index 770ff28ad..d8f140c41 100644 --- a/website/src/content/docs/docs/concepts/python-nodes/providers.mdx +++ b/website/src/content/docs/docs/concepts/python-nodes/providers.mdx @@ -7,10 +7,10 @@ Providers are shared runtime services that SQLBuild discovers, configures, and i ## Defining a provider -Create a Python file under `providers/` in your project. A provider is a class that subclasses `Provider` from `sqlbuild.providers`: +Create a `provider.py` in a capability folder under `providers/` (see [Project layout](#project-layout)). A provider is a class that subclasses `Provider` from `sqlbuild.providers`: ```python -# providers/warehouse_client.py +# providers/warehouse_client/provider.py from sqlbuild.providers import Provider class WarehouseClient(Provider): @@ -66,7 +66,7 @@ You can also type-annotate the parameter for IDE support and compile-time valida ```python from sqlbuild.tasks import task -from providers.warehouse_client import WarehouseClient +from providers.warehouse_client.provider import WarehouseClient @task def export_orders(ctx, warehouse_client: WarehouseClient): @@ -200,19 +200,30 @@ Setting values, defaults included, are never published. ## Project layout +Give each provider its own folder under `providers/`, named for the capability it provides: + ``` my-project/ providers/ - warehouse_client.py - slack_notifier.py + warehouse_client/ + provider.py # the Provider subclass + constants.py + _helpers/ # private to warehouse_client + slack_notifier/ + provider.py hooks/ sql/ record_notification.sql python/ notify.py python/ - loaders/ - load_orders.py - tasks/ - export_orders.py + orders/ + load_orders/ + node.py + main.py ``` + +Discovery also accepts `.py` files directly in `providers/`. A project that selects the +[Python Rules](/docs/concepts/rules/python-rules#laying-out-providers) reports them, along with +anything else in a provider folder besides `provider.py`, its role files, `_helpers/` and +`_classes/`. diff --git a/website/src/content/docs/docs/concepts/rules/python-rules.mdx b/website/src/content/docs/docs/concepts/rules/python-rules.mdx index a6d101d2e..5e2c3263b 100644 --- a/website/src/content/docs/docs/concepts/rules/python-rules.mdx +++ b/website/src/content/docs/docs/concepts/rules/python-rules.mdx @@ -145,6 +145,35 @@ A unit may import another unit's `node.py` only to name that node in `depends_on A factory's `node.py` holds one `@factory` function, and a check generated by a factory may stay in `node.py`. `@audit_factory` functions are not units and may sit anywhere under `python/`. +## Laying out providers + +`SQBRPYTHON013` to `SQBRPYTHON015` keep each provider in its own capability folder, with its +private code beside it: + +```text +providers/ + storefront_api/ + provider.py # the one public Provider subclass + models.py + _classes/ + page_cursor.py # a private collaborator class + carrier_api/ + provider.py +``` + +A capability folder may also hold `types.py`, `constants.py`, `exceptions.py`, `__init__.py` and +`_helpers/`. These are reported: + +| Code | Finding | Fix | +|---|---|---| +| `SQBRPYTHON013` | A module directly in `providers/`, a capability folder without `provider.py`, or another module or folder in it | Move it to `providers//provider.py`, `_helpers/`, `_classes/` or a role file | +| `SQBRPYTHON014` | `provider.py` holding a function, a public constant, another class or a second provider | Keep one public `Provider` subclass and private constants; move the rest to `_helpers/`, `_classes/` or `constants.py` | +| `SQBRPYTHON015` | An import of a provider's `_helpers/` or `_classes/` from outside its folder, or a provider importing `python/` outside `utils/` or anything in `orchestration/` | Import the provider through `provider.py` or its role files; share code through `python/utils/` | + +`SQBRPYTHON014` follows a class's bases through the project's modules, so a provider may subclass +`Provider` through an alias or a base class in `_classes/`. A class whose base comes from another +package is never reported as not being a provider. + ## Main modules and entry modules Some Rules apply only to a domain's main modules: files under a `main/` folder, and a unit's @@ -226,6 +255,14 @@ remediation. | `SQBRPYTHON011` | node-description-not-docstring | `python/` | yes | Tasks, assets and checks set `description=` without a docstring; loaders set neither | | `SQBRPYTHON012` | node-declared-in-unit | `python/` | yes | Nodes are declared in a unit's `node.py`, and checks in its `checks.py` | +### Providers + +| Code | Name | Files | Default | Checks | +|---|---|---|---|---| +| `SQBRPYTHON013` | provider-layout | All | yes | `providers/` holds capability folders with `provider.py`, role files, `_helpers/` and `_classes/` only | +| `SQBRPYTHON014` | provider-module-shape | All | yes | `provider.py` holds imports, private constants and one public `Provider` subclass | +| `SQBRPYTHON015` | provider-import-boundaries | All | yes | A provider's `_helpers/` and `_classes/` are private to it; providers import only `python/utils/` from `python/` and nothing from `orchestration/` | + ### Type annotations | Code | Name | Files | Default | Checks |