diff --git a/changelog.d/126-source-schema-checks.md b/changelog.d/126-source-schema-checks.md new file mode 100644 index 0000000..93d6999 --- /dev/null +++ b/changelog.d/126-source-schema-checks.md @@ -0,0 +1,3 @@ +### Added + +- `cloudfall migrate` takes a repeatable `--data-schema DATABASE=NAME` beside `--data DATABASE=PATH` (and the `migrate` MCP tool a `data_schema` list): the `data:` step copies only the named schemas, as `data copy --source-schema` does, and the plan names them. A `--source-schema` or `--data-schema` name must now match `^[a-z_][a-z0-9_]*$` and be at most 63 characters, and may be given once per copy; a name that doesn't, a repeated name, or a `--data-schema` for a database with no `--data` exits `PRECONDITION` (code `data_copy_schema_invalid` or `migrate_data_schema_without_data`) before any playbook runs, where a malformed name was a usage error and a repeat was silently dropped (#126) diff --git a/docs/neon-migration-guide.md b/docs/neon-migration-guide.md index dc94f24..e617441 100644 --- a/docs/neon-migration-guide.md +++ b/docs/neon-migration-guide.md @@ -181,7 +181,10 @@ By default the copy takes every non-system schema of the source, and the row-count check and the receipt cover all of them. To take only some, name each with a repeatable `--source-schema`, such as `--source-schema public`; the plan names the schemas it will copy, and the -receipt lists them under `spec.schemas`, each table with its `schema`. +receipt lists them under `spec.schemas`, each table with its `schema`. A +name must match `^[a-z_][a-z0-9_]*$`, be at most 63 characters, and be +named once; anything else stops the command with `PRECONDITION` before +any playbook runs. The row-count check is the verify step and the receipt is the record's entry for the migration. The restore is not done because `pg_restore` @@ -193,7 +196,8 @@ refusal guards, and cannot skip either. One database at a time; repeat per declared database. If the application is also moving in the same operation, `cloudfall migrate --data app=tmp/neon-app-url` inserts this step into the orchestrated plan -between services and deploys. +between services and deploys; `--data-schema app=public` (repeatable) +narrows it to the named schemas, as `--source-schema` does. Note the one-time egress: Neon meters the dump like any other transfer, so a 14 GB dump costs about a dollar — the last transfer bill this diff --git a/docs/render-migration-guide.md b/docs/render-migration-guide.md index 77e7f67..4b6a9e1 100644 --- a/docs/render-migration-guide.md +++ b/docs/render-migration-guide.md @@ -152,8 +152,10 @@ database that already has tables in the copied schemas, verifies per-table row counts, and writes a `DataMigrationReceipt`. The row-count check is the verify step: the migration is not done because the restore exited zero, it is done because the counts match. The copy takes every non-system schema of -the source; `data copy --source-schema NAME` (repeatable) takes only the -named ones. +the source; `data copy --source-schema NAME` and `migrate --data-schema +DATABASE=NAME` (both repeatable) take only the named ones. A name must +match `^[a-z_][a-z0-9_]*$` and be named once; anything else stops the +command with `PRECONDITION` before any playbook runs. ## Step 7: cut over DNS diff --git a/sdk/src/cloudfall/app.py b/sdk/src/cloudfall/app.py index da63d41..2f95dcc 100644 --- a/sdk/src/cloudfall/app.py +++ b/sdk/src/cloudfall/app.py @@ -129,7 +129,6 @@ ConnectionAddress, Hostname, LinuxUser, - PostgresSchemaName, ReleaseId, ResourceId, ResourceKind, @@ -3113,6 +3112,15 @@ class MigrateArgs(EngineArgs): "Source URL file for a database, as DATABASE=PATH (repeatable)" ), ) + data_schema: tuple[str, ...] = Flag( + default=(), + description=( + "Source schema to copy into a database given --data, as " + "DATABASE=NAME (repeatable); NAME matches ^[a-z_][a-z0-9_]*$, at " + "most 63 characters, each named once per database; without it " + "every non-system schema of the source is copied" + ), + ) yes: bool = Flag( default=False, confirm=True, @@ -3154,9 +3162,15 @@ def __post_init__(self) -> None: ("data", self.data), ): _pairs(entries, option) + _grouped_pairs(self.data_schema, "data-schema") def options(self, fleet: Fleet) -> MigrateOptions: - """Return what this run builds, deploys and migrates.""" + """Return what this run builds, deploys and migrates. + + A malformed or repeated ``--data-schema`` name raises + ``LifecycleError``, which the command answers with ``PRECONDITION`` + (S-007-12). + """ return MigrateOptions( plan_file=fleet.path(self.plan_file), builds=_pairs(self.build, "build"), @@ -3172,6 +3186,12 @@ def options(self, fleet: Fleet) -> MigrateOptions: database: fleet.path(Path(value)) for database, value in _pairs(self.data, "data").items() }, + data_schemas={ + database: DataCopySchemas.from_boundary(names) + for database, names in _grouped_pairs( + self.data_schema, "data-schema" + ).items() + }, execute=self.yes, restart=self.restart, ) @@ -3205,6 +3225,18 @@ def _pairs(entries: tuple[str, ...], option: str) -> dict[str, str]: return pairs +def _grouped_pairs(entries: tuple[str, ...], option: str) -> dict[str, list[str]]: + """Group repeatable NAME=VALUE pairs by name, keeping every value given.""" + grouped: dict[str, list[str]] = {} + for entry in entries: + key, separator, value = entry.partition("=") + if not separator or not key or not value: + message = f"--{option} expects NAME=VALUE, got {entry!r}" + raise ParseError(message, context={"flag": option}) + grouped.setdefault(key, []).append(value) + return grouped + + @dataclass(frozen=True, slots=True) class Migration: """The migration plan's steps and how far the run got.""" @@ -3244,6 +3276,8 @@ def migrate(args: MigrateArgs, ctx: Ctx, fleet: Fleet) -> Migration: """Run each pending step, saving progress after every one.""" try: result = execute_migration(args.config(fleet, ctx), args.options(fleet)) + except LifecycleError as error: + raise _lifecycle_failed(error) from error except MigrateError as error: raise Exit.PRECONDITION( error.detail, context={"code": error.code} @@ -3843,8 +3877,9 @@ class DataMigrateArgs(EngineArgs): source_schema: tuple[str, ...] = Flag( default=(), description=( - "Source schema to copy (repeatable); without it every non-system " - "schema of the source is copied" + "Source schema to copy (repeatable), matching ^[a-z_][a-z0-9_]*$ " + "and at most 63 characters, each named once; without it every " + "non-system schema of the source is copied" ), ) data_migrations: Path = Flag( @@ -3861,18 +3896,18 @@ class DataMigrateArgs(EngineArgs): ) def __post_init__(self) -> None: - """Keep relative paths inside the project; check the schema names.""" + """Keep relative paths inside the project.""" EngineArgs.__post_init__(self) _inside_project(self.data_migrations, "data-migrations") - self.source_schemas() def source_schemas(self) -> DataCopySchemas: - """Return the named source schemas, refusing a malformed name.""" - try: - names = tuple(PostgresSchemaName(name) for name in self.source_schema) - except ValueError as error: - raise ParseError(str(error), context={"flag": "source-schema"}) from error - return DataCopySchemas(names) + """Return the named source schemas. + + A malformed or repeated name raises ``LifecycleError``, which the + command answers with ``PRECONDITION`` before any playbook (S-007-12), + not a usage error. + """ + return DataCopySchemas.from_boundary(self.source_schema) class DeployPayload(Payload): @@ -4066,8 +4101,8 @@ def data_copy( """Restore the source database into the service, or show the plan.""" context = args.context(fleet, ctx) source = fleet.path(args.source_url_file) - schemas = args.source_schemas() try: + schemas = args.source_schemas() if not args.yes: return DataMigratePayload( _plan( diff --git a/sdk/src/cloudfall/domain.py b/sdk/src/cloudfall/domain.py index 10d30eb..d0209c6 100644 --- a/sdk/src/cloudfall/domain.py +++ b/sdk/src/cloudfall/domain.py @@ -27,6 +27,9 @@ _BUILD_TOOL_NAME_PATTERN = re.compile(r"[A-Za-z0-9][A-Za-z0-9._+-]{0,63}") _BUILD_TOOL_VERSION_PATTERN = re.compile(r"[0-9]{1,9}(?:\.[0-9]{1,9}){0,7}") _CONTROL_CHARACTER_PATTERN = re.compile(r"[\x00-\x1f\x7f]") +# ^[a-z_][a-z0-9_]*$ and at most 63 characters (S-007), read by fullmatch so +# a trailing newline can't pass as it would with $. +_POSTGRES_SCHEMA_NAME_PATTERN = re.compile(r"[a-z_][a-z0-9_]{0,62}") BUILD_STEP_DEFAULT_TIMEOUT_SECONDS = 900 BUILD_STEP_MAXIMUM_TIMEOUT_SECONDS = 3600 ARTIFACT_CLONE_TIMEOUT_SECONDS = 1800 @@ -34,7 +37,6 @@ _FILE_MODE_PATTERN = re.compile(r"^0[0-7]{3}$") _SHA256_PATTERN = re.compile(r"^[a-f0-9]{64}$") _MAX_HOSTNAME_LENGTH = 253 -_POSTGRES_NAME_MAX_BYTES = 63 _MAX_TCP_PORT = 65535 _MIN_HTTP_STATUS = 100 _MAX_HTTP_STATUS = 599 @@ -342,26 +344,22 @@ def from_boundary(cls, value: object) -> LinuxUser: class PostgresSchemaName: """A PostgreSQL schema name a data copy may select. - The name travels to the engine as an Ansible extra var, which Ansible - templates, so braces are refused; PostgreSQL truncates a name past 63 - bytes, and a control character breaks the row-count lines. + Lower-case letters, digits and underscores, not starting with a digit, + at most 63 characters (S-007-12). The name travels to the engine as an + Ansible extra var, which Ansible templates, so no brace may pass; + PostgreSQL truncates a name past 63 bytes, and a quote, a semicolon, or a + control character has no place in a name the dump, the checks, and the + row-count lines all carry. """ value: str def __post_init__(self) -> None: - """Refuse a name PostgreSQL or Ansible would read differently.""" - if not self.value: - message = "schema name must not be empty" - raise ValueError(message) - if len(self.value.encode()) > _POSTGRES_NAME_MAX_BYTES: - message = f"schema name exceeds 63 bytes: {self.value!r}" - raise ValueError(message) - if _CONTROL_CHARACTER_PATTERN.search(self.value) or any( - brace in self.value for brace in "{}" - ): + """Refuse a name outside ``^[a-z_][a-z0-9_]*$`` or past 63 characters.""" + if not _POSTGRES_SCHEMA_NAME_PATTERN.fullmatch(self.value): message = ( - f"schema name holds a control character or a brace: {self.value!r}" + f"schema name {self.value!r} must match ^[a-z_][a-z0-9_]*$ " + "and be at most 63 characters" ) raise ValueError(message) diff --git a/sdk/src/cloudfall/lifecycle.py b/sdk/src/cloudfall/lifecycle.py index 126a5a3..9457c0d 100644 --- a/sdk/src/cloudfall/lifecycle.py +++ b/sdk/src/cloudfall/lifecycle.py @@ -30,6 +30,7 @@ DnsMode, Hostname, IpAddress, + PostgresSchemaName, Sha256Digest, TlsMode, ) @@ -44,12 +45,11 @@ from cloudfall.validation import SchemaCatalog, validate_config if TYPE_CHECKING: - from collections.abc import Callable, Mapping + from collections.abc import Callable, Mapping, Sequence from pathlib import Path from cloudfall.domain import ( CommandArgv, - PostgresSchemaName, ReleaseCommandTimeout, ReleaseId, ResourceId, @@ -81,6 +81,7 @@ _ERROR_SERVICE_MISSING = "lifecycle_service_missing" _ERROR_DATABASE_MISSING = "lifecycle_database_missing" _ERROR_SOURCE_URL_MISSING = "lifecycle_source_url_missing" +ERROR_DATA_COPY_SCHEMA_INVALID = "data_copy_schema_invalid" class LifecycleError(RuntimeError): @@ -269,9 +270,28 @@ class DataCopySchemas: names: tuple[PostgresSchemaName, ...] = () def __post_init__(self) -> None: - """Keep the names sorted so plans and receipts compare verbatim.""" - unique = sorted(set(self.names), key=lambda name: name.value) - object.__setattr__(self, "names", tuple(unique)) + """Refuse a name given twice; keep the names sorted for plans and receipts.""" + repeated = sorted( + {name.value for name in self.names if self.names.count(name) > 1} + ) + if repeated: + message = f"schema named more than once: {', '.join(repeated)}" + raise ValueError(message) + ordered = sorted(self.names, key=lambda name: name.value) + object.__setattr__(self, "names", tuple(ordered)) + + @classmethod + def from_boundary(cls, values: Sequence[str]) -> DataCopySchemas: + """Check the names a flag or a tool call gave, refusing a bad one. + + Raises ``LifecycleError`` with code ``data_copy_schema_invalid`` for + a name outside ``^[a-z_][a-z0-9_]*$``, past 63 characters, or given + twice (S-007-12), before any playbook runs. + """ + try: + return cls(tuple(PostgresSchemaName(value) for value in values)) + except ValueError as error: + raise LifecycleError(ERROR_DATA_COPY_SCHEMA_INVALID, str(error)) from error @property def every_schema(self) -> bool: diff --git a/sdk/src/cloudfall/migrate.py b/sdk/src/cloudfall/migrate.py index cc42e5e..dda44c6 100644 --- a/sdk/src/cloudfall/migrate.py +++ b/sdk/src/cloudfall/migrate.py @@ -28,6 +28,7 @@ from cloudfall.domain import ReleaseId, ResourceId, TcpPort from cloudfall.inventory import PlatformInventory from cloudfall.lifecycle import ( + DataCopySchemas, DeployOptions, LifecycleError, build_release_artifact, @@ -80,6 +81,7 @@ _ERROR_AUDIT_DRIFT = "migrate_audit_drift" _ERROR_ROUTES_UNHEALTHY = "migrate_routes_unhealthy" _ERROR_DATABASE_UNKNOWN = "migrate_database_unknown" +_ERROR_DATA_SCHEMA_WITHOUT_DATA = "migrate_data_schema_without_data" class MigrateError(RuntimeError): @@ -139,6 +141,8 @@ class MigrateOptions: releases: Mapping[str, str] environment_files: Mapping[str, Path] data_migrations: Mapping[str, Path] = field(default_factory=dict) + data_schemas: Mapping[str, DataCopySchemas] = field(default_factory=dict) + """The schemas each ``data:`` step copies; absent copies all.""" execute: bool = False restart: bool = False @@ -235,8 +239,9 @@ def _computed_steps( steps.extend( PlanStep( f"data:{database}", - f"migrate external data into database {database} with " - "row-count verification", + f"migrate {_data_schemas(options, database).describe()} of the " + f"external data into database {database} with row-count " + "verification", ) for database in sorted(options.data_migrations) ) @@ -344,6 +349,19 @@ def _require_known_databases( f"service: {', '.join(unknown)}" ) raise MigrateError(_ERROR_DATABASE_UNKNOWN, detail) + # A schema for a database nothing is copied into would be silently + # ignored; refuse it before any step runs (S-007-12). + unpaired = sorted(set(options.data_schemas) - set(options.data_migrations)) + if unpaired: + detail = ( + "--data-schema names databases with no --data DATABASE=PATH: " + f"{', '.join(unpaired)}" + ) + raise MigrateError(_ERROR_DATA_SCHEMA_WITHOUT_DATA, detail) + + +def _data_schemas(options: MigrateOptions, database: str) -> DataCopySchemas: + return options.data_schemas.get(database, DataCopySchemas()) def _load_plan( @@ -388,6 +406,11 @@ def _load_plan( "with --restart to discard it" ) raise MigrateError(_ERROR_PLAN_STALE, detail) + # A pending step runs with this run's options (a data step's schemas + # among them), so it is described by them, not by the saved plan's text. + for saved, fresh in zip(persisted, computed, strict=True): + if saved.status is StepStatus.PENDING: + saved.description = fresh.description return persisted @@ -511,7 +534,11 @@ def _run() -> dict[str, object]: runners[f"deploy:{component_id}"] = _deploy_runner(component_id) for database, source_url_file in options.data_migrations.items(): runners[f"data:{database}"] = _data_runner( - config, inventory, database, source_url_file + config, + inventory, + database, + source_url_file, + _data_schemas(options, database), ) return runners @@ -521,6 +548,7 @@ def _data_runner( inventory: PlatformInventory, database: str, source_url_file: Path, + schemas: DataCopySchemas, ) -> Runner: def _run() -> dict[str, object]: service = next( @@ -538,6 +566,7 @@ def _run() -> dict[str, object]: database, source_url_file, config.data_migrations_directory, + schemas, ) return _run diff --git a/sdk/tests/test_agent_server.py b/sdk/tests/test_agent_server.py index a3d5afa..9627e71 100644 --- a/sdk/tests/test_agent_server.py +++ b/sdk/tests/test_agent_server.py @@ -277,6 +277,47 @@ def test_s004_16_a_project_serves_domains_apply_and_observe(tmp_path: Path) -> N assert planned["meta"]["dry_run"] is True +def test_s007_12_the_data_tools_refuse_a_bad_schema_as_the_cli_does( + tmp_path: Path, +) -> None: + # Without yes each call is the plan, so no playbook could run either way. + tools, (repeated, malformed, unpaired) = _talk( + _serve("--project", str(EXAMPLES), cwd=tmp_path), + ( + "data_copy", + { + "service": "postgresql-main", + "database": "crm", + "source_url_file": "tmp/source.url", + "source_schema": ["app", "app"], + }, + ), + ( + "data_copy", + { + "service": "postgresql-main", + "database": "crm", + "source_url_file": "tmp/source.url", + "source_schema": ["{{ lookup('pipe', 'id') }}"], + }, + ), + ("migrate", {"build": ["crm-backend=main"], "data_schema": ["crm=public"]}), + ) + + copy_schema = tools["data_copy"].input_schema["properties"]["source_schema"] + assert copy_schema["type"] == "array" + assert copy_schema["items"]["type"] == "string" + assert "data_schema" in tools["migrate"].input_schema["properties"] + for refused, code in ( + (repeated, "data_copy_schema_invalid"), + (malformed, "data_copy_schema_invalid"), + (unpaired, "migrate_data_schema_without_data"), + ): + assert refused["ok"] is False, refused + assert refused["error"]["code"] == "PRECONDITION" + assert refused["error"]["context"]["code"] == code + + def test_a_project_call_cannot_approve_its_own_proposal(tmp_path: Path) -> None: # A person approves a proposal from a shell; an agent that could call # operator approve would run and verify the change it proposed itself. diff --git a/sdk/tests/test_app.py b/sdk/tests/test_app.py index b176734..77ec5ef 100644 --- a/sdk/tests/test_app.py +++ b/sdk/tests/test_app.py @@ -96,6 +96,22 @@ def test_migrate_resolves_env_and_data_files_against_the_project( assert options.data_migrations == {"crm": tmp_path / "source.url"} +def test_s007_13_migrate_groups_each_data_schema_under_its_database( + tmp_path: Path, +) -> None: + fleet = Fleet(EXAMPLES, validate_config(EXAMPLES, SCHEMAS)) + args = MigrateArgs( + data=(f"crm={tmp_path / 'source.url'}",), + data_schema=("crm=public", "crm=app"), + ) + + schemas = args.options(fleet).data_schemas + + assert {database: entry.values for database, entry in schemas.items()} == { + "crm": ["app", "public"] + } + + def test_dashboard_serve_streams_where_it_listens_until_stopped( tmp_path: Path, ) -> None: diff --git a/sdk/tests/test_data_migration.py b/sdk/tests/test_data_migration.py index a017180..a450f5b 100644 --- a/sdk/tests/test_data_migration.py +++ b/sdk/tests/test_data_migration.py @@ -3,8 +3,10 @@ from __future__ import annotations import json +import subprocess +from dataclasses import replace from pathlib import Path -from typing import cast +from typing import TYPE_CHECKING, cast import pytest from cloudfall.agent_tools import AgentConfig @@ -13,12 +15,17 @@ DataCopySchemas, EngineContext, ExecutionPlan, + ExecutionStep, LifecycleError, migrate_data, plan_data_migration, + subprocess_step, ) from cloudfall.migrate import MigrateError, MigrateOptions, execute_migration +if TYPE_CHECKING: + from cloudfall.lifecycle import StepRunner + ROOT = Path(__file__).parents[2] SCHEMAS = ROOT / "config" / "schemas" / "v1" EXAMPLES = ROOT / "config" / "examples" @@ -115,7 +122,22 @@ def test_s007_2_data_migration_plan_without_schemas_is_unchanged( def test_data_copy_schemas_describe_the_selection() -> None: assert DataCopySchemas().describe() == "every non-system schema" assert _schemas("public").describe() == "schema public" - assert _schemas("public", "app", "public").describe() == "schemas app, public" + assert _schemas("public", "app").describe() == "schemas app, public" + + +def test_s007_12_data_copy_schemas_refuse_a_repeat_and_a_malformed_name() -> None: + with pytest.raises(ValueError, match="more than once: app"): + _schemas("app", "public", "app") + for malformed in ("Bad;Name", "Public", "1app", "app\n", "", "a" * 64): + with pytest.raises(ValueError, match=r"\^\[a-z_\]"): + PostgresSchemaName(malformed) + with pytest.raises(LifecycleError) as error: + DataCopySchemas.from_boundary(["app", "app"]) + assert error.value.code == "data_copy_schema_invalid" + assert DataCopySchemas.from_boundary(["public", "_app"]).values == [ + "_app", + "public", + ] def test_data_migration_rejects_an_undeclared_service(tmp_path: Path) -> None: @@ -157,7 +179,9 @@ def test_data_migration_rejects_an_empty_source_url_file( assert error.value.code == "lifecycle_source_url_missing" -def _agent_config(tmp_path: Path) -> AgentConfig: +def _agent_config( + tmp_path: Path, run: StepRunner = subprocess_step +) -> AgentConfig: return AgentConfig( project_directory=EXAMPLES, schema_directory=SCHEMAS, @@ -169,9 +193,125 @@ def _agent_config(tmp_path: Path) -> AgentConfig: releases_directory=tmp_path / "releases", artifacts_directory=tmp_path / "artifacts", data_migrations_directory=tmp_path / "data-migrations", + run=run, ) +def _succeeding(calls: list[ExecutionStep]) -> StepRunner: + """Record each engine step and answer it as a run that passed.""" + + def run(step: ExecutionStep) -> subprocess.CompletedProcess[str]: + calls.append(step) + return subprocess.CompletedProcess(step.argv, 0, "", "") + + return run + + +def test_s007_13_migrate_copies_only_the_data_schema_as_data_copy_does( + tmp_path: Path, +) -> None: + source = _url_file(tmp_path) + calls: list[ExecutionStep] = [] + config = _agent_config(tmp_path, _succeeding(calls)) + options = MigrateOptions( + plan_file=tmp_path / "plan.json", + builds={"crm-backend": "main"}, + releases={}, + environment_files={}, + data_migrations={"crm": source}, + data_schemas={"crm": _schemas("public")}, + execute=True, + ) + planned = execute_migration(config, replace(options, execute=False)) + steps = cast("list[dict[str, object]]", planned["steps"]) + (described,) = [step for step in steps if step["id"] == "data:crm"] + assert "schema public" in str(described["description"]) + # Every other step already ran, so the resumed run is the data step alone. + for step in steps: + if step["id"] != "data:crm": + step["status"] = "completed" + options.plan_file.write_text( + json.dumps({"version": 1, "steps": steps}), encoding="utf-8" + ) + + result = execute_migration(config, options) + + assert result["status"] == "ok", result + (playbook,) = [step for step in calls if "--extra-vars" in step.argv] + copied = plan_data_migration( + _context(tmp_path), + ResourceId("postgresql-main"), + "crm", + source, + config.data_migrations_directory, + _schemas("public"), + ) + assert playbook.argv == copied.steps[-1].argv + assert _extra_vars(copied)["cloudfall_data_schemas"] == ["public"] + + +def test_s007_13_a_resumed_data_step_names_the_schemas_it_copies( + tmp_path: Path, +) -> None: + source = _url_file(tmp_path) + calls: list[ExecutionStep] = [] + config = _agent_config(tmp_path, _succeeding(calls)) + options = MigrateOptions( + plan_file=tmp_path / "plan.json", + builds={"crm-backend": "main"}, + releases={}, + environment_files={}, + data_migrations={"crm": source}, + data_schemas={"crm": _schemas("public")}, + execute=True, + ) + steps = cast( + "list[dict[str, object]]", + execute_migration(config, replace(options, execute=False))["steps"], + ) + for step in steps: + if step["id"] != "data:crm": + step["status"] = "completed" + options.plan_file.write_text( + json.dumps({"version": 1, "steps": steps}), encoding="utf-8" + ) + + # Resumed without --data-schema, the step copies every schema, and the + # plan it reports must say so, not the "schema public" it was saved with. + result = execute_migration(config, replace(options, data_schemas={})) + + (playbook,) = [step for step in calls if "--extra-vars" in step.argv] + extra_vars = json.loads(playbook.argv[playbook.argv.index("--extra-vars") + 1]) + assert "cloudfall_data_schemas" not in extra_vars + (described,) = [ + step + for step in cast("list[dict[str, object]]", result["steps"]) + if step["id"] == "data:crm" + ] + assert "every non-system schema" in str(described["description"]) + + +def test_s007_12_migrate_refuses_a_data_schema_without_its_data( + tmp_path: Path, +) -> None: + calls: list[ExecutionStep] = [] + options = MigrateOptions( + plan_file=tmp_path / "plan.json", + builds={"crm-backend": "main"}, + releases={}, + environment_files={}, + data_schemas={"crm": _schemas("public")}, + execute=True, + ) + + with pytest.raises(MigrateError) as error: + execute_migration(_agent_config(tmp_path, _succeeding(calls)), options) + + assert error.value.code == "migrate_data_schema_without_data" + assert calls == [] + assert not options.plan_file.exists() + + def test_migrate_plan_orders_data_steps_after_services( tmp_path: Path, ) -> None: diff --git a/sdk/tests/test_lifecycle.py b/sdk/tests/test_lifecycle.py index bc1b3c3..90bb209 100644 --- a/sdk/tests/test_lifecycle.py +++ b/sdk/tests/test_lifecycle.py @@ -369,15 +369,63 @@ def test_s007_14_data_copy_plan_names_the_selected_schemas( @pytest.mark.parametrize( - "schema", - ["", "{{ lookup('pipe', 'id') }}", "{% raw %}", "a\nb", "x" * 64], + "schemas", + [ + ["Bad;Name"], + ["app", "app"], + [""], + ["{{ lookup('pipe', 'id') }}"], + ["{% raw %}"], + ["app\t"], + ["Public"], + ["1app"], + ['app"'], + ["x" * 64], + ], ) -def test_data_copy_refuses_a_schema_name_ansible_or_postgres_would_misread( - schema: str, tmp_path: Path, capsys: pytest.CaptureFixture[str] +@pytest.mark.parametrize("confirmed", [False, True]) +def test_s007_12_data_copy_refuses_a_malformed_or_repeated_schema( + schemas: list[str], + confirmed: bool, # noqa: FBT001 - a pytest parameter. + tmp_path: Path, + capsys: pytest.CaptureFixture[str], ) -> None: """A name reaches the engine as an extra var, which Ansible templates.""" source_url_file = tmp_path / "source.url" source_url_file.write_text("postgresql://u:p@db.example.test/crm\n") + flags = [argument for name in schemas for argument in ("--source-schema", name)] + + exit_code, payload = _cli_lifecycle( + [ + "data", + "copy", + "postgresql-main", + "--database", + "crm", + "--source-url-file", + str(source_url_file), + *flags, + *(["--yes"] if confirmed else []), + ], + tmp_path, + capsys, + ) + + assert exit_code == 4 + assert payload["error"]["code"] == "PRECONDITION" + assert payload["error"]["context"]["code"] == "data_copy_schema_invalid" + assert "u:p@" not in json.dumps(payload) + # The inventory render is every playbook run's first step. + assert not (tmp_path / "inventory.json").exists() + + +@pytest.mark.parametrize("schema", ["a\nb", "app\n"]) +def test_data_copy_argv_refuses_a_schema_with_a_newline_before_the_command( + schema: str, tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """The CLI framework refuses a newline in any argv value as a usage error.""" + source_url_file = tmp_path / "source.url" + source_url_file.write_text("postgresql://u:p@db.example.test/crm\n") exit_code, payload = _cli_lifecycle( [ @@ -390,13 +438,44 @@ def test_data_copy_refuses_a_schema_name_ansible_or_postgres_would_misread( str(source_url_file), "--source-schema", schema, + "--yes", ], tmp_path, capsys, ) assert exit_code == 2 - assert payload["ok"] is False + assert payload["error"]["code"] == "ARG_ERROR" + assert not (tmp_path / "inventory.json").exists() + + +def test_s007_12_data_copy_takes_the_longest_schema_name( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + source_url_file = tmp_path / "source.url" + source_url_file.write_text("postgresql://u:p@db.example.test/crm\n") + longest = "_" + "a1" * 31 + + exit_code, payload = _cli_lifecycle( + [ + "data", + "copy", + "postgresql-main", + "--database", + "crm", + "--source-url-file", + str(source_url_file), + "--source-schema", + longest, + ], + tmp_path, + capsys, + ) + + assert exit_code == 0, payload + assert f"dump schema {longest} of the database" in str( + payload["data"]["would_run"] + ) def _git(*arguments: str) -> None: diff --git a/sdk/tests/test_migrate.py b/sdk/tests/test_migrate.py index 79bb811..354d08b 100644 --- a/sdk/tests/test_migrate.py +++ b/sdk/tests/test_migrate.py @@ -266,6 +266,65 @@ def test_cli_migrate_rejects_malformed_component_pairs( ) +@pytest.mark.parametrize( + ("flags", "code"), + [ + (["--data-schema", "crm=public"], "migrate_data_schema_without_data"), + ( + ["--data", "crm=SOURCE", "--data-schema", "crm=Bad;Name"], + "data_copy_schema_invalid", + ), + ( + [ + "--data", + "crm=SOURCE", + "--data-schema", + "crm=app", + "--data-schema", + "crm=app", + ], + "data_copy_schema_invalid", + ), + ], +) +def test_s007_12_cli_migrate_refuses_a_bad_data_schema_before_any_playbook( + flags: list[str], + code: str, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + source = tmp_path / "source.url" + source.write_text("postgresql://u:p@db.example.test/crm\n", encoding="utf-8") + + exit_code = main( + [ + "migrate", + "--project", + str(EXAMPLES), + "--schemas", + str(SCHEMAS), + "--engine", + str(ENGINE), + "--plan-file", + str(tmp_path / "plan.json"), + "--inventory-file", + str(tmp_path / "inventory.json"), + "--build", + "crm-backend=main", + *(flag.replace("SOURCE", str(source)) for flag in flags), + "--yes", + ] + ) + + payload = json.loads(capsys.readouterr().out) + assert exit_code == 4 + assert payload["error"]["code"] == "PRECONDITION" + assert payload["error"]["context"]["code"] == code + assert "u:p@" not in json.dumps(payload) + assert not (tmp_path / "plan.json").exists() + assert not (tmp_path / "inventory.json").exists() + + # dns-verify's proxy addresses, pinned as they were before domains apply.