From 26c54fbfc8e1af824f95429938872681ced0caf3 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 26 Oct 2026 05:09:39 +0000 Subject: [PATCH 1/5] fix(rules): load project rule helpers in the rule test harness evaluate_rule copied only the rule's own file into its temporary project, so a rule importing a helper under rules/ failed to load in its tests. When the rule file belongs to a project's rules/ directory, the harness now copies that directory, as sqb compile sees it, keeping symlinks as links and leaving out bytecode and dot-directories such as tool caches. The rule under test is still written last, so a RuleCase file cannot replace it. An integration test runs a project's rule tests in a pytest subprocess under the prepend and importlib import modes, and the testing docs describe the pytest configuration importlib needs. --- .../custom-rules/testing-and-determinism.md | 15 +++ .../_helpers/engine/rule_harness.py | 43 ++++++- src/sqlbuild/rule_engine/constants.py | 2 + .../project_rule_tests/_test_types.py | 9 ++ .../test_project_rule_tests.py | 113 ++++++++++++++++++ .../main/evaluate_rule_project/_test_types.py | 11 ++ .../main/evaluate_rule_project/helpers.py | 45 +++++++ .../test_project_rules_directory.py | 64 ++++++++++ .../custom-rules/testing-and-determinism.mdx | 15 +++ 9 files changed, 313 insertions(+), 4 deletions(-) create mode 100644 tests/integration/src/sqlbuild/rule_engine/project_rule_tests/_test_types.py create mode 100644 tests/integration/src/sqlbuild/rule_engine/project_rule_tests/test_project_rule_tests.py create mode 100644 tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/_test_types.py create mode 100644 tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/helpers.py create mode 100644 tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/test_project_rules_directory.py diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/custom-rules/testing-and-determinism.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/custom-rules/testing-and-determinism.md index 5fafa49622..af117ddc32 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/custom-rules/testing-and-determinism.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/custom-rules/testing-and-determinism.md @@ -39,6 +39,21 @@ def test_given_missing_order_id_when_evaluating_then_reports_finding() -> None: `RuleCase.files` adds supporting project files. `RuleCase.config` supplies option values. Include passing cases, failing cases, near misses, and exact source-position assertions. +When the Rule's file lives in the project's `rules/` directory, the harness loads it with that whole +directory available, as `sqb compile` does, so a Rule that imports a shared helper such as +`from rules._helpers.naming import has_layer_prefix` runs under test too. + +The test imports the Rule from the `rules` package, so the project directory must be on the Python +module search path. Under pytest's default import mode, a `conftest.py` in the project directory +adds it. With `--import-mode=importlib`, pytest adds no test directories, so set `pythonpath` +instead: + +```toml +# pyproject.toml +[tool.pytest.ini_options] +pythonpath = ["."] +``` + Each case also checks determinism. The harness evaluates every subject once, then evaluates every subject again in reverse order in the same process. If the Rule's findings differ between the two passes, the case fails with `custom rule is not deterministic`. The usual cause is diff --git a/src/sqlbuild/rule_engine/_helpers/engine/rule_harness.py b/src/sqlbuild/rule_engine/_helpers/engine/rule_harness.py index 4acfa16047..a2324e8f54 100644 --- a/src/sqlbuild/rule_engine/_helpers/engine/rule_harness.py +++ b/src/sqlbuild/rule_engine/_helpers/engine/rule_harness.py @@ -3,11 +3,13 @@ from __future__ import annotations import inspect +import shutil from pathlib import Path from tempfile import TemporaryDirectory from sqlbuild._native import ( MIN_CUSTOM_RULE_TEST_CASES, + PROJECT_CONFIG_FILENAME, DiscoveredProjectInputs, Rule, RuleAssertionError, @@ -28,6 +30,7 @@ resolve_rule_signature, rule_from_value, ) +from sqlbuild.rule_engine.constants import RULES_COPY_IGNORED_PATTERNS, RULES_DIRECTORY_NAME from sqlbuild.rule_engine.types import RuleCheck @@ -80,6 +83,8 @@ def _write_rule_project(*, root: Path, rule: Rule, test_case: RuleCase) -> Path: config_lines.append("") project_config: str = "\n".join(config_lines) (root / "sqlbuild_project.toml").write_text(project_config, encoding="utf-8") + source_path: Path = _rule_source_path(rule=rule) + rule_path: Path = _copy_rules_directory(root=root, source_path=source_path) model_path: Path = root / test_case.path model_path.parent.mkdir(parents=True, exist_ok=True) model_path.write_text(test_case.source, encoding="utf-8") @@ -87,13 +92,43 @@ def _write_rule_project(*, root: Path, rule: Rule, test_case: RuleCase) -> Path: extra_path: Path = root / file.path extra_path.parent.mkdir(parents=True, exist_ok=True) extra_path.write_text(file.source, encoding="utf-8") + rule_path.parent.mkdir(parents=True, exist_ok=True) + rule_path.write_text(source_path.read_text(encoding="utf-8"), encoding="utf-8") + return rule_path + + +def _rule_source_path(*, rule: Rule) -> Path: source: str | None = inspect.getsourcefile(rule.check) if source is None or not Path(source).is_file(): raise RuleUsageError("evaluate_rule requires a rule declared in a source file") - rule_path: Path = root / "rules" / "custom.py" - rule_path.parent.mkdir(parents=True, exist_ok=True) - rule_path.write_text(Path(source).read_text(encoding="utf-8"), encoding="utf-8") - return rule_path + return Path(source).resolve() + + +def _copy_rules_directory(*, root: Path, source_path: Path) -> Path: + """Copy the rule's project `rules/` directory, helpers included; return where the rule goes.""" + + rules_dir: Path | None = _project_rules_dir(source_path=source_path) + if rules_dir is None: + return root / RULES_DIRECTORY_NAME / "custom.py" + _ = shutil.copytree( + rules_dir, + root / RULES_DIRECTORY_NAME, + symlinks=True, + ignore=shutil.ignore_patterns(*RULES_COPY_IGNORED_PATTERNS), + ) + return root / RULES_DIRECTORY_NAME / source_path.relative_to(rules_dir) + + +def _project_rules_dir(*, source_path: Path) -> Path | None: + """The `rules/` directory of the SQLBuild project the rule file belongs to, if it has one.""" + + for directory in source_path.parents: + if ( + directory.name == RULES_DIRECTORY_NAME + and (directory.parent / PROJECT_CONFIG_FILENAME).is_file() + ): + return directory + return None def _compile_rule_project(*, root: Path) -> CompiledProjectView: diff --git a/src/sqlbuild/rule_engine/constants.py b/src/sqlbuild/rule_engine/constants.py index e7275bc033..42a0ce8325 100644 --- a/src/sqlbuild/rule_engine/constants.py +++ b/src/sqlbuild/rule_engine/constants.py @@ -18,6 +18,8 @@ "SYSTEMROOT", ) RULE_DECORATOR_TOKEN: str = "@rule" +RULES_DIRECTORY_NAME: str = "rules" +RULES_COPY_IGNORED_PATTERNS: tuple[str, ...] = (".*", "__pycache__", "*.pyc") FACT_PROJECT_MODELS: str = "project.models" FACT_PROJECT_SOURCES: str = "project.sources" diff --git a/tests/integration/src/sqlbuild/rule_engine/project_rule_tests/_test_types.py b/tests/integration/src/sqlbuild/rule_engine/project_rule_tests/_test_types.py new file mode 100644 index 0000000000..cae6af4eb0 --- /dev/null +++ b/tests/integration/src/sqlbuild/rule_engine/project_rule_tests/_test_types.py @@ -0,0 +1,9 @@ +from dataclasses import dataclass + + +@dataclass(frozen=True) +class ProjectRuleTestsTestCase: + description: str + import_mode: str + rule_test_path: str + expected_summary: str = "2 passed" diff --git a/tests/integration/src/sqlbuild/rule_engine/project_rule_tests/test_project_rule_tests.py b/tests/integration/src/sqlbuild/rule_engine/project_rule_tests/test_project_rule_tests.py new file mode 100644 index 0000000000..6434a95ede --- /dev/null +++ b/tests/integration/src/sqlbuild/rule_engine/project_rule_tests/test_project_rule_tests.py @@ -0,0 +1,113 @@ +"""A project's own Rule tests load shared helpers under `rules/`, whatever pytest's import mode.""" + +import subprocess +import sys +from pathlib import Path + +import pytest + +from tests.integration.src.sqlbuild.rule_engine.project_rule_tests._test_types import ( + ProjectRuleTestsTestCase, +) + +_PROJECT_FILES: dict[str, str] = { + "sqlbuild_project.toml": 'name = "orders"\nadapter = "duckdb"\n', + "rules/_helpers/naming.py": ( + 'LAYER_PREFIXES: tuple[str, ...] = ("stg_", "int_", "fct_")\n\n\n' + "def has_layer_prefix(name: str) -> bool:\n" + " return name.startswith(LAYER_PREFIXES)\n" + ), + "rules/model_names.py": ( + "from rules._helpers.naming import has_layer_prefix\n" + "from sqlbuild.rules import Finding, Model, RuleContext, rule\n\n\n" + "@rule(\n" + ' code="XSQBRNAME001",\n' + ' message="Model names start with their layer",\n' + ' remediation="Rename the model with a stg_, int_ or fct_ prefix.",\n' + ")\n" + "def model_names(*, model: Model, ctx: RuleContext) -> list[Finding]:\n" + " if has_layer_prefix(model.name):\n" + " return []\n" + " return [ctx.finding(subject=model, line=1)]\n" + ), +} +_RULE_TEST: str = ( + "from rules.model_names import model_names\n" + "from sqlbuild.rules.testing import RuleCase, evaluate_rule\n\n\n" + "def test_given_unprefixed_model_when_evaluating_then_reports_finding() -> None:\n" + " result = evaluate_rule(\n" + " rule=model_names,\n" + " test_case=RuleCase(\n" + ' description="unprefixed orders model",\n' + " source='MODEL (description \"Orders\");\\nSELECT 1 AS order_id\\n',\n" + ' path="models/orders.sql",\n' + " expected_finding_count=1,\n" + " ),\n" + " )\n" + " assert result.finding_count == 1\n\n\n" + "def test_given_prefixed_model_when_evaluating_then_reports_nothing() -> None:\n" + " result = evaluate_rule(\n" + " rule=model_names,\n" + " test_case=RuleCase(\n" + ' description="staged orders model",\n' + " source='MODEL (description \"Orders\");\\nSELECT 1 AS order_id\\n',\n" + ' path="models/stg_orders.sql",\n' + " expected_finding_count=0,\n" + " ),\n" + " )\n" + " assert result.finding_count == 0\n" +) + + +@pytest.mark.parametrize( + "test_case", + ( + ProjectRuleTestsTestCase( + description="prepend import mode", + import_mode="prepend", + rule_test_path="tests/rules/test_model_names.py", + ), + ProjectRuleTestsTestCase( + description="importlib import mode", + import_mode="importlib", + rule_test_path="tests/rules/test_model_names.py", + ), + ProjectRuleTestsTestCase( + description="importlib import mode inside the rules package", + import_mode="importlib", + rule_test_path="rules/tests/test_model_names.py", + ), + ), + ids=lambda case: case.description, +) +def test_given_rule_importing_a_shared_helper_when_running_its_tests_then_they_pass( + test_case: ProjectRuleTestsTestCase, tmp_path: Path +) -> None: + for relative_path, contents in {**_PROJECT_FILES, test_case.rule_test_path: _RULE_TEST}.items(): + path: Path = tmp_path / relative_path + path.parent.mkdir(parents=True, exist_ok=True) + _ = path.write_text(contents, encoding="utf-8") + + result: subprocess.CompletedProcess[str] = subprocess.run( + [ + str(Path(sys.executable).with_name("pytest")), + "-q", + "-p", + "no:cacheprovider", + f"--import-mode={test_case.import_mode}", + "-o", + "pythonpath=.", + test_case.rule_test_path, + ], + cwd=tmp_path, + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stdout + result.stderr + assert test_case.expected_summary in result.stdout + + +if __name__ == "__main__": + raise SystemExit(pytest.main([__file__, "-n", "auto", "--dist", "loadfile"])) diff --git a/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/_test_types.py b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/_test_types.py new file mode 100644 index 0000000000..7eebc05a62 --- /dev/null +++ b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/_test_types.py @@ -0,0 +1,11 @@ +"""Test case declarations for project rules under the public harness.""" + +from dataclasses import dataclass + + +@dataclass(frozen=True) +class ProjectRulesDirectoryTestCase: + description: str + extra_project_files: dict[str, str] + case_files: tuple[tuple[str, str], ...] + expected_finding_count: int diff --git a/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/helpers.py b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/helpers.py new file mode 100644 index 0000000000..a3e80efcc9 --- /dev/null +++ b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/helpers.py @@ -0,0 +1,45 @@ +"""A project with a `rules/` package whose rule imports a shared helper.""" + +import importlib.util +import sys +from importlib.machinery import ModuleSpec +from pathlib import Path +from types import ModuleType + +PROJECT_RULE_FILES: dict[str, str] = { + "sqlbuild_project.toml": 'name = "orders"\nadapter = "duckdb"\n', + "rules/_helpers/naming.py": ( + 'def has_layer_prefix(name: str) -> bool:\n return name.startswith(("stg_", "fct_"))\n' + ), + "rules/model_names.py": ( + "from rules._helpers.naming import has_layer_prefix\n" + "from sqlbuild.rules import Finding, Model, RuleContext, rule\n\n\n" + "@rule(\n" + ' code="XSQBRNAME001",\n' + ' message="Model names start with their layer",\n' + ' remediation="Rename the model with a stg_ or fct_ prefix.",\n' + ")\n" + "def model_names(*, model: Model, ctx: RuleContext) -> list[Finding]:\n" + " return [ctx.finding(subject=model)] * int(not has_layer_prefix(model.name))\n" + ), +} + + +def load_project_rule(*, root: Path, files: dict[str, str]) -> object: + """Write a project with a `rules/` package and import its `model_names` rule from the file.""" + + for relative_path, contents in files.items(): + path: Path = root / relative_path + path.parent.mkdir(parents=True, exist_ok=True) + _ = path.write_text(contents, encoding="utf-8") + sys.path.insert(0, str(root)) + try: + spec: ModuleSpec | None = importlib.util.spec_from_file_location( + "rules.model_names", root / "rules" / "model_names.py" + ) + assert spec is not None and spec.loader is not None + module: ModuleType = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + finally: + sys.path.remove(str(root)) + return module.model_names diff --git a/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/test_project_rules_directory.py b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/test_project_rules_directory.py new file mode 100644 index 0000000000..55de77eaee --- /dev/null +++ b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule_project/test_project_rules_directory.py @@ -0,0 +1,64 @@ +"""The harness copies a project rule's `rules/` directory and writes the rule under test last.""" + +from pathlib import Path +from typing import cast + +import pytest + +from sqlbuild.rule_engine.types import RuleCheck +from sqlbuild.rules.testing import RuleCase, RuleFile, RuleResult, evaluate_rule +from tests.unit.src.sqlbuild.rule_engine.main.evaluate_rule_project._test_types import ( + ProjectRulesDirectoryTestCase, +) +from tests.unit.src.sqlbuild.rule_engine.main.evaluate_rule_project.helpers import ( + PROJECT_RULE_FILES, + load_project_rule, +) + + +@pytest.mark.parametrize( + "test_case", + ( + ProjectRulesDirectoryTestCase( + description="a case file at the rule's path does not replace the rule", + extra_project_files={}, + case_files=(("rules/model_names.py", "raise RuntimeError('replaced')\n"),), + expected_finding_count=1, + ), + ProjectRulesDirectoryTestCase( + description="tool caches and dot-directories under rules/ are not copied", + extra_project_files={ + "rules/.scratch/broken.py": "@rule(\n", + "rules/.pytest_cache/broken.py": "@rule(\n", + "rules/__pycache__/broken.py": "@rule(\n", + }, + case_files=(), + expected_finding_count=1, + ), + ), + ids=lambda case: case.description, +) +def test_given_project_rule_when_evaluating_then_uses_its_own_rules_directory( + test_case: ProjectRulesDirectoryTestCase, tmp_path: Path +) -> None: + model_names: RuleCheck = cast( + RuleCheck, + load_project_rule( + root=tmp_path, files={**PROJECT_RULE_FILES, **test_case.extra_project_files} + ), + ) + + result: RuleResult = evaluate_rule( + rule=model_names, + test_case=RuleCase( + description=test_case.description, + source='MODEL (description "Orders");\nSELECT 1 AS order_id\n', + path="models/orders.sql", + files=tuple( + RuleFile(path=path, source=source) for path, source in test_case.case_files + ), + expected_finding_count=test_case.expected_finding_count, + ), + ) + + assert result.finding_count == test_case.expected_finding_count diff --git a/website/src/content/docs/docs/concepts/rules/custom-rules/testing-and-determinism.mdx b/website/src/content/docs/docs/concepts/rules/custom-rules/testing-and-determinism.mdx index 972fb6bd07..8567dc9569 100644 --- a/website/src/content/docs/docs/concepts/rules/custom-rules/testing-and-determinism.mdx +++ b/website/src/content/docs/docs/concepts/rules/custom-rules/testing-and-determinism.mdx @@ -29,6 +29,21 @@ def test_given_missing_order_id_when_evaluating_then_reports_finding() -> None: `RuleCase.files` adds supporting project files. `RuleCase.config` supplies option values. Include passing cases, failing cases, near misses, and exact source-position assertions. +When the Rule's file lives in the project's `rules/` directory, the harness loads it with that whole +directory available, as `sqb compile` does, so a Rule that imports a shared helper such as +`from rules._helpers.naming import has_layer_prefix` runs under test too. + +The test imports the Rule from the `rules` package, so the project directory must be on the Python +module search path. Under pytest's default import mode, a `conftest.py` in the project directory +adds it. With `--import-mode=importlib`, pytest adds no test directories, so set `pythonpath` +instead: + +```toml +# pyproject.toml +[tool.pytest.ini_options] +pythonpath = ["."] +``` + Each case also checks determinism. The harness evaluates every subject once, then evaluates every subject again in reverse order in the same process. If the Rule's findings differ between the two passes, the case fails with `custom rule is not deterministic`. The usual cause is From ef24454769aec96738199c04e7d9860fdbcdda3e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 26 Oct 2026 05:09:51 +0000 Subject: [PATCH 2/5] feat(contracts): check enum-typed columns against string literals at compile time An enum-typed column of an enforced contract was only checked by its generated accepted_values audit, so a union branch filling it with a string that is not an enum member compiled cleanly and failed after the build. sqb compile now reports K013 for each constant string literal that fills such a column, written directly, in any UNION branch, or passed through a CTE or derived table, naming the value, the model and the enum and pointing at the projected literal. The check never fires where the audit could pass: it stops at a passthrough read through a filter, join or row limit, at EXCEPT and INTERSECT, and at a positional read of a by-name union, and it is skipped on SQL Server, whose default collation ignores case and trailing spaces. Computed values stay with the audit. Rules can read each declared column's enum with ctx.columns.enum_types(model); declared column types are unchanged. The new fact is tracked for the Rules cache and invalidated by header column edits. --- .../src/contracts/_helpers/enum_literals.rs | 377 ++++++++++++++++++ .../src/contracts/_helpers/mod.rs | 1 + .../src/contracts/constants.rs | 3 + .../contracts/main/enum_literal_violations.rs | 64 +++ .../src/contracts/main/mod.rs | 1 + .../sqlbuild-analysis/src/contracts/models.rs | 44 ++ .../sqlbuild-analysis/src/contracts/tests.rs | 2 + .../src/contracts/tests/helpers.rs | 24 +- .../tests/test_enum_literal_violations.rs | 219 ++++++++++ .../src/contracts/tests/test_types.rs | 23 ++ .../header_changes/main/custom_rule_reads.rs | 1 + .../tests/test_custom_rule_reads.rs | 6 + .../contracts/contract_diagnostics.rs | 128 +++++- .../_helpers/rules/custom_host_payload.rs | 21 +- .../bindings/_helpers/rules/fact_replay.rs | 24 +- .../_helpers/analysis_corpus/failure_cases.py | 20 + .../sqlbuild/references/docs/cli/compile.md | 1 + .../docs/concepts/enums/model-contracts.md | 34 ++ .../rules/custom-rules/rule-context.md | 12 + .../rule_engine/_helpers/host/host_payload.py | 4 + .../_helpers/host/tracked_facts.py | 5 + .../rule_engine/classes/column_facts.py | 3 + src/sqlbuild/rule_engine/constants.py | 1 + src/sqlbuild/rule_engine/types.py | 3 + .../sqlbuild/cli/commands/main/_test_types.py | 9 + .../src/sqlbuild/cli/commands/main/helpers.py | 25 ++ .../main/test_compile_enum_literals.py | 143 +++++++ .../fact_digests/test_fact_digests.py | 15 + .../rule_engine/host_payload/helpers.py | 4 + .../host_payload/test_host_payload_facts.py | 1 + .../main/evaluate_rule/_test_types.py | 7 + .../rule_engine/main/evaluate_rule/helpers.py | 12 + .../main/evaluate_rule/test_evaluate_rule.py | 45 ++- website/src/content/docs/docs/cli/compile.mdx | 1 + .../docs/concepts/enums/model-contracts.mdx | 34 ++ .../rules/custom-rules/rule-context.mdx | 12 + 36 files changed, 1320 insertions(+), 9 deletions(-) create mode 100644 crates/sqlbuild-analysis/src/contracts/_helpers/enum_literals.rs create mode 100644 crates/sqlbuild-analysis/src/contracts/main/enum_literal_violations.rs create mode 100644 crates/sqlbuild-analysis/src/contracts/tests/test_enum_literal_violations.rs create mode 100644 tests/integration/src/sqlbuild/cli/commands/main/test_compile_enum_literals.py diff --git a/crates/sqlbuild-analysis/src/contracts/_helpers/enum_literals.rs b/crates/sqlbuild-analysis/src/contracts/_helpers/enum_literals.rs new file mode 100644 index 0000000000..adaf9e1f93 --- /dev/null +++ b/crates/sqlbuild-analysis/src/contracts/_helpers/enum_literals.rs @@ -0,0 +1,377 @@ +//! Constant string literals that reach an output column, and where the authored file writes them. + +use polyglot_sql::expressions::{Cte, Literal, Select, With}; +use polyglot_sql::tokens::{Token, TokenType}; +use polyglot_sql::{Dialect, DialectType, Expression}; + +use crate::contracts::models::LiteralLocation; + +/// How deep the walk follows set operations and passthrough columns before it stops. +const MAX_DEPTH: usize = 64; + +/// The output column a walk follows: by name, or by position in a set-operation branch. +#[derive(Clone, Copy)] +enum Target<'n> { + Name(&'n str), + Position(usize), +} + +/// The CTEs a query can read, in definition order, and how deep the walk has gone. +#[derive(Clone)] +struct Scope<'a> { + ctes: Vec<&'a Cte>, + depth: usize, +} + +impl<'a> Scope<'a> { + /// The scope one step deeper, with `with`'s CTEs visible unless they are recursive. + fn within(&self, with: Option<&'a With>) -> Self { + let mut ctes: Vec<&'a Cte> = self.ctes.clone(); + if let Some(with) = with + && !with.recursive + { + ctes.extend(with.ctes.iter()); + } + Self { + ctes, + depth: self.depth + 1, + } + } + + /// The scope a CTE body sees: the CTEs defined before it. + fn before(&self, index: usize) -> Self { + Self { + ctes: self.ctes[..index].to_vec(), + depth: self.depth + 1, + } + } +} + +/// The literals found so far. +#[derive(Default)] +struct LiteralWalk { + found: Vec, +} + +/// Every constant string literal that fills output column `column` of `query`, in query order. +pub(crate) fn output_literals(query: &Expression, column: &str) -> Vec { + let mut walk: LiteralWalk = LiteralWalk::default(); + let scope: Scope<'_> = Scope { + ctes: Vec::new(), + depth: 0, + }; + walk.query(query, Target::Name(column), &scope); + walk.found +} + +/// The parsed statement of `sql` under `dialect`, when it is one query. +pub(crate) fn parsed_query(sql: &str, dialect: &str) -> Option { + let Ok(dialect) = dialect.parse::() else { + return None; + }; + let Ok(mut statements) = polyglot_sql::parse(sql, dialect) else { + return None; + }; + (statements.len() == 1).then(|| statements.pop()).flatten() +} + +/// The first string literal with text `value` in `query`, located in `authored`, the whole file. +pub(crate) fn literal_location( + (authored, query): (&str, &str), + value: &str, + dialect: &str, +) -> Option { + let dialect: DialectType = dialect + .parse::() + .unwrap_or(DialectType::Generic); + let Ok(tokens) = Dialect::get(dialect).tokenize(query) else { + return None; + }; + let matches = |index: &usize| { + tokens[*index].token_type == TokenType::String && tokens[*index].text == value + }; + let index: usize = (0..tokens.len()) + .filter(matches) + .find(|index| select_item(&tokens, *index)) + .or_else(|| (0..tokens.len()).find(matches))?; + let token = &tokens[index]; + let query_start: usize = authored.find(query)?; + let offset: usize = authored[..query_start].chars().count(); + let (line, column) = line_column(authored, offset + token.span.start); + let (end_line, end_column) = line_column(authored, offset + token.span.end); + Some(LiteralLocation { + line, + column, + end_line, + end_column, + }) +} + +/// Whether the token at `index` stands alone as a select-list item, as a projected literal does. +fn select_item(tokens: &[Token], index: usize) -> bool { + let opens: bool = index.checked_sub(1).is_some_and(|previous| { + matches!( + tokens[previous].token_type, + TokenType::Select | TokenType::Distinct | TokenType::All | TokenType::Comma + ) + }); + let closes: bool = tokens.get(index + 1).is_none_or(|next| { + matches!( + next.token_type, + TokenType::As + | TokenType::Comma + | TokenType::From + | TokenType::Union + | TokenType::Semicolon + | TokenType::RParen + | TokenType::Identifier + | TokenType::QuotedIdentifier + | TokenType::Var + ) + }); + opens && closes +} + +/// The one-based line and column of character `offset` in `text`. +fn line_column(text: &str, offset: usize) -> (usize, usize) { + let mut line: usize = 1; + let mut column: usize = 1; + for character in text.chars().take(offset) { + if character == '\n' { + line += 1; + column = 1; + } else { + column += 1; + } + } + (line, column) +} + +/// The position of output column `name` in the leftmost branch of `query`, before any star. +fn output_position(query: &Expression, name: &str, depth: usize) -> Option { + if depth > MAX_DEPTH { + return None; + } + match query { + Expression::Select(select) => { + for (position, projection) in select.expressions.iter().enumerate() { + match projection { + Expression::Star(_) => return None, + projection + if output_name(projection).is_some_and(|known| same(known, name)) => + { + return Some(position); + } + _ => {} + } + } + None + } + Expression::Union(union) => output_position(&union.left, name, depth + 1), + Expression::Intersect(intersect) => output_position(&intersect.left, name, depth + 1), + Expression::Subquery(subquery) if subquery.column_aliases.is_empty() => { + output_position(&subquery.this, name, depth + 1) + } + Expression::Paren(paren) => output_position(&paren.this, name, depth + 1), + _ => None, + } +} + +/// The name a projection outputs, when it names one. +fn output_name(projection: &Expression) -> Option<&str> { + match projection { + Expression::Alias(alias) => Some(alias.alias.name.as_str()), + Expression::Column(column) => Some(column.name.name.as_str()), + _ => None, + } +} + +fn same(left: &str, right: &str) -> bool { + left.eq_ignore_ascii_case(right) +} + +/// The one `FROM` item a select passes every row of through, when `qualifier` names it or is absent. +fn passthrough_source<'a>(select: &'a Select, qualifier: Option<&str>) -> Option<&'a Expression> { + if !keeps_every_row(select) { + return None; + } + let [source] = select.from.as_ref()?.expressions.as_slice() else { + return None; + }; + match qualifier { + Some(qualifier) if !source_alias(source).is_some_and(|alias| same(alias, qualifier)) => { + None + } + _ => Some(source), + } +} + +/// Whether a select outputs a row for every row it reads: no filter, join, limit or sampling. +fn keeps_every_row(select: &Select) -> bool { + keeps_any_row(select) + && select.joins.is_empty() + && select.lateral_views.is_empty() + && select.prewhere.is_none() + && select.where_clause.is_none() + && select.having.is_none() + && select.qualify.is_none() + && select.distinct_on.is_none() + && select.connect.is_none() +} + +/// Whether a select has no row limit or sampling, which could leave a literal with no rows. +fn keeps_any_row(select: &Select) -> bool { + select.limit.is_none() + && select.offset.is_none() + && select.limit_by.is_none() + && select.fetch.is_none() + && select.top.is_none() + && select.sample.is_none() +} + +/// The name a `FROM` item is read by: its alias, or a table's own name. +fn source_alias(source: &Expression) -> Option<&str> { + match source { + Expression::Table(table) => Some( + table + .alias + .as_ref() + .map_or(table.name.name.as_str(), |alias| alias.name.as_str()), + ), + Expression::Subquery(subquery) => subquery.alias.as_ref().map(|alias| alias.name.as_str()), + _ => None, + } +} + +impl LiteralWalk { + fn query<'a>(&mut self, query: &'a Expression, target: Target<'_>, scope: &Scope<'a>) { + if scope.depth > MAX_DEPTH { + return; + } + match query { + Expression::Select(select) => { + self.select(select, target, &scope.within(select.with.as_ref())) + } + Expression::Union(union) if union.limit.is_none() && union.offset.is_none() => { + let scope: Scope<'a> = scope.within(union.with.as_ref()); + let target: Option> = match target { + Target::Name(_) if union.by_name => Some(target), + Target::Position(_) if union.by_name => None, + Target::Name(name) => { + output_position(&union.left, name, 0).map(Target::Position) + } + Target::Position(_) => Some(target), + }; + if let Some(target) = target { + self.query(&union.left, target, &scope); + self.query(&union.right, target, &scope); + } + } + Expression::Subquery(subquery) + if subquery.column_aliases.is_empty() + && subquery.limit.is_none() + && subquery.offset.is_none() => + { + self.query(&subquery.this, target, &scope.within(None)); + } + Expression::Paren(paren) => self.query(&paren.this, target, &scope.within(None)), + _ => {} + } + } + + fn select<'a>(&mut self, select: &'a Select, target: Target<'_>, scope: &Scope<'a>) { + if !keeps_any_row(select) { + return; + } + match target { + Target::Position(position) => { + let starred: bool = select + .expressions + .iter() + .take(position + 1) + .any(|projection| matches!(projection, Expression::Star(_))); + if let Some(projection) = select.expressions.get(position) + && !starred + { + self.expression(projection, select, scope); + } + } + Target::Name(name) => { + let named: Vec<&Expression> = select + .expressions + .iter() + .filter(|projection| { + output_name(projection).is_some_and(|known| same(known, name)) + }) + .collect(); + if let [projection] = named.as_slice() { + self.expression(projection, select, scope); + return; + } + if !named.is_empty() { + return; + } + let [Expression::Star(star)] = select.expressions.as_slice() else { + return; + }; + if star.except.is_some() || star.replace.is_some() || star.rename.is_some() { + return; + } + let qualifier: Option<&str> = star.table.as_ref().map(|table| table.name.as_str()); + if let Some(source) = passthrough_source(select, qualifier) { + self.source(source, name, scope); + } + } + } + } + + fn expression<'a>( + &mut self, + expression: &'a Expression, + select: &'a Select, + scope: &Scope<'a>, + ) { + match expression { + Expression::Alias(alias) => self.expression(&alias.this, select, scope), + Expression::Paren(paren) => self.expression(&paren.this, select, scope), + Expression::Literal(literal) => { + if let Literal::String(value) = literal.as_ref() { + self.found.push(value.clone()); + } + } + Expression::Column(column) => { + let qualifier: Option<&str> = + column.table.as_ref().map(|table| table.name.as_str()); + if let Some(source) = passthrough_source(select, qualifier) { + self.source(source, &column.name.name, scope); + } + } + _ => {} + } + } + + /// The literals filling column `name` of a CTE or derived table that a `FROM` item reads. + fn source<'a>(&mut self, source: &'a Expression, name: &str, scope: &Scope<'a>) { + match source { + Expression::Table(table) if table.schema.is_none() && table.catalog.is_none() => { + let Some(index) = scope + .ctes + .iter() + .rposition(|cte| same(&cte.alias.name, &table.name.name)) + else { + return; + }; + let cte: &Cte = scope.ctes[index]; + if table.column_aliases.is_empty() && cte.columns.is_empty() { + self.query(&cte.this, Target::Name(name), &scope.before(index)); + } + } + Expression::Subquery(subquery) + if subquery.column_aliases.is_empty() && !subquery.lateral => + { + self.query(&subquery.this, Target::Name(name), &scope.within(None)); + } + _ => {} + } + } +} diff --git a/crates/sqlbuild-analysis/src/contracts/_helpers/mod.rs b/crates/sqlbuild-analysis/src/contracts/_helpers/mod.rs index 32e0493cc5..21ef383460 100644 --- a/crates/sqlbuild-analysis/src/contracts/_helpers/mod.rs +++ b/crates/sqlbuild-analysis/src/contracts/_helpers/mod.rs @@ -1,5 +1,6 @@ pub(crate) mod columns; pub(crate) mod dynamic; +pub(crate) mod enum_literals; pub(crate) mod promotion; pub(crate) mod setting_help; pub(crate) mod type_comparison; diff --git a/crates/sqlbuild-analysis/src/contracts/constants.rs b/crates/sqlbuild-analysis/src/contracts/constants.rs index 35d840309d..c1edad4679 100644 --- a/crates/sqlbuild-analysis/src/contracts/constants.rs +++ b/crates/sqlbuild-analysis/src/contracts/constants.rs @@ -8,6 +8,9 @@ pub(crate) const EXTRA_COLUMN_CODE: &str = "K005"; pub(crate) const MISSING_DECLARATIONS_CODE: &str = "K006"; pub(crate) const DYNAMIC_OUTPUT_NOT_PROVEN_CODE: &str = "K011"; pub(crate) const PROMOTION_CONFLICT_CODE: &str = "K011"; +pub(crate) const ENUM_LITERAL_CODE: &str = "K013"; +/// Dialects whose default string comparison ignores case or trailing spaces, so K013 is skipped. +pub(crate) const CASE_INSENSITIVE_DIALECTS: [&str; 1] = ["tsql"]; pub const CONTRACT_ENFORCED: &str = "enforced"; pub(crate) const CONTRACT_NONE: &str = "none"; diff --git a/crates/sqlbuild-analysis/src/contracts/main/enum_literal_violations.rs b/crates/sqlbuild-analysis/src/contracts/main/enum_literal_violations.rs new file mode 100644 index 0000000000..fcc9bb2ab2 --- /dev/null +++ b/crates/sqlbuild-analysis/src/contracts/main/enum_literal_violations.rs @@ -0,0 +1,64 @@ +//! String literals that fill an enum-typed output column but are not members of the enum. + +use crate::contracts::_helpers::enum_literals::{literal_location, output_literals, parsed_query}; +use crate::contracts::constants::{CASE_INSENSITIVE_DIALECTS, ENUM_LITERAL_CODE}; +use crate::contracts::models::{EnumColumn, EnumLiteralModel, EnumLiteralViolation}; + +/// One violation per column and distinct constant literal, in column order and then query order. +#[must_use] +pub fn enum_literal_violations(model: &EnumLiteralModel<'_>) -> Vec { + if model.columns.is_empty() || CASE_INSENSITIVE_DIALECTS.contains(&model.dialect) { + return Vec::new(); + } + let Some(query) = parsed_query(model.query_sql, model.dialect) else { + return Vec::new(); + }; + let mut violations: Vec = Vec::new(); + for column in &model.columns { + let mut reported: Vec = Vec::new(); + for value in output_literals(&query, &column.name) { + if column.members.contains(&value) || reported.contains(&value) { + continue; + } + violations.push(violation(model, column, &value)); + reported.push(value); + } + } + violations +} + +fn violation( + model: &EnumLiteralModel<'_>, + column: &EnumColumn, + value: &str, +) -> EnumLiteralViolation { + let members: Vec = column.members.iter().map(|member| quoted(member)).collect(); + EnumLiteralViolation { + code: ENUM_LITERAL_CODE, + column_name: column.name.clone(), + value: value.to_owned(), + message: format!( + "value {} in column '{}' of model '{}' is not a member of enum {}", + quoted(value), + column.name, + model.name, + column.enum_name + ), + help: format!( + "add {} to enum {} or use one of its members: {}", + quoted(value), + column.enum_name, + members.join(", ") + ), + location: literal_location( + (model.authored_sql, model.authored_query_sql), + value, + model.dialect, + ), + } +} + +/// `value` as a SQL string literal. +fn quoted(value: &str) -> String { + format!("'{}'", value.replace('\'', "''")) +} diff --git a/crates/sqlbuild-analysis/src/contracts/main/mod.rs b/crates/sqlbuild-analysis/src/contracts/main/mod.rs index 2cea672288..23d64b30fb 100644 --- a/crates/sqlbuild-analysis/src/contracts/main/mod.rs +++ b/crates/sqlbuild-analysis/src/contracts/main/mod.rs @@ -1,3 +1,4 @@ +pub mod enum_literal_violations; pub mod evaluate_model_contracts; pub mod join_helps; pub mod promotion_conflicts; diff --git a/crates/sqlbuild-analysis/src/contracts/models.rs b/crates/sqlbuild-analysis/src/contracts/models.rs index 868b8904eb..a4cd413255 100644 --- a/crates/sqlbuild-analysis/src/contracts/models.rs +++ b/crates/sqlbuild-analysis/src/contracts/models.rs @@ -165,3 +165,47 @@ pub struct PromotionConflict { pub message: String, pub help: String, } + +/// One model whose enum-typed output columns are checked against the literals that fill them. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct EnumLiteralModel<'a> { + pub name: &'a str, + /// The dialect name of the project's adapter. + pub dialect: &'a str, + /// SQL after variable, declaration and macro expansion. + pub query_sql: &'a str, + /// The whole authored model file. + pub authored_sql: &'a str, + /// The authored query below the `MODEL (...)` header. + pub authored_query_sql: &'a str, + pub columns: Vec, +} + +/// One output column declared with an enum type, and the enum's string members. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct EnumColumn { + pub name: String, + pub enum_name: String, + pub members: Vec, +} + +/// A constant string literal that fills an enum-typed output column but is not a member. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct EnumLiteralViolation { + pub code: &'static str, + pub column_name: String, + pub value: String, + pub message: String, + pub help: String, + /// The literal in the authored model file, when it is written there. + pub location: Option, +} + +/// One-based line and column of a literal in the authored file; the end is exclusive. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct LiteralLocation { + pub line: usize, + pub column: usize, + pub end_line: usize, + pub end_column: usize, +} diff --git a/crates/sqlbuild-analysis/src/contracts/tests.rs b/crates/sqlbuild-analysis/src/contracts/tests.rs index b4e1f8772a..5c48604400 100644 --- a/crates/sqlbuild-analysis/src/contracts/tests.rs +++ b/crates/sqlbuild-analysis/src/contracts/tests.rs @@ -1,3 +1,5 @@ +#[path = "tests/test_enum_literal_violations.rs"] +mod enum_literal_violations; #[path = "tests/test_evaluate_model_contracts.rs"] mod evaluate_model_contracts; #[path = "tests/helpers.rs"] diff --git a/crates/sqlbuild-analysis/src/contracts/tests/helpers.rs b/crates/sqlbuild-analysis/src/contracts/tests/helpers.rs index bb4ff5d21a..51f93d6e1f 100644 --- a/crates/sqlbuild-analysis/src/contracts/tests/helpers.rs +++ b/crates/sqlbuild-analysis/src/contracts/tests/helpers.rs @@ -1,6 +1,7 @@ use crate::contracts::models::{ ContractDiagnostic, ContractModel, ContractSchema, DeclaredColumn, DeclaredColumnFamily, - DynamicContractProof, InferredOutputColumn, PromotionModel, PromotionRequest, + DynamicContractProof, EnumColumn, EnumLiteralViolation, InferredOutputColumn, PromotionModel, + PromotionRequest, }; pub(crate) fn model(contract: Option<&str>, schema: Option) -> ContractModel { @@ -142,3 +143,24 @@ pub(crate) fn promotion_request( ], } } + +/// The header of every authored model the enum literal tests check. +pub(crate) const ENUM_HEADER: &str = "MODEL (description 'Orders');\n"; + +/// An order status column of a two-member string enum. +pub(crate) fn status_column() -> EnumColumn { + EnumColumn { + name: "status".to_owned(), + enum_name: "order_status".to_owned(), + members: vec!["open".to_owned(), "closed".to_owned()], + } +} + +/// One violation as `column value line:column`, `-` when it has no location. +pub(crate) fn enum_violation_line(violation: &EnumLiteralViolation) -> String { + let location: String = violation.location.map_or_else( + || "-".to_owned(), + |location| format!("{}:{}", location.line, location.column), + ); + format!("{} {} {location}", violation.column_name, violation.value) +} diff --git a/crates/sqlbuild-analysis/src/contracts/tests/test_enum_literal_violations.rs b/crates/sqlbuild-analysis/src/contracts/tests/test_enum_literal_violations.rs new file mode 100644 index 0000000000..323061b32d --- /dev/null +++ b/crates/sqlbuild-analysis/src/contracts/tests/test_enum_literal_violations.rs @@ -0,0 +1,219 @@ +use crate::contracts::main::enum_literal_violations::enum_literal_violations; +use crate::contracts::models::{EnumLiteralModel, EnumLiteralViolation}; +use crate::contracts::tests::helpers::{ENUM_HEADER, enum_violation_line, status_column}; +use crate::contracts::tests::test_types::{ + EnumLiteralDialectTestCase, EnumLiteralMessageTestCase, EnumLiteralTestCase, +}; + +#[test] +fn given_enum_column_when_checking_literals_then_reports_non_members() { + let test_cases = [ + EnumLiteralTestCase { + description: "a literal written directly as the column", + authored: "MODEL (description 'Orders');\nSELECT 1 AS order_id, 'pending' AS status\n", + expanded: None, + expected_lines: &["status pending 2:23"], + }, + EnumLiteralTestCase { + description: "a member literal is accepted", + authored: "MODEL (description 'Orders');\nSELECT 1 AS order_id, 'open' AS status\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "every UNION ALL branch, matched by position", + authored: "MODEL (description 'Orders');\nSELECT 'open' AS status, 1 AS order_id\nUNION ALL\nSELECT 'pending', 2\nUNION ALL\nSELECT 'void' AS other, 3\n", + expanded: None, + expected_lines: &["status pending 4:8", "status void 6:8"], + }, + EnumLiteralTestCase { + description: "a literal passed through CTEs and a derived table", + authored: "MODEL (description 'Orders');\nWITH raw AS (SELECT 'pending' AS status),\nrenamed AS (SELECT r.status FROM raw AS r)\nSELECT status FROM (SELECT * FROM renamed) AS d\n", + expanded: None, + expected_lines: &["status pending 2:21"], + }, + EnumLiteralTestCase { + description: "a union inside a CTE read through a star", + authored: "MODEL (description 'Orders');\nWITH unioned AS (\n SELECT 'open' AS status FROM a\n UNION ALL\n SELECT 'held' AS status FROM b\n)\nSELECT * FROM unioned\n", + expanded: None, + expected_lines: &["status held 5:10"], + }, + EnumLiteralTestCase { + description: "a repeated value is reported once", + authored: "MODEL (description 'Orders');\nSELECT 'held' AS status\nUNION ALL\nSELECT 'held'\n", + expanded: None, + expected_lines: &["status held 2:8"], + }, + EnumLiteralTestCase { + description: "computed values are left to the audit", + authored: "MODEL (description 'Orders');\nWITH raw AS (SELECT LOWER('PENDING') AS status, 'x' AS code)\nSELECT CASE WHEN code = 'x' THEN 'held' ELSE status END AS status FROM raw\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "an ambiguous unqualified read across a join is not followed", + authored: "MODEL (description 'Orders');\nWITH a AS (SELECT 'held' AS status), b AS (SELECT 1 AS id)\nSELECT status FROM a CROSS JOIN b\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "an EXCEPT may remove the literal's rows", + authored: "MODEL (description 'Orders');\nSELECT 'held' AS status\nEXCEPT\nSELECT 'held'\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "a filter on the passed-through column may remove the literal's rows", + authored: "MODEL (description 'Orders');\nWITH a AS (\n SELECT 1 AS order_id, 'open' AS status\n UNION ALL\n SELECT 2, 'other'\n)\nSELECT order_id, status FROM a WHERE status <> 'other'\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "a filter on another column may remove the literal's rows", + authored: "MODEL (description 'Orders');\nWITH a AS (SELECT 2 AS order_id, 'other' AS status)\nSELECT * FROM a WHERE order_id <> 2\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "a HAVING or QUALIFY on the reading select stops the passthrough", + authored: "MODEL (description 'Orders');\nWITH a AS (SELECT 'other' AS status)\nSELECT status FROM a GROUP BY status HAVING COUNT(*) > 1\nUNION ALL\nSELECT status FROM a QUALIFY ROW_NUMBER() OVER (ORDER BY status) > 1\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "an inner join may remove the passed-through rows", + authored: "MODEL (description 'Orders');\nWITH a AS (SELECT 1 AS id, 'other' AS status), b AS (SELECT 2 AS id)\nSELECT a.status FROM a JOIN b ON a.id = b.id\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "a limited select may drop the literal's rows", + authored: "MODEL (description 'Orders');\nSELECT status FROM (SELECT 'other' AS status LIMIT 0) AS d\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "a position reaching a by-name union stops the walk", + authored: "MODEL (description 'Orders');\nSELECT 'open' AS status, 'x' AS note\nUNION ALL\n(SELECT 'held' AS note, 'open' AS status UNION ALL BY NAME SELECT 'open' AS status, 'held' AS note)\n", + expanded: None, + expected_lines: &[], + }, + EnumLiteralTestCase { + description: "the location is the projected literal, not an earlier comparison", + authored: "MODEL (description 'Orders');\nWITH f AS (SELECT * FROM t WHERE note = 'held')\nSELECT 'held' AS status FROM f\n", + expanded: None, + expected_lines: &["status held 3:8"], + }, + EnumLiteralTestCase { + description: "a literal produced by expansion is reported without a location", + authored: "MODEL (description 'Orders');\nSELECT @py.status() AS status\n", + expanded: Some("SELECT 'held' AS status\n"), + expected_lines: &["status held -"], + }, + EnumLiteralTestCase { + description: "a query that does not parse has no violations", + authored: "MODEL (description 'Orders');\nSELECT 'held' AS status FROM (\n", + expanded: None, + expected_lines: &[], + }, + ]; + + for test_case in test_cases { + let query: &str = test_case + .authored + .strip_prefix(ENUM_HEADER) + .expect("authored file starts with the header"); + let model = EnumLiteralModel { + name: "orders", + dialect: "duckdb", + query_sql: test_case.expanded.unwrap_or(query), + authored_sql: test_case.authored, + authored_query_sql: query, + columns: vec![status_column()], + }; + + let lines: Vec = enum_literal_violations(&model) + .iter() + .map(enum_violation_line) + .collect(); + + assert_eq!(lines, test_case.expected_lines, "{}", test_case.description); + } +} + +#[test] +fn given_non_member_literal_when_checking_then_message_names_value_model_and_enum() { + let test_cases = [EnumLiteralMessageTestCase { + description: "a non-member literal", + query: "SELECT 'pending' AS status", + expected_violations: &[( + "K013", + "value 'pending' in column 'status' of model 'orders' is not a member of enum order_status", + "add 'pending' to enum order_status or use one of its members: 'open', 'closed'", + )], + }]; + + for test_case in test_cases { + let authored: String = format!("{ENUM_HEADER}{}", test_case.query); + let model = EnumLiteralModel { + name: "orders", + dialect: "duckdb", + query_sql: test_case.query, + authored_sql: &authored, + authored_query_sql: test_case.query, + columns: vec![status_column()], + }; + + let violations: Vec = enum_literal_violations(&model); + + assert_eq!( + violations + .iter() + .map(|violation| ( + violation.code, + violation.message.as_str(), + violation.help.as_str() + )) + .collect::>(), + test_case.expected_violations, + "{}", + test_case.description + ); + } +} + +#[test] +fn given_case_insensitive_dialect_when_checking_literals_then_skips_the_check() { + let test_cases = [ + EnumLiteralDialectTestCase { + description: "SQL Server compares strings ignoring case and trailing spaces", + dialect: "tsql", + expected_count: 0, + }, + EnumLiteralDialectTestCase { + description: "DuckDB compares strings exactly", + dialect: "duckdb", + expected_count: 1, + }, + ]; + + for test_case in test_cases { + let query: &str = "SELECT 'Open' AS status"; + let authored: String = format!("{ENUM_HEADER}{query}"); + let model = EnumLiteralModel { + name: "orders", + dialect: test_case.dialect, + query_sql: query, + authored_sql: &authored, + authored_query_sql: query, + columns: vec![status_column()], + }; + + assert_eq!( + enum_literal_violations(&model).len(), + test_case.expected_count, + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-analysis/src/contracts/tests/test_types.rs b/crates/sqlbuild-analysis/src/contracts/tests/test_types.rs index 13d7a49533..a58545ab59 100644 --- a/crates/sqlbuild-analysis/src/contracts/tests/test_types.rs +++ b/crates/sqlbuild-analysis/src/contracts/tests/test_types.rs @@ -27,3 +27,26 @@ pub(crate) struct UnknownDialectTestCase { pub(crate) dialect: &'static str, pub(crate) expected_error: &'static str, } + +pub(crate) struct EnumLiteralTestCase { + pub(crate) description: &'static str, + /// The authored model file; its query starts after the first `);\n`. + pub(crate) authored: &'static str, + /// The expanded query, when it differs from the authored one. + pub(crate) expanded: Option<&'static str>, + /// One `column value line:column` line per violation, `-` for an unlocated literal. + pub(crate) expected_lines: &'static [&'static str], +} + +pub(crate) struct EnumLiteralMessageTestCase { + pub(crate) description: &'static str, + pub(crate) query: &'static str, + /// `(code, message, help)` per violation. + pub(crate) expected_violations: &'static [(&'static str, &'static str, &'static str)], +} + +pub(crate) struct EnumLiteralDialectTestCase { + pub(crate) description: &'static str, + pub(crate) dialect: &'static str, + pub(crate) expected_count: usize, +} diff --git a/crates/sqlbuild-discovery/src/header_changes/main/custom_rule_reads.rs b/crates/sqlbuild-discovery/src/header_changes/main/custom_rule_reads.rs index e833b04d0c..59ece8b769 100644 --- a/crates/sqlbuild-discovery/src/header_changes/main/custom_rule_reads.rs +++ b/crates/sqlbuild-discovery/src/header_changes/main/custom_rule_reads.rs @@ -24,6 +24,7 @@ pub fn custom_rule_reads( "contracts.enforced" | "contract" => &["contract"], "contracts.grain" | "grain" => &["contract", "columns"], "columns.declared" | "columns.names" => &["columns", "dynamic_columns"], + "columns.enum_types" => &["columns"], "materialized" => &["materialized"], "tests.for_model" | "audits.for_model" => &["audits", "columns"], _ => &[], diff --git a/crates/sqlbuild-discovery/src/header_changes/tests/test_custom_rule_reads.rs b/crates/sqlbuild-discovery/src/header_changes/tests/test_custom_rule_reads.rs index c64d0166e6..4b3a805e39 100644 --- a/crates/sqlbuild-discovery/src/header_changes/tests/test_custom_rule_reads.rs +++ b/crates/sqlbuild-discovery/src/header_changes/tests/test_custom_rule_reads.rs @@ -11,6 +11,12 @@ fn given_custom_rule_facts_when_a_header_changes_then_reads_of_changed_keys_are_ changed: vec!["columns"], expected_reason: Some("a custom rule reads orders's columns"), }, + CustomRuleReadsTestCase { + description: "a model's enum column types are decided by its columns", + fact_key: vec!["columns.enum_types", "models/orders.sql"], + changed: vec!["columns"], + expected_reason: Some("a custom rule reads orders's columns"), + }, CustomRuleReadsTestCase { description: "a fact of another model is ignored", fact_key: vec!["columns.names", "customers"], diff --git a/crates/sqlbuild-python/src/bindings/_helpers/contracts/contract_diagnostics.rs b/crates/sqlbuild-python/src/bindings/_helpers/contracts/contract_diagnostics.rs index 21a1f8c95e..e3a7afd533 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/contracts/contract_diagnostics.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/contracts/contract_diagnostics.rs @@ -7,9 +7,18 @@ use pyo3::prelude::{ }; use pyo3::types::{PyDict, PyDictMethods, PyTuple}; use pyo3::{IntoPyObject, pyfunction, wrap_pyfunction}; +use serde_json::Value; +use sqlbuild_analysis::compiled_project::main::project_models::project_models; +use sqlbuild_analysis::compiled_project::models::CompiledModelFacts; +use sqlbuild_analysis::contracts::constants::CONTRACT_ENFORCED; +use sqlbuild_analysis::contracts::main::enum_literal_violations::enum_literal_violations; +use sqlbuild_analysis::contracts::models::{ + EnumColumn, EnumLiteralModel, EnumLiteralViolation, LiteralLocation, +}; use sqlbuild_sqltext::compiler::main::model_output_columns::model_output_columns; use crate::bindings::_helpers::compiled_project::project::NativeCompiledProject; +use crate::bindings::_helpers::compiled_views::model_reads::enum_columns; use crate::bindings::_helpers::compiled_views::records::compiled_project_facts; use crate::bindings::_helpers::contracts::model_contracts::{ ConflictRow, DiagnosticRow, evaluate_project_model_contracts, project_promotion_conflicts, @@ -25,6 +34,7 @@ const GENERIC_DIALECT: &str = "generic"; const OUTPUT_LABEL: &str = "output"; const CONTRACT_PHASE: &str = "contract"; const MODEL_RESOURCE: &str = "model"; +const SQL_ANALYSIS_KEY: &str = "sql_analysis"; fn keyword_call<'py>( callable: &Bound<'py, PyAny>, @@ -167,6 +177,109 @@ fn contract_diagnostic<'py>( ) } +/// The model's enum-typed columns whose enum has only string members, in column order. +fn string_enum_columns(model: &Bound<'_, PyAny>) -> PyResult> { + let mut columns: Vec = Vec::new(); + for (name, declaration) in enum_columns(model)?.downcast_into::()?.iter() { + let name: String = name.extract()?; + let mut members: Vec = Vec::new(); + for member in native_field(&declaration, "members")?.try_iter()? { + match native_field(&member?, "value")?.extract::() { + Ok(value) => members.push(value), + Err(_) => { + members.clear(); + break; + } + } + } + if !members.is_empty() { + columns.push(EnumColumn { + name, + enum_name: native_field(&declaration, "name")?.extract()?, + members, + }); + } + } + Ok(columns) +} + +/// The K013 violations of a model with an enforced contract and SQL analysis left on. +fn model_enum_violations( + model: &Bound<'_, PyAny>, + facts: Option<&CompiledModelFacts>, + dialect: &str, +) -> PyResult> { + let Some(facts) = facts else { + return Ok(Vec::new()); + }; + let enforced: bool = facts.config.contract.as_deref() == Some(CONTRACT_ENFORCED); + let analysed: bool = facts.config.values.get(SQL_ANALYSIS_KEY) != Some(&Value::Bool(false)); + if !enforced || !analysed { + return Ok(Vec::new()); + } + let columns: Vec = string_enum_columns(model)?; + if columns.is_empty() { + return Ok(Vec::new()); + } + Ok(enum_literal_violations(&EnumLiteralModel { + name: &facts.name, + dialect, + query_sql: &facts.query_sql, + authored_sql: &facts.authored_sql, + authored_query_sql: &facts.authored_query_sql, + columns, + })) +} + +fn literal_source_location<'py>( + model: &Bound<'py, PyAny>, + location: LiteralLocation, +) -> PyResult> { + let py: Python<'py> = model.py(); + keyword_call( + &record_class(py, RecordId::SourceLocation)?, + vec![ + ("path", native_field(model, "relative_path")?), + ("line", location.line.into_pyobject(py)?.into_any()), + ("column", location.column.into_pyobject(py)?.into_any()), + ("end_line", location.end_line.into_pyobject(py)?.into_any()), + ( + "end_column", + location.end_column.into_pyobject(py)?.into_any(), + ), + ], + ) +} + +fn enum_literal_diagnostic<'py>( + model: &Bound<'py, PyAny>, + violation: EnumLiteralViolation, +) -> PyResult> { + let py: Python<'py> = model.py(); + let location: Bound<'py, PyAny> = match violation.location { + Some(location) => literal_source_location(model, location)?, + None => diagnostic_location(model, (None, Some(&violation.column_name)))?, + }; + keyword_call( + &record_class(py, RecordId::CompilerDiagnostic)?, + vec![ + ("phase", CONTRACT_PHASE.into_pyobject(py)?.into_any()), + ("severity", "error".into_pyobject(py)?.into_any()), + ("code", violation.code.into_pyobject(py)?.into_any()), + ("message", violation.message.into_pyobject(py)?.into_any()), + ("resource_type", model_resource_type(py)?), + ("resource_name", native_field(model, "name")?), + ( + "column_name", + violation.column_name.into_pyobject(py)?.into_any(), + ), + ("path", native_field(model, "relative_path")?), + ("location", location), + ("help", violation.help.into_pyobject(py)?.into_any()), + ], + ) +} + fn model_resource_type(py: Python<'_>) -> PyResult> { enum_from_value(py, RecordId::CompiledResourceType, MODEL_RESOURCE) } @@ -193,9 +306,13 @@ pub(crate) fn evaluate_model_contracts<'py>( let native: PyRef<'py, NativeCompiledProject> = native.extract()?; let outcomes: Vec<(String, Vec)> = evaluate_project_model_contracts( py, - (dialect_name, mode == IMPLICIT_COLUMN_CONTRACTS), + (dialect_name.clone(), mode == IMPLICIT_COLUMN_CONTRACTS), &native, )?; + let facts: HashMap<&str, &CompiledModelFacts> = project_models(native.facts()) + .iter() + .map(|facts| (facts.name.as_str(), facts)) + .collect(); let latest: HashMap<&str, &[DiagnosticRow]> = outcomes .iter() .map(|(owner, rows)| (owner.as_str(), rows.as_slice())) @@ -204,12 +321,13 @@ pub(crate) fn evaluate_model_contracts<'py>( for model in native_field(project, "models")?.try_iter()? { let model: Bound<'py, PyAny> = model?; let name: String = native_field(&model, "name")?.extract()?; - let Some(rows) = latest.get(name.as_str()).copied() else { - continue; - }; - for row in rows { + for row in latest.get(name.as_str()).copied().unwrap_or_default() { diagnostics.push(contract_diagnostic(&model, row.clone())?); } + let model_facts: Option<&CompiledModelFacts> = facts.get(name.as_str()).copied(); + for violation in model_enum_violations(&model, model_facts, &dialect_name)? { + diagnostics.push(enum_literal_diagnostic(&model, violation)?); + } } keyword_call( &record_class(py, RecordId::ContractValidationResult)?, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/rules/custom_host_payload.rs b/crates/sqlbuild-python/src/bindings/_helpers/rules/custom_host_payload.rs index 7f924a3723..3a788c6c1c 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/rules/custom_host_payload.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/rules/custom_host_payload.rs @@ -6,12 +6,14 @@ use std::sync::Arc; use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyResult, PyTypeMethods, Python}; use pyo3::pyfunction; use pyo3::types::{ - PyBytes, PyBytesMethods, PyDictMethods, PyString, PyStringMethods, PyTuple, PyTupleMethods, + PyBytes, PyBytesMethods, PyDict, PyDictMethods, PyString, PyStringMethods, PyTuple, + PyTupleMethods, }; use serde_json::{Map, Value, json}; use sqlbuild_discovery::models::LineColumnSpan; use crate::bindings::_helpers::compiled_views::compiled_model_view::NativeCompiledModel; +use crate::bindings::_helpers::compiled_views::model_reads::enum_columns; use crate::bindings::_helpers::compiled_views::records::{ compiled_audit_record, compiled_function_record, compiled_seed_record, compiled_source_record, }; @@ -225,6 +227,8 @@ impl PayloadWriter { model.insert("declared".to_owned(), declared); let enums: Bound<'_, PyAny> = view.enum_declarations(py)?.into_bound(py).into_any(); model.insert("enums".to_owned(), self.declarations(&enums)?); + let columns: Bound<'_, PyAny> = view.enum_columns(py)?.into_bound(py).into_any(); + model.insert("enum_types".to_owned(), enum_type_names(&columns)?); let constants: Bound<'_, PyAny> = view.constant_declarations(py)?.into_bound(py).into_any(); model.insert("constants".to_owned(), self.declarations(&constants)?); Ok(model) @@ -253,6 +257,10 @@ impl PayloadWriter { model.insert("declared".to_owned(), declared); let enums: Value = self.declarations(&native_field(record, "enum_declarations")?)?; model.insert("enums".to_owned(), enums); + model.insert( + "enum_types".to_owned(), + enum_type_names(&enum_columns(record)?)?, + ); let constants: Value = self.declarations(&native_field(record, "constant_declarations")?)?; model.insert("constants".to_owned(), constants); @@ -260,6 +268,17 @@ impl PayloadWriter { } } +/// `[column, enum name]` for each enum-typed column, in column order. +fn enum_type_names(columns: &Bound<'_, PyAny>) -> PyResult { + let mut pairs: Vec = Vec::new(); + for (column, declaration) in columns.downcast::()?.iter() { + let column: String = column.extract()?; + let name: String = native_field(&declaration, "name")?.extract()?; + pairs.push(json!([column, name])); + } + Ok(Value::Array(pairs)) +} + /// The view's declared columns when every one was parsed natively at a plain path. fn native_declared(py: Python<'_>, view: &NativeCompiledModel) -> PyResult { view.read(py, |record, py| { diff --git a/crates/sqlbuild-python/src/bindings/_helpers/rules/fact_replay.rs b/crates/sqlbuild-python/src/bindings/_helpers/rules/fact_replay.rs index a794a92a43..9b2a93cd6a 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/rules/fact_replay.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/rules/fact_replay.rs @@ -61,6 +61,7 @@ enum Fact { Declared, Inferred, Names, + EnumTypes, Enforced, Grain, TestsForModel, @@ -83,7 +84,7 @@ enum Arguments { } /// Every fact by key name, globals first, in the order full read sets list them. -const FACTS: [(&str, Fact, Arguments); 29] = [ +const FACTS: [(&str, Fact, Arguments); 30] = [ ("project.models", Fact::Models, Arguments::None), ("project.sources", Fact::Sources, Arguments::None), ("project.seeds", Fact::Seeds, Arguments::None), @@ -115,6 +116,7 @@ const FACTS: [(&str, Fact, Arguments); 29] = [ ("columns.declared", Fact::Declared, Arguments::ModelPath), ("columns.inferred", Fact::Inferred, Arguments::ModelPath), ("columns.names", Fact::Names, Arguments::ModelPath), + ("columns.enum_types", Fact::EnumTypes, Arguments::ModelPath), ("contracts.enforced", Fact::Enforced, Arguments::ModelPath), ("contracts.grain", Fact::Grain, Arguments::ModelPath), ("tests.for_model", Fact::TestsForModel, Arguments::ModelName), @@ -187,6 +189,7 @@ pub(crate) fn delta_safe_fact( | Fact::Declared | Fact::Inferred | Fact::Names + | Fact::EnumTypes | Fact::Enforced | Fact::Grain ) @@ -472,6 +475,25 @@ impl<'py> FactReplay<'py> { } } }, + Fact::EnumTypes => { + let columns: Bound<'py, PyAny> = match &view { + Some(view) => view.enum_columns(py)?.into_bound(py).into_any(), + None => record_field(&self.encoder, &model, "enum_columns")?, + }; + let mut pairs: Vec = Vec::new(); + for (column, declaration) in + columns.downcast::().map_err(PyErr::from)?.iter() + { + let column: String = column.extract()?; + let name: String = + record_field(&self.encoder, &declaration, "name")?.extract()?; + pairs.push(JsonValue::Array(vec![ + JsonValue::String(column), + JsonValue::String(name), + ])); + } + JsonValue::Array(pairs) + } Fact::Inferred | Fact::Names => { let rows: Option> = match &view { Some(view) => view.inferred_rows(py)?, diff --git a/scripts/compiler_differential/_helpers/analysis_corpus/failure_cases.py b/scripts/compiler_differential/_helpers/analysis_corpus/failure_cases.py index f0b3c21434..72be261887 100644 --- a/scripts/compiler_differential/_helpers/analysis_corpus/failure_cases.py +++ b/scripts/compiler_differential/_helpers/analysis_corpus/failure_cases.py @@ -342,6 +342,26 @@ def _contract_cases() -> tuple[FailureCase, ...]: expected_message="model 'stg_orders' has contract enforced but declares no columns", files=staging_files(f"MODEL (\n{contract_header});\n\n{query}"), ), + failure_case( + name="contract-enum-literal", + expected_code="K013", + expected_message=( + "value 'held' in column 'status' of model 'stg_orders' is not a member of enum " + "order_status" + ), + files={ + "models/staging/_enums/order_status.sql": ( + 'ENUM (name order_status, members (PLACED "placed", SHIPPED "shipped"));\n' + ), + **staging_files( + f"MODEL (\n{contract_header}" + " columns (\n order_id (type INTEGER),\n customer_id (type INTEGER),\n" + " amount (type DOUBLE),\n status (type order_status),\n ),\n);\n\n" + "SELECT order_id, customer_id, amount, 'held' AS status\n" + 'FROM __source("raw_orders")\n' + ), + }, + ), failure_case( name="contract-immediate-promotion", expected_code="K011", diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/compile.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/compile.md index 86e3b3251d..ca2647b029 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/compile.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/compile.md @@ -259,6 +259,7 @@ Diagnostic codes: | `K001` | A declared column is missing from the model's query output | | `K002` | A column's inferred type does not match the declared type | | `K003` | A column's type could not be proven (with `type_enforcement` enabled) | +| `K013` | A string literal fills an enum-typed column of an enforced contract but is not a member of the enum (not checked on SQL Server) | Compile returns exit code `1` when any error-severity diagnostic is found, making it suitable for CI checks. diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/enums/model-contracts.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/enums/model-contracts.md index 46cd77daa9..4e54453243 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/enums/model-contracts.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/enums/model-contracts.md @@ -44,6 +44,40 @@ audits. Its severity and timing follow the model's ordinary audit and materializ With `contract none`, SQLBuild still resolves the enum to its portable scalar column type but does not generate the domain audit. +## Literals checked at compile time + +With `contract enforced`, `sqb compile` also checks every constant string literal that fills an +enum-typed column of a string enum, so a missing member fails before the build rather than in the +audit afterwards. A literal counts when it is the column's whole expression, when it fills the +column in any `UNION` or `UNION ALL` branch, or when the column passes it through unfiltered from a +CTE or derived table: + +```sql +MODEL ( + description "Orders from every channel", + contract enforced, + columns ( + fulfillment_method (type fulfillment_method), + ), +); + +SELECT order_id, 'DELIVERY' AS fulfillment_method FROM __ref("stg_web_orders") +UNION ALL +SELECT order_id, 'COURIER' FROM __ref("stg_partner_orders") +``` + +``` +error[K013]: value 'COURIER' in column 'fulfillment_method' of model 'orders' is not a member of enum fulfillment_method +``` + +The error points at the literal in the model file. Computed values, such as a `CASE` expression or +a function call, are not checked at compile time, and neither is a column read from a CTE or +derived table through a filter, join or row limit that could remove the literal's rows; the +generated `accepted_values` audit still checks them when the model builds. + +SQL Server compares strings ignoring case and trailing spaces under its default collation, so the +compile-time check is skipped on the SQL Server adapter and the audit alone checks the column. + This behavior is the same on adapters with and without native enum support. The physical warehouse column remains an ordinary string or integer column. diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/custom-rules/rule-context.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/custom-rules/rule-context.md index 58b0857ffc..9daf4094f6 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/custom-rules/rule-context.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/custom-rules/rule-context.md @@ -42,6 +42,18 @@ for star in sql.authored.star_projections(): ... ``` +## Declared enum types + +`ctx.columns.declared(model)` reports each declared column with its portable scalar type, so an +enum-typed column reads as `VARCHAR` or `INTEGER`. `ctx.columns.enum_types(model)` maps each +enum-typed column's name to the enum it declares: + +```python +enum_types = ctx.columns.enum_types(model) +if enum_types.get("fulfillment_method") != "fulfillment_method": + ... +``` + ## Lazy Polyglot access Use the full Polyglot AST only when common projections are insufficient: diff --git a/src/sqlbuild/rule_engine/_helpers/host/host_payload.py b/src/sqlbuild/rule_engine/_helpers/host/host_payload.py index c2e4a64844..46228fcec7 100644 --- a/src/sqlbuild/rule_engine/_helpers/host/host_payload.py +++ b/src/sqlbuild/rule_engine/_helpers/host/host_payload.py @@ -128,6 +128,10 @@ def constant_declarations(self) -> tuple[ConstantDeclaration, ...]: encoded: object = self._entry["constants"] return () if encoded is None else self._payload.value(encoded) + @cached_property + def enum_type_names(self) -> dict[str, str]: + return {column: enum_name for column, enum_name in self._entry["enum_types"]} + class _HostProject: """One compiled project's rule facts, each collection decoded on first read.""" diff --git a/src/sqlbuild/rule_engine/_helpers/host/tracked_facts.py b/src/sqlbuild/rule_engine/_helpers/host/tracked_facts.py index 619fac553a..c20f2a33ed 100644 --- a/src/sqlbuild/rule_engine/_helpers/host/tracked_facts.py +++ b/src/sqlbuild/rule_engine/_helpers/host/tracked_facts.py @@ -24,6 +24,7 @@ FACT_AUDITS_ALL, FACT_AUDITS_FOR_MODEL, FACT_COLUMNS_DECLARED, + FACT_COLUMNS_ENUM_TYPES, FACT_COLUMNS_INFERRED, FACT_COLUMNS_NAMES, FACT_CONTRACTS_ENFORCED, @@ -221,6 +222,10 @@ def names(self, model: Model) -> tuple[str, ...] | None: self._tracked_reads.record_model_path(fact=FACT_COLUMNS_NAMES, model=model) return self._tracked_view.names(model) + def enum_types(self, model: Model) -> dict[str, str]: + self._tracked_reads.record_model_path(fact=FACT_COLUMNS_ENUM_TYPES, model=model) + return self._tracked_view.enum_types(model) + class _TrackedContractFacts(_TrackedView): __slots__ = () diff --git a/src/sqlbuild/rule_engine/classes/column_facts.py b/src/sqlbuild/rule_engine/classes/column_facts.py index 42551fe6c7..2f31eb9482 100644 --- a/src/sqlbuild/rule_engine/classes/column_facts.py +++ b/src/sqlbuild/rule_engine/classes/column_facts.py @@ -22,6 +22,9 @@ def declared(self, model: Model) -> tuple[object, ...]: def inferred(self, model: Model) -> tuple[InferredColumn, ...] | None: return self._models[model.path].inferred_columns + def enum_types(self, model: Model) -> dict[str, str]: + return dict(self._models[model.path].enum_type_names) + def names(self, model: Model) -> tuple[str, ...] | None: inferred: tuple[InferredColumn, ...] | None = self.inferred(model) return None if inferred is None else tuple(column.name for column in inferred) diff --git a/src/sqlbuild/rule_engine/constants.py b/src/sqlbuild/rule_engine/constants.py index 42a0ce8325..6e2e067e6d 100644 --- a/src/sqlbuild/rule_engine/constants.py +++ b/src/sqlbuild/rule_engine/constants.py @@ -40,6 +40,7 @@ FACT_COLUMNS_DECLARED: str = "columns.declared" FACT_COLUMNS_INFERRED: str = "columns.inferred" FACT_COLUMNS_NAMES: str = "columns.names" +FACT_COLUMNS_ENUM_TYPES: str = "columns.enum_types" FACT_CONTRACTS_ENFORCED: str = "contracts.enforced" FACT_CONTRACTS_GRAIN: str = "contracts.grain" FACT_TESTS_ALL: str = "tests.all" diff --git a/src/sqlbuild/rule_engine/types.py b/src/sqlbuild/rule_engine/types.py index 5f052e4ae9..f520c9fee4 100644 --- a/src/sqlbuild/rule_engine/types.py +++ b/src/sqlbuild/rule_engine/types.py @@ -71,6 +71,9 @@ def enum_declarations(self) -> tuple[EnumDeclaration, ...]: ... @property def constant_declarations(self) -> tuple[ConstantDeclaration, ...]: ... + @property + def enum_type_names(self) -> dict[str, str]: ... + def config_value(self, name: str) -> object: ... diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 07377d7931..84f3707c59 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -1301,3 +1301,12 @@ class CompiledViewFallbacksTestCase: command: tuple[str, ...] expected_exit_code: int expected_fallbacks: int + + +@dataclass(frozen=True) +class EnumLiteralCompileTestCase: + description: str + model_sql: str + expected_exit_code: int + expected_diagnostics: tuple[tuple[str, str, int | None], ...] + contract: str = "enforced" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py index d118f061f7..e69977b35f 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py @@ -1189,3 +1189,28 @@ def write_project_scratch_rules(project_dir: Path) -> None: "rules/scratch.py": PROJECT_SCRATCH_RULES, }, ) + + +def write_enum_project(*, root: Path, model_sql: str, contract: str) -> None: + """Write an orders project whose model fills an `order_status` column from `model_sql`.""" + + (root / "sqlbuild_project.toml").write_text('name = "orders"\nadapter = "duckdb"\n') + (root / "models" / "_enums").mkdir(parents=True) + (root / "models" / "_enums" / "order_status.sql").write_text( + 'ENUM (name order_status, members (OPEN "open", CLOSED "closed"));\n' + ) + (root / "sources").mkdir() + (root / "sources" / "orders.yml").write_text( + "sources:\n" + " - name: web_orders\n description: Web orders.\n contract: enforced\n" + ' expression: "(SELECT CAST(1 AS INTEGER) AS order_id)"\n' + " columns:\n - name: order_id\n type: INTEGER\n" + " - name: partner_orders\n description: Partner orders.\n contract: enforced\n" + ' expression: "(SELECT CAST(2 AS INTEGER) AS order_id)"\n' + " columns:\n - name: order_id\n type: INTEGER\n" + ) + (root / "models" / "orders.sql").write_text( + f"MODEL (\n description 'Orders from every channel.',\n contract {contract},\n" + " columns (\n order_id (type INTEGER),\n status (type order_status),\n ),\n);\n\n" + f"{model_sql}" + ) diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_compile_enum_literals.py b/tests/integration/src/sqlbuild/cli/commands/main/test_compile_enum_literals.py new file mode 100644 index 0000000000..18e43a01fe --- /dev/null +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_compile_enum_literals.py @@ -0,0 +1,143 @@ +"""Compile checks the string literals that fill enum-typed columns against the enum's members.""" + +from __future__ import annotations + +import json +from pathlib import Path +from typing import cast + +import pytest + +from sqlbuild._native import run_cli as main +from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( + EnumLiteralCompileTestCase, +) +from tests.integration.src.sqlbuild.cli.commands.main.helpers import write_enum_project + +_UNION_MODEL: str = ( + "SELECT order_id, 'open' AS status FROM __source(\"web_orders\")\n" + "UNION ALL\n" + "SELECT order_id, 'held' FROM __source(\"partner_orders\")\n" +) + + +@pytest.mark.parametrize( + "test_case", + ( + EnumLiteralCompileTestCase( + description="a union branch literal outside the enum fails at the literal", + model_sql=_UNION_MODEL, + expected_exit_code=1, + expected_diagnostics=( + ( + "K013", + "value 'held' in column 'status' of model 'orders' is not a member of " + "enum order_status", + 12, + ), + ), + ), + EnumLiteralCompileTestCase( + description="a literal passed through a CTE fails", + model_sql=( + "WITH flagged AS (\n" + " SELECT order_id, 'pending' AS status FROM __source(\"web_orders\")\n" + ")\n" + "SELECT order_id, status FROM flagged\n" + ), + expected_exit_code=1, + expected_diagnostics=( + ( + "K013", + "value 'pending' in column 'status' of model 'orders' is not a member of " + "enum order_status", + 11, + ), + ), + ), + EnumLiteralCompileTestCase( + description="a filter on the passed-through column leaves the check to the audit", + model_sql=( + "WITH unioned AS (\n" + f"{_UNION_MODEL}" + ")\n" + "SELECT order_id, status FROM unioned WHERE status <> 'held'\n" + ), + expected_exit_code=0, + expected_diagnostics=(), + ), + EnumLiteralCompileTestCase( + description="an unfiltered passthrough of the union is still checked", + model_sql=f"WITH unioned AS (\n{_UNION_MODEL})\nSELECT order_id, status FROM unioned\n", + expected_exit_code=1, + expected_diagnostics=( + ( + "K013", + "value 'held' in column 'status' of model 'orders' is not a member of " + "enum order_status", + 13, + ), + ), + ), + EnumLiteralCompileTestCase( + description="member literals compile", + model_sql=_UNION_MODEL.replace("'held'", "'closed'"), + expected_exit_code=0, + expected_diagnostics=(), + ), + EnumLiteralCompileTestCase( + description="enum member references compile", + model_sql=( + 'SELECT order_id, @enum("order_status").CLOSED AS status ' + 'FROM __source("web_orders")\n' + ), + expected_exit_code=0, + expected_diagnostics=(), + ), + EnumLiteralCompileTestCase( + description="a computed value is left to the generated audit", + model_sql=( + "SELECT order_id, CASE WHEN order_id > 1 THEN 'held' ELSE 'open' END AS status " + 'FROM __source("web_orders")\n' + ), + expected_exit_code=0, + expected_diagnostics=(), + ), + EnumLiteralCompileTestCase( + description="without an enforced contract the enum domain is not checked", + model_sql=_UNION_MODEL, + contract="none", + expected_exit_code=0, + expected_diagnostics=(), + ), + ), + ids=lambda case: case.description, +) +def test_given_enum_typed_column_when_compiling_then_non_member_literals_fail( + test_case: EnumLiteralCompileTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + write_enum_project(root=tmp_path, model_sql=test_case.model_sql, contract=test_case.contract) + + exit_code: int = main( + ["--project-dir", str(tmp_path), "--no-color", "compile", "--json", "--no-cache"] + ) + + result: dict[str, object] = json.loads(capsys.readouterr().out) + diagnostics: list[dict[str, object]] = cast(list[dict[str, object]], result["diagnostics"]) + assert exit_code == test_case.expected_exit_code + errors: list[dict[str, object]] = list( + filter(lambda item: item["severity"] == "error", diagnostics) + ) + assert ( + tuple( + (item["code"], item["message"], cast(dict[str, object], item["location"])["line"]) + for item in errors + ) + == test_case.expected_diagnostics + ) + + +if __name__ == "__main__": + raise SystemExit(pytest.main([__file__, "-n", "auto", "--dist", "loadfile"])) diff --git a/tests/integration/src/sqlbuild/rule_engine/fact_digests/test_fact_digests.py b/tests/integration/src/sqlbuild/rule_engine/fact_digests/test_fact_digests.py index 057b2efafd..57e900dc41 100644 --- a/tests/integration/src/sqlbuild/rule_engine/fact_digests/test_fact_digests.py +++ b/tests/integration/src/sqlbuild/rule_engine/fact_digests/test_fact_digests.py @@ -50,6 +50,15 @@ _STG_ACCEPTED_STRINGS: dict[str, str] = { _STG: _STG_ACCEPTED_INTEGERS[_STG].replace("values [1, 2]", "values ['1', '2']") } +_STG_STATUS: dict[str, str] = { + _STG: ( + "MODEL (\n columns (\n order_id (type INTEGER),\n status (type VARCHAR),\n ),\n);\n\n" + "SELECT CAST(id AS INTEGER) AS order_id, 'PAID' AS status FROM __source(\"raw_orders\")\n" + ) +} +_STG_STATUS_ENUM: dict[str, str] = { + _STG: _STG_STATUS[_STG].replace("status (type VARCHAR)", "status (type order_status)") +} _TOTALS_WITHOUT_REF: dict[str, str] = {_TOTALS: "MODEL ();\n\nSELECT 1 AS order_id\n"} _TEST_EDITED: dict[str, str] = { "tests/unit/test_order_totals.sql": ( @@ -117,6 +126,12 @@ "inferred columns", ("columns.inferred", _TOTALS), _TOTALS_WITH_COLUMN ), FactDigestEditTestCase("column names", ("columns.names", _TOTALS), _TOTALS_WITH_COLUMN), + FactDigestEditTestCase( + "enum column types", + ("columns.enum_types", _STG), + _STG_STATUS_ENUM, + base_files=_STG_STATUS, + ), FactDigestEditTestCase("contract", ("contracts.enforced", _TOTALS), _TOTALS_CONTRACT), FactDigestEditTestCase("model tests", ("tests.for_model", "order_totals"), _TEST_EDITED), FactDigestEditTestCase("model audits", ("audits.for_model", "stg_orders"), _STG_AUDITED), diff --git a/tests/integration/src/sqlbuild/rule_engine/host_payload/helpers.py b/tests/integration/src/sqlbuild/rule_engine/host_payload/helpers.py index 1d6805bf08..7fdf6fd621 100644 --- a/tests/integration/src/sqlbuild/rule_engine/host_payload/helpers.py +++ b/tests/integration/src/sqlbuild/rule_engine/host_payload/helpers.py @@ -63,6 +63,9 @@ def __init__(self, model: CompiledModelView) -> None: self.enum_declarations: tuple[object, ...] = model.enum_declarations self.constant_declarations: tuple[object, ...] = model.constant_declarations self.declared_columns: tuple[object, ...] = getattr(model.schema_entry, "columns", ()) + self.enum_type_names: dict[str, str] = { + column: declaration.name for column, declaration in model.enum_columns.items() + } def config_value(self, name: str) -> object: return self._model.config.values.get(name) @@ -144,6 +147,7 @@ def every_fact_value(views: RuleFactViews) -> dict[str, object]: values[f"columns.declared:{subject}"] = views.columns.declared(model) values[f"columns.inferred:{subject}"] = views.columns.inferred(model) values[f"columns.names:{subject}"] = views.columns.names(model) + values[f"columns.enum_types:{subject}"] = views.columns.enum_types(model) values[f"contracts.enforced:{subject}"] = views.contracts.enforced(model) values[f"contracts.grain:{subject}"] = views.contracts.grain(model) values[f"tests.for_model:{subject}"] = views.tests.for_model(model) diff --git a/tests/integration/src/sqlbuild/rule_engine/host_payload/test_host_payload_facts.py b/tests/integration/src/sqlbuild/rule_engine/host_payload/test_host_payload_facts.py index c674418157..f153d147fc 100644 --- a/tests/integration/src/sqlbuild/rule_engine/host_payload/test_host_payload_facts.py +++ b/tests/integration/src/sqlbuild/rule_engine/host_payload/test_host_payload_facts.py @@ -88,6 +88,7 @@ "declarations.constants", f"columns.declared:{_STAGING}", f"columns.inferred:{_STAGING}", + f"columns.enum_types:{_STAGING}", f"audits.for_model:{_STAGING}", "tests.for_model:models/marts/order_totals.sql", "graph.dependents:models/marts/order_flags.sql", diff --git a/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/_test_types.py b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/_test_types.py index b5cd76213b..4b3871fc45 100644 --- a/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/_test_types.py +++ b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/_test_types.py @@ -33,3 +33,10 @@ class AuthoringImportTestCase: imported_module: str checked_modules: tuple[str, ...] expected_loaded_modules: tuple[str, ...] + + +@dataclass(frozen=True) +class EnumTypesRuleTestCase: + description: str + status_type: str + expected_finding_count: int diff --git a/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/helpers.py b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/helpers.py index 7a650c16b8..f2999fc798 100644 --- a/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/helpers.py +++ b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/helpers.py @@ -33,3 +33,15 @@ def first_evaluation_only(*, model: Model, ctx: RuleContext) -> list[Finding]: repeated: bool = model.name in _EVALUATED_MODELS _EVALUATED_MODELS.add(model.name) return [ctx.finding(subject=model)] * int(repeated) + + +@rule( + code="XSQBRE001", + slug="status-enum", + message="status column must use the order_status enum", + remediation="Declare the column as status (type order_status).", +) +def status_enum(*, model: Model, ctx: RuleContext) -> list[Finding]: + names: tuple[str, ...] = ctx.columns.names(model) or () + typed: bool = ctx.columns.enum_types(model).get("status") == "order_status" + return [ctx.finding(subject=model)] * int("status" in names and not typed) diff --git a/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/test_evaluate_rule.py b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/test_evaluate_rule.py index 7b5140b457..fe27c524b7 100644 --- a/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/test_evaluate_rule.py +++ b/tests/unit/src/sqlbuild/rule_engine/main/evaluate_rule/test_evaluate_rule.py @@ -16,9 +16,10 @@ from sqlbuild._native import evaluate_project_rules as evaluate from sqlbuild.compiler.compile.types import CompiledProjectView from sqlbuild.rule_engine.types import RuleOptionValue -from sqlbuild.rules.testing import RuleCase, RuleResult, evaluate_rule +from sqlbuild.rules.testing import RuleCase, RuleFile, RuleResult, evaluate_rule from tests.unit.src.sqlbuild.rule_engine.main.evaluate.helpers import build_project from tests.unit.src.sqlbuild.rule_engine.main.evaluate_rule._test_types import ( + EnumTypesRuleTestCase, EvaluateRuleDeterminismTestCase, EvaluateRuleParityTestCase, EvaluateRuleTestCase, @@ -26,6 +27,7 @@ from tests.unit.src.sqlbuild.rule_engine.main.evaluate_rule.helpers import ( first_evaluation_only, required_domain, + status_enum, ) @@ -150,3 +152,44 @@ def test_given_rule_with_module_state_when_evaluating_case_then_determinism_chec ) -> None: with pytest.raises(RulesError, match=test_case.expected_error_pattern): evaluate_rule(rule=first_evaluation_only, test_case=test_case.rule_case) + + +@pytest.mark.parametrize( + "test_case", + ( + EnumTypesRuleTestCase( + description="an enum-typed column reports its enum", + status_type="order_status", + expected_finding_count=0, + ), + EnumTypesRuleTestCase( + description="a plain VARCHAR column reports no enum", + status_type="VARCHAR", + expected_finding_count=1, + ), + ), + ids=lambda case: case.description, +) +def test_given_declared_status_column_when_rule_reads_enum_types_then_sees_declared_enum( + test_case: EnumTypesRuleTestCase, +) -> None: + rule_case: RuleCase = RuleCase( + description=test_case.description, + source=( + "MODEL (\n description 'Orders.',\n" + f" columns (\n status (type {test_case.status_type}),\n ),\n);\n\n" + "SELECT 'open' AS status\n" + ), + path="models/orders.sql", + files=( + RuleFile( + path="models/_enums/order_status.sql", + source='ENUM (name order_status, members (OPEN "open", CLOSED "closed"));\n', + ), + ), + expected_finding_count=test_case.expected_finding_count, + ) + + result: RuleResult = evaluate_rule(rule=status_enum, test_case=rule_case) + + assert result.finding_count == test_case.expected_finding_count diff --git a/website/src/content/docs/docs/cli/compile.mdx b/website/src/content/docs/docs/cli/compile.mdx index 42f65754c8..9322d31b43 100644 --- a/website/src/content/docs/docs/cli/compile.mdx +++ b/website/src/content/docs/docs/cli/compile.mdx @@ -253,6 +253,7 @@ Diagnostic codes: | `K001` | A declared column is missing from the model's query output | | `K002` | A column's inferred type does not match the declared type | | `K003` | A column's type could not be proven (with `type_enforcement` enabled) | +| `K013` | A string literal fills an enum-typed column of an enforced contract but is not a member of the enum (not checked on SQL Server) | Compile returns exit code `1` when any error-severity diagnostic is found, making it suitable for CI checks. diff --git a/website/src/content/docs/docs/concepts/enums/model-contracts.mdx b/website/src/content/docs/docs/concepts/enums/model-contracts.mdx index 635a51819a..0ae560cb14 100644 --- a/website/src/content/docs/docs/concepts/enums/model-contracts.mdx +++ b/website/src/content/docs/docs/concepts/enums/model-contracts.mdx @@ -41,6 +41,40 @@ audits. Its severity and timing follow the model's ordinary audit and materializ With `contract none`, SQLBuild still resolves the enum to its portable scalar column type but does not generate the domain audit. +## Literals checked at compile time + +With `contract enforced`, `sqb compile` also checks every constant string literal that fills an +enum-typed column of a string enum, so a missing member fails before the build rather than in the +audit afterwards. A literal counts when it is the column's whole expression, when it fills the +column in any `UNION` or `UNION ALL` branch, or when the column passes it through unfiltered from a +CTE or derived table: + +```sql +MODEL ( + description "Orders from every channel", + contract enforced, + columns ( + fulfillment_method (type fulfillment_method), + ), +); + +SELECT order_id, 'DELIVERY' AS fulfillment_method FROM __ref("stg_web_orders") +UNION ALL +SELECT order_id, 'COURIER' FROM __ref("stg_partner_orders") +``` + +``` +error[K013]: value 'COURIER' in column 'fulfillment_method' of model 'orders' is not a member of enum fulfillment_method +``` + +The error points at the literal in the model file. Computed values, such as a `CASE` expression or +a function call, are not checked at compile time, and neither is a column read from a CTE or +derived table through a filter, join or row limit that could remove the literal's rows; the +generated `accepted_values` audit still checks them when the model builds. + +SQL Server compares strings ignoring case and trailing spaces under its default collation, so the +compile-time check is skipped on the SQL Server adapter and the audit alone checks the column. + This behavior is the same on adapters with and without native enum support. The physical warehouse column remains an ordinary string or integer column. diff --git a/website/src/content/docs/docs/concepts/rules/custom-rules/rule-context.mdx b/website/src/content/docs/docs/concepts/rules/custom-rules/rule-context.mdx index 18c35f43ff..6f0d16b2eb 100644 --- a/website/src/content/docs/docs/concepts/rules/custom-rules/rule-context.mdx +++ b/website/src/content/docs/docs/concepts/rules/custom-rules/rule-context.mdx @@ -39,6 +39,18 @@ for star in sql.authored.star_projections(): ... ``` +## Declared enum types + +`ctx.columns.declared(model)` reports each declared column with its portable scalar type, so an +enum-typed column reads as `VARCHAR` or `INTEGER`. `ctx.columns.enum_types(model)` maps each +enum-typed column's name to the enum it declares: + +```python +enum_types = ctx.columns.enum_types(model) +if enum_types.get("fulfillment_method") != "fulfillment_method": + ... +``` + ## Lazy Polyglot access Use the full Polyglot AST only when common projections are insufficient: From 5b1f51ec5f2462c75a4d938b9f6df515a733f569 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 26 Oct 2026 05:10:01 +0000 Subject: [PATCH 3/5] fix(sql-macros): check LATERAL subqueries and relation reads in slot checks The output-select walk shared the capture walk's scope classification, which treats a LATERAL subquery as an expression, so a mapped single-column slot read without AS inside a LATERAL whose columns reach the output through l.* escaped the AS rule. The output walk now treats every FROM and JOIN item, LATERAL included, like a derived table; the capture check is unchanged. The P018 clash check counted only bare columns beside a group, so an undeclared column read through the relation, such as @weekly.region, next to a group holding the same column produced a duplicate output column. Such reads now count as output columns. --- .../src/sql_macros/_helpers/parsing/body.rs | 9 +++- .../sql_macros/_helpers/parsing/captures.rs | 17 +++++++- .../sql_macros/tests/test_relation_slots.rs | 43 +++++++++++++++++++ .../docs/concepts/macros/sql-macros.md | 6 ++- .../sql_macros/test_sql_macro_slot_scopes.py | 36 ++++++++++++++++ .../docs/docs/concepts/macros/sql-macros.mdx | 6 ++- 6 files changed, 109 insertions(+), 8 deletions(-) diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/parsing/body.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/parsing/body.rs index 94772236a3..fa92f6a590 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/parsing/body.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/parsing/body.rs @@ -297,7 +297,7 @@ fn read_mapping<'a>( .find(|mapping| mapping.slot.eq_ignore_ascii_case(&read.slot)) } -/// A bare column `select` outputs twice: once by name and once in a group read beside it. +/// A column `select` outputs twice: bare or read through a relation, and in a group beside it. fn select_clash( definition: &SqlMacroDefinition, select: &Select, @@ -313,7 +313,12 @@ fn select_clash( .as_ref() .and_then(|table| parameter_of(definition, table)) { - Some(parameter) => read.push((parameter.name.as_str(), &column.name.name)), + Some(parameter) => { + read.push((parameter.name.as_str(), &column.name.name)); + if !parameter.kind.declares_slot(&column.name.name) { + bare.push(&column.name.name); + } + } None => bare.push(&column.name.name), } } diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/parsing/captures.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/parsing/captures.rs index d106dde872..b76ad701af 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/parsing/captures.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/parsing/captures.rs @@ -52,6 +52,12 @@ fn source_names(source: &Expression) -> Vec> { /// Whether `child` of `select` is a query of its own: a CTE body, or a non-`LATERAL` `FROM` item. fn own_scope(select: &Select, child: &Expression) -> bool { let lateral: bool = matches!(child, Expression::Subquery(subquery) if subquery.lateral); + let (cte, source) = child_kind(select, child); + cte || (source && !lateral) +} + +/// Whether `child` of `select` is a CTE body, and whether it is a `FROM` or `JOIN` item. +fn child_kind(select: &Select, child: &Expression) -> (bool, bool) { let source: bool = select .from .iter() @@ -63,7 +69,13 @@ fn own_scope(select: &Select, child: &Expression) -> bool { .iter() .flat_map(|with| &with.ctes) .any(|cte| std::ptr::eq(&cte.this, child)); - cte || (source && !lateral) + (cte, source) +} + +/// Whether `child` of `select` can reach its output: a CTE body or any `FROM` item, `LATERAL` too. +fn output_scope(select: &Select, child: &Expression) -> bool { + let (cte, source) = child_kind(select, child); + cte || source } /// The selects whose output names reach the query's output: all but those inside expressions. @@ -84,7 +96,8 @@ fn selects_under(node: &Expression, in_expression: bool) -> Vec<&Select> { found.push(select); } for child in node.children() { - let inside: bool = in_expression || select.is_some_and(|select| !own_scope(select, child)); + let inside: bool = + in_expression || select.is_some_and(|select| !output_scope(select, child)); found.extend(selects_under(child, inside)); } found diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_relation_slots.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_relation_slots.rs index b38f3bc2bb..68ec90d02e 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_relation_slots.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_relation_slots.rs @@ -97,6 +97,16 @@ fn given_relation_slots_when_expanding_then_each_read_becomes_the_mapped_columns sql: "SELECT @sql.counted_regions(w, weekly.order_count := orders_placed) FROM weekly_orders AS w", expected_sql: "SELECT w.orders_placed AS orders, w.region FROM weekly_orders AS w", }, + ExpansionTestCase { + description: "a mapped single-column slot read with AS in a LATERAL subquery", + adapter: "", + macros: &[( + "lateral_named_counts", + "MACRO (description \"Latest order count per customer\", arguments (weekly RELATION (order_count BIGINT)));\n\nWITH latest AS (SELECT c.customer_id, l.* FROM customers AS c, LATERAL (SELECT @weekly.order_count AS orders FROM @weekly) AS l)\n", + )], + sql: "WITH w AS (SELECT 2 AS orders_placed), @sql.lateral_named_counts(w, weekly.order_count := orders_placed) SELECT * FROM latest", + expected_sql: "WITH w AS (SELECT 2 AS orders_placed), latest AS (SELECT c.customer_id, l.* FROM customers AS c, LATERAL (SELECT w.orders_placed AS orders FROM w) AS l) SELECT * FROM latest", + }, ExpansionTestCase { description: "an expression macro reads mapped single-column slots", adapter: "", @@ -271,6 +281,39 @@ fn given_wrong_slot_mappings_when_expanding_then_each_call_fails_with_its_messag expected_code: "P018", expected_message: "column \"region\" from macro rate_columns() clashes with a column already defined in model:orders_clean", }, + ExpansionErrorTestCase { + description: "a grouped column read through the relation beside its group in a column list", + adapter: "", + macros: &[( + "region_columns", + "MACRO (description \"Grain and region columns\", arguments (weekly RELATION (keys COLUMNS)));\n\nSELECT @weekly.keys, @weekly.region", + )], + sql: "SELECT @sql.region_columns(w, weekly.keys := (product_id, region)) FROM weekly_rates AS w", + expected_code: "P018", + expected_message: "column region of slot weekly.keys clashes with column region that macro region_columns() defines", + }, + ExpansionErrorTestCase { + description: "a grouped column read through the relation beside its group in a CTE", + adapter: "", + macros: &[( + "region_totals", + "MACRO (description \"Totals per grain with region\", arguments (weekly RELATION (keys COLUMNS)));\n\nWITH totals AS (SELECT @weekly.keys, @weekly.region FROM @weekly)\n", + )], + sql: "WITH w AS (SELECT 1 AS product_id, 'EU' AS region), @sql.region_totals(w, weekly.keys := (product_id, region)) SELECT * FROM totals", + expected_code: "P018", + expected_message: "column region of slot weekly.keys clashes with column region that macro region_totals() defines", + }, + ExpansionErrorTestCase { + description: "a mapped single-column slot read without AS in a LATERAL subquery", + adapter: "", + macros: &[( + "lateral_counts", + "MACRO (description \"Latest order count per customer\", arguments (weekly RELATION (order_count BIGINT)));\n\nWITH latest AS (\n SELECT c.customer_id, l.*\n FROM customers AS c,\n LATERAL (SELECT @weekly.order_count FROM @weekly WHERE @weekly.customer_id = c.customer_id) AS l\n)\n", + )], + sql: "WITH w AS (SELECT 1 AS customer_id, 2 AS orders_placed), @sql.lateral_counts(w, weekly.order_count := orders_placed) SELECT * FROM latest", + expected_code: "P017", + expected_message: "slot weekly.order_count of macro lateral_counts() is mapped to orders_placed, but the macro reads it in a select list without AS, so the output column's name would depend on the mapping", + }, ExpansionErrorTestCase { description: "a positional argument after a slot mapping", adapter: "", diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md index d8437e6e70..fa1c77ee06 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md @@ -389,12 +389,14 @@ error[P017]: slot weekly.order_count needs a BIGINT column; weekly has no column run on the expanded SQL. - Overlapping groups, or a grouped column that also fills a single-column slot, fail at the call. - A grouped column named like a column the macro defines, or like a bare column such as `s.week` - beside the group in the same select list, fails with `P018`, naming both: + or a column read through the relation such as `@weekly.region` beside the group in the same + select list, fails with `P018`, naming both: `column refunds of slot weekly.keys clashes with column refunds that macro weekly_rates() defines`. - A mapped single-column slot read in a select list needs an `AS` in the macro, so output names don't depend on the caller's mapping. Groups are exempt, because they keep their real names, and so is an unmapped slot, which keeps its own name. So are reads in `IN`, `EXISTS` and scalar - subqueries, whose column names never reach the macro's output. + subqueries, whose column names never reach the macro's output. A `LATERAL` subquery in `FROM` or + `JOIN` is checked like a derived table, because its columns reach the output, as through `l.*`. ## Compose macros diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_slot_scopes.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_slot_scopes.py index ed5a70a7fc..b5fc4d0a8b 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_slot_scopes.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_slot_scopes.py @@ -311,6 +311,42 @@ "macro listed_weeks() defines", ), ), + SqlMacroCompileTestCase( + description="relation_read_of_a_column_beside_a_group_holding_it", + files=scoped_model( + macro_file( + "listed_regions", + "weekly RELATION (keys COLUMNS)", + "WITH listed AS (SELECT @weekly.keys, @weekly.region FROM @weekly)", + ), + "@sql.listed_regions(weekly, weekly.keys := (product_id, region))\n" + "SELECT * FROM listed", + ), + expected_returncode=1, + expected_fragments=( + "error[P018]: column region of slot weekly.keys clashes with column region that " + "macro listed_regions() defines", + ), + ), + SqlMacroCompileTestCase( + description="mapped_slot_without_as_in_a_lateral_subquery", + files=scoped_model( + macro_file( + "lateral_orders", + "weekly RELATION (order_count BIGINT, week BIGINT), refunds RELATION (week BIGINT)", + "WITH latest AS (SELECT r.week, l.* FROM @refunds AS r,\n" + "LATERAL (SELECT w.@weekly.order_count FROM @weekly AS w " + "WHERE w.week = r.week) AS l)", + ), + "@sql.lateral_orders(weekly, refunds, weekly.order_count := orders_placed)\n" + "SELECT * FROM latest", + ), + expected_returncode=1, + expected_fragments=( + "error[P017]: slot weekly.order_count of macro lateral_orders() is mapped to " + "orders_placed, but the macro reads it in a select list without AS", + ), + ), ], ids=lambda case: case.description, ) diff --git a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx index 034cd9760f..7d87eff1b0 100644 --- a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx +++ b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx @@ -374,12 +374,14 @@ error[P017]: slot weekly.order_count needs a BIGINT column; weekly has no column run on the expanded SQL. - Overlapping groups, or a grouped column that also fills a single-column slot, fail at the call. - A grouped column named like a column the macro defines, or like a bare column such as `s.week` - beside the group in the same select list, fails with `P018`, naming both: + or a column read through the relation such as `@weekly.region` beside the group in the same + select list, fails with `P018`, naming both: `column refunds of slot weekly.keys clashes with column refunds that macro weekly_rates() defines`. - A mapped single-column slot read in a select list needs an `AS` in the macro, so output names don't depend on the caller's mapping. Groups are exempt, because they keep their real names, and so is an unmapped slot, which keeps its own name. So are reads in `IN`, `EXISTS` and scalar - subqueries, whose column names never reach the macro's output. + subqueries, whose column names never reach the macro's output. A `LATERAL` subquery in `FROM` or + `JOIN` is checked like a derived table, because its columns reach the output, as through `l.*`. ## Compose macros From 55ddb9945aef9931c474b3eb0864be02b67b7e47 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 26 Oct 2026 05:10:02 +0000 Subject: [PATCH 4/5] test(sql-macros): cover forwarded aliased reads of slots the outer does not declare A forwarded aliased slot read whose slot the outer macro's relation does not declare compiles and reads the column through the alias, without a P022 at the outer macro. --- .../sql_macros/test_sql_macro_slot_scopes.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_slot_scopes.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_slot_scopes.py index b5fc4d0a8b..62057a7b45 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_slot_scopes.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_slot_scopes.py @@ -108,6 +108,29 @@ unexpected_fragments=("weekly.product_id", "P022"), inspected_paths=_COMPILED, ), + SqlMacroCompileTestCase( + description="forwarded_aliased_read_of_a_slot_the_outer_does_not_declare", + files=scoped_model( + { + **macro_file( + "counted_weeks", + "r RELATION (order_count BIGINT, week BIGINT)", + "WITH counted AS (SELECT s.@r.week, s.@r.order_count AS orders " + "FROM @r AS s)", + ), + **macro_file( + "outer_counted", + "w RELATION (placed BIGINT)", + "WITH @sql.counted_weeks(r := @w, r.order_count := placed)", + ), + }, + "@sql.outer_counted(weekly, w.placed := orders_placed)\nSELECT * FROM counted", + ), + expected_returncode=0, + expected_fragments=("SELECT s.week, s.orders_placed AS orders FROM weekly AS s",), + unexpected_fragments=("P022",), + inspected_paths=_COMPILED, + ), SqlMacroCompileTestCase( description="forwarded_group_mapped_to_a_single_column_outer_slot", files=scoped_model( From 2893a1bc73c60da864b36545fe34432a0c3e3b2d Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 26 Oct 2026 06:40:05 +0000 Subject: [PATCH 5/5] test(goldens): record the compiler golden for the enum literal failure fixture --- .../failure/contract-enum-literal.json | 319 ++++++++++++++++++ 1 file changed, 319 insertions(+) create mode 100644 tests/goldens/compiler/failure/contract-enum-literal.json diff --git a/tests/goldens/compiler/failure/contract-enum-literal.json b/tests/goldens/compiler/failure/contract-enum-literal.json new file mode 100644 index 0000000000..d535e52379 --- /dev/null +++ b/tests/goldens/compiler/failure/contract-enum-literal.json @@ -0,0 +1,319 @@ +{ + "commands": [ + { + "label": "compile", + "exit_code": 1, + "diagnostics": [ + { + "phase": "contract", + "severity": "error", + "code": "K013", + "message": "value 'held' in column 'status' of model 'stg_orders' is not a member of enum order_status", + "resource_type": "model", + "resource_name": "stg_orders", + "column_name": "status", + "path": "models/staging/stg_orders.sql", + "line": 12, + "column": 39, + "location": { + "path": "models/staging/stg_orders.sql", + "line": 12, + "column": 39, + "end_line": 12, + "end_column": 45 + }, + "help": "add 'held' to enum order_status or use one of its members: 'placed', 'shipped'" + } + ] + } + ], + "compiled": { + "audits/generic/stg_orders/accepted_values__status.sql": [ + "SELECT status", + "FROM __ref(\"stg_orders\")", + "WHERE status IS NOT NULL", + " AND status NOT IN ('placed', 'shipped')", + "" + ], + "models/marts/customer_totals.sql": [ + "SELECT customer_id, SUM(amount) AS total_amount", + "FROM __ref(\"stg_orders\")", + "GROUP BY customer_id", + "" + ], + "models/staging/stg_orders.sql": [ + "SELECT order_id, customer_id, amount, 'held' AS status", + "FROM __source(\"raw_orders\")", + "" + ] + }, + "manifest": { + "nodes": { + "model.failure_corpus.customer_totals": { + "database": null, + "schema": "main", + "name": "customer_totals", + "resource_type": "model", + "package_name": "failure_corpus", + "path": "models/marts/customer_totals.sql", + "original_file_path": "models/marts/customer_totals.sql", + "unique_id": "model.failure_corpus.customer_totals", + "fqn": [ + "failure_corpus", + "models", + "marts", + "customer_totals" + ], + "alias": "customer_totals", + "config": { + "enabled": true, + "alias": null, + "schema": null, + "database": null, + "tags": [], + "meta": {}, + "group": null, + "materialized": "view", + "incremental_strategy": null, + "merge_exclude_columns": null, + "persist_docs": {}, + "post-hook": [], + "pre-hook": [], + "quoting": {}, + "column_types": {}, + "full_refresh": null, + "unique_key": null, + "on_schema_change": "ignore", + "on_configuration_change": "apply", + "grants": {}, + "packages": [], + "docs": { + "show": true, + "node_color": null + }, + "contract": { + "enforced": false, + "alias_types": true + } + }, + "tags": [], + "description": "Order totals per customer", + "columns": {}, + "meta": {}, + "group": null, + "docs": { + "show": true, + "node_color": null + }, + "patch_path": null, + "build_path": null, + "unrendered_config": {}, + "config_call_dict": {}, + "unrendered_config_call_dict": {}, + "relation_name": "main.customer_totals", + "depends_on": { + "macros": [], + "nodes": [ + "model.failure_corpus.stg_orders" + ] + } + }, + "model.failure_corpus.stg_orders": { + "database": null, + "schema": "main", + "name": "stg_orders", + "resource_type": "model", + "package_name": "failure_corpus", + "path": "models/staging/stg_orders.sql", + "original_file_path": "models/staging/stg_orders.sql", + "unique_id": "model.failure_corpus.stg_orders", + "fqn": [ + "failure_corpus", + "models", + "staging", + "stg_orders" + ], + "alias": "stg_orders", + "config": { + "enabled": true, + "alias": null, + "schema": null, + "database": null, + "tags": [], + "meta": {}, + "group": null, + "materialized": "view", + "incremental_strategy": null, + "merge_exclude_columns": null, + "persist_docs": {}, + "post-hook": [], + "pre-hook": [], + "quoting": {}, + "column_types": {}, + "full_refresh": null, + "unique_key": null, + "on_schema_change": "ignore", + "on_configuration_change": "apply", + "grants": {}, + "packages": [], + "docs": { + "show": true, + "node_color": null + }, + "contract": { + "enforced": false, + "alias_types": true + } + }, + "tags": [], + "description": "Staged orders", + "columns": { + "order_id": { + "name": "order_id", + "description": "", + "meta": {}, + "data_type": "INTEGER", + "constraints": [], + "quote": null, + "tags": [] + }, + "customer_id": { + "name": "customer_id", + "description": "", + "meta": {}, + "data_type": "INTEGER", + "constraints": [], + "quote": null, + "tags": [] + }, + "amount": { + "name": "amount", + "description": "", + "meta": {}, + "data_type": "DOUBLE", + "constraints": [], + "quote": null, + "tags": [] + }, + "status": { + "name": "status", + "description": "", + "meta": {}, + "data_type": "VARCHAR", + "constraints": [], + "quote": null, + "tags": [] + } + }, + "meta": {}, + "group": null, + "docs": { + "show": true, + "node_color": null + }, + "patch_path": null, + "build_path": null, + "unrendered_config": {}, + "config_call_dict": {}, + "unrendered_config_call_dict": {}, + "relation_name": "main.stg_orders", + "depends_on": { + "macros": [], + "nodes": [ + "source.failure_corpus.raw_orders" + ] + } + } + }, + "sources": { + "source.failure_corpus.raw_orders": { + "database": null, + "schema": "", + "name": "raw_orders", + "resource_type": "source", + "package_name": "failure_corpus", + "path": "sources/raw.yml", + "original_file_path": "sources/raw.yml", + "unique_id": "source.failure_corpus.raw_orders", + "fqn": [ + "failure_corpus", + "raw_orders" + ], + "source_name": "raw_orders", + "source_description": "", + "loader": "", + "identifier": "raw_orders", + "quoting": {}, + "loaded_at_field": null, + "freshness": null, + "external": null, + "description": "Orders feed.", + "columns": { + "order_id": { + "name": "order_id", + "description": "", + "meta": {}, + "data_type": "INTEGER", + "constraints": [], + "quote": null, + "tags": [] + }, + "customer_id": { + "name": "customer_id", + "description": "", + "meta": {}, + "data_type": "INTEGER", + "constraints": [], + "quote": null, + "tags": [] + }, + "amount": { + "name": "amount", + "description": "", + "meta": {}, + "data_type": "DOUBLE", + "constraints": [], + "quote": null, + "tags": [] + }, + "status": { + "name": "status", + "description": "", + "meta": {}, + "data_type": "VARCHAR", + "constraints": [], + "quote": null, + "tags": [] + } + }, + "meta": {}, + "source_meta": {}, + "tags": [], + "config": {}, + "patch_path": null, + "unrendered_config": {}, + "relation_name": null + } + }, + "macros": {}, + "exposures": {}, + "metrics": {}, + "groups": {}, + "selectors": {}, + "disabled": {}, + "parent_map": { + "model.failure_corpus.customer_totals": [ + "model.failure_corpus.stg_orders" + ], + "model.failure_corpus.stg_orders": [ + "source.failure_corpus.raw_orders" + ], + "source.failure_corpus.raw_orders": [] + }, + "group_map": {}, + "docs": {}, + "saved_queries": {}, + "semantic_models": {}, + "unit_tests": {} + } +}