diff --git a/README.md b/README.md index 79dc572..f477cb2 100644 --- a/README.md +++ b/README.md @@ -85,56 +85,45 @@ SQLClean in other configurations, on the same files: ## Custom Rules in Python A custom Rule is a Python function over each statement, already parsed and located by the Rust -engine. This one enforces a team convention, not a rule of SQL: the columns the team lists in -`EMAIL_COLUMNS` are compared case-insensitively. An `=` or `<>` that reads one of them, bare or as -`LOWER(email)`, passes only when both sides are `LOWER(...)` or a lower-case string literal. It -reads comparisons from the statement's syntax tree, so comments and string literals never match, -and it only reports, because changing a comparison is the author's call. +engine. This team uses `COALESCE` instead of `IFNULL` to keep queries portable. The Rule reads +function facts, not SQL text, so comments and string literals don't match. It only reports; +the author chooses the replacement. ```python # sql_rules/team.py -import polyglot_sql as ast # pip install "sqlclean[ast]" - from sqlclean import Finding, RuleContext, Statement, rule -EMAIL_COLUMNS = {"email", "billing_email"} # the columns this team treats as email addresses - - -def email_column(operand: ast.Expression) -> ast.Column | None: - """The designated email column an operand reads, written bare or as LOWER(column).""" - column = operand.this if isinstance(operand, ast.Lower) else operand - is_email = isinstance(column, ast.Column) and column.name.lower() in EMAIL_COLUMNS - return column if is_email else None - - -def lowered(operand: ast.Expression) -> bool: - """Whether an operand is LOWER(...) or a string literal with no upper-case letters.""" - literal = isinstance(operand, ast.Literal) and operand.is_string - return isinstance(operand, ast.Lower) or (literal and operand.name == operand.name.lower()) - @rule( code="XSQLCLEANTEAM001", - message="Email comparisons must be case-insensitive", - remediation="Compare LOWER(email) with LOWER(...) instead.", + message="Use COALESCE instead of IFNULL", + remediation="Write COALESCE(value, fallback).", enabled_by_default=True, ) -def case_insensitive_email(*, statement: Statement, ctx: RuleContext) -> list[Finding]: - starts = { - column.to_dict()["column"]["span"]["start"] - for comparison in statement.polyglot_ast().find_all(ast.Eq, ast.Neq) - for column in map(email_column, (comparison.this, comparison.expression)) - if column is not None and not all(map(lowered, (comparison.this, comparison.expression))) - } +def prefer_coalesce(*, statement: Statement, ctx: RuleContext) -> list[Finding]: return [ - ctx.finding(subject=statement, node=column) - for column in statement.columns - if column.start - statement.start in starts + ctx.finding(subject=statement, node=function) + for function in statement.functions + if function.name.upper() == "IFNULL" ] ``` -Point `[rules] custom = ["sql_rules"]` in `sqlclean.toml` at it. The syntax tree needs the `ast` -extra: `pip install "sqlclean[ast]"`. See +Point `[rules] custom = ["sql_rules"]` in `sqlclean.toml` at it, then run `sqlclean check`: + +```sql +SELECT IFNULL(discount, 0) FROM orders; +``` + +```text +error[XSQLCLEANTEAM001]: Use COALESCE instead of IFNULL + --> orders.sql:1:8 + | + 1 | SELECT IFNULL(discount, 0) FROM orders; + | ^^^^^^ + = help: Write COALESCE(value, fallback). +``` + +No extra dependencies are needed. See [custom Rules](https://sqlclean.io/docs/custom-rules/overview/), [statement facts](https://sqlclean.io/docs/custom-rules/statement-facts/) and [testing custom Rules](https://sqlclean.io/docs/custom-rules/testing/). diff --git a/crates/sqlclean-cli/skill/references/docs/custom-rules/overview.md b/crates/sqlclean-cli/skill/references/docs/custom-rules/overview.md index de3a62d..e53d762 100644 --- a/crates/sqlclean-cli/skill/references/docs/custom-rules/overview.md +++ b/crates/sqlclean-cli/skill/references/docs/custom-rules/overview.md @@ -21,21 +21,25 @@ A custom Rule is a Python function decorated with `@rule`. It receives a stateme whole repository, and returns findings. Custom Rules use the same selection, exceptions, fixes, output and cache as the built-in Rules. +This team uses `COALESCE` instead of `IFNULL` to keep queries portable. The Rule reads function +facts, so comments and string literals don't match. It only reports, leaving the replacement to +the author. No extra dependencies are needed. + ```python -# sql_rules/naming.py +# sql_rules/team.py from sqlclean import Finding, RuleContext, Statement, rule @rule( - code="XSQLCLEANNAME001", - message="Query reads a temporary table", - remediation="Read from the published table instead.", + code="XSQLCLEANTEAM001", + message="Use COALESCE instead of IFNULL", + remediation="Write COALESCE(value, fallback).", enabled_by_default=True, ) -def temporary_table(*, statement: Statement, ctx: RuleContext) -> list[Finding]: +def prefer_coalesce(*, statement: Statement, ctx: RuleContext) -> list[Finding]: return [ - ctx.finding(subject=statement, node=table) - for table in statement.tables - if table.name.startswith("tmp_") + ctx.finding(subject=statement, node=function) + for function in statement.functions + if function.name.upper() == "IFNULL" ] ``` @@ -46,15 +50,23 @@ Point `[rules] custom` at the file or its directory: custom = ["sql_rules"] ``` +Run `sqlclean check` on `orders.sql`: + +```sql +SELECT IFNULL(discount, 0) FROM orders; +``` + ```text -error[XSQLCLEANNAME001]: Query reads a temporary table - --> models/orders.sql:2:6 +error[XSQLCLEANTEAM001]: Use COALESCE instead of IFNULL + --> orders.sql:1:8 | - 2 | FROM tmp_orders - | ^^^^^^^^^^ - = help: Read from the published table instead. + 1 | SELECT IFNULL(discount, 0) FROM orders; + | ^^^^^^ + = help: Write COALESCE(value, fallback). ``` +Write `SELECT COALESCE(discount, 0) FROM orders;` to satisfy this convention. + ## Where Rules live `[rules] custom` lists Python files or directories, relative to the configuration root. SQLClean @@ -63,8 +75,8 @@ functions, constants and classes stay ordinary Python. - Files named `test_*.py`, `conftest.py` and `__init__.py` are not loaded as Rules, so tests can live next to the Rules they test. -- Modules are imported by their path from the configuration root: `sql_rules/naming.py` is - `sql_rules.naming`. A Rule module can import a helper the same way, for example +- Modules are imported by their path from the configuration root: `sql_rules/team.py` is + `sql_rules.team`. A Rule module can import a helper the same way, for example `from sql_rules.helpers import table_prefixes`. - Two Rules with the same code are an error: `custom Rule code XSQLCLEANNAME001 is defined twice`. diff --git a/pyproject.toml b/pyproject.toml index 8e95e97..954000e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,7 +42,7 @@ packages = ["src/sqlclean"] dev = [ "commitizen>=4.9.1", "fensu>=0.28.0", - "polyglot-sql-chio==0.13.10", # the ast extra, for the README custom Rule test + "polyglot-sql-chio==0.13.10", # optional AST API "pre-commit>=4.3.0", "pytest>=8.4.2", "pytest-xdist>=3.8.0", diff --git a/tests/integration/src/sqlclean/rule_testing/_test_types.py b/tests/integration/src/sqlclean/rule_testing/_test_types.py index 195a0f2..6b20016 100644 --- a/tests/integration/src/sqlclean/rule_testing/_test_types.py +++ b/tests/integration/src/sqlclean/rule_testing/_test_types.py @@ -30,7 +30,7 @@ class HarnessFailureTestCase: @dataclass(frozen=True) class ReadmeRuleTestCase: - """SQL for the README custom Rule and the text of every column it must report.""" + """SQL for the introductory custom Rule and the text of every function it must report.""" description: str source: str diff --git a/tests/integration/src/sqlclean/rule_testing/helpers.py b/tests/integration/src/sqlclean/rule_testing/helpers.py index 2f8ae80..5a44191 100644 --- a/tests/integration/src/sqlclean/rule_testing/helpers.py +++ b/tests/integration/src/sqlclean/rule_testing/helpers.py @@ -3,10 +3,14 @@ from __future__ import annotations import importlib.util +import re +from html import unescape from importlib.machinery import ModuleSpec from pathlib import Path from types import ModuleType +import pytest + from sqlclean.rules.types import RuleCheck SAMPLE_RULES: str = """ @@ -106,19 +110,32 @@ def sample_rule(directory: Path, name: str) -> RuleCheck: README: Path = Path(__file__).resolve().parents[5] / "README.md" -BLOCK_START: str = "```python\n# sql_rules/team.py\n" +INTRODUCTIONS: tuple[Path, ...] = ( + README, + README.parent / "website/public/index.html", + README.parent / "website/src/content/docs/docs/custom-rules/overview.mdx", +) +BLOCK_START: str = "# sql_rules/team.py\n" + + +@pytest.fixture(params=INTRODUCTIONS, ids=lambda path: path.name) +def document(request: pytest.FixtureRequest) -> Path: + """Run each case against every published introductory snippet.""" + + path: Path = request.param + return path -def readme_rule(directory: Path) -> RuleCheck: - """Return the README's case_insensitive_email Rule, imported from its code block.""" +def readme_rule(directory: Path, document: Path = README) -> RuleCheck: + """Import prefer_coalesce from an introductory example, removing HTML markup if needed.""" - text: str = README.read_text(encoding="utf-8") + text: str = unescape(re.sub(r"]*>", "", document.read_text(encoding="utf-8"))) start: int = text.index(BLOCK_START) + len(BLOCK_START) path: Path = directory / "readme_team.py" - path.write_text(text[start : text.index("```", start)], encoding="utf-8") + path.write_text(text[start:].split("```")[0].split("# sqlclean.toml:")[0], encoding="utf-8") spec: ModuleSpec | None = importlib.util.spec_from_file_location("readme_team", path) assert spec is not None and spec.loader is not None module: ModuleType = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) - check: RuleCheck = module.case_insensitive_email + check: RuleCheck = module.prefer_coalesce return check diff --git a/tests/integration/src/sqlclean/rule_testing/test_readme_rule.py b/tests/integration/src/sqlclean/rule_testing/test_readme_rule.py index 1b2c950..7d5874c 100644 --- a/tests/integration/src/sqlclean/rule_testing/test_readme_rule.py +++ b/tests/integration/src/sqlclean/rule_testing/test_readme_rule.py @@ -1,4 +1,4 @@ -"""The custom Rule example in README.md, run as written through the Rule harness.""" +"""The introductory custom Rule examples, run as written through the Rule harness.""" from __future__ import annotations @@ -9,6 +9,7 @@ from sqlclean.rules.types import RuleCheck from sqlclean.testing import RuleCase, RuleResult, evaluate_rule from tests.integration.src.sqlclean.rule_testing._test_types import ReadmeRuleTestCase +from tests.integration.src.sqlclean.rule_testing.helpers import document as document from tests.integration.src.sqlclean.rule_testing.helpers import readme_rule @@ -16,59 +17,40 @@ "test_case", [ ReadmeRuleTestCase( - description="a bare email column compared with a mixed-case literal", - source="SELECT id FROM customers c WHERE c.email = 'Alice@example.com'", - expected_texts=("c.email",), + description="the documented SQL reports the function name", + source="SELECT IFNULL(discount, 0) FROM orders;", + expected_texts=("IFNULL",), ), ReadmeRuleTestCase( - description="LOWER on one side only still compares with a mixed-case literal", - source="SELECT id FROM customers WHERE LOWER(email) = 'Alice@Example.com'", - expected_texts=("email",), + description="mixed-case calls nested in a CTE and subquery are reported", + source="WITH totals AS (SELECT ifnull(discount, 0) AS discount FROM orders) " + "SELECT (SELECT IfNull(MAX(discount), 0) FROM totals)", + expected_texts=("ifnull", "IfNull"), ), ReadmeRuleTestCase( - description="a designated column on the right of <> in upper case", - source="SELECT id FROM customers WHERE 'Carol@example.com' <> BILLING_EMAIL", - expected_texts=("BILLING_EMAIL",), + description="a later statement after a multibyte literal uses file character offsets", + source="SELECT 'é';\nSELECT IFNULL(discount, 0) FROM orders;", + expected_texts=("IFNULL",), ), ReadmeRuleTestCase( - description="UPPER does not normalize the other side to lower case", - source="SELECT id FROM customers WHERE LOWER(email) = UPPER('x')", - expected_texts=("email",), - ), - ReadmeRuleTestCase( - description="a bare column against a lower-case literal is still not normalized", - source="SELECT id FROM customers WHERE email = 'carol@example.com'", - expected_texts=("email",), - ), - ReadmeRuleTestCase( - description="a comparison inside a CTE after a multibyte literal", - source="WITH u AS (SELECT 'é' AS m, email FROM users WHERE email = 'X@y.com') " - "SELECT * FROM u", - expected_texts=("email",), - ), - ReadmeRuleTestCase( - description="LOWER on both sides passes", - source="SELECT * FROM u JOIN a ON LOWER(u.email) = LOWER(a.email)", - expected_texts=(), - ), - ReadmeRuleTestCase( - description="LOWER against a lower-case literal passes", - source="SELECT id FROM customers WHERE LOWER(email) = 'bob@example.com'", + description="the recommended COALESCE replacement passes", + source="SELECT COALESCE(discount, 0) FROM orders;", expected_texts=(), ), ReadmeRuleTestCase( - description="comments, strings, other columns, LIKE and projections never match", - source="-- email = 'A@b.c'\nSELECT email, 'email = x' AS note FROM customers " - "WHERE email_verified = TRUE AND email LIKE '%dave%'", + description="comments, strings, identifiers and longer function names never match", + source="-- IFNULL(discount, 0)\n" + "SELECT 'IFNULL(discount, 0)' AS note, \"IFNULL\", " + "my_ifnull(discount, 0) FROM orders /* IFNULL(discount, 0) */", expected_texts=(), ), ], ids=lambda case: case.description, ) -def test_given_readme_email_rule_when_evaluating_then_unnormalized_comparisons_are_reported( - tmp_path: Path, test_case: ReadmeRuleTestCase +def test_given_introductory_rule_when_evaluating_then_only_ifnull_calls_are_reported( + tmp_path: Path, document: Path, test_case: ReadmeRuleTestCase ) -> None: - rule: RuleCheck = readme_rule(tmp_path) + rule: RuleCheck = readme_rule(tmp_path, document) result: RuleResult = evaluate_rule( rule=rule, @@ -84,4 +66,5 @@ def test_given_readme_email_rule_when_evaluating_then_unnormalized_comparisons_a ) assert texts == test_case.expected_texts + assert all(finding.edits == () for finding in result.findings) assert result.fixed_source == test_case.source diff --git a/website/public/index.html b/website/public/index.html index dead8ae..a450ba2 100644 --- a/website/public/index.html +++ b/website/public/index.html @@ -116,7 +116,7 @@

