aminghadersohi commented on code in PR #44874:
URL: https://github.com/apache/superset/pull/44874#discussion_r4161735725
##########
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:
CI `unit-tests (current)` fails here: other unit tests leave
`SoftDeleteMixin` subclasses (e.g. `_soft_deletable_test`) in
`_registered_subclasses`, so they are skipped too and `unsupported_models` gets
7 keys. Reproduced locally; scoping to the registry passes.
```suggestion
[UnsupportedModel, *supported_models],
```
--
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]
