feat(charts): soft-delete and restore#40129
Open
mikebridge wants to merge 29 commits into
Open
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.30% 64.36% +0.05%
==========================================
Files 2657 2658 +1
Lines 144060 144109 +49
Branches 33216 33220 +4
==========================================
+ Hits 92641 92758 +117
+ Misses 49797 49726 -71
- Partials 1622 1625 +3
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
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
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
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
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
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
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>
Follow-up to 58368a9fce. The if/elif structure introduced there still gated id-preservation on `user`, which broke the example-loader path (load-examples CLI passes ignore_permissions=True with no logged-in user). For that call shape, neither inner branch fires and the import falls through to INSERT, colliding with the UUID unique index on the existing row. Restructured to mirror master's original sequential-if shape: 1) gate the owner check on `overwrite and can_write and user` (unchanged) 2) gate the early return on `not overwrite or not can_write` 3) fall through to the overwrite path: restore-in-place + set config[id] The overwrite path now runs whenever the function did not return early, matching pre-soft-delete behavior on the user-None case. New test pins the example-loader regression: test_import_soft_deleted_chart_ignore_permissions_restores_in_place Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (Option C) Previously, my fix raised ImportFailedError on non-overwrite + soft-deleted match. That broke the legitimate re-import-after-delete UX flow exercised by the playwright dashboard zip import test: user deletes (soft) a chart, later re-uploads the zip, expects to get it back. The agent's "silent resurrection" concern was specifically about dashboard imports REATTACHING to soft-deleted charts via chart_ids[uuid] indexing, bypassing the explicit RestoreChartCommand's ownership check. Addressed here by applying that same ownership check at the importer level: any mutation path (overwrite OR soft-delete restore) verifies the user owns the chart or is admin before mutating. Non-owner attempts to re-import a soft-deleted UUID raise the same error as non-owner overwrite attempts. Net behavior: - alive + overwrite: same as before (owner check, UPDATE) - alive + non-overwrite: same as before (return existing) - soft-deleted + overwrite + owner: restore in place + UPDATE (unchanged from prior fix) - soft-deleted + non-overwrite + owner: NEW — restore in place + UPDATE (treats re-import as implicit restore; matches user intent and unblocks the playwright zip-import test) - soft-deleted + any-overwrite + non-owner: raise (security preserved) - soft-deleted + overwrite + user=None (example loader): restore + UPDATE Tests updated: - test_import_soft_deleted_chart_non_overwrite_raises -> renamed to test_import_soft_deleted_chart_non_overwrite_restores_for_owner, asserting the new implicit-restore behavior with an admin user - Added test_import_soft_deleted_chart_non_overwrite_raises_for_non_owner, pinning the ownership check that protects against unauthorized restore Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the remaining gap in the Option-C restore-via-import path. The ownership check at f4768c4 protected the case where a caller has can_write but isn't the owner. This commit handles the orthogonal case: a caller without can_write at all. Real-world scenario: a user holds can_write Dashboard but not can_write Chart, and imports a dashboard zip referencing a soft-deleted chart. Previously: import_chart fell through to `if not needs_mutation or not can_write: return existing` and silently returned the soft-deleted row. The dashboard importer then indexed it via chart_ids[uuid] and produced a dashboard pointing at a hidden chart -- "successful" import, broken result. Now: raise ImportFailedError before the fall-through if the row is soft-deleted and the caller lacks can_write. Surfaces the broken state to the operator instead of silently producing it. Also tightened the ownership-check error message to distinguish restore-vs-overwrite intent ("permissions to restore" vs "permissions to overwrite"), since the user didn't explicitly request an overwrite on the implicit-restore path. Added a Case B test pinning the new raise. Updated the existing non-owner-raises test to match the new message text. Added a docstring matrix on import_chart documenting the full permission model -- the next contributor sees the cross-entity rules inline. See specs/sc-103157-soft-deletes/bypass-primer.md for the shared reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…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>
…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>
…1.2.0) The migration docstring previously opened with a sentence fragment (``add deleted_at to slices for soft delete`` — lowercase, no period), which is the Alembic auto-generated template format. The constitution v1.2.0 PEP 257 rule (sourced from Beto's review on apache#39977) requires the first line to be a self-contained one-line summary ending in a period that reads as a complete sentence on its own. Tightening just this migration to comply, not a project-wide sweep — the rule applies going forward. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…l location PR apache#40573 moved SKIP_VISIBILITY_FILTER_CLASSES from superset/models/helpers.py to superset/constants.py (which is where plain string constants belong and which doesn't trigger the eager model-load chain). superset/models/helpers.py still re-exports it for backwards compat, but the canonical import path is now superset.constants. Updating to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "See specs/sc-103157-soft-deletes/bypass-primer.md ..." line points at a path in the dev team's private spec repo, not the apache/superset tree. External readers (PMC reviewers, downstream consumers) see a dead reference. The permission-matrix docstring above already states the rules in full, so the pointer was redundant rather than additive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Shortcut ticket IDs (sc-XXXXXX) are internal to the dev team and unresolvable for external readers. Drop them from the slices soft-delete migration docstring and the integration-test module docstring; the PR description and commit history carry the trail for anyone who needs it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four follow-ups addressing items flagged on apache#40129: 1. Move `find_existing_for_import` to module-top imports (commands/chart/importers/v1/utils.py). The import was inside the function body without a circular-import comment. Verified no cycle exists: `superset.commands.importers.v1.utils` does not reach `superset.commands.chart.importers.v1.utils` through any of its imports. Project convention requires a `# avoid circular import` comment when an inline import is genuinely necessary; without that justification the import belongs at module level. 2. Document the `user=None` + `can_write=True` permission-skip path. When `get_user()` returns None (background import / example loader) and `can_write` is True (typically from `ignore_permissions=True`), the ownership check at lines 88-93 short-circuits because the guard reads `if needs_mutation and can_write and user:`. The matrix docstring did not cover this row; added an inline comment at the `user = get_user()` call explaining the intentional bypass and how it relates to `needs_mutation` now applying to soft-deleted matches (previously only `overwrite=True` triggered mutation). 3. Drop the redundant `get_user()` call near the end of `import_chart`. The local `user` binding from line 88 is still in scope; the walrus expression `if (user := get_user())` was both rebinding the outer scope and issuing a second call for the same value within the same request. Replaced with `if user and user not in chart.owners:`. 4. Correct UPDATING.md's lock semantics for the schema migration. The note claimed the index build "may briefly block reads on the slices table", but Postgres `CREATE INDEX` (non-CONCURRENT) acquires a `ShareLock` which blocks INSERT/UPDATE/DELETE, not SELECT. Reads continue uninterrupted; writes are queued during the build. MySQL InnoDB builds the index online with no blocking (unchanged). Also added a rollback note about visibility-filter semantics: rolling back the application code after rows have been soft-deleted exposes those rows to the older code path, which had no `deleted_at` filter. Operators should pair rollback with a data decision. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Amin's NIT-tier flag on apache#40129: all twelve test methods in ``tests/integration_tests/charts/soft_delete_tests.py`` were missing ``-> None`` annotations. The unit tests in ``tests/unit_tests/commands/chart/restore_test.py`` already follow this convention, as do the wider codebase's test suites. Not addressed in this commit (deferred): - ``self.addCleanup(_hard_delete_chart, chart.id)`` pattern. Amin also flagged that the trailing ``_hard_delete_chart(chart_id)`` cleanup calls don't run if an earlier assertion raises, leaving the chart orphaned in the test DB. Switching to ``addCleanup`` would be a worthwhile refactor but expands the diff substantially across all twelve tests; the current trailing-cleanup pattern matches what the rest of the soft-delete test suites do and works in the green-CI happy path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
``ChartRestoreFailedError`` inherited ``CommandException`` directly, skipping the typed middle-tier base used by every other failure exception in this module (``ChartDeleteFailedError(DeleteFailedError)``, ``ChartUpdateFailedError(UpdateFailedError)``, etc.). Inherit from ``UpdateFailedError`` — restore semantically clears ``deleted_at``, so it is an UPDATE, not a new row. This is the same change applied to ``DatasetRestoreFailedError`` in the datasets PR (apache#40130) per Amin's review. ``DashboardRestoreFailedError`` gets the same treatment on its branch (apache#40128). The three branches end up consistent; a dedicated ``RestoreFailedError`` in ``superset/commands/exceptions.py`` would be more precise still but lives in already-merged infrastructure (apache#39977) and can be a cross-entity follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per bito-code-review's flag on PR apache#40129 discussion r3338050658: ``ChartRestApi.method_permission_name`` set to bare ``MODEL_API_RW_METHOD_PERMISSION_MAP`` did not contain a mapping for ``restore``, so FAB's ``@protect()`` decorator was falling back to ``can_<method>_<class>`` (i.e. ``can_restore_Chart``) for the endpoint gate. That permission does not exist on any standard role — Alpha (the intended caller for owner-driven restore) carries ``can_write_Chart``, not ``can_restore_Chart``. So the endpoint was silently broken for non-admin owners: every restore call would have been rejected at FAB before reaching the command layer. Admin calls succeeded only because Admin bypasses FAB permission checks entirely, which is why the admin-authed integration tests went green and the gap stayed latent. The PR description states the restore endpoint "mirrors delete exactly: endpoint-level can_write, resource-level raise_for_ownership." That claim now matches reality. Fix mirrors the pattern in ``themes/api.py`` for custom methods (``set_system_default``, etc.): spread ``MODEL_API_RW_METHOD_PERMISSION_MAP`` into a new dict and add ``"restore": "write"``. Inline comment notes the FAB fallback behaviour so a future contributor doesn't undo the mapping by accident. Regression test ``test_restore_uses_can_write_permission`` logs in as Alpha (who carries ``can_write_Chart`` but not ``can_restore_Chart``), creates an Alpha-owned chart, soft-deletes it, and verifies the restore endpoint returns 200. If the mapping is ever removed, this test will fail with a 403 from FAB at the endpoint gate, before the command layer runs. Same gap exists on dashboards (apache#40128) and datasets (apache#40130); fixed on those branches in companion commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sion Two follow-ups from Amin's re-review on apache#40129: 1. Move all eight unit-test inline imports in ``tests/unit_tests/commands/chart/restore_test.py`` to module top. The imports (RestoreChartCommand, ChartNotFoundError, ChartForbiddenError, SupersetSecurityException) are pure Python class definitions with no app-context dependency, no circular imports, and no reason to defer beyond cargo-cult Bucket 3. Per constitution v1.3.2 the burden is on the inline form to justify itself; without that justification module-top is the default. Also tightened ``datetime(2026, 1, 1)`` calls with ``tzinfo=timezone.utc`` for consistency with dashboards (bito raised this on dashboards earlier). 2. Move two integration-test inline imports in ``tests/integration_tests/charts/soft_delete_tests.py`` to module top: ``dashboard_slices`` (line 331) is in the same module as ``Dashboard`` which is already imported at the top, and the ``superset.reports.models`` symbols (line 185) are safe to import at module top in an integration-test context where the app is initialized via ``SupersetTestCase``. 3. Update migration ``down_revision`` from ``33d7e0e21daa`` to ``a1b2c3d4e5f6`` (the current alembic head on master). Without this update, ``alembic upgrade head`` would fail with "Multiple head revisions present" once the migration lands on a database that already has master's chain through ``a1b2c3d4e5f6``. The sibling dashboards (apache#40128) and datasets (apache#40130) PRs need the same update on their migrations; cross-PR merge-order coordination still applies — whichever lands first stays at ``a1b2c3d4e5f6``, the others rebase onto the newly-merged revision. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… head) CI failed across all three soft-delete PRs with "Multiple head revisions present" after the recent down_revision update. Root cause: I misread the alembic chain order based on file dates rather than the actual ``down_revision`` linkage. Tracing the chain on master: 4b2a8c9d3e1f → a1b2c3d4e5f6 → ce6bd21901ab → 33d7e0e21daa Verifying each link directly: 33d7e0e21daa.down_revision = ce6bd21901ab ce6bd21901ab.down_revision = a1b2c3d4e5f6 Nothing chains off ``33d7e0e21daa``; multiple things (including ``ce6bd21901ab``) chain off ``a1b2c3d4e5f6``. The true alembic head on master is therefore ``33d7e0e21daa``, not ``a1b2c3d4e5f6``. The file-date heuristic was misleading because ``33d7e0e21daa`` (created 2025-11-04 on a feature branch) was merged after ``a1b2c3d4e5f6`` / ``ce6bd21901ab`` (created 2026-03-02) — file creation dates do not reflect alembic chain order when migrations land via feature branches. Reverting ``down_revision`` to ``33d7e0e21daa`` restores the original (correct) chain placement. The migration once again makes ``7c4a8d09ca37`` the single alembic head when this PR is applied, and ``alembic upgrade head`` no longer reports multiple heads. The sibling dashboards (apache#40128) and datasets (apache#40130) PRs get the same revert in companion commits. 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>
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #1bfcc1
Actionable Suggestions - 2
-
superset/translations/th/LC_MESSAGES/messages.po - 1
- Missing Thai translation · Line 5239-5240
-
superset/translations/cs/LC_MESSAGES/messages.po - 1
- Missing Czech translation · Line 3110-3110
Additional Suggestions - 9
-
tests/unit_tests/commands/chart/restore_test.py - 1
-
Missing ChartRestoreFailedError test · Line 34-90The test file covers ChartNotFoundError and ChartForbiddenError but omits testing the ChartRestoreFailedError path. RestoreChartCommand.run() wraps the restore in @transaction which re-throws SQLAlchemy errors as ChartRestoreFailedError (restore.py line 69).
-
-
superset/translations/fr/LC_MESSAGES/messages.po - 1
-
Missing French Translation · Line 3120-3121The `msgstr` field is empty. French users will see the untranslated English text "Chart could not be restored." instead of a proper French translation. Compare with the pattern used for similar messages on lines 3118 and 3124: "Le graphique n'a pas pu être créé." and "Le graphique n'a pas pu être mis à jour."
Code suggestion
--- superset/translations/fr/LC_MESSAGES/messages.po (lines 3120-3121)--- 3120: msgid "Chart could not be restored." 3121:-msgstr "" 3121:+msgstr "Le graphique n'a pas pu être restauré."
-
-
superset/translations/fa/LC_MESSAGES/messages.po - 1
-
Missing Persian translation · Line 3068-3069The `msgstr` is empty for the new translation entry. Persian users will see the untranslated English message 'Chart could not be restored.' when a chart restore operation fails. Compare with adjacent entries: 'Chart could not be created.' → 'نمودار نتوانسته ایجاد شود.' and 'Chart could not be updated.' → 'نمودار نتوانست بهروزرسانی شود.' — the pattern uses 'نتوانست' (could not) + verb.
Code suggestion
--- superset/translations/fa/LC_MESSAGES/messages.po (lines 3068-3069) --- 3068: msgid "Chart could not be restored." 3069: -msgstr "" 3069: +msgstr "نمودار نتوانست بازگردانی شود."
-
-
superset/translations/nl/LC_MESSAGES/messages.po - 1
-
CWE-20: Empty Translation String · Line 3122-3122The `msgstr` is empty, causing Dutch-speaking users to see the English fallback message 'Chart could not be restored.' instead of a translated error. Based on the translation patterns for similar messages ('Grafiek kon niet worden aangemaakt.' and 'De grafiek kon niet worden bijgewerkt.'), the correct Dutch translation should be 'De grafiek kon niet worden hersteld.'.
Code suggestion
--- a/superset/translations/nl/LC_MESSAGES/messages.po +++ b/superset/translations/nl/LC_MESSAGES/messages.po @@ -3119,7 +3119,7 @@ msgstr "Grafiek kon niet worden aangemaakt." msgid "Chart could not be restored." -msgstr "" +msgstr "De grafiek kon niet worden hersteld." msgid "Chart could not be updated." msgstr "De grafiek kon niet worden bijgewerkt."
-
-
superset/translations/pt_BR/LC_MESSAGES/messages.po - 1
-
Missing Portuguese translation · Line 3197-3198The new translation entry for "Chart could not be restored." has an empty `msgstr`. Users will see the English text instead of Portuguese, unlike the adjacent messages "Chart could not be created." and "Chart could not be updated." which are properly translated. Add the translation: "Não foi possível restaurar o gráfico."
Code suggestion
--- superset/translations/pt_BR/LC_MESSAGES/messages.po +++ superset/translations/pt_BR/LC_MESSAGES/messages.po @@ -3195,7 +3195,7 @@ msgstr "Não foi possível criar o gráfico." msgid "Chart could not be restored." -msgstr "" +msgstr "Não foi possível restaurar o gráfico." msgid "Chart could not be updated." msgstr "Não foi possível atualizar o gráfico."
-
-
superset/charts/api.py - 1
-
Missing non-owner restore test · Line 594-648The unit test `test_restore_chart_forbidden_raises` mocks `raise_for_ownership` to verify `ChartForbiddenError` is raised, but no integration test exercises the full authorization path from the API endpoint. Adding a test where a non-owner Gamma user attempts `POST /api/v1/chart//restore` on an admin-owned soft-deleted chart would confirm the 403 response is returned end-to-end.
-
-
superset/translations/pt/LC_MESSAGES/messages.po - 1
-
Missing Portuguese translation · Line 3048-3049The Portuguese translation for this new message is empty. When users trigger a chart-restore failure, they will see an untranslated message instead of a localized error. Following the pattern of adjacent entries (lines 3045-3052), use the form 'Não foi possível [action]' — e.g., 'Não foi possível restaurar o seu gráfico.'
Code suggestion
--- superset/translations/pt/LC_MESSAGES/messages.po (line 3048-3049) --- 3048: msgid "Chart could not be restored." 3049:-msgstr "" 3049:+msgstr "Não foi possível restaurar o seu gráfico."
-
-
superset/translations/it/LC_MESSAGES/messages.po - 1
-
Missing Italian translation · Line 3002-3003Empty msgstr leaves Italian users seeing untranslated English error message. While gettext falls back to msgid when msgstr is empty, this creates a degraded user experience. Compare with adjacent "Chart could not be created" and "Chart could not be updated" which have Italian translations (though they use identical incorrect text "La tua query non può essere salvata"). Consider adding a consistent Italian translation like "Il grafico non può essere ripristinato." for proper localization.
Code suggestion
--- superset/translations/it/LC_MESSAGES/messages.po (lines 3002-3003) --- 3002: msgid "Chart could not be restored." -msgstr "" +msgstr "Il grafico non pu\u00f2 essere ripristinato." 3003: +
-
-
superset/charts/filters.py - 1
-
Missing unit tests for filter · Line 186-220ChartDeletedStateFilter has integration tests but no dedicated unit tests. Add unit tests following the pattern in test_soft_delete_filter.py to cover the ownership-scoped visibility logic (admin bypass, owner access, non-owner exclusion) at the filter class level.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/commands/chart/restore_test.py - 1
- Wrong UUID parameter in command · Line 48-48
Review Details
-
Files reviewed - 40 · Commit Range:
753c184..b353519- superset/charts/api.py
- superset/charts/filters.py
- superset/commands/chart/delete.py
- superset/commands/chart/exceptions.py
- superset/commands/chart/importers/v1/utils.py
- superset/commands/chart/restore.py
- superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py
- superset/models/slice.py
- superset/translations/ar/LC_MESSAGES/messages.po
- superset/translations/ca/LC_MESSAGES/messages.po
- superset/translations/cs/LC_MESSAGES/messages.po
- superset/translations/de/LC_MESSAGES/messages.po
- superset/translations/en/LC_MESSAGES/messages.po
- superset/translations/es/LC_MESSAGES/messages.po
- superset/translations/fa/LC_MESSAGES/messages.po
- superset/translations/fi/LC_MESSAGES/messages.po
- superset/translations/fr/LC_MESSAGES/messages.po
- superset/translations/it/LC_MESSAGES/messages.po
- superset/translations/ja/LC_MESSAGES/messages.po
- superset/translations/ko/LC_MESSAGES/messages.po
- superset/translations/lv/LC_MESSAGES/messages.po
- superset/translations/messages.pot
- superset/translations/mi/LC_MESSAGES/messages.po
- superset/translations/nl/LC_MESSAGES/messages.po
- superset/translations/pl/LC_MESSAGES/messages.po
- superset/translations/pt/LC_MESSAGES/messages.po
- superset/translations/pt_BR/LC_MESSAGES/messages.po
- superset/translations/ru/LC_MESSAGES/messages.po
- superset/translations/sk/LC_MESSAGES/messages.po
- superset/translations/sl/LC_MESSAGES/messages.po
- superset/translations/th/LC_MESSAGES/messages.po
- superset/translations/tr/LC_MESSAGES/messages.po
- superset/translations/uk/LC_MESSAGES/messages.po
- superset/translations/zh/LC_MESSAGES/messages.po
- superset/translations/zh_TW/LC_MESSAGES/messages.po
- tests/integration_tests/charts/commands_tests.py
- tests/integration_tests/charts/soft_delete_tests.py
- tests/unit_tests/charts/commands/importers/v1/import_test.py
- tests/unit_tests/commands/chart/restore_test.py
- tests/unit_tests/migrations/test_add_deleted_at_to_slices.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
…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>
Contributor
Author
|
Final triage sweep of the open bot threads (codeant + bito), regenerated by the recent pushes — resolving as not actionable:
|
Contributor
Code Review Agent Run #2bf6c1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
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
5 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 shipped in #39977. One of three entity rollouts (charts / dashboards / datasets) decomposing the soft-delete work per SIP-208 (passed 2026-05-08).
#39977 (
b2320820b4) merged on 2026-05-29; this PR consumes the abstractions (SoftDeleteMixin,BaseRestoreCommand,BaseDeletedStateFilter,SoftDeleteApiMixin,find_existing_for_import) and wires them toSlice.What this PR ships
Migration (
superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py)revision = 7c4a8d09ca37,down_revision = 31dae2559c05. Adds a nullabledeleted_atcolumn andix_slices_deleted_atindex toslices. Reversible. Usessuperset.migrations.shared.utilshelpers so the migration is idempotent.down_revision = 31dae2559c05with the dashboards (feat(dashboards): soft-delete and restore #40128) and datasets (feat(datasets): soft-delete and restore #40130) sibling PRs. Merge sequence: dashboards (feat(dashboards): soft-delete and restore #40128) → charts (feat(charts): soft-delete and restore #40129) → datasets (feat(datasets): soft-delete and restore #40130); each later PR re-points itsdown_revisionto the just-merged migration before merging (a plain rebase would create an alembic multi-head).Model + DAO
SliceinheritsSoftDeleteMixin(superset/models/slice.py). Thedo_orm_executelistener from the infrastructure PR begins filteringSlicequeries (including relationship lazy loads, viawith_loader_criteria(..., propagate_to_loaders=True)).DeleteChartCommandroutes throughBaseDAO.delete()— which now detects the mixin and dispatches tosoft_delete()— so no source change to the delete command is needed.API
RestoreChartCommand(superset/commands/chart/restore.py) — 4 ClassVar declarations subclassingBaseRestoreCommand[Slice]. No method overrides; the base class owns validation, ownership checks, and the transactional wrapper.POST /api/v1/chart/<uuid>/restoreendpoint (superset/charts/api.py) with the standard decorator stack (@protect,@safe,@statsd_metrics,@event_logger.log_this_with_context). Permission model mirrors delete exactly: endpoint-levelcan_write, resource-levelraise_for_ownership. UUIDs in the path per the new-endpoints convention.ChartDeletedStateFilter(superset/charts/filters.py) — 2-line subclass ofBaseDeletedStateFiltersettingarg_name = "chart_deleted_state"andmodel = Slice. Wired intosearch_filtersonChartRestApi.ChartRestApigainsSoftDeleteApiMixinand"restore"is added toinclude_route_methods.ChartRestoreFailedErrorexception type added (superset/commands/chart/exceptions.py).Import pipeline (
superset/commands/chart/importers/v1/utils.py)import_chartto follow the Option C permission model: re-importing a UUID that matches a soft-deleted chart is treated as an implicit restore-and-update. For an owner (or admin) withcan_write, the importer clearsdeleted_aton the existing row and routesimport_from_dictthrough UPDATE, preserving the PK and out-of-archive references (dashboard_slicesjunctions,report.chart_id, tags). Non-owners getImportFailedError. Callers withoutcan_writealso getImportFailedErrorrather than silently receiving the soft-deleted row (Case B — protects dashboard imports from quietly reattaching to deleted charts). The full matrix is documented in the docstring onimport_chart.db.session.flush()after clearingdeleted_atmakes the restore-then-UPDATE chain independent of session autoflush (import_from_dictre-queries by UUID; the listener filtersdeleted_at IS NULL; the explicit flush guarantees the cleared value is visible before that query).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes in this PR — REST endpoints only. The frontend "Restore" button is a follow-up PR.
DELETE /api/v1/chart/<pk>deleted_at; row remains but is 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
The integration suite covers:
deleted_atinstead of removing the rowchart_deleted_state=onlyreturns soft-deleted withdeleted_atpopulatedchart_deleted_state=includereturns both, response augmenteddeleted_at(admin and owner paths)raise_for_ownership)ADDITIONAL INFORMATION
slicestable. Even on large deployments, adding a nullable column with no default is instant on Postgres 11+ and MySQL 8+. The index build is the only meaningful cost; on a 1M-rowslicestable, expect a few seconds withALGORITHM=INPLACE, LOCK=NONE(MySQL/InnoDB) or non-blocking on Postgres for a small column. No lock-holding for typical deployments.chart_deleted_staterison filter,POST /api/v1/chart/<uuid>/restoreendpoint,deleted_atfield on list responses when augmentation is opted intoBehaviour changes worth flagging
DELETE /api/v1/chart/<pk>is now soft. Existing API consumers that called DELETE expecting permanent removal will see the chart reappear if it's restored. The row is no longer findable via the default API after DELETE (the listener hides it), so most downstream behaviour is preserved — but anyone doing direct DB queries on theslicestable will see the row.Depends on
Coordination with sibling PRs
This PR's migration shares
down_revision = 31dae2559c05with:9e1f3b8c4d2a3a8e6f2c1b95The first of the three to merge (dashboards) keeps its
down_revision; each later PR re-points itsdown_revisionto the just-merged migration before merging — a plain rebase would create an alembic multi-head, which theCheck DB migration conflictCI job will catch.