mikebridge commented on code in PR #41550:
URL: https://github.com/apache/superset/pull/41550#discussion_r3691310482
##########
superset/commands/deletion_retention/purge_cascade.py:
##########
@@ -239,6 +273,28 @@ def cascade_hard_delete(
entity_uuid=uuid,
blocked_reason=str(ex),
)
+ except IntegrityError as ex:
+ # Not a policy decision: a restrictive FK the cascade did not handle.
+ # Two audiences, two messages. The curated reason goes to the caller
+ # (and from there into a user toast), because raw driver text carries
+ # the failing SQL and bind parameters. The constraint detail goes to
+ # the log at WARNING, because an entity permanently unpurgeable via an
+ # unknown FK is a cascade-coverage bug someone has to be able to
+ # diagnose -- reported at INFO as a policy block, it read as intended
+ # behaviour.
+ logger.warning(
+ "deletion_retention: %s id=%s purge failed on a restrictive "
+ "foreign key the cascade does not handle: %s",
+ entity_type,
+ entity_id,
+ ex,
+ )
+ return CascadeResult(
+ purged=False,
+ entity_type=entity_type,
+ entity_uuid=uuid,
+ blocked_reason="blocked by database references",
+ )
Review Comment:
Declining, deliberately: routing `str(ex)` into `blocked_reason` is exactly
what an earlier review round removed — the reason string reaches user-facing
toasts, and raw driver/SQL text (which can embed table names and parameter
values) must not. The full original error is preserved at WARNING in the server
log with the entity identity, which is where an operator identifying an
external FK blocker is working anyway. If a machine-readable discriminator is
ever needed, the right shape is a constraint-name field parsed server-side, not
the raw exception text.
##########
superset/views/filters.py:
##########
@@ -292,6 +314,37 @@ def _mark_response_for_deleted_at_augmentation() -> None:
setattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, True)
+class BaseDeletedRecencyFilter(BaseFilter): # pylint:
disable=too-few-public-methods
+ """Keep rows archived within the last *value* days, by the server's clock.
+
+ The archive UI's time-range presets used to send an absolute cutoff
+ computed client-side in UTC. ``deleted_at`` is stamped with the server's
+ naive-local ``datetime.now()``, so on any non-UTC deployment those
+ cutoffs were shifted by the server offset -- and because the cutoff was
+ frozen when the page mounted, a long-lived tab drifted further. Taking a
+ day count and resolving it here, on the clock that stamped the column,
+ removes both failure modes and lets the client keep stable, shareable
+ filter values.
+
+ Subclasses set ``arg_name`` (e.g. ``"chart_deleted_recency"``).
+ """
+
+ name = lazy_gettext("Archived within")
+
+ def apply(self, query: Query, value: Any) -> Query:
+ try:
+ days = int(value)
+ except (TypeError, ValueError):
+ # Filter values arrive from the URL; refusing loudly would turn a
+ # mangled query string into a 500. An unfiltered list is the same
+ # answer every other malformed FAB filter value produces.
+ return query
+ if days <= 0:
+ return query
+ cutoff = datetime.now() - timedelta(days=days)
+ return query.filter(self.model.deleted_at > cutoff)
Review Comment:
Working as intended: the recency filter deliberately does not opt into the
visibility bypass, because the bypass and its restore-audience scoping live in
the `deleted_state` filter — making recency imply the bypass would let a
request window archived rows while sidestepping the audience scoping that
`deleted_state:only` enforces. The two compose (the archive page always sends
both), and standalone recency returning nothing is the fail-closed direction.
--
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]
