seam: Auditable.audit_entity_id / before_flush hook broken for server-generated (identity) PKs #57

New issue

Open

opened 2026-09-11 10:51:46 +00:00 by patrick · 0 comments

patrick commented

2026-09-11 10:51:46 +00:00

Owner

Discovered while building PH1.1 (#9, feature/db-schema-master-data). backend/src/polaris/db/audit.py is outside PH1.1's owned paths (backend/alembic/versions/**, backend/src/polaris/db/models/**), but the bug blocks every future Auditable mapped class that uses the project's own PK convention (docs/08-data-model.md: "Primary keys id bigint generated always as identity"), so I fixed it directly in the PH1.1 PR rather than widen the ticket or leave PH1.1 broken — flagging it here per the seam-request process in CLAUDE.md.

Bug: Auditable.audit_entity_id's default implementation read inspect(self).identity, and the hook fired on before_flush. Two problems, both real:

before_flush runs before the flush's INSERT executes, so a server-generated PK (the standard case for every master-data table) genuinely has no value yet — raises ValueError: audit_entity_id: instance has no identity (not yet flushed).
Even switching the hook to after_flush wasn't enough on its own: inspect(obj).identity is still None at that point (SQLAlchemy doesn't register the identity key until later in the flush's finalize step, confirmed empirically against 2.0.52) even though the plain attribute (obj.id) is already populated by then.

It never showed up in 0001 because Setting's primary key is a client-supplied string (key), and its audit_entity_id override reads self.key directly rather than going through inspect().identity — it never exercises either bug. task opts out of Auditable entirely. So this was latent until a mapped class with a server-generated int PK actually mixed in Auditable and got inserted, which is exactly what every PH1.1 master-data table does.

Fix (already merged in the PH1.1 PR, backend/src/polaris/db/audit.py):

Hook moved from before_flush to after_flush (still same transaction — new AuditLog rows added here just join the next autoflush/commit rather than this exact flush call, so a rollback still rolls them back too).
Auditable.audit_entity_id now reads the primary-key column's attribute directly (getattr(self, mapper.primary_key[0].key)) instead of inspect(self).identity, since the attribute is populated by after_flush even though the identity key isn't yet.

All existing tests/integration/db/test_audit_hook.py cases (insert/update/delete/actor-context/task-opt-out) still pass unchanged. New coverage: tests/integration/db/test_master_data_models.py exercises insert of several Auditable classes with identity PKs.

No action needed unless another session already has local changes to db/audit.py on a different branch — rebase onto develop after this merges. Opened for traceability, not as a blocker (already resolved).

Discovered while building PH1.1 (#9, `feature/db-schema-master-data`). `backend/src/polaris/db/audit.py` is outside PH1.1's owned paths (`backend/alembic/versions/**`, `backend/src/polaris/db/models/**`), but the bug blocks every future `Auditable` mapped class that uses the project's own PK convention (docs/08-data-model.md: "Primary keys `id bigint generated always as identity`"), so I fixed it directly in the PH1.1 PR rather than widen the ticket or leave PH1.1 broken — flagging it here per the seam-request process in CLAUDE.md. **Bug**: `Auditable.audit_entity_id`'s default implementation read `inspect(self).identity`, and the hook fired on `before_flush`. Two problems, both real: 1. `before_flush` runs *before* the flush's `INSERT` executes, so a server-generated PK (the standard case for every master-data table) genuinely has no value yet — raises `ValueError: audit_entity_id: instance has no identity (not yet flushed)`. 2. Even switching the hook to `after_flush` wasn't enough on its own: `inspect(obj).identity` is *still* `None` at that point (SQLAlchemy doesn't register the identity key until later in the flush's finalize step, confirmed empirically against 2.0.52) even though the plain attribute (`obj.id`) is already populated by then. It never showed up in `0001` because `Setting`'s primary key is a client-supplied string (`key`), and its `audit_entity_id` override reads `self.key` directly rather than going through `inspect().identity` — it never exercises either bug. `task` opts out of `Auditable` entirely. So this was latent until a mapped class with a server-generated int PK actually mixed in `Auditable` and got inserted, which is exactly what every PH1.1 master-data table does. **Fix** (already merged in the PH1.1 PR, `backend/src/polaris/db/audit.py`): - Hook moved from `before_flush` to `after_flush` (still same transaction — new `AuditLog` rows added here just join the next autoflush/commit rather than this exact flush call, so a rollback still rolls them back too). - `Auditable.audit_entity_id` now reads the primary-key column's *attribute* directly (`getattr(self, mapper.primary_key[0].key)`) instead of `inspect(self).identity`, since the attribute is populated by `after_flush` even though the identity key isn't yet. All existing `tests/integration/db/test_audit_hook.py` cases (insert/update/delete/actor-context/task-opt-out) still pass unchanged. New coverage: `tests/integration/db/test_master_data_models.py` exercises insert of several `Auditable` classes with identity PKs. No action needed unless another session already has local changes to `db/audit.py` on a different branch — rebase onto `develop` after this merges. Opened for traceability, not as a blocker (already resolved).

patrick referenced this issue from a commit

2026-09-11 10:52:36 +00:00

fix(db): make Auditable audit hook work with identity PKs

patrick referenced this issue from a commit

2026-09-11 10:53:45 +00:00

fix(db): make Auditable audit hook work with identity PKs

patrick referenced this issue

2026-09-11 10:53:56 +00:00

feat(db): schema 0002 master data tables #58

patrick referenced this issue

2026-09-11 10:54:08 +00:00

PH1.1 — Schema 0002: master data tables and typed mapped classes #9