codeant-ai-for-open-source[bot] commented on code in PR #40129:
URL: https://github.com/apache/superset/pull/40129#discussion_r3492944643
##########
superset/charts/api.py:
##########
@@ -134,7 +142,17 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
"warm_up_cache",
}
class_permission_name = "Chart"
- method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
+ # Custom methods (``restore``) need an explicit entry; FAB's @protect()
+ # decorator falls back to ``can_<method>_<class>`` (i.e.
+ # ``can_restore_Chart``) when the mapping is missing, which standard
+ # roles don't carry. Mirrors the permission model documented for
+ # ``DELETE`` / ``bulk_delete``: endpoint-level ``can_write`` plus
+ # resource-level ``raise_for_ownership``. See themes/api.py for the
+ # established pattern.
+ method_permission_name = {
+ **MODEL_API_RW_METHOD_PERMISSION_MAP,
+ "restore": "write",
+ }
Review Comment:
**Suggestion:** Add an explicit type annotation to this new class-level
mapping so the newly introduced variable is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new class-level mapping is introduced without any type annotation, and
it is a variable that can be annotated. This matches the Python type-hint rule
for newly modified code.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ad66017cbfd243a08d75b25cc16a36c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ad66017cbfd243a08d75b25cc16a36c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/api.py
**Line:** 152:155
**Comment:**
*Custom Rule: Add an explicit type annotation to this new class-level
mapping so the newly introduced variable is fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=252d1c2cda404aca07ae1e56485f2b12b791c80d93e7a88523d56779fa19e40d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=252d1c2cda404aca07ae1e56485f2b12b791c80d93e7a88523d56779fa19e40d&reaction=dislike'>👎</a>
##########
superset/charts/filters.py:
##########
@@ -196,3 +197,40 @@ def apply(self, query: Query, value: Any) -> Query:
FavStar.user_id == get_user_id(),
)
)
+
+
+class ChartDeletedStateFilter( # pylint: disable=too-few-public-methods
+ BaseDeletedStateFilter
+):
+ """Rison filter for the GET list that exposes soft-deleted charts.
+
+ Soft-deleted rows are additionally scoped to the **restore audience**: only
+ the chart's owners (or admins) may enumerate them. This mirrors
+ ``RestoreChartCommand``'s ``raise_for_ownership`` check, so a read-access
+ non-owner (who can see the chart via datasource access) cannot list
+ soft-deleted charts they could never restore. Live rows are unaffected —
+ they keep their normal ``ChartFilter`` visibility. The ownership scoping is
+ part of the cross-entity deleted-state contract: only the restore audience
+ may enumerate soft-deleted rows (kept consistent with the deleted-state
+ filters in the dashboard and dataset soft-delete rollouts).
+ """
+
+ arg_name = "chart_deleted_state"
+ model = Slice
+
+ def apply(self, query: Query, value: Any) -> Query:
+ query = super().apply(query, value)
+ normalized = str(value).lower().strip() if value is not None else ""
+ if normalized not in {"include", "only"} or
security_manager.is_admin():
+ return query
+
+ # Non-admins may only see soft-deleted charts they own. ``any()`` emits
+ # an EXISTS subquery so it composes with the base access filter without
+ # producing duplicate rows from a join.
+ owned = Slice.owners.any(security_manager.user_model.id ==
get_user_id())
Review Comment:
**Suggestion:** Add a precise type annotation for this SQLAlchemy expression
variable so the new logic is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python local variable holding a SQLAlchemy boolean
expression and it lacks a type annotation. The custom rule explicitly requires
type hints for relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=83c8b615d2dc44d6b06f4f139d6b41c5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=83c8b615d2dc44d6b06f4f139d6b41c5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/filters.py
**Line:** 230:230
**Comment:**
*Custom Rule: Add a precise type annotation for this SQLAlchemy
expression variable so the new logic is fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=b005b86d5d01f4fecc4457b4d25715bab6ab2810ca103cfcfb3a7cde787f3790&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=b005b86d5d01f4fecc4457b4d25715bab6ab2810ca103cfcfb3a7cde787f3790&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py:
##########
@@ -0,0 +1,52 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Add deleted_at column and index to slices for soft-delete.
+
+Adds a nullable ``deleted_at`` column and an index on it to the
+``slices`` table to support soft deletion of charts. Companion to
+the ``SoftDeleteMixin`` infrastructure shipped in PR #39977.
+
+Revision ID: 7c4a8d09ca37
+Revises: 78a40c08b4be
+Create Date: 2026-05-08 12:00:00.000000
+"""
+
+from sqlalchemy import Column, DateTime
+
+from superset.migrations.shared.utils import (
+ add_columns,
+ create_index,
+ drop_columns,
+ drop_index,
+)
+
+# revision identifiers, used by Alembic.
+revision = "7c4a8d09ca37"
+down_revision = "78a40c08b4be"
+
+TABLE_NAME = "slices"
+INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at"
Review Comment:
**Suggestion:** Add explicit type annotations to the new module-level
variables so the migration satisfies the type-hint requirement for relevant
annotatable variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The custom rule requires type hints for new or modified Python variables
that can be annotated. These module-level constants are newly introduced and
have no type annotations, so the suggestion correctly identifies a real
violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f6f898808160453784508de4df20490c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f6f898808160453784508de4df20490c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py
**Line:** 38:42
**Comment:**
*Custom Rule: Add explicit type annotations to the new module-level
variables so the migration satisfies the type-hint requirement for relevant
annotatable variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=59e755ac64d014e5d2da7c2b83e0503e55bc4c6e4d8d68d9e1ea5ba09e6d13ca&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=59e755ac64d014e5d2da7c2b83e0503e55bc4c6e4d8d68d9e1ea5ba09e6d13ca&reaction=dislike'>👎</a>
##########
superset/charts/filters.py:
##########
@@ -196,3 +197,40 @@ def apply(self, query: Query, value: Any) -> Query:
FavStar.user_id == get_user_id(),
)
)
+
+
+class ChartDeletedStateFilter( # pylint: disable=too-few-public-methods
+ BaseDeletedStateFilter
+):
+ """Rison filter for the GET list that exposes soft-deleted charts.
+
+ Soft-deleted rows are additionally scoped to the **restore audience**: only
+ the chart's owners (or admins) may enumerate them. This mirrors
+ ``RestoreChartCommand``'s ``raise_for_ownership`` check, so a read-access
+ non-owner (who can see the chart via datasource access) cannot list
+ soft-deleted charts they could never restore. Live rows are unaffected —
+ they keep their normal ``ChartFilter`` visibility. The ownership scoping is
+ part of the cross-entity deleted-state contract: only the restore audience
+ may enumerate soft-deleted rows (kept consistent with the deleted-state
+ filters in the dashboard and dataset soft-delete rollouts).
+ """
+
+ arg_name = "chart_deleted_state"
+ model = Slice
+
+ def apply(self, query: Query, value: Any) -> Query:
+ query = super().apply(query, value)
+ normalized = str(value).lower().strip() if value is not None else ""
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced
local variable to satisfy the type-hint requirement for relevant variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced Python local variable with an obvious type of
str, and it is unannotated. That matches the rule requiring type hints on
relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ab8cdeb7dddb40b0ac9d3d750b6b508c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ab8cdeb7dddb40b0ac9d3d750b6b508c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/filters.py
**Line:** 223:223
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced
local variable to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=75a6d6cf012c5a2acfa5e2dec623d46c6edd5e63075be9a919255361ae1bf25b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=75a6d6cf012c5a2acfa5e2dec623d46c6edd5e63075be9a919255361ae1bf25b&reaction=dislike'>👎</a>
##########
tests/integration_tests/charts/soft_delete_tests.py:
##########
@@ -0,0 +1,560 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Integration tests for chart soft-delete and restore."""
+
+from datetime import datetime
+
+from superset import security_manager
+from superset.connectors.sqla.models import SqlaTable
+from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
+from superset.extensions import db
+from superset.models.dashboard import Dashboard, dashboard_slices
+from superset.models.slice import Slice
+from superset.reports.models import (
+ ReportCreationMethod,
+ ReportSchedule,
+ ReportScheduleType,
+)
+from superset.utils import json
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.constants import (
+ ADMIN_USERNAME,
+ ALPHA_USERNAME,
+ GAMMA_USERNAME,
+)
+from tests.integration_tests.insert_chart_mixin import InsertChartMixin
+
+
+def _hard_delete_chart(chart_id: int) -> None:
+ """Hard-delete a chart row regardless of soft-delete state."""
+ row = (
+ db.session.query(Slice)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}})
+ .filter(Slice.id == chart_id)
+ .one_or_none()
+ )
+ if row:
+ db.session.delete(row)
+ db.session.commit()
+
+
+def _hard_delete_dashboard_for_charts_test(dashboard_id: int) -> None:
+ """Hard-delete a dashboard row regardless of soft-delete state."""
+ row = (
+ db.session.query(Dashboard)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}})
+ .filter(Dashboard.id == dashboard_id)
+ .one_or_none()
+ )
Review Comment:
**Suggestion:** Add a concrete type annotation to this dashboard query
result variable so it satisfies the rule requiring type hints where feasible.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another annotatable local ORM result. Since `one_or_none()` returns
an optional dashboard instance, the code satisfies the pattern targeted by the
type-hint rule but does not include a type annotation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=397db10edf2f4b709e464ed8fef233ac&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=397db10edf2f4b709e464ed8fef233ac&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/charts/soft_delete_tests.py
**Line:** 57:62
**Comment:**
*Custom Rule: Add a concrete type annotation to this dashboard query
result variable so it satisfies the rule requiring type hints where feasible.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=be2478e8fe6d06ea07ac37f733845454ef716e45b55bf530715f6c888064a37a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=be2478e8fe6d06ea07ac37f733845454ef716e45b55bf530715f6c888064a37a&reaction=dislike'>👎</a>
##########
tests/integration_tests/charts/soft_delete_tests.py:
##########
@@ -0,0 +1,560 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Integration tests for chart soft-delete and restore."""
+
+from datetime import datetime
+
+from superset import security_manager
+from superset.connectors.sqla.models import SqlaTable
+from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
+from superset.extensions import db
+from superset.models.dashboard import Dashboard, dashboard_slices
+from superset.models.slice import Slice
+from superset.reports.models import (
+ ReportCreationMethod,
+ ReportSchedule,
+ ReportScheduleType,
+)
+from superset.utils import json
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.constants import (
+ ADMIN_USERNAME,
+ ALPHA_USERNAME,
+ GAMMA_USERNAME,
+)
+from tests.integration_tests.insert_chart_mixin import InsertChartMixin
+
+
+def _hard_delete_chart(chart_id: int) -> None:
+ """Hard-delete a chart row regardless of soft-delete state."""
+ row = (
+ db.session.query(Slice)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}})
+ .filter(Slice.id == chart_id)
+ .one_or_none()
+ )
+ if row:
+ db.session.delete(row)
+ db.session.commit()
+
+
+def _hard_delete_dashboard_for_charts_test(dashboard_id: int) -> None:
+ """Hard-delete a dashboard row regardless of soft-delete state."""
+ row = (
+ db.session.query(Dashboard)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}})
+ .filter(Dashboard.id == dashboard_id)
+ .one_or_none()
+ )
+ if row:
+ db.session.delete(row)
+ db.session.commit()
+
+
+class TestChartSoftDelete(InsertChartMixin, SupersetTestCase):
+ """Tests for chart soft-delete behaviour (T013, T016)."""
+
+ def test_delete_chart_soft_deletes(self) -> None:
+ """DELETE /api/v1/chart/<pk> sets deleted_at instead of removing."""
+ admin_id = self.get_user("admin").id
+ chart = self.insert_chart("soft_delete_test", [admin_id], 1)
+ chart_id = chart.id
+ self.login(ADMIN_USERNAME)
+
+ rv = self.client.delete(f"/api/v1/chart/{chart_id}")
+ assert rv.status_code == 200
+
+ # Row still exists in DB with deleted_at set
+ row = (
+ db.session.query(Slice)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}})
+ .filter(Slice.id == chart_id)
+ .one_or_none()
+ )
+ assert row is not None
+ assert row.deleted_at is not None
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_soft_deleted_chart_excluded_from_get(self) -> None:
+ """GET /api/v1/chart/<pk> returns 404 for a soft-deleted chart."""
+ admin_id = self.get_user("admin").id
+ chart = self.insert_chart("invisible_chart", [admin_id], 1)
+ chart_id = chart.id
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/chart/{chart_id}")
+ rv = self.client.get(f"/api/v1/chart/{chart_id}")
+ assert rv.status_code == 404
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_soft_deleted_chart_excluded_from_list(self) -> None:
+ """GET /api/v1/chart/ should not include soft-deleted charts."""
+ admin_id = self.get_user("admin").id
+ chart = self.insert_chart("listed_then_deleted", [admin_id], 1)
+ chart_id = chart.id
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/chart/{chart_id}")
+ rv = self.client.get("/api/v1/chart/")
+ data = json.loads(rv.data)
+ chart_ids = [c["id"] for c in data["result"]]
+ assert chart_id not in chart_ids
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_soft_deleted_chart_included_in_list_when_requested(self) -> None:
+ """GET /api/v1/chart/ with chart_deleted_state=include returns deleted
charts.""" # noqa: E501
+ admin_id = self.get_user("admin").id
+ chart = self.insert_chart("listed_with_deleted", [admin_id], 1)
+ chart_id = chart.id
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/chart/{chart_id}")
+
+ rison_query =
"(filters:!((col:id,opr:chart_deleted_state,value:include)))"
+ rv = self.client.get(f"/api/v1/chart/?q={rison_query}")
+ assert rv.status_code == 200
+
+ data = json.loads(rv.data)
+ deleted_row = next(
+ (row for row in data["result"] if row["id"] == chart_id),
+ None,
+ )
+ assert deleted_row is not None
+ assert deleted_row["deleted_at"] is not None
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_only_filter_returns_only_soft_deleted_charts(self) -> None:
+ """chart_deleted_state=only excludes live rows and returns only
deleted ones."""
+ admin_id = self.get_user("admin").id
+ live_chart = self.insert_chart("only_live", [admin_id], 1)
+ deleted_chart = self.insert_chart("only_deleted", [admin_id], 1)
+ live_id = live_chart.id
+ deleted_id = deleted_chart.id
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/chart/{deleted_id}")
+
+ rison_query =
"(filters:!((col:id,opr:chart_deleted_state,value:only)))"
+ rv = self.client.get(f"/api/v1/chart/?q={rison_query}")
+ assert rv.status_code == 200
+
+ data = json.loads(rv.data)
+ returned_ids = {row["id"] for row in data["result"]}
+ assert deleted_id in returned_ids
+ assert live_id not in returned_ids
+
+ # Cleanup
+ _hard_delete_chart(live_id)
+ _hard_delete_chart(deleted_id)
+
+ def test_deleted_state_list_shows_owner_their_own_deleted(self) -> None:
+ """A non-admin owner can still enumerate their own soft-deleted charts.
+ Deleted-state scoping mirrors the restore audience, so it must not lock
+ owners out of their own trash."""
+ alpha_id = self.get_user(ALPHA_USERNAME).id
+ chart = self.insert_chart("sd_owner_chart", [alpha_id], 1)
+ chart_id = chart.id
+
+ chart.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.commit()
+
+ self.login(ALPHA_USERNAME)
+ rison_query = (
+
"(filters:!((col:id,opr:chart_deleted_state,value:only)),page_size:200)"
+ )
+ rv = self.client.get(f"/api/v1/chart/?q={rison_query}")
+ assert rv.status_code == 200
+ ids = [c["id"] for c in json.loads(rv.data)["result"]]
+ assert chart_id in ids
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_deleted_state_list_hides_non_owned_from_read_access_user(self) ->
None:
+ """A read-access non-owner must not enumerate a chart once it is
+ soft-deleted.
+
+ Gamma is granted ``datasource_access`` to the chart's dataset, so
+ ``ChartFilter`` makes the chart visible to gamma while it is live.
After
+ soft-delete, the deleted-state list is scoped to the restore audience
+ (owners/admins), so gamma — who could never restore it — must not see
it
+ via ``include`` or ``only``.
+ """
+ admin_id = self.get_user(ADMIN_USERNAME).id
+ chart = self.insert_chart("sd_acl_chart", [admin_id], 1)
+ chart_id = chart.id
+
+ table = db.session.query(SqlaTable).get(1)
+ gamma_role = security_manager.find_role("Gamma")
+ pvm = security_manager.add_permission_view_menu("datasource_access",
table.perm)
+ gamma_role.permissions.append(pvm)
+ db.session.commit()
+
+ try:
+ # Precondition: gamma can see the chart while it is live.
+ self.login(GAMMA_USERNAME)
+ rv = self.client.get("/api/v1/chart/?q=(page_size:200)")
+ assert chart_id in [c["id"] for c in
json.loads(rv.data)["result"]], (
+ "precondition: gamma should see the live chart via datasource
access"
+ )
+
+ # Soft-delete directly (avoids a mid-test re-login to admin).
+ reloaded = (
+ db.session.query(Slice)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}})
+ .filter(Slice.id == chart_id)
+ .one()
+ )
+ reloaded.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.commit()
+
+ # Gamma must not see the soft-deleted chart in either mode.
+ for value in ("include", "only"):
+ rison_query = (
+
f"(filters:!((col:id,opr:chart_deleted_state,value:{value})),"
+ "page_size:200)"
+ )
+ rv = self.client.get(f"/api/v1/chart/?q={rison_query}")
+ assert rv.status_code == 200
+ ids = [c["id"] for c in json.loads(rv.data)["result"]]
+ assert chart_id not in ids, (
+ "read-access non-owner must not enumerate a soft-deleted "
+ f"chart via chart_deleted_state={value}"
+ )
+ finally:
+ pvm = security_manager.find_permission_view_menu(
+ "datasource_access", table.perm
+ )
+ if pvm:
+ security_manager.del_permission_role(gamma_role, pvm)
+ db.session.commit()
+ _hard_delete_chart(chart_id)
+
+ def test_delete_already_soft_deleted_chart_returns_404(self) -> None:
+ """DELETE on an already soft-deleted chart returns 404 (FR-008)."""
+ admin_id = self.get_user("admin").id
+ chart = self.insert_chart("double_delete_test", [admin_id], 1)
+ chart_id = chart.id
+ self.login(ADMIN_USERNAME)
+
+ rv = self.client.delete(f"/api/v1/chart/{chart_id}")
+ assert rv.status_code == 200
+ rv = self.client.delete(f"/api/v1/chart/{chart_id}")
+ assert rv.status_code == 404
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_delete_chart_blocked_when_active_report_references_it(self) ->
None:
+ """DELETE /api/v1/chart/<id> returns 422 when a report references it.
+
+ Pins down the existing API protection in
`DeleteChartCommand.validate()`:
+ when a `report_schedule` row references the chart, the validation
+ raises `ChartDeleteFailedReportsExistError` *before*
`ChartDAO.delete()`
+ is invoked, so no soft-delete routing happens. This is the contract
+ soft-delete inherits from the pre-existing API and is what makes the
+ "report-execution against soft-deleted target" crash class
+ (commands/report/execute.py:_get_url) unreachable through the API.
+ """
+ admin_id = self.get_user("admin").id
+ chart = self.insert_chart("blocked_by_report_test", [admin_id], 1)
+ chart_id = chart.id
+
+ report = ReportSchedule(
+ type=ReportScheduleType.REPORT,
+ name="blocking_report_for_chart_delete",
+ description="Report that should block chart deletion",
+ crontab="0 9 * * *",
+ chart=chart,
+ creation_method=ReportCreationMethod.ALERTS_REPORTS,
+ )
+ db.session.add(report)
+ db.session.commit()
+ report_id = report.id
+
+ self.login(ADMIN_USERNAME)
+
+ rv = self.client.delete(f"/api/v1/chart/{chart_id}")
+ assert rv.status_code == 422
+ body = json.loads(rv.data)
+ assert "associated alerts or reports" in body.get("message",
"").lower() or (
+ "associated" in body.get("message", "").lower()
+ and "report" in body.get("message", "").lower()
+ )
+ assert "blocking_report_for_chart_delete" in body.get("message", "")
+
+ # Confirm the chart was NOT soft-deleted (deleted_at remains NULL).
+ row = (
+ db.session.query(Slice)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}})
+ .filter(Slice.id == chart_id)
+ .one()
+ )
+ assert row.deleted_at is None
+
+ # Cleanup
+ db.session.delete(
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.id == report_id)
+ .one()
+ )
+ db.session.commit()
+ _hard_delete_chart(chart_id)
+
+
+class TestChartRestore(InsertChartMixin, SupersetTestCase):
+ """Tests for chart restore behaviour (T025)."""
+
+ def test_restore_soft_deleted_chart(self) -> None:
+ """POST /api/v1/chart/<uuid>/restore makes the chart visible again."""
+ admin_id = self.get_user("admin").id
+ chart = self.insert_chart("restore_test", [admin_id], 1)
+ chart_id = chart.id
+ chart_uuid = str(chart.uuid)
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/chart/{chart_id}")
+ rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore")
+ assert rv.status_code == 200
+
+ rv = self.client.get(f"/api/v1/chart/{chart_id}")
+ assert rv.status_code == 200
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_restore_failure_returns_422(self) -> None:
+ """A failure during restore surfaces as a clean 422 via the
+ ``ChartRestoreFailedError`` handler rather than an unhandled 500.
+
+ ``RestoreChartCommand.run`` wraps the restore in ``@transaction``
+ and rethrows ``ChartRestoreFailedError`` on any underlying
+ SQLAlchemy error; this pins that the endpoint maps it to 422.
+ """
+ from unittest.mock import patch
+
+ from superset.commands.chart.exceptions import (
+ ChartRestoreFailedError,
+ )
+
+ admin_id = self.get_user("admin").id
+ chart = self.insert_chart("restore_fail_test", [admin_id], 1)
+ chart_id = chart.id
+ chart_uuid = str(chart.uuid)
+ self.login(ADMIN_USERNAME)
+ self.client.delete(f"/api/v1/chart/{chart_id}")
+
+ with patch(
+ "superset.commands.chart.restore.RestoreChartCommand.run",
+ side_effect=ChartRestoreFailedError(),
+ ):
+ rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore")
+ assert rv.status_code == 422
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_restore_nonexistent_chart_returns_404(self) -> None:
+ """POST /api/v1/chart/<uuid>/restore returns 404 for unknown UUID."""
+ self.login(ADMIN_USERNAME)
+ rv = self.client.post(
+ "/api/v1/chart/00000000-0000-0000-0000-000000000000/restore"
+ )
+ assert rv.status_code == 404
+
+ def test_restore_active_chart_returns_404(self) -> None:
+ """POST /api/v1/chart/<uuid>/restore on active chart returns 404."""
+ admin_id = self.get_user("admin").id
+ chart = self.insert_chart("active_restore_test", [admin_id], 1)
+ chart_id = chart.id
+ chart_uuid = str(chart.uuid)
+ self.login(ADMIN_USERNAME)
+
+ rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore")
+ assert rv.status_code == 404
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_restore_uses_can_write_permission(self) -> None:
+ """Non-admin owner with ``can_write_Chart`` can hit the restore
+ endpoint.
+
+ Pins the permission contract: ``method_permission_name`` must map
+ ``restore`` to ``write`` so FAB's ``@protect`` resolves the gate to
+ ``can_write_Chart`` (which Alpha already carries), not the implicit
+ fallback ``can_restore_Chart`` (which no standard role carries).
+
+ Without the mapping FAB defaults to ``can_<method>_<class>`` and
+ every non-admin would get 403 here — admins bypass FAB permission
+ checks entirely, so the admin-authed restore tests above don't
+ exercise the mapping.
+ """
+ alpha = self.get_user(ALPHA_USERNAME)
+ chart = self.insert_chart("restore_perm_test", [alpha.id], 1)
+ chart_id = chart.id
+ chart_uuid = str(chart.uuid)
+
+ self.login(ALPHA_USERNAME)
+ rv = self.client.delete(f"/api/v1/chart/{chart_id}")
+ assert rv.status_code == 200, (
+ f"Alpha owner soft-delete failed: {rv.status_code} {rv.data!r}"
+ )
+
+ rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore")
+ assert rv.status_code == 200, (
+ f"Expected 200 from Alpha owner restore (can_write_Chart), got "
+ f"{rv.status_code}: {rv.data!r}. If 403, "
+ "method_permission_name is missing 'restore': 'write'."
+ )
+
+ # Cleanup
+ _hard_delete_chart(chart_id)
+
+ def test_restore_chart_reattaches_to_dashboards(self) -> None:
+ """Soft-deleting a chart preserves dashboard_slices junction rows;
+ restore makes the chart reappear in its dashboards automatically.
+
+ This is the positive test that pins down the SIP's "no cascade"
+ contract and the corrected commit ``feat(soft-delete): preserve
+ dashboard_slices on chart soft-delete (MissingChart handles UI)``.
+ Soft-delete leaves the junction intact so:
+
+ - dashboards continue to render the chart slot (frontend uses
+ ``MissingChart`` placeholder while the chart is hidden via the
+ visibility filter)
+ - on restore the chart is automatically a member of every
+ dashboard it was a member of before, with no manual
+ re-attachment step
+ """
+ admin = self.get_user("admin")
+ admin_id = admin.id
+
+ chart = self.insert_chart("reattach_test_chart", [admin_id], 1)
+ chart_id = chart.id
+ chart_uuid = str(chart.uuid)
+
+ dashboard = Dashboard(
+ dashboard_title="reattach_test_dashboard",
+ slug="slug_reattach_test",
+ owners=[admin],
+ published=True,
+ )
+ dashboard.slices = [chart]
+ db.session.add(dashboard)
+ db.session.commit()
+ dashboard_id = dashboard.id
+
+ # Sanity: the junction row exists
+ junction_count = (
+ db.session.query(dashboard_slices)
+ .filter(
+ dashboard_slices.c.dashboard_id == dashboard_id,
+ dashboard_slices.c.slice_id == chart_id,
+ )
+ .count()
+ )
+ assert junction_count == 1, "junction row should exist after dashboard
creation"
+
+ self.login(ADMIN_USERNAME)
+
+ # Soft-delete the chart
+ rv = self.client.delete(f"/api/v1/chart/{chart_id}")
+ assert rv.status_code == 200
+
+ # The junction row is preserved (no cascade)
+ junction_count_after_delete = (
+ db.session.query(dashboard_slices)
+ .filter(
+ dashboard_slices.c.dashboard_id == dashboard_id,
+ dashboard_slices.c.slice_id == chart_id,
+ )
+ .count()
+ )
+ assert junction_count_after_delete == 1, (
+ "junction row should remain intact on chart soft-delete; "
+ "MissingChart placeholder handles the UI gap"
+ )
+
+ # The dashboard's loaded `slices` collection no longer includes the
+ # soft-deleted chart (the global visibility filter applies to
+ # relationship loads via `with_loader_criteria(...,
include_aliases=True)`).
+ db.session.expire_all()
+ dashboard_after_delete = (
+ db.session.query(Dashboard).filter(Dashboard.id ==
dashboard_id).one()
+ )
+ assert chart_id not in [s.id for s in dashboard_after_delete.slices], (
+ "soft-deleted chart should be filtered out of dashboard.slices "
+ "by the visibility-filter listener"
+ )
+
+ # Restore the chart
+ rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore")
+ assert rv.status_code == 200
+
+ # The chart automatically reappears in the dashboard — junction row
+ # was preserved, so no manual reattach was needed.
+ db.session.expire_all()
+ dashboard_after_restore = (
+ db.session.query(Dashboard).filter(Dashboard.id ==
dashboard_id).one()
+ )
+ assert chart_id in [s.id for s in dashboard_after_restore.slices], (
+ "restored chart should reappear in dashboard.slices automatically;
"
+ "the junction row was never removed by soft-delete"
+ )
+
+ # Cleanup
+ _hard_delete_dashboard_for_charts_test(dashboard_id)
+ _hard_delete_chart(chart_id)
+
+ def test_restore_chart_by_non_admin_owner(self) -> None:
+ """Non-admin owners can restore their own soft-deleted charts.
+
+ The unit-level restore command tests mock security; this
+ integration test exercises the FAB security wiring end-to-end
+ so a future change that breaks the owner check on a non-admin
+ path can't slip through.
+ """
+ alpha = self.get_user(ALPHA_USERNAME)
+ alpha_id = alpha.id
+
+ chart = self.insert_chart("alpha_owned_chart", [alpha_id], 1)
+ chart_id = chart.id
+ chart_uuid = str(chart.uuid)
+
+ self.login(ALPHA_USERNAME)
+ rv = self.client.delete(f"/api/v1/chart/{chart_id}")
+ assert rv.status_code == 200
+
+ rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore")
+ assert rv.status_code == 200, rv.data
+
+ db.session.expire_all()
+ restored = db.session.query(Slice).filter(Slice.id ==
chart_id).one_or_none()
Review Comment:
**Suggestion:** Add an explicit optional type annotation to this restored
entity variable to align with the type-hint requirement for relevant local
variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The variable `restored` is assigned the result of `one_or_none()`, so it is
a relevant local variable that can be annotated as optional. The existing code
leaves it untyped, matching the rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6df42d5cb8b74ff8b84720f1dfc36e9d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6df42d5cb8b74ff8b84720f1dfc36e9d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/charts/soft_delete_tests.py
**Line:** 555:555
**Comment:**
*Custom Rule: Add an explicit optional type annotation to this restored
entity variable to align with the type-hint requirement for relevant local
variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=8edf7355fa168c675b996089efe8f0241b3496ab06c7d273640a58c39b45851f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=8edf7355fa168c675b996089efe8f0241b3496ab06c7d273640a58c39b45851f&reaction=dislike'>👎</a>
##########
tests/integration_tests/charts/soft_delete_tests.py:
##########
@@ -0,0 +1,560 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Integration tests for chart soft-delete and restore."""
+
+from datetime import datetime
+
+from superset import security_manager
+from superset.connectors.sqla.models import SqlaTable
+from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
+from superset.extensions import db
+from superset.models.dashboard import Dashboard, dashboard_slices
+from superset.models.slice import Slice
+from superset.reports.models import (
+ ReportCreationMethod,
+ ReportSchedule,
+ ReportScheduleType,
+)
+from superset.utils import json
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.constants import (
+ ADMIN_USERNAME,
+ ALPHA_USERNAME,
+ GAMMA_USERNAME,
+)
+from tests.integration_tests.insert_chart_mixin import InsertChartMixin
+
+
+def _hard_delete_chart(chart_id: int) -> None:
+ """Hard-delete a chart row regardless of soft-delete state."""
+ row = (
+ db.session.query(Slice)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}})
+ .filter(Slice.id == chart_id)
+ .one_or_none()
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this query result
variable to comply with the required type-hint rule for annotatable variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The rule requires type hints on relevant variables that can be annotated.
This local ORM result variable is assigned from `one_or_none()` and can be
typed as an optional `Slice`, but the existing code omits that annotation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f5c0b0262f2846d98c4776d99644a9e4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f5c0b0262f2846d98c4776d99644a9e4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/charts/soft_delete_tests.py
**Line:** 44:49
**Comment:**
*Custom Rule: Add an explicit type annotation for this query result
variable to comply with the required type-hint rule for annotatable variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=cc0150db73394e16037ce102c42cd84df3fbc35bc75d0174bfea544fbaa427df&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=cc0150db73394e16037ce102c42cd84df3fbc35bc75d0174bfea544fbaa427df&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
