From 46b63729341e48319c0ec82d0b4d3cfea80370c6 Mon Sep 17 00:00:00 2001 From: MigraDiff Agent Date: Sun, 9 Aug 2026 07:41:05 -0700 Subject: [PATCH] fix: comma-separated --schema/--exclude_schema silently matched nothing --schema public,reporting was documented as supported but the raw comma-joined string was passed straight through to schemainspect's filter_schema(), which compares it for exact equality against each object's schema name -- "public,reporting" never equals "public" or "reporting", so the diff was always empty with no error. Single-schema and no-schema calls were never affected. Adds parse_schema_arg()/filter_inspector_schemas() in migra/util.py (the latter reuses schemainspect's own PROPS list rather than a hardcoded copy, so it can't drift out of sync) and _get_inspector() in migra/migra.py, wired into all 5 of Migration's inspector-construction call sites. Single/no-schema calls pass straight through to the original get_inspector() unchanged; only 2+ comma-separated names take the new post-filter path. New tests each include a third, unlisted schema in the fixture data to prove real filtering rather than "happens to work with one schema", and exercise both the CLI path and Migration.apply() directly. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 25 ++++++++++ migra/migra.py | 31 ++++++++++-- migra/util.py | 49 +++++++++++++++++++ status.md | 46 ++++++++--------- tests/FIXTURES/exclude_multischema/a.sql | 13 +++++ .../exclude_multischema/additions.sql | 0 tests/FIXTURES/exclude_multischema/b.sql | 17 +++++++ .../FIXTURES/exclude_multischema/expected.sql | 7 +++ .../exclude_multischema/expected2.sql | 0 tests/FIXTURES/multischema/a.sql | 13 +++++ tests/FIXTURES/multischema/additions.sql | 0 tests/FIXTURES/multischema/b.sql | 17 +++++++ tests/FIXTURES/multischema/expected.sql | 7 +++ tests/FIXTURES/multischema/expected2.sql | 0 tests/test_migra.py | 16 ++++++ 15 files changed, 214 insertions(+), 27 deletions(-) create mode 100644 tests/FIXTURES/exclude_multischema/a.sql create mode 100644 tests/FIXTURES/exclude_multischema/additions.sql create mode 100644 tests/FIXTURES/exclude_multischema/b.sql create mode 100644 tests/FIXTURES/exclude_multischema/expected.sql create mode 100644 tests/FIXTURES/exclude_multischema/expected2.sql create mode 100644 tests/FIXTURES/multischema/a.sql create mode 100644 tests/FIXTURES/multischema/additions.sql create mode 100644 tests/FIXTURES/multischema/b.sql create mode 100644 tests/FIXTURES/multischema/expected.sql create mode 100644 tests/FIXTURES/multischema/expected2.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index c3f6e3d..266101b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,31 @@ - New tests: `test_command_apply.py` (11 tests, real Postgres, including a direct atomicity test that forces a mid-migration failure) +### Fixed + +- **Comma-separated `--schema`/`--exclude_schema` silently matched nothing**: + `--schema public,reporting` was documented as supported but the raw + string was passed straight to `schemainspect`, which compares it for + *exact* equality against each object's schema name — `"public,reporting"` + never equals `"public"` or `"reporting"`, so the diff was always empty + and nothing was reported to the user. Single-schema (`--schema public`) + and no-schema calls were never affected — this only broke 2+ names. + - Fix: `migra/util.py` gains `parse_schema_arg()` (splits and trims the + comma-separated value) and `filter_inspector_schemas()` (post-filters + an inspector's tracked object collections using schemainspect's own + `PROPS` list, so it stays in sync with whatever object types + schemainspect adds in the future). + - `migra/migra.py` gains `_get_inspector()`, used at all 5 of + `Migration`'s inspector-construction call sites (`__init__` x2, + `inspect_from()`, `inspect_target()`, `apply()`). Single-schema/no-schema + calls are passed straight through unchanged (zero behavior change); + only 2+ comma-separated names take the new post-filter path. + - New tests: `test_multischema`, `test_multischema_whitespace_tolerant`, + `test_exclude_multischema` in `test_migra.py`, plus new fixtures under + `tests/FIXTURES/multischema/` and `tests/FIXTURES/exclude_multischema/` + — each includes a third, unlisted schema to prove filtering actually + excludes it, not just that it happens to work with one schema. + ### Notes - This is the foundational layer for the upcoming "Control Plane" feature set. diff --git a/migra/migra.py b/migra/migra.py index f7a5932..1150692 100644 --- a/migra/migra.py +++ b/migra/migra.py @@ -4,6 +4,27 @@ from .changes import Changes from .statements import Statements +from .util import filter_inspector_schemas, parse_schema_arg + + +def _get_inspector(x, schema=None, exclude_schema=None): + """Like schemainspect's get_inspector(), but also supports a + comma-separated `schema`/`exclude_schema` (2+ names). schemainspect's + own one_schema()/filter_schema() only ever compare against a single + schema name, so a raw multi-name string would silently match nothing. + Single-schema (or no-schema) calls are passed straight through + unchanged. + """ + schema_list = parse_schema_arg(schema) + exclude_list = parse_schema_arg(exclude_schema) + + if schema_list and len(schema_list) > 1: + return filter_inspector_schemas(get_inspector(x), schema_list) + + if exclude_list and len(exclude_list) > 1: + return filter_inspector_schemas(get_inspector(x), None, exclude=exclude_list) + + return get_inspector(x, schema=schema, exclude_schema=exclude_schema) class Migration(object): @@ -28,7 +49,7 @@ def __init__( if isinstance(x_from, DBInspector): self.changes.i_from = x_from else: - self.changes.i_from = get_inspector( + self.changes.i_from = _get_inspector( x_from, schema=schema, exclude_schema=exclude_schema ) if x_from: @@ -36,7 +57,7 @@ def __init__( if isinstance(x_target, DBInspector): self.changes.i_target = x_target else: - self.changes.i_target = get_inspector( + self.changes.i_target = _get_inspector( x_target, schema=schema, exclude_schema=exclude_schema ) if x_target: @@ -45,12 +66,12 @@ def __init__( self.changes.ignore_extension_versions = ignore_extension_versions def inspect_from(self): - self.changes.i_from = get_inspector( + self.changes.i_from = _get_inspector( self.s_from, schema=self.schema, exclude_schema=self.exclude_schema ) def inspect_target(self): - self.changes.i_target = get_inspector( + self.changes.i_target = _get_inspector( self.s_target, schema=self.schema, exclude_schema=self.exclude_schema ) @@ -62,7 +83,7 @@ def apply(self): for stmt in self.statements: raw_execute(self.s_from, stmt) - self.changes.i_from = get_inspector( + self.changes.i_from = _get_inspector( self.s_from, schema=self.schema, exclude_schema=self.exclude_schema ) safety_on = self.statements.safe diff --git a/migra/util.py b/migra/util.py index 4d0c788..952199c 100644 --- a/migra/util.py +++ b/migra/util.py @@ -3,6 +3,55 @@ from collections import OrderedDict as od +def parse_schema_arg(schema): + """Split a comma-separated --schema/--exclude_schema value into a list + of trimmed, non-empty schema names. Returns None for a falsy input.""" + if not schema: + return None + parts = [s.strip() for s in schema.split(",")] + return [p for p in parts if p] + + +def filter_inspector_schemas(inspector, schemas=None, exclude=None): + """Filter an already-built (unfiltered) inspector down to only the given + schemas, or to exclude the given schemas. Needed for the multi-schema + case (2+ comma-separated names), since schemainspect's own + `filter_schema`/`one_schema` only accepts a single schema name and + compares it for exact equality -- passing it a raw "a,b" string silently + matches nothing. + + Mutates and returns `inspector`. Reuses schemainspect's own PROPS list + (the same one `DBInspector.filter_schema` iterates over) so this stays + in sync with whatever object types schemainspect tracks, rather than + maintaining a second, driftable copy of that list here. + """ + if not schemas and not exclude: + return inspector + if not hasattr(inspector, "filter_schema"): + return inspector + + from schemainspect.pg.obj import PROPS + + schema_set = set(schemas) if schemas else None + exclude_set = set(exclude) if exclude else None + for prop in PROPS.split(): + att = getattr(inspector, prop, {}) + if schema_set: + filtered = { + k: v + for k, v in att.items() + if hasattr(v, "schema") and v.schema in schema_set + } + else: + filtered = { + k: v + for k, v in att.items() + if hasattr(v, "schema") and v.schema not in exclude_set + } + setattr(inspector, prop, filtered) + return inspector + + def differences(a, b, add_dependencies_for_modifications=True): a_keys = set(a.keys()) b_keys = set(b.keys()) diff --git a/status.md b/status.md index 506f1ff..664da83 100644 --- a/status.md +++ b/status.md @@ -3,28 +3,30 @@ Snapshot of current work-in-progress on this branch. Update as work progresses; this is not a changelog (see CHANGELOG.md for that). -## Current branch: `feature/apply-flag` (off `main` at `46c2447`) +## Current branch: `fix/multi-schema-filtering` (off `main` at `a762dec`) -Migration state tracking (`--status`/`--history`/`--record-history`/`--promote`/ -`--record-rollback`) and the CI-trigger/SQLAlchemy-2.x fixes are already merged to `main` -(PRs #8, #9, #10) — see git log, not this file, for that history. +Migration state tracking, the CI-trigger/SQLAlchemy-2.x fixes, and `--apply` are already merged +to `main` (PRs #8–#11) — see git log, not this file, for that history. **In progress / uncommitted:** -- Modified: `CHANGELOG.md`, `README.md`, `migra/command.py`, `CLAUDE.md`, - `PROJECT_PLAN.md`, `PROJECT_PLAN2.md` — new `--apply` flag -- New: `tests/test_command_apply.py` - -**What `--apply` does:** executes the generated migration against `dburl_from` in a single -transaction instead of only printing it; on success, automatically records it in `dburl_from`'s -`migradiff_history` table (no need to also pass `--record-history`); on failure, rolls back -everything and records nothing, exits 4. Rejected up front when combined with `--from-file` or -`--promote` — see CLAUDE.md's "Migration state tracking" section for the from/target direction -reasoning. - -**Verified 2026-08-08:** 11 new tests pass (including a direct atomicity test against -`_apply_migration()` that forces a mid-migration failure); full suite is 353 passed / 2 skipped, -no regressions; flake8 and black clean; also verified against SQLAlchemy 2.0.51 in an isolated -venv (no other raw-SQL issues found). - -**Next steps:** commit, push, open PR. Known follow-up (not done here): reconcile `--promote`'s -from/to direction with `--apply` before wiring the two together (see PROJECT_PLAN.md backlog). +- Modified: `CHANGELOG.md`, `migra/migra.py`, `migra/util.py`, `tests/test_migra.py` +- New: `tests/FIXTURES/multischema/`, `tests/FIXTURES/exclude_multischema/` + +**The bug:** `--schema public,reporting` was documented as supported but silently produced an +empty diff — the raw comma-joined string was passed straight to `schemainspect`, which compares +it for *exact* equality against each object's schema name. Single-schema and no-schema calls were +never affected. + +**The fix:** `migra/util.py` gains `parse_schema_arg()`/`filter_inspector_schemas()` (the latter +reuses schemainspect's own `PROPS` list rather than duplicating it, so it stays in sync with +whatever object types schemainspect tracks). `migra/migra.py` gains `_get_inspector()`, wired into +all 5 of `Migration`'s inspector-construction call sites; single/no-schema calls pass straight +through unchanged, only 2+ comma-separated names take the new post-filter path. + +**Verified 2026-08-08:** new tests (`test_multischema`, `test_multischema_whitespace_tolerant`, +`test_exclude_multischema`) each include a third, unlisted schema in the fixture to prove +filtering actually excludes it, not just that it happens to work with one schema; also exercises +`Migration.apply()` directly (via `do_fixture_test`'s second half), not just the CLI path. Full +suite: 356 passed / 2 skipped, no regressions. flake8/black clean. + +**Next steps:** commit, push, open PR. diff --git a/tests/FIXTURES/exclude_multischema/a.sql b/tests/FIXTURES/exclude_multischema/a.sql new file mode 100644 index 0000000..072e678 --- /dev/null +++ b/tests/FIXTURES/exclude_multischema/a.sql @@ -0,0 +1,13 @@ +create schema schema1; + +create table schema1.t(id uuid, value text); + +create schema schema2; + +create table schema2.x(id uuid, value text); + +create schema schema3; + +create table schema3.untouched(id uuid); + +create table public.other(id uuid, value text); diff --git a/tests/FIXTURES/exclude_multischema/additions.sql b/tests/FIXTURES/exclude_multischema/additions.sql new file mode 100644 index 0000000..e69de29 diff --git a/tests/FIXTURES/exclude_multischema/b.sql b/tests/FIXTURES/exclude_multischema/b.sql new file mode 100644 index 0000000..fd7f2bb --- /dev/null +++ b/tests/FIXTURES/exclude_multischema/b.sql @@ -0,0 +1,17 @@ +create schema schema1; + +create table schema1.t(id uuid, value text, name text); + +create schema schema2; + +create table schema2.x(id uuid, value text); + +create table schema2.y(id uuid, value text); + +create schema schema3; + +create table schema3.untouched(id uuid); + +create table schema3.should_be_ignored(id uuid); + +create table public.other(id uuid); diff --git a/tests/FIXTURES/exclude_multischema/expected.sql b/tests/FIXTURES/exclude_multischema/expected.sql new file mode 100644 index 0000000..492ab16 --- /dev/null +++ b/tests/FIXTURES/exclude_multischema/expected.sql @@ -0,0 +1,7 @@ +create table "schema2"."y" ( + "id" uuid, + "value" text +); + + +alter table "schema1"."t" add column "name" text; diff --git a/tests/FIXTURES/exclude_multischema/expected2.sql b/tests/FIXTURES/exclude_multischema/expected2.sql new file mode 100644 index 0000000..e69de29 diff --git a/tests/FIXTURES/multischema/a.sql b/tests/FIXTURES/multischema/a.sql new file mode 100644 index 0000000..072e678 --- /dev/null +++ b/tests/FIXTURES/multischema/a.sql @@ -0,0 +1,13 @@ +create schema schema1; + +create table schema1.t(id uuid, value text); + +create schema schema2; + +create table schema2.x(id uuid, value text); + +create schema schema3; + +create table schema3.untouched(id uuid); + +create table public.other(id uuid, value text); diff --git a/tests/FIXTURES/multischema/additions.sql b/tests/FIXTURES/multischema/additions.sql new file mode 100644 index 0000000..e69de29 diff --git a/tests/FIXTURES/multischema/b.sql b/tests/FIXTURES/multischema/b.sql new file mode 100644 index 0000000..fd7f2bb --- /dev/null +++ b/tests/FIXTURES/multischema/b.sql @@ -0,0 +1,17 @@ +create schema schema1; + +create table schema1.t(id uuid, value text, name text); + +create schema schema2; + +create table schema2.x(id uuid, value text); + +create table schema2.y(id uuid, value text); + +create schema schema3; + +create table schema3.untouched(id uuid); + +create table schema3.should_be_ignored(id uuid); + +create table public.other(id uuid); diff --git a/tests/FIXTURES/multischema/expected.sql b/tests/FIXTURES/multischema/expected.sql new file mode 100644 index 0000000..492ab16 --- /dev/null +++ b/tests/FIXTURES/multischema/expected.sql @@ -0,0 +1,7 @@ +create table "schema2"."y" ( + "id" uuid, + "value" text +); + + +alter table "schema1"."t" add column "name" text; diff --git a/tests/FIXTURES/multischema/expected2.sql b/tests/FIXTURES/multischema/expected2.sql new file mode 100644 index 0000000..e69de29 diff --git a/tests/test_migra.py b/tests/test_migra.py index c61cd84..ebf502e 100644 --- a/tests/test_migra.py +++ b/tests/test_migra.py @@ -60,11 +60,27 @@ def test_singleschema(): do_fixture_test(FIXTURE_NAME, schema="goodschema") +def test_multischema(): + for FIXTURE_NAME in ["multischema"]: + do_fixture_test(FIXTURE_NAME, schema="schema1,schema2") + + +def test_multischema_whitespace_tolerant(): + # comma-separated schema names should tolerate surrounding whitespace + for FIXTURE_NAME in ["multischema"]: + do_fixture_test(FIXTURE_NAME, schema=" schema1 , schema2 ") + + def test_excludeschema(): for FIXTURE_NAME in ["excludeschema"]: do_fixture_test(FIXTURE_NAME, exclude_schema="excludedschema") +def test_exclude_multischema(): + for FIXTURE_NAME in ["exclude_multischema"]: + do_fixture_test(FIXTURE_NAME, exclude_schema="schema3,public") + + def test_singleschema_ext(): for FIXTURE_NAME in ["singleschema_ext"]: do_fixture_test(FIXTURE_NAME, create_extensions_only=True)