From ec70f3b49363cb33e563d5e118b26502a9ebb0d2 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 4 Nov 2026 16:02:55 +0100 Subject: [PATCH 1/4] fix(db,audit_log): bulk audit prefetch keeps statement options; ORM restores and PK changes are audited (#810, #811) Claude-Session: https://claude.ai/code/session_01FXHA3ZicrtZ4C5QPEgxfgq --- CHANGELOG.md | 4 + docs/modules/audit_log.md | 13 ++- .../simple_module_db/_bulk_audit_records.py | 11 -- framework/db/simple_module_db/audit.py | 22 ++-- framework/db/simple_module_db/bulk_audit.py | 13 ++- .../db/tests/test_db_audit_regressions.py | 104 ++++++++++++++++++ modules/audit_log/audit_log/constants.py | 2 + modules/audit_log/audit_log/locales/de.json | 1 + modules/audit_log/audit_log/locales/en.json | 1 + .../pages/components/ChangesList.tsx | 1 + .../pages/components/EntriesTable.tsx | 1 + .../audit_log/pages/components/FilterBar.tsx | 1 + .../audit_log/tests-js/ChangesList.test.tsx | 6 + packages/i18n/src/generated-resources.ts | 1 + packages/i18n/src/keys.generated.ts | 1 + 15 files changed, 148 insertions(+), 34 deletions(-) create mode 100644 framework/db/tests/test_db_audit_regressions.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d6ced9f0..f462833b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,10 @@ All notable changes to this project are documented in this file. The format is b `account` or whose `azp` is another client (only `oidc` pins both). ### Fixed +- Bulk-DML audit prefetch preserves statement options, including loader criteria, + so it records only the rows selected by the UPDATE or DELETE (#810). +- ORM restores now emit `restored` audit entries with translated action labels; + ORM primary-key updates include the old and new key in their diffs (#811). - Tenant scoping in `simple_module_db` now applies to more statement shapes: ORM bulk UPDATE by primary key (`session.execute(update(Model), rows)`), `update().ordered_values()` (assigning `tenant_id` raises like `.values()`), diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index 6e0b0d85a..fd90697e5 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -1,6 +1,6 @@ # audit_log -Automatic field-level audit trail for every SQLModel entity in the app. Each create / update / delete / soft-delete is captured in `audit_log_audit_entry` with the changed fields, the user who made the change, and the request correlation id — **no instrumentation required in module code**. Entities opt out, they don't opt in. +Automatic field-level audit trail for every SQLModel entity in the app. Each create / update / delete / soft-delete / restore is captured in `audit_log_audit_entry` with the changed fields, the user who made the change, and the request correlation id — **no instrumentation required in module code**. Entities opt out, they don't opt in. ## ModuleMeta @@ -37,7 +37,7 @@ audit_log = "audit_log.capture:audit_callback" `session.execute(update(Model)...)`, its `delete()` / `insert()` siblings and Core DML on `Model.__table__` never reach a flush, so a `do_orm_execute` listener (`simple_module_db.bulk_audit`) audits them instead, through the same per-engine `audit_callback` (requests and Celery workers alike): -- **`UPDATE` / `DELETE`.** Before the statement runs, the listener reads the primary keys it will touch, with the same `WHERE` and the same tenant scope (soft-deleted rows included, as the write includes them). Up to 500 rows each get one entry (`bulk_updated`, `bulk_deleted`; an update that writes `is_deleted` is `bulk_soft_deleted` when the value is truthy or SQL such as `sa.true()`, else `bulk_restored`, decided per row for executemany; the statement's other written columns stay in `changes`); `changes` holds the `SET` values as `{field, new}` (a SQL expression is stored as SQL with bind placeholders). `session.execute(update(Model), [{"id": ..., ...}, ...])` takes the ids from the rows instead. +- **`UPDATE` / `DELETE`.** Before the statement runs, the listener reads the primary keys it will touch, with the same `WHERE`, statement options (including `with_loader_criteria`) and tenant scope (soft-deleted rows included, as the write includes them). Up to 500 rows each get one entry (`bulk_updated`, `bulk_deleted`; an update that writes `is_deleted` is `bulk_soft_deleted` when the value is truthy or SQL such as `sa.true()`, else `bulk_restored`, decided per row for executemany; the statement's other written columns stay in `changes`); `changes` holds the `SET` values as `{field, new}` (a SQL expression is stored as SQL with bind placeholders). `session.execute(update(Model), [{"id": ..., ...}, ...])` takes the ids from the rows instead. - **More than 500 rows.** One summary entry with `entity_id` `"*"`: the `SET` values plus `where_sql` (the `WHERE` with bind placeholders, never the literal values), `rowcount` and `truncated: true`. - **`INSERT`.** One `bulk_inserted` summary entry per statement (`entity_id` `"*"`, the column values, or the column names for a multi-row insert). - **Only real targets count.** `changes` comes from the statement's `SET` / `VALUES` targets, each resolved to its model attribute (bound values read through the execute params). An execute-param key that names a column (and is not one of the statement's bind parameters) is written by SQLAlchemy, so it is a target too, for a single dict and for every executemany row; any other param key is ignored. Primary-key and `is_deleted` writes are never treated as excluded bookkeeping. @@ -64,11 +64,12 @@ record_bulk_audit(session, Order, "bulk_updated", ids, {"status": "archived"}) | `created` | object in `session.new` | | `updated` | object in `session.dirty` with real column changes (empty diffs are skipped) | | `deleted` | object in `session.deleted` (hard delete) | -| `soft_deleted` | a `SoftDeleteMixin` row the soft-delete listener moved into `session.new` with `is_deleted=True` | +| `soft_deleted` | a persistent `SoftDeleteMixin` row whose `is_deleted` changes to `True` (including a delete intercepted by the soft-delete listener) | +| `restored` | a persistent `SoftDeleteMixin` row whose `is_deleted` changes from `True` to `False` | | `bulk_updated` / `bulk_deleted` / `bulk_soft_deleted` / `bulk_restored` | statement `UPDATE` / `DELETE` (see above), one per row or one `"*"` summary | | `bulk_inserted` | statement `INSERT`, one `"*"` summary per statement | -`changes` is a list of `{field, new}` (creates) or `{field, old, new}` (updates) entries; deletes and soft-deletes carry an empty change list. Column values are normalised to JSON-safe scalars (non-primitives are stringified). +`changes` is a list of `{field, new}` (creates) or `{field, old, new}` (updates) entries; hard deletes carry an empty change list. Soft-deletes and restores retain any other field edits made in the same flush, or carry an empty list when only the lifecycle flag changed. Column values are normalised to JSON-safe scalars (non-primitives are stringified). ## Opting out @@ -99,7 +100,7 @@ class Cache(Base, table=True): An update whose changed columns are *all* excluded writes no audit row at all, so excluding a model's volatile columns is how to audit its meaningful transitions without one row per heartbeat. `background_tasks` does both: `OutboxMessage` (delivery bookkeeping) sets `__audit_exclude__ = True`, and `TaskExecution` excludes `heartbeat_at`, `queued_at`, `started_at`, `finished_at` and `retries`, so its creation and status changes are audited while the writes in between are not. This applies to request-path and Celery-worker writes alike. -Primary-key columns and the bookkeeping fields injected by `AuditMixin` (`created_at`, `updated_at`, `created_by`, `updated_by`) and `SoftDeleteMixin` (`is_deleted`, `deleted_at`, `deleted_by`) are excluded from diffs automatically. +Primary-key columns are excluded on create; ORM updates include their old and new values, and `entity_id` resolves to the new key after flush (#811). The bookkeeping fields injected by `AuditMixin` (`created_at`, `updated_at`, `created_by`, `updated_by`) and `SoftDeleteMixin` (`is_deleted`, `deleted_at`, `deleted_by`) are excluded from diffs automatically. ## Routes @@ -146,7 +147,7 @@ from audit_log.contracts.schemas import AuditEntryRead, AuditEntryList | `id` | `UUID` | PK | | `entity_type` | `str(255)` | indexed; the model class name | | `entity_id` | `str(255)` | indexed; the resolved PK as a string | -| `action` | `str(20)` | `created` / `updated` / `deleted` / `soft_deleted` | +| `action` | `str(20)` | `created` / `updated` / `deleted` / `soft_deleted` / `restored`, plus the statement-DML actions above | | `changes` | JSON | list of field diffs | | `user_id` | `str(255) \| None` | indexed; from the request's `current_user_id` | | `correlation_id` | `str(255) \| None` | request correlation id | diff --git a/framework/db/simple_module_db/_bulk_audit_records.py b/framework/db/simple_module_db/_bulk_audit_records.py index 7c142378f..b4286f7aa 100644 --- a/framework/db/simple_module_db/_bulk_audit_records.py +++ b/framework/db/simple_module_db/_bulk_audit_records.py @@ -9,8 +9,6 @@ from simple_module_core.correlation import correlation_id from sqlalchemy import inspect as sa_inspect from sqlalchemy.orm import ORMExecuteState, Session -from sqlalchemy.sql.elements import BindParameter -from sqlalchemy.sql.visitors import iterate from simple_module_db.audit import AuditRecord, _serialize, format_pk from simple_module_db.query_filter import engine_policy @@ -61,15 +59,6 @@ def _current_user() -> str | None: return current_user_id.get() -def _where_params(stmt: Any, params: Any) -> dict[str, Any] | None: - """The execute params the ``WHERE`` clause binds, so the prefetch can run it.""" - where = getattr(stmt, "whereclause", None) - if where is None or not isinstance(params, dict): - return None - keys = {e.key for e in iterate(where) if isinstance(e, BindParameter)} - return {k: v for k, v in params.items() if k in keys} or None - - def _bulk_by_pk_ids(execute_state: ORMExecuteState, cls: type) -> list[str] | None: """``session.execute(update(Model), [{pk..., col...}, ...])``: ids from the rows.""" params = execute_state.parameters diff --git a/framework/db/simple_module_db/audit.py b/framework/db/simple_module_db/audit.py index ba0498d93..a52eaa10b 100644 --- a/framework/db/simple_module_db/audit.py +++ b/framework/db/simple_module_db/audit.py @@ -61,7 +61,7 @@ class _PendingChange: obj_ref: object entity_type: str - action: str # "created" | "updated" | "deleted" | "soft_deleted" + action: str # "created" | "updated" | "deleted" | "soft_deleted" | "restored" changes: list[dict[str, Any]] user_id: str | None correlation_id: str | None @@ -116,13 +116,11 @@ def _serialize(value: Any) -> Any: return str(value) -def _column_diffs( - obj: object, inspector: Any, excl: frozenset[str], pk_cols: set[str] -) -> list[dict[str, Any]]: - """Old/new diffs for the changed, non-excluded, non-PK columns of *obj*.""" +def _column_diffs(obj: object, inspector: Any, excl: frozenset[str]) -> list[dict[str, Any]]: + """Old/new diffs for the changed, non-excluded columns of *obj*.""" changes: list[dict[str, Any]] = [] for col_name in _column_names(obj): - if col_name in excl or col_name in pk_cols: + if col_name in excl: continue hist = inspector.attrs[col_name].history if not hist.has_changes(): @@ -197,19 +195,15 @@ def _get_pk_cols(obj: object) -> set[str]: if _is_excluded(obj): continue inspector = sa_inspect(obj) - changes = _column_diffs(obj, inspector, _get_excluded(obj), _get_pk_cols(obj)) + changes = _column_diffs(obj, inspector, _get_excluded(obj)) # The soft-delete listener re-adds persistent objects as dirty. Edits made in - # the same flush stay on the soft_deleted record. - if ( - isinstance(obj, SoftDeleteMixin) - and obj.is_deleted - and inspector.attrs.is_deleted.history.has_changes() - ): + # the same flush stay on the lifecycle record, including a restore. + if isinstance(obj, SoftDeleteMixin) and inspector.attrs.is_deleted.history.has_changes(): pending.append( _PendingChange( obj_ref=obj, entity_type=type(obj).__name__, - action="soft_deleted", + action="soft_deleted" if obj.is_deleted else "restored", changes=changes, user_id=user_id, correlation_id=correlation_id, diff --git a/framework/db/simple_module_db/bulk_audit.py b/framework/db/simple_module_db/bulk_audit.py index 84d957b51..1b86e11c2 100644 --- a/framework/db/simple_module_db/bulk_audit.py +++ b/framework/db/simple_module_db/bulk_audit.py @@ -46,7 +46,6 @@ _current_user, _deliver, _summary, - _where_params, record_bulk_audit, ) from simple_module_db.audit import _excluded_fields, format_pk @@ -115,6 +114,14 @@ def _plan(execute_state: ORMExecuteState, cls: type) -> _Ctx | None: return ctx +def _single_params(params: Any) -> dict[str, Any] | None: + """A single parameter dict (the SELECT ignores keys it does not bind), else ``None``. + + An executemany list cannot be run as one SELECT, so it prefetches unparameterized. + """ + return params if isinstance(params, dict) and params else None + + def _prefetch_ids(execute_state: ORMExecuteState, cls: type, cap: int) -> list[str]: """Primary keys of the rows the statement will touch, at most ``cap + 1``.""" stmt = execute_state.statement @@ -122,14 +129,14 @@ def _prefetch_ids(execute_state: ORMExecuteState, cls: type, cap: int) -> list[s cols = [getattr(cls, col.key) for col in sa_inspect(cls).primary_key] else: cols = list(stmt.table.primary_key.columns) - query = select(*cols).limit(cap + 1) + query = select(*cols).options(*stmt._with_options).limit(cap + 1) if stmt.whereclause is not None: query = query.where(stmt.whereclause) options = {"include_deleted": True} if execute_state.execution_options.get(ALL_TENANTS_OPTION): options[ALL_TENANTS_OPTION] = True rows = execute_state.session.execute( - query.execution_options(**options), _where_params(stmt, execute_state.parameters) + query.execution_options(**options), _single_params(execute_state.parameters) ).all() return [format_pk(tuple(row)) for row in rows] diff --git a/framework/db/tests/test_db_audit_regressions.py b/framework/db/tests/test_db_audit_regressions.py new file mode 100644 index 000000000..90282861f --- /dev/null +++ b/framework/db/tests/test_db_audit_regressions.py @@ -0,0 +1,104 @@ +"""Audit targets and ORM lifecycle regressions (#810, #811).""" + +import pytest +from _audit_models import AuditTestItem, SoftDeleteItem +from simple_module_core.correlation import correlation_id +from simple_module_db.listeners import current_user_id +from sqlalchemy import bindparam, delete, select, update +from sqlalchemy.orm import with_loader_criteria + + +@pytest.mark.parametrize("operation", [update, delete], ids=["update", "delete"]) +@pytest.mark.parametrize("value", [2, 99], ids=["one-match", "no-match"]) +async def test_bulk_audit_prefetch_preserves_loader_criteria(audited, operation, value): + _state, session, recorder = audited + before = dict((await session.execute(select(AuditTestItem.id, AuditTestItem.name))).all()) + expected = set( + (await session.execute(select(AuditTestItem.id).where(AuditTestItem.value == value))) + .scalars() + .all() + ) + stmt = operation(AuditTestItem).options( + with_loader_criteria(AuditTestItem, AuditTestItem.value == value) + ) + if operation is update: + stmt = stmt.values(name="changed") + result = await session.execute(stmt) + after = dict((await session.execute(select(AuditTestItem.id, AuditTestItem.name))).all()) + changed = {pk for pk, name in before.items() if after.get(pk) != name} + assert changed == expected + assert result.rowcount == len(expected) + assert {r.entity_id for r in recorder.records} == {str(pk) for pk in changed} + assert len(recorder.records) == len(changed) + assert all( + r.action == ("bulk_updated" if operation is update else "bulk_deleted") + for r in recorder.records + ) + + +@pytest.mark.parametrize("operation", [update, delete], ids=["update", "delete"]) +async def test_bulk_audit_loader_criteria_keeps_execute_parameters(audited, operation): + _state, session, recorder = audited + target = ( + await session.execute(select(AuditTestItem.id).where(AuditTestItem.value == 2)) + ).scalar_one() + stmt = ( + operation(AuditTestItem) + .where(AuditTestItem.value >= 1) + .options( + with_loader_criteria(AuditTestItem, AuditTestItem.value == bindparam("wanted_value")) + ) + ) + if operation is update: + stmt = stmt.values(name="changed") + result = await session.execute(stmt, {"wanted_value": 2}) + assert result.rowcount == 1 + assert [r.entity_id for r in recorder.records] == [str(target)] + + +@pytest.mark.parametrize("edit_title", [False, True]) +async def test_orm_restore_records_lifecycle_and_concurrent_edits(audited, edit_title): + _state, session, recorder = audited + item = SoftDeleteItem(title="archived", is_deleted=True) + session.add(item) + await session.flush() + recorder.records.clear() + item.is_deleted = False + if edit_title: + item.title = "restored title" + user_token = current_user_id.set("admin") + correlation_token = correlation_id.set("restore-request") + try: + await session.flush() + finally: + current_user_id.reset(user_token) + correlation_id.reset(correlation_token) + [record] = recorder.records + assert record.action == "restored" + assert record.entity_type == "SoftDeleteItem" + assert record.entity_id == str(item.id) + assert record.user_id == "admin" + assert record.correlation_id == "restore-request" + assert record.changes == ( + [{"field": "title", "old": "archived", "new": "restored title"}] if edit_title else [] + ) + recorder.records.clear() + item.title = "later edit" + await session.flush() + assert [r.action for r in recorder.records] == ["updated"] + + +async def test_orm_primary_key_update_records_old_and_new_key(audited): + _state, session, recorder = audited + item = ( + await session.execute(select(AuditTestItem).where(AuditTestItem.value == 1)) + ).scalar_one() + old_id = item.id + item.id = 999 + await session.flush() + [record] = recorder.records + assert record.action == "updated" + assert record.entity_id == "999" + assert record.changes == [{"field": "id", "old": old_id, "new": 999}] + assert await session.get(AuditTestItem, old_id) is None + assert await session.get(AuditTestItem, 999) is item diff --git a/modules/audit_log/audit_log/constants.py b/modules/audit_log/audit_log/constants.py index e13229868..806bb8727 100644 --- a/modules/audit_log/audit_log/constants.py +++ b/modules/audit_log/audit_log/constants.py @@ -28,6 +28,7 @@ ACTION_UPDATED: Final = "updated" ACTION_DELETED: Final = "deleted" ACTION_SOFT_DELETED: Final = "soft_deleted" +ACTION_RESTORED: Final = "restored" # Statement DML (#557); the names match ``simple_module_db.bulk_audit``. ACTION_BULK_INSERTED: Final = "bulk_inserted" ACTION_BULK_UPDATED: Final = "bulk_updated" @@ -39,6 +40,7 @@ ACTION_UPDATED, ACTION_DELETED, ACTION_SOFT_DELETED, + ACTION_RESTORED, ACTION_BULK_INSERTED, ACTION_BULK_UPDATED, ACTION_BULK_DELETED, diff --git a/modules/audit_log/audit_log/locales/de.json b/modules/audit_log/audit_log/locales/de.json index c421f93ad..9d08f320c 100644 --- a/modules/audit_log/audit_log/locales/de.json +++ b/modules/audit_log/audit_log/locales/de.json @@ -38,6 +38,7 @@ "updated": "aktualisiert", "deleted": "gelöscht", "soft_deleted": "archiviert", + "restored": "wiederhergestellt", "bulk_inserted": "massenweise erstellt", "bulk_updated": "massenweise geändert", "bulk_deleted": "massenweise gelöscht", diff --git a/modules/audit_log/audit_log/locales/en.json b/modules/audit_log/audit_log/locales/en.json index 7a5878aba..4a03b3c3c 100644 --- a/modules/audit_log/audit_log/locales/en.json +++ b/modules/audit_log/audit_log/locales/en.json @@ -38,6 +38,7 @@ "updated": "updated", "deleted": "deleted", "soft_deleted": "archived", + "restored": "restored", "bulk_inserted": "bulk created", "bulk_updated": "bulk updated", "bulk_deleted": "bulk deleted", diff --git a/modules/audit_log/audit_log/pages/components/ChangesList.tsx b/modules/audit_log/audit_log/pages/components/ChangesList.tsx index 5d83c4339..cd31c1930 100644 --- a/modules/audit_log/audit_log/pages/components/ChangesList.tsx +++ b/modules/audit_log/audit_log/pages/components/ChangesList.tsx @@ -20,6 +20,7 @@ const VISIBLE_LIMIT = 2; const NO_DIFF_ACTIONS = new Set([ 'deleted', 'soft_deleted', + 'restored', 'bulk_deleted', 'bulk_soft_deleted', 'bulk_restored', diff --git a/modules/audit_log/audit_log/pages/components/EntriesTable.tsx b/modules/audit_log/audit_log/pages/components/EntriesTable.tsx index d86af243a..99cf27c4e 100644 --- a/modules/audit_log/audit_log/pages/components/EntriesTable.tsx +++ b/modules/audit_log/audit_log/pages/components/EntriesTable.tsx @@ -42,6 +42,7 @@ const ACTION_PILL: Record = { updated: 'bg-blue-50 text-blue-700', deleted: 'bg-red-50 text-red-700', soft_deleted: 'bg-amber-50 text-amber-700', + restored: 'bg-blue-50 text-blue-700', bulk_inserted: 'bg-primary-600/10 text-primary-700', bulk_updated: 'bg-blue-50 text-blue-700', bulk_deleted: 'bg-red-50 text-red-700', diff --git a/modules/audit_log/audit_log/pages/components/FilterBar.tsx b/modules/audit_log/audit_log/pages/components/FilterBar.tsx index 7fa3b6527..20fc22bc9 100644 --- a/modules/audit_log/audit_log/pages/components/FilterBar.tsx +++ b/modules/audit_log/audit_log/pages/components/FilterBar.tsx @@ -20,6 +20,7 @@ export const ACTIONS = [ 'updated', 'deleted', 'soft_deleted', + 'restored', 'bulk_inserted', 'bulk_updated', 'bulk_deleted', diff --git a/modules/audit_log/tests-js/ChangesList.test.tsx b/modules/audit_log/tests-js/ChangesList.test.tsx index 18d57844c..d3f3fadb8 100644 --- a/modules/audit_log/tests-js/ChangesList.test.tsx +++ b/modules/audit_log/tests-js/ChangesList.test.tsx @@ -36,6 +36,12 @@ describe('ChangesList', () => { expect(screen.getByText('no changes recorded')).toBeInTheDocument(); }); + test('a restore without field edits explains the empty diff', () => { + render(); + + expect(screen.getByText('no changes recorded')).toBeInTheDocument(); + }); + test('a create counts the fields instead of listing them', () => { render(); diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 5018b7802..acb7c807f 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -86,6 +86,7 @@ export default { 'audit_log.actions.bulk_updated': '', 'audit_log.actions.created': '', 'audit_log.actions.deleted': '', + 'audit_log.actions.restored': '', 'audit_log.actions.soft_deleted': '', 'audit_log.actions.updated': '', 'audit_log.browse.clear_filters': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index da0de60d4..1d4543c38 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -119,6 +119,7 @@ export const keys = { bulk_updated: 'audit_log.actions.bulk_updated', created: 'audit_log.actions.created', deleted: 'audit_log.actions.deleted', + restored: 'audit_log.actions.restored', soft_deleted: 'audit_log.actions.soft_deleted', updated: 'audit_log.actions.updated', }, From d1a272b3b9be9556a02115aa841393e1150f68fe Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 4 Nov 2026 16:04:42 +0100 Subject: [PATCH 2/4] refactor(optimize): type audit action pill map by the shared ACTIONS list Claude-Session: https://claude.ai/code/session_01FXHA3ZicrtZ4C5QPEgxfgq --- modules/audit_log/audit_log/pages/components/EntriesTable.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/audit_log/audit_log/pages/components/EntriesTable.tsx b/modules/audit_log/audit_log/pages/components/EntriesTable.tsx index 99cf27c4e..4200afe4c 100644 --- a/modules/audit_log/audit_log/pages/components/EntriesTable.tsx +++ b/modules/audit_log/audit_log/pages/components/EntriesTable.tsx @@ -37,7 +37,7 @@ export interface AuditEntryRead { // Borderless tints, lowercase values: the pill is a value in a dense table, // not a badge competing with the row's links for attention. -const ACTION_PILL: Record = { +const ACTION_PILL: Record<(typeof ACTIONS)[number], string> = { created: 'bg-primary-600/10 text-primary-700', updated: 'bg-blue-50 text-blue-700', deleted: 'bg-red-50 text-red-700', From 5de2ed1611f77a8da79c963873bc3a1711d8a494 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 4 Nov 2026 16:09:35 +0100 Subject: [PATCH 3/4] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01FXHA3ZicrtZ4C5QPEgxfgq --- CHANGELOG.md | 5 +++-- docs/modules/audit_log.md | 2 +- framework/db/simple_module_db/bulk_audit.py | 8 ++++++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f462833b3..37c064c15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,8 +55,9 @@ All notable changes to this project are documented in this file. The format is b ### Fixed - Bulk-DML audit prefetch preserves statement options, including loader criteria, so it records only the rows selected by the UPDATE or DELETE (#810). -- ORM restores now emit `restored` audit entries with translated action labels; - ORM primary-key updates include the old and new key in their diffs (#811). +- ORM restores now emit `restored` audit entries with translated action labels + and ORM primary-key updates include the old and new key in their diffs + (behaviour change: both used to leave no entry) (#811). - Tenant scoping in `simple_module_db` now applies to more statement shapes: ORM bulk UPDATE by primary key (`session.execute(update(Model), rows)`), `update().ordered_values()` (assigning `tenant_id` raises like `.values()`), diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index fd90697e5..79314a927 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -69,7 +69,7 @@ record_bulk_audit(session, Order, "bulk_updated", ids, {"status": "archived"}) | `bulk_updated` / `bulk_deleted` / `bulk_soft_deleted` / `bulk_restored` | statement `UPDATE` / `DELETE` (see above), one per row or one `"*"` summary | | `bulk_inserted` | statement `INSERT`, one `"*"` summary per statement | -`changes` is a list of `{field, new}` (creates) or `{field, old, new}` (updates) entries; hard deletes carry an empty change list. Soft-deletes and restores retain any other field edits made in the same flush, or carry an empty list when only the lifecycle flag changed. Column values are normalised to JSON-safe scalars (non-primitives are stringified). +`changes` is a list of `{field, new}` (creates) or `{field, old, new}` (updates) entries; hard deletes carry an empty change list. Soft-deletes and restores retain any other field edits made in the same flush, or carry an empty list when only the lifecycle flag changed (a `VersionedMixin` model also lists its bumped `version`). Column values are normalised to JSON-safe scalars (non-primitives are stringified). ## Opting out diff --git a/framework/db/simple_module_db/bulk_audit.py b/framework/db/simple_module_db/bulk_audit.py index 1b86e11c2..2ca98e188 100644 --- a/framework/db/simple_module_db/bulk_audit.py +++ b/framework/db/simple_module_db/bulk_audit.py @@ -30,6 +30,7 @@ from sqlalchemy import inspect as sa_inspect from sqlalchemy import select from sqlalchemy.orm import ORMExecuteState +from sqlalchemy.orm.util import LoaderCriteriaOption from simple_module_db import model_registry as registry from simple_module_db._bulk_audit_records import ( # noqa: F401 (re-exported) @@ -117,7 +118,7 @@ def _plan(execute_state: ORMExecuteState, cls: type) -> _Ctx | None: def _single_params(params: Any) -> dict[str, Any] | None: """A single parameter dict (the SELECT ignores keys it does not bind), else ``None``. - An executemany list cannot be run as one SELECT, so it prefetches unparameterized. + The prefetch never runs for an executemany list, so only a dict reaches here. """ return params if isinstance(params, dict) and params else None @@ -129,7 +130,10 @@ def _prefetch_ids(execute_state: ORMExecuteState, cls: type, cap: int) -> list[s cols = [getattr(cls, col.key) for col in sa_inspect(cls).primary_key] else: cols = list(stmt.table.primary_key.columns) - query = select(*cols).options(*stmt._with_options).limit(cap + 1) + # Loader criteria narrow the rows the DML touches; other options (load + # strategies) mean nothing on a PK-only SELECT and can refuse it. + criteria = [o for o in stmt._with_options if isinstance(o, LoaderCriteriaOption)] + query = select(*cols).options(*criteria).limit(cap + 1) if stmt.whereclause is not None: query = query.where(stmt.whereclause) options = {"include_deleted": True} From 315609d029aba88871a7fc1d2e32082564d575a3 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 4 Nov 2026 16:15:01 +0100 Subject: [PATCH 4/4] fix: address code review findings (round 1, pass 2) Claude-Session: https://claude.ai/code/session_01FXHA3ZicrtZ4C5QPEgxfgq --- framework/db/simple_module_db/bulk_audit.py | 27 +++++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/framework/db/simple_module_db/bulk_audit.py b/framework/db/simple_module_db/bulk_audit.py index 2ca98e188..d23955661 100644 --- a/framework/db/simple_module_db/bulk_audit.py +++ b/framework/db/simple_module_db/bulk_audit.py @@ -31,6 +31,8 @@ from sqlalchemy import select from sqlalchemy.orm import ORMExecuteState from sqlalchemy.orm.util import LoaderCriteriaOption +from sqlalchemy.sql.elements import BindParameter +from sqlalchemy.sql.visitors import iterate from simple_module_db import model_registry as registry from simple_module_db._bulk_audit_records import ( # noqa: F401 (re-exported) @@ -115,12 +117,22 @@ def _plan(execute_state: ORMExecuteState, cls: type) -> _Ctx | None: return ctx -def _single_params(params: Any) -> dict[str, Any] | None: - """A single parameter dict (the SELECT ignores keys it does not bind), else ``None``. +def _bound_params(params: Any, *clauses: Any) -> dict[str, Any] | None: + """The execute params the prefetch's own clauses bind (``WHERE`` and loader criteria). - The prefetch never runs for an executemany list, so only a dict reaches here. + Only those keys pass, so a caller's param cannot collide with a bind the SELECT + adds itself (its ``LIMIT``). An executemany list never reaches the prefetch. """ - return params if isinstance(params, dict) and params else None + if not isinstance(params, dict) or not params: + return None + keys = { + el.key + for clause in clauses + if clause is not None + for el in iterate(clause) + if isinstance(el, BindParameter) + } + return {k: v for k, v in params.items() if k in keys} or None def _prefetch_ids(execute_state: ORMExecuteState, cls: type, cap: int) -> list[str]: @@ -140,7 +152,12 @@ def _prefetch_ids(execute_state: ORMExecuteState, cls: type, cap: int) -> list[s if execute_state.execution_options.get(ALL_TENANTS_OPTION): options[ALL_TENANTS_OPTION] = True rows = execute_state.session.execute( - query.execution_options(**options), _single_params(execute_state.parameters) + query.execution_options(**options), + _bound_params( + execute_state.parameters, + stmt.whereclause, + *(c.where_criteria for c in criteria), + ), ).all() return [format_pk(tuple(row)) for row in rows]