mikebridge commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4066510538
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,701 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
[email protected](
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect:
Any) -> None:
+ """Limit history to candidate entities only when a batch scope is
supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now,
scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
[email protected](
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
[email protected]("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first
connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
[email protected](
+ ("history", "repeat_indices"),
+ [
+ pytest.param(
+ [
+ ("chart", STATUS_BLOCKED, 1, "same"),
+ ("dashboard", STATUS_BLOCKED, 2, "other"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="partition-reason-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_CONFIRMED, 1, None),
+ ("chart", STATUS_BLOCKED, 5, "same"),
+ ("chart", STATUS_CONFIRMED, 6, None),
+ ("chart", STATUS_BLOCKED, 7, "same"),
+ ("chart", STATUS_BLOCKED, 8, "same"),
+ ],
+ [4],
+ id="boundary-join-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_BLOCKED, 1, "same"),
+ ("chart", STATUS_BLOCKED, 2, "same"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="predecessor-type-isolation",
+ ),
Review Comment:
Addressed in `1e2982ea941e126924cb0a5fc9418e4259a69b65`: the
foreign-newer-streak-breaker case now pins the scalar boundary's entity_type
guard, and the existing isolation cases are retained. Thanks for confirming the
mutation is caught in your subsequent review.
##########
superset/commands/deletion_retention/prune_audit.py:
##########
@@ -615,15 +800,17 @@ def _evidence_candidates(now: datetime, cutoff: datetime,
limit: int) -> sa.sql.
"""
table: sa.Table = PurgeAuditLog.__table__
return (
- sa.select(table.c.id)
+ sa.select(table.c.id, table.c.entity_type, table.c.entity_uuid)
Review Comment:
Addressed in `1e2982ea941e126924cb0a5fc9418e4259a69b65`: evidence discovery
now selects only the audit ID, as suggested. The locked candidacy recheck and
delete behavior are preserved. The final affected suite passed all 161 tests.
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,710 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
[email protected](
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect:
Any) -> None:
+ """Limit history to candidate entities only when a batch scope is
supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now,
scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
[email protected](
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
[email protected]("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first
connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
[email protected](
+ ("history", "repeat_indices"),
+ [
+ pytest.param(
+ [
+ ("chart", STATUS_BLOCKED, 1, "same"),
+ ("dashboard", STATUS_BLOCKED, 2, "other"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="partition-reason-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_CONFIRMED, 1, None),
+ ("chart", STATUS_BLOCKED, 5, "same"),
+ ("chart", STATUS_CONFIRMED, 6, None),
+ ("chart", STATUS_BLOCKED, 7, "same"),
+ ("chart", STATUS_BLOCKED, 8, "same"),
+ ],
+ [4],
+ id="boundary-join-isolation",
+ ),
+ pytest.param(
+ [
+ ("dashboard", STATUS_BLOCKED, 1, "same"),
+ ("chart", STATUS_BLOCKED, 2, "same"),
+ ("chart", STATUS_BLOCKED, 3, "same"),
+ ],
+ [2],
+ id="predecessor-type-isolation",
+ ),
+ pytest.param(
+ [
+ ("chart", STATUS_BLOCKED, 5, "same"),
+ ("chart", STATUS_BLOCKED, 7, "same"),
+ ("dashboard", STATUS_CONFIRMED, 9, None),
+ ],
+ [1],
+ id="foreign-newer-streak-breaker",
Review Comment:
Addressed in `1e2982ea941e126924cb0a5fc9418e4259a69b65`: added the
two-chart/different-UUID isolation case. Removing the boundary entity_uuid
guard fails the new case; the unmodified control passes. Thanks for
independently confirming that this arm closes the gap in your latest review.
--
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]
