bito-code-review[bot] commented on code in PR #44514:
URL: https://github.com/apache/superset/pull/44514#discussion_r4076267706
##########
superset/versioning/utils.py:
##########
@@ -44,8 +49,25 @@ def capture_enabled() -> bool:
would let this gate pass while listeners stay detached, producing
exactly the untracked write it exists to prevent. Restart the process
(or re-run ``init_versioning()``) after changing the flag.
+
+ VERSIONING_CAPTURE_PREDICATE is a separate runtime decision, consulted by
+ the baseline/change listeners and CaptureUnitOfWork as well as restore.
+ The host owns tenant identity, bounded transaction memoization and expected
+ service-failure handling. Database/programming errors are not suppressed.
"""
- return bool(current_app.config.get("ENABLE_VERSIONING_CAPTURE", False))
+ if not current_app.config.get("ENABLE_VERSIONING_CAPTURE", False):
+ return False
+ predicate: Callable[[Session], bool] | None = current_app.config.get(
+ "VERSIONING_CAPTURE_PREDICATE"
+ )
+ if predicate is None:
+ return True
+ if session is None:
+ # Deferred because extensions configures the UnitOfWork at import time.
+ from superset.extensions import db # pylint:
disable=import-outside-toplevel
+
+ session = db.session()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Implicit session fallback in gate</b></div>
<div id="fix">
When `capture_enabled()` is called without a session it now falls back to
`db.session()`, so a request-scoped caller like
`RestoreVersionCommand.validate` consults `VERSIONING_CAPTURE_PREDICATE` with
whatever ambient session exists, and the gate can materialize a session where
the caller previously ran session-free. Since config.py requires the
predicate's decision to be stable per transaction, prefer passing the caller's
session explicitly.
</div>
</div>
<small><i>Code Review Run #1af5f9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
docs/docs/using-superset/version-history.mdx:
##########
@@ -30,15 +30,59 @@ FEATURE_FLAGS = {"VERSION_HISTORY": False}
ENABLE_VERSIONING_CAPTURE = False
```
-Restart Superset and its workers for the capture change to take effect.
Existing
-history remains readable while capture is off, but **Restore** is unavailable
-(404).
+Restart Superset and its workers for the capture change to take effect.
+With `VERSION_HISTORY` off, the version-list, version-snapshot, activity, and
+version-restore endpoints return 404 for callers who pass the existing route
+permissions. The flag does not grant access: when enabled, route permissions
+and object-level editorship are still required.
+
+Disabling only capture leaves existing history readable when `VERSION_HISTORY`
+is enabled, but **Restore** is unavailable (404). Disabling only
+`VERSION_HISTORY` does not stop capture or retention, and does not change
+ordinary chart, dashboard, or dataset CRUD or soft-delete recovery.
+
+Hosts can separately configure `VERSIONING_CAPTURE_PREDICATE`, a callable
+receiving the SQLAlchemy session. Its default, `None`, preserves capture
behavior.
+A false decision skips baseline, shadow, association-history, and change-record
+writes while ordinary ORM saves still persist. Version restore returns 404 when
+capture is denied. The startup `ENABLE_VERSIONING_CAPTURE` kill switch still
wins.
+The host must keep decisions tenant-local and stable for a transaction, handle
+expected service outages, and bound any cache to the transaction/request. This
+hook does not govern retention or delete existing history.
+
+Host integrations can require
`superset.versioning.utils.HOST_POLICY_API_VERSION`
+equal to `1` before installing policies. This contract includes the version API
+feature gates, transaction-scoped capture and restore, canonical retention key,
+and authoritative soft-delete retention callback. A downstream host must adopt
a
+core revision providing the entire contract, not only configure a UI flag.
+
+Imports and background ORM writes use the same predicate; hosts must supply
their
+tenant context there too. Bulk SQL retains its existing capture limitations.
+After re-enabling, skipped edits are not reconstructed. If an entity has no
+history, its first enabled edit records the existing pre-edit baseline of its
+then-current state; an entity with history resumes with its next captured
change.
Disable them together: capture off with the UI left on gives a panel that
stops filling — an empty or stale history misrepresents the entity as
unchanged. History only accrues while capture is on; edits made while it was
off are not reconstructed.
+## Retention
+
+History retention is configured independently with
`VERSION_HISTORY_RETENTION_DAYS`
+in `superset_config.py` or the environment variable of the same name. Its
default
+is 30 days; set it to `0` to disable pruning. The scheduled retention task
preserves
+live versions and prunes eligible closed history older than the configured
window.
+Changing retention does not enable history access or capture.
+
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Retention cap undocumented, silent fallback</b></div>
<div id="fix">
The doc states the `SOFT_DELETE_RETENTION_DAYS` range (0-36500) but omits
that `VERSION_HISTORY_RETENTION_DAYS` has the same cap:
`_parse_version_history_retention_days` (`superset/config.py:1806`) silently
falls back to 30 days for values above 36500, so a host setting 40000 gets far
shorter retention than intended. Document the valid range.
</div>
</div>
<small><i>Code Review Run #1af5f9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
docs/docs/using-superset/version-history.mdx:
##########
@@ -30,15 +30,59 @@ FEATURE_FLAGS = {"VERSION_HISTORY": False}
ENABLE_VERSIONING_CAPTURE = False
```
-Restart Superset and its workers for the capture change to take effect.
Existing
-history remains readable while capture is off, but **Restore** is unavailable
-(404).
+Restart Superset and its workers for the capture change to take effect.
+With `VERSION_HISTORY` off, the version-list, version-snapshot, activity, and
+version-restore endpoints return 404 for callers who pass the existing route
+permissions. The flag does not grant access: when enabled, route permissions
+and object-level editorship are still required.
+
+Disabling only capture leaves existing history readable when `VERSION_HISTORY`
+is enabled, but **Restore** is unavailable (404). Disabling only
+`VERSION_HISTORY` does not stop capture or retention, and does not change
+ordinary chart, dashboard, or dataset CRUD or soft-delete recovery.
+
+Hosts can separately configure `VERSIONING_CAPTURE_PREDICATE`, a callable
+receiving the SQLAlchemy session. Its default, `None`, preserves capture
behavior.
+A false decision skips baseline, shadow, association-history, and change-record
+writes while ordinary ORM saves still persist. Version restore returns 404 when
+capture is denied. The startup `ENABLE_VERSIONING_CAPTURE` kill switch still
wins.
+The host must keep decisions tenant-local and stable for a transaction, handle
+expected service outages, and bound any cache to the transaction/request. This
+hook does not govern retention or delete existing history.
+
+Host integrations can require
`superset.versioning.utils.HOST_POLICY_API_VERSION`
+equal to `1` before installing policies. This contract includes the version API
+feature gates, transaction-scoped capture and restore, canonical retention key,
+and authoritative soft-delete retention callback. A downstream host must adopt
a
+core revision providing the entire contract, not only configure a UI flag.
+
+Imports and background ORM writes use the same predicate; hosts must supply
their
+tenant context there too. Bulk SQL retains its existing capture limitations.
+After re-enabling, skipped edits are not reconstructed. If an entity has no
+history, its first enabled edit records the existing pre-edit baseline of its
+then-current state; an entity with history resumes with its next captured
change.
Disable them together: capture off with the UI left on gives a panel that
stops filling — an empty or stale history misrepresents the entity as
unchanged. History only accrues while capture is on; edits made while it was
off are not reconstructed.
+## Retention
+
+History retention is configured independently with
`VERSION_HISTORY_RETENTION_DAYS`
+in `superset_config.py` or the environment variable of the same name. Its
default
+is 30 days; set it to `0` to disable pruning. The scheduled retention task
preserves
+live versions and prunes eligible closed history older than the configured
window.
+Changing retention does not enable history access or capture.
+
+Archived entity cleanup is separate: `SOFT_DELETE_RETENTION_DAYS` defaults to
+30 days and accepts the same-name environment seed (0 through 36500; 0 disables
+scheduled purge). A host can install `SOFT_DELETE_RETENTION_DAYS_FUNC` to
supply
+an authoritative integer window before the stored CLI override. Invalid or
+unavailable callback results defer purge with 0; they do not use stored values.
+Without that callback, the stored CLI window still takes precedence over
config.
+This does not change explicit force-purge or supply downgrade grace protection.
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Undocumented downgrade-grace clause</b></div>
<div id="fix">
The clause 'supply downgrade grace protection' has no counterpart in the
deletion-retention code: grep for `grace`/`downgrade` across
`superset/tasks/deletion_retention.py`,
`superset/commands/deletion_retention/`, and
`superset/cli/deletion_retention.py` returns nothing. The force-purge half is
real (`tasks/deletion_retention.py:247`). Drop or reword the unverifiable
clause.
</div>
</div>
<small><i>Code Review Run #1af5f9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/config_test.py:
##########
@@ -47,17 +49,73 @@
}
[email protected](
+ ("value", "expected"),
+ [
+ (None, 30),
+ ("360", 360),
+ ("0", 0),
+ ("-1", -1),
+ ("36500", 36500),
+ ("36501", 30),
+ ("30d", 30),
+ ],
+)
+def test_version_history_retention_env_loads_application_config(
+ monkeypatch: pytest.MonkeyPatch, value: str | None, expected: int
+) -> None:
+ """The canonical environment key populates integer application config."""
+ from superset import config
+
+ monkeypatch.delenv("VERSION_HISTORY_RETENTION_DAYS", raising=False)
+ if value is not None:
+ monkeypatch.setenv("VERSION_HISTORY_RETENTION_DAYS", value)
+ monkeypatch.setenv("SUPERSET_VERSION_HISTORY_RETENTION_DAYS", "180")
+ loaded: dict[str, Any] = runpy.run_path(config.__file__)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>runpy config override taint</b></div>
<div id="fix">
`runpy.run_path(config.__file__)` re-executes the local-override blocks at
`superset/config.py:3484-3533`: a developer's `SUPERSET_CONFIG_PATH` or an
importable `superset_config` taints `loaded`, so assertions at lines 77-79 can
false-fail or false-pass locally. `config_swagger_ui_test.py:40-46` strips
exactly these vars for the same reason; please strip them before both
`run_path` calls.
</div>
</div>
<small><i>Code Review Run #1af5f9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