Style eats review time

Team rules live in people's heads

-

"Marts read staging, not raw" holds until someone new misses it.

+

"Use COALESCE, not IFNULL" holds until someone new misses it.

Autofixes you can't trust

@@ -246,35 +246,40 @@

Stop arguing about layout

04 · Make lessons permanent

Turn review comments into Rules

- A custom Rule is a Python function over the tables, joins, columns and literals of each statement, - already parsed and located for you. No regular expressions, no parser to learn. Here marts must read - staging models, never raw tables. + A custom Rule is a Python function over statement facts, already parsed and located for you. + This team uses COALESCE instead of IFNULL to keep queries portable. The Rule reads function calls, + not comments or strings. No extra dependencies, no automatic rewrite: the author chooses the replacement.

Write a custom Rule →

-
# sql_rules/layers.py
+
# sql_rules/team.py
+from sqlclean import Finding, RuleContext, Statement, rule
+
+
 @rule(
-    code="XSQLCLEANLAYER001",
-    message="Marts must not read raw tables",
-    remediation="Read the staging model instead.",
+    code="XSQLCLEANTEAM001",
+    message="Use COALESCE instead of IFNULL",
+    remediation="Write COALESCE(value, fallback).",
     enabled_by_default=True,
 )
-def marts_read_staging(*, statement: Statement, ctx: RuleContext) -> list:
-    if not statement.path.startswith("models/marts/"):
-        return []
+def prefer_coalesce(*, statement: Statement, ctx: RuleContext) -> list[Finding]:
     return [
-        ctx.finding(subject=statement, node=table)
-        for table in statement.tables
-        if table.qualified_name.startswith("raw.")
+        ctx.finding(subject=statement, node=function)
+        for function in statement.functions
+        if function.name.upper() == "IFNULL"
     ]
 
