feat(datasets): soft-delete and restore#40130
Merged
rusackas merged 14 commits intoJul 7, 2026
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 #40130 +/- ##
==========================================
- Coverage 64.74% 64.73% -0.01%
==========================================
Files 2687 2688 +1
Lines 148743 148942 +199
Branches 34329 34356 +27
==========================================
+ Hits 96301 96419 +118
- Misses 50678 50748 +70
- Partials 1764 1775 +11
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-106191-datasets-soft-delete
branch
from
May 18, 2026 17:03
fefb486 to
ec98d74
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-106191-datasets-soft-delete
branch
5 times, most recently
from
May 20, 2026 22:36
5995e2d to
8da8ce4
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-106191-datasets-soft-delete
branch
2 times, most recently
from
May 21, 2026 15:44
1d78910 to
8cb25e1
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
force-pushed
the
sc-106191-datasets-soft-delete
branch
2 times, most recently
from
May 25, 2026 19:41
5cb3b5f to
bf87b03
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 25, 2026
CI test-postgres (current) on PR apache#40130 cascade-failed with: psycopg2.errors.UniqueViolation: duplicate key value violates unique constraint "_customer_location_uc" DETAIL: Key (database_id, schema, table_name)=(1, public, birth_names) already exists. Root cause: dashboard_utils.get_table queries SqlaTable through the ORM, which the soft-delete listener now filters. When a prior test in the same session soft-deletes the birth_names row (any test exercising the new soft-delete DELETE behavior on datasets), the row stays in the DB but is hidden from the helper. create_table_metadata's "doesn't exist, INSERT" path then collides with the underlying (database_id, schema, table_name) unique constraint that survives soft-delete (the constraint is enforced even though the SQLAlchemy comment claims it's not physical — Postgres test environments enforce it via a named constraint that an old migration tried to drop). Two-part fix in tests/integration_tests/dashboard_utils.py: 1. get_table attaches a per-query SKIP_VISIBILITY_FILTER_CLASSES execution option so it sees soft-deleted leftovers. Scoped to the single query — no leak to anything else in the session. 2. create_table_metadata restores any soft-deleted row it finds before mutating it for the new test's setup. Without this, the test would succeed in finding the row but then operate on a row whose `deleted_at` is still set, which the listener would hide from any subsequent API queries inside the same test. Mirrors the pattern the dashboards branch (sc-106190) uses for insert_dashboard's defensive cleanup, scoped narrowly to the test helper and limited to SqlaTable. 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
CI test-postgres (current) on PR apache#40130 cascade-failed with: psycopg2.errors.UniqueViolation: duplicate key value violates unique constraint "_customer_location_uc" DETAIL: Key (database_id, schema, table_name)=(1, public, birth_names) already exists. Root cause: dashboard_utils.get_table queries SqlaTable through the ORM, which the soft-delete listener now filters. When a prior test in the same session soft-deletes the birth_names row (any test exercising the new soft-delete DELETE behavior on datasets), the row stays in the DB but is hidden from the helper. create_table_metadata's "doesn't exist, INSERT" path then collides with the underlying (database_id, schema, table_name) unique constraint that survives soft-delete (the constraint is enforced even though the SQLAlchemy comment claims it's not physical — Postgres test environments enforce it via a named constraint that an old migration tried to drop). Two-part fix in tests/integration_tests/dashboard_utils.py: 1. get_table attaches a per-query SKIP_VISIBILITY_FILTER_CLASSES execution option so it sees soft-deleted leftovers. Scoped to the single query — no leak to anything else in the session. 2. create_table_metadata restores any soft-deleted row it finds before mutating it for the new test's setup. Without this, the test would succeed in finding the row but then operate on a row whose `deleted_at` is still set, which the listener would hide from any subsequent API queries inside the same test. Mirrors the pattern the dashboards branch (sc-106190) uses for insert_dashboard's defensive cleanup, scoped narrowly to the test helper and limited to SqlaTable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106191-datasets-soft-delete
branch
from
May 28, 2026 19:59
6a9640a to
c9c9011
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
CI test-postgres (current) on PR apache#40130 cascade-failed with: psycopg2.errors.UniqueViolation: duplicate key value violates unique constraint "_customer_location_uc" DETAIL: Key (database_id, schema, table_name)=(1, public, birth_names) already exists. Root cause: dashboard_utils.get_table queries SqlaTable through the ORM, which the soft-delete listener now filters. When a prior test in the same session soft-deletes the birth_names row (any test exercising the new soft-delete DELETE behavior on datasets), the row stays in the DB but is hidden from the helper. create_table_metadata's "doesn't exist, INSERT" path then collides with the underlying (database_id, schema, table_name) unique constraint that survives soft-delete (the constraint is enforced even though the SQLAlchemy comment claims it's not physical — Postgres test environments enforce it via a named constraint that an old migration tried to drop). Two-part fix in tests/integration_tests/dashboard_utils.py: 1. get_table attaches a per-query SKIP_VISIBILITY_FILTER_CLASSES execution option so it sees soft-deleted leftovers. Scoped to the single query — no leak to anything else in the session. 2. create_table_metadata restores any soft-deleted row it finds before mutating it for the new test's setup. Without this, the test would succeed in finding the row but then operate on a row whose `deleted_at` is still set, which the listener would hide from any subsequent API queries inside the same test. Mirrors the pattern the dashboards branch (sc-106190) uses for insert_dashboard's defensive cleanup, scoped narrowly to the test helper and limited to SqlaTable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106191-datasets-soft-delete
branch
from
May 28, 2026 20:08
c9c9011 to
83445b3
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
Three SQLAlchemy-review follow-ups in import_dataset, mirroring the charts (16b5ae9) and dashboards (adf511b) passes: 1. **Explicit flush on restore-and-update path + rewritten comment.** The implicit-restore flow relied on autoflush to push the deleted_at = NULL update before SqlaTable.import_from_dict's internal query-by- uuid lookup ran. Without it, the listener would have filtered the row out, 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. The inline comment is rewritten to describe the actual mechanism (the previous wording claimed config["id"] "routes import_from_dict through UPDATE", but ImportExportMixin strips non-export_fields keys — so config["id"] is defensive and the real bind happens via uuid uniqueness inside import_from_dict). 2. **Force sync on the implicit-restore path.** Previously `sync = ["columns", "metrics"] if overwrite else []`, so re-importing a soft-deleted dataset hit `sync = []` and ImportExportMixin would upsert children by UUID without removing rows present in the live DB but absent from the upload. Net effect: a stale-then-restored dataset carried orphan columns from before the soft-delete — a surprising merge of two states. Now ``sync = ["columns", "metrics"] if (overwrite or is_soft_deleted_match) else []`` so re-import of a soft-deleted UUID is treated as a clean replacement, matching what an explicit overwrite would do anyway. New test test_import_soft_deleted_dataset_restore_removes_orphan_children pins this contract. 3. **MultipleResultsFound fallback bypass.** The post-exception query `db.session.query(SqlaTable).filter_by(uuid=...).one()` didn't bypass the soft-delete listener, so a soft-deleted duplicate would be hidden and `.one()` would raise NoResultFound, masking the original MultipleResultsFound. Added SKIP_VISIBILITY_FILTER_CLASSES execution option to the fallback so it can locate either state. All three items raised by the SQLAlchemy lens review on PR apache#40130. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106191-datasets-soft-delete
branch
from
June 1, 2026 18:50
acc8884 to
72049a9
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Contributor
Author
|
@aminghadersohi — closing out the two remaining items from your 2026-06-29 review body (the two inline findings from that pass were fixed in
That closes every finding from all four of your passes on this PR. |
![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 #1dca5c
Actionable Suggestions - 3
-
superset/translations/fi/LC_MESSAGES/messages.po - 1
- Unreviewed fuzzy translation flag · Line 8304-8304
-
tests/unit_tests/commands/dataset/restore_test.py - 1
- Missing assertion on DAO call arguments · Line 57-57
-
superset/commands/database/delete.py - 1
- Missing test for soft-deleted datasets · Line 101-102
Additional Suggestions - 2
-
tests/unit_tests/databases/commands/delete_test.py - 1
-
Test fixture inconsistency · Line 25-63Replace the two inline ``SqlaTable.metadata.create_all(session.get_bind())`` calls with a module-scoped ``_synthetic_tables`` fixture (drop-in copy of the pattern in ``test_soft_delete_mixin.py``) plus ``@pytest.mark.usefixtures("_synthetic_tables")`` decorators — consistent with the 18 other tests in that file that follow this convention.
-
-
superset/commands/dataset/duplicate.py - 1
-
Missing test coverage for run() · Line 68-74The existing test suite for duplicate.py has no tests for the run() method. While the catalog assignment (line 71) is correct and consistent with the model, there is no test verifying that the catalog attribute is preserved when duplicating a dataset.
-
Review Details
-
Files reviewed - 55 · Commit Range:
55419f6..0b05c4b- superset/commands/database/delete.py
- superset/commands/database/exceptions.py
- superset/commands/database/sync_permissions.py
- superset/commands/database/uploaders/base.py
- superset/commands/dataset/duplicate.py
- superset/commands/dataset/exceptions.py
- superset/commands/dataset/importers/v1/utils.py
- superset/commands/dataset/restore.py
- superset/connectors/sqla/models.py
- superset/daos/dataset.py
- superset/daos/datasource.py
- superset/datasets/api.py
- superset/datasets/filters.py
- superset/migrations/versions/2026-05-08_12-10_3a8e6f2c1b95_add_deleted_at_to_tables.py
- superset/security/manager.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/dashboard_utils.py
- tests/integration_tests/datasets/api_tests.py
- tests/integration_tests/datasets/soft_delete_tests.py
- tests/integration_tests/security_tests.py
- tests/unit_tests/commands/databases/sync_permissions_test.py
- tests/unit_tests/commands/databases/upload_command_test.py
- tests/unit_tests/commands/dataset/restore_test.py
- tests/unit_tests/commands/dataset/test_duplicate.py
- tests/unit_tests/dao/dataset_test.py
- tests/unit_tests/dao/datasource_test.py
- tests/unit_tests/databases/commands/delete_test.py
- tests/unit_tests/datasets/commands/importers/v1/import_test.py
- tests/unit_tests/migrations/test_add_deleted_at_to_tables.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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
When the SOFT_DELETE feature flag is enabled (default off — ships dark), DELETE /api/v1/dataset/<id> and the bulk-delete endpoint soft-delete: the row is marked with deleted_at, hidden from list/detail/lookup endpoints and relationship loads, and recoverable via the new POST /api/v1/dataset/<uuid>/restore endpoint (can_write + ownership, or admin). The restore audience can enumerate soft-deleted rows via the dataset_deleted_state rison filter (include|only). With the flag off, deletes remain permanent hard deletes. Highlights: - Migration 3a8e6f2c1b95 adds nullable deleted_at + index to tables (down_revision 2bee73611e32, current master head). - Create/update/duplicate uniqueness checks bypass the visibility filter (session-scoped bypass — per-query execution options don't reach the listener from EXISTS subqueries) so hidden twins still block; restore validates against active logical duplicates and returns 422. - Importer: a dataset YAML whose UUID matches a soft-deleted row is an implicit restore-with-update (ownership-gated), preserving the PK, table_columns, sql_metrics, and chart back-references; duplicate guards cover both directions. - Deleting a database stays blocked while soft-deleted datasets still reference it (documented trade-off until purge lands). - Perm-string maintenance on database rename rewrites soft-deleted datasets' perm/schema_perm/catalog_perm (and their charts') through the visibility bypass, in both the security-manager and SyncPermissionsCommand paths. - FAB view-menu/permission rows are preserved on soft delete so restore brings access back intact; hard delete (flag off) still cleans up. - Flag-gated integration suites for soft delete and restore, importer permission-matrix unit tests, migration up/down tests; UPDATING.md documents the flag gate, semantics, and the two validation changes that apply regardless of the flag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…leted rows Two review-bot findings verified and fixed: - The create_datasets fixture purge deleted every dataset matching the fixture table names (the lookup bypasses the visibility filter and matches by name only), so it could hard-delete live datasets other suites created over the same AB tables. Restrict it to rows with deleted_at set, matching the comment's stated intent. - test_deleted_state_list_shows_owner_their_own_deleted ran its teardown only on the happy path, unlike its siblings which use try/finally; a failed assertion stranded the soft-deleted dataset/database in the shared test DB. Wrap in try/finally to match.
… head apache#40128 (dashboards soft-delete) merged with migration 9e1f3b8c4d2a chaining off the previous head; re-point onto it to keep a single alembic head. Rebased onto post-apache#40128 master (0 conflicts).
Addresses the two remaining review-body items from aminghadersohi's 2026-06-29 pass (the inline findings from that pass were fixed in b9f6282; these two lived in the review body): - MEDIUM: get_or_create_dataset returned an opaque "already exists" 422 when a soft-deleted dataset holds the same physical table — while the caller's dataset list looks empty. Pre-check with find_soft_deleted_logical_duplicate (the same helper the YAML importer uses) and return a targeted message naming the hidden twin's uuid and the restore endpoint. - NIT: drop the duplicated "uuid" entry in list_columns.
Review-panel finding (verified): UploadCommand located its target dataset with a plain visibility-filtered query, so with SOFT_DELETE on, a soft-deleted dataset over the same (database, schema, table_name) was invisible and the upload created an active twin — permanently blocking the hidden row's restore — or died on the legacy unique constraint. Either way the file's contents had already been written to the analytics database, outside the metadata transaction. The natural trigger is the most common flow for uploaded datasets: soft-delete, then re-upload the same file. Guard with find_soft_deleted_logical_duplicate (the same helper the create/get_or_create/duplicate/import paths use) BEFORE reader.read, so a blocked upload leaves the analytics database untouched, and raise a typed 422 naming the twin's uuid and the restore endpoint (executable recoveries only). The DatasetDAO import is deferred inside run() — the module-top form is a circular import via views.base (the same apache#40573 constraint documented in daos/dataset.py). Tests pin both the block (raises before reader.read — fails without the guard) and the no-twin control path.
…dard Review-panel finding (6 of 8 lenses): the datasets UPDATING.md section and the delete/bulk_delete/import OpenAPI docstrings predated the flag-accuracy standard established on apache#40128/apache#40129 and described soft delete as unconditional — false on default deployments, where SOFT_DELETE is off and DELETE hard-deletes permanently. - UPDATING.md now leads with the flag gate + default, documents the flag-toggle caveat, and adds an explicit list of the flag-INDEPENDENT parts (restore endpoint, deleted-state filter, DB-deletion guard, get_or_create pre-check, the uniqueness changes) versus the flag-gated soft DELETE itself. - Added the migration-downgrade consequence (dropping deleted_at silently activates un-restored trash; reconcile first, flag off first) and a SQL Lab / dataset-creation note describing which flows return the targeted hidden-twin 422 (get_or_create, upload) versus the generic "already exists" (plain create/update/duplicate), plus the perm-string maintenance guarantee on database rename. - Importer paragraph and import_ docstring state the deliberate restore-with-update-without-overwrite asymmetry, matching the siblings. - delete/bulk_delete docstrings + summaries flag-qualified.
…rced messages, shared helpers Six coordinated fixes from the 8-lens review panel: 1. Importer overwrite path now re-validates the incoming identity via validate_update_uniqueness (mirroring UpdateDatasetCommand): an uploaded config can no longer rename an alive dataset onto a soft-deleted twin's physical identity — which import_from_dict cannot see — silently squatting it and permanently blocking the trash row's restore. 2. Restore-via-import probes the POST-update identity from the config (has_active_logical_duplicate gained an optional identity override) instead of the stale stored identity, so a rename onto an active dataset is refused up front (row stays soft-deleted) rather than falling through to MultipleResultsFound; that fallback message also no longer asserts a NULL-schema cause it hasn't established. 3. The hidden-twin blocking policy is single-sourced: a new typed DatasetSoftDeletedTwinExistsError (422, message defined once) raised from CreateDatasetCommand.validate. The plain POST /api/v1/dataset/ now gets the targeted uuid+restore message instead of the opaque "already exists", and get_or_create_dataset's view-level pre-check is replaced by the command path (Table import dropped). 4. All importer restore/overwrite errors carry the entity identity (name + uuid), adopting the charts identity-bearing form as canonical. 5. Messages name only executable recoveries: dropped "hard-delete it" (no such API surface) and reworded the database-delete blocker's "permanently purge them first". 6. validate_update_uniqueness collapsed to a one-line delegate of validate_uniqueness (rationale lives once); DatasetDeletedStateFilter reuses BaseDeletedStateFilter._normalize instead of re-inlining the expression the helper exists to prevent drifting from. New tests pin the create-command twin block (typed 422, executable recoveries only), the overwrite rename block, and the incoming-identity restore probe (both importer tests fail without the fixes); two existing message asserts updated. 905 tests green across the affected suites.
…ate audience scoping Three approved cross-entity review decisions: 1. Restore-audience ownership leg. An owner who has lost (or never had) a datasource grant could neither enumerate nor restore their own soft-deleted dataset — a 404 on restore and an empty trash list — while the docs said "owner or admin" and raise_for_access counts ownership AS datasource access. Two changes close the inconsistency: BaseRestoreCommand's lookup adds skip_base_filter=True (the restore audience is enforced by raise_for_ownership — owners or admin; non-owners holding a valid uuid now get 403 rather than 404), and DatasourceFilter gains an owned-soft-deleted-rows leg (inert for live rows, and only ever reachable when a deleted-state rison filter has opted the request into seeing soft-deleted rows). The dashboards access filter already had an ownership criterion; charts parity (ChartFilter) follows once apache#40129 is on master. 2. Deleted-state owner-scoping hoisted into BaseDeletedStateFilter. The 30-line restore-audience body was a verbatim copy in the merged dashboards filter and the datasets filter, with charts carrying a third. It lives once on the base (_scope_to_restore_audience); both subclasses shrink to pure declarations. Entities without an owners relationship opt out automatically, which also keeps the synthetic-model unit tests meaningful. 3. The combined-datasource listing's always-on deleted_at filter (daos/datasource.py) is documented as deliberately flag-independent (code comment + UPDATING.md's flag-independent list) rather than gated: always-hiding is the safer failure mode for that aggregate endpoint. Tests: the base-restore contract test now pins BOTH bypasses (it previously pinned base_filter NOT being skipped — the deliberate reversal); the dataset restore happy-path pins the same call shape; a new integration test proves a gamma owner with no datasource grant can enumerate their own trash via include/only while NOT seeing the row when it is live (the leg must not leak live rows).
…wire Review-panel test tier: - Three tests had success-path-only cleanup of soft-deleted state; a failed assertion would strand a soft-deleted row (one of them the SHARED example dataset) and cascade failures through later suites. All three now clean up in finally, matching the module's own _restore_dataset contract. - New flag-OFF contract test: with SOFT_DELETE disabled (the default), DELETE physically removes the row (a bypass query finds nothing) and restore 404s. Every other delete-path test runs flag-ON, so the path every default deployment runs was the untested one. - Bypass tripwire for has_active_logical_duplicate: the helper's active-rows-only semantics forbid skip_visibility_filter (adding it would silently refuse legitimate restores of legacy twin pairs), but the suite stayed green under that mutation. The new test spies the bypass and fails if it is ever entered — validated by applying the mutation (test fails) and reverting (suite green). Also de-staled the PR body: the merge-ordering warning claimed soft-delete ships unconditional (the SOFT_DELETE gate is in the merge base), and QA step 11 asserted flag-off restore inertness, contradicting the deliberate cross-entity rollback contract.
…logs Proactive port of @rusackas's pre-merge finding on the charts sibling (apache#40129): this branch's new _()-wrapped messages were missing from messages.pot and every messages.po. Added to all 30 catalogs with empty msgstrs at alphabetical positions: - the soft-deleted-twin 422 (create path) and its upload-path variant, both flagged python-format for the %(uuid)s parameter - the restore blocked-by-active-duplicate message - the database-delete blocked-by-soft-deleted-datasets message Every catalog validated to parse, carry byte-exact msgids matching the code strings, and carry the python-format flags.
… head Master's migration head moved to b4a3f2e1d0c9 (apache#40527); chaining off it restores the single-head invariant. Note: if the charts sibling (apache#40129) merges first, this needs one final re-point onto 7c4a8d09ca37. No schema change.
Charts soft-delete (add_deleted_at_to_slices, 7c4a8d09ca37) merged and is now the master head. add_deleted_at_to_tables still pointed at the prior head (b4a3f2e1d0c9), which is now a mid-chain ancestor, so it forked a second alembic head. Re-point down_revision (and the Revises header) to 7c4a8d09ca37 so the branch presents a single head. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address substantive automated-review findings on the soft-delete PR: - MCP create_dataset / create_virtual_dataset caught only DatasetInvalidError, but validate() raises DatasetSoftDeletedTwinExistsError directly (a top-level CommandException), so the twin case fell through to the generic "unexpected error" branch instead of the actionable restore-or-rename message the REST API returns. Catch it explicitly in both tools. - Tighten the database-delete test to assert the specific DatabaseDeleteSoftDeletedDatasetsExistFailedError, and add a live-dataset test pinning that the live branch raises the base error (not the subclass). - Document that the active-duplicate DAO read is a best-effort friendly-error check; the DB UniqueConstraint (database_id, catalog, schema, table_name) is the real guard against a concurrent duplicate create. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test_restore_soft_deleted_dataset's finally called self._restore_dataset, but _restore_dataset is a method only on TestDatasetSoftDelete — on TestDatasetRestore it raised AttributeError, failing the test (and leaving the shared example dataset soft-deleted) across all DB suites. Call the module-level _restore_dataset helper directly, matching the sibling test in the same class. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks all for the earlier reviews — this PR's head was refreshed and is green and ready for a final look:
No behavior/scope change — same soft-delete + restore, gated behind This comment was generated by Claude (AI) on behalf of @mikebridge. |
rusackas
approved these changes
Jul 6, 2026
rusackas
left a comment
rusackas
left a comment
Member
There was a problem hiding this comment.
LGTM, approving. Pulled the branch to double-check my earlier asks rather than taking the thread's word for it... the duplicate command now carries catalog, the migration chain is single-headed on current master (no new migrations landed since the rebase), and the PR's 116 unit tests pass locally.
On the one-way door: with the distinct DatabaseDeleteSoftDeletedDatasetsExistFailedError message and the UPDATING.md note, I'm comfortable shipping it ahead of the purge work. And since the whole thing ships dark behind SOFT_DELETE and #40129 already set the precedent on master, I don't think we need hold:next-major after all.
Thanks for the marathon here @mikebridge.
This was referenced Jul 8, 2026
9 tasks
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
6 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 datasets (
SqlaTable/ thetablestable) into the soft-delete infrastructure from #39977 — one of three per-entity rollouts (charts / dashboards / datasets) decomposing SIP-208.DELETE /api/v1/dataset/<id>and the bulk-delete endpoint stop hard-deleting: the row is marked with adeleted_attimestamp and hidden from all list, detail, lookup, and relationship-load queries (e.g.database.tables). A newPOST /api/v1/dataset/<uuid>/restoreendpoint clearsdeleted_at. The v1 YAML importer treats re-import of a soft-deleted UUID as an implicit restore-and-update, and a newdataset_deleted_staterison filter lets the list endpoint surface soft-deleted rows.Referential behavior (datasets are referenced by charts/dashboards):
tablesrows still reference the database. Because v1 ships no hard-delete/purge, a database that has ever had datasets cannot be deleted through the API once those datasets are soft-deleted, until purge work lands. Deliberate trade-off (no orphaned rows / datasets stay restorable).(database_id, catalog, schema, table_name). Restore, create, update, and re-import all refuse to produce two active datasets for one physical table, returning a clean 422 /ImportFailedErrorinstead of an opaque DBIntegrityError(500). On create/update this is enforced by bypassing the visibility filter so a soft-deleted twin still blocks; on restore/import byDatasetDAO.has_active_logical_duplicate/find_soft_deleted_logical_duplicate.API CHANGES
New endpoint — restore a soft-deleted dataset
POST /api/v1/dataset/<uuid>/restoreuuid(string, formatuuid) — the dataset UUIDcan_write on Dataset(endpoint) and ownership of the row, or admin (object-levelraise_for_ownership)200{ "message": "OK" }—deleted_atcleared, dataset active again401403DatasetForbiddenError)404DatasetNotFoundError)422DatasetLogicalDuplicateError), or the restore UPDATE failed (DatasetRestoreFailedError)Permission model mirrors
DELETE/bulk_delete: endpoint-levelcan_write(viamethod_permission_name["restore"] = "write") plus object-level ownership. Existingcan_write on Datasetgrants cover restore automatically — no role migration required.Changed — DELETE / bulk delete become soft.
DELETE /api/v1/dataset/<pk>andDELETE /api/v1/dataset/(bulk) setdeleted_atinstead of removing the row. Same response codes as before. The row is hidden from the default API but reappears if restored, and direct DB queries ontablesstill see it. FABab_view_menu/ permission-view rows tied to the dataset are preserved.New —
dataset_deleted_staterison filter (GET list). A search filter onGET /api/v1/dataset/:include— live rows + soft-deleted rowsonly— only soft-deleted rowsWhen opted in, each result row is augmented with a
deleted_atfield (nullfor live, ISO-8601 for soft-deleted). Access scoping: admins see all soft-deleted rows; non-admins see only soft-deleted datasets they own — the same audience that can restore them.Combined
GET /api/v1/datasource/no longer counts soft-deleted datasets in pagination totals (explicitdeleted_at IS NULLpredicate on the Coreselect).Importer (
import_dataset). Re-importing a UUID that matches a soft-deleted dataset is an implicit restore-and-update for an owner/admin withcan_write(clearsdeleted_at, UPDATEs in place, preserving PK / chart back-reference / columns / metrics). Non-owners and callers withoutcan_writegetImportFailedError. Importing a fresh-UUID dataset whose physical table is claimed by a soft-deleted row also raisesImportFailedError, directing the user to restore instead.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes — REST endpoints only. A frontend "Restore" affordance is a follow-up.
DELETE /api/v1/dataset/<pk>deleted_at; row hidden, restorableGET /api/v1/dataset/?...&dataset_deleted_state=onlydeleted_atpopulatedGET /api/v1/dataset/?...&dataset_deleted_state=includedeleted_atpopulated on deleted rowsPOST /api/v1/dataset/<uuid>/restoredeleted_at; 422 if a live dataset already claims the same physical tablePOST /api/v1/dataset/colliding with a soft-deleted rowImportFailedErrordirecting to restoreDELETEof a database that has soft-deleted datasetsTESTING INSTRUCTIONS
Automated
QA / manual — enable the
SOFT_DELETEfeature flag first (from #41166); on a build that has both this PR and #41166:uuid.DELETE /api/v1/dataset/<id>→ 200.GET /api/v1/dataset/no longer lists it;GET /api/v1/dataset/<id>→ 404.GET /api/v1/dataset/?q=(filters:!((col:id,opr:dataset_deleted_state,value:only)))→ the deleted dataset withdeleted_atpopulated. Tryinclude→ live + deleted. As a non-admin non-owner, confirm the soft-deleted row is NOT listed even withvalue:only.POST /api/v1/dataset/<uuid>/restore→200 {"message":"OK"}. Confirm the dataset reappears and the chart from step 2 loads again.(database, catalog, schema, table), then restore A → 422DatasetLogicalDuplicateError. (With default DB constraints the create step is rejected first;restore_test.py::test_restore_dataset_logical_duplicate_raisescovers the command path with mocks.)POST /api/v1/dataset/with the same physical identity → 422 (not a 500).can_write→ImportFailedError.ImportFailedErrordirecting you to restore.DatabaseDeleteDatasetsExistFailedError).SOFT_DELETEand confirmDELETEhard-deletes as before. The restore endpoint and thedataset_deleted_statefilter remain functional by design (so rows soft-deleted during a flag-on window stay discoverable and restorable after a flag rollback): confirm a previously soft-deleted dataset can still be restored with the flag off. Covered bytest_delete_dataset_flag_off_hard_deletesfor the hard-delete + restore-404 contract.ADDITIONAL INFORMATION
SOFT_DELETE(defined in feat(soft-delete): gate soft delete behind a temporary SOFT_DELETE release toggle #41166 — merged; gating verified on this branch)deleted_atcolumn +ix_tables_deleted_atindex totables; downgrade drops bothtables. Column add is instant; the index build runs inline (noCONCURRENTLY) and may briefly block writes ontableson very large Postgres deployments (reads unaffected; MySQL InnoDB builds online). Run during a maintenance window on large installs.POST /api/v1/dataset/<uuid>/restore, thedataset_deleted_staterison filter, and thedeleted_atfield on list responses when deleted-state visibility is opted intoBehavior changes worth flagging
DELETE/bulk_deleteare now soft (whenSOFT_DELETEis on). Hidden from the default API after delete; restorable; visible via direct DB queries.ImportFailedErrorinstead of opaque 500s.GET /api/v1/datasource/pagination excludes soft-deleted datasets.dataset_deleted_state— the restore audience.Depends on
SOFT_DELETErelease toggle) — merged ✅ (2026-07-01); this branch is rebased onto it