feat(versioning): cross-entity version activity view - #41076
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
466687e to
84ef36f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41076 +/- ##
==========================================
+ Coverage 65.05% 65.12% +0.07%
==========================================
Files 2747 2756 +9
Lines 153766 154583 +817
Branches 35250 35395 +145
==========================================
+ Hits 100027 100675 +648
- Misses 51829 51952 +123
- Partials 1910 1956 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ce8a3f6 to
df3100c
Compare
de238f8 to
b3ad697
Compare
b0a166e to
6c585cd
Compare
6c585cd to
84f3d8e
Compare
134cf0e to
c5414b2
Compare
Apply code-review fixes to the cross-entity activity view (apache#41076): - Redact tombstoned related-entity metadata (entity_name, summary, changed_by) so a hard-delete can't widen what a requester entitled only to the path entity can see. - Bound the self window at the entity's first tracked (id, uuid) transaction so a reused integer id can't inherit a hard-deleted predecessor's history. - Make the change-record fetch ceiling per-kind (<= n_kinds * 5000) and collapse truncation to a clean time cut instead of per-kind holes. - Guard Window construction against degenerate shadow rows (log-and-skip instead of 500). - Bound the name-denormalization and impact-count queries by the page-set's transaction range. - Correct the dataset include=related docs: it returns an empty stream. - Drop the dead SC-003 restore perf test (restore endpoint ships later). - Skip the redundant second path-entity resolve; rename load_shadow_model -> load_live_model; use Window.merges_with in union_windows; fix the summary schema doc and a time-specific comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Versioning base-infra — verified review notes (head
|
Apply code-review fixes to the cross-entity activity view (apache#41076): - Redact tombstoned related-entity metadata (entity_name, summary, changed_by) so a hard-delete can't widen what a requester entitled only to the path entity can see. - Bound the self window at the entity's first tracked (id, uuid) transaction so a reused integer id can't inherit a hard-deleted predecessor's history. - Make the change-record fetch ceiling per-kind (<= n_kinds * 5000) and collapse truncation to a clean time cut instead of per-kind holes. - Guard Window construction against degenerate shadow rows (log-and-skip instead of 500). - Bound the name-denormalization and impact-count queries by the page-set's transaction range. - Correct the dataset include=related docs: it returns an empty stream. - Drop the dead SC-003 restore perf test (restore endpoint ships later). - Skip the redundant second path-entity resolve; rename load_shadow_model -> load_live_model; use Window.merges_with in union_windows; fix the summary schema doc and a time-specific comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bf31b23 to
e3f960a
Compare
|
🤖 This review was generated via Codex using parallel Preset, Python, React, and Superset committer review passes. Thanks for the substantial work on the cross-entity activity view. The implementation and test coverage are extensive, but I found two correctness issues that should be resolved before merging. Blocking findings1. The per-kind cap can omit newer activity across ID chunks In An earlier chunk can consume the 5,000-row cap even when a later chunk contains newer activity. The resulting stream can therefore return older records while silently omitting newer ones. Could we instead fetch a bounded candidate set from every chunk, merge the candidates using the complete ordering key, and retain the newest global A regression test with more than 500 related entities, where a later chunk contains the newest record, would help lock this down. 2. Related history is not bounded to the current entity incarnation In If the database reuses an ID, the newly created dashboard or chart could display activity belonging to its predecessor. Could we propagate the lower transaction boundary for the current entity into related-scope resolution and clip the relationship windows accordingly? Matching shadow records against the current UUID where available would provide additional protection. Minor documentation points
Overall, the architecture looks thoughtfully developed, but the chunk-ordering and entity-incarnation issues can affect the correctness of the activity stream and should be addressed before merge. |
Apply code-review fixes to the cross-entity activity view (apache#41076): - Redact tombstoned related-entity metadata (entity_name, summary, changed_by) so a hard-delete can't widen what a requester entitled only to the path entity can see. - Bound the self window at the entity's first tracked (id, uuid) transaction so a reused integer id can't inherit a hard-deleted predecessor's history. - Make the change-record fetch ceiling per-kind (<= n_kinds * 5000) and collapse truncation to a clean time cut instead of per-kind holes. - Guard Window construction against degenerate shadow rows (log-and-skip instead of 500). - Bound the name-denormalization and impact-count queries by the page-set's transaction range. - Correct the dataset include=related docs: it returns an empty stream. - Drop the dead SC-003 restore perf test (restore endpoint ships later). - Skip the redundant second path-entity resolve; rename load_shadow_model -> load_live_model; use Window.merges_with in union_windows; fix the summary schema doc and a time-specific comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Apply code-review fixes to the cross-entity activity view (apache#41076): - Redact tombstoned related-entity metadata (entity_name, summary, changed_by) so a hard-delete can't widen what a requester entitled only to the path entity can see. - Bound the self window at the entity's first tracked (id, uuid) transaction so a reused integer id can't inherit a hard-deleted predecessor's history. - Make the change-record fetch ceiling per-kind (<= n_kinds * 5000) and collapse truncation to a clean time cut instead of per-kind holes. - Guard Window construction against degenerate shadow rows (log-and-skip instead of 500). - Bound the name-denormalization and impact-count queries by the page-set's transaction range. - Correct the dataset include=related docs: it returns an empty stream. - Drop the dead SC-003 restore perf test (restore endpoint ships later). - Skip the redundant second path-entity resolve; rename load_shadow_model -> load_live_model; use Window.merges_with in union_windows; fix the summary schema doc and a time-specific comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mikebridge
left a comment
There was a problem hiding this comment.
Review generated by Codex.
The activity stream can disclose historical data under integer-ID reuse and can omit valid records when its fetch ceiling interacts with broad windows or ID chunking. Its OpenAPI response contract is also incomplete and inconsistent with tombstone responses.
P1 — Treat reused related IDs as tombstones
superset/versioning/activity/visibility.py:201-205
When a related chart or dataset is hard-deleted and its integer ID is later reused, this checks historical records against the replacement live row because identity is keyed only by (kind, id). A Gamma principal who can read the path dashboard and replacement dataset can then receive the old dataset's unredacted name, editor, path, and values despite not being granted that dataset, violating the SECURITY.md Gamma row (only granted datasets). Please compare the historical shadow UUID with the live UUID and treat mismatches as tombstones.
P2 — Apply the fetch limit after exact window filtering
superset/versioning/activity/queries.py:460-463
When one entity has many recent changes outside its own association window, the kind-wide transaction bounds still admit those rows, allowing them to consume the 5,000-row SQL limit before the Python window filter discards them. For example, a heavily edited chart after detachment can crowd out valid older activity from attached charts, yielding a silently incomplete stream. Please apply the limit across exact per-entity windows or continue fetching after discarded rows.
P2 — Merge chunk results before enforcing the per-kind limit
superset/versioning/activity/queries.py:445-448
When a kind spans more than 500 entity IDs, chunks are queried sequentially and the first chunks consume the shared limit without comparison to later chunks. If an early chunk supplies 5,000 rows, newer records from every later chunk are omitted, and the reported truncation floor does not make the returned interval complete. Please fetch per-chunk candidates and globally merge/order them before applying the limit.
P2 — Register the activity response schema
superset/charts/api.py:1593
The new endpoint references ActivityResponseSchema, but none of the chart, dashboard, or dataset APIs imports it or adds it to openapi_spec_component_schemas (unlike VersionListItemSchema). Consequently, the generated OpenAPI document cannot resolve this response schema. Please register it for all three resource APIs.
P2 — Mark tombstone version UUIDs as nullable
superset/versioning/schemas.py:275-284
For hard-deleted related entities, decoration deliberately emits version_uuid=None because no live UUID is available, but this field is declared as a non-null string. Once the response schema is exposed in OpenAPI, generated clients will reject a documented tombstone response. Please set allow_none=True and document the tombstone case.
Apply code-review fixes to the cross-entity activity view (apache#41076): - Redact tombstoned related-entity metadata (entity_name, summary, changed_by) so a hard-delete can't widen what a requester entitled only to the path entity can see. - Bound the self window at the entity's first tracked (id, uuid) transaction so a reused integer id can't inherit a hard-deleted predecessor's history. - Make the change-record fetch ceiling per-kind (<= n_kinds * 5000) and collapse truncation to a clean time cut instead of per-kind holes. - Guard Window construction against degenerate shadow rows (log-and-skip instead of 500). - Bound the name-denormalization and impact-count queries by the page-set's transaction range. - Correct the dataset include=related docs: it returns an empty stream. - Drop the dead SC-003 restore perf test (restore endpoint ships later). - Skip the redundant second path-entity resolve; rename load_shadow_model -> load_live_model; use Window.merges_with in union_windows; fix the summary schema doc and a time-specific comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cd0b769 to
8b19744
Compare
A per-entity activity feed built on the versioning base infrastructure: it assembles chart/dashboard/dataset change records into a single visibility- scoped, paginated timeline (the /activity/ read surface), with SQL-side access filtering, tombstone redaction for deleted related entities, and headline rendering. Adds two OpenAPI contract fields the write side already emits (the operation verb, path nullability). Stacked on apache#41176. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apply code-review fixes to the cross-entity activity view (apache#41076): - Redact tombstoned related-entity metadata (entity_name, summary, changed_by) so a hard-delete can't widen what a requester entitled only to the path entity can see. - Bound the self window at the entity's first tracked (id, uuid) transaction so a reused integer id can't inherit a hard-deleted predecessor's history. - Make the change-record fetch ceiling per-kind (<= n_kinds * 5000) and collapse truncation to a clean time cut instead of per-kind holes. - Guard Window construction against degenerate shadow rows (log-and-skip instead of 500). - Bound the name-denormalization and impact-count queries by the page-set's transaction range. - Correct the dataset include=related docs: it returns an empty stream. - Drop the dead SC-003 restore perf test (restore endpoint ships later). - Skip the redundant second path-entity resolve; rename load_shadow_model -> load_live_model; use Window.merges_with in union_windows; fix the summary schema doc and a time-specific comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dashboard's `owners` relationship was removed in favor of subject-based `editors`; the activity visibility test constructed dashboards with `owners=[user]`, which now raises `TypeError: 'owners' is an invalid keyword`. Grant read access via each user's Subject (`editors=[...]`) so the DashboardAccessFilter still gates Gamma as intended. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8b19744 to
ea43cd3
Compare
|
Follow-up on the review findings posted above: the blocking correctness items and associated cleanup were addressed in In particular, the updated implementation:
I also performed a fresh Docker/PostgreSQL dark-launch verification with
Authenticated requests to each entity's |
rusackas
left a comment
There was a problem hiding this comment.
Thanks @mikebridge, this is a lot of careful work, and the review threads look properly resolved rather than just closed. LGTM.
One loose end left: the schema-registration and allow_none items from your own July 13 pass (ActivityResponseSchema missing from openapi_spec_component_schemas, version_uuid still non-nullable) don't look like they made it into the fixes. Worth a follow-up commit, but not holding up the merge for it.
SUMMARY
Adds a backend-only cross-entity version activity view for dashboards, charts, and datasets:
The endpoints return a unified, newest-first stream of atomic version changes. Dashboard activity includes its own edits, edits to charts during their dashboard-association windows, and edits to datasets during the relevant chart/dataset windows. Chart activity includes its own edits plus dataset edits during association. Dataset activity is self-only.
The implementation reads the versioning infrastructure already present on
master:dashboard_slices_version,slices_version,version_changes, andversion_transaction. This PR contains no database migration.Related records are filtered through the caller's read visibility. Historical identity uses stable UUIDs in addition to internal integer IDs so ID reuse cannot authorize predecessor history. Hard-deleted or UUID-mismatched related entities appear only as redacted tombstones:
entity_deleted=true,entity_uuid=null,version_uuid=null, emptyentity_name, generic(deleted) <kind>summary, and no editor/path/diff values.API contract
Query parameters:
sinceissued_atbounduntilissued_atboundincludeall|self|relatedallrelatedis empty andallis equivalent toselfqpage0page_size25Response:
{ "result": ["/* ActivityRecordSchema[] */"], "count": 0, "truncated": false }countcovers the materialized filtered stream and is a floor whentruncated=true. The fetch ceiling is applied across exact association windows after globally merging entity-ID chunks.truncated=truemeans an additional older exact-window match was observed beyond the ceiling.Review remediation is tracked in
specs/sc-107283-versioning-activity-view/spec.md(AV-021–AV-035) andtasks.md(T054–T068).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable. This PR is backend-only; the consuming UI is tracked separately in #41551.
TESTING INSTRUCTIONS
Manual smoke test after generating version history for a dashboard and its related charts/datasets:
The last request should return an empty result because datasets have no related layer in this version.
ADDITIONAL INFORMATION
Prerequisites already merged: #39859 and #41176.