feat(charts): soft-delete and restore#40129
Merged
Merged
Conversation
github-actions
Bot
added
risk:db-migration
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40129 +/- ##
==========================================
- Coverage 64.72% 59.35% -5.37%
==========================================
Files 2686 822 -1864
Lines 148651 70487 -78164
Branches 34309 8838 -25471
==========================================
- Hits 96213 41840 -54373
+ Misses 50676 26880 -23796
- Partials 1762 1767 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6 tasks
mikebridge
force-pushed
the
sc-106189-charts-soft-delete
branch
from
May 18, 2026 17:03
e53c17e to
7230227
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 20, 2026
Eight items from the 2026-05-20 review on the infrastructure PR: * **H1 — subclass walk cached via ``__init_subclass__``.** The previous ``_all_soft_delete_subclasses()`` walked ``SoftDeleteMixin.__subclasses__()`` on every primary ORM SELECT. With one adopter today the cost is negligible, but the listener fires on every query in the app and the walk grows linearly with each adopted entity. ``SoftDeleteMixin`` now maintains a sorted ``_registered_subclasses`` list updated by ``__init_subclass__`` at class-definition time; the listener reads the cached list with no walk. Sort happens once per registration (rare event) rather than per query. * **M3 — ``SoftDeleteMixin`` import in ``BaseDAO.delete`` moved to top.** ``SKIP_VISIBILITY_FILTER_CLASSES`` from the same module was already top-level; ``SoftDeleteMixin`` had no real reason to be inline. Cleaner. * **M4 — pylint-disable in ``raise_for_ownership`` documented.** A top-level import here actually does cause a circular (security ↔ models.core ↔ superset's lazy ``feature_flag_manager``). Kept the inline import but added a comment naming the cycle so future maintainers don't try to "fix" it and re-trip the issue. * **M5 — ``BaseDeletedStateFilter.model`` typed as ``ClassVar[type[SoftDeleteMixin]]``.** Previously ``Any``, which meant a subclass accidentally binding ``model`` to a non-soft-deletable entity would crash at runtime on ``.deleted_at`` rather than failing mypy. * **M6 — listener placement documented.** ``setup_soft_delete_listener`` is called outside the ``with app_context()`` block because the ``do_orm_execute`` hook attaches to the ``Session`` class (process-wide), not to a Session instance, so it doesn't need Flask app context. Added an explaining comment in ``init_app_in_ctx``'s caller; ``setup_db()`` already ran earlier so the Session import is initialised. * **M7 — unit tests for ``BaseDeletedStateFilter`` + ``SoftDeleteApiMixin``.** New ``tests/unit_tests/views/test_soft_delete_filter.py`` (10 tests): filter no-op / include / only / case-insensitivity / unknown value; mixin no-op-without-flag / inject-with-flag / consume-flag-on-call; ``_serialize_deleted_at`` None / datetime handling. * **Nit — explicit ``orig_resource is None`` guard in ``raise_for_ownership``.** Previously fell through to ``hasattr(None, "owners")`` returning ``False`` → ``owners = []`` → generic ownership error. Now raises the proper "resource removed before ownership could be verified" error so the cause is clear in the rare race-with-hard-delete case. * **Nit — unit tests for ``find_existing_for_import`` / ``clear_soft_deleted_for_import``.** New ``tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py`` (5 tests): no-match-returns-None, live-row-as-is, soft-deleted-row-as-is (pure lookup, no side effect), ``clear`` hard-deletes via ORM (listener-firing path), and the composed find-then-clear contract that's the documented caller sequence. Deferred with explanation in the PR-comment reply (not in this commit): * **H2 — non-restorable / GDPR hook**: V2 concern. Designing the interface without a concrete use case risks getting it wrong; follow-up ticket if compliance work is ever scoped. * **M8 — ``deleted_at`` schema mutation**: documented in the entity- rollout PR bodies (apache#40128/apache#40129/apache#40130) rather than blocking infrastructure. 40 unit tests pass (was 26; added 14 covering filter, mixin, importer helpers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106189-charts-soft-delete
branch
5 times, most recently
from
May 20, 2026 22:36
c5bcec6 to
625b2bb
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 20, 2026
The listener's loader_criteria was passing a concrete SQL expression (``cls.where_not_deleted()``) which rendered as the raw table name (``slices.deleted_at``) regardless of how the table was aliased in the statement. When the same soft-deletable model appeared under an alias in a JOIN — e.g., FAB's outer/inner reconstruction or ``report_schedule.chart`` aliasing ``slices AS chart`` — the JOIN's ON clause produced ``Unknown column 'slices.deleted_at' in 'on clause'`` and broke the entire query. Surfaced as CI failures on the entity-rollout PRs (apache#40128 / apache#40129 / apache#40130) — specifically pre-existing report-schedule tests that join to ``slices AS chart`` started failing once ``Slice`` inherited ``SoftDeleteMixin``. The infrastructure PR itself doesn't exercise this because no entity has the mixin yet. Fix: switch the criteria to a lambda (the form Bayer's canonical pattern uses). SQLAlchemy invokes the lambda per occurrence and adapts the column reference to the alias at each site. The lambda's body is trivial (``c.deleted_at.is_(None)``) so the ``DeferredLambdaElement`` parser handles it without issue — we originally moved AWAY from a lambda because of an ``if cls in bypass_classes`` conditional that DeferredLambdaElement mis-parsed as a SQL ``IN`` operator, but the conditional now lives outside the lambda (in the per-class iteration), so the lambda body itself is clean. Test added: test_listener_adapts_criteria_to_aliased_table_in_joins — reproduces the aliased-join shape using the synthetic ``_SoftDeletableChild`` model. Without the fix, this query raises ``OperationalError: no such column: ..._soft_deletable_child.deleted_at`` because the criteria renders as the raw table name. With the fix, the query runs cleanly. 45 unit tests pass (was 44 + 1 regression test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106189-charts-soft-delete
branch
from
May 20, 2026 22:39
625b2bb to
7f017e0
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 21, 2026
Bayer's canonical pattern excludes ``is_relationship_load`` events on the assumption that ``with_loader_criteria(propagate_to_loaders=True)`` (the default) carries criteria from the parent statement to the relationship loaders. That works for the simple case where the parent statement references the target class. It does NOT reliably work when the parent statement references a *different* class and the criteria targets the relationship's target only. The Superset case that surfaces this: * A ``Dashboard`` SELECT fires. The listener attaches ``with_loader_criteria(Slice, lambda, propagate_to_loaders=True)``. ``Slice`` never appears in the Dashboard statement. * ``dashboard.slices`` lazy-loads via the many-to-many through ``dashboard_slices``. The criteria targeting ``Slice`` does not reach this lazy load. * The relationship load returns soft-deleted charts the listener was supposed to exclude. Surfaced as the failing ``test_restore_chart_reattaches_to_dashboards`` integration test on the charts-rollout PR (apache#40129): the soft-deleted chart was still in ``dashboard.slices``. Fix: drop the ``is_relationship_load`` exclusion. The listener now fires on relationship loads too and re-attaches the criteria directly. The resulting WHERE clause has ``deleted_at IS NULL`` once (when propagation didn't work) or twice (when it did) — both are idempotent and correct. Bayer's concern was about "redundant clauses," not correctness, and a tiny SQL redundancy is the right trade for closing a real visibility leak. ``is_column_load`` exclusion is kept — those events fire for attribute refreshes on already-loaded objects, not entity loads, and attaching loader criteria there would be both pointless and potentially harmful. The predicate is renamed ``_is_primary_user_select`` → ``_should_attach_soft_delete_criteria`` to reflect the broader scope; docstring updated to explain the deliberate inclusion of relationship loads and the redundancy-vs-correctness trade-off. 899 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106189-charts-soft-delete
branch
from
May 21, 2026 15:44
7f017e0 to
c5832d0
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 25, 2026
Eight items from the 2026-05-20 review on the infrastructure PR: * **H1 — subclass walk cached via ``__init_subclass__``.** The previous ``_all_soft_delete_subclasses()`` walked ``SoftDeleteMixin.__subclasses__()`` on every primary ORM SELECT. With one adopter today the cost is negligible, but the listener fires on every query in the app and the walk grows linearly with each adopted entity. ``SoftDeleteMixin`` now maintains a sorted ``_registered_subclasses`` list updated by ``__init_subclass__`` at class-definition time; the listener reads the cached list with no walk. Sort happens once per registration (rare event) rather than per query. * **M3 — ``SoftDeleteMixin`` import in ``BaseDAO.delete`` moved to top.** ``SKIP_VISIBILITY_FILTER_CLASSES`` from the same module was already top-level; ``SoftDeleteMixin`` had no real reason to be inline. Cleaner. * **M4 — pylint-disable in ``raise_for_ownership`` documented.** A top-level import here actually does cause a circular (security ↔ models.core ↔ superset's lazy ``feature_flag_manager``). Kept the inline import but added a comment naming the cycle so future maintainers don't try to "fix" it and re-trip the issue. * **M5 — ``BaseDeletedStateFilter.model`` typed as ``ClassVar[type[SoftDeleteMixin]]``.** Previously ``Any``, which meant a subclass accidentally binding ``model`` to a non-soft-deletable entity would crash at runtime on ``.deleted_at`` rather than failing mypy. * **M6 — listener placement documented.** ``setup_soft_delete_listener`` is called outside the ``with app_context()`` block because the ``do_orm_execute`` hook attaches to the ``Session`` class (process-wide), not to a Session instance, so it doesn't need Flask app context. Added an explaining comment in ``init_app_in_ctx``'s caller; ``setup_db()`` already ran earlier so the Session import is initialised. * **M7 — unit tests for ``BaseDeletedStateFilter`` + ``SoftDeleteApiMixin``.** New ``tests/unit_tests/views/test_soft_delete_filter.py`` (10 tests): filter no-op / include / only / case-insensitivity / unknown value; mixin no-op-without-flag / inject-with-flag / consume-flag-on-call; ``_serialize_deleted_at`` None / datetime handling. * **Nit — explicit ``orig_resource is None`` guard in ``raise_for_ownership``.** Previously fell through to ``hasattr(None, "owners")`` returning ``False`` → ``owners = []`` → generic ownership error. Now raises the proper "resource removed before ownership could be verified" error so the cause is clear in the rare race-with-hard-delete case. * **Nit — unit tests for ``find_existing_for_import`` / ``clear_soft_deleted_for_import``.** New ``tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py`` (5 tests): no-match-returns-None, live-row-as-is, soft-deleted-row-as-is (pure lookup, no side effect), ``clear`` hard-deletes via ORM (listener-firing path), and the composed find-then-clear contract that's the documented caller sequence. Deferred with explanation in the PR-comment reply (not in this commit): * **H2 — non-restorable / GDPR hook**: V2 concern. Designing the interface without a concrete use case risks getting it wrong; follow-up ticket if compliance work is ever scoped. * **M8 — ``deleted_at`` schema mutation**: documented in the entity- rollout PR bodies (apache#40128/apache#40129/apache#40130) rather than blocking infrastructure. 40 unit tests pass (was 26; added 14 covering filter, mixin, importer helpers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 25, 2026
The listener's loader_criteria was passing a concrete SQL expression (``cls.where_not_deleted()``) which rendered as the raw table name (``slices.deleted_at``) regardless of how the table was aliased in the statement. When the same soft-deletable model appeared under an alias in a JOIN — e.g., FAB's outer/inner reconstruction or ``report_schedule.chart`` aliasing ``slices AS chart`` — the JOIN's ON clause produced ``Unknown column 'slices.deleted_at' in 'on clause'`` and broke the entire query. Surfaced as CI failures on the entity-rollout PRs (apache#40128 / apache#40129 / apache#40130) — specifically pre-existing report-schedule tests that join to ``slices AS chart`` started failing once ``Slice`` inherited ``SoftDeleteMixin``. The infrastructure PR itself doesn't exercise this because no entity has the mixin yet. Fix: switch the criteria to a lambda (the form Bayer's canonical pattern uses). SQLAlchemy invokes the lambda per occurrence and adapts the column reference to the alias at each site. The lambda's body is trivial (``c.deleted_at.is_(None)``) so the ``DeferredLambdaElement`` parser handles it without issue — we originally moved AWAY from a lambda because of an ``if cls in bypass_classes`` conditional that DeferredLambdaElement mis-parsed as a SQL ``IN`` operator, but the conditional now lives outside the lambda (in the per-class iteration), so the lambda body itself is clean. Test added: test_listener_adapts_criteria_to_aliased_table_in_joins — reproduces the aliased-join shape using the synthetic ``_SoftDeletableChild`` model. Without the fix, this query raises ``OperationalError: no such column: ..._soft_deletable_child.deleted_at`` because the criteria renders as the raw table name. With the fix, the query runs cleanly. 45 unit tests pass (was 44 + 1 regression test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 25, 2026
Bayer's canonical pattern excludes ``is_relationship_load`` events on the assumption that ``with_loader_criteria(propagate_to_loaders=True)`` (the default) carries criteria from the parent statement to the relationship loaders. That works for the simple case where the parent statement references the target class. It does NOT reliably work when the parent statement references a *different* class and the criteria targets the relationship's target only. The Superset case that surfaces this: * A ``Dashboard`` SELECT fires. The listener attaches ``with_loader_criteria(Slice, lambda, propagate_to_loaders=True)``. ``Slice`` never appears in the Dashboard statement. * ``dashboard.slices`` lazy-loads via the many-to-many through ``dashboard_slices``. The criteria targeting ``Slice`` does not reach this lazy load. * The relationship load returns soft-deleted charts the listener was supposed to exclude. Surfaced as the failing ``test_restore_chart_reattaches_to_dashboards`` integration test on the charts-rollout PR (apache#40129): the soft-deleted chart was still in ``dashboard.slices``. Fix: drop the ``is_relationship_load`` exclusion. The listener now fires on relationship loads too and re-attaches the criteria directly. The resulting WHERE clause has ``deleted_at IS NULL`` once (when propagation didn't work) or twice (when it did) — both are idempotent and correct. Bayer's concern was about "redundant clauses," not correctness, and a tiny SQL redundancy is the right trade for closing a real visibility leak. ``is_column_load`` exclusion is kept — those events fire for attribute refreshes on already-loaded objects, not entity loads, and attaching loader criteria there would be both pointless and potentially harmful. The predicate is renamed ``_is_primary_user_select`` → ``_should_attach_soft_delete_criteria`` to reflect the broader scope; docstring updated to explain the deliberate inclusion of relationship loads and the redundancy-vs-correctness trade-off. 899 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106189-charts-soft-delete
branch
3 times, most recently
from
May 28, 2026 19:59
3721e6f to
27aa3e3
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
Eight items from the 2026-05-20 review on the infrastructure PR: * **H1 — subclass walk cached via ``__init_subclass__``.** The previous ``_all_soft_delete_subclasses()`` walked ``SoftDeleteMixin.__subclasses__()`` on every primary ORM SELECT. With one adopter today the cost is negligible, but the listener fires on every query in the app and the walk grows linearly with each adopted entity. ``SoftDeleteMixin`` now maintains a sorted ``_registered_subclasses`` list updated by ``__init_subclass__`` at class-definition time; the listener reads the cached list with no walk. Sort happens once per registration (rare event) rather than per query. * **M3 — ``SoftDeleteMixin`` import in ``BaseDAO.delete`` moved to top.** ``SKIP_VISIBILITY_FILTER_CLASSES`` from the same module was already top-level; ``SoftDeleteMixin`` had no real reason to be inline. Cleaner. * **M4 — pylint-disable in ``raise_for_ownership`` documented.** A top-level import here actually does cause a circular (security ↔ models.core ↔ superset's lazy ``feature_flag_manager``). Kept the inline import but added a comment naming the cycle so future maintainers don't try to "fix" it and re-trip the issue. * **M5 — ``BaseDeletedStateFilter.model`` typed as ``ClassVar[type[SoftDeleteMixin]]``.** Previously ``Any``, which meant a subclass accidentally binding ``model`` to a non-soft-deletable entity would crash at runtime on ``.deleted_at`` rather than failing mypy. * **M6 — listener placement documented.** ``setup_soft_delete_listener`` is called outside the ``with app_context()`` block because the ``do_orm_execute`` hook attaches to the ``Session`` class (process-wide), not to a Session instance, so it doesn't need Flask app context. Added an explaining comment in ``init_app_in_ctx``'s caller; ``setup_db()`` already ran earlier so the Session import is initialised. * **M7 — unit tests for ``BaseDeletedStateFilter`` + ``SoftDeleteApiMixin``.** New ``tests/unit_tests/views/test_soft_delete_filter.py`` (10 tests): filter no-op / include / only / case-insensitivity / unknown value; mixin no-op-without-flag / inject-with-flag / consume-flag-on-call; ``_serialize_deleted_at`` None / datetime handling. * **Nit — explicit ``orig_resource is None`` guard in ``raise_for_ownership``.** Previously fell through to ``hasattr(None, "owners")`` returning ``False`` → ``owners = []`` → generic ownership error. Now raises the proper "resource removed before ownership could be verified" error so the cause is clear in the rare race-with-hard-delete case. * **Nit — unit tests for ``find_existing_for_import`` / ``clear_soft_deleted_for_import``.** New ``tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py`` (5 tests): no-match-returns-None, live-row-as-is, soft-deleted-row-as-is (pure lookup, no side effect), ``clear`` hard-deletes via ORM (listener-firing path), and the composed find-then-clear contract that's the documented caller sequence. Deferred with explanation in the PR-comment reply (not in this commit): * **H2 — non-restorable / GDPR hook**: V2 concern. Designing the interface without a concrete use case risks getting it wrong; follow-up ticket if compliance work is ever scoped. * **M8 — ``deleted_at`` schema mutation**: documented in the entity- rollout PR bodies (apache#40128/apache#40129/apache#40130) rather than blocking infrastructure. 40 unit tests pass (was 26; added 14 covering filter, mixin, importer helpers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
The listener's loader_criteria was passing a concrete SQL expression (``cls.where_not_deleted()``) which rendered as the raw table name (``slices.deleted_at``) regardless of how the table was aliased in the statement. When the same soft-deletable model appeared under an alias in a JOIN — e.g., FAB's outer/inner reconstruction or ``report_schedule.chart`` aliasing ``slices AS chart`` — the JOIN's ON clause produced ``Unknown column 'slices.deleted_at' in 'on clause'`` and broke the entire query. Surfaced as CI failures on the entity-rollout PRs (apache#40128 / apache#40129 / apache#40130) — specifically pre-existing report-schedule tests that join to ``slices AS chart`` started failing once ``Slice`` inherited ``SoftDeleteMixin``. The infrastructure PR itself doesn't exercise this because no entity has the mixin yet. Fix: switch the criteria to a lambda (the form Bayer's canonical pattern uses). SQLAlchemy invokes the lambda per occurrence and adapts the column reference to the alias at each site. The lambda's body is trivial (``c.deleted_at.is_(None)``) so the ``DeferredLambdaElement`` parser handles it without issue — we originally moved AWAY from a lambda because of an ``if cls in bypass_classes`` conditional that DeferredLambdaElement mis-parsed as a SQL ``IN`` operator, but the conditional now lives outside the lambda (in the per-class iteration), so the lambda body itself is clean. Test added: test_listener_adapts_criteria_to_aliased_table_in_joins — reproduces the aliased-join shape using the synthetic ``_SoftDeletableChild`` model. Without the fix, this query raises ``OperationalError: no such column: ..._soft_deletable_child.deleted_at`` because the criteria renders as the raw table name. With the fix, the query runs cleanly. 45 unit tests pass (was 44 + 1 regression test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
Bayer's canonical pattern excludes ``is_relationship_load`` events on the assumption that ``with_loader_criteria(propagate_to_loaders=True)`` (the default) carries criteria from the parent statement to the relationship loaders. That works for the simple case where the parent statement references the target class. It does NOT reliably work when the parent statement references a *different* class and the criteria targets the relationship's target only. The Superset case that surfaces this: * A ``Dashboard`` SELECT fires. The listener attaches ``with_loader_criteria(Slice, lambda, propagate_to_loaders=True)``. ``Slice`` never appears in the Dashboard statement. * ``dashboard.slices`` lazy-loads via the many-to-many through ``dashboard_slices``. The criteria targeting ``Slice`` does not reach this lazy load. * The relationship load returns soft-deleted charts the listener was supposed to exclude. Surfaced as the failing ``test_restore_chart_reattaches_to_dashboards`` integration test on the charts-rollout PR (apache#40129): the soft-deleted chart was still in ``dashboard.slices``. Fix: drop the ``is_relationship_load`` exclusion. The listener now fires on relationship loads too and re-attaches the criteria directly. The resulting WHERE clause has ``deleted_at IS NULL`` once (when propagation didn't work) or twice (when it did) — both are idempotent and correct. Bayer's concern was about "redundant clauses," not correctness, and a tiny SQL redundancy is the right trade for closing a real visibility leak. ``is_column_load`` exclusion is kept — those events fire for attribute refreshes on already-loaded objects, not entity loads, and attaching loader criteria there would be both pointless and potentially harmful. The predicate is renamed ``_is_primary_user_select`` → ``_should_attach_soft_delete_criteria`` to reflect the broader scope; docstring updated to explain the deliberate inclusion of relationship loads and the redundancy-vs-correctness trade-off. 899 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106189-charts-soft-delete
branch
from
May 28, 2026 20:08
27aa3e3 to
6ea8518
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
…orter text, migration warning Three doc-only corrections to the charts soft-delete entry: 1. Rison filter values were wrong: the entry advertised chart_deleted_state=(deleted|active), but the implementation (BaseDeletedStateFilter.apply in superset/views/filters.py:185-189) only accepts include / only — other values silently no-op. 2. Importer description was stale (described the pre-Option-C raise behavior). Rewrote to reflect the implicit restore-and-update Option C now does: owner gets the chart back in place, non-owners and callers without can_write get ImportFailedError. Reflects the permission matrix in the in-tree docstring. 3. Added a migration-safety note following the precedent set by PR apache#27392 ("Adds an index... may cause downtime on large deployments") and PR apache#27119. Index creation runs inline (no CONCURRENTLY, per Superset convention) and briefly blocks reads on the slices table during the build on Postgres; MySQL InnoDB builds it online. Sourced from the SQLAlchemy review on PR apache#40129. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
…test Two SQLAlchemy-review follow-ups: 1. **Explicit flush on restore-and-update path.** The implicit-restore flow in import_chart relied on autoflush to push the deleted_at = NULL update before Slice.import_from_dict's internal query-by-uuid lookup ran. Without it, the listener would have filtered the row out of the lookup, import_from_dict would have taken the "new object" branch, and the insert would have hit the uuid unique constraint at flush time. Adding db.session.flush() after clearing deleted_at makes the contract independent of autoflush — robust against any caller that disables autoflush on the session, and easier for the next reader to reason about. The inline comment is rewritten to describe the actual mechanism (the previous wording incorrectly claimed config["id"] = existing.id "routes import_from_dict through UPDATE", but ImportExportMixin strips non-export_fields keys from config — so config["id"] is in fact defensive and the real bind happens via the uuid uniqueness lookup inside import_from_dict). 2. **Non-admin owner restore integration test.** The existing test_restore_chart_reattaches_to_dashboards uses ADMIN_USERNAME, so the FAB security wiring on the owner path is only exercised via unit-test mocks. Added test_restore_chart_by_non_admin_owner to log in as ALPHA_USERNAME, soft-delete and then restore an alpha-owned chart, and assert the row's deleted_at is cleared. Catches regressions that would break the can_write + ownership check on the non-admin path. Both items raised by the SQLAlchemy lens review on PR apache#40129. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… all .po `pybabel update` fuzzy-matches the new exception string against the existing similar "Chart could not be created."/"updated." entries in every catalog, which trips the translation-regression CI check (each language ticks up by one fuzzy entry). Pre-seeding an empty `msgstr ""` entry for the new msgid keeps pybabel from inventing a fuzzy match — when it sees the entry already exists it leaves it alone. Translators can fill in the real string later through the normal translation flow. Affects all 26 active catalogs.
…sent Adds first test coverage for migration ``7c4a8d09ca37_add_deleted_at_to_slices``: * End-to-end exercise of ``upgrade()`` and ``downgrade()`` against an in-memory SQLite engine with a real Alembic ``Operations`` context, not just the underlying helper functions. * Downgrade-with-soft-deleted-rows-present is pinned: rows that had ``deleted_at`` set before downgrade survive the migration and become indistinguishable from live rows. Matches the "Rollback note" in ``UPDATING.md``; makes the contract regression-proof. * Idempotency on both directions is covered. The test uses a minimal pre-seeded ``slices`` stub (id + slice_name) since the migration only touches ``deleted_at`` and its index.
Superset is on SQLAlchemy 1.4; ``Connection`` doesn't have a ``commit()`` method until 2.0. The implicit transaction over the single ``with engine.connect() as conn:`` block keeps reads-after-writes visible within the same connection.
The command does no logging; remove the dead 'import logging' and 'logger = logging.getLogger(__name__)' (codeant), matching the datasets restore command which already had this cleaned up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Extract the repeated 'find chart by uuid -> set deleted_at -> flush' block in the soft-delete import tests into a _soft_delete_existing_chart helper (bito) and make its timestamp timezone-aware. - Add 'Chart could not be restored.' to messages.pot (the .po placeholders were already seeded in all languages; the template was missing it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An active chart re-imported with overwrite=True but no can_write wrongly entered the restore-without-permission branch and raised the deleted-chart restore error, instead of falling through to returning the existing row (the pre-soft-delete overwrite-without-permission behaviour). Gate Case B on is_soft_deleted so only genuine restores are blocked. Add regression test for the active overwrite-without-can_write path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…igration Two pre-merge fixes surfaced by reviewing charts against current master: - ChartDeletedStateFilter now restricts soft-deleted rows to the restore audience (owners/admins), mirroring DashboardDeletedStateFilter and RestoreChartCommand's raise_for_ownership — a read-access non-owner can no longer enumerate soft-deleted charts they could never restore. Live rows keep normal ChartFilter visibility. Adds integration coverage (owner sees own deleted; gamma with datasource access does not). - Re-point the add_deleted_at_to_slices migration's down_revision from 33d7e0e21daa to 31dae2559c05 (master's current head) so it no longer creates an alembic multi-head when merged onto / rebased atop current master. Will be re-pointed again onto the dashboards migration during the merge sequence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Formatting-only; the file predates this pass and the pinned ruff-format flagged it in CI. No logic change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror of the dashboards split: import_chart no longer fuses overwrite and restore behind needs_mutation. A soft-deleted match is a RESTORE (own can_write gate + ownership check + in-place deleted_at clear); an alive match is an OVERWRITE (return existing unless overwrite+can_write, then ownership check). Behaviour unchanged; importer matrix tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Parallel to the dashboards/datasets restore-failure tests: assert that an error raised inside RestoreChartCommand.run surfaces as a clean 422 via the ChartRestoreFailedError handler rather than an unhandled 500. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- importer: use existing.restore() instead of writing deleted_at directly (speak the domain operation; same SQL, future-proof if restore() grows) - filters: make the deleted-state docstring merge-order-proof instead of naming sibling filter classes that may not exist in the tree yet Mirrors the datasets commit ebda9ad. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…save Multi-lens review-panel findings (sqlalchemy + committer + DDD lenses): - DashboardDAO.set_dash_metadata: bypass the soft-delete visibility filter when resolving incoming chart ids. The assignment rebuilds dashboard.slices wholesale, so with Slice now carrying SoftDeleteMixin a save during a member chart's soft-deleted window silently severed the dashboard_slices junction (breaking the documented restore- reattach contract) and nulled the chart's uuid in position_json. New unit test pins it (verified red without the fix, green with). - importer: include the chart name and uuid in both soft-deleted-match error messages, so a failed multi-chart dashboard-bundle import names the culprit. - api: DELETE/bulk-delete OpenAPI docstrings now describe soft-delete semantics and the restore path. - UPDATING.md: note bulk delete is also soft; note the deleted-state filter is owner-scoped for non-admins. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…master head Master advanced past 31dae2559c05 (now 78a40c08b4be); point the soft-delete migration at the current head so it forms a single linear head against master. Branch/merge sequence unchanged.
Per @aminghadersohi review: the five `from superset...` imports were inside the test body without a circular-import justification. They have no top-level side effects (db is a proxy; the models register on metadata at import, and create_all runs after import either way), so they move cleanly to the module-level import block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… head Was chained off 78a40c08b4be (old head); master advanced via apache#39859, so re-point onto 2bee73611e32 to keep a single migration head.
Soft-delete is gated behind the temporary SOFT_DELETE rollout flag (off by default since apache#41166). Decorate each soft-delete test with @with_feature_flags(SOFT_DELETE=True) so they exercise the gated behaviour once rebased onto the gated master.
Review finding: pre-soft-delete, report_schedule.chart_id's FK made a dangling target chart structurally impossible. With soft delete the only protection is DeleteChartCommand's validation, which a validate/commit race or a SOFT_DELETE flag toggle can slip past — and the executor's `if self._report_schedule.chart:` then silently falls through to the dashboard branch (chart is a visibility-filtered relationship that loads None) and fails opaquely. Add a guard in _get_url — the single choke point every content path (screenshots, CSV, embedded data, notification) funnels through — raising a dedicated ReportScheduleTargetChartDeletedError inside the state-machine envelope, so the ERROR log row and owner notification carry an actionable message. Also rename the delete-blocked test: find_by_chart_ids matches any report row (active or paused), not just active ones.
Review finding (verified by reproduction): the bypass covered only the incoming slice-resolution query. Assigning dashboard.slices makes the unit of work diff the new collection against the existing one, which it lazy-loads at that moment THROUGH the visibility listener — so a trashed member was missing from the baseline, treated as net-new, and INSERTed into dashboard_slices where its junction row still exists (soft delete never removes junction rows). Since apache#39859 gave dashboard_slices a composite primary key, every save of a dashboard containing a soft-deleted chart raised IntegrityError (500) with SOFT_DELETE on; on pre-apache#39859 schemas it silently wrote a duplicate junction row. Wrap both the resolution query and the assignment in a session-scoped skip_visibility_filter(Slice) so the baseline load sees the trashed member and the diff is a no-op for it. The existing unit test masked this: its fixture left dashboard.slices in-session (flush-only, flag off), so the filtered baseline reload never happened. Hardened to the production shape — SOFT_DELETE enabled, collection expired (as with a fresh find_by_id load in the PUT flow), and a post-call flush so the diff's SQL reaches the composite-PK junction. The hardened test fails with the exact IntegrityError against the previous code and passes with the fix.
…andard Review panel finding (unanimous, 7 of 8 lenses): the charts UPDATING.md section and the delete/bulk_delete/import OpenAPI docstrings described soft delete as unconditional, but SOFT_DELETE defaults to False — on a default deployment DELETE hard-deletes permanently. Port the adopted dashboards (apache#40128) wording: - UPDATING.md now leads with the flag gate + default, documents the flag-toggle caveat (soft-deleted rows reappear when the flag is turned off; restore and the deleted-state filter deliberately stay live), and qualifies the behavior paragraph. - Charts-specific additions: migration-downgrade consequence (dropping deleted_at silently activates not-yet-restored trash — reconcile before downgrading) and operational notes (reports targeting a soft-deleted chart fail with an explicit error; dashboards preserve trashed-member junctions across saves). - The importer paragraph and import_ docstring state the deliberate restore-with-update-without-overwrite asymmetry, matching apache#40128. - delete/bulk_delete docstrings + summaries qualified with the flag. Also drop the whitespace-only hunk in commands/chart/delete.py flagged by five review lenses as diff noise.
…filter normalization Two cross-entity consistency findings from the review panel: 1. The chart-target guard in _get_url had no dashboard twin: a report whose visibility-filtered dashboard relationship loads None while dashboard_id is set would fall into ``dashboard.id`` on None — an opaque AttributeError instead of an actionable error. Add ReportScheduleTargetDashboardDeletedError and guard both entry points (_get_url, and get_dashboard_urls which the async command enters directly for the permalink pre-commit). Dashboard soft delete lands in the sibling rollout, so the guard is inert until then — but the report error vocabulary stays parallel across entities from day one. Both guards are pinned by tests that fail without them. 2. Hoist the deleted-state token normalization into BaseDeletedStateFilter._normalize (single source of truth, identical text to the sibling dashboards PR so the merge is clean) and have ChartDeletedStateFilter reuse it instead of re-deriving the expression the helper exists to eliminate. Also reword the filter docstring to be timeless (no reference to unlanded rollouts).
… head apache#40128 (dashboards soft-delete) merged with migration 9e1f3b8c4d2a chaining off the previous head, so this migration re-points onto it to keep a single alembic head. Rebased onto post-apache#40128 master (0 conflicts; the shared BaseDeletedStateFilter._normalize addition merged cleanly by design — identical text on both branches).
…alogs Per @rusackas's pre-merge review: the two new messages in superset/commands/report/exceptions.py (report targets a deleted chart / dashboard) were _()-wrapped but missing from messages.pot and every messages.po. Added to all 30 catalogs with empty msgstrs, matching how the branch carries "Chart could not be restored."; every catalog validated to parse and contain byte-exact msgids. Also adds the reviewer-endorsed docstrings to the migration unit tests (helpers and the two schema tests) and type annotations on the module constants.
… head Master's migration head moved to b4a3f2e1d0c9 (apache#40527) after the last rebase; chaining off it restores the single-head invariant (per @rusackas's review note). No schema change.
Contributor
Author
|
Thanks @rusackas — done in For future rounds this is now guarded locally: a pre-push hook fails my pushes if the branch's migration graph has more than one head, so a stale |
rusackas
approved these changes
Jul 6, 2026
rusackas
left a comment
rusackas
left a comment
Member
There was a problem hiding this comment.
LGTM — all human review threads are settled (89 of 100 threads resolved; everything from @aminghadersohi, @richardfogaca, and my earlier pass is closed out, and Richard approved). CI is fully green and the whole thing ships dark behind SOFT_DELETE.
I dug into the one substantive bot thread (the error-envelope concern on get_dashboard_urls): technically observant but not blocking. That pre-commit call site raising outside the state machine predates this PR... without the guard, the same site would AttributeError opaquely instead, so the guard strictly improves the failure. The dashboard case can only fire via the validate/commit race anyway, since DeleteDashboardCommand blocks deletion with dependent reports, and the chart guard (this PR's actual subject) lives in _get_url, inside the envelope. The remaining open bot threads are style/type-hint noise and empty msgstr nags, and new strings landing untranslated is our normal flow... the i18n backfill sweeps pick those up.
aminghadersohi
approved these changes
Jul 6, 2026
Contributor
|
Bito Automatic Review Skipped – PR Already Merged |
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Wire charts to the soft-delete infrastructure from #39977 — one of three entity rollouts (charts / dashboards / datasets) decomposing the soft-delete work per SIP-208.
Slicenow inheritsSoftDeleteMixin. A globaldo_orm_executelistener excludes soft-deleted rows from every ORM SELECT, including relationship lazy loads (e.g.dashboard.slices).DELETEbecomes a soft delete; a newrestoreendpoint and achart_deleted_staterison filter let owners/admins enumerate and recover soft-deleted charts; the v1 importer treats re-import of a soft-deleted UUID as an implicit restore-with-update.What this PR ships
Migration —
superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.pyrevision = 7c4a8d09ca37,down_revision = 9e1f3b8c4d2a(master's current head, from the dashboards soft-delete migration). Adds a nullabledeleted_atcolumn and theix_slices_deleted_atindex toslices. Reversible; usessuperset.migrations.shared.utilshelpers.Model + DAO
SliceinheritsSoftDeleteMixin. The infrastructure listener filtersSlicequeries, including relationship loads viawith_loader_criteria.DeleteChartCommandroutes throughBaseDAO.delete(), which detects the mixin and dispatches tosoft_delete()— no source change to the delete command.Import pipeline —
superset/commands/chart/importers/v1/utils.pycan_writegetsdeleted_atcleared and the upload applied via UPDATE, preserving the PK and out-of-archive references (dashboard_slicesjunctions,report.chart_id, tag rows). Non-owners getImportFailedError. Callers withoutcan_writegetImportFailedErrorrather than silently receiving the soft-deleted row. Full matrix in theimport_chartdocstring.API CHANGES
POST /api/v1/chart/<uuid>/restore(new)POST /api/v1/chart/<uuid>/restore(UUID path param,format: uuid).can_write on Chart(method_permission_name["restore"] = "write",class_permission_name = "Chart") plus resource-level ownership viasecurity_manager.raise_for_ownership(admins pass).200—{ "message": "OK" };deleted_atcleared, chart returns to active state.403— caller hascan_writebut is not an owner/admin (ChartForbiddenError).404— no chart with that UUID, or the chart exists but is not soft-deleted / not visible to the caller (ChartNotFoundError).422— restore aborted by an underlying DB error (ChartRestoreFailedError).401/500— standard.chart_deleted_staterison list filter (new) — onGET /api/v1/chart/include(live + soft-deleted),only(soft-deleted only); absent / any other value → live rows only.include/onlyis used, each result row is augmented with adeleted_atfield.DELETE /api/v1/chart/<pk>and bulkDELETE /api/v1/chart/(behavior change)deleted_at, preserve the row, hide it from list/detail/lookup and relationship loads. Recoverable via the restore endpoint. Existing alert/report protection still blocks deletion of charts with active reports.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes — REST endpoints only. A frontend "Restore" affordance is a follow-up.
DELETE /api/v1/chart/<pk>deleted_at; row hidden from default queriesGET /api/v1/chart/?...&chart_deleted_state=onlydeleted_atpopulatedGET /api/v1/chart/?...&chart_deleted_state=includedeleted_atpopulated on deleted rowsPOST /api/v1/chart/<uuid>/restoredeleted_aton the soft-deleted rowTESTING INSTRUCTIONS
Automated
QA / manual — enable the
SOFT_DELETEfeature flag first (it is onmastervia #41166, default off); then:DELETE /api/v1/chart/<pk>(or delete in the UI). Confirm200, thenGET /api/v1/chart/<pk>→404and the chart no longer appears inGET /api/v1/chart/. Confirm theslicesrow still exists in the DB withdeleted_atset.GET /api/v1/chart/?q=(filters:!((col:id,opr:chart_deleted_state,value:only)))→ the chart appears with a populateddeleted_at. Repeat withvalue:include→ both live and soft-deleted rows.POST /api/v1/chart/<uuid>/restore→200 {"message":"OK"}, thenGET /api/v1/chart/<pk>returns the chart and it reappears in default lists; dashboard slot renders again.can_writebut no ownership, restore someone else's soft-deleted chart →403.404; restore a live (non-soft-deleted) UUID →404.can_write→ restored in place (same PK, dashboards reattached). Re-import as a non-owner →ImportFailedError; as a caller withoutcan_write→ImportFailedError.SOFT_DELETEand confirmDELETEhard-deletes as before and the restore endpoint /chart_deleted_statefilter are inert.ADDITIONAL INFORMATION
SOFT_DELETE(merged in feat(soft-delete): gate soft delete behind a temporary SOFT_DELETE release toggle #41166, default off — this PR ships dark)deleted_atcolumn with no default (instant on Postgres 11+ / MySQL 8+) plus a single-column indexix_slices_deleted_at. The index build is the only meaningful cost (non-blocking online on MySQL InnoDB; a few seconds on a largeslicestable on Postgres — noCONCURRENTLY, so writes toslicesmay briefly queue during the build; reads unaffected).SOFT_DELETE), thechart_deleted_staterison filter,POST /api/v1/chart/<uuid>/restore, and adeleted_atfield on list responses when the deleted-state filter is usedBehavior changes worth flagging
DELETE /api/v1/chart/<pk>is soft whenSOFT_DELETEis on. Consumers expecting permanent removal will find the chart recoverable. The row is hidden from the default API after DELETE, but directslicestable queries still see it.Depends on
SOFT_DELETErelease toggle) — merged 2026-07-01 ✅