Skip to content

feat(versioning): version-restore engine and endpoints for charts, dashboards, and datasets - #42469

Merged
eschutho merged 14 commits into
apache:masterfrom
mikebridge:sc-115279-version-restore
Jul 29, 2026
Merged

feat(versioning): version-restore engine and endpoints for charts, dashboards, and datasets#42469
eschutho merged 14 commits into
apache:masterfrom
mikebridge:sc-115279-version-restore

Conversation

@mikebridge

@mikebridge mikebridge commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

Adds the write side of entity versioning: a non-destructive version-restore engine and POST /api/v1/{chart,dashboard,dataset}/<uuid>/versions/<version_uuid>/restore endpoints. This is the backend half that superset/versioning/api_helpers.py declared "ships in a later PR" when the read side merged in #41176 — and the hard dependency for the version-history UI (#41551), whose Restore flow calls exactly these routes.

Design:

  • Restore is strictly per-entity. A restore rewrites the target entity's own fields (for datasets, also its own columns/metrics — the aggregate's internal parts), never the content of other entities. A dashboard restore reconstructs chart membership from the dashboard_slices_version validity windows, reattaching only charts that still exist: live member charts' content is never rewritten (charts are shared entities with their own restore), and deleted charts are never revived — skipped members are surfaced in the response message.
  • Append-only. A restore writes a new version (authored by the restoring user, stamped action_kind='restore' plus the __meta__ headline so the activity feed renders "Restored to version N"), so history is never rewritten and a restore is itself reversible.
  • Stable version identity end-to-end. resolve_version() translates the client's version_uuid to the Continuum transaction_id, and the engine targets the row by it — no positional-index handoff, so concurrent retention pruning cannot shift the target. DELETE-type version rows are refused (404) — a Reverter on one would delete the live entity.
  • Kill-switch aware. With ENABLE_VERSIONING_CAPTURE off, Continuum's write listeners are detached and a revert would be a destructive, untracked write — so the restore surface is inert (404), matching the read-side convention; the operator-facing init log says so.
  • Layering: superset/versioning/restore.py (engine, single_flush_scoped so the whole revert lands in one Continuum transaction) ← BaseRestoreVersionCommand (capture gate → lookup → raise_for_editorship → resolve → stamp → engine; @transaction boundary built from the failed_exc ClassVar so subclasses are pure declarations, mirroring BaseRestoreCommand) ← a shared restore_version_endpoint in api_helpers.py (the three API methods are one-line delegations, matching the read-side pattern).

No DB migration. Reviewed pre-filing by an 8-lens parallel review; all HIGH findings addressed in ddfcb19645.

Merge order: this PR unblocks #41551 (version-history UI), which stays draft until this lands. Tracked in sc-115279.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — API only.

TESTING INSTRUCTIONS

  1. Enable ENABLE_VERSIONING_CAPTURE = True in config.
  2. Edit a chart a few times; GET /api/v1/chart/<uuid>/versions/ and pick an older version_uuid.
  3. POST /api/v1/chart/<uuid>/versions/<version_uuid>/restore — expect 200 with a fresh ETag; the chart's live state matches the target version, the versions list has grown by one, and the newest activity entry renders as a restore (action_kind='restore').
  4. Repeat for dashboards (note: membership is restored; member charts keep their current content) and datasets.
  5. Authorization: route level requires can_write; object level, a non-editor of the specific entity gets 403. Unknown UUIDs → 404; malformed → 400; another entity's version_uuid → 404. With ENABLE_VERSIONING_CAPTURE = False the endpoints return 404 and nothing is mutated.

Automated coverage: DB-free unit tests for the engine guard branches (missing entity/transaction, DELETE-row target, unregistered-model fail-closed) and the single_flush_scope flush contract; 33 integration tests across the three entities covering happy path, editorship 403s (all three entities), capture-off 404, cross-entity version_uuid mismatch, action_kind stamping, restore attribution, dashboard membership reattachment, live-chart-content preservation, and deleted-member skipping.

ADDITIONAL INFORMATION

  • Has associated issue: sc-115279 (Preset Shortcut); hard prerequisite of feat(versioning): version-history UI #41551
  • Required feature flags: none new — meaningful only with ENABLE_VERSIONING_CAPTURE (config, off by default)
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

🤖 Generated with Claude Code

Mike Bridge and others added 9 commits July 27, 2026 10:55
Adds the write side of the entity-versioning DAO that the base-infra
slice's docstrings already reference but never shipped:

- superset/versioning/restore.py — non-destructive, Continuum-based
  restore engine (restore_version)
- superset/commands/version_restore.py — BaseRestoreVersionCommand,
  the shared workflow for the per-entity restore commands
- VersionDAO.restore_version wired in superset/daos/version.py

Foundation for the chart/dashboard/dataset restore commands and routes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
DB-free control-flow test for VersionDAO.restore_version; the happy
path is covered end-to-end in the per-entity integration suites.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…squash