+# sqlclean.toml: [rules] custom = ["sql_rules"]
+# orders.sql
+SELECT IFNULL(discount, 0) FROM orders;
+
 sqlclean check
-error[XSQLCLEANLAYER001]: Marts must not read raw tables
-  --> models/marts/revenue.sql:5:12
+error[XSQLCLEANTEAM001]: Use COALESCE instead of IFNULL
+  --> orders.sql:1:8
    |
- 5 | INNER JOIN raw.payments p
-   |            ^^^^^^^^^^^^
-  = help: Read the staging model instead.

+ 1 | SELECT IFNULL(discount, 0) FROM orders;
+   |        ^^^^^^
+  = help: Write COALESCE(value, fallback).
@@ -288,24 +293,24 @@

Rules with tests, like any other code

Test a custom Rule →

-
# tests/test_layers.py
+
# tests/test_team.py
+from sql_rules.team import prefer_coalesce
 from sqlclean.testing import RuleCase, evaluate_rule
 
-def test_marts_reading_raw_tables_are_reported() -> None:
+
+def test_ifnull_is_reported() -> None:
     evaluate_rule(
-        rule=marts_read_staging,
+        rule=prefer_coalesce,
         test_case=RuleCase(
-            description="a mart joins raw.payments",
-            path="models/marts/revenue.sql",
-            source="SELECT o.order_id FROM stg_orders o JOIN raw.payments p ON …",
-            dialect="duckdb",
+            description="prefer portable null handling",
+            source="SELECT IFNULL(discount, 0) FROM orders;",
             expected_finding_count=1,
         ),
     )
 
 pytest tests
