diff --git a/AGENTS.md b/AGENTS.md index f6c17dc22..9fbc04cc4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -104,7 +104,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### Data source isolation -- Checked: `api.catalog.extractor-import-isolation`, `api.metadata.read-modules-isolated`, `api.reports.v1-persistence-only`, `api.stage12.comparison-tables-isolated`, `api.stage12.retirement-owned`, `api.v2.single-orm-layer` +- Checked: `api.catalog.extractor-import-isolation`, `api.catalog.selection-validation-no-database`, `api.metadata.read-modules-isolated`, `api.reports.v1-persistence-only`, `api.stage12.comparison-tables-isolated`, `api.stage12.retirement-owned`, `api.v2.single-orm-layer` ### Migration and schema diff --git a/changelog.d/4176.changed.md b/changelog.d/4176.changed.md new file mode 100644 index 000000000..807b5b944 --- /dev/null +++ b/changelog.d/4176.changed.md @@ -0,0 +1 @@ +Split the v2 catalog version selection into `catalog_selection_validation`, which loads no database code, and `catalog_selection_reads`, which holds the one catalog query, and stop the `policyengine_api.data.v2.catalog` package importing its extraction module when it loads. Importing a v2 validators, transformations or types module no longer loads SQLAlchemy unless that module imports the table models itself. diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index b32fe2af1..a02ce5dfe 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -173,6 +173,13 @@ route -> service -> database session + database connectors -> SQLModel tables -> types ``` +Catalog version validation, its error types and `SelectedCatalog` live in +`policyengine_api/data/v2/catalog/catalog_selection_validation.py`, which +loads no database code, so validators, transformations, types and routes may +import it (rule `api.catalog.selection-validation-no-database`). The catalog +query itself, `select_catalog`, is in `catalog_selection_reads.py`; the +policies and metadata read connectors call it. + Database connectors may consume service-layer input types when inserting or updating rows. They must not import route modules. Validators and transformations may consume types and already-loaded database model instances, diff --git a/docs/generated/rules.md b/docs/generated/rules.md index bf8654928..91f413a01 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 | 60 | 0 | 1 | 12 | 73 | +| mechanical | 61 | 0 | 1 | 12 | 74 | | metric | 1 | 0 | 0 | 0 | 1 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 61 of 75 rules. +**Blocking:** 62 of 76 rules. ## Gaps @@ -200,6 +200,7 @@ flowchart LR | Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof | | --- | --- | --- | --- | --- | --- | --- | --- | | `api.catalog.extractor-import-isolation` | The catalog extractor never imports PolicyEngine core, country packages or v1 metadata directly. | mechanical | enforced | yes | pytest `tests/unit/v2/test_catalog_extraction.py` | `pr.yml:test` | **unproven** | +| `api.catalog.selection-validation-no-database` | Importing catalog_selection_validation (catalog version validation, its error types and SelectedCatalog) loads no SQLAlchemy, SQLModel, v2 table model or v2 database module; the catalog query lives in catalog_selection_reads. | mechanical | enforced | yes | pytest `tests/unit/v2/test_catalog_selection_modules.py` | `pr.yml:test` | yes | | `api.metadata.read-modules-isolated` | v2 metadata read modules never import PolicyEngine packages or v1 metadata sources. | mechanical | enforced | yes | pytest `tests/unit/v2/test_metadata_service.py` | `pr.yml:test` | **unproven** | | `api.reports.v1-persistence-only` | The existing report route and service import only v1 persistence. | mechanical | enforced | yes | pytest `tests/unit/v2/test_report_v1_runtime_selection.py` | `pr.yml:test` | **unproven** | | `api.stage12.comparison-tables-isolated` | Serving routes and services never read the Stage 12 comparison tables, and production tables never depend on them. | mechanical | enforced | yes | pytest `tests/unit/v2/test_stage12_comparison_isolation.py` | `pr.yml:test` | **unproven** | @@ -211,6 +212,8 @@ flowchart LR r_api_catalog_extractor_import_isolation("api.catalog.extractor-import-isolation (enforced)") e_pytest_tests_unit_v2_test_catalog_extraction_py["pytest tests/unit/v2/test_catalog_extraction.py"] j_CI_pr_yml_test["CI pr.yml:test"] + r_api_catalog_selection_validation_no_database("api.catalog.selection-validation-no-database (enforced)") + e_pytest_tests_unit_v2_test_catalog_selection_modules_py["pytest tests/unit/v2/test_catalog_selection_modules.py"] r_api_metadata_read_modules_isolated("api.metadata.read-modules-isolated (enforced)") e_pytest_tests_unit_v2_test_metadata_service_py["pytest tests/unit/v2/test_metadata_service.py"] r_api_reports_v1_persistence_only("api.reports.v1-persistence-only (enforced)") @@ -222,6 +225,8 @@ flowchart LR e_pytest_tests_unit_v2_test_model_persistence_py["pytest tests/unit/v2/test_model_persistence.py"] r_api_catalog_extractor_import_isolation --> e_pytest_tests_unit_v2_test_catalog_extraction_py e_pytest_tests_unit_v2_test_catalog_extraction_py --> j_CI_pr_yml_test + r_api_catalog_selection_validation_no_database --> e_pytest_tests_unit_v2_test_catalog_selection_modules_py + e_pytest_tests_unit_v2_test_catalog_selection_modules_py --> j_CI_pr_yml_test r_api_metadata_read_modules_isolated --> e_pytest_tests_unit_v2_test_metadata_service_py e_pytest_tests_unit_v2_test_metadata_service_py --> j_CI_pr_yml_test r_api_reports_v1_persistence_only --> e_pytest_tests_unit_v2_test_report_v1_runtime_selection_py diff --git a/policyengine_api/data/v2/catalog/__init__.py b/policyengine_api/data/v2/catalog/__init__.py index a0d453d12..6bc693567 100644 --- a/policyengine_api/data/v2/catalog/__init__.py +++ b/policyengine_api/data/v2/catalog/__init__.py @@ -1,15 +1,7 @@ -"""PolicyEngine.py-derived v2 reference catalog extraction and publication.""" +"""PolicyEngine.py-derived v2 reference catalog extraction and publication. -from policyengine_api.data.v2.catalog.extraction import ( - CatalogExtractionError, - extract_catalog, - extract_installed_catalog, -) -from policyengine_api.data.v2.catalog.records import NormalizedCatalog - -__all__ = [ - "CatalogExtractionError", - "NormalizedCatalog", - "extract_catalog", - "extract_installed_catalog", -] +This package imports nothing when it loads, so importing one of its modules +(such as ``catalog_selection_validation``) loads only what that module +imports. Import names from their modules, for example +``policyengine_api.data.v2.catalog.extraction.extract_catalog``. +""" diff --git a/policyengine_api/data/v2/catalog/catalog_selection_reads.py b/policyengine_api/data/v2/catalog/catalog_selection_reads.py new file mode 100644 index 000000000..f8c62ef53 --- /dev/null +++ b/policyengine_api/data/v2/catalog/catalog_selection_reads.py @@ -0,0 +1,76 @@ +"""The database read that selects one v2 catalog by PolicyEngine.py version. + +Version validation, the error types and ``SelectedCatalog`` are in +``catalog_selection_validation``, which loads no database code when imported. +""" + +from __future__ import annotations + +from sqlalchemy.exc import SQLAlchemyError +from sqlmodel import Session, select + +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( + SUPPORTED_V2_COUNTRY_IDS, + MetadataCatalogNotPublishedError, + MetadataCatalogUnavailableError, + MetadataCatalogVersionNotFoundError, + SelectedCatalog, + UnsupportedPreviewCountryError, + validate_policyengine_version, +) +from policyengine_api.data.v2.models import ( + TaxBenefitModel, + TaxBenefitModelVersion, +) + + +def select_catalog( + session: Session, + *, + country_id: str, + running_policyengine_version: str, + policyengine_version: str | None = None, +) -> SelectedCatalog: + """Select one country catalog using the requested or running package version.""" + if country_id not in SUPPORTED_V2_COUNTRY_IDS: + raise UnsupportedPreviewCountryError(country_id) + explicit_version = policyengine_version is not None + selected_version = ( + validate_policyengine_version(policyengine_version) + if explicit_version + else running_policyengine_version + ) + try: + row = session.exec( + select(TaxBenefitModel, TaxBenefitModelVersion) + .join( + TaxBenefitModelVersion, + TaxBenefitModelVersion.model_id == TaxBenefitModel.id, + ) + .where( + TaxBenefitModel.name == f"policyengine-{country_id}", + TaxBenefitModelVersion.version == selected_version, + ) + ).one_or_none() + except SQLAlchemyError as error: + msg = "the v2 metadata catalog cannot be queried" + raise MetadataCatalogUnavailableError(msg) from error + + if row is None: + if explicit_version: + msg = ( + f"PolicyEngine.py {selected_version} is not published for {country_id}" + ) + raise MetadataCatalogVersionNotFoundError(msg) + msg = ( + f"the running PolicyEngine.py {selected_version} catalog " + f"is absent for {country_id}" + ) + raise MetadataCatalogNotPublishedError(msg) + model, model_version = row + return SelectedCatalog( + country_id=country_id, + policyengine_version=selected_version, + model=model, + model_version=model_version, + ) diff --git a/policyengine_api/data/v2/catalog/catalog_selection.py b/policyengine_api/data/v2/catalog/catalog_selection_validation.py similarity index 52% rename from policyengine_api/data/v2/catalog/catalog_selection.py rename to policyengine_api/data/v2/catalog/catalog_selection_validation.py index 1df25b0dd..0c94fb7ba 100644 --- a/policyengine_api/data/v2/catalog/catalog_selection.py +++ b/policyengine_api/data/v2/catalog/catalog_selection_validation.py @@ -1,19 +1,26 @@ -"""PolicyEngine.py catalog version selection for v2 resource reads.""" +"""Version validation, error types and the result type for v2 catalog selection. + +Importing this module loads no SQLAlchemy, SQLModel, table model or database +module (rule ``api.catalog.selection-validation-no-database``), so v2 +validators, transformations, types and routes can use it. The one database +read, ``select_catalog``, is in ``catalog_selection_reads``. +""" from __future__ import annotations from dataclasses import dataclass +from typing import TYPE_CHECKING from packaging.version import InvalidVersion, Version -from sqlalchemy.exc import SQLAlchemyError -from sqlmodel import Session, select -from policyengine_api.data.v2.models import ( - TaxBenefitModel, - TaxBenefitModelVersion, -) from policyengine_api.supported_countries import SUPPORTED_COUNTRY_IDS +if TYPE_CHECKING: + from policyengine_api.data.v2.models import ( + TaxBenefitModel, + TaxBenefitModelVersion, + ) + SUPPORTED_V2_COUNTRY_IDS = frozenset(SUPPORTED_COUNTRY_IDS) MAXIMUM_POLICYENGINE_VERSION_LENGTH = 128 @@ -72,55 +79,3 @@ def validate_policyengine_version(value: str) -> str: msg = "policyengine_version must be a canonical non-placeholder version" raise InvalidPolicyEngineVersionError(msg) return value - - -def select_catalog( - session: Session, - *, - country_id: str, - running_policyengine_version: str, - policyengine_version: str | None = None, -) -> SelectedCatalog: - """Select one country catalog using the requested or running package version.""" - if country_id not in SUPPORTED_V2_COUNTRY_IDS: - raise UnsupportedPreviewCountryError(country_id) - explicit_version = policyengine_version is not None - selected_version = ( - validate_policyengine_version(policyengine_version) - if explicit_version - else running_policyengine_version - ) - try: - row = session.exec( - select(TaxBenefitModel, TaxBenefitModelVersion) - .join( - TaxBenefitModelVersion, - TaxBenefitModelVersion.model_id == TaxBenefitModel.id, - ) - .where( - TaxBenefitModel.name == f"policyengine-{country_id}", - TaxBenefitModelVersion.version == selected_version, - ) - ).one_or_none() - except SQLAlchemyError as error: - msg = "the v2 metadata catalog cannot be queried" - raise MetadataCatalogUnavailableError(msg) from error - - if row is None: - if explicit_version: - msg = ( - f"PolicyEngine.py {selected_version} is not published for {country_id}" - ) - raise MetadataCatalogVersionNotFoundError(msg) - msg = ( - f"the running PolicyEngine.py {selected_version} catalog " - f"is absent for {country_id}" - ) - raise MetadataCatalogNotPublishedError(msg) - model, model_version = row - return SelectedCatalog( - country_id=country_id, - policyengine_version=selected_version, - model=model, - model_version=model_version, - ) diff --git a/policyengine_api/fastapi_routes/v2/metadata/common.py b/policyengine_api/fastapi_routes/v2/metadata/common.py index 17143afaf..185fbe4d8 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/common.py +++ b/policyengine_api/fastapi_routes/v2/metadata/common.py @@ -11,7 +11,7 @@ from starlette.responses import Response from starlette.status import HTTP_200_OK -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( InvalidPolicyEngineVersionError, MetadataCatalogUnavailableError, MetadataCatalogVersionNotFoundError, diff --git a/policyengine_api/fastapi_routes/v2/policies/routes.py b/policyengine_api/fastapi_routes/v2/policies/routes.py index 88bcc0444..e65e3e5bc 100644 --- a/policyengine_api/fastapi_routes/v2/policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/policies/routes.py @@ -9,7 +9,7 @@ from sqlalchemy.exc import SQLAlchemyError from starlette.responses import JSONResponse -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogUnavailableError, MetadataCatalogVersionNotFoundError, ) diff --git a/policyengine_api/query_parameters.py b/policyengine_api/query_parameters.py index 73d11cb8b..694b06b4b 100644 --- a/policyengine_api/query_parameters.py +++ b/policyengine_api/query_parameters.py @@ -26,7 +26,7 @@ ) from policyengine_api.constants import LOCAL_AREA_BREAKDOWN_REGIONS -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( validate_policyengine_version, ) from policyengine_api.services.economic_impact_request import LocalAreaBreakdown diff --git a/policyengine_api/routes/household_routes.py b/policyengine_api/routes/household_routes.py index 1d214dbb6..32f21fd60 100644 --- a/policyengine_api/routes/household_routes.py +++ b/policyengine_api/routes/household_routes.py @@ -15,7 +15,9 @@ from policyengine_api.constants import COUNTRY_PACKAGE_VERSIONS, POLICYENGINE_VERSION from policyengine_api.data.v1_models import Household -from policyengine_api.data.v2.catalog.catalog_selection import SUPPORTED_V2_COUNTRY_IDS +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( + SUPPORTED_V2_COUNTRY_IDS, +) from policyengine_api.extensions import cache from policyengine_api.gcp_logging import logger from policyengine_api.migration_flags import ( diff --git a/policyengine_api/routes/policy_routes.py b/policyengine_api/routes/policy_routes.py index 5b67aa52b..189eb06bf 100644 --- a/policyengine_api/routes/policy_routes.py +++ b/policyengine_api/routes/policy_routes.py @@ -7,7 +7,7 @@ from werkzeug.exceptions import BadRequest, NotFound from policyengine_api.data.v1_models import Policy -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( SUPPORTED_V2_COUNTRY_IDS, ) from policyengine_api.gcp_logging import logger diff --git a/policyengine_api/services/policy_mirroring.py b/policyengine_api/services/policy_mirroring.py index 30363a30b..b46299a6f 100644 --- a/policyengine_api/services/policy_mirroring.py +++ b/policyengine_api/services/policy_mirroring.py @@ -10,7 +10,7 @@ from sqlalchemy.exc import SQLAlchemyError -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogNotPublishedError, MetadataCatalogUnavailableError, MetadataCatalogVersionNotFoundError, diff --git a/policyengine_api/services/v2/metadata/database_connectors/reads.py b/policyengine_api/services/v2/metadata/database_connectors/reads.py index 27f5db17a..4afa826d0 100644 --- a/policyengine_api/services/v2/metadata/database_connectors/reads.py +++ b/policyengine_api/services/v2/metadata/database_connectors/reads.py @@ -6,10 +6,10 @@ from sqlalchemy.exc import SQLAlchemyError -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_reads import select_catalog +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogUnavailableError, SelectedCatalog, - select_catalog, ) if TYPE_CHECKING: @@ -25,7 +25,7 @@ def read_metadata_catalog( ) -> SelectedCatalog: """Return the country's catalog for policyengine_version, else the running version. - Raise the catalog_selection errors for an unsupported country, an + Raise the catalog_selection_validation errors for an unsupported country, an unpublished version or an unreachable database. """ return select_catalog( diff --git a/policyengine_api/services/v2/metadata/selected_catalogs.py b/policyengine_api/services/v2/metadata/selected_catalogs.py index 832705432..60d507dad 100644 --- a/policyengine_api/services/v2/metadata/selected_catalogs.py +++ b/policyengine_api/services/v2/metadata/selected_catalogs.py @@ -6,7 +6,9 @@ from typing import TYPE_CHECKING if TYPE_CHECKING: - from policyengine_api.data.v2.catalog.catalog_selection import SelectedCatalog + from policyengine_api.data.v2.catalog.catalog_selection_validation import ( + SelectedCatalog, + ) CatalogSelectionKey = tuple[str, str | None] """A country ID and the requested PolicyEngine.py version. diff --git a/policyengine_api/services/v2/metadata/services.py b/policyengine_api/services/v2/metadata/services.py index 489d4e2da..8938f42c6 100644 --- a/policyengine_api/services/v2/metadata/services.py +++ b/policyengine_api/services/v2/metadata/services.py @@ -4,7 +4,7 @@ from typing import TYPE_CHECKING -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( SelectedCatalog, validate_policyengine_version, ) diff --git a/policyengine_api/services/v2/metadata/transformations.py b/policyengine_api/services/v2/metadata/transformations.py index 4566097a8..f2586c653 100644 --- a/policyengine_api/services/v2/metadata/transformations.py +++ b/policyengine_api/services/v2/metadata/transformations.py @@ -37,7 +37,9 @@ ) if TYPE_CHECKING: - from policyengine_api.data.v2.catalog.catalog_selection import SelectedCatalog + from policyengine_api.data.v2.catalog.catalog_selection_validation import ( + SelectedCatalog, + ) from policyengine_api.data.v2.models import ( Dataset, Parameter, diff --git a/policyengine_api/services/v2/metadata/validators.py b/policyengine_api/services/v2/metadata/validators.py index d74d7f5c0..ce99cf343 100644 --- a/policyengine_api/services/v2/metadata/validators.py +++ b/policyengine_api/services/v2/metadata/validators.py @@ -4,12 +4,14 @@ from typing import TYPE_CHECKING, TypeVar -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogUnavailableError, ) if TYPE_CHECKING: - from policyengine_api.data.v2.catalog.catalog_selection import SelectedCatalog + from policyengine_api.data.v2.catalog.catalog_selection_validation import ( + SelectedCatalog, + ) from policyengine_api.data.v2.models import Dataset, Region diff --git a/policyengine_api/services/v2/policies/database_connectors/reads.py b/policyengine_api/services/v2/policies/database_connectors/reads.py index bd1685d01..1eba1685b 100644 --- a/policyengine_api/services/v2/policies/database_connectors/reads.py +++ b/policyengine_api/services/v2/policies/database_connectors/reads.py @@ -7,10 +7,7 @@ from sqlmodel import Session, col, select from policyengine_api.constants import POLICYENGINE_VERSION -from policyengine_api.data.v2.catalog.catalog_selection import ( - SelectedCatalog, - select_catalog, -) +from policyengine_api.data.v2.catalog.catalog_selection_reads import select_catalog from policyengine_api.data.v2.models import ( LegacyPolicyMapping, Parameter, @@ -22,6 +19,10 @@ if TYPE_CHECKING: from uuid import UUID + from policyengine_api.data.v2.catalog.catalog_selection_validation import ( + SelectedCatalog, + ) + # Parameter names bound in one statement; far below PostgreSQL's 65,535 limit. PARAMETER_NAME_BATCH_SIZE = 1_000 diff --git a/policyengine_api/services/v2/policies/services.py b/policyengine_api/services/v2/policies/services.py index a77cea0be..6829d826b 100644 --- a/policyengine_api/services/v2/policies/services.py +++ b/policyengine_api/services/v2/policies/services.py @@ -5,7 +5,7 @@ from typing import TYPE_CHECKING from policyengine_api.constants import POLICYENGINE_VERSION -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogNotPublishedError, ) from policyengine_api.services.v2.policies.database_connectors.creates import ( diff --git a/policyengine_api/services/v2/policies/transformations.py b/policyengine_api/services/v2/policies/transformations.py index f41569862..799fac982 100644 --- a/policyengine_api/services/v2/policies/transformations.py +++ b/policyengine_api/services/v2/policies/transformations.py @@ -36,7 +36,9 @@ from collections.abc import Container, Mapping from uuid import UUID - from policyengine_api.data.v2.catalog.catalog_selection import SelectedCatalog + from policyengine_api.data.v2.catalog.catalog_selection_validation import ( + SelectedCatalog, + ) from policyengine_api.data.v2.models import ( Parameter, ParameterValue, diff --git a/policyengine_api/services/v2/policies/validators.py b/policyengine_api/services/v2/policies/validators.py index 67b03e6ab..0f8b1d61a 100644 --- a/policyengine_api/services/v2/policies/validators.py +++ b/policyengine_api/services/v2/policies/validators.py @@ -10,7 +10,9 @@ from collections.abc import Container, Iterable from uuid import UUID - from policyengine_api.data.v2.catalog.catalog_selection import SelectedCatalog + from policyengine_api.data.v2.catalog.catalog_selection_validation import ( + SelectedCatalog, + ) from policyengine_api.data.v2.models import LegacyPolicyMapping from policyengine_api.services.v2.policies.types import ( LegacyPolicyTranslationReason, diff --git a/rules/fixtures/api.catalog.selection-validation-no-database/bad_catalog_validation.py b/rules/fixtures/api.catalog.selection-validation-no-database/bad_catalog_validation.py new file mode 100644 index 000000000..a9fe22d7f --- /dev/null +++ b/rules/fixtures/api.catalog.selection-validation-no-database/bad_catalog_validation.py @@ -0,0 +1,25 @@ +"""Proof fixture: version validation beside the catalog query. The rule rejects it. + +This is the shape of the former mixed catalog selection module: importing it +for ``validate_policyengine_version`` also loads SQLAlchemy and SQLModel. +""" + +from sqlmodel import Session, select + +from policyengine_api.data.v2.models import TaxBenefitModelVersion + + +def validate_policyengine_version(value: str) -> str: + """Return a non-empty version string.""" + if not value: + msg = "policyengine_version must be non-empty" + raise ValueError(msg) + return value + + +def select_catalog(session: Session, version: str) -> TaxBenefitModelVersion | None: + """Return the catalog version row for ``version``.""" + statement = select(TaxBenefitModelVersion).where( + TaxBenefitModelVersion.version == validate_policyengine_version(version) + ) + return session.exec(statement).first() diff --git a/rules/fixtures/api.catalog.selection-validation-no-database/good_catalog_validation.py b/rules/fixtures/api.catalog.selection-validation-no-database/good_catalog_validation.py new file mode 100644 index 000000000..c3b6fba9e --- /dev/null +++ b/rules/fixtures/api.catalog.selection-validation-no-database/good_catalog_validation.py @@ -0,0 +1,11 @@ +"""Proof fixture: version validation with no database imports. The rule accepts it.""" + +from packaging.version import Version + + +def validate_policyengine_version(value: str) -> str: + """Return a canonical version string.""" + if str(Version(value)) != value: + msg = "policyengine_version must be a canonical version" + raise ValueError(msg) + return value diff --git a/rules/registry.toml b/rules/registry.toml index 77d9b17d8..9ed53232d 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -66,6 +66,9 @@ guard_test_globs = [ "tests/unit/v2/test_stage12_comparison_isolation.py", ] extra_guard_tests = [ + "tests/unit/v2/test_catalog_selection_modules.py::test_importing_the_validation_module_loads_no_database_code", + "tests/unit/v2/test_catalog_selection_modules.py::test_the_reads_module_holds_the_catalog_query", + "tests/unit/v2/test_catalog_selection_modules.py::test_the_validation_module_imports_no_database_code", "tests/unit/v2/test_catalog_extraction.py::test_extractor_has_no_direct_core_country_or_v1_metadata_imports", "tests/unit/v2/test_catalog_extraction.py::test_extractor_modules_have_no_direct_core_country_or_v1_metadata_imports", "tests/unit/v2/test_catalog_initialization.py::test_initializer_is_not_imported_by_application_startup_modules", @@ -527,6 +530,31 @@ workflow = ".github/workflows/pr.yml" job = "test" invocation = "make test" +[[rule]] +id = "api.catalog.selection-validation-no-database" +category = "isolation" +statement = "Importing catalog_selection_validation (catalog version validation, its error types and SelectedCatalog) loads no SQLAlchemy, SQLModel, v2 table model or v2 database module; the catalog query lives in catalog_selection_reads." +rationale = "docs/engineering/skills/v2-code-organization.md#dependency-direction" +scope = ["policyengine_api/data/__init__.py", "policyengine_api/data/v2/__init__.py", "policyengine_api/data/v2/catalog/__init__.py", "policyengine_api/data/v2/catalog/catalog_selection_validation.py", "policyengine_api/supported_countries.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "pytest" +nodes = [ + "tests/unit/v2/test_catalog_selection_modules.py::test_the_validation_module_imports_no_database_code", + "tests/unit/v2/test_catalog_selection_modules.py::test_importing_the_validation_module_loads_no_database_code", + "tests/unit/v2/test_catalog_selection_modules.py::test_the_reads_module_holds_the_catalog_query", +] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "test" +invocation = "make test" +[rule.proof] +bad_fixture = "rules/fixtures/api.catalog.selection-validation-no-database/bad_catalog_validation.py" +good_fixture = "rules/fixtures/api.catalog.selection-validation-no-database/good_catalog_validation.py" +test = "tests/unit/v2/test_catalog_selection_modules.py::test_rule_proof_selection_validation_no_database" + [[rule]] id = "api.metadata.read-modules-isolated" category = "isolation" diff --git a/tests/unit/services/test_policy_mirroring.py b/tests/unit/services/test_policy_mirroring.py index 9394f3b88..de695a56d 100644 --- a/tests/unit/services/test_policy_mirroring.py +++ b/tests/unit/services/test_policy_mirroring.py @@ -9,7 +9,7 @@ from sqlalchemy.exc import OperationalError from sqlalchemy.exc import TimeoutError as PoolTimeoutError -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogNotPublishedError, MetadataCatalogUnavailableError, MetadataCatalogVersionNotFoundError, diff --git a/tests/unit/test_supported_countries.py b/tests/unit/test_supported_countries.py index 68fe331fc..9565bec1a 100644 --- a/tests/unit/test_supported_countries.py +++ b/tests/unit/test_supported_countries.py @@ -50,7 +50,7 @@ from pydantic import TypeAdapter from policyengine_api import constants -from policyengine_api.data.v2.catalog import catalog_selection, extraction +from policyengine_api.data.v2.catalog import catalog_selection_validation, extraction from policyengine_api.query_parameters import CountryId from scripts import process_v1_household_mirror_event @@ -75,7 +75,9 @@ def capture_extraction(**arguments): "extracted_countries": list(extraction.SUPPORTED_COUNTRIES), "required_dependencies": list(extraction.REQUIRED_DEPENDENCIES), "installed_models": captured_arguments["models"], - "dual_write_countries": sorted(catalog_selection.SUPPORTED_V2_COUNTRY_IDS), + "dual_write_countries": sorted( + catalog_selection_validation.SUPPORTED_V2_COUNTRY_IDS + ), "household_copy_countries": list(country_option.choices), "v1_countries": list(constants.COUNTRIES), "v1_country_packages": list(constants.COUNTRY_PACKAGE_NAMES), diff --git a/tests/unit/v2/test_catalog_selection_modules.py b/tests/unit/v2/test_catalog_selection_modules.py new file mode 100644 index 000000000..3ce88c651 --- /dev/null +++ b/tests/unit/v2/test_catalog_selection_modules.py @@ -0,0 +1,311 @@ +"""Catalog version selection keeps its database read apart from its validation. + +Rule api.catalog.selection-validation-no-database (rules/registry.toml). +""" + +from __future__ import annotations + +import ast +import inspect +import json +import re +import subprocess +import sys +from pathlib import Path + +PROJECT_ROOT = Path(__file__).parents[3] +CATALOG_DIRECTORY = PROJECT_ROOT / "policyengine_api" / "data" / "v2" / "catalog" +VALIDATION_MODULE = CATALOG_DIRECTORY / "catalog_selection_validation.py" +VALIDATION_MODULE_NAME = "policyengine_api.data.v2.catalog.catalog_selection_validation" +PROOF_FIXTURES = ( + PROJECT_ROOT / "rules" / "fixtures" / "api.catalog.selection-validation-no-database" +) +# Never imported by the validation module, even under TYPE_CHECKING. +DATABASE_PACKAGES = ("sqlalchemy", "sqlmodel", "policyengine_api.data.v2.database") +# Imported by the validation module only under TYPE_CHECKING, for annotations. +TABLE_MODELS_PACKAGE = "policyengine_api.data.v2.models" +# Never loaded when the validation module is imported. +RUNTIME_DATABASE_PACKAGES = (*DATABASE_PACKAGES, TABLE_MODELS_PACKAGE) +FORMER_MODULE = "policyengine_api.data.v2.catalog.catalog_selection" +# A reference to the former module: a dotted module path, the file name, or +# an import of the module by name. The bare word may still be used. +FORMER_MODULE_NAME = re.compile( + r"\.catalog_selection(?!\w)|\bcatalog_selection\.py\b" + r"|\bimport\s+catalog_selection(?!\w)" +) +# Release notes may name the former module: they describe past releases. +RELEASE_NOTES = ("CHANGELOG.md", "changelog.d/") +LOADED_MODULES_SCRIPT = """ +import importlib +import json +import sys + +sys.path[:0] = json.loads(sys.argv[2]) +importlib.import_module(sys.argv[1]) +print(json.dumps(sorted(sys.modules))) +""" + + +def _is_type_checking_block(node: ast.AST) -> bool: + if not isinstance(node, ast.If): + return False + condition = node.test + return (isinstance(condition, ast.Name) and condition.id == "TYPE_CHECKING") or ( + isinstance(condition, ast.Attribute) and condition.attr == "TYPE_CHECKING" + ) + + +def _imported_from(node: ast.ImportFrom, module_name: str, *, is_package: bool) -> str: + """Return the absolute module a ``from ... import`` statement reads from.""" + if node.level == 0: + return node.module or "" + package_parts = module_name.split(".") + if not is_package: + package_parts = package_parts[:-1] + if node.level > 1: + package_parts = package_parts[: -(node.level - 1)] + return ".".join([*package_parts, *([node.module] if node.module else [])]) + + +def _imported_modules( + tree: ast.Module, module_name: str = "", *, is_package: bool = False +) -> list[tuple[str, bool]]: + """Return every imported module name, and whether it is imported at runtime. + + ``from package import name`` is recorded as both ``package`` and + ``package.name``, because ``name`` may be a submodule. Relative imports are + resolved against ``module_name``. + """ + type_checking_imports = { + id(import_node) + for block in ast.walk(tree) + if _is_type_checking_block(block) + for statement in block.body + for import_node in ast.walk(statement) + } + imported = [] + for node in ast.walk(tree): + if isinstance(node, ast.Import): + names = [alias.name for alias in node.names] + elif isinstance(node, ast.ImportFrom): + source = _imported_from(node, module_name, is_package=is_package) + names = [source, *(f"{source}.{alias.name}" for alias in node.names)] + else: + continue + imported.extend((name, id(node) not in type_checking_imports) for name in names) + return imported + + +def _is_within(module_name: str, package: str) -> bool: + return module_name == package or module_name.startswith(f"{package}.") + + +def _database_modules_loaded_by( + module_name: str, search_paths: tuple[Path, ...] = () +) -> list[str]: + """List the database modules loaded by importing ``module_name``. + + The import runs in a fresh interpreter, so modules that its parent + packages' ``__init__`` files load are counted too. + """ + completed = subprocess.run( + [ + sys.executable, + "-c", + LOADED_MODULES_SCRIPT, + module_name, + json.dumps([str(path) for path in search_paths]), + ], + cwd=PROJECT_ROOT, + capture_output=True, + text=True, + check=True, + timeout=120, + ) + loaded = json.loads(completed.stdout.splitlines()[-1]) + return [ + name + for name in loaded + if any(_is_within(name, package) for package in RUNTIME_DATABASE_PACKAGES) + ] + + +def _database_imports_in(path: Path, module_name: str) -> list[str]: + """List the imports of database code a module's own source makes. + + These are SQLAlchemy, SQLModel and the v2 database module anywhere, even + under TYPE_CHECKING, and the v2 table models outside TYPE_CHECKING. + """ + imported = _imported_modules(ast.parse(path.read_text("utf-8")), module_name) + return [ + imported_name + for imported_name, at_runtime in imported + if any(_is_within(imported_name, package) for package in DATABASE_PACKAGES) + or (at_runtime and _is_within(imported_name, TABLE_MODELS_PACKAGE)) + ] + + +def test_the_validation_module_imports_no_database_code() -> None: + assert _database_imports_in(VALIDATION_MODULE, VALIDATION_MODULE_NAME) == [] + + +def test_importing_the_validation_module_loads_no_database_code() -> None: + assert _database_modules_loaded_by(VALIDATION_MODULE_NAME) == [] + + +def test_rule_proof_selection_validation_no_database() -> None: + """Proof for rule api.catalog.selection-validation-no-database. + + Both of the rule's checks reject the bad fixture and accept the good one. + """ + bad_fixture = PROOF_FIXTURES / "bad_catalog_validation.py" + good_fixture = PROOF_FIXTURES / "good_catalog_validation.py" + + assert _database_imports_in(bad_fixture, "bad_catalog_validation") + assert not _database_imports_in(good_fixture, "good_catalog_validation") + assert _database_modules_loaded_by("bad_catalog_validation", (PROOF_FIXTURES,)) + assert not _database_modules_loaded_by("good_catalog_validation", (PROOF_FIXTURES,)) + + +def test_only_imports_under_type_checking_count_as_annotation_only() -> None: + tree = ast.parse( + "import typing\n" + "from typing import TYPE_CHECKING\n" + "if TYPE_CHECKING:\n" + " from policyengine_api.data.v2.models import Policy\n" + "if typing.TYPE_CHECKING:\n" + " from policyengine_api.data.v2.models import Report\n" + "if __debug__:\n" + " from policyengine_api.data.v2.models import Household\n" + "def load():\n" + " if True:\n" + " from policyengine_api.data.v2.models import Simulation\n" + ) + + at_runtime = { + module_name: runtime + for module_name, runtime in _imported_modules(tree, VALIDATION_MODULE_NAME) + if module_name.startswith(f"{TABLE_MODELS_PACKAGE}.") + } + + assert at_runtime == { + f"{TABLE_MODELS_PACKAGE}.Policy": False, + f"{TABLE_MODELS_PACKAGE}.Report": False, + f"{TABLE_MODELS_PACKAGE}.Household": True, + f"{TABLE_MODELS_PACKAGE}.Simulation": True, + } + + +def test_the_import_helper_resolves_package_and_relative_imports() -> None: + tree = ast.parse( + "from policyengine_api.data.v2 import models\n" + "from .. import database\n" + "from . import catalog_selection\n" + ) + + imported = { + module_name + for module_name, _ in _imported_modules(tree, VALIDATION_MODULE_NAME) + } + + assert "policyengine_api.data.v2.models" in imported + assert "policyengine_api.data.v2.database" in imported + assert FORMER_MODULE in imported + + +def test_the_reads_module_holds_the_catalog_query() -> None: + from policyengine_api.data.v2.catalog import ( + catalog_selection_reads, + catalog_selection_validation, + ) + + reads_functions = { + name + for name, member in inspect.getmembers(catalog_selection_reads) + if inspect.isfunction(member) + and member.__module__ == catalog_selection_reads.__name__ + } + validation_functions = { + name + for name, member in inspect.getmembers(catalog_selection_validation) + if inspect.isfunction(member) + and member.__module__ == catalog_selection_validation.__name__ + } + + assert reads_functions == {"select_catalog"} + assert validation_functions == {"validate_policyengine_version"} + + +def _module_name(path: Path) -> tuple[str, bool]: + relative = path.relative_to(PROJECT_ROOT).with_suffix("") + parts = list(relative.parts) + is_package = parts[-1] == "__init__" + if is_package: + parts = parts[:-1] + return ".".join(parts), is_package + + +def _imports_the_former_module(path: Path) -> bool: + module_name, is_package = _module_name(path) + tree = ast.parse(path.read_text("utf-8")) + return any( + _is_within(imported, FORMER_MODULE) + for imported, _ in _imported_modules(tree, module_name, is_package=is_package) + ) + + +def test_no_code_imports_the_former_mixed_module() -> None: + importers = [ + str(path.relative_to(PROJECT_ROOT)) + for directory in ("policyengine_api", "tests", "scripts") + for path in sorted((PROJECT_ROOT / directory).rglob("*.py")) + if _imports_the_former_module(path) + ] + + assert importers == [] + assert not (CATALOG_DIRECTORY / "catalog_selection.py").exists() + + +def _tracked_files() -> list[str]: + listed = subprocess.run( + ["git", "ls-files", "-z"], + cwd=PROJECT_ROOT, + capture_output=True, + text=True, + check=True, + ) + return sorted(name for name in listed.stdout.split("\0") if name) + + +def test_the_name_search_matches_module_references_only() -> None: + references = ( + 'patch("policyengine_api.data.v2.catalog.catalog_selection.select_catalog")', + "See `policyengine_api/data/v2/catalog/catalog_selection.py`.", + "from policyengine_api.data.v2.catalog import catalog_selection", + ) + unrelated = ( + "catalog_selection = select_catalog(session, country_id='us')", + "The catalog_selection step picks one version.", + "from policyengine_api.data.v2.catalog import catalog_selection_reads", + "policyengine_api.data.v2.catalog.catalog_selection_validation", + ) + + assert all(FORMER_MODULE_NAME.search(line) for line in references) + assert not any(FORMER_MODULE_NAME.search(line) for line in unrelated) + + +def test_no_file_names_the_former_mixed_module() -> None: + """Search every tracked file, which catches mock.patch targets and docs too.""" + this_file = Path(__file__).resolve().relative_to(PROJECT_ROOT).as_posix() + naming_files = [ + name + for name in _tracked_files() + if name != this_file + and not name.startswith(RELEASE_NOTES) + and (PROJECT_ROOT / name).is_file() + and FORMER_MODULE_NAME.search( + (PROJECT_ROOT / name).read_text("utf-8", errors="replace") + ) + ] + + assert naming_files == [] diff --git a/tests/unit/v2/test_metadata_route_caching.py b/tests/unit/v2/test_metadata_route_caching.py index 7db0f0cc4..89719991e 100644 --- a/tests/unit/v2/test_metadata_route_caching.py +++ b/tests/unit/v2/test_metadata_route_caching.py @@ -6,7 +6,7 @@ import pytest -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogUnavailableError, ) from policyengine_api.fastapi_routes import dependencies as route_dependencies diff --git a/tests/unit/v2/test_metadata_routes.py b/tests/unit/v2/test_metadata_routes.py index 6f7aae73e..6d92bf348 100644 --- a/tests/unit/v2/test_metadata_routes.py +++ b/tests/unit/v2/test_metadata_routes.py @@ -6,7 +6,7 @@ import pytest -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( InvalidPolicyEngineVersionError, MetadataCatalogUnavailableError, MetadataCatalogVersionNotFoundError, diff --git a/tests/unit/v2/test_metadata_search_file_routes.py b/tests/unit/v2/test_metadata_search_file_routes.py index 71cf630a3..778c4aee1 100644 --- a/tests/unit/v2/test_metadata_search_file_routes.py +++ b/tests/unit/v2/test_metadata_search_file_routes.py @@ -10,7 +10,7 @@ import pytest from pydantic import BaseModel -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogVersionNotFoundError, ) from policyengine_api.data.v2.catalog.parameter_search_file import ( diff --git a/tests/unit/v2/test_metadata_service.py b/tests/unit/v2/test_metadata_service.py index b78d2f6b7..33e699264 100644 --- a/tests/unit/v2/test_metadata_service.py +++ b/tests/unit/v2/test_metadata_service.py @@ -11,7 +11,7 @@ from sqlalchemy import event from sqlmodel import Session, select -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( InvalidPolicyEngineVersionError, MetadataCatalogUnavailableError, MetadataCatalogVersionNotFoundError, @@ -557,7 +557,8 @@ def test_read_modules_import_no_policyengine_or_v1_metadata_source() -> None: project_package / "services" / "v2" / "metadata" / "database_connectors" ) modules = ( - project_package / "data" / "v2" / "catalog" / "catalog_selection.py", + project_package / "data" / "v2" / "catalog" / "catalog_selection_reads.py", + project_package / "data" / "v2" / "catalog" / "catalog_selection_validation.py", connector_directory / "reads.py", connector_directory / "reads_datasets.py", connector_directory / "reads_parameter_nodes.py", diff --git a/tests/unit/v2/test_named_limit_messages.py b/tests/unit/v2/test_named_limit_messages.py index a55c2a46b..f916ae37c 100644 --- a/tests/unit/v2/test_named_limit_messages.py +++ b/tests/unit/v2/test_named_limit_messages.py @@ -5,7 +5,7 @@ import pytest from policyengine_api.data.places import validate_place_code -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( InvalidPolicyEngineVersionError, validate_policyengine_version, ) diff --git a/tests/unit/v2/test_policy_catalog.py b/tests/unit/v2/test_policy_catalog.py index 3dc5f5235..87bffb464 100644 --- a/tests/unit/v2/test_policy_catalog.py +++ b/tests/unit/v2/test_policy_catalog.py @@ -7,7 +7,7 @@ import pytest from sqlmodel import Session, create_engine -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogVersionNotFoundError, ) from policyengine_api.data.v2.models import ( diff --git a/tests/unit/v2/test_policy_legacy_translation.py b/tests/unit/v2/test_policy_legacy_translation.py index 6f780fb79..35dfdaa87 100644 --- a/tests/unit/v2/test_policy_legacy_translation.py +++ b/tests/unit/v2/test_policy_legacy_translation.py @@ -10,7 +10,7 @@ from sqlalchemy.orm import sessionmaker from sqlmodel import Session, create_engine -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogNotPublishedError, ) from policyengine_api.data.v2.models import ( diff --git a/tests/unit/v2/test_policy_missing_parameters.py b/tests/unit/v2/test_policy_missing_parameters.py index 83b896cc2..ebf41c08d 100644 --- a/tests/unit/v2/test_policy_missing_parameters.py +++ b/tests/unit/v2/test_policy_missing_parameters.py @@ -10,7 +10,7 @@ from sqlalchemy.orm import sessionmaker from sqlmodel import Session, create_engine -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogUnavailableError, ) from policyengine_api.data.v2.models import ( diff --git a/tests/unit/v2/test_policy_routes.py b/tests/unit/v2/test_policy_routes.py index 790b0d413..8dad07c50 100644 --- a/tests/unit/v2/test_policy_routes.py +++ b/tests/unit/v2/test_policy_routes.py @@ -12,7 +12,7 @@ from sqlalchemy.exc import TimeoutError as PoolTimeoutError from policyengine_api.asgi_factory import create_asgi_app -from policyengine_api.data.v2.catalog.catalog_selection import ( +from policyengine_api.data.v2.catalog.catalog_selection_validation import ( MetadataCatalogUnavailableError, MetadataCatalogVersionNotFoundError, )