diff --git a/AGENTS.md b/AGENTS.md index 31043e75f..6379fb19d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -91,11 +91,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..f0763f03b --- /dev/null +++ b/changelog.d/4190.added.md @@ -0,0 +1 @@ +Check that every API v2 route and service file is named after its 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, session modules do no data access, and validators, transformations and types make no session calls, in a new v2-layers quality guard. Eight v2 modules are renamed to fit (for example `fastapi_routes/v2/errors.py` is now `errors_support.py`). diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index 23a0b5f17..0e601790e 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -1,15 +1,59 @@ # 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 alone: + +- under `services/v2//`: `services.py` (service), `validators.py`, + `transformations.py`, `types.py`, `process_state.py` (process-wide state), + `database_session.py` (directly in the resource package) and + `database_connectors/[_name].py`; +- under `fastapi_routes/v2/`: `routes.py` and `_routes.py` (routes), + `_models.py` (request, response and document schemas) and + `_support.py` (HTTP code the route modules share); +- `__init__.py` anywhere. + +A file with another name fails the guard (rule +`api.v2.files-assigned-to-layers`): rename it. A file that can't be renamed +needs a `[[rule.exception]]` for that rule in `rules/registry.toml`, with its +`path`, a `reason` and an `expires` date, by which the file should be renamed. +An exception chooses no layer: the file gets the route checks under +`fastapi_routes/v2`, and the validator, transformation and types checks +(rule `api.v2.pure-modules-no-session-calls`) under `services/v2`. The guard +fails on an exception for a file that no longer exists or whose name already +gives it a layer. + +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 Resource-specific FastAPI code lives under `policyengine_api/fastapi_routes/v2/`: ```text -errors.py +errors_support.py +user_auth_support.py +user_resources_support.py policies/ request_models.py response_models.py @@ -20,27 +64,39 @@ user_policies/ routes.py metadata/ response_models.py - common.py - query_dependencies.py - response_cache.py + route_support.py + query_support.py + response_cache_support.py + search_file_support.py *_routes.py ``` -`common.py` also holds the route class that gives metadata responses their -caching headers. `query_dependencies.py` defines the typed FastAPI +`route_support.py` runs the metadata reads for the route modules, maps their +failures to HTTP, and holds the route class that gives metadata responses +their caching headers. `query_support.py` defines the typed FastAPI dependencies for the shared query models once, for every metadata route -module to import. `response_cache.py` keeps serialized whole-version +module to import. `response_cache_support.py` keeps serialized whole-version responses (bodies and ETags) for the process; it is HTTP-level, so it lives -with the routes. - -`errors.py` defines the strict error response and serialization function shared -by every API v2 resource. Application-wide exception handling distinguishes +with the routes. `search_file_support.py` serves a stored parameter search +file with its content encoding. + +`errors_support.py` defines the strict error response and serialization +function shared by every API v2 resource. `user_auth_support.py` turns +request credentials into an account principal, and +`user_resources_support.py` holds the HTTP conventions the user-owned +resource routes share. Application-wide exception handling distinguishes API v2 from non-v2 requests but must not select error behavior by matching 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 @@ -61,10 +117,9 @@ services/v2// deletes.py ``` -A resource may also add a module for process-wide state its service reuses -across requests, named for what it keeps, such as the metadata package's -`selected_catalogs.py` (each catalog version a request has selected, which -never changes once published). Such a module holds plain data, constructs no +A resource may also add a `process_state.py` module for process-wide state its +service reuses across requests, such as the metadata package's (each catalog +version a request has selected, which never changes once published). Such a module holds plain data, constructs no SQL and owns no session; the composition code creates one instance per process and injects it into the service. @@ -86,14 +141,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 +190,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 +210,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 06f5cb0aa..42e051213 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 | 4 | 1 | 11 | 79 | +| mechanical | 68 | 5 | 1 | 8 | 82 | | metric | 1 | 0 | 0 | 0 | 1 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 68 of 81 rules. +**Blocking:** 74 of 84 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) @@ -67,6 +64,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce - `api.dependencies.declared-and-used`: 16 remaining in `rules/baselines/deptry.txt` - `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 @@ -94,15 +92,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 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `api.v2.import-layers` | v2 route modules may import v2 services and v2 data modules, and v2 services may import v2 data modules, never the reverse, directly or through other modules. | mechanical | enforced | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | | `api.v2.pure-modules-no-database` | v2 validators, transformations and types never reach SQLAlchemy, SQLModel, a connector, a database_session module or the v2 database module, directly or through other modules, except through the v2 table models; imports under TYPE_CHECKING count. | mechanical | ratcheted | yes | import-linter `pyproject.toml` | `pr.yml:quality-guards` | yes | +| `api.v2.pure-modules-no-session-calls` | v2 validators, transformations, types, process-state modules, package __init__ files and files under services/v2 excepted from api.v2.files-assigned-to-layers import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded. | mechanical | enforced | yes | quality guard `v2-layers` | `pr.yml:quality-guards` | yes | | `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 | @@ -115,17 +116,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)") @@ -133,16 +138,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/policyengine_api/asgi_factory.py b/policyengine_api/asgi_factory.py index 415fd09bb..0d6adf0f0 100644 --- a/policyengine_api/asgi_factory.py +++ b/policyengine_api/asgi_factory.py @@ -21,7 +21,7 @@ from policyengine_api.fastapi_routes.dependencies import NativeRouteDependencies from policyengine_api.fastapi_routes.health import build_core_health_router from policyengine_api.fastapi_routes.metadata import build_metadata_router -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2AuthenticationRequiredError, V2AuthenticationUnavailableError, V2PermissionDeniedError, diff --git a/policyengine_api/fastapi_routes/dependencies.py b/policyengine_api/fastapi_routes/dependencies.py index 6e2927f83..25f8664cb 100644 --- a/policyengine_api/fastapi_routes/dependencies.py +++ b/policyengine_api/fastapi_routes/dependencies.py @@ -24,13 +24,15 @@ V2UserReportResourceService, V2UserSimulationResourceService, ) -from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( +from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( MetadataResponseCache, ) -from policyengine_api.fastapi_routes.v2.user_auth import authenticate_account_request +from policyengine_api.fastapi_routes.v2.user_auth_support import ( + authenticate_account_request, +) from policyengine_api.libs.user_auth import UNAUTHENTICATED_USER_HEADER from policyengine_api.readiness import is_ready -from policyengine_api.services.v2.metadata.selected_catalogs import ( +from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) diff --git a/policyengine_api/fastapi_routes/v2/countries/response_models.py b/policyengine_api/fastapi_routes/v2/countries/response_models.py index 3ea3c7ee8..4d2abe90f 100644 --- a/policyengine_api/fastapi_routes/v2/countries/response_models.py +++ b/policyengine_api/fastapi_routes/v2/countries/response_models.py @@ -6,7 +6,7 @@ from pydantic import BaseModel, ConfigDict -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse if TYPE_CHECKING: from policyengine_api.services.v2.countries.types import CountryRead diff --git a/policyengine_api/fastapi_routes/v2/countries/routes.py b/policyengine_api/fastapi_routes/v2/countries/routes.py index 279aaf54f..1f15c3f70 100644 --- a/policyengine_api/fastapi_routes/v2/countries/routes.py +++ b/policyengine_api/fastapi_routes/v2/countries/routes.py @@ -19,7 +19,10 @@ CountryListResponse, CountryListResult, ) -from policyengine_api.fastapi_routes.v2.errors import V2ErrorMapping, run_v2_operation +from policyengine_api.fastapi_routes.v2.errors_support import ( + V2ErrorMapping, + run_v2_operation, +) from policyengine_api.query_parameters import CountryListQuery if TYPE_CHECKING: diff --git a/policyengine_api/fastapi_routes/v2/errors.py b/policyengine_api/fastapi_routes/v2/errors_support.py similarity index 100% rename from policyengine_api/fastapi_routes/v2/errors.py rename to policyengine_api/fastapi_routes/v2/errors_support.py diff --git a/policyengine_api/fastapi_routes/v2/households/response_models.py b/policyengine_api/fastapi_routes/v2/households/response_models.py index 35c645c9c..f778c7caa 100644 --- a/policyengine_api/fastapi_routes/v2/households/response_models.py +++ b/policyengine_api/fastapi_routes/v2/households/response_models.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, ConfigDict -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse from policyengine_api.fastapi_routes.v2.households.document_models import ( HouseholdDocument, ) diff --git a/policyengine_api/fastapi_routes/v2/households/routes.py b/policyengine_api/fastapi_routes/v2/households/routes.py index 54e24dbf5..52b773e95 100644 --- a/policyengine_api/fastapi_routes/v2/households/routes.py +++ b/policyengine_api/fastapi_routes/v2/households/routes.py @@ -14,7 +14,7 @@ _default_v2_household_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2ErrorMapping, V2RequestTooLargeError, run_v2_operation, diff --git a/policyengine_api/fastapi_routes/v2/me/routes.py b/policyengine_api/fastapi_routes/v2/me/routes.py index 0919c938c..81c517fc7 100644 --- a/policyengine_api/fastapi_routes/v2/me/routes.py +++ b/policyengine_api/fastapi_routes/v2/me/routes.py @@ -20,7 +20,7 @@ _default_v2_account_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse from policyengine_api.fastapi_routes.v2.me.request_models import ( AccountPatchRequest, ) @@ -33,8 +33,10 @@ MeUpgradeResponse, MeUpgradeResult, ) -from policyengine_api.fastapi_routes.v2.user_auth import require_signed_in_principal -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_auth_support import ( + require_signed_in_principal, +) +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, enforce_user_write_request_size, user_resource_operation, diff --git a/policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py b/policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py index 968c802c4..5490ea28a 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py @@ -7,13 +7,7 @@ from fastapi import APIRouter from starlette.responses import JSONResponse, Response -from policyengine_api.fastapi_routes.v2.metadata.common import ( - METADATA_RESPONSES, - CacheableMetadataRoute, - read_resource, - read_whole_version_resource, -) -from policyengine_api.fastapi_routes.v2.metadata.query_dependencies import ( +from policyengine_api.fastapi_routes.v2.metadata.query_support import ( CatalogParameterQueryDependency, CatalogQueryDependency, ) @@ -23,6 +17,12 @@ MetadataModelCatalogResponse, MetadataResourceSuccessResponse, ) +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( + METADATA_RESPONSES, + CacheableMetadataRoute, + read_resource, + read_whole_version_resource, +) from policyengine_api.services.v2.metadata.types import ( MetadataCatalogVariablesResult, MetadataModelCatalogResult, diff --git a/policyengine_api/fastapi_routes/v2/metadata/geography_routes.py b/policyengine_api/fastapi_routes/v2/metadata/geography_routes.py index 448826c5d..bffe931cd 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/geography_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/geography_routes.py @@ -8,12 +8,7 @@ from fastapi import APIRouter from starlette.responses import JSONResponse -from policyengine_api.fastapi_routes.v2.metadata.common import ( - METADATA_RESPONSES, - CacheableMetadataRoute, - read_resource, -) -from policyengine_api.fastapi_routes.v2.metadata.query_dependencies import ( +from policyengine_api.fastapi_routes.v2.metadata.query_support import ( CatalogPageQueryDependency, CatalogQueryDependency, RegionCollectionQueryDependency, @@ -25,6 +20,11 @@ MetadataRegionDetailResponse, MetadataRegionPageResponse, ) +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( + METADATA_RESPONSES, + CacheableMetadataRoute, + read_resource, +) if TYPE_CHECKING: from policyengine_api.fastapi_routes.dependencies import NativeRouteDependencies diff --git a/policyengine_api/fastapi_routes/v2/metadata/model_routes.py b/policyengine_api/fastapi_routes/v2/metadata/model_routes.py index ae3127725..7fbbe530f 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/model_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/model_routes.py @@ -8,13 +8,7 @@ from fastapi import APIRouter from starlette.responses import JSONResponse -from policyengine_api.fastapi_routes.v2.metadata.common import ( - METADATA_RESPONSES, - PATH_COUNTRY_ERROR_RESPONSES, - CacheableMetadataRoute, - read_resource, -) -from policyengine_api.fastapi_routes.v2.metadata.query_dependencies import ( +from policyengine_api.fastapi_routes.v2.metadata.query_support import ( CatalogPageQueryDependency, CatalogQueryDependency, CatalogVersionQueryDependency, @@ -29,6 +23,12 @@ MetadataVariableDetailResponse, MetadataVariablePageResponse, ) +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( + METADATA_RESPONSES, + PATH_COUNTRY_ERROR_RESPONSES, + CacheableMetadataRoute, + read_resource, +) if TYPE_CHECKING: from policyengine_api.fastapi_routes.dependencies import NativeRouteDependencies diff --git a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py index 11471f41c..040ff8f3c 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py @@ -8,17 +8,7 @@ from fastapi import APIRouter, Request from starlette.responses import JSONResponse, Response -from policyengine_api.fastapi_routes.v2.metadata.common import ( - METADATA_RESPONSES, - PARAMETER_SEARCH_FILE_ERROR_RESPONSES, - PARAMETER_TREE_ERROR_RESPONSES, - CacheableMetadataRoute, - names_entity_tag, - read_resource, - run_metadata_read, - take_utc_instant_for_read, -) -from policyengine_api.fastapi_routes.v2.metadata.query_dependencies import ( +from policyengine_api.fastapi_routes.v2.metadata.query_support import ( CatalogQueryDependency, ParameterChildrenQueryDependency, ParameterCollectionQueryDependency, @@ -32,7 +22,17 @@ MetadataParameterValueDetailResponse, MetadataParameterValuePageResponse, ) -from policyengine_api.fastapi_routes.v2.metadata.search_file_response import ( +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( + METADATA_RESPONSES, + PARAMETER_SEARCH_FILE_ERROR_RESPONSES, + PARAMETER_TREE_ERROR_RESPONSES, + CacheableMetadataRoute, + names_entity_tag, + read_resource, + run_metadata_read, + take_utc_instant_for_read, +) +from policyengine_api.fastapi_routes.v2.metadata.search_file_support import ( SEARCH_FILE_RESPONSE_HEADERS, parameter_search_file_response, search_file_entity_tag, diff --git a/policyengine_api/fastapi_routes/v2/metadata/query_dependencies.py b/policyengine_api/fastapi_routes/v2/metadata/query_support.py similarity index 100% rename from policyengine_api/fastapi_routes/v2/metadata/query_dependencies.py rename to policyengine_api/fastapi_routes/v2/metadata/query_support.py diff --git a/policyengine_api/fastapi_routes/v2/metadata/response_cache.py b/policyengine_api/fastapi_routes/v2/metadata/response_cache_support.py similarity index 100% rename from policyengine_api/fastapi_routes/v2/metadata/response_cache.py rename to policyengine_api/fastapi_routes/v2/metadata/response_cache_support.py diff --git a/policyengine_api/fastapi_routes/v2/metadata/common.py b/policyengine_api/fastapi_routes/v2/metadata/route_support.py similarity index 99% rename from policyengine_api/fastapi_routes/v2/metadata/common.py rename to policyengine_api/fastapi_routes/v2/metadata/route_support.py index 185fbe4d8..d8e99930f 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/common.py +++ b/policyengine_api/fastapi_routes/v2/metadata/route_support.py @@ -18,11 +18,11 @@ UnsupportedPreviewCountryError, ) from policyengine_api.data.v2.settings import V2ConfigurationError -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2ErrorResponse, v2_error_response, ) -from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( +from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( BuiltResponse, KeptResponse, body_entity_tag, diff --git a/policyengine_api/fastapi_routes/v2/metadata/search_file_response.py b/policyengine_api/fastapi_routes/v2/metadata/search_file_support.py similarity index 97% rename from policyengine_api/fastapi_routes/v2/metadata/search_file_response.py rename to policyengine_api/fastapi_routes/v2/metadata/search_file_support.py index 3a2b5b1b0..be224e72d 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/search_file_response.py +++ b/policyengine_api/fastapi_routes/v2/metadata/search_file_support.py @@ -17,7 +17,7 @@ from starlette.responses import Response from policyengine_api.content_encoding import accepts_gzip -from policyengine_api.fastapi_routes.v2.metadata.common import ( +from policyengine_api.fastapi_routes.v2.metadata.route_support import ( CACHE_HEADER_DESCRIPTIONS, ) diff --git a/policyengine_api/fastapi_routes/v2/policies/response_models.py b/policyengine_api/fastapi_routes/v2/policies/response_models.py index 771aaa12d..a73c24ce8 100644 --- a/policyengine_api/fastapi_routes/v2/policies/response_models.py +++ b/policyengine_api/fastapi_routes/v2/policies/response_models.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, ConfigDict, Field, JsonValue -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse if TYPE_CHECKING: from policyengine_api.services.v2.policies.types import ( diff --git a/policyengine_api/fastapi_routes/v2/policies/routes.py b/policyengine_api/fastapi_routes/v2/policies/routes.py index e65e3e5bc..8b07feeaa 100644 --- a/policyengine_api/fastapi_routes/v2/policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/policies/routes.py @@ -18,7 +18,7 @@ _default_v2_policy_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2ErrorMapping, V2RequestTooLargeError, run_v2_operation, diff --git a/policyengine_api/fastapi_routes/v2/routes.py b/policyengine_api/fastapi_routes/v2/routes.py index d3b412252..2309feb46 100644 --- a/policyengine_api/fastapi_routes/v2/routes.py +++ b/policyengine_api/fastapi_routes/v2/routes.py @@ -10,7 +10,7 @@ from policyengine_api.fastapi_routes.v2.countries.routes import ( build_v2_country_router, ) -from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse +from policyengine_api.fastapi_routes.v2.errors_support import V2ErrorResponse from policyengine_api.fastapi_routes.v2.households.routes import ( build_v2_household_router, ) diff --git a/policyengine_api/fastapi_routes/v2/user_auth.py b/policyengine_api/fastapi_routes/v2/user_auth_support.py similarity index 98% rename from policyengine_api/fastapi_routes/v2/user_auth.py rename to policyengine_api/fastapi_routes/v2/user_auth_support.py index aeeb5e46f..617f43703 100644 --- a/policyengine_api/fastapi_routes/v2/user_auth.py +++ b/policyengine_api/fastapi_routes/v2/user_auth_support.py @@ -21,7 +21,7 @@ from __future__ import annotations -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2AuthenticationRequiredError, V2AuthenticationUnavailableError, V2PermissionDeniedError, diff --git a/policyengine_api/fastapi_routes/v2/user_households/response_models.py b/policyengine_api/fastapi_routes/v2/user_households/response_models.py index df4137499..eeb480fe3 100644 --- a/policyengine_api/fastapi_routes/v2/user_households/response_models.py +++ b/policyengine_api/fastapi_routes/v2/user_households/response_models.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, ConfigDict -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, ) from policyengine_api.query_parameters import CountryId diff --git a/policyengine_api/fastapi_routes/v2/user_households/routes.py b/policyengine_api/fastapi_routes/v2/user_households/routes.py index 4e86c9b86..43841b9b9 100644 --- a/policyengine_api/fastapi_routes/v2/user_households/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_households/routes.py @@ -13,7 +13,7 @@ _default_v2_user_household_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import v2_error_response +from policyengine_api.fastapi_routes.v2.errors_support import v2_error_response from policyengine_api.fastapi_routes.v2.user_households.request_models import ( UserHouseholdCreateRequest, UserHouseholdPatchRequest, @@ -26,7 +26,7 @@ UserHouseholdPageResponse, UserHouseholdPageResult, ) -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( enforce_user_write_request_size, user_resource_operation, ) diff --git a/policyengine_api/fastapi_routes/v2/user_policies/response_models.py b/policyengine_api/fastapi_routes/v2/user_policies/response_models.py index 7886c40c7..08912e1d7 100644 --- a/policyengine_api/fastapi_routes/v2/user_policies/response_models.py +++ b/policyengine_api/fastapi_routes/v2/user_policies/response_models.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, ConfigDict -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, ) from policyengine_api.query_parameters import CountryId diff --git a/policyengine_api/fastapi_routes/v2/user_policies/routes.py b/policyengine_api/fastapi_routes/v2/user_policies/routes.py index 6ff192fee..0f55c1fa4 100644 --- a/policyengine_api/fastapi_routes/v2/user_policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_policies/routes.py @@ -13,7 +13,7 @@ _default_v2_user_policy_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import v2_error_response +from policyengine_api.fastapi_routes.v2.errors_support import v2_error_response from policyengine_api.fastapi_routes.v2.user_policies.request_models import ( UserPolicyCreateRequest, UserPolicyPatchRequest, @@ -26,7 +26,7 @@ UserPolicyPageResponse, UserPolicyPageResult, ) -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( enforce_user_write_request_size, user_resource_operation, ) diff --git a/policyengine_api/fastapi_routes/v2/user_reports/routes.py b/policyengine_api/fastapi_routes/v2/user_reports/routes.py index 241f19882..44b7f607b 100644 --- a/policyengine_api/fastapi_routes/v2/user_reports/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_reports/routes.py @@ -17,7 +17,7 @@ _default_v2_user_report_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( V2ErrorResponse, v2_error_response, ) @@ -35,7 +35,7 @@ UserReportPageResponse, UserReportPageResult, ) -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, enforce_user_write_request_size, user_resource_operation, diff --git a/policyengine_api/fastapi_routes/v2/user_resources.py b/policyengine_api/fastapi_routes/v2/user_resources_support.py similarity index 99% rename from policyengine_api/fastapi_routes/v2/user_resources.py rename to policyengine_api/fastapi_routes/v2/user_resources_support.py index 581065a57..b71f76bb2 100644 --- a/policyengine_api/fastapi_routes/v2/user_resources.py +++ b/policyengine_api/fastapi_routes/v2/user_resources_support.py @@ -13,7 +13,7 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from policyengine_api.data.v2.settings import V2ConfigurationError -from policyengine_api.fastapi_routes.v2.errors import ( +from policyengine_api.fastapi_routes.v2.errors_support import ( BEARER_CHALLENGE, V2ErrorResponse, V2RequestTooLargeError, diff --git a/policyengine_api/fastapi_routes/v2/user_simulations/routes.py b/policyengine_api/fastapi_routes/v2/user_simulations/routes.py index 0cac726a4..fbb98c51d 100644 --- a/policyengine_api/fastapi_routes/v2/user_simulations/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_simulations/routes.py @@ -13,8 +13,8 @@ _default_v2_user_simulation_service_factory, ) from policyengine_api.fastapi_routes.query_parameters import query_dependency -from policyengine_api.fastapi_routes.v2.errors import v2_error_response -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.errors_support import v2_error_response +from policyengine_api.fastapi_routes.v2.user_resources_support import ( USER_RESOURCE_ERROR_RESPONSES, enforce_user_write_request_size, user_resource_operation, diff --git a/policyengine_api/services/v2/metadata/selected_catalogs.py b/policyengine_api/services/v2/metadata/process_state.py similarity index 100% rename from policyengine_api/services/v2/metadata/selected_catalogs.py rename to policyengine_api/services/v2/metadata/process_state.py diff --git a/policyengine_api/services/v2/metadata/services.py b/policyengine_api/services/v2/metadata/services.py index 8938f42c6..b8e32a624 100644 --- a/policyengine_api/services/v2/metadata/services.py +++ b/policyengine_api/services/v2/metadata/services.py @@ -107,7 +107,7 @@ from policyengine_api.services.v2.metadata.database_session import ( MetadataDatabaseSession, ) - from policyengine_api.services.v2.metadata.selected_catalogs import ( + from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) diff --git a/rules/baselines/v2-layers-routes.txt b/rules/baselines/v2-layers-routes.txt new file mode 100644 index 000000000..43fb5fa5f --- /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_support.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..a85a5145c --- /dev/null +++ b/rules/fixtures/api.v2.connectors-never-commit/bad_tree.toml @@ -0,0 +1,20 @@ +# Proof fixture for rule api.v2.connectors-never-commit. The guard must reject +# it: the connector opens its own session and commits it. + +# Each expected finding is ": ". +expected_findings = [ + "api.v2.connectors-never-commit: policyengine_api/services/v2/policies/database_connectors/creates.py calls get_v2_session_factory", + "api.v2.connectors-never-commit: policyengine_api/services/v2/policies/database_connectors/creates.py calls session.commit", + "api.v2.connectors-never-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..96af20577 --- /dev/null +++ b/rules/fixtures/api.v2.files-assigned-to-layers/bad_tree.toml @@ -0,0 +1,37 @@ +# Proof fixture for rule api.v2.files-assigned-to-layers. The guard must reject +# it: +# - simulation_client.py's name gives it no layer, and no [[rule.exception]] +# names it (the case of an open pull request that adds a v2 resource); +# - the two files listed in ``exceptions`` stand for files a +# [[rule.exception]] names. They get no layer, but the pure-module checks +# under services/v2 and the route checks under fastapi_routes/v2. +# scripts/rules/tests/test_v2_layers.py writes each file into a temporary +# directory and runs the v2-layers guard's scan on it. + +exceptions = [ + "policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py", + "policyengine_api/services/v2/household_calculations/legacy_client.py", +] +# Each expected finding is ": ". +expected_findings = [ + "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py calls session.add", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/household_calculations/legacy_client.py calls session.get", + "api.v2.files-assigned-to-layers: policyengine_api/services/v2/household_calculations/simulation_client.py matches no v2 layer", +] + +[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 +""" +"policyengine_api/services/v2/household_calculations/legacy_client.py" = """ +def load(session, household_id): + return session.get(household_id) +""" +"policyengine_api/fastapi_routes/v2/reports/legacy_helpers.py" = """ +def keep(session, row): + session.add(row) +""" 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..bed2c9564 --- /dev/null +++ b/rules/fixtures/api.v2.files-assigned-to-layers/good_tree.toml @@ -0,0 +1,33 @@ +# Proof fixture for rule api.v2.files-assigned-to-layers. The guard must accept +# it: every file is named after its layer, and the one excepted file passes +# the pure-module checks it gets under services/v2. + +exceptions = ["policyengine_api/services/v2/household_calculations/legacy_client.py"] +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/process_state.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/services/v2/household_calculations/legacy_client.py" = """ +def describe(household): + return household.label +""" +"policyengine_api/fastapi_routes/v2/__init__.py" = "" +"policyengine_api/fastapi_routes/v2/routes.py" = "" +"policyengine_api/fastapi_routes/v2/errors_support.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/households/document_models.py" = "" +"policyengine_api/fastapi_routes/v2/metadata/catalog_routes.py" = "" +"policyengine_api/fastapi_routes/v2/metadata/route_support.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..f4e4cfc60 --- /dev/null +++ b/rules/fixtures/api.v2.pure-modules-no-session-calls/bad_tree.toml @@ -0,0 +1,35 @@ +# 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. + +# Each expected finding is ": ". +expected_findings = [ + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/policies/__init__.py calls session.commit", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/policies/types.py calls session.exec", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/policies/types.py calls sqlmodel.select", + "api.v2.pure-modules-no-session-calls: policyengine_api/services/v2/policies/types.py imports sqlmodel.select", + "api.v2.pure-modules-no-session-calls: 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-import-no-database-access/bad_package.toml b/rules/fixtures/api.v2.routes-import-no-database-access/bad_package.toml index d77345733..e4d0b82ee 100644 --- a/rules/fixtures/api.v2.routes-import-no-database-access/bad_package.toml +++ b/rules/fixtures/api.v2.routes-import-no-database-access/bad_package.toml @@ -16,7 +16,7 @@ from fixture_api.services.v2.policies.database_connectors.reads import read_poli "fixture_api/fastapi_routes/v2/households/routes.py" = """ from fixture_api.services.v2.households.database_session import HouseholdDatabaseSession """ -"fixture_api/fastapi_routes/v2/metadata/common.py" = """ +"fixture_api/fastapi_routes/v2/metadata/route_support.py" = """ from fixture_api.data.v2.database import get_v2_session_factory """ "fixture_api/services/v2/policies/database_connectors/reads.py" = """ 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..fc78accf3 --- /dev/null +++ b/rules/fixtures/api.v2.routes-no-sessions-or-sql/bad_tree.toml @@ -0,0 +1,23 @@ +# 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. + +# Each expected finding is ": ". +expected_findings = [ + "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/policies/routes.py calls session.add", + "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/policies/routes.py calls sqlmodel.select", + "api.v2.routes-no-sessions-or-sql: policyengine_api/fastapi_routes/v2/policies/routes.py imports policyengine_api.services.v2.policies.database_connectors.reads", + "api.v2.routes-no-sessions-or-sql: 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..79cc14e4d --- /dev/null +++ b/rules/fixtures/api.v2.services-no-sql/bad_tree.toml @@ -0,0 +1,23 @@ +# 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. + +# Each expected finding is ": ". +expected_findings = [ + "api.v2.services-no-sql: policyengine_api/services/v2/policies/services.py calls _session.exec", + "api.v2.services-no-sql: policyengine_api/services/v2/policies/services.py calls sqlmodel.select", + "api.v2.services-no-sql: policyengine_api/services/v2/policies/services.py imports fastapi.responses", + "api.v2.services-no-sql: 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..55289073f --- /dev/null +++ b/rules/fixtures/api.v2.session-modules-no-data-access/bad_tree.toml @@ -0,0 +1,26 @@ +# 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. + +# Each expected finding is ": ". +expected_findings = [ + "api.v2.session-modules-no-data-access: policyengine_api/services/v2/policies/database_session.py calls session.add", + "api.v2.session-modules-no-data-access: policyengine_api/services/v2/policies/database_session.py calls session.execute", + "api.v2.session-modules-no-data-access: policyengine_api/services/v2/policies/database_session.py calls session.get", + "api.v2.session-modules-no-data-access: 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 f5b6c7926..dcd923794 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 gets a layer from its file name alone, so the layer rules check it. A file that can't be renamed needs a [[rule.exception]] for this rule in rules/registry.toml; it then gets the route checks under fastapi_routes/v2, or the validator, transformation and types checks under services/v2." +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, package __init__ files and files under services/v2 excepted from api.v2.files-assigned-to-layers import nothing from SQLAlchemy or SQLModel but the Session type, import no connector, database_session, v1 ORM or v2 database module, and call no session method, SQLAlchemy or SQLModel function or session factory; they work on values and rows a connector loaded." +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/*/process_state.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/scripts/rules/tests/test_v2_layers.py b/scripts/rules/tests/test_v2_layers.py new file mode 100644 index 000000000..ce55711ec --- /dev/null +++ b/scripts/rules/tests/test_v2_layers.py @@ -0,0 +1,815 @@ +"""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.load import load_registry +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 _all_findings(files: dict[str, str], exceptions: tuple[str, ...]) -> set[str]: + """Return every finding as ``: ``.""" + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + _write_files(files, root) + return {f"{finding.rule_id}: {finding}" for finding in scan(root, exceptions)} + + +def _assert_proof(test: unittest.TestCase, rule_id: str) -> None: + """Each tree gives exactly its expected findings, each under its rule. + + A tree's ``exceptions`` list the files a ``[[rule.exception]]`` for + api.v2.files-assigned-to-layers would name. + """ + for fixture in ("bad_tree.toml", "good_tree.toml"): + tree = tomllib.loads((FIXTURES / rule_id / fixture).read_text()) + exceptions = tuple(tree.get("exceptions", ())) + with test.subTest(fixture=fixture): + test.assertEqual( + _all_findings(tree["files"], exceptions), + 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" + ), + "policyengine_api/services/v2/metadata/process_state.py": "process-state", + "policyengine_api/fastapi_routes/v2/households/document_models.py": ( + "http-model" + ), + "policyengine_api/fastapi_routes/v2/reports/" + "household_response_models.py": "http-model", + "policyengine_api/fastapi_routes/v2/errors_support.py": "http-support", + "policyengine_api/fastapi_routes/v2/metadata/route_support.py": ( + "http-support" + ), + } + 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_excepted(self) -> None: + unnamed = "policyengine_api/fastapi_routes/v2/metadata/response_cache.py" + misnamed_connector = ( + "policyengine_api/services/v2/policies/database_connectors/helpers.py" + ) + support_outside_routes = "policyengine_api/services/v2/policies/x_support.py" + + self.assertIsNone(assign_layer(unnamed, ())) + self.assertIsNone(assign_layer(misnamed_connector, ())) + self.assertIsNone(assign_layer(support_outside_routes, ())) + self.assertEqual(assign_layer(unnamed, (unnamed,)), "excepted") + + 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_an_excepted_file_gets_its_directory_s_strictest_checks(self) -> None: + client = "policyengine_api/services/v2/household_calculations/client.py" + helper = "policyengine_api/fastapi_routes/v2/metadata/legacy_helper.py" + process_state = "policyengine_api/services/v2/metadata/process_state.py" + files = { + client: ( + "from policyengine_api.data.orm import get_v1_session_factory\n" + "def load(session):\n" + " get_v1_session_factory()\n" + " return session.exec(1)\n" + ), + helper: "def keep(session, row):\n session.add(row)\n", + process_state: "def load(session):\n return session.exec(1)\n", + } + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + _write_files(files, root) + found = { + (finding.rule_id, str(finding)) + for finding in scan(root, (client, helper)) + } + + self.assertEqual( + found, + { + (PURE_RULE, f"{client} calls get_v1_session_factory"), + (PURE_RULE, f"{client} calls session.exec"), + (PURE_RULE, f"{client} imports policyengine_api.data.orm"), + (PURE_RULE, f"{process_state} calls session.exec"), + (ROUTES_RULE, f"{helper} 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 TestBaselineAndExceptions(unittest.TestCase): + """run_check compares findings with the baseline and checks the exceptions.""" + + 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, exceptions: tuple[str, ...] = () + ) -> 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 "" + ) + excepted = "".join( + f'[[rule.exception]]\npath = "{path}"\nreason = "Generated."\n' + "expires = 2099-01-01\n" + for path in (exceptions if rule_id == FILES_RULE 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 + + excepted + ) + header = ( + 'schema_version = 1\nrepo = "example"\n\n' + '[[category]]\nid = "layers"\ntitle = "Layers"\n' + ) + return header + "".join(rules) + + def _messages(self) -> list[str]: + _, problems = load_registry(self.root) + self.assertEqual([str(problem) for problem in problems], []) + 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_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, process_state.py, " + "database_session.py or " + "database_connectors/[_name].py), " + "or, if it can't be renamed, add a [[rule.exception]] for " + "api.v2.files-assigned-to-layers to rules/registry.toml with its " + "path, a reason and an expires date." + ], + ) + + def test_an_unassigned_route_file_is_told_the_route_names(self) -> None: + unassigned = "policyengine_api/fastapi_routes/v2/metadata/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 (routes.py, _routes.py, " + "_models.py or _support.py), or, if it can't be " + "renamed, add a [[rule.exception]] for " + "api.v2.files-assigned-to-layers to rules/registry.toml with its " + "path, a reason and an expires date." + ], + ) + + def test_an_exception_gives_no_layer_but_the_strictest_checks(self) -> None: + client = "policyengine_api/services/v2/household_calculations/client.py" + _write_files( + { + client: "def load(session):\n return session.get(1)\n", + "rules/registry.toml": self._registry(None, (client,)), + }, + self.root, + ) + + self.assertEqual( + self._messages(), + [ + f"[{PURE_RULE}] v2-layers: {client} calls session.get. Fix: " + "validators, transformations and types work on values and loaded " + "rows; load them in a connector and pass them in from the service " + "(docs/engineering/skills/v2-code-organization.md)." + ], + ) + + def test_exceptions_must_name_one_existing_v2_file_without_a_layer(self) -> None: + named = ROUTE + missing = "policyengine_api/services/v2/policies/gone.py" + pattern = "policyengine_api/services/v2/*/client.py" + outside = "policyengine_api/libs/user_auth.py" + _write_files( + { + named: "", + outside: "", + "rules/registry.toml": self._registry( + None, (named, missing, pattern, outside) + ), + }, + self.root, + ) + prefix = f"[{FILES_RULE}] v2-layers: rules/registry.toml excepts" + one_file = ( + "which is not one file under policyengine_api/services/v2 or " + "policyengine_api/fastapi_routes/v2. Fix: name one file there, or " + "remove the exception." + ) + + self.assertEqual( + sorted(self._messages()), + sorted( + [ + f"{prefix} {named} from {FILES_RULE}, but its name already " + "gives it a layer. Fix: remove the exception.", + f"{prefix} {missing} from {FILES_RULE}, but it does not " + "exist. Fix: remove the exception.", + f"{prefix} {pattern} from {FILES_RULE}, {one_file}", + f"{prefix} {outside} from {FILES_RULE}, {one_file}", + ] + ), + ) + + +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..279c4a6cf --- /dev/null +++ b/scripts/rules/v2_layers.py @@ -0,0 +1,786 @@ +"""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 alone +(see ``assign_layer``). A file whose name gives none fails (rule +``api.v2.files-assigned-to-layers``), so a new file is never left unchecked. +A file that can't be renamed needs a ``[[rule.exception]]`` for that rule in +rules/registry.toml, ending on an ``expires`` date. The exception gives it no +layer: it gets the route checks under ``fastapi_routes/v2``, and the +pure-module (validator, transformation and types) checks under +``services/v2``. + +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 +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, Collection, Iterable, Mapping + + from scripts.rules.model import Rule + +REPO_ROOT = Path(__file__).resolve().parents[2] +CHECK_NAME = "v2-layers" +REGISTRY_FILE = "rules/registry.toml" +SERVICES_DIRECTORY = "policyengine_api/services/v2" +ROUTES_DIRECTORY = "policyengine_api/fastapi_routes/v2" +ORGANIZATION_DOC = "docs/engineering/skills/v2-code-organization.md" + +FILES_RULE = "api.v2.files-assigned-to-layers" +ROUTES_RULE = "api.v2.routes-no-sessions-or-sql" +SERVICES_RULE = "api.v2.services-no-sql" +CONNECTORS_RULE = "api.v2.connectors-never-commit" +PURE_RULE = "api.v2.pure-modules-no-session-calls" +SESSION_RULE = "api.v2.session-modules-no-data-access" +RULES = ( + FILES_RULE, + ROUTES_RULE, + SERVICES_RULE, + CONNECTORS_RULE, + PURE_RULE, + SESSION_RULE, +) + +SERVICE_FILE_LAYERS = { + "services.py": "service", + "validators.py": "validator", + "transformations.py": "transformation", + "types.py": "types", + "process_state.py": "process-state", + "database_session.py": "session", +} +CONNECTOR_OPERATIONS = ("creates", "reads", "updates", "deletes") +# The names that give a layer, by directory, for the files rule's fix message. +LAYER_NAMES = { + SERVICES_DIRECTORY: ( + "services.py, validators.py, transformations.py, types.py, " + "process_state.py, database_session.py or " + "database_connectors/[_name].py" + ), + ROUTES_DIRECTORY: ( + "routes.py, _routes.py, _models.py or _support.py" + ), +} +# What a [[rule.exception]] for the files rule makes a file: no layer of its +# own; under services/v2 it gets the pure-module checks (under +# fastapi_routes/v2 every file gets the route checks). +EXCEPTED = "excepted" +PURE_LAYERS = frozenset( + {EXCEPTED, "package", "process-state", "transformation", "types", "validator"} +) +GLOB_CHARACTERS = frozenset("*?[") + +SQL_PACKAGES = ("sqlalchemy", "sqlmodel") +DATABASE_PACKAGES = ("policyengine_api.data.orm", "policyengine_api.data.v2.database") +SESSION_TYPES = ("sqlmodel.Session", "sqlalchemy.orm.Session") +SQL_EXCEPTIONS = "sqlalchemy.exc" +# What a service may import from SQLAlchemy or SQLModel: the session type, for +# annotations, and the exception classes, to raise typed failures. +SERVICE_SQL_IMPORTS = (*SESSION_TYPES, SQL_EXCEPTIONS) +SQL_BUILDERS = frozenset( + { + "delete", + "except_", + "exists", + "insert", + "intersect", + "literal_column", + "select", + "text", + "union", + "union_all", + "update", + "values", + } +) +SQL_SESSION_FACTORIES = frozenset({"Session", "create_engine", "sessionmaker"}) +TRANSACTION_METHODS = frozenset( + {"begin", "begin_nested", "close", "commit", "invalidate", "reset", "rollback"} +) +DATA_ACCESS_METHODS = frozenset( + { + "add", + "add_all", + "bulk_insert_mappings", + "bulk_save_objects", + "bulk_update_mappings", + "connection", + "delete", + "exec", + "execute", + "expunge", + "expunge_all", + "flush", + "get", + "get_one", + "merge", + "query", + "refresh", + "scalar", + "scalars", + } +) +# A database_session container's context managers, which open a session. +CONTAINER_METHODS = frozenset({"read", "transaction"}) +# Receiver name prefixes, before "_session", of web rather than database sessions. +WEB_SESSION_WORDS = ("client", "http", "requests") +# Owners whose ``.session`` is Starlette's session data, not a database session. +REQUEST_OWNERS = frozenset({"req", "request", "websocket", "ws"}) +HTTP_PACKAGES = ("fastapi", "starlette") + +FIXES = { + FILES_RULE: ( + "name it after its layer ({names}), or, if it can't be renamed, add a " + f"[[rule.exception]] for {FILES_RULE} to {REGISTRY_FILE} with its path, " + "a reason and an expires date" + ), + 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, exceptions: Collection[str] = ()) -> str | None: + """Return the layer of the file at ``path`` (repository-relative), or None. + + A path in ``exceptions`` is ``excepted``: under services/v2 it gets the + pure-module checks, and under fastapi_routes/v2 the route checks. + """ + if path in exceptions: + return EXCEPTED + 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" + if name.endswith("_models.py"): + return "http-model" + return "http-support" if name.endswith("_support.py") else None + + +# --- imports -------------------------------------------------------------- + + +def _from_source(node: ast.ImportFrom, path: str) -> str: + """Return the absolute module a ``from ... import`` reads from.""" + if not node.level: + return node.module or "" + module_name, is_package = _module_name(path) + base = module_name.split(".") + if not is_package: + base = base[:-1] + if node.level > 1: + base = base[: -(node.level - 1)] + return ".".join([*base, *([node.module] if node.module else [])]) + + +@dataclass(frozen=True) +class ImportStatement: + """One imported module, or one ``from`` import with the names it binds.""" + + module: str + names: tuple[str, ...] + + +def _imports(tree: ast.Module, path: str) -> list[ImportStatement]: + """Return every import in a module; relative imports are resolved. + + Imports under TYPE_CHECKING and inside functions count like any other. + """ + statements = [] + for node in ast.walk(tree): + if isinstance(node, ast.Import): + statements.extend(ImportStatement(alias.name, ()) for alias in node.names) + elif isinstance(node, ast.ImportFrom): + source = _from_source(node, path) + names = tuple(f"{source}.{alias.name}" for alias in node.names) + statements.append(ImportStatement(source, names)) + return statements + + +def _imported_names(tree: ast.Module, path: str) -> dict[str, str]: + """Map each name a module binds by importing to the dotted name it refers to.""" + names = {} + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + bound = alias.asname or alias.name.split(".")[0] + names[bound] = alias.name if alias.asname else bound + elif isinstance(node, ast.ImportFrom): + source = _from_source(node, path) + for alias in node.names: + names[alias.asname or alias.name] = f"{source}.{alias.name}" + return names + + +def _is_session_or_connector_module(module: str) -> bool: + parts = module.split(".") + return _within(module, "policyengine_api.services.v2") and ( + "database_connectors" in parts or "database_session" in parts + ) + + +def _is_route_forbidden_import(module: str) -> bool: + return ( + _within_any(module, SQL_PACKAGES) + or _within_any(module, DATABASE_PACKAGES) + or _is_session_or_connector_module(module) + ) + + +def _is_connector_module(module: str) -> bool: + return _within(module, "policyengine_api.services.v2") and ( + "database_connectors" in module.split(".") + ) + + +def _is_connector_forbidden_import(module: str) -> bool: + parts = module.split(".") + return _within_any(module, DATABASE_PACKAGES) or ( + _within(module, "policyengine_api.services.v2") and "database_session" in parts + ) + + +def _is_http_import(module: str) -> bool: + return _within_any(module, HTTP_PACKAGES) + + +def _is_service_forbidden_sql_name(name: str) -> bool: + return _within_any(name, SQL_PACKAGES) and not _within_any( + name, SERVICE_SQL_IMPORTS + ) + + +def _is_pure_forbidden_import(module: str) -> bool: + return _within_any(module, DATABASE_PACKAGES) or _is_session_or_connector_module( + module + ) + + +def _is_pure_forbidden_sql_name(name: str) -> bool: + return _within_any(name, SQL_PACKAGES) and not _within_any(name, SESSION_TYPES) + + +def _never(_module: str) -> bool: + return False + + +# --- calls ---------------------------------------------------------------- + + +def _dotted_name(node: ast.expr, imported: Mapping[str, str]) -> str | None: + """Resolve a called name such as ``select`` or ``sa.text`` through the imports.""" + attributes = [] + while isinstance(node, ast.Attribute): + attributes.append(node.attr) + node = node.value + if not isinstance(node, ast.Name) or node.id not in imported: + return None + return ".".join([imported[node.id], *reversed(attributes)]) + + +def _receiver_name(node: ast.expr) -> str: + if isinstance(node, ast.Attribute): + return node.attr + if isinstance(node, ast.Name): + return node.id + return "" + + +def _is_request_owner(node: ast.expr) -> bool: + name = _receiver_name(node).lower().lstrip("_") + return name in REQUEST_OWNERS or name.endswith("_request") + + +def _is_session_receiver(node: ast.expr) -> bool: + """Whether a method's receiver is a database session or its container.""" + if ( + isinstance(node, ast.Attribute) + and node.attr == "session" + and _is_request_owner(node.value) + ): + return False + name = _receiver_name(node).lower().lstrip("_") + if name == "session": + return True + prefix = name.removesuffix("_session") + return prefix != name and not any(word in prefix for word in WEB_SESSION_WORDS) + + +def _is_session_factory_name(name: str) -> bool: + return name.lower().lstrip("_").endswith("session_factory") + + +def _injected_factory_detail(call: ast.Call) -> str | None: + """Return ``calls `` when a call opens a session from an injected factory. + + ``session_factory()``, ``self._session_factory()`` and + ``session_factory.begin()`` count. + """ + func = call.func + if _is_session_factory_name(_receiver_name(func)): + return f"calls {_receiver_name(func)}" + if ( + isinstance(func, ast.Attribute) + and func.attr == "begin" + and _is_session_factory_name(_receiver_name(func.value)) + ): + return f"calls {_receiver_name(func.value)}.begin" + return None + + +def _calls_on_a_table(call: ast.Call) -> bool: + """Whether a call is a SQL builder on a table, as ``Model.__table__.insert()``.""" + func = call.func + return ( + isinstance(func, ast.Attribute) + and func.attr in SQL_BUILDERS - {"values"} + and isinstance(func.value, ast.Attribute) + and func.value.attr == "__table__" + ) + + +# --- policies ------------------------------------------------------------- + + +@dataclass(frozen=True) +class LayerPolicy: + """What one layer's rule forbids: imports, session methods and call kinds. + + ``session_methods`` of None forbids every method on a session. + ``checks_sql_calls`` forbids every call into SQLAlchemy or SQLModel but + its exception classes; ``checks_builders`` only its SQL builders. Either + also forbids builders called on a model's ``__table__``. + ``checks_factories`` forbids opening a session from a session factory. + """ + + rule_id: str + forbidden_module: Callable[[str], bool] + session_methods: frozenset[str] | None + checks_sql_calls: bool + checks_factories: bool + forbidden_name: Callable[[str], bool] = _never + checks_builders: bool = False + + +ROUTE_POLICY = LayerPolicy( + ROUTES_RULE, + _is_route_forbidden_import, + None, + checks_sql_calls=True, + checks_factories=True, +) +PURE_POLICY = LayerPolicy( + PURE_RULE, + _is_pure_forbidden_import, + None, + checks_sql_calls=True, + checks_factories=True, + forbidden_name=_is_pure_forbidden_sql_name, +) +LAYER_POLICIES = { + "service": LayerPolicy( + SERVICES_RULE, + _is_http_import, + DATA_ACCESS_METHODS, + checks_sql_calls=True, + checks_factories=False, + forbidden_name=_is_service_forbidden_sql_name, + ), + "connector": LayerPolicy( + CONNECTORS_RULE, + _is_connector_forbidden_import, + TRANSACTION_METHODS | CONTAINER_METHODS, + checks_sql_calls=False, + checks_factories=True, + ), + "session": LayerPolicy( + SESSION_RULE, + _is_connector_module, + DATA_ACCESS_METHODS, + checks_sql_calls=False, + checks_factories=False, + checks_builders=True, + ), + **dict.fromkeys(PURE_LAYERS, PURE_POLICY), +} + + +def _policy(path: str, layer: str) -> LayerPolicy | None: + if path.startswith(f"{ROUTES_DIRECTORY}/"): + return ROUTE_POLICY + return LAYER_POLICIES.get(layer) + + +def _import_details(statement: ImportStatement, policy: LayerPolicy) -> list[str]: + """Return ``imports `` for each module or name the policy forbids.""" + if policy.forbidden_module(statement.module): + return [f"imports {statement.module}"] + module_level = next( + (name for name in statement.names if policy.forbidden_module(name)), None + ) + if module_level is not None: + return [f"imports {module_level}"] + if not statement.names: + forbidden = policy.forbidden_name(statement.module) + return [f"imports {statement.module}"] if forbidden else [] + return [ + f"imports {name}" for name in statement.names if policy.forbidden_name(name) + ] + + +def _named_call_details(dotted: str, policy: LayerPolicy) -> list[str]: + """Return the findings for one call to an imported name. + + For example ``calls sqlmodel.select`` or ``calls get_v2_session_factory``. + """ + last = dotted.rsplit(".", 1)[-1] + from_sql = _within_any(dotted, SQL_PACKAGES) + opens_a_session = _within_any(dotted, DATABASE_PACKAGES) or ( + from_sql and last in SQL_SESSION_FACTORIES + ) + if opens_a_session: + return [f"calls {last}"] if policy.checks_factories else [] + if policy.checks_sql_calls and from_sql and not _within(dotted, SQL_EXCEPTIONS): + return [f"calls {dotted}"] + if policy.checks_builders and from_sql and last in SQL_BUILDERS: + return [f"calls {dotted}"] + return [] + + +def _call_details( + call: ast.Call, imported: Mapping[str, str], policy: LayerPolicy +) -> list[str]: + details = [] + dotted = _dotted_name(call.func, imported) + if dotted is not None: + details.extend(_named_call_details(dotted, policy)) + if (policy.checks_sql_calls or policy.checks_builders) and _calls_on_a_table(call): + details.append(f"calls {ast.unparse(call.func)}") + factory = _injected_factory_detail(call) if policy.checks_factories else None + if factory is not None: + details.append(factory) + func = call.func + forbidden_method = isinstance(func, ast.Attribute) and ( + policy.session_methods is None or func.attr in policy.session_methods + ) + if forbidden_method and _is_session_receiver(func.value): + details.append(f"calls {_receiver_name(func.value)}.{func.attr}") + return details + + +def _file_findings(path: str, source: str, layer: str) -> list[tuple[str, str]]: + """Return (rule id, detail) for each forbidden import and call in one file.""" + policy = _policy(path, layer) + if policy is None: + return [] + tree = ast.parse(source) + details = [ + detail + for statement in _imports(tree, path) + for detail in _import_details(statement, policy) + ] + imported = _imported_names(tree, path) + for call in (node for node in ast.walk(tree) if isinstance(node, ast.Call)): + details.extend(_call_details(call, imported, policy)) + return [(policy.rule_id, detail) for detail in details] + + +# --- scanning and comparing ----------------------------------------------- + + +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, exceptions: Collection[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, exceptions) + 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 _exception_violations(root: Path, path: str) -> list[Violation]: + """Report a files-rule exception that names no unnamed v2 file.""" + + def violation(problem: str, fix: str) -> Violation: + return Violation( + FILES_RULE, + CHECK_NAME, + f"{REGISTRY_FILE} excepts {path} from {FILES_RULE}, {problem}", + fix, + ) + + in_v2 = path.startswith((f"{SERVICES_DIRECTORY}/", f"{ROUTES_DIRECTORY}/")) + if not in_v2 or GLOB_CHARACTERS & set(path) or not path.endswith(".py"): + return [ + violation( + f"which is not one file under {SERVICES_DIRECTORY} or " + f"{ROUTES_DIRECTORY}", + "name one file there, or remove the exception", + ) + ] + if not (root / path).is_file(): + return [violation("but it does not exist", "remove the exception")] + if _layer_by_name(path) is not None: + return [ + violation("but its name already gives it a layer", "remove the exception") + ] + return [] + + +def file_exceptions(root: Path, rule: Rule | None) -> tuple[list[str], list[Violation]]: + """Return the files the files rule excepts, and its exceptions that don't belong.""" + if rule is None: + return [], [] + excepted = [] + violations = [] + for exception in rule.exceptions: + problems = _exception_violations(root, exception.path) + violations += problems + if not problems: + excepted.append(exception.path) + return excepted, 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 _fix(rule_id: str, entry: str) -> str: + """Return the rule's fix; the files rule's names the layers of its directory.""" + if rule_id != FILES_RULE: + return FIXES[rule_id] + directory = ( + SERVICES_DIRECTORY if entry.startswith(SERVICES_DIRECTORY) else ROUTES_DIRECTORY + ) + return FIXES[FILES_RULE].format(names=LAYER_NAMES[directory]) + + +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, _fix(rule_id, entry))) + 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 + excepted, violations = file_exceptions(root, registry.rule(FILES_RULE)) + findings = scan(root, excepted) + 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 46a79bab3..410d31303 100644 --- a/scripts/run_quality_guards.py +++ b/scripts/run_quality_guards.py @@ -16,6 +16,7 @@ declared_dependencies, file_length, locked_installs, + v2_layers, ) from scripts.rules import guard as rules_guard # noqa: E402 @@ -28,6 +29,7 @@ ("locked-installs", locked_installs.check), ("file-length", file_length.check), ("declared-dependencies", declared_dependencies.check), + ("v2-layers", v2_layers.check), ) diff --git a/tests/fixtures/v2_metadata_service_helpers.py b/tests/fixtures/v2_metadata_service_helpers.py index fdc6dbd5c..f847d3017 100644 --- a/tests/fixtures/v2_metadata_service_helpers.py +++ b/tests/fixtures/v2_metadata_service_helpers.py @@ -25,7 +25,7 @@ from tests.fixtures.v2_catalog import POLICYENGINE_VERSION, normalized_catalog if TYPE_CHECKING: - from policyengine_api.services.v2.metadata.selected_catalogs import ( + from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) diff --git a/tests/integration/test_v2_metadata_routes.py b/tests/integration/test_v2_metadata_routes.py index 82f42645d..ec1af10b2 100644 --- a/tests/integration/test_v2_metadata_routes.py +++ b/tests/integration/test_v2_metadata_routes.py @@ -616,10 +616,10 @@ def test_postgres_production_metadata_reader_keeps_catalogs_and_responses( # The production combination: autocommit read sessions, the process's # selected catalogs and its whole-version response cache. from policyengine_api.data.v2.database import build_v2_read_session_factory - from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( + from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( MetadataResponseCache, ) - from policyengine_api.services.v2.metadata.selected_catalogs import ( + from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) @@ -715,11 +715,11 @@ def test_postgres_current_values_and_their_lifetime_use_one_instant( monkeypatch: pytest.MonkeyPatch, case: tuple[datetime, datetime, list[float], str], ) -> None: - from policyengine_api.fastapi_routes.v2.metadata import common + from policyengine_api.fastapi_routes.v2.metadata import route_support read_instant, header_instant, expected_values, expected_cache_control = case instants = iter([read_instant, header_instant]) - monkeypatch.setattr(common, "utc_now", lambda: next(instants)) + monkeypatch.setattr(route_support, "utc_now", lambda: next(instants)) response = _client(published_engine).get( "/v2/parameter-values", diff --git a/tests/unit/v2/test_metadata_response_cache.py b/tests/unit/v2/test_metadata_response_cache.py index a0a788c1c..fafa93b1f 100644 --- a/tests/unit/v2/test_metadata_response_cache.py +++ b/tests/unit/v2/test_metadata_response_cache.py @@ -8,7 +8,7 @@ import pytest from starlette.responses import JSONResponse -from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( +from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( BuiltResponse, KeptResponse, MetadataResponseCache, diff --git a/tests/unit/v2/test_metadata_route_caching.py b/tests/unit/v2/test_metadata_route_caching.py index 89719991e..c4c8e77d0 100644 --- a/tests/unit/v2/test_metadata_route_caching.py +++ b/tests/unit/v2/test_metadata_route_caching.py @@ -10,7 +10,7 @@ MetadataCatalogUnavailableError, ) from policyengine_api.fastapi_routes import dependencies as route_dependencies -from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( +from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( MetadataResponseCache, ) from policyengine_api.services.v2.metadata.validators import ( @@ -190,9 +190,9 @@ def test_current_parameter_values_are_reused_only_until_the_next_utc_day( query: str, expected_cache_control: str, ) -> None: - from policyengine_api.fastapi_routes.v2.metadata import common + from policyengine_api.fastapi_routes.v2.metadata import route_support - monkeypatch.setattr(common, "utc_now", lambda: now) + monkeypatch.setattr(route_support, "utc_now", lambda: now) results, _ids = resource_results() response = metadata_route_client(lambda: ResourceReader(results)).get( @@ -206,17 +206,20 @@ def test_current_parameter_values_are_reused_only_until_the_next_utc_day( def test_a_kept_response_is_served_without_hashing_its_body_again( monkeypatch: pytest.MonkeyPatch, ) -> None: - from policyengine_api.fastapi_routes.v2.metadata import common, response_cache + from policyengine_api.fastapi_routes.v2.metadata import ( + response_cache_support, + route_support, + ) hashed_bodies: list[int] = [] - original_entity_tag = response_cache.body_entity_tag + original_entity_tag = response_cache_support.body_entity_tag def counting_entity_tag(body: bytes) -> str: hashed_bodies.append(len(body)) return original_entity_tag(body) - monkeypatch.setattr(response_cache, "body_entity_tag", counting_entity_tag) - monkeypatch.setattr(common, "body_entity_tag", counting_entity_tag) + monkeypatch.setattr(response_cache_support, "body_entity_tag", counting_entity_tag) + monkeypatch.setattr(route_support, "body_entity_tag", counting_entity_tag) results, _ids = resource_results() client = metadata_route_client( lambda: ResourceReader(results), response_cache=MetadataResponseCache() @@ -259,11 +262,11 @@ def test_current_values_read_just_before_midnight_expire_at_that_midnight( # The read and the response headers must use the same day: values read for # 31 December may not be reused into 1 January, even when the headers are # written after midnight. - from policyengine_api.fastapi_routes.v2.metadata import common + from policyengine_api.fastapi_routes.v2.metadata import route_support read_instant = datetime(2026, 12, 31, 23, 59, 59, tzinfo=UTC) instants = iter([read_instant, datetime(2027, 1, 1, 0, 0, 1, tzinfo=UTC)]) - monkeypatch.setattr(common, "utc_now", lambda: next(instants)) + monkeypatch.setattr(route_support, "utc_now", lambda: next(instants)) results, _ids = resource_results() reader = ResourceReader(results) diff --git a/tests/unit/v2/test_metadata_routes.py b/tests/unit/v2/test_metadata_routes.py index 6d92bf348..fbab7a0dc 100644 --- a/tests/unit/v2/test_metadata_routes.py +++ b/tests/unit/v2/test_metadata_routes.py @@ -14,7 +14,7 @@ ) from policyengine_api.data.v2.settings import V2ConfigurationError from policyengine_api.fastapi_routes import dependencies as route_dependencies -from policyengine_api.services.v2.metadata.selected_catalogs import ( +from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) from policyengine_api.services.v2.metadata.types import ( diff --git a/tests/unit/v2/test_metadata_service.py b/tests/unit/v2/test_metadata_service.py index 33e699264..2fa5f0d93 100644 --- a/tests/unit/v2/test_metadata_service.py +++ b/tests/unit/v2/test_metadata_service.py @@ -32,7 +32,7 @@ from policyengine_api.services.v2.metadata.database_session import ( MetadataDatabaseSession, ) -from policyengine_api.services.v2.metadata.selected_catalogs import ( +from policyengine_api.services.v2.metadata.process_state import ( SelectedCatalogStore, ) from policyengine_api.services.v2.metadata.services import V2MetadataService diff --git a/tests/unit/v2/test_metadata_service_catalog_records.py b/tests/unit/v2/test_metadata_service_catalog_records.py index da9183e3e..0d7bf614f 100644 --- a/tests/unit/v2/test_metadata_service_catalog_records.py +++ b/tests/unit/v2/test_metadata_service_catalog_records.py @@ -416,7 +416,7 @@ def test_both_variables_routes_serve_unit_and_definition_period( expected_catalog_format: int | None, expected_fields: dict[str, tuple[str | None, str | None]], ) -> None: - from policyengine_api.fastapi_routes.v2.metadata.response_cache import ( + from policyengine_api.fastapi_routes.v2.metadata.response_cache_support import ( MetadataResponseCache, ) from tests.fixtures.v2_metadata_routes import metadata_route_client diff --git a/tests/unit/v2/test_parameter_value_listing_route.py b/tests/unit/v2/test_parameter_value_listing_route.py index 73b98d472..2e297119d 100644 --- a/tests/unit/v2/test_parameter_value_listing_route.py +++ b/tests/unit/v2/test_parameter_value_listing_route.py @@ -17,10 +17,10 @@ def test_listing_forwards_every_query_parameter( monkeypatch: pytest.MonkeyPatch, ) -> None: - from policyengine_api.fastapi_routes.v2.metadata import common + from policyengine_api.fastapi_routes.v2.metadata import route_support request_instant = datetime(2026, 6, 1, 12, 0, tzinfo=UTC) - monkeypatch.setattr(common, "utc_now", lambda: request_instant) + monkeypatch.setattr(route_support, "utc_now", lambda: request_instant) results, ids = resource_results() reader = ResourceReader(results) diff --git a/tests/unit/v2/test_user_resource_failures.py b/tests/unit/v2/test_user_resource_failures.py index 420fc26b2..b08df7ea6 100644 --- a/tests/unit/v2/test_user_resource_failures.py +++ b/tests/unit/v2/test_user_resource_failures.py @@ -10,7 +10,7 @@ from sqlalchemy.exc import IntegrityError, OperationalError from policyengine_api.data.v2.settings import V2ConfigurationError -from policyengine_api.fastapi_routes.v2 import user_resources +from policyengine_api.fastapi_routes.v2 import user_resources_support from policyengine_api.services.v2.associations.validators import ( AssociationConflictError, AssociationLinkNotFoundError, @@ -60,10 +60,10 @@ def failure_records( # Alembic tests run logging.config.fileConfig, which disables loggers that # already exist, so re-enable this one and record on it directly. handler = _RecordingHandler() - monkeypatch.setattr(user_resources.logger, "disabled", False) - user_resources.logger.addHandler(handler) + monkeypatch.setattr(user_resources_support.logger, "disabled", False) + user_resources_support.logger.addHandler(handler) yield handler.records - user_resources.logger.removeHandler(handler) + user_resources_support.logger.removeHandler(handler) def _failing(error: Exception): @@ -183,7 +183,7 @@ class _FailureCase(NamedTuple): def test_each_failure_maps_to_its_typed_response( failure_records: list[logging.LogRecord], case: _FailureCase ) -> None: - response = user_resources.user_resource_operation(_failing(case.error)) + response = user_resources_support.user_resource_operation(_failing(case.error)) assert response.status_code == case.status_code body = json.loads(response.body) @@ -195,4 +195,4 @@ def test_each_failure_maps_to_its_typed_response( def test_a_successful_operation_returns_its_result() -> None: - assert user_resources.user_resource_operation(lambda: "result") == "result" + assert user_resources_support.user_resource_operation(lambda: "result") == "result" diff --git a/tests/unit/v2/test_user_route_auth.py b/tests/unit/v2/test_user_route_auth.py index 37c4c4c35..d6df2dcc2 100644 --- a/tests/unit/v2/test_user_route_auth.py +++ b/tests/unit/v2/test_user_route_auth.py @@ -245,7 +245,7 @@ def test_default_dependencies_publish_the_bearer_security_scheme() -> None: def _recorded_failure_logs(operation) -> tuple[object, list]: import logging - from policyengine_api.fastapi_routes.v2 import user_resources + from policyengine_api.fastapi_routes.v2 import user_resources_support class RecordingHandler(logging.Handler): def __init__(self) -> None: @@ -258,14 +258,14 @@ def emit(self, record: logging.LogRecord) -> None: # Alembic tests run logging.config.fileConfig, which disables loggers that # already exist, so re-enable this one and record on it directly. handler = RecordingHandler() - was_disabled = user_resources.logger.disabled - user_resources.logger.disabled = False - user_resources.logger.addHandler(handler) + was_disabled = user_resources_support.logger.disabled + user_resources_support.logger.disabled = False + user_resources_support.logger.addHandler(handler) try: - response = user_resources.user_resource_operation(operation) + response = user_resources_support.user_resource_operation(operation) finally: - user_resources.logger.removeHandler(handler) - user_resources.logger.disabled = was_disabled + user_resources_support.logger.removeHandler(handler) + user_resources_support.logger.disabled = was_disabled return response, handler.records diff --git a/tests/unit/v2/test_user_write_limits.py b/tests/unit/v2/test_user_write_limits.py index da691a477..85789a9b9 100644 --- a/tests/unit/v2/test_user_write_limits.py +++ b/tests/unit/v2/test_user_write_limits.py @@ -9,7 +9,7 @@ import pytest from pydantic import ValidationError -from policyengine_api.fastapi_routes.v2.user_resources import ( +from policyengine_api.fastapi_routes.v2.user_resources_support import ( MAXIMUM_USER_WRITE_REQUEST_BYTES, ) from policyengine_api.services.v2.associations.types import (