alexandrusoare commented on code in PR #44892:
URL: https://github.com/apache/superset/pull/44892#discussion_r4183363293
##########
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:
Good catch — visited restarted per branch, so every sibling re-walked all
the others. Now tracked across the whole traversal, with a test pinning the
count. One subtlety: a mapper walk sees relationship edges a plain table walk
can't, so the two are tracked separately — merging them dropped sql_metrics →
sql_metrics_version from the dataset policy, which the existing tests caught.
##########
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:
Agreed. Went with your first option: two same-depth owned tables that
reference each other are now rejected.
--
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]
