tegwick
8a1a2426d5
Three defects the test suite could not have caught, because each needed a real cluster, a mounted secret, or a live PostgreSQL. env.py read database_url rather than resolved_database_url. `configured` was true because a file was set, and the field it then read was empty — so Alembic received an empty URL and the migration could never run in the cluster. The value is also now escaped for ConfigParser interpolation, since a `%` in a generated password would otherwise raise at credential rotation, which is the worst time to find out. SET ROLE opened an implicit transaction that Alembic then nested inside rather than owning, so it never committed and leaving the connection block rolled everything back. Alembic logged "Running upgrade" for every revision against a database that stayed empty. SET ROLE is session-scoped, so committing immediately ends the implicit transaction without discarding the role. A missing optional publish-token file was treated as a hard failure. The absence is the documented read-only posture — the secret is mounted optional and deliberately not issued — so treating it as a fault turned an intended state into a 500 rather than the 503 that explains it. `required` now separates the two cases: a missing database URL still fails loudly, because there the silence would hide a real fault. Migrations also assume the durable owner role rather than creating objects as the leased migration login, per the rapp-postgres database-owner boundary. The role name is validated against an identifier pattern because SET ROLE cannot be parameterised. Service tests 36 -> 47. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bjefh8NUiEiahN4JLwoSKM Assistant: claude-code Assistant-Model: opus Assistant-Process: 388925@bnt-lap001 Assistant-Session: 3507023f-e0fd-4a1e-9d90-a0d4217d1502
107 lines
4 KiB
Python
107 lines
4 KiB
Python
"""Schema obligations from business-app-service-contract_v0.1 § 1.3."""
|
|
|
|
from __future__ import annotations
|
|
|
|
from canned_prompts_service import models
|
|
|
|
|
|
def test_every_owned_table_is_tenant_keyed() -> None:
|
|
for table in models.Base.metadata.sorted_tables:
|
|
assert "tenant" in table.c, f"{table.name} holds owned data without a tenant key"
|
|
|
|
|
|
def test_uniqueness_is_scoped_by_tenant_and_registry() -> None:
|
|
"""Identity is registry-scoped (§ 3.2); two tenants may hold the same id."""
|
|
constraint = next(
|
|
c for c in models.PackageVersion.__table__.constraints
|
|
if getattr(c, "name", "") == "uq_package_version"
|
|
)
|
|
assert [c.name for c in constraint.columns] == ["tenant", "registry", "package_id", "version"]
|
|
|
|
|
|
def test_index_entry_records_arrival_not_authorship() -> None:
|
|
columns = models.IndexEntry.__table__.c
|
|
assert "included_at" in columns and "last_seen_at" in columns
|
|
assert "source" in columns and "method" in columns
|
|
|
|
|
|
# --- migration role assumption (rapp-postgres boundary requirement) ---
|
|
|
|
import pytest
|
|
|
|
from canned_prompts_service import db as service_db
|
|
|
|
|
|
class FakeConnection:
|
|
def __init__(self) -> None:
|
|
self.statements: list[str] = []
|
|
self.commits = 0
|
|
|
|
def execute(self, statement) -> None:
|
|
self.statements.append(str(statement))
|
|
|
|
def commit(self) -> None:
|
|
self.commits += 1
|
|
|
|
|
|
def test_no_role_assumed_when_unset(monkeypatch) -> None:
|
|
monkeypatch.delenv("CANNED_PROMPTS_MIGRATION_ROLE", raising=False)
|
|
conn = FakeConnection()
|
|
service_db.assume_owner_role(conn)
|
|
assert conn.statements == []
|
|
|
|
|
|
def test_owner_role_is_assumed(monkeypatch) -> None:
|
|
"""Objects must end up owned by the durable role, not a leased login."""
|
|
monkeypatch.setenv("CANNED_PROMPTS_MIGRATION_ROLE", "canned_prompts_owner")
|
|
conn = FakeConnection()
|
|
service_db.assume_owner_role(conn)
|
|
assert conn.statements == ['SET ROLE "canned_prompts_owner"']
|
|
|
|
|
|
@pytest.mark.parametrize("bad", ['x"; drop schema public; --', "role-with-dash", "1role", ""])
|
|
def test_role_name_is_validated_not_trusted(monkeypatch, bad) -> None:
|
|
"""SET ROLE cannot be parameterised, so the name is validated."""
|
|
monkeypatch.setenv("CANNED_PROMPTS_MIGRATION_ROLE", bad)
|
|
conn = FakeConnection()
|
|
if bad == "":
|
|
service_db.assume_owner_role(conn)
|
|
assert conn.statements == []
|
|
else:
|
|
with pytest.raises(ValueError, match="invalid migration role"):
|
|
service_db.assume_owner_role(conn)
|
|
|
|
|
|
def test_configured_but_env_field_empty_is_the_cluster_case() -> None:
|
|
"""The bug that broke the first migration: `configured` was true because a
|
|
file was set, while the plain field it then read was empty."""
|
|
from canned_prompts_service.settings import Settings
|
|
import tempfile, os
|
|
|
|
with tempfile.NamedTemporaryFile("w", suffix=".url", delete=False) as f:
|
|
f.write("postgresql+psycopg://u:p%40x@h/db")
|
|
path = f.name
|
|
settings = Settings(database_url_file=path)
|
|
assert settings.configured is True
|
|
assert settings.database_url == ""
|
|
assert settings.resolved_database_url.startswith("postgresql+psycopg://")
|
|
# A percent in the password must survive ConfigParser interpolation.
|
|
assert settings.resolved_database_url.replace("%", "%%").count("%%") == 1
|
|
os.unlink(path)
|
|
|
|
|
|
def test_set_role_commits_so_alembic_owns_its_transaction(monkeypatch) -> None:
|
|
"""Regression: SET ROLE opened an implicit transaction that Alembic then
|
|
nested inside rather than owning, so every migration was rolled back while
|
|
logging as applied."""
|
|
monkeypatch.setenv("CANNED_PROMPTS_MIGRATION_ROLE", "canned_prompts_owner")
|
|
conn = FakeConnection()
|
|
service_db.assume_owner_role(conn)
|
|
assert conn.commits == 1, "SET ROLE must not leave an open transaction"
|
|
|
|
|
|
def test_no_commit_when_no_role_is_assumed(monkeypatch) -> None:
|
|
monkeypatch.delenv("CANNED_PROMPTS_MIGRATION_ROLE", raising=False)
|
|
conn = FakeConnection()
|
|
service_db.assume_owner_role(conn)
|
|
assert conn.commits == 0
|