mikebridge commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4039499608
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ 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](
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ 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(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
Review Comment:
Good catch; addressed in e16fe624684ba50877b2570c658455896b170819 by varying
the reason by entity type as suggested. Removing the between.entity_type
conjunct passes the old fixture but fails the amended fixture; restoration
passes, independently reproduced in review. All 105 affected unit tests and
mandatory branch-file hooks including MyPy passed; fresh CI is queued. Could
you take another look? The adjacent earlier.entity_type discrimination case
remains an explicitly recorded follow-up, not coverage claimed by this change.
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ 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](
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ 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(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
+ }
+ for id_, entity_type, day in zip(
+ ids, ["chart", "dashboard", "chart"], [1, 2, 3], strict=True
+ )
+ ]
+ try:
+ metadata.create_all(engine)
+ with engine.begin() as connection:
+ connection.execute(sa.insert(table), rows)
+ predicate: Callable[..., sa.ColumnElement[bool]]
+ for predicate in (
+ prune_audit._window_repeats_an_earlier_block,
+ prune_audit._legacy_repeats_an_earlier_block,
+ ):
+ assert set(
+ connection.scalars(
+ sa.select(table.c.id).where(
+ table.c.id.in_(ids if scope_entities != [] else
[]),
+ predicate(table, datetime(2026, 2, 1)),
Review Comment:
Good catch; addressed in e16fe624684ba50877b2570c658455896b170819. The
misleading parameterization is removed, and a separate fixture passes
scope_entities directly into the window predicate with no outer ID filter:
unscoped, two-pair, one-pair and empty scope yield 3/2/1/0 repeats. Forcing
scope to None produces three failures; restoration passes, independently
reproduced in review. Could you check that this captures the intended contract?
The legacy path intentionally has no scope parameter; deliberate cross-product
widening remains documented rather than newly pinned by this fixture.
--
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]
