diff --git a/AGENTS.md b/AGENTS.md index 1430243e9..68cb7ae1f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,11 +87,8 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### API v2 layer boundaries -- Checked: `api.v2.connectors-mutations-no-reads`, `api.v2.connectors-no-cross-calls`, `api.v2.connectors-reads-no-mutations`, `api.v2.crud-checks-cover-every-resource`, `api.v2.import-layers`, `api.v2.pure-modules-no-database`, `api.v2.routes-import-no-database-access`, `api.v2.session-modules-no-queries`, `api.v2.types-transformations-no-validators`, `api.v2.user-services-no-sql`, `api.v2.validators-transformations-no-db` +- Checked: `api.v2.connectors-mutations-no-reads`, `api.v2.connectors-never-commit`, `api.v2.connectors-no-cross-calls`, `api.v2.connectors-reads-no-mutations`, `api.v2.crud-checks-cover-every-resource`, `api.v2.files-assigned-to-layers`, `api.v2.import-layers`, `api.v2.pure-modules-no-database`, `api.v2.pure-modules-no-session-calls`, `api.v2.routes-import-no-database-access`, `api.v2.routes-no-sessions-or-sql`, `api.v2.services-no-sql`, `api.v2.session-modules-no-data-access`, `api.v2.session-modules-no-queries`, `api.v2.types-transformations-no-validators`, `api.v2.user-services-no-sql`, `api.v2.validators-transformations-no-db` - `api.routes.shared-query-parameters`: Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. (doc-only, not checked: follow it yourself) -- `api.v2.connectors-never-commit`: v2 connectors receive a session from the service and never open, commit, roll back or close it. (doc-only, not checked: follow it yourself) -- `api.v2.routes-no-sessions-or-sql`: v2 route modules never open database sessions or construct SQL; they call one service method and map typed failures to HTTP. (doc-only, not checked: follow it yourself) -- `api.v2.services-no-sql`: v2 services sequence work but never construct SQL expressions, call Session.exec, or define HTTP response models. (doc-only, not checked: follow it yourself) ### Runtime persistence and route boundaries diff --git a/changelog.d/4190.added.md b/changelog.d/4190.added.md new file mode 100644 index 000000000..0cc611cf1 --- /dev/null +++ b/changelog.d/4190.added.md @@ -0,0 +1 @@ +Check that every API v2 route and service file has a layer, and that v2 route modules open no session and build no SQL, services construct no SQL, connectors never open, commit, roll back or close a session, and validators, transformations and types make no session calls, in a new v2-layers quality guard. diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 23a0b5f17..fb3d964e0 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -1,8 +1,41 @@ # API v2 Code Organization Organize API v2 code first by resource and then by one explicit technical -responsibility. Route modules must not open database sessions or construct SQL. -Service modules must sequence work but must not construct or execute SQL. +responsibility. Route modules must not open database sessions or construct SQL +(rule `api.v2.routes-no-sessions-or-sql`). Service modules must sequence work +but must not construct or execute SQL (rule `api.v2.services-no-sql`). + + +## File layers + +Every `.py` file under `policyengine_api/services/v2` and +`policyengine_api/fastapi_routes/v2` has a layer, which the `v2-layers` quality +guard (`scripts/rules/v2_layers.py`) takes from the file's name: `services.py`, +`validators.py`, `transformations.py`, `types.py`, `database_session.py`, +`database_connectors/[_name].py`, `routes.py`, +`_routes.py`, `request_models.py`, `response_models.py` or `__init__.py`. +A file with another name needs an entry, with its layer and a reason, in +`rules/v2-layer-manifest.toml`, or the guard fails (rule +`api.v2.files-assigned-to-layers`). The manifest can give a file under +`services/v2` the layer `service`, `validator`, `transformation`, `types` or +`process-state`, and a file under `fastapi_routes/v2` the layer `route`, +`http-model` or `http-support`; each layer follows that layer's rules below. +A `database_session.py` sits directly in its resource package. + +The guard counts a method call as a session call when its receiver is named +`session` or `_session`, but not a request's `.session` or an HTTP +or client session (`http_session`, `requests_session`). Route modules and the +pure layers may call no method on a session at all. In routes, services and +the pure layers a call into SQLAlchemy or SQLModel counts, except to its +exception classes and, in services, to a session factory; a builder called on +a model's `__table__` counts too. A session factory is any call into +`policyengine_api.data.orm` or `policyengine_api.data.v2.database`, +SQLAlchemy's `Session`, `sessionmaker` or `create_engine`, or an injected +`*session_factory` or its `begin()`; only services and `database_session` +modules may use one. The guard doesn't +recognise a session bound to another name, an alias of a model's table, or a +name re-exported through another module. A ratcheted rule's baseline lists one +line per tolerated occurrence. ## HTTP adapters @@ -40,7 +73,13 @@ individual resource paths. Request models describe HTTP bodies. Response models describe public response response structures and OpenAPI output. Route functions handle HTTP-only conditions, -invoke one service method, and convert typed failures to HTTP responses. +invoke one service method, and convert typed failures to HTTP responses. No module +under `fastapi_routes/v2` imports SQLAlchemy, SQLModel, a connector, a +`database_session` module, the v1 ORM module `policyengine_api.data.orm` or +`policyengine_api.data.v2.database`, or calls a SQLAlchemy or SQLModel +function, a session method or a session factory (rule +`api.v2.routes-no-sessions-or-sql`; today's `sqlalchemy.exc` imports are in its +baseline). ## Resource service packages @@ -86,14 +125,20 @@ or more connector functions. A service may expose an entrypoint that accepts an existing session when several resources must change atomically in one caller- owned transaction. -Do not construct SQL expressions, call `Session.exec`, or define HTTP response -models in this module. +Do not construct SQL expressions, call `Session.exec` or another data-access +session method, or define HTTP response models in this module (rule +`api.v2.services-no-sql`, which also forbids importing FastAPI or Starlette). ### `validators.py` Define functions that inspect already-available values and either return a validated value or raise a typed exception. Validators must not load records, open sessions, execute SQL, mutate database rows, or construct HTTP responses. +Validators, transformations, types, process-state modules and package +`__init__` files import nothing from SQLAlchemy or SQLModel but the `Session` +type, import no connector or session module, and call no session method, +SQLAlchemy or SQLModel function or session factory (rule +`api.v2.pure-modules-no-session-calls`). When validation depends on stored state, a database connector loads the required rows and the service passes those rows to a validator. For example, @@ -129,7 +174,9 @@ baseline). Define the resource's database-session lifetime container. It may open and close sessions, begin transactions, commit, or roll back. It must not construct SQL, choose records, validate business rules, transform records into response -types, or determine multi-step operation order. +types, or determine multi-step operation order. It calls no data-access +session method (`add`, `get`, `merge`, `exec`, `execute` and so on) and +imports no connector (rule `api.v2.session-modules-no-data-access`). Composition code should construct this container and inject it into the service. A request-scoped read service may wrap one already-open session; a @@ -147,7 +194,7 @@ ORM rows belongs in this package. Organize connector files by CRUD operation: - `deletes.py` removes rows. Connector functions receive a session from the service. They do not open, -commit, roll back, or close it. They do not call route functions, perform +commit, roll back, or close it (rule `api.v2.connectors-never-commit`). They do not call route functions, perform request-level or business validation, or convert rows into public response types. Connector modules do not call one another to sequence a workflow; the service makes that ordering explicit. They never import one another (rule diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 46a6496d3..c425fa6e9 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 63 | 3 | 1 | 11 | 78 | +| mechanical | 68 | 4 | 1 | 8 | 81 | | metric | 1 | 0 | 0 | 0 | 1 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 67 of 80 rules. +**Blocking:** 73 of 83 rules. ## Gaps @@ -27,9 +27,6 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce - `api.process.makefile-phony`: Command-only Makefile targets are declared in .PHONY, and .PHONY stays in sync when targets are renamed or removed. - `api.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/guards/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. - `api.routes.shared-query-parameters`: Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. -- `api.v2.connectors-never-commit`: v2 connectors receive a session from the service and never open, commit, roll back or close it. -- `api.v2.routes-no-sessions-or-sql`: v2 route modules never open database sessions or construct SQL; they call one service method and map typed failures to HTTP. -- `api.v2.services-no-sql`: v2 services sequence work but never construct SQL expressions, call Session.exec, or define HTTP response models. ### Advisory (checked but not blocking) @@ -66,6 +63,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce - `api.v2.connectors-no-cross-calls`: 7 remaining in `rules/baselines/import-linter-api-v2-connectors-independent.txt` - `api.v2.pure-modules-no-database`: 1 remaining in `rules/baselines/import-linter-api-v2-pure-modules-no-database.txt` +- `api.v2.routes-no-sessions-or-sql`: 4 remaining in `rules/baselines/v2-layers-routes.txt` - `api.v2.types-transformations-no-validators`: 4 remaining in `rules/baselines/import-linter-api-v2-types-transformations-no-validators.txt` ### Exceptions and review dates @@ -84,15 +82,18 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | --- | --- | --- | --- | --- | --- | --- | --- | | `api.routes.shared-query-parameters` | Routes that accept query parameters define them through the shared typed query models, never with a second manual parser. | mechanical | doc-only | no | nothing | — | — | | `api.v2.connectors-mutations-no-reads` | v2 mutation connectors (creates, updates, deletes) never issue select or get reads. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | -| `api.v2.connectors-never-commit` | v2 connectors receive a session from the service and never open, commit, roll back or close it. | mechanical | doc-only | no | nothing | — | — | +| `api.v2.connectors-never-commit` | v2 connectors receive a session from the service and never open (with a session factory or a database_session container), commit, roll back or close it, or import the v1 ORM, the v2 database or a database_session module. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `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 has a layer, from its file name or from rules/v2-layer-manifest.toml, so the layer rules check it. | 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 and package __init__ files 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 | | `api.v2.routes-import-no-database-access` | v2 route modules never import a connector, a database_session module or the v2 database module; they call a service, which does. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | -| `api.v2.routes-no-sessions-or-sql` | v2 route modules never open database sessions or construct SQL; they call one service method and map typed failures to HTTP. | mechanical | doc-only | no | nothing | — | — | -| `api.v2.services-no-sql` | v2 services sequence work but never construct SQL expressions, call Session.exec, or define HTTP response models. | mechanical | doc-only | no | nothing | — | — | +| `api.v2.routes-no-sessions-or-sql` | v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session, v1 ORM or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP. | mechanical | ratcheted | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.services-no-sql` | v2 services sequence work but never construct SQL expressions (they import nothing from SQLAlchemy or SQLModel but the Session type and sqlalchemy.exc), call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette). | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | +| `api.v2.session-modules-no-data-access` | v2 database_session modules sit directly in their resource package and only open, commit, roll back and close sessions: they call no data-access session method (add, get, merge, exec, execute and so on) or SQL builder, and import no connector. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.session-modules-no-queries` | v2 database_session modules own session lifetime only and never build select, insert, update, delete or exec calls. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | **unproven** | | `api.v2.types-transformations-no-validators` | v2 types and transformations modules never import a validators module, directly or through other modules; validators import types, not the reverse. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.user-services-no-sql` | v2 user services (identity, accounts, legacy references and user associations) never build or run SQL themselves or import SQLAlchemy; their connectors do. | mechanical | enforced | yes | pytest `tests/unit/v2/test_data_crud_boundaries.py` | `pr.yml:test` | yes | @@ -105,17 +106,21 @@ flowchart LR r_api_v2_connectors_mutations_no_reads("api.v2.connectors-mutations-no-reads (enforced)") e_pytest_tests_unit_v2_test_data_crud_boundaries_py["pytest tests/unit/v2/test_data_crud_boundaries.py"] j_CI_pr_yml_test["CI pr.yml:test"] - r_api_v2_connectors_never_commit("api.v2.connectors-never-commit (doc-only)") + r_api_v2_connectors_never_commit("api.v2.connectors-never-commit (enforced)") + e_quality_guard_v2_layers["quality guard v2-layers"] + j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] r_api_v2_connectors_no_cross_calls("api.v2.connectors-no-cross-calls (ratcheted)") e_import_linter_pyproject_toml["import-linter pyproject.toml"] - j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] r_api_v2_connectors_reads_no_mutations("api.v2.connectors-reads-no-mutations (enforced)") r_api_v2_crud_checks_cover_every_resource("api.v2.crud-checks-cover-every-resource (enforced)") + r_api_v2_files_assigned_to_layers("api.v2.files-assigned-to-layers (enforced)") r_api_v2_import_layers("api.v2.import-layers (enforced)") r_api_v2_pure_modules_no_database("api.v2.pure-modules-no-database (ratcheted)") + r_api_v2_pure_modules_no_session_calls("api.v2.pure-modules-no-session-calls (enforced)") r_api_v2_routes_import_no_database_access("api.v2.routes-import-no-database-access (enforced)") - r_api_v2_routes_no_sessions_or_sql("api.v2.routes-no-sessions-or-sql (doc-only)") - r_api_v2_services_no_sql("api.v2.services-no-sql (doc-only)") + r_api_v2_routes_no_sessions_or_sql("api.v2.routes-no-sessions-or-sql (ratcheted)") + r_api_v2_services_no_sql("api.v2.services-no-sql (enforced)") + r_api_v2_session_modules_no_data_access("api.v2.session-modules-no-data-access (enforced)") r_api_v2_session_modules_no_queries("api.v2.session-modules-no-queries (enforced)") r_api_v2_types_transformations_no_validators("api.v2.types-transformations-no-validators (ratcheted)") r_api_v2_user_services_no_sql("api.v2.user-services-no-sql (enforced)") @@ -123,16 +128,20 @@ flowchart LR r_api_routes_shared_query_parameters -.-> gap_unenforced r_api_v2_connectors_mutations_no_reads --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py e_pytest_tests_unit_v2_test_data_crud_boundaries_py --> j_CI_pr_yml_test - r_api_v2_connectors_never_commit -.-> gap_unenforced + r_api_v2_connectors_never_commit --> e_quality_guard_v2_layers + e_quality_guard_v2_layers --> j_CI_pr_yml_quality_guards r_api_v2_connectors_no_cross_calls --> e_import_linter_pyproject_toml e_import_linter_pyproject_toml --> j_CI_pr_yml_quality_guards r_api_v2_connectors_reads_no_mutations --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py r_api_v2_crud_checks_cover_every_resource --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py + r_api_v2_files_assigned_to_layers --> e_quality_guard_v2_layers r_api_v2_import_layers --> e_import_linter_pyproject_toml r_api_v2_pure_modules_no_database --> e_import_linter_pyproject_toml + r_api_v2_pure_modules_no_session_calls --> e_quality_guard_v2_layers r_api_v2_routes_import_no_database_access --> e_import_linter_pyproject_toml - r_api_v2_routes_no_sessions_or_sql -.-> gap_unenforced - r_api_v2_services_no_sql -.-> gap_unenforced + r_api_v2_routes_no_sessions_or_sql --> e_quality_guard_v2_layers + r_api_v2_services_no_sql --> e_quality_guard_v2_layers + r_api_v2_session_modules_no_data_access --> e_quality_guard_v2_layers r_api_v2_session_modules_no_queries --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py r_api_v2_types_transformations_no_validators --> e_import_linter_pyproject_toml r_api_v2_user_services_no_sql --> e_pytest_tests_unit_v2_test_data_crud_boundaries_py diff --git a/rules/baselines/v2-layers-routes.txt b/rules/baselines/v2-layers-routes.txt new file mode 100644 index 000000000..a7684b874 --- /dev/null +++ b/rules/baselines/v2-layers-routes.txt @@ -0,0 +1,8 @@ +# Tolerated existing violations. This list may only shrink. +# Each line is one finding of the v2-layers guard for rule api.v2.routes-no-sessions-or-sql, +# as the guard prints it. These route modules import sqlalchemy.exc to catch SQLAlchemyError; +# the fix is for their services to raise typed failures instead. +policyengine_api/fastapi_routes/v2/countries/routes.py imports sqlalchemy.exc +policyengine_api/fastapi_routes/v2/households/routes.py imports sqlalchemy.exc +policyengine_api/fastapi_routes/v2/policies/routes.py imports sqlalchemy.exc +policyengine_api/fastapi_routes/v2/user_resources.py imports sqlalchemy.exc diff --git a/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml b/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml new file mode 100644 index 000000000..caed48788 --- /dev/null +++ b/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml @@ -0,0 +1,19 @@ +# Proof fixture for rule api.v2.connectors-never-commit. The guard must reject +# it: the connector opens its own session and commits it. + +expected_findings = [ + "policyengine_api/services/v2/policies/database_connectors/creates.py calls get_v2_session_factory", + "policyengine_api/services/v2/policies/database_connectors/creates.py calls session.commit", + "policyengine_api/services/v2/policies/database_connectors/creates.py imports policyengine_api.data.v2.database", +] + +[files] +"policyengine_api/services/v2/policies/database_connectors/creates.py" = """ +from policyengine_api.data.v2.database import get_v2_session_factory + + +def create_policy(row): + session = get_v2_session_factory()() + session.add(row) + session.commit() +""" diff --git a/rules/fixtures/api.v2.connectors-never-commit/good_tree.toml b/rules/fixtures/api.v2.connectors-never-commit/good_tree.toml new file mode 100644 index 000000000..e4d2dfcc2 --- /dev/null +++ b/rules/fixtures/api.v2.connectors-never-commit/good_tree.toml @@ -0,0 +1,16 @@ +# Proof fixture for rule api.v2.connectors-never-commit. The guard must accept +# it: the connector adds the row to the session it receives and flushes it; +# the service's database-session container commits. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/policies/database_connectors/creates.py" = """ +from sqlmodel import Session + + +def create_policy(session: Session, row): + session.add(row) + session.flush() + return row +""" 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 new file mode 100644 index 000000000..872476c4e --- /dev/null +++ b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml @@ -0,0 +1,18 @@ +# Proof fixture for rule api.v2.files-assigned-to-layers. The guard must reject +# it: simulation_client.py matches no layer's file name and no manifest entry +# names it (the case of an open pull request that adds a v2 resource). +# scripts/rules/tests/test_v2_layers.py writes each file into a temporary +# directory and runs the v2-layers guard's scan on it, with no manifest. + +expected_findings = [ + "policyengine_api/services/v2/household_calculations/simulation_client.py matches no v2 layer", +] + +[files] +"policyengine_api/services/v2/household_calculations/__init__.py" = "" +"policyengine_api/services/v2/household_calculations/services.py" = """ +from policyengine_api.services.v2.household_calculations.simulation_client import submit +""" +"policyengine_api/services/v2/household_calculations/simulation_client.py" = """ +import httpx +""" diff --git a/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml b/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml new file mode 100644 index 000000000..72f7a07cb --- /dev/null +++ b/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml @@ -0,0 +1,23 @@ +# Proof fixture for rule api.v2.files-assigned-to-layers. The guard must accept +# it: every file is named after its layer. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/__init__.py" = "" +"policyengine_api/services/v2/policies/__init__.py" = "" +"policyengine_api/services/v2/policies/services.py" = "" +"policyengine_api/services/v2/policies/validators.py" = "" +"policyengine_api/services/v2/policies/transformations.py" = "" +"policyengine_api/services/v2/policies/types.py" = "" +"policyengine_api/services/v2/policies/database_session.py" = "" +"policyengine_api/services/v2/policies/database_connectors/__init__.py" = "" +"policyengine_api/services/v2/policies/database_connectors/creates.py" = "" +"policyengine_api/services/v2/policies/database_connectors/reads_parameters.py" = "" +"policyengine_api/fastapi_routes/v2/__init__.py" = "" +"policyengine_api/fastapi_routes/v2/routes.py" = "" +"policyengine_api/fastapi_routes/v2/policies/__init__.py" = "" +"policyengine_api/fastapi_routes/v2/policies/routes.py" = "" +"policyengine_api/fastapi_routes/v2/policies/request_models.py" = "" +"policyengine_api/fastapi_routes/v2/policies/response_models.py" = "" +"policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py" = "" diff --git a/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml new file mode 100644 index 000000000..f0ebcdcf5 --- /dev/null +++ b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml @@ -0,0 +1,34 @@ +# Proof fixture for rule api.v2.pure-modules-no-session-calls. The guard must +# reject it: a validator loads a row through the session it was handed, a +# types module runs a query, and a package __init__ commits. +# scripts/rules/tests/test_v2_layers.py writes each file into a temporary +# directory and runs the v2-layers guard's scan on it, with no manifest. + +expected_findings = [ + "policyengine_api/services/v2/policies/__init__.py calls session.commit", + "policyengine_api/services/v2/policies/types.py calls session.exec", + "policyengine_api/services/v2/policies/types.py calls sqlmodel.select", + "policyengine_api/services/v2/policies/types.py imports sqlmodel.select", + "policyengine_api/services/v2/policies/validators.py calls session.get", +] + +[files] +"policyengine_api/services/v2/policies/validators.py" = """ +from policyengine_api.data.v2.models import Policy + + +def validate_policy_exists(session, policy_id): + if session.get(Policy, policy_id) is None: + raise LookupError(policy_id) +""" +"policyengine_api/services/v2/policies/types.py" = """ +from sqlmodel import select + + +def latest(session, model): + return session.exec(select(model)).first() +""" +"policyengine_api/services/v2/policies/__init__.py" = """ +def flush_everything(session): + session.commit() +""" diff --git a/rules/fixtures/api.v2.pure-modules-no-session-calls/good_tree.toml b/rules/fixtures/api.v2.pure-modules-no-session-calls/good_tree.toml new file mode 100644 index 000000000..900fdd36d --- /dev/null +++ b/rules/fixtures/api.v2.pure-modules-no-session-calls/good_tree.toml @@ -0,0 +1,19 @@ +# Proof fixture for rule api.v2.pure-modules-no-session-calls. The guard must +# accept it: the validator checks a row a connector already loaded, and the +# package __init__ holds only a docstring. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/policies/validators.py" = """ +from policyengine_api.data.v2.models import Policy + + +def validate_policy_exists(policy: Policy | None, policy_id): + if policy is None: + raise LookupError(policy_id) + return policy +""" +"policyengine_api/services/v2/policies/__init__.py" = """ +\"\"\"Immutable v2 policies.\"\"\" +""" diff --git a/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml b/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml new file mode 100644 index 000000000..4d6b3fe08 --- /dev/null +++ b/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml @@ -0,0 +1,22 @@ +# Proof fixture for rule api.v2.routes-no-sessions-or-sql. The guard must +# reject it: a route module imports a connector and SQLModel, builds a select +# and calls a method on a session. + +expected_findings = [ + "policyengine_api/fastapi_routes/v2/policies/routes.py calls session.add", + "policyengine_api/fastapi_routes/v2/policies/routes.py calls sqlmodel.select", + "policyengine_api/fastapi_routes/v2/policies/routes.py imports policyengine_api.services.v2.policies.database_connectors.reads", + "policyengine_api/fastapi_routes/v2/policies/routes.py imports sqlmodel", +] + +[files] +"policyengine_api/fastapi_routes/v2/policies/routes.py" = """ +from sqlmodel import select + +from policyengine_api.services.v2.policies.database_connectors.reads import read_policy + + +def create_policy(session, row): + session.add(row) + return read_policy(session, select(row)) +""" diff --git a/rules/fixtures/api.v2.routes-no-sessions-or-sql/good_tree.toml b/rules/fixtures/api.v2.routes-no-sessions-or-sql/good_tree.toml new file mode 100644 index 000000000..cf65a1149 --- /dev/null +++ b/rules/fixtures/api.v2.routes-no-sessions-or-sql/good_tree.toml @@ -0,0 +1,15 @@ +# Proof fixture for rule api.v2.routes-no-sessions-or-sql. The guard must +# accept it: the route calls one service method. Adding to a set is not a +# session call. + +expected_findings = [] + +[files] +"policyengine_api/fastapi_routes/v2/policies/routes.py" = """ +from policyengine_api.services.v2.policies.services import V2PolicyService + + +def create_policy(service: V2PolicyService, body, seen_ids: set): + seen_ids.add(body.id) + return service.create_policy(body) +""" diff --git a/rules/fixtures/api.v2.services-no-sql/bad_tree.toml b/rules/fixtures/api.v2.services-no-sql/bad_tree.toml new file mode 100644 index 000000000..4bd955ed0 --- /dev/null +++ b/rules/fixtures/api.v2.services-no-sql/bad_tree.toml @@ -0,0 +1,22 @@ +# Proof fixture for rule api.v2.services-no-sql. The guard must reject it: the +# service imports and builds a select, runs it with Session.exec and defines +# an HTTP response with FastAPI. + +expected_findings = [ + "policyengine_api/services/v2/policies/services.py calls _session.exec", + "policyengine_api/services/v2/policies/services.py calls sqlmodel.select", + "policyengine_api/services/v2/policies/services.py imports fastapi.responses", + "policyengine_api/services/v2/policies/services.py imports sqlmodel.select", +] + +[files] +"policyengine_api/services/v2/policies/services.py" = """ +from fastapi.responses import JSONResponse +from sqlmodel import select + + +class V2PolicyService: + def get_policy(self, policy_id): + row = self._session.exec(select(policy_id)).one() + return JSONResponse(row) +""" diff --git a/rules/fixtures/api.v2.services-no-sql/good_tree.toml b/rules/fixtures/api.v2.services-no-sql/good_tree.toml new file mode 100644 index 000000000..210358c37 --- /dev/null +++ b/rules/fixtures/api.v2.services-no-sql/good_tree.toml @@ -0,0 +1,16 @@ +# Proof fixture for rule api.v2.services-no-sql. The guard must accept it: the +# service opens a read through its database-session container and passes the +# session to a connector. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/policies/services.py" = """ +from policyengine_api.services.v2.policies.database_connectors.reads import read_policy + + +class V2PolicyService: + def get_policy(self, policy_id): + with self._database_session.read() as session: + return read_policy(session, policy_id) +""" diff --git a/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml b/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml new file mode 100644 index 000000000..882e00f99 --- /dev/null +++ b/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml @@ -0,0 +1,25 @@ +# Proof fixture for rule api.v2.session-modules-no-data-access. The guard must +# reject it: the database_session module adds a row, loads one with +# Session.get and runs raw SQL, work that belongs in a connector. +# scripts/rules/tests/test_v2_layers.py writes each file into a temporary +# directory and runs the v2-layers guard's scan on it, with no manifest. + +expected_findings = [ + "policyengine_api/services/v2/policies/database_session.py calls session.add", + "policyengine_api/services/v2/policies/database_session.py calls session.execute", + "policyengine_api/services/v2/policies/database_session.py calls session.get", + "policyengine_api/services/v2/policies/database_session.py calls sqlalchemy.text", +] + +[files] +"policyengine_api/services/v2/policies/database_session.py" = """ +from sqlalchemy import text + + +class PolicyDatabaseSession: + def save(self, row, policy_id): + with self._session_factory.begin() as session: + session.add(row) + session.execute(text("DELETE FROM policies")) + return session.get(type(row), policy_id) +""" diff --git a/rules/fixtures/api.v2.session-modules-no-data-access/good_tree.toml b/rules/fixtures/api.v2.session-modules-no-data-access/good_tree.toml new file mode 100644 index 000000000..c271d4ee7 --- /dev/null +++ b/rules/fixtures/api.v2.session-modules-no-data-access/good_tree.toml @@ -0,0 +1,28 @@ +# Proof fixture for rule api.v2.session-modules-no-data-access. The guard must +# accept it: the module opens sessions and transactions, commits and rolls +# back, and leaves reads and writes to the connectors. + +expected_findings = [] + +[files] +"policyengine_api/services/v2/policies/database_session.py" = """ +from contextlib import contextmanager + + +class PolicyDatabaseSession: + @contextmanager + def read(self): + with self._session_factory() as session: + yield session + + @contextmanager + def transaction(self): + with self._session_factory.begin() as session: + savepoint = session.begin_nested() + try: + yield session + savepoint.commit() + except Exception: + savepoint.rollback() + raise +""" diff --git a/rules/registry.toml b/rules/registry.toml index 2002491f8..3ddccd0c6 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -180,29 +180,80 @@ workflow = ".github/workflows/pr.yml" job = "test" 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 has a layer, from its file name or from rules/v2-layer-manifest.toml, so the layer rules check it." +rationale = "docs/engineering/skills/v2-code-organization.md#file-layers" +scope = ["policyengine_api/services/v2/**/*.py", "policyengine_api/fastapi_routes/v2/**/*.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_files_assigned_to_layers" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" + [[rule]] id = "api.v2.routes-no-sessions-or-sql" category = "v2-layers" -statement = "v2 route modules never open database sessions or construct SQL; they call one service method and map typed failures to HTTP." +statement = "v2 route modules never open database sessions or construct SQL: they import no SQLAlchemy, SQLModel, connector, database_session, v1 ORM or v2 database module, and call no SQL builder, session method or session factory; they call one service method and map typed failures to HTTP." rationale = "docs/engineering/skills/v2-code-organization.md#http-adapters" scope = ["policyengine_api/fastapi_routes/v2/**/*.py"] tier = "mechanical" -status = "doc-only" +status = "ratcheted" autofix = false [rule.enforcer] -kind = "none" +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.routes-no-sessions-or-sql/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_routes_no_sessions_or_sql" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" +[rule.baseline] +file = "rules/baselines/v2-layers-routes.txt" +count = 4 [[rule]] id = "api.v2.services-no-sql" category = "v2-layers" -statement = "v2 services sequence work but never construct SQL expressions, call Session.exec, or define HTTP response models." +statement = "v2 services sequence work but never construct SQL expressions (they import nothing from SQLAlchemy or SQLModel but the Session type and sqlalchemy.exc), call Session.exec or another data-access session method, or define HTTP response models (they import no FastAPI or Starlette)." rationale = "docs/engineering/skills/v2-code-organization.md#servicespy" scope = ["policyengine_api/services/v2/*/services.py"] tier = "mechanical" -status = "doc-only" +status = "enforced" autofix = false [rule.enforcer] -kind = "none" +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.services-no-sql/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.services-no-sql/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_services_no_sql" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" [[rule]] id = "api.v2.user-services-no-sql" @@ -225,17 +276,77 @@ bad_fixture = "rules/fixtures/api.v2.user-services-no-sql/bad_services.py" good_fixture = "rules/fixtures/api.v2.user-services-no-sql/good_services.py" test = "tests/unit/v2/test_data_crud_boundaries.py::test_rule_proof_user_services_no_sql" +[[rule]] +id = "api.v2.pure-modules-no-session-calls" +category = "v2-layers" +statement = "v2 validators, transformations, types, process-state modules and package __init__ files 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." +rationale = "docs/engineering/skills/v2-code-organization.md#validatorspy" +scope = ["policyengine_api/services/v2/*/validators.py", "policyengine_api/services/v2/*/transformations.py", "policyengine_api/services/v2/*/types.py", "policyengine_api/services/v2/metadata/selected_catalogs.py", "policyengine_api/services/v2/__init__.py", "policyengine_api/services/v2/*/__init__.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.pure-modules-no-session-calls/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_pure_modules_no_session_calls" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" + +[[rule]] +id = "api.v2.session-modules-no-data-access" +category = "v2-layers" +statement = "v2 database_session modules sit directly in their resource package and only open, commit, roll back and close sessions: they call no data-access session method (add, get, merge, exec, execute and so on) or SQL builder, and import no connector." +rationale = "docs/engineering/skills/v2-code-organization.md#database_sessionpy" +scope = ["policyengine_api/services/v2/*/database_session.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.session-modules-no-data-access/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_session_modules_no_data_access" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" + [[rule]] id = "api.v2.connectors-never-commit" category = "v2-layers" -statement = "v2 connectors receive a session from the service and never open, commit, roll back or close it." +statement = "v2 connectors receive a session from the service and never open (with a session factory or a database_session container), commit, roll back or close it, or import the v1 ORM, the v2 database or a database_session module." rationale = "docs/engineering/skills/v2-code-organization.md#database_connectors" scope = ["policyengine_api/services/v2/*/database_connectors/*.py"] tier = "mechanical" -status = "doc-only" +status = "enforced" autofix = false [rule.enforcer] -kind = "none" +kind = "quality-guard" +name = "v2-layers" +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "run_quality_guards.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml" +good_fixture = "rules/fixtures/api.v2.connectors-never-commit/good_tree.toml" +test = "scripts/rules/tests/test_v2_layers.py::TestV2LayersProof::test_rule_proof_connectors_never_commit" +workflow = ".github/workflows/pr.yml" +job = "quality-guards" +invocation = "unittest discover -s scripts/rules/tests" [[rule]] id = "api.v2.connectors-no-cross-calls" diff --git a/rules/v2-layer-manifest.toml b/rules/v2-layer-manifest.toml new file mode 100644 index 000000000..985ffcdca --- /dev/null +++ b/rules/v2-layer-manifest.toml @@ -0,0 +1,50 @@ +# Layers for the API v2 files whose names don't give one. +# +# The v2-layers quality guard (scripts/rules/v2_layers.py) assigns every .py +# file under policyengine_api/services/v2 and policyengine_api/fastapi_routes/v2 +# a layer by its file name: services.py, validators.py, transformations.py, +# types.py, database_session.py, database_connectors/[_name].py, +# routes.py, _routes.py, request_models.py, response_models.py or +# __init__.py. A file with another name needs an entry here, or the guard fails +# (rule api.v2.files-assigned-to-layers). Every file under fastapi_routes/v2 +# follows rule api.v2.routes-no-sessions-or-sql whatever its layer. +# +# Layers: http-model (request or response schemas), http-support (shared HTTP +# code for route modules) and process-state (data a service keeps for the +# process, docs/engineering/skills/v2-code-organization.md#resource-service-packages). + +[files."policyengine_api/fastapi_routes/v2/errors.py"] +layer = "http-support" +reason = "The error response and serialization every v2 route shares." + +[files."policyengine_api/fastapi_routes/v2/households/document_models.py"] +layer = "http-model" +reason = "Household document schemas shared by the request and response models." + +[files."policyengine_api/fastapi_routes/v2/metadata/common.py"] +layer = "http-support" +reason = "Runs metadata reads for the route modules and maps their failures to HTTP, and the route class that sets caching headers." + +[files."policyengine_api/fastapi_routes/v2/metadata/query_dependencies.py"] +layer = "http-support" +reason = "Typed FastAPI dependencies for the shared metadata query models." + +[files."policyengine_api/fastapi_routes/v2/metadata/response_cache.py"] +layer = "http-support" +reason = "The process-wide store of serialized whole-version responses and their entity tags." + +[files."policyengine_api/fastapi_routes/v2/metadata/search_file_response.py"] +layer = "http-support" +reason = "The HTTP representation of a stored parameter search file, with content encoding." + +[files."policyengine_api/fastapi_routes/v2/user_auth.py"] +layer = "http-support" +reason = "Resolves request credentials to an account principal for user-owned routes." + +[files."policyengine_api/fastapi_routes/v2/user_resources.py"] +layer = "http-support" +reason = "HTTP conventions shared by the user-owned resource routes." + +[files."policyengine_api/services/v2/metadata/selected_catalogs.py"] +layer = "process-state" +reason = "The catalog versions metadata reads have selected, kept for the process." diff --git a/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py new file mode 100644 index 000000000..4e338dbe6 --- /dev/null +++ b/scripts/rules/tests/test_v2_layers.py @@ -0,0 +1,791 @@ +"""Tests for the v2-layers quality guard (scripts/rules/v2_layers.py).""" + +from __future__ import annotations + +import tempfile +import textwrap +import tomllib +import unittest +from pathlib import Path + +from scripts.rules.v2_layers import ( + CONNECTORS_RULE, + FILES_RULE, + PURE_RULE, + ROUTES_RULE, + SERVICES_RULE, + SESSION_RULE, + assign_layer, + run_check, + scan, +) + +REPO_ROOT = Path(__file__).resolve().parents[3] +FIXTURES = REPO_ROOT / "rules" / "fixtures" +ROUTE = "policyengine_api/fastapi_routes/v2/policies/routes.py" +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" + + +def _write_files(files: dict[str, str], root: Path) -> None: + for relative_path, source in files.items(): + path = root / relative_path + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(textwrap.dedent(source)) + + +def _findings(files: dict[str, str], rule_id: str) -> set[str]: + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + _write_files(files, root) + return { + str(finding) for finding in scan(root, {}) if finding.rule_id == rule_id + } + + +def _assert_proof(test: unittest.TestCase, rule_id: str) -> None: + for fixture in ("bad_tree.toml", "good_tree.toml"): + tree = tomllib.loads((FIXTURES / rule_id / fixture).read_text()) + with test.subTest(fixture=fixture): + test.assertEqual( + _findings(tree["files"], rule_id), set(tree["expected_findings"]) + ) + bad = tomllib.loads((FIXTURES / rule_id / "bad_tree.toml").read_text()) + test.assertTrue(bad["expected_findings"], "the bad fixture must be rejected") + + +class TestV2LayersProof(unittest.TestCase): + """Proofs: a bad tree gives exactly its expected findings, a good tree none.""" + + def test_rule_proof_files_assigned_to_layers(self) -> None: + _assert_proof(self, FILES_RULE) + + def test_rule_proof_routes_no_sessions_or_sql(self) -> None: + _assert_proof(self, ROUTES_RULE) + + def test_rule_proof_services_no_sql(self) -> None: + _assert_proof(self, SERVICES_RULE) + + def test_rule_proof_connectors_never_commit(self) -> None: + _assert_proof(self, CONNECTORS_RULE) + + def test_rule_proof_pure_modules_no_session_calls(self) -> None: + _assert_proof(self, PURE_RULE) + + def test_rule_proof_session_modules_no_data_access(self) -> None: + _assert_proof(self, SESSION_RULE) + + +class TestLayerAssignment(unittest.TestCase): + def test_files_are_assigned_by_name(self) -> None: + cases = { + "policyengine_api/services/v2/__init__.py": "package", + "policyengine_api/services/v2/policies/services.py": "service", + "policyengine_api/services/v2/policies/validators.py": "validator", + "policyengine_api/services/v2/policies/transformations.py": ( + "transformation" + ), + "policyengine_api/services/v2/policies/types.py": "types", + "policyengine_api/services/v2/policies/database_session.py": "session", + "policyengine_api/services/v2/policies/database_connectors/reads.py": ( + "connector" + ), + "policyengine_api/services/v2/metadata/database_connectors/" + "reads_parameter_tree.py": "connector", + "policyengine_api/fastapi_routes/v2/routes.py": "route", + "policyengine_api/fastapi_routes/v2/policies/routes.py": "route", + "policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py": "route", + "policyengine_api/fastapi_routes/v2/policies/request_models.py": ( + "http-model" + ), + "policyengine_api/fastapi_routes/v2/policies/response_models.py": ( + "http-model" + ), + } + for path, layer in cases.items(): + with self.subTest(path=path): + self.assertEqual(assign_layer(path, {}), layer) + + def test_other_names_have_no_layer_unless_the_manifest_assigns_one(self) -> None: + unnamed = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + misnamed_connector = ( + "policyengine_api/services/v2/policies/database_connectors/helpers.py" + ) + + self.assertIsNone(assign_layer(unnamed, {})) + self.assertIsNone(assign_layer(misnamed_connector, {})) + self.assertEqual( + assign_layer(unnamed, {unnamed: "http-support"}), "http-support" + ) + + def test_every_file_in_the_repository_has_a_layer(self) -> None: + violations = [ + str(violation) + for violation in run_check(REPO_ROOT) + if violation.rule_id == FILES_RULE + ] + + self.assertEqual(violations, []) + + +class TestCallChecks(unittest.TestCase): + def test_only_calls_on_a_session_count_as_session_calls(self) -> None: + source = """ + def handle(session, read_session, names: set, cache: dict): + names.add("a") + cache.get("b") + read_session.exec("c") + session.get("d") + """ + + self.assertEqual( + _findings({ROUTE: source}, ROUTES_RULE), + { + f"{ROUTE} calls read_session.exec", + f"{ROUTE} calls session.get", + }, + ) + + def test_sql_builders_count_only_when_imported_from_sqlalchemy_or_sqlmodel( + self, + ) -> None: + source = """ + import sqlalchemy as sa + from policyengine_api.services.v2.policies.queries import select + + + def handle(): + select("local helper") + return sa.text("raw") + """ + + self.assertEqual( + _findings({SERVICE: source}, SERVICES_RULE), + {f"{SERVICE} calls sqlalchemy.text", f"{SERVICE} imports sqlalchemy"}, + ) + + def test_imports_under_type_checking_count(self) -> None: + source = """ + from typing import TYPE_CHECKING + + if TYPE_CHECKING: + from policyengine_api.services.v2.policies.database_session import ( + PolicyDatabaseSession, + ) + """ + + self.assertEqual( + _findings({ROUTE: source}, ROUTES_RULE), + {f"{ROUTE} imports policyengine_api.services.v2.policies.database_session"}, + ) + + def test_package_and_relative_imports_are_resolved(self) -> None: + source = """ + from policyengine_api.data.v2 import database + from .. import database_session + """ + + self.assertEqual( + _findings({CONNECTOR: source}, CONNECTORS_RULE), + { + f"{CONNECTOR} imports policyengine_api.data.v2.database", + f"{CONNECTOR} imports " + "policyengine_api.services.v2.policies.database_session", + }, + ) + + def test_connectors_may_add_and_flush_but_not_end_a_transaction(self) -> None: + source = """ + def write(session, row): + session.add(row) + session.flush() + session.rollback() + session.close() + """ + + self.assertEqual( + _findings({CONNECTOR: source}, CONNECTORS_RULE), + {f"{CONNECTOR} calls session.rollback", f"{CONNECTOR} calls session.close"}, + ) + + +class TestEveryLayerIsChecked(unittest.TestCase): + """Only connectors and session modules may touch a session (review findings).""" + + SESSION_WORK = """ + from sqlmodel import select + + + def work(session, row): + session.add(row) + session.commit() + return session.exec(select(row)) + """ + + def test_validators_transformations_types_and_process_state_touch_no_session( + self, + ) -> None: + for path in ( + "policyengine_api/services/v2/policies/validators.py", + "policyengine_api/services/v2/policies/transformations.py", + "policyengine_api/services/v2/policies/types.py", + "policyengine_api/services/v2/policies/__init__.py", + ): + with self.subTest(path=path): + self.assertEqual( + _findings({path: self.SESSION_WORK}, PURE_RULE), + { + f"{path} calls session.add", + f"{path} calls session.commit", + f"{path} calls session.exec", + f"{path} calls sqlmodel.select", + f"{path} imports sqlmodel.select", + }, + ) + + def test_a_connector_package_init_is_a_connector(self) -> None: + path = "policyengine_api/services/v2/policies/database_connectors/__init__.py" + + self.assertEqual( + _findings({path: self.SESSION_WORK}, CONNECTORS_RULE), + {f"{path} calls session.commit"}, + ) + + def test_the_repository_session_factories_count(self) -> None: + route_source = """ + from policyengine_api.data.orm import get_v1_session_factory + from policyengine_api.data.v2.database import build_v2_session_factory + + + def route(database_session): + with database_session.transaction() as session: + get_v1_session_factory() + build_v2_session_factory() + """ + connector_source = """ + from policyengine_api.data.orm import get_v1_engine + + + def read(): + return get_v1_engine() + """ + + self.assertEqual( + _findings({ROUTE: route_source}, ROUTES_RULE), + { + f"{ROUTE} calls build_v2_session_factory", + f"{ROUTE} calls database_session.transaction", + f"{ROUTE} calls get_v1_session_factory", + f"{ROUTE} imports policyengine_api.data.orm", + f"{ROUTE} imports policyengine_api.data.v2.database", + }, + ) + self.assertEqual( + _findings({CONNECTOR: connector_source}, CONNECTORS_RULE), + { + f"{CONNECTOR} calls get_v1_engine", + f"{CONNECTOR} imports policyengine_api.data.orm", + }, + ) + + def test_services_import_only_the_session_type_and_errors_from_sql_packages( + self, + ) -> None: + source = """ + from typing import TYPE_CHECKING + + from sqlalchemy import and_, func, or_ + from sqlalchemy.exc import IntegrityError + from sqlmodel import col + + if TYPE_CHECKING: + from sqlmodel import Session + + + def build(Policy, ids): + Policy.__table__.select() + return and_(col(Policy.id).in_(ids), or_(func.lower(Policy.name))) + """ + + self.assertEqual( + _findings({SERVICE: source}, SERVICES_RULE), + { + f"{SERVICE} calls Policy.__table__.select", + f"{SERVICE} calls sqlalchemy.and_", + f"{SERVICE} calls sqlalchemy.func.lower", + f"{SERVICE} calls sqlalchemy.or_", + f"{SERVICE} calls sqlmodel.col", + f"{SERVICE} imports sqlalchemy.and_", + f"{SERVICE} imports sqlalchemy.func", + f"{SERVICE} imports sqlalchemy.or_", + f"{SERVICE} imports sqlmodel.col", + }, + ) + + def test_names_that_only_contain_session_are_not_sessions(self) -> None: + source = """ + @sessions_router.get("/sessions") + def route(request, session_ids: set): + session_ids.add(1) + request.session.get("user") + return self._sessions_by_country.get("us") + """ + + self.assertEqual(_findings({ROUTE: source}, ROUTES_RULE), set()) + + +class TestSecondReview(unittest.TestCase): + """Cases from the second review round.""" + + def test_routes_and_pure_layers_may_call_no_session_method_at_all(self) -> None: + source = """ + def handle(session, row): + session.get_one(row, 1) + session.bulk_save_objects([row]) + session.connection() + session.expunge(row) + """ + validator = "policyengine_api/services/v2/policies/validators.py" + + for path, rule_id in ((ROUTE, ROUTES_RULE), (validator, PURE_RULE)): + with self.subTest(path=path): + self.assertEqual( + _findings({path: source}, rule_id), + { + f"{path} calls session.bulk_save_objects", + f"{path} calls session.connection", + f"{path} calls session.expunge", + f"{path} calls session.get_one", + }, + ) + + def test_services_and_connectors_know_every_relevant_session_method(self) -> None: + service_source = """ + class Service: + def run(self, rows): + with self._database_session.transaction(): + self._session.get_one(rows, 1) + self._session.bulk_insert_mappings(rows, []) + """ + connector_source = """ + def write(session): + session.reset() + session.invalidate() + """ + + self.assertEqual( + _findings({SERVICE: service_source}, SERVICES_RULE), + { + f"{SERVICE} calls _session.bulk_insert_mappings", + f"{SERVICE} calls _session.get_one", + }, + ) + self.assertEqual( + _findings({CONNECTOR: connector_source}, CONNECTORS_RULE), + { + f"{CONNECTOR} calls session.invalidate", + f"{CONNECTOR} calls session.reset", + }, + ) + + def test_manifest_layers_get_their_layer_s_checks(self) -> None: + process_state = "policyengine_api/services/v2/metadata/selected_catalogs.py" + support = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + manifest = {process_state: "process-state", support: "http-support"} + files = { + process_state: ( + "from policyengine_api.data.orm import get_v1_session_factory\n" + "from .database_connectors.reads import read_rows\n" + "def load(session):\n" + " get_v1_session_factory()\n" + " return session.exec(read_rows)\n" + ), + support: "def keep(session, row):\n session.add(row)\n", + } + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + _write_files(files, root) + found = { + (finding.rule_id, str(finding)) for finding in scan(root, manifest) + } + + self.assertEqual( + found, + { + (PURE_RULE, f"{process_state} calls get_v1_session_factory"), + (PURE_RULE, f"{process_state} calls session.exec"), + (PURE_RULE, f"{process_state} imports policyengine_api.data.orm"), + ( + PURE_RULE, + f"{process_state} imports " + "policyengine_api.services.v2.metadata.database_connectors.reads", + ), + (ROUTES_RULE, f"{support} calls session.add"), + }, + ) + + def test_a_session_module_below_a_resource_package_has_no_layer(self) -> None: + nested = "policyengine_api/services/v2/metadata/catalogs/database_session.py" + + self.assertIsNone(assign_layer(nested, {})) + self.assertEqual( + assign_layer( + "policyengine_api/services/v2/metadata/database_session.py", {} + ), + "session", + ) + + def test_every_call_into_sqlalchemy_counts_except_its_exceptions(self) -> None: + source = """ + import sqlalchemy.exc + + + def build(Policy): + try: + return sqlalchemy.and_(sqlalchemy.func.lower(Policy.name) == "x") + except sqlalchemy.exc.IntegrityError: + raise sqlalchemy.exc.IntegrityError("x", None, None) + """ + + self.assertEqual( + _findings({SERVICE: source}, SERVICES_RULE), + { + f"{SERVICE} calls sqlalchemy.and_", + f"{SERVICE} calls sqlalchemy.func.lower", + }, + ) + + def test_web_sessions_and_clients_are_not_database_sessions(self) -> None: + source = """ + def handle(self, http_request, requests_session): + self.request.session.get("user") + http_request.session.get("user") + self._client_session.get("https://example.org") + requests_session.get("https://example.org") + """ + + self.assertEqual(_findings({ROUTE: source}, ROUTES_RULE), set()) + + def test_only_a_builder_called_on_a_table_counts(self) -> None: + source = """ + from policyengine_api.data.v2.models import Policy + + + def build(rows): + Policy.__table__.insert().values(name="x") + Policy.__table__.columns.values() + rows[Policy.__table__.name].values() + """ + + self.assertEqual( + _findings({SERVICE: source}, SERVICES_RULE), + {f"{SERVICE} calls Policy.__table__.insert"}, + ) + + +class TestThirdReview(unittest.TestCase): + """Cases from the third review round.""" + + def test_injected_session_factories_and_containers_open_sessions(self) -> None: + connector_source = """ + def create(session_factory, database_session, row): + with session_factory.begin() as session: + session.add(row) + with database_session.transaction() as session: + session.add(row) + + + class Writer: + def write(self, row): + with self._session_factory() as session: + session.add(row) + """ + route_source = """ + def route(session_factory, row): + with session_factory.begin() as db: + return db + """ + + self.assertEqual( + _findings({CONNECTOR: connector_source}, CONNECTORS_RULE), + { + f"{CONNECTOR} calls _session_factory", + f"{CONNECTOR} calls database_session.transaction", + f"{CONNECTOR} calls session_factory.begin", + }, + ) + self.assertEqual( + _findings({ROUTE: route_source}, ROUTES_RULE), + {f"{ROUTE} calls session_factory.begin"}, + ) + + def test_request_session_data_is_recognised_by_its_owner(self) -> None: + route_source = """ + def route(req, websocket, request_context, row): + req.session.get("user") + websocket.session.get("user") + request_context.session.add(row) + """ + service_source = """ + def run(request_session, statement): + return request_session.exec(statement) + """ + + self.assertEqual( + _findings({ROUTE: route_source}, ROUTES_RULE), + {f"{ROUTE} calls session.add"}, + ) + self.assertEqual( + _findings({SERVICE: service_source}, SERVICES_RULE), + {f"{SERVICE} calls request_session.exec"}, + ) + + +class TestBaselineAndManifest(unittest.TestCase): + """run_check compares findings with the rule's baseline and checks the manifest.""" + + def setUp(self) -> None: + self._directory = tempfile.TemporaryDirectory() + self.addCleanup(self._directory.cleanup) + self.root = Path(self._directory.name) + + def _registry(self, baseline_count: int | None) -> str: + rules = [] + for rule_id in (FILES_RULE, ROUTES_RULE, SERVICES_RULE, CONNECTORS_RULE): + ratcheted = rule_id == ROUTES_RULE and baseline_count is not None + baseline = ( + f'[rule.baseline]\nfile = "{BASELINE_FILE}"\ncount = {baseline_count}\n' + if ratcheted + else "" + ) + rules.append( + textwrap.dedent( + f""" + [[rule]] + id = "{rule_id}" + category = "layers" + statement = "A statement." + rationale = "docs/guide.md" + scope = ["policyengine_api/**/*.py"] + tier = "mechanical" + status = "{"ratcheted" if ratcheted else "enforced"}" + autofix = false + [rule.enforcer] + kind = "quality-guard" + name = "v2-layers" + [[rule.ci]] + workflow = ".github/workflows/pr.yml" + job = "quality-guards" + invocation = "run_quality_guards.py" + """ + ) + + baseline + ) + header = ( + 'schema_version = 1\nrepo = "example"\n\n' + '[[category]]\nid = "layers"\ntitle = "Layers"\n' + ) + return header + "".join(rules) + + def _messages(self) -> list[str]: + return [str(violation) for violation in run_check(self.root)] + + def test_a_finding_the_baseline_lists_passes(self) -> None: + _write_files({ROUTE: "from sqlalchemy.exc import SQLAlchemyError\n"}, self.root) + _write_files( + { + "rules/registry.toml": self._registry(1), + BASELINE_FILE: f"{ROUTE} imports sqlalchemy.exc\n", + }, + self.root, + ) + + self.assertEqual(self._messages(), []) + + def test_a_finding_the_baseline_does_not_list_fails(self) -> None: + _write_files({ROUTE: "from sqlalchemy.exc import SQLAlchemyError\n"}, self.root) + _write_files({"rules/registry.toml": self._registry(None)}, self.root) + + self.assertEqual( + self._messages(), + [ + f"[{ROUTES_RULE}] v2-layers: {ROUTE} imports sqlalchemy.exc. Fix: " + "v2 route modules call one service method and map its typed " + "failures to HTTP; move session and SQL work into the service and " + "its connectors (docs/engineering/skills/v2-code-organization.md)." + ], + ) + + def test_each_occurrence_needs_its_own_baseline_entry(self) -> None: + _write_files( + { + ROUTE: ( + "from sqlalchemy.exc import SQLAlchemyError\n" + "def later():\n" + " from sqlalchemy.exc import IntegrityError\n" + ), + "rules/registry.toml": self._registry(1), + BASELINE_FILE: f"{ROUTE} imports sqlalchemy.exc\n", + }, + self.root, + ) + + self.assertEqual( + self._messages(), + [ + f"[{ROUTES_RULE}] v2-layers: {ROUTE} imports sqlalchemy.exc (2 " + "occurrences, the baseline tolerates 1). Fix: v2 route modules call " + "one service method and map its typed failures to HTTP; move session " + "and SQL work into the service and its connectors " + "(docs/engineering/skills/v2-code-organization.md)." + ], + ) + + def test_a_duplicated_baseline_entry_is_stale(self) -> None: + _write_files( + { + ROUTE: "from sqlalchemy.exc import SQLAlchemyError\n", + "rules/registry.toml": self._registry(2), + BASELINE_FILE: ( + f"{ROUTE} imports sqlalchemy.exc\n{ROUTE} imports sqlalchemy.exc\n" + ), + }, + self.root, + ) + + self.assertEqual( + self._messages(), + [ + f"[{ROUTES_RULE}] v2-layers: baseline entry no longer applies: " + f"{ROUTE} imports sqlalchemy.exc (1 occurrence, the baseline lists " + f"2). Fix: remove it from {BASELINE_FILE} and lower the rule's " + "baseline count." + ], + ) + + def test_the_manifest_gives_only_layers_of_the_file_s_directory(self) -> None: + client = "policyengine_api/services/v2/household_calculations/client.py" + support = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + _write_files( + { + client: "", + support: "", + "rules/registry.toml": self._registry(None), + "rules/v2-layer-manifest.toml": textwrap.dedent( + f""" + [files."{client}"] + layer = "package" + reason = "Not a package." + + [files."{support}"] + layer = "http-support" + reason = "" + """ + ), + }, + self.root, + ) + + self.assertEqual( + sorted(self._messages()), + sorted( + [ + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " + f"{client} the layer 'package', which a file under " + "policyengine_api/services/v2 can't have from the manifest. Fix: " + "use one of process-state, service, transformation, types, " + "validator.", + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " + f"{support} no reason. Fix: say why the file has that layer.", + ] + ), + ) + + def test_a_baseline_entry_that_no_longer_applies_fails(self) -> None: + _write_files({ROUTE: "def route():\n return 1\n"}, self.root) + _write_files( + { + "rules/registry.toml": self._registry(1), + BASELINE_FILE: f"{ROUTE} imports sqlalchemy.exc\n", + }, + self.root, + ) + + self.assertEqual( + self._messages(), + [ + f"[{ROUTES_RULE}] v2-layers: baseline entry no longer applies: " + f"{ROUTE} imports sqlalchemy.exc. Fix: remove it from " + f"{BASELINE_FILE} and lower the rule's baseline count." + ], + ) + + def test_an_unassigned_file_fails_and_names_the_fix(self) -> None: + unassigned = "policyengine_api/services/v2/policies/helpers.py" + _write_files( + {unassigned: "", "rules/registry.toml": self._registry(None)}, self.root + ) + + self.assertEqual( + self._messages(), + [ + f"[{FILES_RULE}] v2-layers: {unassigned} matches no v2 layer. Fix: " + "name it after its layer (services.py, validators.py, " + "transformations.py, types.py, database_session.py, " + "database_connectors/[_name].py, " + "routes.py, _routes.py, request_models.py or " + "response_models.py), or add it to rules/v2-layer-manifest.toml " + "with its layer and a reason." + ], + ) + + def test_manifest_entries_must_name_existing_unmatched_files_and_known_layers( + self, + ) -> None: + cache = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + _write_files( + { + cache: "", + ROUTE: "", + "rules/registry.toml": self._registry(None), + "rules/v2-layer-manifest.toml": textwrap.dedent( + f""" + [files."{cache}"] + layer = "cache" + reason = "Keeps serialized responses." + + [files."{ROUTE}"] + layer = "route" + reason = "Named routes.py already." + + [files."policyengine_api/services/v2/gone.py"] + layer = "service" + reason = "Removed." + """ + ), + }, + self.root, + ) + + self.assertEqual( + sorted(self._messages()), + sorted( + [ + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml gives " + f"{cache} the layer 'cache', which a file under " + "policyengine_api/fastapi_routes/v2 can't have from the " + "manifest. Fix: use one of http-model, http-support, route.", + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml lists " + f"{ROUTE}, which its name already assigns to a layer. Fix: " + "remove the entry.", + f"[{FILES_RULE}] v2-layers: rules/v2-layer-manifest.toml lists " + "policyengine_api/services/v2/gone.py, which does not exist. " + "Fix: remove the entry.", + ] + ), + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/rules/v2_layers.py b/scripts/rules/v2_layers.py new file mode 100644 index 000000000..7593a4817 --- /dev/null +++ b/scripts/rules/v2_layers.py @@ -0,0 +1,783 @@ +"""The v2-layers quality guard: every v2 route and service file has a layer. + +Every ``.py`` file under ``policyengine_api/services/v2`` and +``policyengine_api/fastapi_routes/v2`` gets a layer from its file name (see +``assign_layer``), or from ``rules/v2-layer-manifest.toml`` for the few files +with other names. A file with neither fails (rule +``api.v2.files-assigned-to-layers``), so a new file is never left unchecked. + +Then each file's imports and calls are checked against its layer's rule: + +- every file under ``fastapi_routes/v2`` (``api.v2.routes-no-sessions-or-sql``) + imports no SQLAlchemy, SQLModel, connector, ``database_session``, v1 ORM or + v2 database module, and calls no SQL builder, session method or session + factory; +- ``services.py`` modules (``api.v2.services-no-sql``) import nothing from + SQLAlchemy or SQLModel but the ``Session`` type and ``sqlalchemy.exc``, call + no SQL builder or data-access session method, and import no FastAPI or + Starlette; +- connector modules, and a ``database_connectors/__init__.py`` + (``api.v2.connectors-never-commit``), don't open, commit, roll back or close + a session, call no session factory, and import no v1 ORM, v2 database or + ``database_session`` module; +- validators, transformations, types, process-state modules and package + ``__init__`` files (``api.v2.pure-modules-no-session-calls``) 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. + +``database_session.py`` modules, directly in a resource package, own the +session's lifetime (``api.v2.session-modules-no-data-access``): they call no +data-access session method or SQL builder and import no connector. + +A method call counts as a session call when its receiver is named ``session`` +or ``_session`` (``session.add``, ``self._session.exec``), but not +when it is the ``.session`` of a request or websocket (Starlette's session +data), or named like an HTTP or client session (``http_session``, +``requests_session``), or only contains the word (``sessions_router``). +Routes and the pure layers may call no method on a session; services, +connectors and session modules may not call the methods listed below. A call +into SQLAlchemy or SQLModel counts, except to its exception classes; so does a +builder called on a model's ``__table__``. A session factory is any call into +``policyengine_api.data.orm`` or ``policyengine_api.data.v2.database``, +SQLAlchemy's ``Session``, ``sessionmaker`` or ``create_engine``, or an injected +``*session_factory`` (or its ``begin()``); services may open sessions, and +nothing else checked here may. Imports made only under ``TYPE_CHECKING`` +count. Not recognised: a session bound to another name, an alias of a model's +table, and a name re-exported through another module. + +A ratcheted rule's baseline file lists its tolerated findings, one line per +occurrence, as the guard prints them. More occurrences than the baseline lists +fail, and so do fewer, so the baseline only shrinks. + +The rules: docs/engineering/skills/v2-code-organization.md. Standard library +only. +""" + +from __future__ import annotations + +import ast +import tomllib +from collections import Counter +from dataclasses import dataclass +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 + +if TYPE_CHECKING: + from collections.abc import Callable, Iterable, Mapping + +REPO_ROOT = Path(__file__).resolve().parents[2] +CHECK_NAME = "v2-layers" +MANIFEST_PATH = "rules/v2-layer-manifest.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, + SERVICES_RULE, + CONNECTORS_RULE, + PURE_RULE, + SESSION_RULE, +) + +SERVICE_FILE_LAYERS = { + "services.py": "service", + "validators.py": "validator", + "transformations.py": "transformation", + "types.py": "types", + "database_session.py": "session", +} +CONNECTOR_OPERATIONS = ("creates", "reads", "updates", "deletes") +HTTP_MODEL_FILES = frozenset({"request_models.py", "response_models.py"}) +# Layers the manifest may give, by directory. Connectors, session modules and +# packages get their layer from their names only. +MANIFEST_LAYERS = { + SERVICES_DIRECTORY: frozenset( + {"process-state", "service", "transformation", "types", "validator"} + ), + ROUTES_DIRECTORY: frozenset({"http-model", "http-support", "route"}), +} +PURE_LAYERS = frozenset( + {"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") + +FIXES = { + FILES_RULE: ( + "name it after its layer (services.py, validators.py, transformations.py, " + "types.py, database_session.py, " + "database_connectors/[_name].py, routes.py, " + "_routes.py, request_models.py or response_models.py), or add it to " + f"{MANIFEST_PATH} with its layer and a reason" + ), + ROUTES_RULE: ( + "v2 route modules call one service method and map its typed failures to " + "HTTP; move session and SQL work into the service and its connectors " + f"({ORGANIZATION_DOC})" + ), + SERVICES_RULE: ( + "services sequence work: build and run SQL in a connector, pass it the " + f"session, and define HTTP models under fastapi_routes/v2 ({ORGANIZATION_DOC})" + ), + CONNECTORS_RULE: ( + "connectors use the session the service passes them; the service's " + "database_session container opens, commits, rolls back and closes it " + f"({ORGANIZATION_DOC})" + ), + PURE_RULE: ( + "validators, transformations and types work on values and loaded rows; " + "load them in a connector and pass them in from the service " + f"({ORGANIZATION_DOC})" + ), + SESSION_RULE: ( + "a database_session module only opens, commits, rolls back and closes " + f"sessions; read and write rows in a connector ({ORGANIZATION_DOC})" + ), +} + + +@dataclass(frozen=True) +class Finding: + """One import or call a layer's rule forbids, or a file with no layer.""" + + rule_id: str + path: str + detail: str + + def __str__(self) -> str: + """Format as the baseline lists it: `` ``.""" + 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 --------------------------------------------------------------- + + +def assign_layer(path: str, manifest: Mapping[str, str]) -> str | None: + """Return the layer of the file at ``path`` (repository-relative), or None.""" + if path in manifest: + return manifest[path] + return _layer_by_name(path) + + +def _layer_by_name(path: str) -> str | None: + if path.startswith(f"{SERVICES_DIRECTORY}/"): + return _service_layer(PurePosixPath(path).parts) + if path.startswith(f"{ROUTES_DIRECTORY}/"): + return _route_layer(PurePosixPath(path).name) + return None + + +def _service_layer(parts: tuple[str, ...]) -> str | None: + """Assign a file under services/v2 by name, given its path parts.""" + name = parts[-1] + resource_parts = parts[3:-1] + in_connector_package = bool(resource_parts) and ( + resource_parts[-1] == "database_connectors" + ) + if name == "__init__.py": + return "connector" if in_connector_package else "package" + if "database_connectors" not in resource_parts: + layer = SERVICE_FILE_LAYERS.get(name) if resource_parts else None + # A session module sits directly in its resource package, where the + # session-module rule looks for it. + return None if layer == "session" and len(resource_parts) > 1 else layer + stem = name.removesuffix(".py") + is_operation = any( + stem == operation or stem.startswith(f"{operation}_") + for operation in CONNECTOR_OPERATIONS + ) + return "connector" if in_connector_package and is_operation else None + + +def _route_layer(name: str) -> str | None: + """Assign a file under fastapi_routes/v2 by its name.""" + if name == "__init__.py": + return "package" + if name == "routes.py" or name.endswith("_routes.py"): + return "route" + return "http-model" if name in HTTP_MODEL_FILES 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 ----------------------------------------------- + + +def _discovered_files(root: Path) -> list[str]: + return [ + path + for path in tracked_python_files(root) + if path.startswith((f"{SERVICES_DIRECTORY}/", f"{ROUTES_DIRECTORY}/")) + ] + + +def scan( + root: Path, manifest: Mapping[str, str], paths: Iterable[str] | None = None +) -> list[Finding]: + """Return every finding for the v2 files under ``root``, one per occurrence.""" + findings = [] + for path in paths if paths is not None else _discovered_files(root): + layer = assign_layer(path, manifest) + if layer is None: + findings.append(Finding(FILES_RULE, path, "matches no v2 layer")) + continue + 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) + ) + return sorted(findings, key=lambda finding: (finding.rule_id, str(finding))) + + +def _manifest_violation(message: str, fix: str) -> Violation: + return Violation(FILES_RULE, CHECK_NAME, message, fix) + + +def _manifest_entry_violations(root: Path, path: str, entry: object) -> list[Violation]: + layer = entry.get("layer") if isinstance(entry, dict) else None + reason = entry.get("reason") if isinstance(entry, dict) else None + directory = next( + (folder for folder in MANIFEST_LAYERS if path.startswith(f"{folder}/")), None + ) + if directory is None: + return [ + _manifest_violation( + f"{MANIFEST_PATH} lists {path}, which is not under " + f"{SERVICES_DIRECTORY} or {ROUTES_DIRECTORY}", + "remove the entry", + ) + ] + violations = [] + if not (root / path).is_file(): + violations.append( + _manifest_violation( + f"{MANIFEST_PATH} lists {path}, which does not exist", + "remove the entry", + ) + ) + elif _layer_by_name(path) is not None: + violations.append( + _manifest_violation( + f"{MANIFEST_PATH} lists {path}, which its name already assigns to " + "a layer", + "remove the entry", + ) + ) + if layer not in MANIFEST_LAYERS[directory]: + violations.append( + _manifest_violation( + f"{MANIFEST_PATH} gives {path} the layer '{layer}', which a file " + f"under {directory} can't have from the manifest", + f"use one of {', '.join(sorted(MANIFEST_LAYERS[directory]))}", + ) + ) + if not isinstance(reason, str) or not reason.strip(): + violations.append( + _manifest_violation( + f"{MANIFEST_PATH} gives {path} no reason", + "say why the file has that layer", + ) + ) + return violations + + +def load_manifest(root: Path) -> tuple[dict[str, str], list[Violation]]: + """Read the manifest's file layers, and report entries that don't belong.""" + manifest_file = root / MANIFEST_PATH + if not manifest_file.is_file(): + return {}, [] + entries = tomllib.loads(manifest_file.read_text()).get("files", {}) + manifest = {} + violations = [] + for path, entry in sorted(entries.items()): + layer = entry.get("layer") if isinstance(entry, dict) else None + manifest[path] = str(layer) + violations.extend(_manifest_entry_violations(root, path, entry)) + return manifest, violations + + +def _baseline_entries(root: Path, baseline_file: str | None) -> Counter[str]: + if baseline_file is None or not (root / baseline_file).is_file(): + return Counter() + return Counter( + line.strip() + for line in (root / baseline_file).read_text().splitlines() + if line.strip() and not line.lstrip().startswith("#") + ) + + +def _occurrences(count: int) -> str: + return f"{count} occurrence" if count == 1 else f"{count} occurrences" + + +def _compare( + rule_id: str, + found: Counter[str], + tolerated: Counter[str], + baseline_file: str | None, +) -> list[Violation]: + """Report findings beyond the baseline, and baseline entries beyond the findings.""" + violations = [] + for entry in sorted(found): + if found[entry] <= tolerated[entry]: + continue + message = ( + entry + if not tolerated[entry] + else f"{entry} ({_occurrences(found[entry])}, the baseline tolerates " + f"{tolerated[entry]})" + ) + violations.append(Violation(rule_id, CHECK_NAME, message, FIXES[rule_id])) + for entry in sorted(tolerated): + if tolerated[entry] <= found[entry]: + continue + detail = ( + entry + if not found[entry] + else f"{entry} ({_occurrences(found[entry])}, the baseline lists " + f"{tolerated[entry]})" + ) + violations.append( + Violation( + rule_id, + CHECK_NAME, + f"baseline entry no longer applies: {detail}", + f"remove it from {baseline_file} and lower the rule's baseline count", + ) + ) + return violations + + +def run_check(root: Path = REPO_ROOT) -> list[Violation]: + """Scan the v2 files under ``root`` and compare with each rule's baseline.""" + registry, violations = load_registry(root) + if registry is None: + return violations + manifest, violations = load_manifest(root) + findings = scan(root, manifest) + for rule_id in RULES: + rule = registry.rule(rule_id) + baseline_file = rule.baseline.file if rule and rule.baseline else None + found = Counter( + str(finding) for finding in findings if finding.rule_id == rule_id + ) + violations += _compare( + rule_id, found, _baseline_entries(root, baseline_file), baseline_file + ) + return violations + + +def check() -> list[str]: + """Adapter for scripts/run_quality_guards.py.""" + return [str(violation) for violation in run_check(REPO_ROOT)] diff --git a/scripts/run_quality_guards.py b/scripts/run_quality_guards.py index 9ac66caf5..888152db9 100644 --- a/scripts/run_quality_guards.py +++ b/scripts/run_quality_guards.py @@ -11,7 +11,12 @@ sys.path.insert(0, str(REPO_ROOT)) from scripts.guards import migration_contracts # noqa: E402 -from scripts.rules import ci_scripts, file_length, locked_installs # noqa: E402 +from scripts.rules import ( # noqa: E402 + ci_scripts, + file_length, + locked_installs, + v2_layers, +) from scripts.rules import guard as rules_guard # noqa: E402 Guard = tuple[str, Callable[[], list[str]]] @@ -22,6 +27,7 @@ ("ci-scripts", ci_scripts.check), ("locked-installs", locked_installs.check), ("file-length", file_length.check), + ("v2-layers", v2_layers.check), )