mikebridge commented on code in PR #44514:
URL: https://github.com/apache/superset/pull/44514#discussion_r4098569101
##########
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:
Addressed in the current head: the version-history and soft-delete retention
docs both state the supported -1 through 36500 range, the absent-value default
of 30 days, and that invalid supplied values defer cleanup.
##########
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:
Addressed in the current head: the docs explicitly say cleanup does not
supply downgrade-grace protection. This PR does not implement a grace period.
##########
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:
Addressed in the current head: both runpy.run_path tests remove
SUPERSET_CONFIG_PATH and mask superset_config before loading the config module.
##########
superset/tasks/deletion_retention.py:
##########
@@ -133,7 +133,11 @@ def _purge_impl(window_days: int, dry_run: bool) ->
dict[str, Any]:
# UTC-derived cutoff would shift the retention window by the server's
# timezone offset, purging early west of UTC. If deleted_at ever moves
# to UTC-aware, this must move with it.
- cutoff = datetime.now() - timedelta(days=window_days)
+ cutoff: datetime = (
+ datetime.now()
+ if window_days == -1
Review Comment:
I’m keeping -1 as the documented immediate-eligibility value for this PR.
The parsing and task boundaries each validate it explicitly, with tests;
sharing a new constant across config, CLI, and task modules would add coupling
without changing the behavior. We can revisit the naming separately if it
becomes hard to maintain.
##########
superset/config.py:
##########
@@ -1761,22 +1791,24 @@ def sync_theme_logo_href(
def _parse_version_history_retention_days() -> int:
"""Parse the retention window without making invalid input fatal."""
- value: str | None =
os.environ.get("SUPERSET_VERSION_HISTORY_RETENTION_DAYS")
+ value: str | None = os.environ.get("VERSION_HISTORY_RETENTION_DAYS")
if value is None:
return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
try:
- retention_days = int(value)
+ retention_days: int = int(value)
except ValueError:
logger.warning(
- "Invalid SUPERSET_VERSION_HISTORY_RETENTION_DAYS=%r; using %d",
+ "Invalid VERSION_HISTORY_RETENTION_DAYS=%r; using %d",
value,
_DEFAULT_VERSION_HISTORY_RETENTION_DAYS,
)
return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
+ if retention_days < -1:
+ logger.warning("Invalid negative VERSION_HISTORY_RETENTION_DAYS;
skipping")
+ return 0
Review Comment:
Thanks. We intentionally do not fall back to 30 for invalid retention input:
that could purge records sooner than an operator intended. Both soft-delete and
version-history cleanup now fail closed on invalid values, while -1 remains
explicit immediate eligibility and 0 is explicit disable. The behavior is
documented and covered by tests in d658013d57.
--
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]
