sadpandajoe commented on code in PR #44892:
URL: https://github.com/apache/superset/pull/44892#discussion_r4172315726
##########
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:
The new hook admits larger host graphs into discovery, which revisits every
remaining declared table for each traversal permutation: eight directly owned
tables take 109,600 visits for 16 unique dependency keys, and twelve require
about 1.3 billion visits before any row can purge. Could discovery track
visited tables across the whole graph before accepting these policies?
##########
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)
Review Comment:
A graph-complete host model with a UUID primary key and no `id` column is
accepted here, but the scheduled scan raises at `table.c.id` outside its
per-row error handling—even on an empty table or dry run—and skips any later
built-in roots. Could host admission validate the stock scanner’s root
requirements, or could scanning failures be isolated per model?
##########
superset/commands/deletion_retention/purge_policy.py:
##########
@@ -926,6 +1074,7 @@ def _validate_executable_declarations(policy:
PurgeEntityPolicy) -> None:
raise RuntimeError(
f"Missing version target column for
{dependency.key.describe()}"
)
+ _validate_owned_traversal(policy)
Review Comment:
A complete host policy owning both assets and tasks, with `task.asset_id →
asset.id` preserved as a cross-link, passes this validation, but both deletes
have the same depth: declaring assets first makes every purge roll back under
enforced FKs, while reversing the declarations succeeds. Could validation
reject unsupported sibling cross-links, or could cleanup order them by their
foreign-key dependencies rather than declaration order?
##########
superset/commands/deletion_retention/purge_policy.py:
##########
@@ -909,6 +1005,58 @@ def _validated_purge_policy(model: type[Any]) ->
PurgeEntityPolicy:
return policy
+def _ownership_dependency(
+ policy: PurgeEntityPolicy, related_table: str
+) -> DependencyPolicy | None:
+ """The single owned/association edge attaching *related_table*, if
clear."""
+ candidates: tuple[DependencyPolicy, ...] = tuple(
+ dependency
+ for dependency in policy.dependencies
+ if dependency.classification
+ in {DependencyClassification.OWNED,
DependencyClassification.ASSOCIATION}
+ and dependency.key.related_table == related_table
+ and dependency.key.direction == "inbound"
+ )
+ return candidates[0] if len(candidates) == 1 else None
+
+
+def _validate_owned_traversal(policy: PurgeEntityPolicy) -> None:
+ """Reject an owned table reachable only through an association.
+
+ ``cascade_hard_delete`` empties associations before owned children, while
+ an owned table's predicate selects its rows *through* its ownership path
+ (see ``_owner_value_select``). If a hop on that path is an association,
+ its rows are already gone when the owned delete runs: the statement
+ matches nothing, leaving the descendants orphaned where foreign keys are
+ unenforced and blocking the root's delete where they are not.
+
+ Refused at declaration time rather than executed. The shape has a
+ remedy -- classify the intermediate table as owned, which places it in
+ the same phase as what it leads to.
+ """
+ root_table: str = sa.inspect(policy.model).local_table.name
+ for dependency in policy.dependencies:
+ if dependency.classification is not DependencyClassification.OWNED:
+ continue
+ table_name: str = dependency.key.owner_table
+ visited: set[str] = set()
+ while table_name != root_table and table_name not in visited:
Review Comment:
An `OWNED` self-FK passes this check because its owner table is already the
root, but a host tree `1 → 2 → 3` is only deleted one level deep: with SQLite
FKs disabled, purge reports success after removing 1 and 2 while leaving 3
orphaned; restrictive FKs instead roll the purge back. Could recursive
ownership be rejected here unless the cleanup actually deletes the full subtree?
--
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]
