alexandrusoare commented on code in PR #44892:
URL: https://github.com/apache/superset/pull/44892#discussion_r4206022624
##########
superset/commands/deletion_retention/purge_policy.py:
##########
@@ -905,32 +1398,283 @@ def _validated_purge_policy(model: type[Any]) ->
PurgeEntityPolicy:
details = (
f"{details}; " if details else ""
) + f"stale_listeners=[{', '.join(coverage.stale_listeners)}]"
- raise RuntimeError(f"Incomplete purge policy for {model.__name__}:
{details}")
+ raise RuntimeError(
+ f"Incomplete purge policy for {policy.model.__name__}: {details}"
+ )
return policy
-def _validate_executable_declarations(policy: PurgeEntityPolicy) -> None:
- """Reject executable classifications missing their required action
metadata."""
+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.
+
+ The shared cleanup empties associations before owned children, while an
+ owned table's predicate selects its rows *through* its ownership path. If
+ a hop on that path is an association, its rows are already gone when the
+ owned delete runs, so the statement matches nothing.
+
+ Kept where the rest of the ordering rules were dropped because this one
+ can be silent: where foreign keys are enforced the association delete
+ fails loudly, but where they are not -- SQLite -- the root is purged and
+ its descendants are left orphaned with nothing reported. The remedy is to
+ 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
DependencyClassification.LISTENER_EFFECT
- and dependency.listener_action is None
- ):
- raise RuntimeError(
- f"Missing listener action for {dependency.key.describe()}"
- )
- if (
- dependency.classification is DependencyClassification.VERSION_OWNED
- and dependency.version_column is None
+ 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:
+ visited.add(table_name)
+ hop: DependencyPolicy | None = _ownership_dependency(policy,
table_name)
+ if hop is None:
+ # An absent or ambiguous path is reported by coverage, and by
+ # _ownership_edge at execution; not this check's business.
+ break
+ if hop.classification is DependencyClassification.ASSOCIATION:
+ raise RuntimeError(
+ f"Owned dependency {dependency.key.describe()} is
reachable "
+ f"only through association {hop.key.describe()}; "
+ "associations are deleted first, so the owned rows would "
+ "be orphaned"
+ )
+ table_name = hop.key.owner_table
+
+
+def _validate_recursive_ownership(policy: PurgeEntityPolicy) -> None:
+ """Reject a self-referencing owned table under one-level cleanup.
+
+ Cleanup that issues one statement per declared edge prunes a table that
+ owns itself one level deep. Where foreign keys are enforced the root's own
+ delete then fails and rolls back; where they are not -- SQLite -- the root
+ is purged and its grandchildren are left behind with a dangling parent id
+ and nothing reported. A policy declaring a tree walks it itself and says
+ so with ``walks_own_subtree``.
+ """
+ for dependency in policy.dependencies:
+ if dependency.classification is not DependencyClassification.OWNED:
+ continue
+ key: DependencyKey = dependency.key
+ if key.owner_table != key.related_table:
+ continue
+ raise RuntimeError(
+ f"Owned dependency {key.describe()} is self-referencing; the stock
"
+ "owned-child cleanup deletes one level, so this policy must supply
"
+ "its own delete_owned_children"
+ )
+
+
+def _validate_scanner_requirements(policy: PurgeEntityPolicy) -> None:
+ """Reject a root the scheduled scan cannot page through.
+
+ The retention task selects, windows and orders eligible rows by ``id``,
+ and the cascade pins each row by it. A root keyed on something else -- a
+ UUID primary key with no ``id`` column -- raises inside the scan, outside
+ the per-row error handling, so the run aborts before the remaining roots
+ are reached.
+ """
+ mapper: Mapper[Any] = sa.inspect(policy.model)
+ table: sa.Table = mapper.local_table
+ if "id" not in table.c:
+ raise RuntimeError(
+ f"Purge root {policy.model.__name__} has no 'id' column; the "
+ "scheduled scan pages eligible rows by id"
+ )
+ primary_key: tuple[sa.Column[Any], ...] = tuple(mapper.primary_key)
Review Comment:
it fails per row and is counted as a cascade failure with a traceback, which
is the loud category the PR says it doesn't guard. We're not trying to cover
every host shape — added to the known-unsupported list instead
--
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]
