codeant-ai-for-open-source[bot] commented on code in PR #40129: URL: https://github.com/apache/superset/pull/40129#discussion_r3510308185
########## tests/unit_tests/commands/chart/restore_test.py: ########## @@ -0,0 +1,90 @@ +# 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. +"""Unit tests for RestoreChartCommand.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest + +from superset.commands.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.chart.restore import RestoreChartCommand +from superset.exceptions import SupersetSecurityException + + +def test_restore_chart_clears_deleted_at(app_context: None) -> None: + """RestoreChartCommand.run() restores a soft-deleted chart.""" + chart = MagicMock() Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not require explicit type annotations for local test-scaffolding variables in test files when the type is already unambiguous from context. **Applied to:** - `**/test/**` - `**/tests/**` - `**/*test*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/unit_tests/commands/chart/restore_test.py: ########## @@ -0,0 +1,90 @@ +# 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. +"""Unit tests for RestoreChartCommand.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest + +from superset.commands.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.chart.restore import RestoreChartCommand +from superset.exceptions import SupersetSecurityException + + +def test_restore_chart_clears_deleted_at(app_context: None) -> None: + """RestoreChartCommand.run() restores a soft-deleted chart.""" + chart = MagicMock() + chart.deleted_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + chart.id = 1 + + with ( + patch( + "superset.daos.chart.ChartDAO.find_by_id", return_value=chart + ) as mock_find, + patch("superset.commands.restore.security_manager") as mock_sec, + ): + mock_sec.raise_for_ownership.return_value = None + + cmd = RestoreChartCommand("1") + cmd.run() + + mock_find.assert_called_once() + chart.restore.assert_called_once() + + +def test_restore_chart_not_found_raises(app_context: None) -> None: + """RestoreChartCommand raises ChartNotFoundError for missing chart.""" + with patch("superset.daos.chart.ChartDAO.find_by_id", return_value=None): + cmd = RestoreChartCommand("999") + with pytest.raises(ChartNotFoundError): + cmd.run() + + +def test_restore_active_chart_raises_not_found(app_context: None) -> None: + """RestoreChartCommand raises ChartNotFoundError for non-deleted chart.""" + chart = MagicMock() Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not flag missing explicit type annotations for local mock/scaffolding variables in test files when the type is unambiguous from context. **Applied to:** - `**/test/**` - `**/tests/**` - `**/*test*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/unit_tests/migrations/test_add_deleted_at_to_slices.py: ########## @@ -0,0 +1,186 @@ +# 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. +"""Tests for migration ``7c4a8d09ca37_add_deleted_at_to_slices``. + +Runs the migration's ``upgrade()`` and ``downgrade()`` against an +in-memory SQLite engine with a real Alembic ``Operations`` context. +The behaviour being pinned is the operator-facing contract documented +in ``UPDATING.md``: ``downgrade()`` reverses the schema but does not +hard-delete or otherwise mutate rows that were soft-deleted before +the migration was reversed — those rows survive the downgrade and +become visible to any code path that no longer applies the +soft-delete visibility filter. +""" + +from __future__ import annotations + +from datetime import datetime +from importlib import import_module + +import pytest +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import ( + Column, + create_engine, + insert, + inspect, + Integer, + MetaData, + select, + String, + Table, +) +from sqlalchemy.engine import Engine + +migration = import_module( + "superset.migrations.versions." + "2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices" +) + +TABLE_NAME = migration.TABLE_NAME # "slices" +INDEX_NAME = migration.INDEX_NAME # "ix_slices_deleted_at" Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not flag missing type annotations for local/test-scaffolding variables in test files when the type is unambiguous from context and the file follows surrounding test conventions. **Applied to:** - `**/test/**` - `**/tests/**` - `**/*test*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## 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: ✅ **Customized review instruction saved!** **Instruction:** > Do not require explicit type annotations for clearly unambiguous local or class-level scaffolding variables when the surrounding file already follows that convention. **Applied to:** - `superset/charts/**` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## 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" Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > In Alembic migration files, do not flag `down_revision` values that intentionally chain off the current single head on `master`; this is required to avoid forking the migration DAG and to pass migration-conflict checks. **Applied to:** - `**/migrations/versions/*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## 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: ✅ **Customized review instruction saved!** **Instruction:** > Do not require additional type annotations for local variables in this file when the type is already unambiguous from context. **Applied to:** - `superset/charts/filters.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* -- 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]