-..                                                    [100%]
-2 passed in 0.04s

+.                                                     [100%]
+1 passed
@@ -362,8 +367,8 @@

Before every commit and in CI

"runs": [{ "tool": { "driver": { "name": "sqlclean", … } }, "results": [{ - "ruleId": "XSQLCLEANLAYER001", - "message": { "text": "Marts must not read raw tables. Read the staging model instead." }, + "ruleId": "XSQLCLEANTEAM001", + "message": { "text": "Use COALESCE instead of IFNULL. Write COALESCE(value, fallback)." }, …
diff --git a/website/src/content/docs/docs/custom-rules/overview.mdx b/website/src/content/docs/docs/custom-rules/overview.mdx index 5087a6f..a0e48f7 100644 --- a/website/src/content/docs/docs/custom-rules/overview.mdx +++ b/website/src/content/docs/docs/custom-rules/overview.mdx @@ -10,22 +10,26 @@ A custom Rule is a Python function decorated with `@rule`. It receives a stateme whole repository, and returns findings. Custom Rules use the same selection, exceptions, fixes, output and cache as the built-in Rules. +This team uses `COALESCE` instead of `IFNULL` to keep queries portable. The Rule reads function +facts, so comments and string literals don't match. It only reports, leaving the replacement to +the author. No extra dependencies are needed. + ```python -# sql_rules/naming.py +# sql_rules/team.py from sqlclean import Finding, RuleContext, Statement, rule @rule( - code="XSQLCLEANNAME001", - message="Query reads a temporary table", - remediation="Read from the published table instead.", + code="XSQLCLEANTEAM001", + message="Use COALESCE instead of IFNULL", + remediation="Write COALESCE(value, fallback).", enabled_by_default=True, ) -def temporary_table(*, statement: Statement, ctx: RuleContext) -> list[Finding]: +def prefer_coalesce(*, statement: Statement, ctx: RuleContext) -> list[Finding]: return [ - ctx.finding(subject=statement, node=table) - for table in statement.tables - if table.name.startswith("tmp_") + ctx.finding(subject=statement, node=function) + for function in statement.functions + if function.name.upper() == "IFNULL" ] ``` @@ -36,15 +40,23 @@ Point `[rules] custom` at the file or its directory: custom = ["sql_rules"] ``` +Run `sqlclean check` on `orders.sql`: + +```sql +SELECT IFNULL(discount, 0) FROM orders; +``` + ```text -error[XSQLCLEANNAME001]: Query reads a temporary table - --> models/orders.sql:2:6 +error[XSQLCLEANTEAM001]: Use COALESCE instead of IFNULL + --> orders.sql:1:8 | - 2 | FROM tmp_orders - | ^^^^^^^^^^ - = help: Read from the published table instead. + 1 | SELECT IFNULL(discount, 0) FROM orders; + | ^^^^^^ + = help: Write COALESCE(value, fallback). ``` +Write `SELECT COALESCE(discount, 0) FROM orders;` to satisfy this convention. + ## Where Rules live `[rules] custom` lists Python files or directories, relative to the configuration root. SQLClean @@ -53,8 +65,8 @@ functions, constants and classes stay ordinary Python. - Files named `test_*.py`, `conftest.py` and `__init__.py` are not loaded as Rules, so tests can live next to the Rules they test. -- Modules are imported by their path from the configuration root: `sql_rules/naming.py` is - `sql_rules.naming`. A Rule module can import a helper the same way, for example +- Modules are imported by their path from the configuration root: `sql_rules/team.py` is + `sql_rules.team`. A Rule module can import a helper the same way, for example `from sql_rules.helpers import table_prefixes`. - Two Rules with the same code are an error: `custom Rule code XSQLCLEANNAME001 is defined twice`.