mikebridge commented on code in PR #44874:
URL: https://github.com/apache/superset/pull/44874#discussion_r4161875399
##########
tests/unit_tests/tasks/test_deletion_retention.py:
##########
@@ -235,6 +238,160 @@ def test_clock_uses_now_not_utcnow() -> None:
purge.assert_called_once_with(Slice, now - timedelta(days=30), False)
[email protected]("dry_run", [False, True])
[email protected]("row_count", [0, 1, 3])
+def test_unsupported_model_is_reported_without_scanning(
+ app_config: Config,
+ monkeypatch: pytest.MonkeyPatch,
+ caplog: pytest.LogCaptureFixture,
+ dry_run: bool,
+ row_count: int,
+) -> None:
+ """Skip unsupported models once per run, regardless of eligible row
count."""
+ # avoid app-init regression: model helpers require the app_config fixture
first.
+ from superset.models.helpers import SoftDeleteMixin
+ from superset.tasks import deletion_retention as task
+
+ monkeypatch.setattr(SoftDeleteMixin, "_registered_subclasses", [])
+ mapper_registry: registry = registry()
+
+ @mapper_registry.mapped
+ class UnsupportedModel(SoftDeleteMixin):
+ """A host-style model with the legacy integer scan key but no purge
policy."""
+
+ __tablename__: str = "unsupported_purge_test"
+ __table__: ClassVar[sa.Table]
+ id: Mapped[int] = mapped_column(primary_key=True)
+ uuid: Mapped[UUID] = mapped_column(sa.Uuid, default=uuid4)
+
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ monkeypatch.setitem(app_config, "SOFT_DELETE_PURGE_DRY_RUN", dry_run)
+ try:
+ mapper_registry.metadata.create_all(engine)
+ session: Session
+ with Session(engine) as session:
+ if row_count:
+ session.execute(
+ sa.insert(UnsupportedModel),
+ [
+ {
+ "id": index + 1,
+ "deleted_at": datetime.now() - timedelta(days=90),
+ }
+ for index in range(row_count)
+ ],
+ )
+ session.commit()
+ scan: MagicMock
+ rollback: MagicMock
+ write_ahead: MagicMock
+ counter: MagicMock
+ gauge: MagicMock
+ with (
+ patch.object(task.db, "session", session),
+ patch.object(session, "execute", wraps=session.execute) as
scan,
+ patch.object(session, "rollback", wraps=session.rollback) as
rollback,
+ patch.object(task.audit, "reconcile_pending"),
+ patch.object(task.audit, "write_ahead") as write_ahead,
+ patch.object(task, "resolve_retention_window",
return_value=30),
+ patch.object(
+ task.feature_flag_manager, "is_feature_enabled",
return_value=True
+ ),
+ patch.object(task.stats_logger_manager.instance, "incr") as
counter,
+ patch.object(task.stats_logger_manager.instance, "gauge") as
gauge,
+ ):
+ result: dict[str, Any] = task.purge_soft_deleted.run()
+ if dry_run:
+ assert result["would_purge"] == {}, result
+ else:
+ assert result["cascade_failures"] == 0, result
+ assert result["purged"] == {}, result
+ assert result["blocked_by_reference"] == 0, result
+ assert result["unsupported_models"] == {"unsupported_purge_test":
1}
+ scan.assert_not_called()
+ rollback.assert_not_called()
+ write_ahead.assert_not_called()
+ gauge.assert_not_called()
+ counter.assert_called_once_with(
+ "deletion_retention.unsupported_models.unsupported_purge_test"
+ )
+ assert (
+ caplog.text.count("skipping unsupported_purge_test: no purge
policy")
+ == 1
+ )
+ assert (
+ session.scalar(
+
sa.select(sa.func.count()).select_from(UnsupportedModel.__table__)
+ )
+ == row_count
+ )
+ finally:
+ mapper_registry.dispose()
+ engine.dispose()
+
+
[email protected]("dry_run", [False, True])
+def test_unsupported_model_does_not_prevent_supported_models_from_purging(
+ app_config: Config,
+ monkeypatch: pytest.MonkeyPatch,
+ caplog: pytest.LogCaptureFixture,
+ dry_run: bool,
+) -> None:
+ """A skipped model must not stop the remaining supported models in the
run."""
+ # avoid app-init regression: model helpers require the app_config fixture
first.
+ from superset.models.helpers import SoftDeleteMixin
+ from superset.tasks import deletion_retention as task
+
+ registered_models: list[type[SoftDeleteMixin]] = task._soft_delete_models()
+ supported_models: list[type[SoftDeleteMixin]] =
list(task.purge_policy_registry())
+ monkeypatch.setattr(SoftDeleteMixin, "_registered_subclasses",
registered_models[:])
+
+ class UnsupportedModel(SoftDeleteMixin):
+ """An unsupported root that must never reach the row-processing
boundary."""
+
+ __tablename__: str = "unsupported_mixed_purge_test"
+
+ # Put the unsupported model first to detect an early return or break.
+ monkeypatch.setattr(
+ SoftDeleteMixin,
+ "_registered_subclasses",
+ [UnsupportedModel, *registered_models],
Review Comment:
Thanks for the reproduction, and for the suggestion. Applied in fe43b27f03:
the mixed test now scopes the registry to the supported policy models plus its
own unsupported model. With the polluting unit modules loaded alongside, it was
2 failed before and 125 passed after. Our earlier local runs only used a
focused file selection, which is why this slipped through; we now run the whole
`tests/unit_tests` suite before pushing.
--
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]
