From 6b189c352b12485df20716108b1517db74925807 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 14:48:21 +0400 Subject: [PATCH 1/7] Test the catalog_selection split into validation and reads modules (failing) Add structure tests for policyengine_api/data/v2/catalog: the validation module imports no SQLAlchemy, SQLModel or database module (and the table models only under TYPE_CHECKING), the reads module holds select_catalog, and no code imports the former catalog_selection module. Point every test import at catalog_selection_validation. These fail until the split lands: the new modules do not exist yet. Refs #4176 Co-Authored-By: Claude Opus 5.5 --- tests/unit/services/test_policy_mirroring.py | 2 +- tests/unit/test_supported_countries.py | 6 +- .../unit/v2/test_catalog_selection_modules.py | 113 ++++++++++++++++++ tests/unit/v2/test_metadata_route_caching.py | 2 +- tests/unit/v2/test_metadata_routes.py | 2 +- .../v2/test_metadata_search_file_routes.py | 2 +- tests/unit/v2/test_metadata_service.py | 5 +- tests/unit/v2/test_named_limit_messages.py | 2 +- tests/unit/v2/test_policy_catalog.py | 2 +- .../unit/v2/test_policy_legacy_translation.py | 2 +- .../unit/v2/test_policy_missing_parameters.py | 2 +- tests/unit/v2/test_policy_routes.py | 2 +- 12 files changed, 129 insertions(+), 13 deletions(-) create mode 100644 tests/unit/v2/test_catalog_selection_modules.py 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..082b678ab --- /dev/null +++ b/tests/unit/v2/test_catalog_selection_modules.py @@ -0,0 +1,113 @@ +"""Catalog version selection keeps its database read apart from its validation.""" + +from __future__ import annotations + +import ast +import inspect +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" +READS_MODULE = CATALOG_DIRECTORY / "catalog_selection_reads.py" +DATABASE_PACKAGES = ("sqlalchemy", "sqlmodel", "policyengine_api.data.v2.database") +TABLE_MODELS_PACKAGE = "policyengine_api.data.v2.models" + + +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_modules(tree: ast.Module) -> list[tuple[str, bool]]: + """Return every imported module name, and whether it is imported at runtime.""" + 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) and node.level == 0 and node.module: + names = [node.module] + 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 test_the_validation_module_imports_no_database_code() -> None: + imported = _imported_modules(ast.parse(VALIDATION_MODULE.read_text("utf-8"))) + + database_imports = [ + module_name + for module_name, _ in imported + if any(_is_within(module_name, package) for package in DATABASE_PACKAGES) + ] + runtime_table_model_imports = [ + module_name + for module_name, at_runtime in imported + if at_runtime and _is_within(module_name, TABLE_MODELS_PACKAGE) + ] + + assert database_imports == [] + assert runtime_table_model_imports == [] + + +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 _imports_the_former_mixed_module(tree: ast.Module) -> bool: + former_module = "policyengine_api.data.v2.catalog.catalog_selection" + if any(module_name == former_module for module_name, _ in _imported_modules(tree)): + return True + return any( + isinstance(node, ast.ImportFrom) + and node.module == "policyengine_api.data.v2.catalog" + and any(alias.name == "catalog_selection" for alias in node.names) + for node in ast.walk(tree) + ) + + +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_mixed_module(ast.parse(path.read_text("utf-8"))) + ] + + assert importers == [] + assert not (CATALOG_DIRECTORY / "catalog_selection.py").exists() 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, ) From 49d930e19e59c8d0d600ae56d7dae844e67b3e62 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 14:50:21 +0400 Subject: [PATCH 2/7] Split catalog_selection into validation and reads modules policyengine_api/data/v2/catalog/catalog_selection.py mixed catalog version validation, its error types and the SelectedCatalog result type with the one catalog query. Every v2 validator, transformation and type that needed the version validation or the error types therefore imported SQLAlchemy and SQLModel. - catalog_selection_validation.py holds the validation, constants, error types and SelectedCatalog. It imports no SQLAlchemy, SQLModel or database module, and the table models only under TYPE_CHECKING. - catalog_selection_reads.py holds select_catalog. - Every import of the former module now names one of the two. With the table models' own SQLAlchemy imports set aside, no v2 validator, transformation or type module reaches sqlalchemy any more (23 did before). Fixes #4176 Co-Authored-By: Claude Opus 5.5 --- changelog.d/4176.changed.md | 1 + .../v2/catalog/catalog_selection_reads.py | 76 +++++++++++++++++++ ...ion.py => catalog_selection_validation.py} | 72 ++++-------------- .../fastapi_routes/v2/metadata/common.py | 2 +- .../fastapi_routes/v2/policies/routes.py | 2 +- policyengine_api/query_parameters.py | 2 +- policyengine_api/routes/household_routes.py | 4 +- policyengine_api/routes/policy_routes.py | 2 +- policyengine_api/services/policy_mirroring.py | 2 +- .../v2/metadata/database_connectors/reads.py | 6 +- .../services/v2/metadata/selected_catalogs.py | 4 +- .../services/v2/metadata/services.py | 2 +- .../services/v2/metadata/transformations.py | 4 +- .../services/v2/metadata/validators.py | 6 +- .../v2/policies/database_connectors/reads.py | 9 ++- .../services/v2/policies/services.py | 2 +- .../services/v2/policies/transformations.py | 4 +- .../services/v2/policies/validators.py | 4 +- 18 files changed, 124 insertions(+), 80 deletions(-) create mode 100644 changelog.d/4176.changed.md create mode 100644 policyengine_api/data/v2/catalog/catalog_selection_reads.py rename policyengine_api/data/v2/catalog/{catalog_selection.py => catalog_selection_validation.py} (52%) diff --git a/changelog.d/4176.changed.md b/changelog.d/4176.changed.md new file mode 100644 index 000000000..da63c1d19 --- /dev/null +++ b/changelog.d/4176.changed.md @@ -0,0 +1 @@ +Split the v2 catalog version selection into `catalog_selection_validation`, which imports no database code, and `catalog_selection_reads`, which holds the one catalog query, so v2 validators, transformations and types no longer import SQLAlchemy through it. 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..381846b72 --- /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 imports no database code. +""" + +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..a0e00b935 100644 --- a/policyengine_api/data/v2/catalog/catalog_selection.py +++ b/policyengine_api/data/v2/catalog/catalog_selection_validation.py @@ -1,19 +1,25 @@ -"""PolicyEngine.py catalog version selection for v2 resource reads.""" +"""Version validation, error types and the result type for v2 catalog selection. + +This module imports no SQLAlchemy, SQLModel or database module, 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 +78,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, From 14b3fcc99bf7f892ec656ce7578690c60b0afe39 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:04:11 +0400 Subject: [PATCH 3/7] Test that importing the catalog validation module loads no database code (failing) The static check read only catalog_selection_validation.py itself, so it missed that importing the module first runs the catalog package's __init__.py, which imports extraction and through it SQLModel, SQLAlchemy and every v2 table model. - Import the module in a fresh interpreter and list the database modules then loaded; proof fixtures show a mixed module is rejected and a database-free one accepted. - Resolve relative imports and record "from package import name" as package.name, so imports such as "from policyengine_api.data.v2 import models" are seen. - Search the repository's text files for the former module's name, which also catches mock.patch targets and docs. test_importing_the_validation_module_loads_no_database_code fails until the catalog package stops importing extraction when it loads. Refs #4176 Co-Authored-By: Claude Opus 5.5 --- .../bad_catalog_validation.py | 25 +++ .../good_catalog_validation.py | 11 ++ .../unit/v2/test_catalog_selection_modules.py | 171 ++++++++++++++++-- 3 files changed, 191 insertions(+), 16 deletions(-) create mode 100644 rules/fixtures/api.catalog.selection-validation-no-database/bad_catalog_validation.py create mode 100644 rules/fixtures/api.catalog.selection-validation-no-database/good_catalog_validation.py 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/tests/unit/v2/test_catalog_selection_modules.py b/tests/unit/v2/test_catalog_selection_modules.py index 082b678ab..296423931 100644 --- a/tests/unit/v2/test_catalog_selection_modules.py +++ b/tests/unit/v2/test_catalog_selection_modules.py @@ -1,17 +1,55 @@ -"""Catalog version selection keeps its database read apart from its validation.""" +"""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" -READS_MODULE = CATALOG_DIRECTORY / "catalog_selection_reads.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" +FORMER_MODULE_NAME = re.compile(r"\bcatalog_selection\b") +SEARCHED_DIRECTORIES = ( + ".github", + "docs", + "gcp", + "migrations", + "policyengine_api", + "rules", + "scripts", + "tests", +) +SEARCHED_SUFFIXES = frozenset( + {".cfg", ".ini", ".md", ".py", ".sh", ".toml", ".txt", ".yaml", ".yml"} +) +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: @@ -23,8 +61,27 @@ def _is_type_checking_block(node: ast.AST) -> bool: ) -def _imported_modules(tree: ast.Module) -> list[tuple[str, bool]]: - """Return every imported module name, and whether it is imported at runtime.""" +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) @@ -36,8 +93,9 @@ def _imported_modules(tree: ast.Module) -> list[tuple[str, bool]]: for node in ast.walk(tree): if isinstance(node, ast.Import): names = [alias.name for alias in node.names] - elif isinstance(node, ast.ImportFrom) and node.level == 0 and node.module: - names = [node.module] + 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) @@ -48,8 +106,40 @@ 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 test_the_validation_module_imports_no_database_code() -> None: - imported = _imported_modules(ast.parse(VALIDATION_MODULE.read_text("utf-8"))) + imported = _imported_modules( + ast.parse(VALIDATION_MODULE.read_text("utf-8")), VALIDATION_MODULE_NAME + ) database_imports = [ module_name @@ -66,6 +156,33 @@ def test_the_validation_module_imports_no_database_code() -> None: assert runtime_table_model_imports == [] +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.""" + assert _database_modules_loaded_by("bad_catalog_validation", (PROOF_FIXTURES,)) + assert not _database_modules_loaded_by("good_catalog_validation", (PROOF_FIXTURES,)) + + +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, @@ -89,15 +206,21 @@ def test_the_reads_module_holds_the_catalog_query() -> None: assert validation_functions == {"validate_policyengine_version"} -def _imports_the_former_mixed_module(tree: ast.Module) -> bool: - former_module = "policyengine_api.data.v2.catalog.catalog_selection" - if any(module_name == former_module for module_name, _ in _imported_modules(tree)): - return True +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( - isinstance(node, ast.ImportFrom) - and node.module == "policyengine_api.data.v2.catalog" - and any(alias.name == "catalog_selection" for alias in node.names) - for node in ast.walk(tree) + _is_within(imported, FORMER_MODULE) + for imported, _ in _imported_modules(tree, module_name, is_package=is_package) ) @@ -106,8 +229,24 @@ def test_no_code_imports_the_former_mixed_module() -> None: 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_mixed_module(ast.parse(path.read_text("utf-8"))) + if _imports_the_former_module(path) ] assert importers == [] assert not (CATALOG_DIRECTORY / "catalog_selection.py").exists() + + +def test_no_file_names_the_former_mixed_module() -> None: + """Catch string references too, such as mock.patch targets and docs.""" + this_file = Path(__file__).resolve() + naming_files = [ + str(path.relative_to(PROJECT_ROOT)) + for directory in SEARCHED_DIRECTORIES + for path in sorted((PROJECT_ROOT / directory).rglob("*")) + if path.suffix in SEARCHED_SUFFIXES + and path.is_file() + and path.resolve() != this_file + and FORMER_MODULE_NAME.search(path.read_text("utf-8", errors="replace")) + ] + + assert naming_files == [] From 7c3aba249b1bc6b0efd47f1fe39c71954111fdab Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:06:21 +0400 Subject: [PATCH 4/7] Stop the catalog package importing extraction when it loads Importing any module in policyengine_api.data.v2.catalog first ran the package's __init__.py, which imported extraction and, through it, SQLModel, SQLAlchemy and every v2 table model. So importing catalog_selection_validation still loaded the database code it no longer imports itself. No code used the names __init__.py re-exported, so it is now only a docstring. Importing a v2 validators, transformations or types module now loads SQLAlchemy in 3 of 39 cases, each of which imports the table models itself; on master 23 did. Register the checks as rule api.catalog.selection-validation-no-database, with proof fixtures, and cite it in v2-code-organization.md. Refs #4176 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- changelog.d/4176.changed.md | 2 +- .../skills/v2-code-organization.md | 7 +++++ docs/generated/rules.md | 9 +++++-- policyengine_api/data/v2/catalog/__init__.py | 20 +++++--------- .../catalog/catalog_selection_validation.py | 3 ++- rules/registry.toml | 26 +++++++++++++++++++ 7 files changed, 50 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1f7b8c19e..7af27b9a4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -102,7 +102,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 index da63c1d19..807b5b944 100644 --- a/changelog.d/4176.changed.md +++ b/changelog.d/4176.changed.md @@ -1 +1 @@ -Split the v2 catalog version selection into `catalog_selection_validation`, which imports no database code, and `catalog_selection_reads`, which holds the one catalog query, so v2 validators, transformations and types no longer import SQLAlchemy through it. +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..b52b10ae4 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`, and only +database 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 98a079179..04dfd7742 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 | 59 | 0 | 1 | 13 | 73 | +| mechanical | 60 | 0 | 1 | 13 | 74 | | metric | 1 | 0 | 0 | 0 | 1 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 60 of 75 rules. +**Blocking:** 61 of 76 rules. ## Gaps @@ -201,6 +201,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** | @@ -212,6 +213,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)") @@ -223,6 +226,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_validation.py b/policyengine_api/data/v2/catalog/catalog_selection_validation.py index a0e00b935..0c94fb7ba 100644 --- a/policyengine_api/data/v2/catalog/catalog_selection_validation.py +++ b/policyengine_api/data/v2/catalog/catalog_selection_validation.py @@ -1,6 +1,7 @@ """Version validation, error types and the result type for v2 catalog selection. -This module imports no SQLAlchemy, SQLModel or database module, so v2 +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``. """ diff --git a/rules/registry.toml b/rules/registry.toml index 04f5be4bf..dd95851f9 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -66,6 +66,8 @@ 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_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 +529,30 @@ 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/v2/catalog/__init__.py", "policyengine_api/data/v2/catalog/catalog_selection_validation.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", +] +[[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" From 95bab05aa498807c3d167e3bd09ec72355aa7705 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:23:40 +0400 Subject: [PATCH 5/7] Prove both catalog validation checks and search every tracked file - The rule's proof now runs the static import check on the fixtures as well as the fresh-interpreter check. - A new test pins which imports count as TYPE_CHECKING-only: those under "if TYPE_CHECKING:" or "if typing.TYPE_CHECKING:", and no other "if". - The search for the former module's name reads every tracked file (git ls-files) except release notes, instead of a list of directories and suffixes. - The rule's scope adds the files the runtime check depends on: the parent package __init__.py files and supported_countries.py. - v2-code-organization.md describes which connectors call select_catalog instead of stating an unchecked constraint. Refs #4176 Co-Authored-By: Claude Opus 5.5 --- .../skills/v2-code-organization.md | 4 +- .../v2/catalog/catalog_selection_reads.py | 2 +- rules/registry.toml | 2 +- .../unit/v2/test_catalog_selection_modules.py | 113 ++++++++++++------ 4 files changed, 79 insertions(+), 42 deletions(-) diff --git a/docs/engineering/skills/v2-code-organization.md b/docs/engineering/skills/v2-code-organization.md index b52b10ae4..a02ce5dfe 100644 --- a/docs/engineering/skills/v2-code-organization.md +++ b/docs/engineering/skills/v2-code-organization.md @@ -177,8 +177,8 @@ 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`, and only -database connectors call it. +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 diff --git a/policyengine_api/data/v2/catalog/catalog_selection_reads.py b/policyengine_api/data/v2/catalog/catalog_selection_reads.py index 381846b72..f8c62ef53 100644 --- a/policyengine_api/data/v2/catalog/catalog_selection_reads.py +++ b/policyengine_api/data/v2/catalog/catalog_selection_reads.py @@ -1,7 +1,7 @@ """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 imports no database code. +``catalog_selection_validation``, which loads no database code when imported. """ from __future__ import annotations diff --git a/rules/registry.toml b/rules/registry.toml index dd95851f9..ab554ef98 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -534,7 +534,7 @@ 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/v2/catalog/__init__.py", "policyengine_api/data/v2/catalog/catalog_selection_validation.py"] +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 diff --git a/tests/unit/v2/test_catalog_selection_modules.py b/tests/unit/v2/test_catalog_selection_modules.py index 296423931..f93f0aad0 100644 --- a/tests/unit/v2/test_catalog_selection_modules.py +++ b/tests/unit/v2/test_catalog_selection_modules.py @@ -28,19 +28,8 @@ RUNTIME_DATABASE_PACKAGES = (*DATABASE_PACKAGES, TABLE_MODELS_PACKAGE) FORMER_MODULE = "policyengine_api.data.v2.catalog.catalog_selection" FORMER_MODULE_NAME = re.compile(r"\bcatalog_selection\b") -SEARCHED_DIRECTORIES = ( - ".github", - "docs", - "gcp", - "migrations", - "policyengine_api", - "rules", - "scripts", - "tests", -) -SEARCHED_SUFFIXES = frozenset( - {".cfg", ".ini", ".md", ".py", ".sh", ".toml", ".txt", ".yaml", ".yml"} -) +# Release notes may name the former module: they describe past releases. +RELEASE_NOTES = ("CHANGELOG.md", "changelog.d/") LOADED_MODULES_SCRIPT = """ import importlib import json @@ -136,24 +125,23 @@ def _database_modules_loaded_by( ] -def test_the_validation_module_imports_no_database_code() -> None: - imported = _imported_modules( - ast.parse(VALIDATION_MODULE.read_text("utf-8")), VALIDATION_MODULE_NAME - ) +def _database_imports_in(path: Path, module_name: str) -> list[str]: + """List the imports of database code a module's own source makes. - database_imports = [ - module_name - for module_name, _ in imported - if any(_is_within(module_name, package) for package in DATABASE_PACKAGES) - ] - runtime_table_model_imports = [ - module_name - for module_name, at_runtime in imported - if at_runtime and _is_within(module_name, TABLE_MODELS_PACKAGE) + 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)) ] - assert database_imports == [] - assert runtime_table_model_imports == [] + +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: @@ -161,11 +149,48 @@ def test_importing_the_validation_module_loads_no_database_code() -> None: def test_rule_proof_selection_validation_no_database() -> None: - """Proof for rule api.catalog.selection-validation-no-database.""" + """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" @@ -236,17 +261,29 @@ def test_no_code_imports_the_former_mixed_module() -> None: 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_no_file_names_the_former_mixed_module() -> None: - """Catch string references too, such as mock.patch targets and docs.""" - this_file = Path(__file__).resolve() + """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 = [ - str(path.relative_to(PROJECT_ROOT)) - for directory in SEARCHED_DIRECTORIES - for path in sorted((PROJECT_ROOT / directory).rglob("*")) - if path.suffix in SEARCHED_SUFFIXES - and path.is_file() - and path.resolve() != this_file - and FORMER_MODULE_NAME.search(path.read_text("utf-8", errors="replace")) + 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 == [] From f4c11b89118af4b0475f0fa23bdf5f92aff48026 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:44:11 +0400 Subject: [PATCH 6/7] Test that the former-module search ignores unrelated uses of the word (failing) The search matches the bare word catalog_selection, so a later variable or sentence using it would fail the test as if the removed module were still referenced. The new test fails until the pattern matches only module references: a dotted module path, the file name or an import. Refs #4176 Co-Authored-By: Claude Opus 5.5 --- tests/unit/v2/test_catalog_selection_modules.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/unit/v2/test_catalog_selection_modules.py b/tests/unit/v2/test_catalog_selection_modules.py index f93f0aad0..8efc30c5d 100644 --- a/tests/unit/v2/test_catalog_selection_modules.py +++ b/tests/unit/v2/test_catalog_selection_modules.py @@ -272,6 +272,23 @@ def _tracked_files() -> list[str]: 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() From d8c5c319a2158beda5658fb89ee60d0706fe6cd3 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:44:29 +0400 Subject: [PATCH 7/7] Match only module references in the former-module search Match a dotted module path ending in .catalog_selection, the file name catalog_selection.py, or "import catalog_selection", so the bare word stays usable for variables and prose. Also list test_the_reads_module_holds_the_catalog_query under rule api.catalog.selection-validation-no-database, whose statement says the catalog query lives in catalog_selection_reads. Refs #4176 Co-Authored-By: Claude Opus 5.5 --- rules/registry.toml | 2 ++ tests/unit/v2/test_catalog_selection_modules.py | 7 ++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/rules/registry.toml b/rules/registry.toml index ab554ef98..01dd889c2 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -67,6 +67,7 @@ guard_test_globs = [ ] 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", @@ -543,6 +544,7 @@ 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" diff --git a/tests/unit/v2/test_catalog_selection_modules.py b/tests/unit/v2/test_catalog_selection_modules.py index 8efc30c5d..3ce88c651 100644 --- a/tests/unit/v2/test_catalog_selection_modules.py +++ b/tests/unit/v2/test_catalog_selection_modules.py @@ -27,7 +27,12 @@ # 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" -FORMER_MODULE_NAME = re.compile(r"\bcatalog_selection\b") +# 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 = """