The restore engine is the sole consumer of single_flush_scope; the
version-read slice that squash-merged as apache#41176 dropped it from
versioning/utils.py because nothing merged referenced it. Ported back
verbatim alongside its consumer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Wires the chart side of non-destructive version restore on top of the
restore engine:

- superset/commands/chart/restore_version.py — RestoreChartVersionCommand
- POST /api/v1/chart/<uuid>/versions/<version_uuid>/restore in charts/api.py
  (registered in include_route_methods)

Reverts a chart to an earlier version, stamping the resulting
transaction action_kind="restore". Idempotent when already at the
target version.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ports the restore coverage from sc-103156: scalar-field revert with a
new version row, restoring-user attribution, and 400/404 paths for
bad/unknown UUIDs. Runs with ENABLE_VERSIONING_CAPTURE on (global in the
integration test config).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wires the dashboard side of non-destructive version restore on top of
the restore engine:

- superset/commands/dashboard/restore_version.py — RestoreDashboardVersionCommand
- POST /api/v1/dashboard/<uuid>/versions/<version_uuid>/restore in
  dashboards/api.py (registered in include_route_methods)

Reverts a dashboard to an earlier version, stamping the resulting
transaction action_kind="restore". Idempotent when already at target.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ports the restore coverage from sc-103156: scalar-field revert,
reattaching a chart removed after the snapshot, and 404 paths for
unknown entity/version UUIDs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wires the dataset side of non-destructive version restore on top of the
restore engine:

- superset/commands/dataset/restore_version.py — RestoreDatasetVersionCommand
- POST /api/v1/dataset/<uuid>/versions/<version_uuid>/restore in
  datasets/api.py (registered in include_route_methods)

Reverts a dataset to an earlier version, stamping the resulting
transaction action_kind="restore". Idempotent when already at target.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ports the restore coverage from sc-103156: scalar-field revert, child
column/metric reversion (re-add removed, drop added) in one
transaction, the full child diff emission, write-permission denial, and
400/404 paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot added the api Related to the REST API label Jul 27, 2026
@netlify

netlify Bot commented Jul 27, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 48af090
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a672e05700cfd0008f720b2
😎 Deploy Preview https://deploy-preview-42469--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.73239% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.28%. Comparing base (6856d0f) to head (c65d09f).
⚠️ Report is 67 commits behind head on master.

Files with missing lines Patch % Lines
superset/versioning/restore.py 74.60% 11 Missing and 5 partials ⚠️
superset/commands/version_restore.py 94.11% 1 Missing and 2 partials ⚠️
superset/versioning/api_helpers.py 88.88% 3 Missing ⚠️
superset/initialization/__init__.py 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42469      +/-   ##
==========================================
+ Coverage   65.25%   65.28%   +0.02%     
==========================================
  Files        2795     2801       +6     
  Lines      157639   158187     +548     
  Branches    36052    36118      +66     
==========================================
+ Hits       102869   103268     +399     
- Misses      52793    52918     +125     
- Partials     1977     2001      +24     
Flag Coverage Δ
hive 38.23% <25.82%> (-0.15%) ⬇️
mysql 57.66% <88.73%> (+0.10%) ⬆️
postgres 57.69% <88.73%> (+0.10%) ⬆️
presto 40.14% <25.82%> (-0.17%) ⬇️
python 59.09% <88.73%> (+0.09%) ⬆️
sqlite 57.32% <88.26%> (+0.11%) ⬆️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Mike Bridge and others added 2 commits July 27, 2026 11:22
raise_for_ownership was removed from SupersetSecurityManager before the
base-infra squash merged; raise_for_editorship is its successor (admin
bypass built in, soft-delete visibility handled) and is what the merged
soft-delete BaseRestoreCommand uses. Caught by the version-restore
integration suites (11 failures, all this attribute error).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…targeting, action stamp, membership-only dashboards

Addresses the HIGH findings from the parallel review of apache#42469:

- Gate restore on ENABLE_VERSIONING_CAPTURE (validate() -> 404): with
  capture off, Continuum's write listeners are detached and a revert
  would be a destructive, untracked write. Init log text updated.
- Target the version row by transaction_id instead of a positional
  OFFSET re-query: resolve_version() returns (version_number, tx_id),
  removing the prune-shift TOCTOU and the triplicated baseline-first
  ordering. DELETE-row targets are refused (Reverter would delete the
  live entity and report success).
- Stamp action_kind='restore' + the __meta__ headline on the restoring
  transaction so the activity feed renders 'Restored to version N'
  (contract in versioning/changes; matches import/clone stampers).
- Dashboard restore is membership-only: no Reverter recursion into
  member charts (live chart content is never rewritten; deleted charts
  are never revived); membership is reconstructed from the
  dashboard_slices_version validity windows, missing members are
  skipped and surfaced in the response message.
