sadpandajoe commented on code in PR #44892:
URL: https://github.com/apache/superset/pull/44892#discussion_r4165464820
##########
docs/docs/using-superset/recently-archived.mdx:
##########
@@ -123,7 +123,18 @@ Each skip increments
`deletion_retention.unsupported_models.<table>` once per
run. Unsupported models are excluded from `would_purge` and do not count as
`cascade_failures`; supported models continue to use their existing policies.
An empty `unsupported_models` map means no registered models were skipped for
-this reason. This reporting does not add purge support for host-defined models.
+this reason. This reporting does not itself add purge support for host-defined
+models.
+
+A host distribution that carries its own soft-deletable entities can supply
+their purge behavior by installing `PURGE_POLICIES_FUNC`: a callable returning
+one `PurgeEntityPolicy` per root it owns. Installed policies are validated
+against the discovered dependency graph exactly as the built-in ones are, and a
+model that gains an installed policy stops being reported as unsupported. A
+provider that fails, returns something other than a sequence of policies, or
+redeclares a built-in chart, dashboard or dataset root is logged and ignored,
+leaving the built-in roots unchanged. Hosts can require
+`superset.versioning.utils.HOST_POLICY_API_VERSION` before installing.
Review Comment:
`HOST_POLICY_API_VERSION` is still `1` on the base branch, where
`PURGE_POLICIES_FUNC` is never read, so a host using this guard can pass
startup while its archived roots remain unsupported after a rollback. Could
this document a capability check or required version that actually
distinguishes purge-hook support?
##########
superset/commands/deletion_retention/purge_policy.py:
##########
@@ -860,7 +867,96 @@ def declare(
cleanup_permission=cleanup_dataset_permission,
),
}
- return validate_unique_root_policies(registry.values())
+ return tuple(registry.values())
+
+
+def _host_purge_policies() -> 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: an unavailable provider, a malformed payload, a policy that
+ collides with a root declared here, and two policies for one host root are
+ each logged and dropped. A broken host declaration must not take the
+ scheduled purge down with it, and must never redefine how a chart,
+ dashboard or dataset is purged.
+ """
+ if not has_app_context():
+ return ()
+ provider: Callable[[], Any] | None = current_app.config.get(
+ HOST_POLICIES_CONFIG_KEY
+ )
+ 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
+ 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.
+
+ Uncached on purpose, unlike its two inputs: the built-in declarations are
+ built once per process, while a host policy is resolved per call, so a
+ provider installed after the first purge is honored for any root not yet
+ resolved. ``get_purge_policy`` memoizes per model, so replacing the policy
+ of a root it has already resolved needs a restart -- and that memoization
+ is why rebuilding this small index is off the hot path.
+ """
+ return validate_unique_root_policies(
Review Comment:
A host graph classified as root → ASSOCIATION link → OWNED detail passes
validation with the stock cleanup callbacks, but the association phase deletes
the link before the detail query traverses it; SQLite without FK enforcement
then leaves the detail orphaned, while enforced FKs make every purge roll back.
Could validation reject this mixed-phase ordering, or could cleanup preserve
the traversal keys until the descendants are deleted?
--
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]
