diff --git a/changelog.d/4204.fixed.md b/changelog.d/4204.fixed.md new file mode 100644 index 000000000..0b2b600f6 --- /dev/null +++ b/changelog.d/4204.fixed.md @@ -0,0 +1 @@ +Reject an exception from the API v2 file-layer check that has no expires date, sets `category`, `removal` or `match`, names a module in a `database_connectors` package, or gives a path other than the one `git ls-files` lists, and say in the fix message where each layer name must sit. diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 0e601790e..edd12d18e 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -25,7 +25,14 @@ A file with another name fails the guard (rule `api.v2.files-assigned-to-layers`): rename it. A file that can't be renamed needs a `[[rule.exception]]` for that rule in `rules/registry.toml`, with its `path`, a `reason` and an `expires` date, by which the file should be renamed. -An exception chooses no layer: the file gets the route checks under +Write the path as `git ls-files` lists it, for a file git tracks. An exception +covers the whole file, so it sets no `match`, `category` or `removal`. A module +in a `database_connectors` package can't be excepted: that package holds only +connector modules, each named after its operation and placed directly in it +(rule `api.v2.crud-checks-cover-every-resource` also fails a module directly in +`services/v2//database_connectors` whose name doesn't start with an +operation). Move a module that isn't a connector out of the package. An +exception chooses no layer: the file gets the route checks under `fastapi_routes/v2`, and the validator, transformation and types checks (rule `api.v2.pure-modules-no-session-calls`) under `services/v2`. The guard fails on an exception for a file that no longer exists or whose name already diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 118a84b80..6966f393a 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -98,7 +98,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | `api.v2.connectors-no-cross-calls` | v2 connector modules never import one another, so they cannot call one another to sequence a workflow; the service does the sequencing. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.connectors-reads-no-mutations` | v2 read connectors never insert, add or delete rows. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.crud-checks-cover-every-resource` | The v2 CRUD boundary checks find modules by filename, so every resource package is covered, and every connector module is named as a create, read, update or delete module. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | -| `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.files-assigned-to-layers` | Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed, outside a database_connectors package, needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.import-layers` | v2 route modules may import v2 services and v2 data modules, and v2 services may import v2 data modules, never the reverse, directly or through other modules. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-database` | v2 validators, transformations and types never reach SQLAlchemy, SQLModel, a connector, a database_session module or the v2 database module, directly or through other modules, except through the v2 table models; imports under TYPE_CHECKING count. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-session-calls` | v2 validators, transformations, types, process-state modules, package __init__ files and files under services/v2 excepted from api.v2.files-assigned-to-layers import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml index 96af20577..d3e9c58da 100644 --- a/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml +++ b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml @@ -1,7 +1,8 @@ # Proof fixture for rule api.v2.files-assigned-to-layers. The guard must reject # it: -# - simulation_client.py's name gives it no layer, and no [[rule.exception]] -# names it (the case of an open pull request that adds a v2 resource); +# - simulation_client.py and reports/helpers.py have names that give no layer, +# and no [[rule.exception]] names them (simulation_client.py is the case of +# an open pull request that adds a v2 resource); # - the two files listed in ``exceptions`` stand for files a # [[rule.exception]] names. They get no layer, but the pure-module checks # under services/v2 and the route checks under fastapi_routes/v2. @@ -17,6 +18,7 @@ expected_findings = [ "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py calls session.add", "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/household_calculations/legacy_client.py calls session.get", "api.v2.files-assigned-to-layers: policyengine_api/services/v2/household_calculations/simulation_client.py matches no v2 layer", + "api.v2.files-assigned-to-layers: policyengine_api/fastapi_routes/v2/reports/helpers.py matches no v2 layer", ] [files] @@ -35,3 +37,7 @@ def load(session, household_id): def keep(session, row): session.add(row) """ +"policyengine_api/fastapi_routes/v2/reports/helpers.py" = """ +def format_title(report): + return report.title.strip() +""" diff --git a/rules/registry.toml b/rules/registry.toml index 568425a96..04091442e 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -183,7 +183,7 @@ invocation = "make test" [[rule]] id = "api.v2.files-assigned-to-layers" category = "v2-layers" -statement = "Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2." +statement = "Every .py file under services/v2 and fastapi_routes/v2 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed, outside a database_connectors package, needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2." rationale = "docs/engineering/skills/v2-code-organization.md#file-layers" scope = ["policyengine_api/services/v2/**/*.py", "policyengine_api/fastapi_routes/v2/**/*.py"] tier = "mechanical" diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py index ce55711ec..5299d11f4 100644 --- a/scripts/rules/tests/test_v2_layers.py +++ b/scripts/rules/tests/test_v2_layers.py @@ -2,6 +2,7 @@ from __future__ import annotations +import subprocess import tempfile import textwrap import tomllib @@ -27,6 +28,17 @@ SERVICE = "policyengine_api/services/v2/policies/services.py" CONNECTOR = "policyengine_api/services/v2/policies/database_connectors/creates.py" BASELINE_FILE = "rules/baselines/v2-layers-routes.txt" +# What the files rule tells a module in a database_connectors package to do. +CONNECTOR_REMEDY_TEXT = ( + "if it reads or writes rows, name it after its operation, " + "[_name].py, and put it directly in " + "services/v2//database_connectors; otherwise move it out of the " + "database_connectors package and name it after its layer" +) +CONNECTOR_FIX_TEXT = ( + f"{CONNECTOR_REMEDY_TEXT}; an exception can't stand in, because a " + "database_connectors package holds only connector modules" +) def _write_files(files: dict[str, str], root: Path) -> None: @@ -574,7 +586,10 @@ def setUp(self) -> None: self.root = Path(self._directory.name) def _registry( - self, baseline_count: int | None, exceptions: tuple[str, ...] = () + self, + baseline_count: int | None, + exceptions: tuple[str, ...] = (), + exception_fields: str = "expires = 2099-01-01\n", ) -> str: rules = [] for rule_id in (FILES_RULE, ROUTES_RULE, SERVICES_RULE, CONNECTORS_RULE): @@ -586,7 +601,7 @@ def _registry( ) excepted = "".join( f'[[rule.exception]]\npath = "{path}"\nreason = "Generated."\n' - "expires = 2099-01-01\n" + + exception_fields for path in (exceptions if rule_id == FILES_RULE else ()) ) rules.append( @@ -727,15 +742,41 @@ def test_an_unassigned_file_fails_and_names_the_fix(self) -> None: [ f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 layer. Fix: " "name it after its layer (services.py, validators.py, " - "transformations.py, types.py, process_state.py, " - "database_session.py or " - "database_connectors/[_name].py), " - "or, if it can't be renamed, add a [[rule.exception]] for " + "transformations.py, types.py or process_state.py in a resource " + "package under services/v2, database_session.py directly in " + "one, or database_connectors/" + "[_name].py directly in its database_connectors package), or, " + "if it can't be renamed, add a [[rule.exception]] for " "api.v2.files-assigned-to-layers to rules/registry.toml with its " "path, a reason and an expires date." ], ) + def test_an_unassigned_connector_file_is_told_its_name_and_place(self) -> None: + connectors = "policyengine_api/services/v2/policies/database_connectors" + # The second file already has an operation's name, but sits one level + # too deep, so the message must say where it goes too. + for unassigned in ( + f"{connectors}/helpers.py", + f"{connectors}/legacy/creates.py", + ): + with self.subTest(path=unassigned): + _write_files( + {unassigned: "", "rules/registry.toml": self._registry(None)}, + self.root, + ) + + messages = self._messages() + (self.root / unassigned).unlink() + + self.assertEqual( + messages, + [ + f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 " + f"layer. Fix: {CONNECTOR_FIX_TEXT}." + ], + ) + def test_an_unassigned_route_file_is_told_the_route_names(self) -> None: unassigned = "policyengine_api/fastapi_routes/v2/metadata/helpers.py" _write_files( @@ -774,6 +815,169 @@ def test_an_exception_gives_no_layer_but_the_strictest_checks(self) -> None: ], ) + def test_an_exception_needs_an_expires_date_and_covers_the_whole_file( + self, + ) -> None: + client = "policyengine_api/services/v2/household_calculations/client.py" + prefix = f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts {client}" + no_date = ( + f"{prefix} from {FILES_RULE}, but the exception has no expires date. " + "Fix: give it an expires date by which the file is renamed." + ) + unassigned = f"[{FILES_RULE}] v2-layers: {client} matches no v2 layer." + for fields, field_name, has_expires_date in ( + ('category = "forever"\n', "category", False), + ('removal = "never"\n', "removal", False), + ('match = "session.get"\nexpires = 2099-01-01\n', "match", True), + ): + with self.subTest(field=field_name): + _write_files( + { + client: "", + "rules/registry.toml": self._registry( + None, (client,), exception_fields=fields + ), + }, + self.root, + ) + expected = [ + f"{prefix} from {FILES_RULE}, but the exception sets " + f"{field_name}, which this rule doesn't read. Fix: remove it; " + "a files-rule exception covers the whole file and ends on " + "its expires date." + ] + if not has_expires_date: + expected.append(no_date) + + messages = self._messages() + + self.assertEqual( + sorted( + message + for message in messages + if not message.startswith(unassigned) + ), + sorted(expected), + ) + self.assertTrue( + any(message.startswith(unassigned) for message in messages) + ) + + def test_a_connector_module_cannot_be_excepted(self) -> None: + connectors = "policyengine_api/services/v2/policies/database_connectors" + for helper in (f"{connectors}/helpers.py", f"{connectors}/legacy/creates.py"): + with self.subTest(path=helper): + _write_files( + { + helper: "", + "rules/registry.toml": self._registry(None, (helper,)), + }, + self.root, + ) + + messages = self._messages() + (self.root / helper).unlink() + + self.assertEqual( + sorted(messages), + sorted( + [ + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts " + f"{helper} from {FILES_RULE}, but a database_connectors " + "package holds only connector modules (rule " + "api.v2.crud-checks-cover-every-resource also fails a " + "module directly in " + "services/v2//database_connectors whose name " + "doesn't start with an operation). Fix: remove the " + f"exception; {CONNECTOR_REMEDY_TEXT}.", + f"[{FILES_RULE}] v2-layers: {helper} matches no v2 " + f"layer. Fix: {CONNECTOR_FIX_TEXT}.", + ] + ), + ) + + def test_an_exception_with_no_readable_expires_date_is_reported(self) -> None: + # The registry loader also reports these, so this calls the guard + # directly rather than through _messages, which requires a clean load. + client = "policyengine_api/services/v2/household_calculations/client.py" + no_date = ( + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts {client} from " + f"{FILES_RULE}, but the exception has no expires date. Fix: give it " + "an expires date by which the file is renamed." + ) + for fields, case in (("", "no expires"), ('expires = "soon"\n', "bad date")): + with self.subTest(case=case): + _write_files( + { + client: "", + "rules/registry.toml": self._registry( + None, (client,), exception_fields=fields + ), + }, + self.root, + ) + + self.assertIn( + no_date, [str(violation) for violation in run_check(self.root)] + ) + + def test_an_exception_path_must_be_written_as_git_lists_it(self) -> None: + # Each spelling names the real file, so is_file() accepts it, but the + # guard compares exceptions with the paths it scans. + client = "policyengine_api/services/v2/household_calculations/client.py" + _write_files( + {"policyengine_api/services/v2/policies/services.py": ""}, self.root + ) + unassigned = f"[{FILES_RULE}] v2-layers: {client} matches no v2 layer." + for spelling in ( + "policyengine_api/services/v2/household_calculations/./client.py", + "policyengine_api/services/v2/household_calculations//client.py", + "policyengine_api/services/v2/policies/../household_calculations/client.py", + ): + with self.subTest(path=spelling): + _write_files( + { + client: "", + "rules/registry.toml": self._registry(None, (spelling,)), + }, + self.root, + ) + + messages = self._messages() + + self.assertIn( + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts " + f"{spelling} from {FILES_RULE}, but no file the guard scans " + "has that exact path, so the exception would match nothing. " + "Fix: write the path as git ls-files lists it, and git add the " + "file if git doesn't track it.", + messages, + ) + self.assertTrue( + any(message.startswith(unassigned) for message in messages) + ) + + def test_an_exception_for_a_file_git_does_not_track_is_reported(self) -> None: + # In a git checkout the guard scans only the files git tracks, so an + # exactly spelled exception for an untracked file would match nothing. + client = "policyengine_api/services/v2/household_calculations/client.py" + _write_files( + {client: "", "rules/registry.toml": self._registry(None, (client,))}, + self.root, + ) + subprocess.run(["git", "init", "-q"], cwd=self.root, check=True) + + self.assertEqual( + self._messages(), + [ + f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts {client} " + f"from {FILES_RULE}, but no file the guard scans has that exact " + "path, so the exception would match nothing. Fix: write the path " + "as git ls-files lists it, and git add the file if git doesn't " + "track it." + ], + ) + def test_exceptions_must_name_one_existing_v2_file_without_a_layer(self) -> None: named = ROUTE missing = "policyengine_api/services/v2/policies/gone.py" diff --git a/scripts/rules/v2_layer_checks.py b/scripts/rules/v2_layer_checks.py new file mode 100644 index 000000000..5791eceb1 --- /dev/null +++ b/scripts/rules/v2_layer_checks.py @@ -0,0 +1,438 @@ +"""Import and call checks for the v2-layers quality guard, one layer at a time. + +``file_findings`` parses one v2 file and returns what its layer's rule +forbids in it. scripts/rules/v2_layers.py assigns the layers, runs these +checks over every v2 file and compares the findings with the baselines; its +docstring describes what each layer may import and call. + +Standard library only. +""" + +from __future__ import annotations + +import ast +from dataclasses import dataclass +from pathlib import PurePosixPath +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from collections.abc import Callable, Iterable, Mapping + +SERVICES_DIRECTORY = "policyengine_api/services/v2" +ROUTES_DIRECTORY = "policyengine_api/fastapi_routes/v2" + +ROUTES_RULE = "api.v2.routes-no-sessions-or-sql" +SERVICES_RULE = "api.v2.services-no-sql" +CONNECTORS_RULE = "api.v2.connectors-never-commit" +PURE_RULE = "api.v2.pure-modules-no-session-calls" +SESSION_RULE = "api.v2.session-modules-no-data-access" + +# What a [[rule.exception]] for the files rule makes a file: no layer of its +# own; under services/v2 it gets the pure-module checks (under +# fastapi_routes/v2 every file gets the route checks). +EXCEPTED = "excepted" +PURE_LAYERS = frozenset( + {EXCEPTED, "package", "process-state", "transformation", "types", "validator"} +) + +SQL_PACKAGES = ("sqlalchemy", "sqlmodel") +DATABASE_PACKAGES = ("policyengine_api.data.orm", "policyengine_api.data.v2.database") +SESSION_TYPES = ("sqlmodel.Session", "sqlalchemy.orm.Session") +SQL_EXCEPTIONS = "sqlalchemy.exc" +# What a service may import from SQLAlchemy or SQLModel: the session type, for +# annotations, and the exception classes, to raise typed failures. +SERVICE_SQL_IMPORTS = (*SESSION_TYPES, SQL_EXCEPTIONS) +SQL_BUILDERS = frozenset( + { + "delete", + "except_", + "exists", + "insert", + "intersect", + "literal_column", + "select", + "text", + "union", + "union_all", + "update", + "values", + } +) +SQL_SESSION_FACTORIES = frozenset({"Session", "create_engine", "sessionmaker"}) +TRANSACTION_METHODS = frozenset( + {"begin", "begin_nested", "close", "commit", "invalidate", "reset", "rollback"} +) +DATA_ACCESS_METHODS = frozenset( + { + "add", + "add_all", + "bulk_insert_mappings", + "bulk_save_objects", + "bulk_update_mappings", + "connection", + "delete", + "exec", + "execute", + "expunge", + "expunge_all", + "flush", + "get", + "get_one", + "merge", + "query", + "refresh", + "scalar", + "scalars", + } +) +# A database_session container's context managers, which open a session. +CONTAINER_METHODS = frozenset({"read", "transaction"}) +# Receiver name prefixes, before "_session", of web rather than database sessions. +WEB_SESSION_WORDS = ("client", "http", "requests") +# Owners whose ``.session`` is Starlette's session data, not a database session. +REQUEST_OWNERS = frozenset({"req", "request", "websocket", "ws"}) +HTTP_PACKAGES = ("fastapi", "starlette") + + +def _within(module: str, package: str) -> bool: + return module == package or module.startswith(f"{package}.") + + +def _within_any(module: str, packages: Iterable[str]) -> bool: + return any(_within(module, package) for package in packages) + + +def _module_name(path: str) -> tuple[str, bool]: + parts = list(PurePosixPath(path).with_suffix("").parts) + is_package = parts[-1] == "__init__" + if is_package: + parts = parts[:-1] + return ".".join(parts), is_package + + +# --- imports -------------------------------------------------------------- + + +def _from_source(node: ast.ImportFrom, path: str) -> str: + """Return the absolute module a ``from ... import`` reads from.""" + if not node.level: + return node.module or "" + module_name, is_package = _module_name(path) + base = module_name.split(".") + if not is_package: + base = base[:-1] + if node.level > 1: + base = base[: -(node.level - 1)] + return ".".join([*base, *([node.module] if node.module else [])]) + + +@dataclass(frozen=True) +class ImportStatement: + """One imported module, or one ``from`` import with the names it binds.""" + + module: str + names: tuple[str, ...] + + +def _imports(tree: ast.Module, path: str) -> list[ImportStatement]: + """Return every import in a module; relative imports are resolved. + + Imports under TYPE_CHECKING and inside functions count like any other. + """ + statements = [] + for node in ast.walk(tree): + if isinstance(node, ast.Import): + statements.extend(ImportStatement(alias.name, ()) for alias in node.names) + elif isinstance(node, ast.ImportFrom): + source = _from_source(node, path) + names = tuple(f"{source}.{alias.name}" for alias in node.names) + statements.append(ImportStatement(source, names)) + return statements + + +def _imported_names(tree: ast.Module, path: str) -> dict[str, str]: + """Map each name a module binds by importing to the dotted name it refers to.""" + names = {} + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + bound = alias.asname or alias.name.split(".")[0] + names[bound] = alias.name if alias.asname else bound + elif isinstance(node, ast.ImportFrom): + source = _from_source(node, path) + for alias in node.names: + names[alias.asname or alias.name] = f"{source}.{alias.name}" + return names + + +def _is_session_or_connector_module(module: str) -> bool: + parts = module.split(".") + return _within(module, "policyengine_api.services.v2") and ( + "database_connectors" in parts or "database_session" in parts + ) + + +def _is_route_forbidden_import(module: str) -> bool: + return ( + _within_any(module, SQL_PACKAGES) + or _within_any(module, DATABASE_PACKAGES) + or _is_session_or_connector_module(module) + ) + + +def _is_connector_module(module: str) -> bool: + return _within(module, "policyengine_api.services.v2") and ( + "database_connectors" in module.split(".") + ) + + +def _is_connector_forbidden_import(module: str) -> bool: + parts = module.split(".") + return _within_any(module, DATABASE_PACKAGES) or ( + _within(module, "policyengine_api.services.v2") and "database_session" in parts + ) + + +def _is_http_import(module: str) -> bool: + return _within_any(module, HTTP_PACKAGES) + + +def _is_service_forbidden_sql_name(name: str) -> bool: + return _within_any(name, SQL_PACKAGES) and not _within_any( + name, SERVICE_SQL_IMPORTS + ) + + +def _is_pure_forbidden_import(module: str) -> bool: + return _within_any(module, DATABASE_PACKAGES) or _is_session_or_connector_module( + module + ) + + +def _is_pure_forbidden_sql_name(name: str) -> bool: + return _within_any(name, SQL_PACKAGES) and not _within_any(name, SESSION_TYPES) + + +def _never(_module: str) -> bool: + return False + + +# --- calls ---------------------------------------------------------------- + + +def _dotted_name(node: ast.expr, imported: Mapping[str, str]) -> str | None: + """Resolve a called name such as ``select`` or ``sa.text`` through the imports.""" + attributes = [] + while isinstance(node, ast.Attribute): + attributes.append(node.attr) + node = node.value + if not isinstance(node, ast.Name) or node.id not in imported: + return None + return ".".join([imported[node.id], *reversed(attributes)]) + + +def _receiver_name(node: ast.expr) -> str: + if isinstance(node, ast.Attribute): + return node.attr + if isinstance(node, ast.Name): + return node.id + return "" + + +def _is_request_owner(node: ast.expr) -> bool: + name = _receiver_name(node).lower().lstrip("_") + return name in REQUEST_OWNERS or name.endswith("_request") + + +def _is_session_receiver(node: ast.expr) -> bool: + """Whether a method's receiver is a database session or its container.""" + if ( + isinstance(node, ast.Attribute) + and node.attr == "session" + and _is_request_owner(node.value) + ): + return False + name = _receiver_name(node).lower().lstrip("_") + if name == "session": + return True + prefix = name.removesuffix("_session") + return prefix != name and not any(word in prefix for word in WEB_SESSION_WORDS) + + +def _is_session_factory_name(name: str) -> bool: + return name.lower().lstrip("_").endswith("session_factory") + + +def _injected_factory_detail(call: ast.Call) -> str | None: + """Return ``calls `` when a call opens a session from an injected factory. + + ``session_factory()``, ``self._session_factory()`` and + ``session_factory.begin()`` count. + """ + func = call.func + if _is_session_factory_name(_receiver_name(func)): + return f"calls {_receiver_name(func)}" + if ( + isinstance(func, ast.Attribute) + and func.attr == "begin" + and _is_session_factory_name(_receiver_name(func.value)) + ): + return f"calls {_receiver_name(func.value)}.begin" + return None + + +def _calls_on_a_table(call: ast.Call) -> bool: + """Whether a call is a SQL builder on a table, as ``Model.__table__.insert()``.""" + func = call.func + return ( + isinstance(func, ast.Attribute) + and func.attr in SQL_BUILDERS - {"values"} + and isinstance(func.value, ast.Attribute) + and func.value.attr == "__table__" + ) + + +# --- policies ------------------------------------------------------------- + + +@dataclass(frozen=True) +class LayerPolicy: + """What one layer's rule forbids: imports, session methods and call kinds. + + ``session_methods`` of None forbids every method on a session. + ``checks_sql_calls`` forbids every call into SQLAlchemy or SQLModel but + its exception classes; ``checks_builders`` only its SQL builders. Either + also forbids builders called on a model's ``__table__``. + ``checks_factories`` forbids opening a session from a session factory. + """ + + rule_id: str + forbidden_module: Callable[[str], bool] + session_methods: frozenset[str] | None + checks_sql_calls: bool + checks_factories: bool + forbidden_name: Callable[[str], bool] = _never + checks_builders: bool = False + + +ROUTE_POLICY = LayerPolicy( + ROUTES_RULE, + _is_route_forbidden_import, + None, + checks_sql_calls=True, + checks_factories=True, +) +PURE_POLICY = LayerPolicy( + PURE_RULE, + _is_pure_forbidden_import, + None, + checks_sql_calls=True, + checks_factories=True, + forbidden_name=_is_pure_forbidden_sql_name, +) +LAYER_POLICIES = { + "service": LayerPolicy( + SERVICES_RULE, + _is_http_import, + DATA_ACCESS_METHODS, + checks_sql_calls=True, + checks_factories=False, + forbidden_name=_is_service_forbidden_sql_name, + ), + "connector": LayerPolicy( + CONNECTORS_RULE, + _is_connector_forbidden_import, + TRANSACTION_METHODS | CONTAINER_METHODS, + checks_sql_calls=False, + checks_factories=True, + ), + "session": LayerPolicy( + SESSION_RULE, + _is_connector_module, + DATA_ACCESS_METHODS, + checks_sql_calls=False, + checks_factories=False, + checks_builders=True, + ), + **dict.fromkeys(PURE_LAYERS, PURE_POLICY), +} + + +def _policy(path: str, layer: str) -> LayerPolicy | None: + if path.startswith(f"{ROUTES_DIRECTORY}/"): + return ROUTE_POLICY + return LAYER_POLICIES.get(layer) + + +def _import_details(statement: ImportStatement, policy: LayerPolicy) -> list[str]: + """Return ``imports `` for each module or name the policy forbids.""" + if policy.forbidden_module(statement.module): + return [f"imports {statement.module}"] + module_level = next( + (name for name in statement.names if policy.forbidden_module(name)), None + ) + if module_level is not None: + return [f"imports {module_level}"] + if not statement.names: + forbidden = policy.forbidden_name(statement.module) + return [f"imports {statement.module}"] if forbidden else [] + return [ + f"imports {name}" for name in statement.names if policy.forbidden_name(name) + ] + + +def _named_call_details(dotted: str, policy: LayerPolicy) -> list[str]: + """Return the findings for one call to an imported name. + + For example ``calls sqlmodel.select`` or ``calls get_v2_session_factory``. + """ + last = dotted.rsplit(".", 1)[-1] + from_sql = _within_any(dotted, SQL_PACKAGES) + opens_a_session = _within_any(dotted, DATABASE_PACKAGES) or ( + from_sql and last in SQL_SESSION_FACTORIES + ) + if opens_a_session: + return [f"calls {last}"] if policy.checks_factories else [] + if policy.checks_sql_calls and from_sql and not _within(dotted, SQL_EXCEPTIONS): + return [f"calls {dotted}"] + if policy.checks_builders and from_sql and last in SQL_BUILDERS: + return [f"calls {dotted}"] + return [] + + +def _call_details( + call: ast.Call, imported: Mapping[str, str], policy: LayerPolicy +) -> list[str]: + details = [] + dotted = _dotted_name(call.func, imported) + if dotted is not None: + details.extend(_named_call_details(dotted, policy)) + if (policy.checks_sql_calls or policy.checks_builders) and _calls_on_a_table(call): + details.append(f"calls {ast.unparse(call.func)}") + factory = _injected_factory_detail(call) if policy.checks_factories else None + if factory is not None: + details.append(factory) + func = call.func + forbidden_method = isinstance(func, ast.Attribute) and ( + policy.session_methods is None or func.attr in policy.session_methods + ) + if forbidden_method and _is_session_receiver(func.value): + details.append(f"calls {_receiver_name(func.value)}.{func.attr}") + return details + + +def file_findings(path: str, source: str, layer: str) -> list[tuple[str, str]]: + """Return (rule id, detail) for each forbidden import and call in one file.""" + policy = _policy(path, layer) + if policy is None: + return [] + tree = ast.parse(source) + details = [ + detail + for statement in _imports(tree, path) + for detail in _import_details(statement, policy) + ] + imported = _imported_names(tree, path) + for call in (node for node in ast.walk(tree) if isinstance(node, ast.Call)): + details.extend(_call_details(call, imported, policy)) + return [(policy.rule_id, detail) for detail in details] diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py index 279c4a6cf..8d27f205d 100644 --- a/scripts/rules/v2_layers.py +++ b/scripts/rules/v2_layers.py @@ -60,34 +60,38 @@ from __future__ import annotations -import ast from collections import Counter from dataclasses import dataclass +from datetime import date from pathlib import Path, PurePosixPath from typing import TYPE_CHECKING from scripts.rules.file_length import tracked_python_files from scripts.rules.load import load_registry from scripts.rules.model import Violation +from scripts.rules.v2_layer_checks import ( + CONNECTORS_RULE, + EXCEPTED, + PURE_RULE, + ROUTES_DIRECTORY, + ROUTES_RULE, + SERVICES_DIRECTORY, + SERVICES_RULE, + SESSION_RULE, + file_findings, +) if TYPE_CHECKING: - from collections.abc import Callable, Collection, Iterable, Mapping + from collections.abc import Collection, Iterable - from scripts.rules.model import Rule + from scripts.rules.model import Rule, RuleException REPO_ROOT = Path(__file__).resolve().parents[2] CHECK_NAME = "v2-layers" REGISTRY_FILE = "rules/registry.toml" -SERVICES_DIRECTORY = "policyengine_api/services/v2" -ROUTES_DIRECTORY = "policyengine_api/fastapi_routes/v2" ORGANIZATION_DOC = "docs/engineering/skills/v2-code-organization.md" FILES_RULE = "api.v2.files-assigned-to-layers" -ROUTES_RULE = "api.v2.routes-no-sessions-or-sql" -SERVICES_RULE = "api.v2.services-no-sql" -CONNECTORS_RULE = "api.v2.connectors-never-commit" -PURE_RULE = "api.v2.pure-modules-no-session-calls" -SESSION_RULE = "api.v2.session-modules-no-data-access" RULES = ( FILES_RULE, ROUTES_RULE, @@ -109,81 +113,18 @@ # The names that give a layer, by directory, for the files rule's fix message. LAYER_NAMES = { SERVICES_DIRECTORY: ( - "services.py, validators.py, transformations.py, types.py, " - "process_state.py, database_session.py or " - "database_connectors/[_name].py" + "services.py, validators.py, transformations.py, types.py or " + "process_state.py in a resource package under services/v2, " + "database_session.py directly in one, or " + "database_connectors/[_name].py directly " + "in its database_connectors package" ), ROUTES_DIRECTORY: ( "routes.py, _routes.py, _models.py or _support.py" ), } -# What a [[rule.exception]] for the files rule makes a file: no layer of its -# own; under services/v2 it gets the pure-module checks (under -# fastapi_routes/v2 every file gets the route checks). -EXCEPTED = "excepted" -PURE_LAYERS = frozenset( - {EXCEPTED, "package", "process-state", "transformation", "types", "validator"} -) GLOB_CHARACTERS = frozenset("*?[") -SQL_PACKAGES = ("sqlalchemy", "sqlmodel") -DATABASE_PACKAGES = ("policyengine_api.data.orm", "policyengine_api.data.v2.database") -SESSION_TYPES = ("sqlmodel.Session", "sqlalchemy.orm.Session") -SQL_EXCEPTIONS = "sqlalchemy.exc" -# What a service may import from SQLAlchemy or SQLModel: the session type, for -# annotations, and the exception classes, to raise typed failures. -SERVICE_SQL_IMPORTS = (*SESSION_TYPES, SQL_EXCEPTIONS) -SQL_BUILDERS = frozenset( - { - "delete", - "except_", - "exists", - "insert", - "intersect", - "literal_column", - "select", - "text", - "union", - "union_all", - "update", - "values", - } -) -SQL_SESSION_FACTORIES = frozenset({"Session", "create_engine", "sessionmaker"}) -TRANSACTION_METHODS = frozenset( - {"begin", "begin_nested", "close", "commit", "invalidate", "reset", "rollback"} -) -DATA_ACCESS_METHODS = frozenset( - { - "add", - "add_all", - "bulk_insert_mappings", - "bulk_save_objects", - "bulk_update_mappings", - "connection", - "delete", - "exec", - "execute", - "expunge", - "expunge_all", - "flush", - "get", - "get_one", - "merge", - "query", - "refresh", - "scalar", - "scalars", - } -) -# A database_session container's context managers, which open a session. -CONTAINER_METHODS = frozenset({"read", "transaction"}) -# Receiver name prefixes, before "_session", of web rather than database sessions. -WEB_SESSION_WORDS = ("client", "http", "requests") -# Owners whose ``.session`` is Starlette's session data, not a database session. -REQUEST_OWNERS = frozenset({"req", "request", "websocket", "ws"}) -HTTP_PACKAGES = ("fastapi", "starlette") - FIXES = { FILES_RULE: ( "name it after its layer ({names}), or, if it can't be renamed, add a " @@ -229,22 +170,6 @@ def __str__(self) -> str: return f"{self.path} {self.detail}" -def _within(module: str, package: str) -> bool: - return module == package or module.startswith(f"{package}.") - - -def _within_any(module: str, packages: Iterable[str]) -> bool: - return any(_within(module, package) for package in packages) - - -def _module_name(path: str) -> tuple[str, bool]: - parts = list(PurePosixPath(path).with_suffix("").parts) - is_package = parts[-1] == "__init__" - if is_package: - parts = parts[:-1] - return ".".join(parts), is_package - - # --- layers --------------------------------------------------------------- @@ -300,334 +225,6 @@ def _route_layer(name: str) -> str | None: return "http-support" if name.endswith("_support.py") else None -# --- imports -------------------------------------------------------------- - - -def _from_source(node: ast.ImportFrom, path: str) -> str: - """Return the absolute module a ``from ... import`` reads from.""" - if not node.level: - return node.module or "" - module_name, is_package = _module_name(path) - base = module_name.split(".") - if not is_package: - base = base[:-1] - if node.level > 1: - base = base[: -(node.level - 1)] - return ".".join([*base, *([node.module] if node.module else [])]) - - -@dataclass(frozen=True) -class ImportStatement: - """One imported module, or one ``from`` import with the names it binds.""" - - module: str - names: tuple[str, ...] - - -def _imports(tree: ast.Module, path: str) -> list[ImportStatement]: - """Return every import in a module; relative imports are resolved. - - Imports under TYPE_CHECKING and inside functions count like any other. - """ - statements = [] - for node in ast.walk(tree): - if isinstance(node, ast.Import): - statements.extend(ImportStatement(alias.name, ()) for alias in node.names) - elif isinstance(node, ast.ImportFrom): - source = _from_source(node, path) - names = tuple(f"{source}.{alias.name}" for alias in node.names) - statements.append(ImportStatement(source, names)) - return statements - - -def _imported_names(tree: ast.Module, path: str) -> dict[str, str]: - """Map each name a module binds by importing to the dotted name it refers to.""" - names = {} - for node in ast.walk(tree): - if isinstance(node, ast.Import): - for alias in node.names: - bound = alias.asname or alias.name.split(".")[0] - names[bound] = alias.name if alias.asname else bound - elif isinstance(node, ast.ImportFrom): - source = _from_source(node, path) - for alias in node.names: - names[alias.asname or alias.name] = f"{source}.{alias.name}" - return names - - -def _is_session_or_connector_module(module: str) -> bool: - parts = module.split(".") - return _within(module, "policyengine_api.services.v2") and ( - "database_connectors" in parts or "database_session" in parts - ) - - -def _is_route_forbidden_import(module: str) -> bool: - return ( - _within_any(module, SQL_PACKAGES) - or _within_any(module, DATABASE_PACKAGES) - or _is_session_or_connector_module(module) - ) - - -def _is_connector_module(module: str) -> bool: - return _within(module, "policyengine_api.services.v2") and ( - "database_connectors" in module.split(".") - ) - - -def _is_connector_forbidden_import(module: str) -> bool: - parts = module.split(".") - return _within_any(module, DATABASE_PACKAGES) or ( - _within(module, "policyengine_api.services.v2") and "database_session" in parts - ) - - -def _is_http_import(module: str) -> bool: - return _within_any(module, HTTP_PACKAGES) - - -def _is_service_forbidden_sql_name(name: str) -> bool: - return _within_any(name, SQL_PACKAGES) and not _within_any( - name, SERVICE_SQL_IMPORTS - ) - - -def _is_pure_forbidden_import(module: str) -> bool: - return _within_any(module, DATABASE_PACKAGES) or _is_session_or_connector_module( - module - ) - - -def _is_pure_forbidden_sql_name(name: str) -> bool: - return _within_any(name, SQL_PACKAGES) and not _within_any(name, SESSION_TYPES) - - -def _never(_module: str) -> bool: - return False - - -# --- calls ---------------------------------------------------------------- - - -def _dotted_name(node: ast.expr, imported: Mapping[str, str]) -> str | None: - """Resolve a called name such as ``select`` or ``sa.text`` through the imports.""" - attributes = [] - while isinstance(node, ast.Attribute): - attributes.append(node.attr) - node = node.value - if not isinstance(node, ast.Name) or node.id not in imported: - return None - return ".".join([imported[node.id], *reversed(attributes)]) - - -def _receiver_name(node: ast.expr) -> str: - if isinstance(node, ast.Attribute): - return node.attr - if isinstance(node, ast.Name): - return node.id - return "" - - -def _is_request_owner(node: ast.expr) -> bool: - name = _receiver_name(node).lower().lstrip("_") - return name in REQUEST_OWNERS or name.endswith("_request") - - -def _is_session_receiver(node: ast.expr) -> bool: - """Whether a method's receiver is a database session or its container.""" - if ( - isinstance(node, ast.Attribute) - and node.attr == "session" - and _is_request_owner(node.value) - ): - return False - name = _receiver_name(node).lower().lstrip("_") - if name == "session": - return True - prefix = name.removesuffix("_session") - return prefix != name and not any(word in prefix for word in WEB_SESSION_WORDS) - - -def _is_session_factory_name(name: str) -> bool: - return name.lower().lstrip("_").endswith("session_factory") - - -def _injected_factory_detail(call: ast.Call) -> str | None: - """Return ``calls `` when a call opens a session from an injected factory. - - ``session_factory()``, ``self._session_factory()`` and - ``session_factory.begin()`` count. - """ - func = call.func - if _is_session_factory_name(_receiver_name(func)): - return f"calls {_receiver_name(func)}" - if ( - isinstance(func, ast.Attribute) - and func.attr == "begin" - and _is_session_factory_name(_receiver_name(func.value)) - ): - return f"calls {_receiver_name(func.value)}.begin" - return None - - -def _calls_on_a_table(call: ast.Call) -> bool: - """Whether a call is a SQL builder on a table, as ``Model.__table__.insert()``.""" - func = call.func - return ( - isinstance(func, ast.Attribute) - and func.attr in SQL_BUILDERS - {"values"} - and isinstance(func.value, ast.Attribute) - and func.value.attr == "__table__" - ) - - -# --- policies ------------------------------------------------------------- - - -@dataclass(frozen=True) -class LayerPolicy: - """What one layer's rule forbids: imports, session methods and call kinds. - - ``session_methods`` of None forbids every method on a session. - ``checks_sql_calls`` forbids every call into SQLAlchemy or SQLModel but - its exception classes; ``checks_builders`` only its SQL builders. Either - also forbids builders called on a model's ``__table__``. - ``checks_factories`` forbids opening a session from a session factory. - """ - - rule_id: str - forbidden_module: Callable[[str], bool] - session_methods: frozenset[str] | None - checks_sql_calls: bool - checks_factories: bool - forbidden_name: Callable[[str], bool] = _never - checks_builders: bool = False - - -ROUTE_POLICY = LayerPolicy( - ROUTES_RULE, - _is_route_forbidden_import, - None, - checks_sql_calls=True, - checks_factories=True, -) -PURE_POLICY = LayerPolicy( - PURE_RULE, - _is_pure_forbidden_import, - None, - checks_sql_calls=True, - checks_factories=True, - forbidden_name=_is_pure_forbidden_sql_name, -) -LAYER_POLICIES = { - "service": LayerPolicy( - SERVICES_RULE, - _is_http_import, - DATA_ACCESS_METHODS, - checks_sql_calls=True, - checks_factories=False, - forbidden_name=_is_service_forbidden_sql_name, - ), - "connector": LayerPolicy( - CONNECTORS_RULE, - _is_connector_forbidden_import, - TRANSACTION_METHODS | CONTAINER_METHODS, - checks_sql_calls=False, - checks_factories=True, - ), - "session": LayerPolicy( - SESSION_RULE, - _is_connector_module, - DATA_ACCESS_METHODS, - checks_sql_calls=False, - checks_factories=False, - checks_builders=True, - ), - **dict.fromkeys(PURE_LAYERS, PURE_POLICY), -} - - -def _policy(path: str, layer: str) -> LayerPolicy | None: - if path.startswith(f"{ROUTES_DIRECTORY}/"): - return ROUTE_POLICY - return LAYER_POLICIES.get(layer) - - -def _import_details(statement: ImportStatement, policy: LayerPolicy) -> list[str]: - """Return ``imports `` for each module or name the policy forbids.""" - if policy.forbidden_module(statement.module): - return [f"imports {statement.module}"] - module_level = next( - (name for name in statement.names if policy.forbidden_module(name)), None - ) - if module_level is not None: - return [f"imports {module_level}"] - if not statement.names: - forbidden = policy.forbidden_name(statement.module) - return [f"imports {statement.module}"] if forbidden else [] - return [ - f"imports {name}" for name in statement.names if policy.forbidden_name(name) - ] - - -def _named_call_details(dotted: str, policy: LayerPolicy) -> list[str]: - """Return the findings for one call to an imported name. - - For example ``calls sqlmodel.select`` or ``calls get_v2_session_factory``. - """ - last = dotted.rsplit(".", 1)[-1] - from_sql = _within_any(dotted, SQL_PACKAGES) - opens_a_session = _within_any(dotted, DATABASE_PACKAGES) or ( - from_sql and last in SQL_SESSION_FACTORIES - ) - if opens_a_session: - return [f"calls {last}"] if policy.checks_factories else [] - if policy.checks_sql_calls and from_sql and not _within(dotted, SQL_EXCEPTIONS): - return [f"calls {dotted}"] - if policy.checks_builders and from_sql and last in SQL_BUILDERS: - return [f"calls {dotted}"] - return [] - - -def _call_details( - call: ast.Call, imported: Mapping[str, str], policy: LayerPolicy -) -> list[str]: - details = [] - dotted = _dotted_name(call.func, imported) - if dotted is not None: - details.extend(_named_call_details(dotted, policy)) - if (policy.checks_sql_calls or policy.checks_builders) and _calls_on_a_table(call): - details.append(f"calls {ast.unparse(call.func)}") - factory = _injected_factory_detail(call) if policy.checks_factories else None - if factory is not None: - details.append(factory) - func = call.func - forbidden_method = isinstance(func, ast.Attribute) and ( - policy.session_methods is None or func.attr in policy.session_methods - ) - if forbidden_method and _is_session_receiver(func.value): - details.append(f"calls {_receiver_name(func.value)}.{func.attr}") - return details - - -def _file_findings(path: str, source: str, layer: str) -> list[tuple[str, str]]: - """Return (rule id, detail) for each forbidden import and call in one file.""" - policy = _policy(path, layer) - if policy is None: - return [] - tree = ast.parse(source) - details = [ - detail - for statement in _imports(tree, path) - for detail in _import_details(statement, policy) - ] - imported = _imported_names(tree, path) - for call in (node for node in ast.walk(tree) if isinstance(node, ast.Call)): - details.extend(_call_details(call, imported, policy)) - return [(policy.rule_id, detail) for detail in details] - - # --- scanning and comparing ----------------------------------------------- @@ -652,13 +249,22 @@ def scan( source = (root / path).read_text(encoding="utf-8") findings.extend( Finding(rule_id, path, detail) - for rule_id, detail in _file_findings(path, source, layer) + for rule_id, detail in file_findings(path, source, layer) ) return sorted(findings, key=lambda finding: (finding.rule_id, str(finding))) -def _exception_violations(root: Path, path: str) -> list[Violation]: - """Report a files-rule exception that names no unnamed v2 file.""" +def _exception_violations( + root: Path, exception: RuleException, scanned: Collection[str] +) -> list[Violation]: + """Report a files-rule exception that doesn't name one unnamed v2 file. + + The path must be one of ``scanned``, the paths as git ls-files writes + them, because assign_layer compares exceptions with those. The exception + must also end on an ``expires`` date and cover the whole file: the guard + reads no ``category``, ``removal`` or ``match``. + """ + path = exception.path def violation(problem: str, fix: str) -> Violation: return Violation( @@ -677,23 +283,66 @@ def violation(problem: str, fix: str) -> Violation: "name one file there, or remove the exception", ) ] - if not (root / path).is_file(): - return [violation("but it does not exist", "remove the exception")] + if path not in scanned: + if not (root / path).is_file(): + return [violation("but it does not exist", "remove the exception")] + # Another spelling of a real file (a '.', '..' or empty segment, a + # symlinked directory, or different letter case on macOS), or a file + # git doesn't track. + return [ + violation( + "but no file the guard scans has that exact path, so the " + "exception would match nothing", + "write the path as git ls-files lists it, and git add the file if " + "git doesn't track it", + ) + ] if _layer_by_name(path) is not None: return [ violation("but its name already gives it a layer", "remove the exception") ] - return [] + if _in_connector_package(path): + return [ + violation( + "but a database_connectors package holds only connector modules " + "(rule api.v2.crud-checks-cover-every-resource also fails a module " + "directly in services/v2//database_connectors whose name " + "doesn't start with an operation)", + f"remove the exception; {CONNECTOR_REMEDY}", + ) + ] + violations = [ + violation( + f"but the exception sets {field}, which this rule doesn't read", + "remove it; a files-rule exception covers the whole file and ends on " + "its expires date", + ) + for field in ("category", "removal", "match") + if getattr(exception, field) is not None + ] + if exception.expires is None or exception.expires == date.max: + violations.append( + violation( + "but the exception has no expires date", + "give it an expires date by which the file is renamed", + ) + ) + return violations -def file_exceptions(root: Path, rule: Rule | None) -> tuple[list[str], list[Violation]]: - """Return the files the files rule excepts, and its exceptions that don't belong.""" +def file_exceptions( + root: Path, rule: Rule | None, scanned: Collection[str] +) -> tuple[list[str], list[Violation]]: + """Return the files the files rule excepts, and its exceptions that don't belong. + + ``scanned`` holds the paths the guard scans. + """ if rule is None: return [], [] excepted = [] violations = [] for exception in rule.exceptions: - problems = _exception_violations(root, exception.path) + problems = _exception_violations(root, exception, scanned) violations += problems if not problems: excepted.append(exception.path) @@ -714,10 +363,35 @@ def _occurrences(count: int) -> str: return f"{count} occurrence" if count == 1 else f"{count} occurrences" +# A connector gets its layer only with an operation's name, directly in a +# database_connectors package, so the fix names both. It points to the +# location the CRUD checks cover: deeper connector packages get a layer from +# this guard but aren't checked by them. A module that isn't a connector +# belongs outside the package. +CONNECTOR_REMEDY = ( + "if it reads or writes rows, name it after its operation, " + "[_name].py, and put it directly in " + "services/v2//database_connectors; otherwise move it out of the " + "database_connectors package and name it after its layer" +) +CONNECTOR_NAME_FIX = ( + f"{CONNECTOR_REMEDY}; an exception can't stand in, because a " + "database_connectors package holds only connector modules" +) + + +def _in_connector_package(path: str) -> bool: + return path.startswith(f"{SERVICES_DIRECTORY}/") and ( + "database_connectors" in PurePosixPath(path).parts[:-1] + ) + + def _fix(rule_id: str, entry: str) -> str: """Return the rule's fix; the files rule's names the layers of its directory.""" if rule_id != FILES_RULE: return FIXES[rule_id] + if _in_connector_package(entry.split(" ", 1)[0]): + return CONNECTOR_NAME_FIX directory = ( SERVICES_DIRECTORY if entry.startswith(SERVICES_DIRECTORY) else ROUTES_DIRECTORY ) @@ -767,8 +441,11 @@ def run_check(root: Path = REPO_ROOT) -> list[Violation]: registry, violations = load_registry(root) if registry is None: return violations - excepted, violations = file_exceptions(root, registry.rule(FILES_RULE)) - findings = scan(root, excepted) + scanned = _discovered_files(root) + excepted, violations = file_exceptions( + root, registry.rule(FILES_RULE), frozenset(scanned) + ) + findings = scan(root, excepted, scanned) for rule_id in RULES: rule = registry.rule(rule_id) baseline_file = rule.baseline.file if rule and rule.baseline else None