- BaseRestoreVersionCommand builds the @transaction boundary from a
  failed_exc ClassVar (mirrors BaseRestoreCommand); subclasses are pure
  declarations. Entity is loaded once and threaded through resolve and
  the engine. _RESTORE_RELATIONS fails closed for unknown models.
- Extract restore_version_endpoint into versioning/api_helpers; the
  three API handlers collapse to delegation calls. Stale 'ships in a
  later PR' docstrings updated.
- Tests: un-skip the chart 403 as a can-write-non-editor case, add the
  dashboard 403, capture-off 404, cross-entity version-uuid mismatch,
  action_kind stamping, membership-preservation and skipped-member
  integration tests, plus DB-free unit tests for the engine guards and
  the single_flush_scope flush contract.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mikebridge
mikebridge marked this pull request as ready for review July 27, 2026 16:03
@dosubot dosubot Bot added api:charts Related to the REST endpoints of charts api:dashboard Related to the REST endpoints of the Dashboard change:backend Requires changing the backend labels Jul 27, 2026

@mikebridge mikebridge left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review: version-restore engine + endpoints (charts/dashboards/datasets)

Went through superset/versioning/restore.py, superset/commands/version_restore.py, the three per-entity command subclasses, the API wiring in charts/dashboards/datasets/api.py, versioning/api_helpers.py, and all four new test files, plus cross-checked the actual sqlalchemy-continuum Reverter source (pinned at 1.7.0) to confirm the revert semantics claimed in the docstrings.

