bito-code-review[bot] commented on code in PR #44892:
URL: https://github.com/apache/superset/pull/44892#discussion_r4185308033
##########
tests/unit_tests/tasks/test_deletion_retention.py:
##########
@@ -330,6 +330,50 @@ class UnsupportedModel(SoftDeleteMixin):
engine.dispose()
[email protected]("dry_run", [False, True])
+def test_scan_failure_does_not_prevent_supported_models_from_purging(
+ app_config: Config,
+ monkeypatch: pytest.MonkeyPatch,
+ dry_run: bool,
+) -> None:
+ """A root whose scan raises is counted and skipped, not fatal to the run.
+
+ The eligible-id scan sits outside the per-entity handler, so without this
+ one unreadable root would abort the pass and the roots that could have
+ purged never would.
+ """
+ # avoid app-init regression: model helpers require the app_config fixture
first.
+ from superset.models.helpers import SoftDeleteMixin
+ from superset.tasks import deletion_retention as task
+
+ supported_models: list[type[SoftDeleteMixin]] =
list(task.purge_policy_registry())
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Registry order mismatch</b></div>
<div id="fix">
`supported_models = list(task.purge_policy_registry())` takes the keys of a
`Mapping` (purge_policy.py:987), whose order is unrelated to the task's
iteration order `_soft_delete_models()` (deletion_retention.py:151).
`supported_models[0]` may therefore not be the first model `_purge_roots`
processes, so `failing_scan` may never raise and `result["scan_failures"] == 1`
fails. The sibling test pins `_registered_subclasses` (line 390) for exactly
this reason.
</div>
</div>
<small><i>Code Review Run #1d7448</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
##########
superset/commands/deletion_retention/purge_policy.py:
##########
@@ -860,16 +894,131 @@ def declare(
cleanup_permission=cleanup_dataset_permission,
),
}
- return validate_unique_root_policies(registry.values())
+ return tuple(registry.values())
+
+
+def _host_purge_policies(
+ provider: Callable[[], Any] | None,
+) -> tuple[PurgeEntityPolicy, ...]:
+ """Return the purge policies a host installed for its own roots.
+
+ A host distribution can carry ``SoftDeleteMixin`` entities this package
+ cannot import. The retention task discovers those entities through the
+ mixin registry, so without a policy they reach the cascade as an
+ unsupported model. The host therefore declares their purge behavior and
+ installs it under ``PURGE_POLICIES_FUNC``.
+
+ Host boundary: everything about a host policy is settled here, where it is
+ admitted -- an unavailable provider, a malformed payload, a collision with
+ a root declared in this package, two declarations for one root, and a
+ declaration the shared cleanup could not execute. Each is logged and
+ dropped, so that root is reported as unsupported instead of failing row by
+ row in a scheduled run. A broken host declaration must not take the purge
+ down with it, and must never redefine how a chart, dashboard or dataset is
+ purged.
+ """
+ if provider is None:
+ return ()
+ try:
+ provided: Any = provider()
+ except Exception: # pylint: disable=broad-except
+ logger.exception(
+ "purge_policy: %s is unavailable; keeping built-in roots only",
+ HOST_POLICIES_CONFIG_KEY,
+ )
+ return ()
+ if not isinstance(provided, (list, tuple)) or not all(
+ isinstance(policy, PurgeEntityPolicy) for policy in provided
+ ):
+ logger.error(
+ "purge_policy: %s returned %s; expected a sequence of
PurgeEntityPolicy",
+ HOST_POLICIES_CONFIG_KEY,
+ type(provided).__name__,
+ )
+ return ()
+ builtin_roots: frozenset[type[Any]] = frozenset(
+ policy.model for policy in _builtin_purge_policies()
+ )
+ declared: dict[type[Any], int] = {}
+ for policy in provided:
+ declared[policy.model] = declared.get(policy.model, 0) + 1
+ # Which of two declarations for one root is authoritative is undecidable,
+ # and the loser would still delete rows. Dropping both leaves the model
+ # reported as unsupported, which is the recoverable outcome. Resolving it
+ # here also keeps the duplicate away from validate_unique_root_policies,
+ # whose ValueError would abort the whole scheduled run.
+ duplicated: list[type[Any]] = [
+ model for model, count in declared.items() if count > 1
+ ]
+ for model in duplicated:
+ logger.error(
+ "purge_policy: %s declares %d policies for %s; ignoring all of
them",
+ HOST_POLICIES_CONFIG_KEY,
+ declared[model],
+ model.__name__,
+ )
+ accepted: list[PurgeEntityPolicy] = []
+ for policy in provided:
+ if policy.model in builtin_roots:
+ logger.error(
+ "purge_policy: host policy for built-in root %s ignored",
+ policy.model.__name__,
+ )
+ continue
+ if policy.model in duplicated:
+ continue
+ try:
+ _validated_policy(policy)
+ except Exception as ex: # pylint: disable=broad-except
+ # Validated at admission rather than when a row is purged: a
+ # declaration the cleanup cannot execute would otherwise fail once
+ # per eligible row, counted as a cascade failure, and stay
+ # invisible until something aged past the window.
+ logger.error(
+ "purge_policy: host policy for %s rejected: %s",
+ policy.model.__name__,
+ ex,
+ )
+ continue
+ accepted.append(policy)
+ return tuple(accepted)
+
+
+def purge_policy_registry() -> Mapping[type[Any], PurgeEntityPolicy]:
+ """Index the built-in purge roots plus any the host installed.
+
+ The index is cached per installed provider, not once per process. Freezing
+ it at first use would pin whatever happened to be installed at that
+ moment -- including nothing at all, for a call made before startup
+ finished -- until a restart. Keying the cache on the provider means it is
+ invoked once however many roots are purged, a host that installs or
+ replaces one is honored, and the ordinary case (no provider) resolves to a
+ single cached index.
+ """
+ provider: Callable[[], Any] | None = (
+ current_app.config.get(HOST_POLICIES_CONFIG_KEY) if has_app_context()
else None
+ )
+ return _registry_for(provider)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unhashable provider crashes registry</b></div>
<div id="fix">
The old code admitted a malformed provider here: `provider()` ran inside
`try/except Exception` and was logged and dropped. Now an unhashable config
value (list/dict) reaches `_registry_for`, whose `lru_cache` hashes the key
first, so TypeError escapes `purge_policy_registry()` before
`_host_purge_policies` can log-and-drop it, contradicting the documented host
boundary. Reject non-callables before the cache lookup.
</div>
</div>
<small><i>Code Review Run #1d7448</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]
