bito-code-review[bot] commented on code in PR #44514:
URL: https://github.com/apache/superset/pull/44514#discussion_r4098570602
##########
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:
<!-- Bito Reply -->
The documentation update in `docs/docs/using-superset/version-history.mdx`
now includes the retention configuration details. It specifies that
`VERSION_HISTORY_RETENTION_DAYS` supports a range of 0 to 36500 days, defaults
to 30 days, and notes that invalid values (which would be outside this range)
defer cleanup, addressing the reviewer's concern about documenting the valid
range and behavior.
**docs/docs/using-superset/version-history.mdx**
```
+## 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.
```
##########
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:
<!-- Bito Reply -->
The reviewer's suggestion is valid. The documentation mentions 'downgrade
grace protection' in the context of archived entity cleanup, but as the
reviewer noted, there is no corresponding implementation or configuration for
such a feature in the deletion-retention codebase. Removing this clause is
recommended to avoid documenting non-existent functionality.
**docs/docs/using-superset/version-history.mdx**
```
This does not change explicit force-purge or supply downgrade grace
protection.
```
##########
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:
<!-- Bito Reply -->
The suggestion to strip `SUPERSET_CONFIG_PATH` and `superset_config` before
`runpy.run_path` is correct and appropriate. Re-executing the configuration
module without isolating it from local environment overrides can lead to
inconsistent test results, as the local environment's configuration may
inadvertently influence the `loaded` dictionary. Applying this cleanup ensures
that the tests remain isolated and deterministic.
**tests/unit_tests/config_test.py**
```
monkeypatch.delenv("SUPERSET_CONFIG_PATH", raising=False)
monkeypatch.delenv("superset_config", raising=False)
loaded: dict[str, Any] = runpy.run_path(config.__file__)
```
##########
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:
<!-- Bito Reply -->
The decision to maintain the sentinel value locally is reasonable given the
existing validation logic and the goal of avoiding unnecessary coupling across
modules. Since the parsing and task boundaries already explicitly handle this
value with tests, the current implementation remains robust and maintainable
for the scope of this pull request.
**superset/tasks/deletion_retention.py**
```
cutoff: datetime = (
datetime.now()
if window_days == -1
```
##########
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:
<!-- Bito Reply -->
The implementation in the provided code snippet is appropriate. By
explicitly handling negative values and returning 0, the code avoids the
potential risk of purging records sooner than intended, which aligns with the
requirement to fail closed on invalid input. This approach is consistent with
the design goal of preventing unintended data loss due to configuration errors.
**superset/config.py**
```
if retention_days < -1:
logger.warning("Invalid negative VERSION_HISTORY_RETENTION_DAYS;
skipping")
return 0
```
--
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]