What's solid (verified, not just taken on faith)

  • DELETE-row guard is load-bearing, not decorative. Confirmed against Reverter.__call__: calling .revert() on a DELETE-type version row does session.delete(self.version_parent) — i.e. it would delete the live entity and report success. The operation_type == _OPERATION_DELETE check in restore_version() is exactly the right place to stop that, and it's pinned by a unit test (test_restore_version_refuses_delete_row_target).
  • Dashboard membership reconstruction can't corrupt live charts. Dashboard.slices is a plain secondary= M2M relationship with no delete-orphan cascade, so dashboard.slices = live_slices only rewrites the association table, never touches Slice rows. Passing relations=[] for Dashboard to the Reverter is also confirmed correct by reading revert_relationships() — an empty relations list means no relationship (including slices) gets recursed into, so Continuum can't fight with _restore_dashboard_membership's manual reattachment. Well covered by test_restore_preserves_live_chart_content, test_restore_reattaches_chart_removed_after_snapshot, test_restore_skips_member_chart_that_no_longer_exists.
  • Authorization is layered correctly and both layers are tested. Route-level can_write (dataset test uses Gamma, who lacks can_write on Dataset, → 403 before the command even runs) is distinct from object-level raise_for_editorship (chart/dashboard tests use Alpha, who has can_write generally but isn't an editor of that entity → 403 from inside validate()). Good that both are exercised separately rather than just one.
  • Cross-entity version_uuid mismatch and capture-off kill-switch are both tested (test_restore_returns_404_for_other_entitys_version_uuid, test_restore_returns_404_when_capture_disabled), and the kill-switch test correctly asserts nothing was mutated, not just the status code.
  • single_flush_scope's exception-skips-the-trailing-flush behavior and the after_transaction_end cleanup of session.info (which resets ACTION_KIND_KEY/ACTION_META_KEY on rollback too, not just commit) mean the "restore raced with a prune, 404'd, but left a stray action_kind stamp in the session" scenario I went looking for doesn't actually happen — it's handled by pre-existing listener infra.

Test coverage is genuinely as described: 33 integration tests + the DB-free unit tests for the guard branches actually exercise the specific edge cases in the prompt (I read every test body, not just the names).

Findings

  1. (Minor, pre-existing behavior inherited rather than introduced) position_json can go stale after a dashboard restore skips a deleted member. _restore_dashboard_membership correctly excludes deleted charts from dashboard.slices, but position_json (the layout JSON) is a plain scalar column that gets reverted wholesale by target_version.revert(relations=[]) with zero coordination with the separate membership reconstruction. So a restored dashboard can end up with a layout node referencing a chart tile that isn't in slices at all. I checked, and this is not a new regression — hard-deleting a chart that's a live dashboard member today already produces the identical divergence (the dashboard_slices FK cascades on delete, but nothing touches position_json) — so restore just inherits an existing product-wide gap rather than creating a new one. Worth a follow-up ticket, not a blocker for this PR.

  2. restore_version_endpoint (versioning/api_helpers.py:322) drops the entity_id optimization its sibling endpoints use. list_versions_endpoint and get_version_endpoint both call set_version_etag_by_uuid(..., entity_id=entity.id) specifically to avoid the extra SELECT id WHERE uuid = ? that set_version_etag_by_uuid otherwise runs (per that function's own docstring). restore_version_endpoint has result.entity.id sitting right there from the command's RestoreResult and doesn't pass it, re-adding that avoidable query on every restore call (a request that already does a Continuum revert + flush, so it's not free either way, but it's a one-line fix: entity_id=result.entity.id).

  3. _restore_dashboard_membership re-implements a validity-window query that already exists. superset/versioning/changes/shadow_queries.py has shadow_rows_valid_at(session, shadow_table, fk_col_name, fk_value, tx), which implements the exact same "transaction_id <= tx AND (end_transaction_id IS NULL OR end_transaction_id > tx) AND operation_type != 2" predicate and is already used for this purpose elsewhere (get_version()'s column/metric reconstruction). _restore_dashboard_membership hand-rolls the same predicate again directly against dashboard_slices_version. Relatedly, the DELETE operation_type value 2 is now a hardcoded/duplicated constant in three places (restore.py's _OPERATION_DELETE, shadow_queries.py's inline != 2, and queries.py's _OP_TYPE_LABELS). Not a bug today, but if the validity-window semantics or the delete op-code ever change, there's no single place to fix it — worth consolidating, or at minimum extracting a shared constant.

  4. BaseRestoreVersionCommand.run()'s error mapping only converts SQLAlchemyError to failed_exc. @transaction(on_error=partial(on_error, reraise=self.failed_exc)) doesn't override on_error's default catches=(SQLAlchemyError,), so a non-DB exception raised inside the engine — e.g. the LookupError restore_version() raises for a model missing from _RESTORE_RELATIONS — bypasses the failed_exc/422 mapping and surfaces as a raw 500 instead. Currently unreachable (all three shipped entities are registered), so low severity today, but it's an easy latent trap for whoever wires up a 4th entity and forgets the registry entry — they'll get a confusing 500 instead of the intended fail-closed 422.

  5. (Low, config-hygiene) capture_enabled() is a runtime config read, decoupled from whether Continuum's write listeners are actually attached. The kill-switch gate and the listener attach/detach (init_versioning()) are two separate mechanisms that happen to agree only because ENABLE_VERSIONING_CAPTURE is expected to be static per-process. If it's ever flipped at runtime without re-running init_versioning() (dynamic config reload, some future admin toggle), validate() would pass while listeners stay detached — precisely the "destructive, untracked write" scenario the kill-switch exists to prevent. Worth a comment noting this operational constraint if there isn't one already, or asserting listener state more directly.

Nothing above blocks correctness for the shipped happy paths — the core guarantees (per-entity restore, non-destructive append-only history, membership reconstruction, authorization, kill-switch) all check out against both the diff and the actual Reverter implementation. Items 2–3 are easy wins worth taking before merge; 1, 4, 5 are fine as follow-ups.

- Pass the already-loaded entity id to set_version_etag_by_uuid from the
  restore endpoint, matching the sibling list/get endpoints' avoidance
  of the id-by-uuid SELECT.
- Reuse shadow_rows_valid_at for dashboard membership reconstruction
  instead of hand-rolling the same validity-window predicate, and hoist
  the Continuum DELETE op-code into a shared OPERATION_DELETE constant
  used by both the restore guard and every validity predicate.
- Document the operational constraint on capture_enabled(): the flag is
  read at call time but listeners attach only in init_versioning(), so
  runtime flips require a restart.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mikebridge

Copy link
Copy Markdown
Contributor Author

Review follow-through (5564494):

  • Taken — finding 2: restore_version_endpoint now passes entity_id=result.entity.id to set_version_etag_by_uuid, matching the sibling endpoints.
  • Taken — finding 3: _restore_dashboard_membership reuses shadow_rows_valid_at instead of hand-rolling the validity-window predicate, and the Continuum DELETE op-code is now a single shared OPERATION_DELETE constant (baseline/shadow.py) used by the restore guard and every validity predicate.
  • Taken — finding 5: capture_enabled() documents the operational constraint (config read at call time vs listeners attached at init_versioning(); runtime flips require a restart).
  • Ticketed — finding 1: sc-115325 (pre-existing position_json / membership divergence, product-wide).
  • Ticketed — finding 4: sc-115326 (non-SQLAlchemy exceptions bypass the 422 mapping; unreachable until a 4th entity is wired).

…ELETE constant

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bito-code-review

bito-code-review Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #108a8e

Actionable Suggestions - 0
Additional Suggestions - 2
  • superset/commands/dashboard/restore_version.py - 1
    • Missing unit tests for new command · Line 1-45
      The new `RestoreDashboardVersionCommand` class has integration test coverage via `version_restore_tests.py` but lacks dedicated unit tests at the command level. Per the codebase standard for new commands (BITO.md adaptive rule [11730]), add unit tests covering the four key paths: not-found, version-not-found, forbidden, and success. This aligns with how other command modules (e.g., `restore_test.py`) are structured.
  • tests/integration_tests/dashboards/version_restore_tests.py - 1
    • Dead helper function · Line 44-51
      The `_get_version_rows` helper is defined but never called anywhere in the file. Unused helper functions create maintenance overhead and can mislead readers. Either remove it or add a comment indicating it is reserved for future use.
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • tests/integration_tests/dashboards/version_restore_tests.py - 5
  • tests/integration_tests/datasets/version_restore_tests.py - 1
    • Dead code: unused helper functions · Line 45-72
Review Details
  • Files reviewed - 20 · Commit Range: 95571de..94e912a
    • superset/charts/api.py
    • superset/commands/chart/restore_version.py
    • superset/commands/dashboard/restore_version.py
    • superset/commands/dataset/restore_version.py
    • superset/commands/version_restore.py
    • superset/daos/version.py
    • superset/dashboards/api.py
    • superset/datasets/api.py
    • superset/initialization/__init__.py
    • superset/versioning/api_helpers.py
    • superset/versioning/baseline/__init__.py
    • superset/versioning/baseline/shadow.py
    • superset/versioning/changes/shadow_queries.py
    • superset/versioning/queries.py
    • superset/versioning/restore.py
    • superset/versioning/utils.py
    • tests/integration_tests/charts/version_restore_tests.py
    • tests/integration_tests/dashboards/version_restore_tests.py
    • tests/integration_tests/datasets/version_restore_tests.py
    • tests/unit_tests/versioning/test_restore.py
  • Files skipped - 0
  • Tools
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@rusackas
rusackas requested review from EnxDev, Copilot and rusackas July 28, 2026 17:38

@rusackas rusackas left a comment

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.

Went through the engine, the command layer, all three API wirings, and the four test files, and cross-checked the actual sqlalchemy-continuum Reverter source (pulled 1.7.0 from PyPI) rather than taking the revert semantics on faith. The DELETE-row guard is real: Reverter.__call__ does session.delete(self.version_parent) on a DELETE-type row, so the operation_type == OPERATION_DELETE check earns its keep. raise_for_editorship runs first in _do_restore(), before resolve_version/restore_version touch anything, so there's no path into the engine that skips it. Nothing SQL-injectable either, it's ORM expressions throughout.

Also traced through the "Taken" claims from the self-review comment. Two check out clean. The _OP_TYPE_LABELS consolidation in queries.py is only partial though, that dict still hardcodes 2 instead of the new OPERATION_DELETE constant. Cosmetic (display labels only, no guard logic touches it), and it's not part of this diff so I can't leave it as a suggestion, but worth a follow-up pass.

One more worth flagging: BaseRestoreVersionCommand has no dedicated unit tests, only integration coverage. BaseRestoreCommand (the soft-delete restore this mirrors) has exactly that pattern in test_base_restore_command.py, fast local feedback pinning the validation contract. Not blocking, just an easy add given the precedent's already sitting right there.

Left suggestions on the three 403 tests (chart/dashboard/dataset) to also assert nothing mutated, matching the capture-disabled test's own pattern in this same PR.

None of this blocks the happy paths. LGTM.

Comment thread tests/integration_tests/charts/version_restore_tests.py
Comment thread tests/integration_tests/dashboards/version_restore_tests.py
Comment thread tests/integration_tests/datasets/version_restore_tests.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds write-side entity version restore for charts, dashboards, and datasets, exposing new restore endpoints and a shared restore engine to support the version-history UI and append-only versioning semantics.

Changes:

  • Introduces restore_version engine with dashboard membership reconstruction and single-flush restore semantics.
  • Adds BaseRestoreVersionCommand plus per-entity restore commands and wires new POST .../versions/<version_uuid>/restore routes.
  • Extends version resolution to return stable transaction_id alongside version_number, and adds unit/integration coverage.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
tests/unit_tests/versioning/test_restore.py DB-free unit tests covering restore engine guard branches and single_flush_scope behavior.
tests/integration_tests/datasets/version_restore_tests.py Integration suite for dataset restore semantics (including columns/metrics) and expected HTTP errors.
tests/integration_tests/dashboards/version_restore_tests.py Integration suite for dashboard restore, including membership-only behavior and skipped deleted charts.
tests/integration_tests/charts/version_restore_tests.py Integration suite for chart restore; validates version append-only, attribution, kill-switch behavior, and errors.
superset/versioning/utils.py Adds shared capture_enabled() gate and single_flush_scope() to enforce single-flush restore semantics.
superset/versioning/restore.py New restore engine implementing per-entity restore and dashboard membership reconstruction.
superset/versioning/queries.py Adds resolve_version() returning (version_number, transaction_id) and keeps resolve_version_uuid() wrapper for read-side callers.
superset/versioning/changes/shadow_queries.py Replaces magic delete op code with shared OPERATION_DELETE constant.
superset/versioning/baseline/shadow.py Introduces shared OPERATION_DELETE constant for delete-row checks.
superset/versioning/baseline/init.py Exports OPERATION_DELETE from baseline package.
superset/versioning/api_helpers.py Adds shared restore_version_endpoint() and unifies capture gate with capture_enabled().
superset/initialization/init.py Updates init log to document restore endpoints refusing when capture is disabled.
superset/datasets/api.py Wires dataset restore-version endpoint and permissions.
superset/dashboards/api.py Wires dashboard restore-version endpoint and permissions.
superset/charts/api.py Wires chart restore-version endpoint and permissions.
superset/daos/version.py Re-exports write-side restore and resolve_version via VersionDAO.
superset/commands/version_restore.py Adds shared base command implementing capture gate, editorship check, resolve, stamping, and transactional boundary.
superset/commands/dataset/restore_version.py Dataset-specific restore command declaration.
superset/commands/dashboard/restore_version.py Dashboard-specific restore command declaration.
superset/commands/chart/restore_version.py Chart-specific restore command declaration.

Comment thread superset/versioning/restore.py
Comment thread superset/versioning/restore.py
Comment thread superset/versioning/restore.py
Comment thread superset/versioning/restore.py
Comment thread superset/versioning/restore.py
Comment thread superset/versioning/restore.py
Comment thread superset/versioning/utils.py
Comment thread superset/versioning/utils.py
Comment thread superset/versioning/api_helpers.py
@rusackas

Copy link
Copy Markdown
Member

12 unresolved review threads to deal with... holler if you want help knocking 'em back.

Address review feedback on apache#42469.

Forbidden-path tests asserted only the status code; the capture-disabled
404 test already went further and checked nothing mutated. Bring the chart,
dashboard, and dataset siblings to the same standard.

Also guard the restore engine's preloaded-entity shortcut: when a caller
passes entity, it must be the row entity_uuid names, or the engine
would restore one entity while the caller logs another. The sole caller
already loads it by that UUID, so this is misuse insurance for future
callers rather than a live fix. And use logger.exception on the 422
path so the traceback survives into production logs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mikebridge

Copy link
Copy Markdown
Contributor Author

Worked through the 12 open review threads — pushed c65d09f94e. Summary for anyone picking this up:

Taken (6 threads):

  • @rusackas's three test nits, applied verbatim — the 403 paths now assert the entity was untouched, matching what the capture-disabled test already did.
  • Copilot's entity_uuid point: the engine's preloaded-entity shortcut now raises if entity.uuid and entity_uuid disagree, with a unit test. No live bug (the one caller loads by that UUID), but the failure mode — restoring one row while the audit trail names another — is worth a guard.
  • Copilot's logging point: logger.exception on the 422 path so the traceback survives.

Not taken (6 threads), reasoning in each:

  • datetime.now() → UTC would be wrong here: changed_on is defined with default=datetime.now, onupdate=datetime.now in models/helpers.py, so UTC would leave one column holding two time bases depending on whether the last write was a save or a restore. Same change was proposed and reverted on feat(soft-delete): deletion-retention purge of soft-deleted entities #41549.
  • The capture_enabled() init-vs-request-time gap is real and already documented in that function's docstring, mitigation included.
  • _RESTORE_RELATIONS string keying is deliberate: it fails closed with a LookupError on an unregistered model, and keying on the class would reintroduce the model-import cycle this module avoids.

100 versioning unit tests green, pre-commit clean. The approval survived the push and I've kept the change surface to tests plus two small guards.

I don't have merge rights on this repo — when CI comes back green, could a committer merge? This is the last gate on #41551 (version-history UI), which is otherwise ready and waiting on it.

Comment thread superset/versioning/utils.py
Comment thread superset/commands/version_restore.py
Comment thread superset/versioning/restore.py
Comment thread superset/versioning/restore.py
Comment thread superset/versioning/restore.py
@mikebridge

Copy link
Copy Markdown
Contributor Author

Second review round (codeant-ai, 5 threads) triaged — replies in each thread. No code change needed on this PR; c65d09f94e still stands.

1 real, filed as a follow-up: the restore path has no row lock or optimistic version check, so a concurrent edit committed between validate() and revert() is overwritten silently. Not merge-blocking — overwriting current state is what restore does, and the clobbered edit is itself captured as a version, so it's recoverable. The fix (optimistic transaction-id re-check → 409) wants a matching affordance in the version-history UI, so it belongs in its own change.

4 rejected, evidence in each thread:

  • Two soft-delete findings (find_active_by_uuid, and the dashboard membership query reattaching trashed charts) — both already handled by the global do_orm_execute listener that attaches deleted_at IS NULL to standard ORM queries. Neither query bypasses it. The membership case already reports trashed members via skipped_slice_ids, which is the exact contract the finding asks for.
  • No-op restores creating no version row is deliberate: SkipUnmodifiedPlugin exists specifically to suppress them, with the rationale and the orphaned-transaction handling documented.
  • The capture-gate init-vs-request-time gap is documented verbatim in capture_enabled()'s docstring, mitigation included. It assumes runtime mutation of ENABLE_VERSIONING_CAPTURE, which isn't a supported operation.

Worth noting for anyone weighing the "Critical 🚨" labels: all three of those rejected as incorrect were labelled Critical, and in each case the behavior the tool asked for is what the code already does — via a global listener or a dedicated plugin rather than a local filter. Static analysis doesn't see the do_orm_execute hook.

Still green, still approved, and still looking for a committer to merge — it's the last gate on #41551.

mikebridge pushed a commit to mikebridge/superset that referenced this pull request Jul 29, 2026
…t the chart

"View version history" was gated on can_overwrite alone, which
hydrateExplore computes purely as "the current user is among the
slice's editors". A chart with no explicit editors yields false for
everyone, so on a fresh install -- where every seeded and example chart
has an empty editors list -- the menu item was absent for all users,
administrators included. The feature was effectively invisible until
someone hand-edited a chart's editors.

The server disagrees with that gate. security_manager.is_editor returns
True for admins outright, and unions extra_editors (granted by a
deployment's EXTRA_EDITORS_RESOLVER) into the editor set before
comparing. So the UI was hiding an action the API would have allowed,
for two distinct groups of users.

SaveModal already worked around the same gap with a broader predicate,
which is why an admin could save over a chart but not see its history.
That predicate is now a shared helper both call sites use, so the two
cannot drift apart again:

  canOverwrite || admin || in editors || in extra_editors,
  and never for an externally managed slice

Two things fixed while extracting it. SaveModal's isCurrentUserOwner
fallback checked slice.owners, which the owners -> editors rename left
behind: Slice.data ships editors, extra_editors and viewers, and no
owners at all, so that branch could never fire. It is dropped rather
than carried into the shared helper. And extra_editors was not consulted
anywhere in the frontend despite being in the payload for exactly this
purpose.

Unit tests mocked redux with can_overwrite: true, so the gate was never
exercised against what hydrateExplore actually computes. The new helper
tests cover the real shapes, starting with an admin on a chart whose
editors list is empty.

Reported from local testing against an integration branch of apache#42469 +
apache#41549 + apache#41551 + apache#41550.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mikebridge pushed a commit to mikebridge/superset that referenced this pull request Jul 29, 2026
…t the chart

"View version history" was gated on can_overwrite alone, which
hydrateExplore computes purely as "the current user is among the
slice's editors". A chart with no explicit editors yields false for
everyone, so on a fresh install -- where every seeded and example chart
has an empty editors list -- the menu item was absent for all users,
administrators included. The feature was effectively invisible until
someone hand-edited a chart's editors.

The server disagrees with that gate. security_manager.is_editor returns
True for admins outright, and unions extra_editors (granted by a
deployment's EXTRA_EDITORS_RESOLVER) into the editor set before
comparing. So the UI was hiding an action the API would have allowed,
for two distinct groups of users.

SaveModal already worked around the same gap with a broader predicate,
which is why an admin could save over a chart but not see its history.
That predicate is now a shared helper both call sites use, so the two
cannot drift apart again:

  canOverwrite || admin || in editors || in extra_editors,
  and never for an externally managed slice

Two things fixed while extracting it. SaveModal's isCurrentUserOwner
fallback checked slice.owners, which the owners -> editors rename left
behind: Slice.data ships editors, extra_editors and viewers, and no
owners at all, so that branch could never fire. It is dropped rather
than carried into the shared helper. And extra_editors was not consulted
anywhere in the frontend despite being in the payload for exactly this
purpose.

Unit tests mocked redux with can_overwrite: true, so the gate was never
exercised against what hydrateExplore actually computes. The new helper
tests cover the real shapes, starting with an admin on a chart whose
editors list is empty.

Reported from local testing against an integration branch of apache#42469 +
apache#41549 + apache#41551 + apache#41550.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bito-code-review

bito-code-review Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #0f0291

Actionable Suggestions - 0
Review Details
  • Files reviewed - 6 · Commit Range: 94e912a..c65d09f
    • superset/versioning/api_helpers.py
    • superset/versioning/restore.py
    • tests/integration_tests/charts/version_restore_tests.py
    • tests/integration_tests/dashboards/version_restore_tests.py
    • tests/integration_tests/datasets/version_restore_tests.py
    • tests/unit_tests/versioning/test_restore.py
  • Files skipped - 0
  • Tools
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@eschutho
eschutho merged commit 940b670 into apache:master Jul 29, 2026
59 checks passed
mikebridge pushed a commit to mikebridge/superset that referenced this pull request Jul 30, 2026
…t the chart

"View version history" was gated on can_overwrite alone, which
hydrateExplore computes purely as "the current user is among the
slice's editors". A chart with no explicit editors yields false for
everyone, so on a fresh install -- where every seeded and example chart
has an empty editors list -- the menu item was absent for all users,
administrators included. The feature was effectively invisible until
someone hand-edited a chart's editors.

The server disagrees with that gate. security_manager.is_editor returns
True for admins outright, and unions extra_editors (granted by a
deployment's EXTRA_EDITORS_RESOLVER) into the editor set before
comparing. So the UI was hiding an action the API would have allowed,
for two distinct groups of users.

SaveModal already worked around the same gap with a broader predicate,
which is why an admin could save over a chart but not see its history.
That predicate is now a shared helper both call sites use, so the two
cannot drift apart again:

  canOverwrite || admin || in editors || in extra_editors,
  and never for an externally managed slice

Two things fixed while extracting it. SaveModal's isCurrentUserOwner
fallback checked slice.owners, which the owners -> editors rename left
behind: Slice.data ships editors, extra_editors and viewers, and no
owners at all, so that branch could never fire. It is dropped rather
than carried into the shared helper. And extra_editors was not consulted
anywhere in the frontend despite being in the payload for exactly this
purpose.

Unit tests mocked redux with can_overwrite: true, so the gate was never
exercised against what hydrateExplore actually computes. The new helper
tests cover the real shapes, starting with an admin on a chart whose
editors list is empty.

Reported from local testing against an integration branch of apache#42469 +
apache#41549 + apache#41551 + apache#41550.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mikebridge pushed a commit to mikebridge/superset that referenced this pull request Jul 31, 2026
…t the chart

"View version history" was gated on can_overwrite alone, which
hydrateExplore computes purely as "the current user is among the
slice's editors". A chart with no explicit editors yields false for
everyone, so on a fresh install -- where every seeded and example chart
has an empty editors list -- the menu item was absent for all users,
administrators included. The feature was effectively invisible until
someone hand-edited a chart's editors.

The server disagrees with that gate. security_manager.is_editor returns
True for admins outright, and unions extra_editors (granted by a
deployment's EXTRA_EDITORS_RESOLVER) into the editor set before
comparing. So the UI was hiding an action the API would have allowed,
for two distinct groups of users.

SaveModal already worked around the same gap with a broader predicate,
which is why an admin could save over a chart but not see its history.
That predicate is now a shared helper both call sites use, so the two
cannot drift apart again:

  canOverwrite || admin || in editors || in extra_editors,
  and never for an externally managed slice

Two things fixed while extracting it. SaveModal's isCurrentUserOwner
fallback checked slice.owners, which the owners -> editors rename left
behind: Slice.data ships editors, extra_editors and viewers, and no
owners at all, so that branch could never fire. It is dropped rather
than carried into the shared helper. And extra_editors was not consulted
anywhere in the frontend despite being in the payload for exactly this
purpose.

Unit tests mocked redux with can_overwrite: true, so the gate was never
exercised against what hydrateExplore actually computes. The new helper
tests cover the real shapes, starting with an admin on a chart whose
editors list is empty.

Reported from local testing against an integration branch of apache#42469 +
apache#41549 + apache#41551 + apache#41550.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mikebridge pushed a commit to mikebridge/superset that referenced this pull request Aug 3, 2026
…t the chart

"View version history" was gated on can_overwrite alone, which
hydrateExplore computes purely as "the current user is among the
slice's editors". A chart with no explicit editors yields false for
everyone, so on a fresh install -- where every seeded and example chart
has an empty editors list -- the menu item was absent for all users,
administrators included. The feature was effectively invisible until
someone hand-edited a chart's editors.

The server disagrees with that gate. security_manager.is_editor returns
True for admins outright, and unions extra_editors (granted by a
deployment's EXTRA_EDITORS_RESOLVER) into the editor set before
comparing. So the UI was hiding an action the API would have allowed,
for two distinct groups of users.

SaveModal already worked around the same gap with a broader predicate,
which is why an admin could save over a chart but not see its history.
That predicate is now a shared helper both call sites use, so the two
cannot drift apart again:

  canOverwrite || admin || in editors || in extra_editors,
  and never for an externally managed slice

Two things fixed while extracting it. SaveModal's isCurrentUserOwner
fallback checked slice.owners, which the owners -> editors rename left
behind: Slice.data ships editors, extra_editors and viewers, and no
owners at all, so that branch could never fire. It is dropped rather
than carried into the shared helper. And extra_editors was not consulted
anywhere in the frontend despite being in the payload for exactly this
purpose.

Unit tests mocked redux with can_overwrite: true, so the gate was never
exercised against what hydrateExplore actually computes. The new helper
tests cover the real shapes, starting with an admin on a chart whose
editors list is empty.

Reported from local testing against an integration branch of apache#42469 +
apache#41549 + apache#41551 + apache#41550.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api:charts Related to the REST endpoints of charts api:dashboard Related to the REST endpoints of the Dashboard api Related to the REST API change:backend Requires changing the backend size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants