diff --git a/docs/static_analysis.md b/docs/static_analysis.md index 4162bd6..0cee514 100644 --- a/docs/static_analysis.md +++ b/docs/static_analysis.md @@ -47,6 +47,38 @@ Scans every Python file inside any `report/` directory in the app (including nes - Not decorated with `@frappe.whitelist()` - Not called directly by `execute()` +### nullable_filters + +Scans `frappe.get_all` / `get_list` / `get_count` / `frappe.db.exists` calls for a list-form filter clause that compares a nullable `Date`, `Datetime`, `Time`, or `Data` field with a range operator (`<`, `>`, `<=`, `>=`) — cross-referenced against the DocType JSON to confirm the field is not `reqd` and has no `default`. + +Frappe's query builder wraps filters on nullable columns in `ifnull(col, '')`. For a comparison operator this makes `''` a valid operand — `'' < '2026-01-01'` is string-true on both MariaDB and Postgres — so a filter like `["due_date", "<", nowdate()]` silently matches rows where `due_date` is `NULL`, not just rows before the date. + +Warns unless the same field also has an `["field", "is", "set"]` (or `"not set"`) clause in the same filter list — Frappe's list-form filters allow two conditions on one fieldname, which is the fix: + +```python +# Warns: a NULL grace_period_deadline reads as '' and '' < now is string-true, +# so not-yet-due rows are swept up as already expired. +frappe.get_all( + "Remote Cloud Bench Purchase", + filters=[ + ["purchase_status", "=", "Past Due"], + ["grace_period_deadline", "<", now], + ], +) + +# Clean: the "is set" guard excludes NULL rows before the comparison runs. +frappe.get_all( + "Remote Cloud Bench Purchase", + filters=[ + ["purchase_status", "=", "Past Due"], + ["grace_period_deadline", "is", "set"], + ["grace_period_deadline", "<", now], + ], +) +``` + +This is always a warning, never a hard error — a caller may have already excluded NULLs earlier in the same request (e.g. via a preceding non-range filter that's mutually exclusive with NULL), and the check doesn't attempt to prove that. + ### orphans Runs [Vulture](https://github.com/jendrikseipp/vulture) against the app to detect unused imports, variables, and functions. Before running, the analyzer seeds a Vulture whitelist with all discovered entry points (whitelisted functions, hooks paths, doctype controllers) so they are not incorrectly flagged. diff --git a/test_utils/pre_commit/static_analysis.py b/test_utils/pre_commit/static_analysis.py index a10133f..348de70 100644 --- a/test_utils/pre_commit/static_analysis.py +++ b/test_utils/pre_commit/static_analysis.py @@ -41,6 +41,7 @@ def _run(args: argparse.Namespace) -> int: validate_python_calls=not args.no_python_calls, validate_jinja=not args.no_jinja, validate_reports=not args.no_reports, + validate_nullable_filters=not args.no_nullable_filters, detect_orphans=not args.no_orphans, min_confidence=args.min_confidence, ) @@ -124,6 +125,16 @@ def _run(args: argparse.Namespace) -> int: else: print(f"\n[reports] {label} — OK") + # Nullable filters + if result.nullable_filter_result is not None: + nfr = result.nullable_filter_result + label = f"Nullable filters ({nfr.calls_checked} calls checked)" + if nfr.warnings: + any_output = True + _print_section(f"[nullable_filters] {label}", nfr.warnings, prefix=" WARN: ") + else: + print(f"\n[nullable_filters] {label} — OK") + # Orphans if result.orphan_result is not None: orph = result.orphan_result @@ -207,6 +218,13 @@ def main(argv: Sequence[str] | None = None) -> None: default=False, help="Skip report directory function validation", ) + parser.add_argument( + "--no-nullable-filters", + action="store_true", + default=False, + dest="no_nullable_filters", + help="Skip nullable Date/Datetime/Data range-filter validation", + ) parser.add_argument( "--no-orphans", action="store_true", diff --git a/test_utils/utils/static_analysis/__init__.py b/test_utils/utils/static_analysis/__init__.py index c424d1d..7abfee5 100644 --- a/test_utils/utils/static_analysis/__init__.py +++ b/test_utils/utils/static_analysis/__init__.py @@ -21,6 +21,7 @@ from .frontend_validator import FrontendValidationResult, FrontendValidator from .hooks_validator import HooksValidationResult, HooksValidator from .jinja_validator import JinjaValidationResult, JinjaValidator +from .nullable_filter_validator import NullableFilterValidationResult, NullableFilterValidator from .orphan_detector import OrphanDetector, OrphanResult from .patches_validator import PatchesValidationResult, PatchesValidator from .path_resolver import PathResolver @@ -43,6 +44,7 @@ class StaticAnalysisConfig: validate_python_calls: bool = True validate_jinja: bool = True validate_reports: bool = True + validate_nullable_filters: bool = True detect_orphans: bool = True min_confidence: int = 80 ignore_patterns: list[str] = field(default_factory=list) @@ -58,6 +60,7 @@ class StaticAnalysisResult: python_call_result: PythonCallValidationResult | None = None jinja_result: JinjaValidationResult | None = None report_result: ReportValidationResult | None = None + nullable_filter_result: NullableFilterValidationResult | None = None orphan_result: OrphanResult | None = None @property @@ -104,6 +107,7 @@ def all_warnings(self) -> list[str]: self.python_call_result, self.jinja_result, self.report_result, + self.nullable_filter_result, ): if r is not None: msgs.extend(r.warnings) @@ -122,6 +126,7 @@ def r(obj) -> dict | None: "python_calls": r(self.python_call_result), "jinja": r(self.jinja_result), "reports": r(self.report_result), + "nullable_filters": r(self.nullable_filter_result), "orphans": r(self.orphan_result), } @@ -301,6 +306,9 @@ def analyze(self) -> StaticAnalysisResult: if self.config.validate_reports: result.report_result = ReportValidator(self.app_path).validate() + if self.config.validate_nullable_filters: + result.nullable_filter_result = NullableFilterValidator().validate(self.app_path) + if self.config.detect_orphans: from .hooks_validator import PathExtractor diff --git a/test_utils/utils/static_analysis/nullable_filter_validator.py b/test_utils/utils/static_analysis/nullable_filter_validator.py new file mode 100644 index 0000000..4b000ce --- /dev/null +++ b/test_utils/utils/static_analysis/nullable_filter_validator.py @@ -0,0 +1,161 @@ +""" +Flag range filters on nullable Date/Datetime/Time/Data fields with no "is set" guard. + +Frappe wraps filters on nullable columns in ``ifnull(col, '')``. That makes +``''`` a valid operand for comparison operators — ``'' < '2026-01-01'`` is +string-true on both MariaDB and Postgres — so ``["due_date", "<", nowdate()]`` +also matches rows where ``due_date`` is NULL. Pairing the comparison with +``["field", "is", "set"]`` (Frappe allows two conditions per fieldname) avoids it. +""" + +import ast +import json +from dataclasses import dataclass, field +from pathlib import Path + +RANGE_OPS = frozenset({"<", ">", "<=", ">="}) +NULLABLE_FIELDTYPES = frozenset({"Date", "Datetime", "Data", "Time"}) + +GET_ALL_FUNCS = frozenset({"get_all", "get_list", "get_count", "exists"}) + + +@dataclass +class NullableFilterValidationResult: + errors: list[str] = field(default_factory=list) + warnings: list[str] = field(default_factory=list) + calls_checked: int = 0 + + def to_dict(self) -> dict: + return { + "errors": self.errors, + "warnings": self.warnings, + "calls_checked": self.calls_checked, + } + + +def _load_nullable_fields(app_path: Path) -> dict[str, set[str]]: + """Map DocType name -> set of non-mandatory Date/Datetime/Data/Time fieldnames.""" + fields_by_doctype: dict[str, set[str]] = {} + for json_file in app_path.rglob("*.json"): + if "doctype" not in json_file.parts: + continue + try: + data = json.loads(json_file.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + continue + if data.get("doctype") != "DocType" or "fields" not in data: + continue + name = data.get("name") or json_file.stem + nullable = { + f["fieldname"] + for f in data["fields"] + if f.get("fieldtype") in NULLABLE_FIELDTYPES and not f.get("reqd") and not f.get("default") + } + if nullable: + fields_by_doctype[name] = nullable + return fields_by_doctype + + +def _filter_entries(node: ast.AST) -> list[tuple[ast.AST, int]]: + """Extract literal filter clauses (as AST list/tuple nodes) from a filters argument.""" + entries: list[tuple[ast.AST, int]] = [] + if isinstance(node, (ast.List, ast.Tuple)): + # Either a single ["field", "op", value] clause or a list of clauses. + if node.elts and all(isinstance(e, ast.Constant) for e in node.elts[:2]): + entries.append((node, node.lineno)) + else: + for elt in node.elts: + entries.extend(_filter_entries(elt)) + return entries + + +def _clause_field_and_op(clause: ast.AST) -> tuple[str, str] | None: + if not isinstance(clause, (ast.List, ast.Tuple)): + return None + elts = clause.elts + if len(elts) == 3 and all(isinstance(e, ast.Constant) for e in (elts[0], elts[1])): + return elts[0].value, elts[1].value + return None + + +class NullableFilterValidator: + def validate(self, app_path: Path) -> NullableFilterValidationResult: + result = NullableFilterValidationResult() + nullable_fields = _load_nullable_fields(app_path) + if not nullable_fields: + return result + + for py_file in app_path.rglob("*.py"): + if any(p in py_file.parts for p in ("__pycache__", "node_modules", "test", "tests")): + continue + try: + source = py_file.read_text(encoding="utf-8", errors="replace") + except OSError: + continue + try: + tree = ast.parse(source, filename=str(py_file)) + except SyntaxError: + continue + + lines = source.splitlines() + + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + func_name = func.attr if isinstance(func, ast.Attribute) else getattr(func, "id", None) + if func_name not in GET_ALL_FUNCS: + continue + + # doctype is first positional arg or `doctype=` kwarg. + doctype = None + if node.args and isinstance(node.args[0], ast.Constant): + doctype = node.args[0].value + else: + for kw in node.keywords: + if kw.arg == "doctype" and isinstance(kw.value, ast.Constant): + doctype = kw.value.value + if doctype not in nullable_fields: + continue + + # filters is the 2nd positional arg or `filters=`/`filter=` kwarg. + filters_node = None + if len(node.args) > 1: + filters_node = node.args[1] + else: + for kw in node.keywords: + if kw.arg in ("filters", "filter"): + filters_node = kw.value + if filters_node is None: + continue + + clauses = _filter_entries(filters_node) + fields_with_range: dict[str, int] = {} + fields_with_guard: set[str] = set() + for clause, lineno in clauses: + parsed = _clause_field_and_op(clause) + if not parsed: + continue + fname, op = parsed + if fname not in nullable_fields[doctype]: + continue + if op in RANGE_OPS: + fields_with_range.setdefault(fname, lineno) + elif op == "is": + fields_with_guard.add(fname) + + result.calls_checked += 1 + for fname, lineno in fields_with_range.items(): + if fname in fields_with_guard: + continue + line_text = lines[lineno - 1] if 0 < lineno <= len(lines) else "" + if "frappe-vulture:ignore" in line_text: + continue + result.warnings.append( + f"{py_file}:{lineno}: {doctype}.{fname} compared with a range operator " + f"but not guarded with [\"{fname}\", \"is\", \"set\"] — Frappe wraps " + f"nullable columns in ifnull(col, ''), so NULL rows can silently match " + f"the comparison" + ) + + return result