feat(versioning): entity-version base infrastructure (gated off, dark launch) - #41176
Conversation
github-actions
Bot
added
risk:db-migration
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41176 +/- ##
==========================================
+ Coverage 64.60% 64.75% +0.14%
==========================================
Files 2712 2733 +21
Lines 151114 152643 +1529
Branches 34751 35011 +260
==========================================
+ Hits 97632 98838 +1206
- Misses 51659 51909 +250
- Partials 1823 1896 +73
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:
|
5f4a3a6 to
880efe5
Compare
880efe5 to
b7c5e3d
Compare
|
Since it touches @mistercrunch @michael-s-molina @betodealmeida @eschutho @sadpandajoe — would any of you have a little time to take a look? It ships dark behind (Posted via Claude on my behalf.) |
|
A multi-model review pass surfaced one correctness gap worth recording. It doesn't block this PR — it's only reachable with Parent baseline capture isn't SAVEPOINT-isolated on PostgreSQL. In Rather than disturb this PR at the merge gate, I've put the one-line fix in a small follow-up: #41910 (draft, stacked on this). (Posted via Claude on my behalf.) |
|
|
||
| @expose( | ||
| "/<uuid_str>/versions/<version_uuid_str>/", | ||
| methods=("GET",), |
There was a problem hiding this comment.
@mikebridge can you check to make sure that this is using an existing perm rather than creating a new one?
There was a problem hiding this comment.
Yep — list_versions and get_version are both mapped to read in MODEL_API_RW_METHOD_PERMISSION_MAP (which method_permission_name spreads in), so they resolve to the existing can_read_Dashboard permission — no new perm is minted. The per-object raise_for_access(dashboard=…) inside list_versions_endpoint adds the object-level gate on top.
(Replying via Claude on my behalf.)
| "Error refreshing dataset during update %s: %s", | ||
| self.__class__.__name__, | ||
| str(ex), | ||
| exc_info=True, |
There was a problem hiding this comment.
if you use exception instead of error, you won't need the exc_info
There was a problem hiding this comment.
Good call — done in 38bb2b0. logger.exception keeps the ERROR-level traceback without the explicit exc_info.
(Replying via Claude on my behalf.)
| fk_col = getattr(child_table.c, fk_column_name) | ||
|
|
||
| rows = ( | ||
| conn.execute(sa.select(child_table).where(fk_col == parent_obj.id)) |
There was a problem hiding this comment.
Is there a way to use SQLAlchemy instead of executing raw sql?
There was a problem hiding this comment.
These are SQLAlchemy Core (conn.execute(sa.select(...))) rather than raw text() — parameterized, no string SQL. They run on session.connection() instead of the ORM session.query deliberately: baseline capture fires inside before_flush, and an ORM query there would trigger an autoflush of Continuum's in-progress transaction mid-flush. Reading straight off the connection avoids that re-entrancy. Happy to add an inline comment making that explicit if it'd help.
(Replying via Claude on my behalf.)
|
|
||
| attached_slice_ids = [ | ||
| r.slice_id | ||
| for r in conn.execute( |
There was a problem hiding this comment.
same thing here for raw execution
There was a problem hiding this comment.
Same here — Core sa.select on the connection, not raw SQL; see the note on the select just above (avoids an autoflush re-entrancy inside before_flush).
(Replying via Claude on my behalf.)
| child_map = child_to_parent_registry() | ||
| for obj in list(session.dirty) + list(session.new) + list(session.deleted): | ||
| if type(obj) in VERSIONED_MODELS: | ||
| parents[id(obj)] = obj |
There was a problem hiding this comment.
can you use get here instead of grabbing the value directly with the key?
There was a problem hiding this comment.
Just to make sure I'm on the right line — on current HEAD, collection.py:66 is parents[id(obj)] = obj (a dict assignment), and the child_map lookup just below already uses .get(). Could you point me at the exact access you'd like switched? The anchor may have drifted after the recent master rebase.
(Replying via Claude on my behalf.)
Per review: logger.exception already logs at ERROR with the traceback, so exc_info=True is redundant. Same behavior, less boilerplate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| #: ISO-8601 (Flask's default JSON provider renders datetimes as RFC-1123 | ||
| #: http-dates) and ``version_uuid`` consistently a string (the list rows | ||
| #: carry UUID instances, the snapshot block pre-stringifies). | ||
| _version_item_schema = VersionListItemSchema() |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation to this module-level schema variable to satisfy the type-hint requirement for relevant variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The custom rule requires type hints on relevant variables when they can be annotated. This module-level schema instance is newly introduced and lacks an explicit type annotation, so the suggestion correctly identifies a real violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/versioning/api_helpers.py
**Line:** 53:53
**Comment:**
*Custom Rule: Add an explicit type annotation to this module-level schema variable to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| json.loads(position_json) | ||
| ) | ||
|
|
||
| dashboard = DashboardDAO.update(self._model, self._properties) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation to the newly introduced local variable so it conforms to the required type-hinting rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new local variable dashboard is introduced in the updated Python code without an explicit type annotation, and it is clearly annotatable from the return type of DashboardDAO.update(...). This matches the rule requiring type hints on new or modified Python variables that can be annotated.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/update.py
**Line:** 86:86
**Comment:**
*Custom Rule: Add an explicit type annotation to the newly introduced local variable so it conforms to the required type-hinting rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| # the old-named row is deleted in the same flush — INSERTs flush | ||
| # before DELETEs, so that collides on the PK / UNIQUE(table_id, | ||
| # column_name) constraints. ``table_id`` is pinned to *model*. | ||
| protected_keys = ("id", "table_id") |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this local variable to satisfy the type-hint requirement for relevant variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The variable is a newly added local constant with a clear, inferable tuple type, so it can be annotated under the Python type-hint rule. This is a real omission in the new code.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/daos/dataset.py
**Line:** 485:485
**Comment:**
*Custom Rule: Add an explicit type annotation for this local variable to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # incoming column whose name case-insensitively matches a removed column | ||
| # (a case-only rename under a case-insensitive collation, e.g. MySQL) | ||
| # would collide on ``UNIQUE(table_id, column_name)`` mid-flush. | ||
| deleted_any = False |
There was a problem hiding this comment.
Suggestion: Declare this boolean local with an explicit type hint to comply with the project’s typing rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The boolean flag is newly introduced and could be explicitly annotated as a typed local variable. This matches the stated type-hint rule for relevant variables.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/daos/dataset.py
**Line:** 501:501
**Comment:**
*Custom Rule: Declare this boolean local with an explicit type hint to comply with the project’s typing rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| setattr(metric, key, value) | ||
|
|
||
| # Delete removed metrics | ||
| ids_to_keep = property_metrics_by_id.keys() |
There was a problem hiding this comment.
Suggestion: Add a type annotation for this derived local collection to keep relevant variables fully typed. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added derived collection with a non-obvious type, so it is a plausible target for an explicit annotation under the rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/daos/dataset.py
**Line:** 608:608
**Comment:**
*Custom Rule: Add a type annotation for this derived local collection to keep relevant variables fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
| # Live version identifiers before the update (empty + query-free when | ||
| # ``ENABLE_VERSIONING_CAPTURE`` is off). | ||
| old_info = current_entity_version_info(Dashboard, pk) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this newly introduced version metadata variable to satisfy the type-hint requirement for relevant variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is newly added Python code that introduces a local variable without an explicit type hint, and the returned value is a well-defined version-info object that can be annotated, so it matches the type-hint rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 944:944
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced version metadata variable to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| new_info = current_entity_version_info( | ||
| Dashboard, changed_model.id, changed_model.uuid | ||
| ) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this newly introduced variable holding updated version metadata to comply with required type hints. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is also newly added Python code that introduces a local variable without an explicit type hint. The value comes from a typed helper returning a version-info record, so the omission is a real violation of the type-hint requirement.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 951:953
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced variable holding updated version metadata to comply with required type hints.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| new_info = current_entity_version_info( | ||
| SqlaTable, changed_model.id, changed_model.uuid | ||
| ) |
There was a problem hiding this comment.
Suggestion: Provide a concrete type hint for this new variable assignment so the added code remains fully typed. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This added assignment omits a type annotation on a newly introduced local variable, which matches the type-hint requirement for modified Python code.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/datasets/api.py
**Line:** 580:582
**Comment:**
*Custom Rule: Provide a concrete type hint for this new variable assignment so the added code remains fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| new_info = current_entity_version_info( | ||
| SqlaTable, changed_model.id, changed_model.uuid | ||
| ) | ||
| etag_version_uuid = new_info.version_uuid |
There was a problem hiding this comment.
Suggestion: Annotate this newly added variable with its expected type to satisfy the enforced typing rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly introduced variable assignment in Python code without an explicit annotation, so it violates the stated type-hint rule if the variable is considered annotatable.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/datasets/api.py
**Line:** 583:583
**Comment:**
*Custom Rule: Annotate this newly added variable with its expected type to satisfy the enforced typing rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| from superset import db | ||
| from superset.versioning.changes import ACTION_KIND_CLONE, ACTION_KIND_KEY | ||
|
|
||
| db.session.info[ACTION_KIND_KEY] = ACTION_KIND_CLONE |
There was a problem hiding this comment.
Suggestion: Setting a session-scoped action marker without guaranteed cleanup leaks state when versioning capture is disabled (the cleanup listeners are not registered in that mode). On a reused SQLAlchemy session, a later unrelated transaction can inherit this stale marker and be misclassified. Clear this key in a local try/finally (or a shared context manager) after the copy operation completes. [logic error]
Severity Level: Major ⚠️
- ❌ Dashboard version history mislabels later edits as clones.
- ⚠️ Operators misinterpret /versions timeline for affected dashboards.
- ⚠️ Multi-app off->on toggles risk polluted action_kind context.Steps of Reproduction ✅
1. Initialize a Superset app with `ENABLE_VERSIONING_CAPTURE=False`, so
`SupersetAppInitializer.init_versioning()` returns early at
`superset/initialization/__init__.py:36-45` and never calls
`register_change_record_listener()`; as a result, the `after_commit` / `after_rollback`
cleanup handlers in `superset/versioning/changes/listener.py:214-247` are not registered.
2. Trigger a dashboard copy via the MCP tool, which calls `CopyDashboardCommand(source,
data).run()` at `superset/mcp_service/dashboard/tool/duplicate_dashboard.py:26-27`. Inside
`CopyDashboardCommand.run` (decorated with `@transaction` at
`superset/commands/dashboard/copy.py:40`), the code at
`superset/commands/dashboard/copy.py:55` executes `db.session.info[ACTION_KIND_KEY] =
ACTION_KIND_CLONE` before calling `DashboardDAO.copy_dashboard(...)` at line 56.
3. The `@transaction` decorator at `superset/utils/decorators.py:235-263` then calls
`db.session.commit()` on success (line 2 of the second chunk), but because the versioning
change-record listeners were never registered, no `after_commit` handler runs, and nothing
pops `ACTION_KIND_KEY` from `db.session.info`. The marker `"_versioning_action_kind":
"clone"` therefore persists on the long-lived SQLAlchemy session beyond the end of this
transaction.
4. In the same Python process, initialize another Superset app (e.g. in tests or multi-app
setups) with `ENABLE_VERSIONING_CAPTURE=True`, so `init_versioning()` proceeds into the ON
branch at `superset/initialization/__init__.py:47-76` and calls
`register_change_record_listener()` at line 859, attaching `flush_change_records` and
`_stamp_action_kind_on_transaction` from `superset/versioning/changes/listener.py:157-188`
and :49-72 to the existing `db.session`. The next dashboard update (e.g. `PUT
/api/v1/dashboard/<id>`, which calls `UpdateDashboardCommand(pk, item).run()` at
`superset/dashboards/api.py:11-18`) runs under this same session; on its first
`after_flush`, `_stamp_action_kind_on_transaction` reads and pops the stale
`ACTION_KIND_KEY` value set by the earlier copy and stamps
`version_transaction.action_kind='clone'` for this unrelated update, misclassifying the
transaction.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/copy.py
**Line:** 55:55
**Comment:**
*Logic Error: Setting a session-scoped action marker without guaranteed cleanup leaks state when versioning capture is disabled (the cleanup listeners are not registered in that mode). On a reused SQLAlchemy session, a later unrelated transaction can inherit this stale marker and be misclassified. Clear this key in a local try/finally (or a shared context manager) after the copy operation completes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # module docstring for the broader init-order rationale. | ||
| from superset.versioning.changes import ACTION_KIND_CLONE, ACTION_KIND_KEY | ||
|
|
||
| db.session.info[ACTION_KIND_KEY] = ACTION_KIND_CLONE |
There was a problem hiding this comment.
Suggestion: This writes a per-transaction marker into session.info but relies on versioning listeners for cleanup; with capture gated off, those listeners are detached, so the marker can persist and incorrectly label subsequent transactions on the same session. Ensure the key is removed in the command path itself (for both success and error paths). [logic error]
Severity Level: Major ⚠️
- ❌ Dataset version history may show normal changes as clones.
- ⚠️ Dataset duplicate endpoint /api/v1/dataset/<pk>/duplicate impacted.
- ⚠️ Off->on capture toggles can inherit stale clone markers.Steps of Reproduction ✅
1. Start a Superset app with `ENABLE_VERSIONING_CAPTURE=False` so
`SupersetAppInitializer.init_versioning()` returns at
`superset/initialization/__init__.py:36-45` without calling
`register_change_record_listener()`, meaning the cleanup hooks that pop `ACTION_KIND_KEY`
in `reset_processed_after_commit` and `reset_action_kind_after_rollback`
(`superset/versioning/changes/listener.py:214-242`) are not attached.
2. Call the dataset duplicate endpoint `PUT /api/v1/dataset/<pk>/duplicate`
(OpenAPI-documented around `superset/datasets/api.py:790-829`), which loads the payload
and invokes `DuplicateDatasetCommand(item).run()` at `superset/datasets/api.py:18-20`.
Inside `DuplicateDatasetCommand.run` (decorated with `@transaction` at
`superset/commands/dataset/duplicate.py:52`), the code at
`superset/commands/dataset/duplicate.py:62-64` imports `ACTION_KIND_CLONE,
ACTION_KIND_KEY` and sets `db.session.info[ACTION_KIND_KEY] = ACTION_KIND_CLONE` before
creating and adding the new `SqlaTable` and related objects.
3. The transaction decorator at `superset/utils/decorators.py:235-263` commits on success
(`db.session.commit()`), but because no change-record listeners are registered when
capture is disabled, no `after_commit` handler fires to clear `ACTION_KIND_KEY`. As a
result, the session-scoped `db.session.info` retains `"_versioning_action_kind": "clone"`
after the duplicate finishes, even though that transaction is complete.
4. Later in the same Python process, a second Superset app is initialized with
`ENABLE_VERSIONING_CAPTURE=True`, causing `init_versioning()` to execute the ON branch at
`superset/initialization/__init__.py:47-76`, which calls
`register_change_record_listener()` at line 859 and attaches `flush_change_records` to
`db.session`. When a subsequent dataset update (for example, `PUT /api/v1/dataset/<pk>`,
which calls `UpdateDatasetCommand` in `superset/datasets/api.py`) performs the first
versioned flush, `flush_change_records` invokes `_stamp_action_kind_on_transaction` at
`superset/versioning/changes/listener.py:49-72`. That helper pops the stale
`ACTION_KIND_KEY` and stamps `version_transaction.action_kind='clone'` for this unrelated
update, causing the dataset version history to misclassify a normal edit as a clone.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dataset/duplicate.py
**Line:** 64:64
**Comment:**
*Logic Error: This writes a per-transaction marker into `session.info` but relies on versioning listeners for cleanup; with capture gated off, those listeners are detached, so the marker can persist and incorrectly label subsequent transactions on the same session. Ensure the key is removed in the command path itself (for both success and error paths).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # module docstring for the broader init-order rationale. | ||
| from superset.versioning.changes import ACTION_KIND_IMPORT, ACTION_KIND_KEY | ||
|
|
||
| db.session.info[ACTION_KIND_KEY] = ACTION_KIND_IMPORT |
There was a problem hiding this comment.
Suggestion: The import command sets a session-level action marker before running import work, but this file does not guarantee key removal itself. In the dark-launch default (ENABLE_VERSIONING_CAPTURE=False), listener-based cleanup is not active, so this marker can leak to later operations on the same scoped session and mis-tag them as imports. Add explicit cleanup around the _import call. [logic error]
Severity Level: Major ⚠️
- ❌ Dashboard version audit may misclassify edits as imports.
- ⚠️ Dashboards import endpoint /api/v1/dashboard/import/ implicated.
- ⚠️ Off->on capture toggles risk stale import markers.Steps of Reproduction ✅
1. Boot a Superset app with `ENABLE_VERSIONING_CAPTURE=False`, so
`SupersetAppInitializer.init_versioning()` takes the OFF path at
`superset/initialization/__init__.py:36-45`, logging that capture is disabled and
returning without calling `register_change_record_listener()` at line 859; thus the
`after_commit` and `after_rollback` handlers that clear `ACTION_KIND_KEY` in
`superset/versioning/changes/listener.py:214-242` are not registered.
2. Use the dashboard import endpoint `POST /api/v1/dashboard/import/`, which constructs an
`ImportDashboardsCommand` at `superset/dashboards/api.py:2176-2190`.
`ImportDashboardsCommand` subclasses `ImportModelsCommand`
(`superset/commands/dashboard/importers/v1/__init__.py:5-18`), whose `run` method is
decorated with `@transaction()` at `superset/commands/importers/v1/__init__.py:85-87`.
Inside `run`, after `self.validate()`, the code at
`superset/commands/importers/v1/__init__.py:98-100` imports `ACTION_KIND_IMPORT,
ACTION_KIND_KEY` and sets `db.session.info[ACTION_KIND_KEY] = ACTION_KIND_IMPORT` before
calling `self._import(...)` in the surrounding try/except block at lines 102-107.
3. The `@transaction` decorator in `superset/utils/decorators.py:235-263` commits on
success (`db.session.commit()` at line 2 of the second chunk) or rolls back on exception
(`db.session.rollback()` at line 5). Because no versioning change-record listeners are
attached when capture is disabled, neither `reset_processed_after_commit` nor
`reset_action_kind_after_rollback` is invoked, so `db.session.info` retains
`"_versioning_action_kind": "import"` after this import transaction completes.
4. In the same interpreter process (for example, in multi-app test setups), initialize
another Superset app with `ENABLE_VERSIONING_CAPTURE=True`, allowing `init_versioning()`
to execute the ON branch at `superset/initialization/__init__.py:47-76` and call
`register_change_record_listener()` at line 859 to attach `flush_change_records` to
`db.session`. On the first subsequent versioned write, such as a dashboard update via `PUT
/api/v1/dashboard/<id>` (`superset/dashboards/api.py:936-975`), the `after_flush` handler
calls `_stamp_action_kind_on_transaction` at
`superset/versioning/changes/listener.py:49-72`, which pops the stale `ACTION_KIND_KEY`
and stamps `version_transaction.action_kind='import'` for this normal edit, causing the
audit trail to present it as an import rather than a regular update.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/importers/v1/__init__.py
**Line:** 100:100
**Comment:**
*Logic Error: The import command sets a session-level action marker before running import work, but this file does not guarantee key removal itself. In the dark-launch default (`ENABLE_VERSIONING_CAPTURE=False`), listener-based cleanup is not active, so this marker can leak to later operations on the same scoped session and mis-tag them as imports. Add explicit cleanup around the `_import` call.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
eschutho
left a comment
eschutho
left a comment
There was a problem hiding this comment.
Posting on Elizabeth's behalf — this is her PR reviewer agent. Forward any pushback to her and she'll loop me back in.
Approving — this is base infra behind a dark-launch gate, and nothing below needs to hold up today's merge. That said, I found enough real correctness issues that I'd like to see them land as a fast-follow PR before the next release cuts, rather than open-ended: a few are ungated (they run on every deployment starting now, flag or not) and a few would corrupt version data or break a save the moment the flag flips on later. Grouped below so they're easy to turn into a checklist. All line numbers verified against PR HEAD 4f6254183.
Summary
This PR lands the base infrastructure for entity versioning — SQLAlchemy-Continuum wiring, two additive Alembic migrations (shadow tables for dashboards/slices/datasets plus a version_transaction/version_changes pair), a flush-time capture engine (baseline synthesis + field-level change records), and read-only GET /api/v1/{chart,dashboard,dataset}/<uuid>/versions/ endpoints — all inert behind ENABLE_VERSIONING_CAPTURE=False. With the gate off, Continuum's write listeners are detached at init and a save writes zero versioning rows; the read endpoints exist but return empty. Restore, retention, and UI are deferred to follow-ups.
The authorization story checks out: every new endpoint carries @protect(), maps to can_read via MODEL_API_RW_METHOD_PERMISSION_MAP, and goes through security_manager.raise_for_access on the specific entity before touching version data; version-UUID resolution is scoped per entity (UUIDv5 of entity_uuid:transaction_id), so cross-entity substitution 404s. Snapshot payloads exclude query_context, perm strings, and audit columns via the __versioned__ exclude lists. The migrations are genuinely additive, column parity with the live models is exact, and downgrade ordering is correct.
Ungated behavior changes (live on merge, regardless of the flag)
superset/daos/dataset.py:448-513
The _override_columns rewrite changes REST semantics beyond the PK preservation the docstring describes. Master's delete-all + bulk_insert_mappings reset every attribute not present in the payload to column defaults; the new in-place setattr loop preserves unspecified attributes (description, verbose_name, is_dttm, groupby, …) on name-matched columns — so a PUT ?override_columns=true with sparse column dicts now leaves stale values where it used to clear them. Two smaller shifts ride along: payload ids were previously honored on insert and are now stripped, and duplicate column_names in a payload previously raised IntegrityError but now silently last-win via incoming_by_name.
WDYT — are these intentional semantic changes? If so, could the docstring/UPDATING.md spell them out (the "override means replace" contract is one clients may rely on); if not, would clearing unspecified attributes on matched columns restore parity?
superset/commands/dashboard/update.py
UpdateDashboardCommand.run is now wrapped in no_autoflush unconditionally, so process_tab_diff, process_native_filter_diff, update_tags, and set_dash_metadata run without autoflush on every deployment even with capture off. Any query inside those helpers that relied on autoflush to see pending inserts/deletes would silently change behavior.
Could this be gated on ENABLE_VERSIONING_CAPTURE, or is there analysis showing those paths are flush-independent? Happy to keep as-is if the latter — a comment noting it would help.
superset/commands/dashboard/importers/v1/__init__.py
The overwrite-import path replaces the Core DELETE FROM dashboard_slices WHERE dashboard_id=… with an ORM dashboard.slices = […] assignment. The relationship load is filtered by the global soft-delete listener, so association rows pointing at soft-deleted slices aren't in the loaded collection and survive the reassignment — master purged them.
Small one, but WDYT about explicitly clearing rows for soft-deleted slices too, or confirming they're harmless to leave behind?
superset/models/helpers.py
Two global shifts worth a conscious ack, since they apply to all models/flows with the flag off: @validates("uuid") now coerces valid-UUID strings to UUID objects at assignment (pre-flush comparisons like obj.uuid == "abc-…" flip from True to False, and the coercion is asymmetric — malformed strings pass through), and reset_ownership stamps g.user onto created_by/changed_by instead of NULLing when a request context exists (overwrite-imports now attribute created_by to the importer). Both look deliberate and UPDATING.md mentions them — flagging mostly so reviewers land on them consciously. Totally fine to keep if intended.
superset/charts/api.py:373-374
if resolver := current_app.config.get("EXTRA_OWNERS_RESOLVER"): result["extra_owners"] = resolver(dash) — this config key appears nowhere else in the PR, master, or the docs. It's an undocumented extension point executing an operator-supplied callable inside chart GET, and it looks unrelated to versioning — probably worth pulling into the fast-follow so it can get its own docs/tests (or just come out), rather than living unexplained in the versioning PR.
Capture-engine correctness (latent until the gate flips on)
superset/versioning/baseline/insertion.py:59-76
The baseline INSERTs (version_transaction row + shadow row + child baselines) run on the session's connection with no SAVEPOINT, but the enclosing try/except swallows failures. On PostgreSQL a failed statement aborts the whole transaction, so swallowing the exception doesn't save the user — their flush/COMMIT then dies with InFailedSqlTransaction. That defeats the "versioning must never break a user's save" contract at exactly the layer with the widest failure surface. The codebase clearly knows the pattern — shadow_row_count (collection.py:133) and _persist_buffered_records (changes/listener.py:368) both wrap in begin_nested() for this exact reason; the baseline write path is the inconsistent omission. Same pattern applies to _stamp_action_kind_on_transaction (changes/listener.py:334-344).
Could the baseline insert path get the same begin_nested() treatment? Relatedly: if _baseline_children_for_parent fails after the parent row is written (insertion.py:142-149), the parent's shadow count is nonzero forever, so the count == 0 gate in baseline/listener.py:117-118 never retries — pre-versioning children then permanently lack baselines and their later edits produce predecessor-less UPDATE rows. Making parent+child baselining atomic under one savepoint would close both at once. WDYT?
superset/versioning/changes/shadow_queries.py:156-181
prior_tx for dataset child diffs is MAX(table_columns_version.transaction_id) < tx, falling back to the metrics shadow only when the columns result is None. If a dataset is baselined at tx B, a metric is edited at M1 (> B, writes only sql_metrics_version), and any later save happens at N, then prior_tx = B (columns untouched since B) even though M1 exists — so pre_metrics is read at pre-M1 state and M1's metric change is re-emitted under transaction N (duplicated/conflated change records for a very common edit sequence).
Should this be max(col_max, metric_max) across both shadows rather than columns-first-with-fallback?
superset/versioning/changes/listener.py:432-438
Once a flush persists records for tx N, N enters the processed set, and any later flush in the same DB transaction discards its freshly-buffered records. A command that mutates a dashboard, triggers a mid-command autoflush (flush 1 persists records for tx N), mutates further, then commits (flush 2, same Continuum tx) loses flush 2's diffs — while Continuum's shadow row is updated to the final state, so the shadow history and change log disagree. The comment frames the discard as dropping stale leftovers, but the buffer can hold genuinely new records.
Would a merge-with-dedup (or per-entity sequence continuation) on the second flush be safer than the blanket discard? Or is there an invariant that record-bearing flushes can't happen twice per transaction that could be documented?
superset/versioning/diff.py:256-258 and :834-859
cap_records groups by path[:1] to enforce MAX_RECORDS_PER_FIELD, but layout records use path=[node_id] — every layout node is its own group, so the cap never binds across a position_json rewrite. The docstring explicitly claims "a thousand-node layout churn … is collapsed"; as written it isn't, and a large dashboard layout rewrite writes one version_changes row per node per save — the unbounded blowup the cap exists to prevent.
Could layout-kind records be grouped under a synthetic ("position_json",) key (or an overall per-entity cap added)?
superset/versioning/diff.py:491-502
_metric_key returns None for non-dict metrics, but chart params["metrics"] is commonly a list of plain strings (saved-metric names). Those all fall back to positional keys, so removing/reordering saved metrics emits mislabeled edit-at-index records instead of add/remove by name. _dimension_key right below already handles str.
Could _metric_key do the same?
superset/versioning/baseline/dirty.py:184-189
The docstring above (lines 156-169) explains carefully why uuid must never be the force-dirty flag column — a documented prior failure where the UUIDType/BLOB round-trip breaks an FK check — yet the fallback chain is still description → uuid → col_keys[0]. Dead code today since all three versioned parents have description, but any future versioned model without one would walk straight into the failure mode the comment warns about.
Small suggestion — could the chain skip from description straight to col_keys[0] so the documented footgun is unreachable?
superset/versioning/changes/shadow_queries.py:301-315 and changes/listener.py:412
Two smaller scope gaps, likely fine to defer but worth confirming as deliberate: (a) a dashboard baselined with zero attached slices has no M2M shadow rows, so the first chart added yields prior_tx is None → skip → no "added chart" record, even though this isn't the dashboard's first edit; (b) the change-record listener iterates only session.dirty, so entity deletions never produce a version_changes row (Continuum's own delete marker still lands in the shadow table). Intentional for the base-infra scope?
Per-flush query fan-out (superset/versioning/factory.py:271, changes/state.py:159, changes/shadow_queries.py)
Nothing quadratic, but each capture layer issues its own independent SELECTs per dirty/affected entity in a flush — one per updated row for the skip-unmodified check, one per dirty parent for the pre-state read, and several per affected dataset/dashboard for the shadow queries. Noise for a single-chart save; for a bulk import or a save touching hundreds of entities in one transaction it stacks up to several O(N) round-trip passes.
Totally fine for a dark launch — just wondering whether a load test on a large dashboard/import is planned as part of the validation before the flag-flip PR?
API surface
superset/versioning/schemas.py:29-35
VersionChangedBySchema returns id, username, first_name, last_name. The existing chart/dashboard payloads deliberately expose only first/last name — usernames were removed from those payloads as prior hardening — so /versions/ reintroduces username (and internal user id) exposure to any principal with read access to the entity.
Could username/id be dropped for consistency with the rest of the API?
superset/versioning/queries.py (list_versions)
The list endpoint returns the full version history with no limit/pagination — unbounded for a heavily-edited entity until retention pruning lands. Worth a simple limit now, or fine to defer alongside the retention follow-up?
ETag (informational, no action needed)
The new ETag is emit-only — no If-Match/If-None-Match handling exists yet, so there's no lost-update protection or 304 path. Presumably that's the follow-up PR; flagging only so it's a conscious sequencing decision. The header itself is safe (same value for all authorized users, no Cache-Control: public added).
Migrations
superset/migrations/versions/2026-05-28_19-50_56cd24c07170_add_versioning_tables.py:112-119, 328-334 and superset/versioning/changes/table.py:43
version_transaction.id/version_changes.id are BigInteger autoincrement PKs. On SQLite only a literal INTEGER PRIMARY KEY gets rowid autoincrement — a bare BIGINT PK fails inserts with NOT NULL constraint failed. This works today only because sqlalchemy_continuum registers a global @compiles(BigInteger, "sqlite") → INTEGER override as an import side effect, and Superset's boot path always imports it before migrations run. That's a load-bearing, undocumented coupling.
Would sa.BigInteger().with_variant(sa.Integer(), "sqlite") on both make this self-contained (plus a one-line comment)?
Both migration files — MySQL retry safety (minor)
The docstring justifies raw op.create_table only for version_transaction's sequence, but all 7 shadow tables and the indexes (including migration 2's upgrade(), whose downgrade() is inspector-guarded) use unguarded raw ops. Since MySQL DDL isn't transactional, a mid-migration failure leaves partial schema that a retried alembic upgrade can't resume past ("table already exists"). Worth switching the non-sequence tables/indexes to the migrations.shared.utils guarded helpers?
superset/migrations/versions/2026-06-03_12-00_8f3a1b2c4d5e_shadow_live_row_indexes.py — blocking index builds (informational)
Plain op.create_index, no CONCURRENTLY/batching. Inert on the expected path (both migrations ship together against empty tables), but if this ever runs after the shadow tables have accumulated real history — e.g. a delayed follow-on deploy — the index builds take blocking locks proportional to table size. No existing Superset migration uses CONCURRENTLY either, so not a convention break; a comment for future deployers would do. Happy to keep as-is.
Test coverage
Coverage is strong where it exists and refreshingly non-mocked — the integration suite runs with ENABLE_VERSIONING_CAPTURE=True so capture is exercised broadly, the capture-off test does a real PUT and asserts zero rows across all version tables (with a capture-on control guarding against vacuous passes), and the listener detach/re-attach round-trip runs against the real SQLAlchemy event registry. Gaps worth a look, roughly in priority order:
- No test forces an exception inside the capture path during a flush to prove the save survives — the fail-open machinery (
changes/listener.py:339-374savepoint recovery, the broad excepts) is the highest-risk untested behavior in the PR, and it's where the PostgreSQL-poisoning finding above lives. - No test asserts diff content —
changesitems are only checked for havingoperation/kindkeys, never that renaming a chart yields aslice_namerecord with the right from/to values. Theprior_txand_metric_keybugs above would both sail through the current suite. - Authorization denial is tested only for Gamma × chart × list — no denial coverage for
get_version, dashboards, datasets, or anonymous. - No cross-entity version-UUID test (chart A's version UUID against chart B) — the code defends via UUIDv5 derivation, but nothing pins it.
- The capture-on control test soft-skips if the "Boys" slice is missing (
capture_disabled_tests.py:148-149) — the one test guarding against vacuous zero-row assertions can silently vacate on fixture drift; a hard assert (like its capture-off sibling uses) seems safer.
Suggested fast-follow checklist
Merging this as-is looks right to me — it's gated off, additive, and the auth/migration fundamentals are sound. For the follow-up PR before the next release cuts, here's what I'd prioritize, roughly in order:
_override_columnssemantics (daos/dataset.py:448-513) — this one's user-facing and ungated, so it's already live; worth confirming with Mike whether it's intentional and documenting it either way.- PostgreSQL savepoint gap in baseline inserts (
baseline/insertion.py:59-76) — would break a user's save the moment the flag flips on, contradicting the PR's own "never break a save" contract. EXTRA_OWNERS_RESOLVER(charts/api.py:373-374) — unrelated rider, cheap to pull out or properly scope.prior_txmetric-diff bug and thecap_recordslayout-cap bypass (shadow_queries.py:156-181,diff.py:256-258) — both would produce incorrect/unbounded version data once capture is on.- Everything else in this review (the remaining capture-engine items, the migration portability notes, the test gaps) — nice to have before the flag flips, but lower urgency than 1–4.
No need to block today's merge on any of this — happy to track it as a linked follow-up issue if that's easier than a checklist in the PR description.
|
OMG that review comment @eschutho :P Love the line at the top saying it's AI. Imagine how long you'd be up at night typing all that if it weren't! Anyway, merging now before the length of the thread breaks github ;) |
A Celery beat task that prunes version_transaction / version_changes / shadow rows beyond the configured retention window, so the version tables don't grow unbounded once capture is enabled. Preserves live child/M2M rows, batches the deletes with bind/retry hardening, and adds an index on version_transaction.issued_at for the prune scan. Stacked on the versioning base infrastructure (apache#41176). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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>
A Celery beat task that prunes version_transaction / version_changes / shadow rows beyond the configured retention window, so the version tables don't grow unbounded once capture is enabled. Preserves live child/M2M rows, batches the deletes with bind/retry hardening, and adds an index on version_transaction.issued_at for the prune scan. Stacked on the versioning base infrastructure (apache#41176). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
A Celery beat task that prunes version_transaction / version_changes / shadow rows beyond the configured retention window, so the version tables don't grow unbounded once capture is enabled. Preserves live child/M2M rows, batches the deletes with bind/retry hardening, and adds an index on version_transaction.issued_at for the prune scan. Stacked on the versioning base infrastructure (apache#41176). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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>
A Celery beat task that prunes version_transaction / version_changes / shadow rows beyond the configured retention window, so the version tables don't grow unbounded once capture is enabled. Preserves live child/M2M rows, batches the deletes with bind/retry hardening, and adds an index on version_transaction.issued_at for the prune scan. Stacked on the versioning base infrastructure (apache#41176). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A Celery beat task that prunes version_transaction / version_changes / shadow rows beyond the configured retention window, so the version tables don't grow unbounded once capture is enabled. Preserves live child/M2M rows, batches the deletes with bind/retry hardening, and adds an index on version_transaction.issued_at for the prune scan. Stacked on the versioning base infrastructure (apache#41176). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A Celery beat task that prunes version_transaction / version_changes / shadow rows beyond the configured retention window, so the version tables don't grow unbounded once capture is enabled. Preserves live child/M2M rows, batches the deletes with bind/retry hardening, and adds an index on version_transaction.issued_at for the prune scan. Stacked on the versioning base infrastructure (apache#41176). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A Celery beat task that prunes version_transaction / version_changes / shadow rows beyond the configured retention window, so the version tables don't grow unbounded once capture is enabled. Preserves live child/M2M rows, batches the deletes with bind/retry hardening, and adds an index on version_transaction.issued_at for the prune scan. Stacked on the versioning base infrastructure (apache#41176). Co-Authored-By: Claude Opus 4.8 (1M context) <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>
3 participants
SUMMARY
Lands the entity-versioning base infrastructure (schema + SQLAlchemy-Continuum wiring) gated off by default, so it deploys inert — a "base infra, dark" PR. Capture is activated later by flipping the gate default on, once validated in production.
With
ENABLE_VERSIONING_CAPTURE=False(the shipped default), the before-flush capture listeners are not armed, so a save writes zeroversion_transactionrows, zeroversion_changesrows, and zero*_versionshadow rows — proven by a behavioral test. The migrations are additive and inert; the read-only/versions/list + get endpoints are wired but return empty until capture is enabled, and the new entity-PUTversion fields all come backnull(with no extra version queries fired) while the gate is off.What's included (all inert with the gate off):
56cd24c07170_add_versioning_tables— createsversion_transaction(audit log keyed by Continuum's per-flush transaction),version_changes(field-level diff log), and the*_versionshadow tables (dashboards_version,slices_version,tables_version, and their child tables).8f3a1b2c4d5e_shadow_live_row_indexes— additive indexes on the shadow tables' live rows.Both have a real, tested
downgrade().make_versioned(), the version-transaction factory, the versioning Flask plugin, and thesuperset/versioning/capture machinery (baseline, change-records, diff engine).ENABLE_VERSIONING_CAPTUREgate (default off), a permanent operational kill-switch.GET /api/v1/{chart,dashboard,dataset}/<uuid>/versions/andGET .../versions/<version_uuid>/.PUTresponse and anETagheader on entity reads/writes (see API).reset_ownership, UUID coercion) so existing import/clone paths behave correctly with the versioned mappers present.Deferred to follow-ups: version restore, the cross-entity activity view, version-history retention/prune, and the frontend UI.
THIS IS A RELEASE TOGGLE / OPS KILL-SWITCH, NOT A LONG-LIVED FEATURE FLAG
ENABLE_VERSIONING_CAPTUREis not a long-lived product feature flag. In the Continuous Delivery taxonomy it begins as a Release Toggle — its only job is to decouple deploy from release, so this not-yet-validated, high-blast-radius change (Continuum writes on every flush) can land onmasterand ship dark, then be switched on once trusted. It does not parameterize product behavior, gate an experiment, or vary per tenant.Because capture is an active write path with a measurable save-path cost, once validated the default flips off→on and the switch is retained as a permanent Ops Toggle — an operational kill-switch giving a ~30-second recovery (flip off, restart workers) if a versioning-induced regression appears in production, instead of revert-and-redeploy.
References: Martin Fowler — Feature Toggles, "Release Toggles" / "Ops Toggles".
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend, gated off; no user-visible change on deploy.
API
All endpoints below are read-only and key on the entity UUID (not the integer pk — note this differs from the dataset API's other routes). They require the resource's read permission via
@protect()and run the per-object access gate (security_manager.raise_for_access). Marshmallow schemas live insuperset/versioning/schemas.pyand are registered in each API'sopenapi_spec_component_schemas.1. List version history
GET /api/v1/chart/<uuid>/versions/GET /api/v1/dashboard/<uuid>/versions/GET /api/v1/dataset/<uuid>/versions/uuid_str(entity UUID).200response:{ "result": [ /* VersionListItemSchema, oldest first */ ], "count": 0 }ETagheader (current liveversion_uuid).400invalid UUID ·403no access ·404entity not found / no version rows.2. Get a single version snapshot
GET /api/v1/{chart,dashboard,dataset}/<uuid>/versions/<version_uuid>/uuid_str(entity UUID),version_uuid_str(aversion_uuidfrom the list endpoint).200response:{ "result": { "/* entity scalar fields at the target version */": "...", "_version": { "/* VersionListItemSchema */": "..." } } }ETagheader.400/403/404as above.VersionListItemSchema(superset/versioning/schemas.py):version_uuidversion_numbertransaction_idoperation_typebaseline|update|delete(restore surfaces as an ordinaryupdate).issued_atchanged_by{id, username, first_name, last_name}; null for CLI/Celery/import commits.changes{kind, path, from_value, to_value}; empty forbaseline.3. Entity
PUT— new response fields +ETagPUT /api/v1/{chart,dashboard,dataset}/<pk>gains six nullable body fields and anETagheader. All arenull(and no version queries are issued) when capture is off:old_version/new_versionversion_numberof the live row before / after the update.old_transaction_id/new_transaction_idold_version_uuid/new_version_uuidversion_uuidbefore / after.The entity single-object
GET /<pk>response also carries anETagheader (current liveversion_uuid).CORS_OPTIONSnow setsexpose_headers: ["ETag"]so cross-originfetch()clients can read it.TESTING INSTRUCTIONS
QA NOTES
version_transaction,version_changes, and*_versionshadow rows. Confirm no save-path latency regression.PUTon each entity returns the six version fields allnull, and issues no extra version queries./versions/endpoints, capture off: list returns{"result": [], "count": 0}(or404if the entity isn't found); the get-single-version endpoint404s. Endpoints reachable read-only.ENABLE_VERSIONING_CAPTURE=True, restart workers): a save mints aversion_transaction,version_changesrecords, and a shadow row;/versions/returns populated rows oldest-first with acount; the get endpoint returns the snapshot +_versionblock.GET, entityPUT, and both/versions/responses carry anETag(current liveversion_uuid) when capture is on; absent when there are no version rows. Verify cross-originfetch()can read it./versions/honors read permission + per-object access — no access →403; bad UUID →400; unknown entity/version →404.56cd24c07170and8f3a1b2c4d5eupgrade cleanly, are additive, anddowngrade()restores prior schema. Test on Postgres and MySQL.ADDITIONAL INFORMATION
ENABLE_VERSIONING_CAPTURE(default off this release; a release toggle that flips on after soak and is retained as a permanent operational kill-switch — not removed).56cd24c07170,8f3a1b2c4d5e)/versions/endpoints + newPUTresponse fields, gated off by default)Rebased onto current
master(the composite-PK change #39859 merged standalone; its migration + tests are de-duped out of this branch).