refactor(versioning): sqlalchemy-review follow-ups (W1–W8)
Cleanup pass from the SQLAlchemy + migration code review. Eight items,
all in the "warnings / suggestions" tier — no behaviour change visible
to the API, but each closes a real correctness, perf, or maintainability
concern surfaced in review.
baseline.py
- Delete unused ``_get_user_id`` (W1). The function wrapped a broad
``except Exception: # noqa: S110`` swallow that hid bugs; grep
confirmed no callers anywhere. The legitimate audit-field paths
(``row.get("changed_by_fk")`` etc.) already drive the
``version_transaction.user_id`` write.
- Batch ``_baseline_attached_slices`` from O(N) round-trips to
three queries (W2): one membership SELECT, one existing-shadow
SELECT, one bulk live-row SELECT for the missing ids. The previous
per-slice ``COUNT(*)`` + ``SELECT`` was a measurable first-save
hotspot on dashboards with many charts. Drops the now-unused
``_slice_has_shadow`` helper.
- Pick a stable column name for ``flag_modified`` in
``_force_parent_dirty_on_child_change`` (W3). ``uuid`` is on all
three versioned parent classes and excluded by none, so the
flagged attribute is deterministic across SQLAlchemy versions /
mapper-config orders instead of depending on
``versioned_column_properties(parent)[0]``. Falls back to the
first available column for forks that exclude ``uuid``.
changes.py
- Add ``Decimal`` handling to ``_jsonable`` (W4) — ``json.dumps``
rejects ``Decimal``, so any numeric column (e.g. ``SqlMetric.currency``
contents, or fork/plugin Decimal columns) would crash the bulk
insert. Stringify rather than ``float()`` to preserve precision;
the diff engine compares ``from_value`` / ``to_value`` by string
equality after this coercion so both sides round-trip identically.
queries.py
- Promote the inline ``{0: "baseline", 1: "update", 2: "delete"}``
dict to module-level ``_OP_TYPE_LABELS`` (W7). The literal was
duplicated across ``list_versions`` and ``get_version``; the third
caller is one bug fix away.
- Comment on ``resolve_version_uuid``'s Python-side ``derive_version_uuid``
loop (W8) — no portable SQL form for UUIDv5 across PostgreSQL /
MySQL / SQLite, iteration count is bounded by the retention
window. Flags the place to revisit if retention is ever disabled
(``=0``) on a heavily-edited entity.
migrations/2026-05-01_23-36 (composite-PK)
- Belt-and-braces guard in ``_downgrade_mysql_table`` (W6): asserts
``t.name in AFFECTED_TABLES`` before interpolating into the
backtick-quoted ALTER statements. The invariant was already
structurally implied (callers iterate ``AFFECTED_TABLES``), but
making it load-bearing means a future refactor can't slip an
arbitrary table name through.
(W5 was verified-no-change: grepped ``tests/`` for ``metadata.create_all``
callers that exercise versioning tables; none. The cascade-FK
gap on ``version_changes.transaction_id`` is already documented
in ``tests/integration_tests/versioning/change_records_tests.py:27-32``.)
62 versioning unit tests pass. M
Mike Bridge committed
40653d52da774349aa34761027884eda409bc65e
Parent: 59045f8