Skip to content

Wire vocab/results schema roles onto CDM models and remove hand-threaded db_schema - #57

Open
nicoloesch wants to merge 33 commits into
mainfrom
fix/schema-translate-map-vocab-role-wiring
Open

nicoloesch wants to merge 33 commits into
mainfrom
fix/schema-translate-map-vocab-role-wiring

Conversation

@nicoloesch

@nicoloesch nicoloesch commented Sep 1, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Depends on AustralianCancerDataNetwork/orm-loader#39

Requires pyproject.toml version update for oa-configurator and orm-loader

What changed

  • Vocabulary and derived/results tables now declare their Role.VOCAB/Role.RESULTS schema tag
    • schema_translate_map's vocab/results keys actually take effect instead of every table silently resolving into the primary schema.
  • backends/ no longer threads a db_schema parameter through 18+ methods by hand
    • each derives it from the connection's own schema_translate_map.
  • Deletes the cli_vocab.py connect-listener/search_path hack it was compensating for.
  • create_missing_tables() now refuses to create tables under a schema that's silently drifted from what was last recorded, via the schema-provenance-drift protection added in oa-configurator
  • reconcile_schema() is now genuinely role-aware
  • _ConnContext trimmed to what it can't already derive from its own resolved field
  • Its own SupportedDialect enum is gone in favour of the shared Dialect enum from oa-configurator instead, with no change in dispatch behaviour.
  • pg_session's test isolation now resets every non-system schema before and after each test, so a test's own committed DDL/DML in a reserved bookkeeping schema like MAINTENANCE_SCHEMA no longer depends on some later, unrelated test to clean it up.

Schema-drift protection now covers every maintenance command

  • install_fulltext_columns, manage_indexes, the standalone indexes cluster command, truncate_tables, and vocabulary-table creation (cli_vocab.py) are now all wrapped in guard_schema_provenance_for
  • approximate_row_counts() deleted (dead code, zero callers, superseded by oa-configurator's own preview_orphan_schema_tables).
  • Adopts oa-configurator's physical_schema_of/schema_tag naming throughout.
  • New test_schema_provenance_guard.py: 5 live-Postgres regression tests, one per guarded call site

Testing

Non-default-schema Postgres coverage added throughout, since this whole class of bug is invisible against the default schema.

Checklist

  • Applied exactly one label (breaking, feature, fix, dependencies, or chore)
  • Tests pass locally (uv run pytest -q): 370 passed
  • Tests pass locally (uv run pytest -q -m postgresql): 38 passed
  • Lint passes (uv run ruff check .)

Nico Loesch added 2 commits September 1, 2026 04:55
…mas to tables and cleanup tables not adhering to previously defined standards, adapt CLI to new standard
@nicoloesch nicoloesch added the fix Bug fix, backwards-compatible. PATCH: x.y.z+1 label Sep 1, 2026
Nico Loesch added 6 commits September 3, 2026 04:05
…es, delegate some functionality to the backends, add the option to reconcile a schema and rectify it if there are stray tables
…per reset and clean-up of test databases, drop the default schema on the primary DB as no longer required
@nicoloesch
nicoloesch marked this pull request as ready for review September 24, 2026 08:06
@gkennos
gkennos self-requested a review September 24, 2026 14:26

Parameters
----------
vocab_engine : sqlalchemy.Engine, optional

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Route vocabulary CSV loading through vocab_engine, not primary.

Please route inspection, staging, the load session, retries, index/FK handling and sequence reset through vocab_engine end-to-end, and add a live split-engine test asserting rows land only on the vocab target.

guard no-ops. A role whose connection is test_only=true also
no-ops, at the guard's own discretion.
"""
vocab_engine = vocab_engine if vocab_engine is not None else engine

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure_schema is now correctly split-aware (good), but collect_missing_tables and the existing-table inspector loop still call sa.inspect(engine) only

Please centralise a schema-tag-to-bind lookup and inspect each table on its actual engine before building the plan --> carry the same lookup into reconciliation.

return Path.cwd() / f"omop-alchemy-backup-{timestamp}{FORMAT_SUFFIXES[backup_format]}"


def create_database_backup(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create_database_backup accepts and records db_schema in its result metadata but never forwards it to backend.prepare_backup, which defaults to the primary role.

restart_identities=restart_identities,
cascade=cascade,
)
# One TRUNCATE batch per schema_tag, since truncate_table_batch qualifies its whole list with a single tag.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truncate cross-role foreign keys in one statement per physical connection, not per schema tag. truncate_tables groups the selection by schema_tag and issues one TRUNCATE per group

PostgreSQL's RESTRICT mode requires referencing and referenced tables to be named in the same statement.

Group by physical connection instead of schema tag, and keep the external-blocker check and the "no silent CASCADE" behaviour as-is.

return tuple(signature)


def _actual_index_signature(actual_index: ReflectedIndex, backend: Backend) -> tuple[str, ...]:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_actual_index_signature only advances the expressions iterator when a column name is None, but PostgreSQL's reflection returns a fully positional expressions list covering every index position.

Please align columns and expressions by shared position.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug fix, backwards-compatible. PATCH: x.y.z+1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants