sadpandajoe commented on code in PR #44892:
URL: https://github.com/apache/superset/pull/44892#discussion_r4195557736
##########
superset/commands/deletion_retention/purge_policy.py:
##########
@@ -905,32 +1353,254 @@ 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 the stock cleanup.
+
+ ``delete_owned_children`` issues one statement per declared edge, so a
+ table that owns itself is pruned 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 host declaring a tree
+ supplies cleanup that walks it.
+ """
+ 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)
+ if (
+ len(primary_key) != 1
+ or primary_key[0].name != "id"
+ or not isinstance(primary_key[0].type, sa.Integer)
+ ):
+ # The scan pages by id and the row is then fetched with a scalar
+ # Session.get, so a composite or differently named key matches
+ # nothing: every row fails while a dry run still counts it eligible.
+ raise RuntimeError(
+ f"Purge root {policy.model.__name__} is keyed on "
+ f"({', '.join(column.name for column in primary_key)}); the "
+ "scheduled purge pages an integer 'id' watermark and looks rows "
+ "up by it"
+ )
+ if "deleted_at" not in table.c:
+ raise RuntimeError(
+ f"Purge root {policy.model.__name__} has no 'deleted_at' column; "
+ "every purge path requires the row to be archived first"
+ )
+
+
+#: Classifications the shared cleanup executes as SQL, through
+#: ``_dependency_predicates`` -- which only knows how to read an inbound
+#: foreign key.
+_EXECUTABLE_CLASSIFICATIONS: frozenset[DependencyClassification] = frozenset(
+ {
+ DependencyClassification.OWNED,
+ DependencyClassification.ASSOCIATION,
+ DependencyClassification.BLOCK,
+ }
+)
+
+
+def _validate_version_target(
+ policy: PurgeEntityPolicy, dependency: DependencyPolicy, metadata:
sa.MetaData
+) -> None:
+ """Reject a version target whose column does not hold the root's id.
+
+ Version cleanup compares the declared column with the root's id directly,
+ so the column has to be root-relative. Shadow tables are generated and may
+ not exist when a policy is validated, so the check reads their live
+ counterpart -- ``<table>_version`` describes ``<table>``, the naming the
+ shadow declarations already rely on -- and accepts the column when it is
+ the root's own primary key or a foreign key into the root.
+
+ A column naming an intermediate owner, a stage id under a workflow root
+ say, matches history belonging to a different root and leaves this one's
+ behind.
+ """
+ root_table: sa.Table = sa.inspect(policy.model).local_table
+ column: str = cast(str, dependency.version_column)
+ shadow_name: str = dependency.key.related_table
+ suffix: str = "_version"
+ live_name: str = (
+ shadow_name[: -len(suffix)] if shadow_name.endswith(suffix) else
shadow_name
+ )
+ live_table: sa.Table | None = metadata.tables.get(live_name)
+ if live_table is not None and column in live_table.c:
+ live_column: sa.Column[Any] = live_table.c[column]
+ primary_key: tuple[sa.Column[Any], ...] =
tuple(root_table.primary_key.columns)
+ if live_table is root_table:
+ # On the root's own shadow only its key identifies it. A
+ # self-referencing column such as ``parent_id`` points at *other*
+ # rows of the same root, so cleanup would delete the children's
+ # history and leave the purged row's own behind.
+ if len(primary_key) == 1 and live_column.name ==
primary_key[0].name:
+ return
+ # Elsewhere the comparison is still against the root's id, so a
+ # foreign key to any other root column matches rows belonging to a
+ # different root.
+ elif len(primary_key) == 1 and any(
+ foreign_key.column.table is root_table
+ and foreign_key.column.name == primary_key[0].name
+ for foreign_key in live_column.foreign_keys
):
- raise RuntimeError(
- f"Missing version target column for
{dependency.key.describe()}"
- )
+ return
+ raise RuntimeError(
+ f"Version target {shadow_name}.{column} for
{dependency.key.describe()} "
+ f"is not root-relative; cleanup compares it with the "
+ f"{root_table.name} id, so it must be that id or a foreign key to it"
+ )
+
+
+def _validate_dependency_declaration(
+ policy: PurgeEntityPolicy,
+ dependency: DependencyPolicy,
+ metadata: sa.MetaData,
+) -> None:
+ """Reject one declaration whose mistake would not announce itself."""
+ key: DependencyKey = dependency.key
+ if (
+ dependency.classification is DependencyClassification.LISTENER_EFFECT
+ and dependency.listener_action is None
+ ):
+ raise RuntimeError(f"Missing listener action for {key.describe()}")
+ if dependency.classification is DependencyClassification.VERSION_OWNED:
+ if dependency.version_column is None:
+ raise RuntimeError(f"Missing version target column for
{key.describe()}")
+ # A version target that is not root-relative deletes another root's
+ # history and leaves this root's behind -- silently, either way.
+ _validate_version_target(policy, dependency, metadata)
Review Comment:
This accepts a root-relative `VERSION_OWNED` target on a child shadow table
even when the host root itself is not versioned. `_delete_version_history` in
`purge_cascade.py` returns 0 as soon as `version_class(root)` raises
`ClassNotVersioned`, so for an unversioned root that owns a versioned child the
declared target is never processed: the root and live child rows are purged and
the child's history survives, with no failure reported. That is the
silent-mistake case this validation is meant to refuse. Should admission reject
a `VERSION_OWNED` dependency when the root has no version class, or should
version cleanup run the declared targets without it?
##########
superset/commands/deletion_retention/purge_policy.py:
##########
@@ -860,16 +914,408 @@ def declare(
cleanup_permission=cleanup_dataset_permission,
),
}
- return validate_unique_root_policies(registry.values())
+ return tuple(registry.values())
-@lru_cache(maxsize=None)
-def _validated_purge_policy(model: type[Any]) -> PurgeEntityPolicy:
- """Validate and return one root policy without blocking unrelated roots."""
+def _policy_label(policy: Any) -> str:
+ """A log-safe name for a payload whose shape is not yet established."""
+ model: Any = getattr(policy, "model", None)
+ return str(getattr(model, "__name__", type(model).__name__))
+
+
+def _host_policy_payload(provided: Any) -> tuple[Any, ...] | None:
+ """Materialize the provider's payload, or reject it.
+
+ Any iterable is accepted -- a list, a tuple, ``dict.values()``, a
+ generator -- since the documented contract is a sequence of policies, not
+ one particular container. Strings and bytes are excluded because they
+ iterate into characters, which would read as a sequence of bad members
+ rather than the wrong type. Materialized once: a generator cannot be
+ walked twice.
+ """
+ if isinstance(provided, (str, bytes)) or not isinstance(provided,
Iterable):
+ logger.error(
+ "purge_policy: %s returned %s; expected an iterable of
PurgeEntityPolicy",
+ HOST_POLICIES_CONFIG_KEY,
+ type(provided).__name__,
+ )
+ return None
try:
- policy: PurgeEntityPolicy = purge_policy_registry()[model]
- except KeyError as ex:
- raise ValueError(f"Unsupported purge model: {model.__name__}") from ex
+ return tuple(provided)
+ except Exception: # pylint: disable=broad-except
+ logger.exception(
+ "purge_policy: %s could not be read; keeping built-in roots only",
+ HOST_POLICIES_CONFIG_KEY,
+ )
+ return None
+
+
+def _unqualified_table_conflict(policy: PurgeEntityPolicy) -> str | None:
+ """Return the first table a bare-name lookup would resolve wrongly.
+
+ Discovery records table identities by bare name while a ``MetaData`` keys
+ them by schema, so a declared ``child`` resolves to the default-schema
+ table even where the policy meant ``host.child`` -- and the cleanup would
+ delete rows belonging to unrelated roots. Carrying schema-qualified
+ identities through discovery and execution is the better fix; until then
+ the ambiguous shape is refused rather than silently mis-resolved.
+ """
+ root_table: sa.Table = sa.inspect(policy.model).local_table
+ metadata: sa.MetaData = root_table.metadata
+ keys_by_name: dict[str, list[str]] = {}
+ for table in metadata.tables.values():
+ keys_by_name.setdefault(table.name, []).append(table.key)
+ declared: set[str] = {root_table.name} | {
+ dependency.key.related_table
+ for dependency in policy.dependencies
+ if dependency.classification in _EXECUTABLE_CLASSIFICATIONS
+ }
+ for name in sorted(declared):
+ keys: list[str] = keys_by_name.get(name, [])
+ if len(keys) != 1 or keys[0] != name:
+ return name
+ return None
+
+
+def _admitted_host_policy(
+ candidate: Any, builtin_roots: frozenset[type[Any]], builtin_types:
frozenset[str]
+) -> PurgeEntityPolicy | None:
+ """Return *candidate* if it is a usable host policy, else ``None``.
+
+ Every check runs behind one handler rather than guarding each attribute:
+ the payload is host-supplied, so reading ``model`` or hashing it may
+ itself raise -- and an exception escaping here would break
+ ``purge_policy_registry`` for the built-in roots too, which is the
+ opposite of what this boundary is for.
+ """
+ try:
+ if not isinstance(candidate, PurgeEntityPolicy):
+ logger.error(
+ "purge_policy: %s returned a %s; expected PurgeEntityPolicy",
+ HOST_POLICIES_CONFIG_KEY,
+ type(candidate).__name__,
+ )
+ return None
+ if not isinstance(candidate.model, type):
+ # Everything downstream treats the root as a mapped class:
+ # sa.inspect, the registry index, the duplicate count.
+ logger.error(
+ "purge_policy: host policy root %s is not a class",
+ _policy_label(candidate),
+ )
+ return None
+ if candidate.model in builtin_roots:
+ logger.error(
+ "purge_policy: host policy for built-in root %s ignored",
+ candidate.model.__name__,
+ )
+ return None
+ if candidate.entity_type in builtin_types:
+ # Core branches on entity_type -- the dataset impact path, the tag
+ # object type, the dataset permission name -- so a host reusing
+ # one of its names would have that logic pointed at its own rows.
+ logger.error(
+ "purge_policy: host policy for %s claims the reserved entity
type %r",
+ candidate.model.__name__,
+ candidate.entity_type,
+ )
+ return None
+ # Validated first, so a table the metadata simply does not contain is
+ # reported as such rather than as an ambiguous name.
+ inherited: list[str] = [
+ name
+ for name, stock in (
+ ("count_dashboard_slices", count_dashboard_slices),
+ ("collect_dangling_chart_uuids", dangling_chart_uuids),
+ )
+ if getattr(candidate, name) is stock
+ ]
+ if inherited:
+ # Cloning a built-in policy with ``replace`` is a natural way to
+ # borrow the generic cleanup, but it also carries these two, which
+ # read ``dashboard_slices`` and the chart table by the root's id.
+ # Left in place on a host root they write a count and a referrer
+ # list for an unrelated entity into the audit record -- wrong
+ # evidence, recorded as fact. The permission pair is inert here by
+ # comparison: both resolve from an entity type a host cannot
+ # claim.
+ raise RuntimeError(
+ f"{', '.join(inherited)} read this package's own tables; a "
+ "host policy supplies its own"
+ )
+ unusable: set[str] = {
+ dependency.classification.value
+ for dependency in candidate.dependencies
+ if dependency.classification
+ in {
+ DependencyClassification.BLOCK,
+ DependencyClassification.LISTENER_EFFECT,
+ }
+ }
+ if unusable:
+ # Both resolve from core's own identities: the stock listener
+ # actions decide what to delete from a built-in entity type, and
+ # blockers are applied by the stock validator. Declared by a host,
+ # they are accepted and then never carried out -- silence rather
+ # than a failure, which is the one outcome worth refusing. A host
+ # blocks by raising PurgeBlockedError from its own validate.
+ raise RuntimeError(
+ f"{', '.join(sorted(unusable))} dependencies cannot be carried
"
+ "out for a host root"
+ )
+ _validated_policy(candidate)
+ conflict: str | None = _unqualified_table_conflict(candidate)
+ if conflict is not None:
+ raise RuntimeError(
+ f"table {conflict!r} cannot be identified unambiguously by "
+ "name; schema-qualified roots are not supported"
+ )
+ 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_label(candidate),
+ ex,
+ )
+ return None
+ return candidate
+
+
+def _host_purge_policies(
+ provider: Callable[[], Any] | None,
+) -> tuple[PurgeEntityPolicy, ...] | None:
+ """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 payload that is not an iterable of
+ policies, a root that is not a class, a collision with a root or entity
+ type 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.
+
+ Returns ``None`` for the one failure that may be transient -- the provider
+ itself could not be called. The caller then serves the built-in roots
+ *without* publishing, so the next resolution tries again instead of
+ freezing a bad moment in for the life of the process. A payload that is
+ merely malformed is a host bug that will not fix itself, so it is
+ published as an empty result rather than retried on every read.
+ """
+ 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 None
+ candidates: tuple[Any, ...] | None = _host_policy_payload(provided)
Review Comment:
A generator provider that raises while it is being iterated (say it looks up
its policies before yielding) fails inside `_host_policy_payload`'s
`tuple(provided)`, which returns `None`; that is turned into `()` here and
published as a settled registry with no `retry_after`. The host roots then stay
unsupported until a restart, whereas a provider that raises when called gets
the 60s retry. Should an iteration failure take the same transient path as a
call failure? The docstring above ("serves the built-in roots *without*
publishing") also no longer matches the 60s deadline the code now publishes.
##########
docs/docs/using-superset/recently-archived.mdx:
##########
@@ -123,7 +123,84 @@ 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. A root whose eligible-row scan raises — an unreadable column, a
+transient database error — is counted in `scan_failures`, increments
+`deletion_retention.scan_failures.<table>` for that root and gauges
+`deletion_retention.scan_failures` for the run, leaving the remaining roots to
+purge normally. Rows that root had already purged before the failure stay in
+its reported counts, since those deletions are committed.
+
+Reporting a model is not the same as purging it: a host adds purge support for
+its own entities by installing a policy, described next.
+
+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, or returns anything other than an iterable of
+`PurgeEntityPolicy` — a list, a tuple, `dict.values()` and a generator all
+qualify — is logged and ignored, leaving the built-in roots unchanged. So is
+a policy whose root is not a class, which everything downstream assumes, and
+one that redeclares a built-in chart, dashboard or dataset root or claims
+one of their `entity_type` names, which the cascade branches on. Two
+declarations for one root are ignored too, so that root is reported as
+unsupported rather than purged by an arbitrary one of them.
+
+Superset guarantees the **frame** around a purge: the locked claim, the
+identity and eligibility predicates, the write-ahead audit record, the
+conditional delete and the blocked/failed accounting. Within that frame a
+policy either uses the shared cleanup callbacks, in which case its declared
+dependencies are the instructions they execute, or supplies its own callbacks
+and owns what happens between the claim and the delete. Either way the frame
+decides whether the row may be purged at all, so no policy can remove a row
+that is not archived and past the window.
+
+Validation is not an attempt to prove that an arbitrary declaration executes
+correctly. A policy is refused when a mistake in it would delete the wrong
+rows or silently skip cleanup, since neither announces itself:
+
+- its root is not keyed on a single `id` column, has no `deleted_at` column,
+ is not a class, or claims an `entity_type` the cascade branches on;
+- a table it names cannot be resolved unambiguously without a schema, so
+ cleanup would resolve it to a different table;
+- a version target is not root-relative, so cleanup would delete another
+ root's history and leave this root's behind;
+- it declares a block or a listener effect, which resolve from Superset's own
+ validator and entity types and so are never carried out for a host root. A
+ host refuses a purge by raising `PurgeBlockedError` from its own validator
+ instead;
+- it keeps the shared owned-child cleanup while declaring a table that owns
+ itself, or one reachable only through an association. The shared cleanup
+ deletes one level and builds each predicate by traversing the ownership
+ path, so without enforced foreign keys the root is purged and the rows
+ below it are orphaned silently. A host declaring either shape supplies its
+ own `delete_owned_children`.
+
+A host root's references from Superset-owned tables are the host's own to
+clean up, in its own callbacks: the tag and permission cleanups Superset runs
+for its roots resolve from entity types a host cannot claim, so they never
+run for one. A host policy also supplies its own `count_dashboard_slices` and
Review Comment:
The stated reason doesn't match the code: for an entity type that isn't
`chart`/`dashboard`/`dataset`, `count_dashboard_slices` returns `0` and
`dangling_chart_uuids` returns `[]` before reading any table, so the stock
callbacks don't record a count or referrer list for an unrelated host entity.
An extension author reading this will think they're avoiding audit-record
contamination when they are only being forced to write no-op callbacks. Is the
intent to require host-supplied callbacks anyway? If so, could the docs (and
the matching comment in `_admitted_host_policy`) say that rather than the
contamination rationale, or should the `inherited` rejection be dropped?
--
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]
