codeant-ai-for-open-source[bot] commented on code in PR #40128: URL: https://github.com/apache/superset/pull/40128#discussion_r3492945498
########## tests/unit_tests/commands/dashboard/restore_test.py: ########## @@ -0,0 +1,201 @@ +# 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 RestoreDashboardCommand.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest + + +def test_restore_dashboard_not_found_raises(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardNotFoundError for missing dashboard.""" + from superset.commands.dashboard.exceptions import DashboardNotFoundError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + with patch("superset.daos.dashboard.DashboardDAO.find_by_id", return_value=None): + cmd = RestoreDashboardCommand("999") Review Comment: **Suggestion:** Add an explicit type annotation for this local command variable to comply with the type-hint requirement for relevant variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This new test code introduces a local variable holding a command instance without a type annotation, and the rule requires annotating relevant variables that can be typed. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=43018294aa5a441684b41d6ae7c90df0&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=43018294aa5a441684b41d6ae7c90df0&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/unit_tests/commands/dashboard/restore_test.py **Line:** 33:33 **Comment:** *Custom Rule: Add an explicit type annotation for this local command variable to comply with 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%2F40128&comment_hash=8306d9fda3d95139f8ccf78b8a7955d3b2c604625e1626de42e02e2991a3b646&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=8306d9fda3d95139f8ccf78b8a7955d3b2c604625e1626de42e02e2991a3b646&reaction=dislike'>👎</a> ########## tests/unit_tests/commands/dashboard/restore_test.py: ########## @@ -0,0 +1,201 @@ +# 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 RestoreDashboardCommand.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest + + +def test_restore_dashboard_not_found_raises(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardNotFoundError for missing dashboard.""" + from superset.commands.dashboard.exceptions import DashboardNotFoundError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + with patch("superset.daos.dashboard.DashboardDAO.find_by_id", return_value=None): + cmd = RestoreDashboardCommand("999") + with pytest.raises(DashboardNotFoundError): + cmd.run() + + +def test_restore_active_dashboard_raises_not_found(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardNotFoundError for non-deleted dashboard.""" # noqa: E501 + from superset.commands.dashboard.exceptions import DashboardNotFoundError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + dashboard = MagicMock() Review Comment: **Suggestion:** Add an explicit type annotation for this local mock variable instead of leaving it untyped. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The local mock variable is introduced without an explicit type annotation, which matches the rule about annotating relevant Python variables that can be typed. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=410285fdfb764e55a3b82b9ef7c6b6d3&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=410285fdfb764e55a3b82b9ef7c6b6d3&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/unit_tests/commands/dashboard/restore_test.py **Line:** 43:43 **Comment:** *Custom Rule: Add an explicit type annotation for this local mock variable instead of leaving it untyped. 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%2F40128&comment_hash=b70d8e1c6561918eaf9ad6e3b2e08e295b779f71cb7aac94bd9d1ce69974265a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=b70d8e1c6561918eaf9ad6e3b2e08e295b779f71cb7aac94bd9d1ce69974265a&reaction=dislike'>👎</a> ########## tests/unit_tests/commands/dashboard/restore_test.py: ########## @@ -0,0 +1,201 @@ +# 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 RestoreDashboardCommand.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest + + +def test_restore_dashboard_not_found_raises(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardNotFoundError for missing dashboard.""" + from superset.commands.dashboard.exceptions import DashboardNotFoundError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + with patch("superset.daos.dashboard.DashboardDAO.find_by_id", return_value=None): + cmd = RestoreDashboardCommand("999") + with pytest.raises(DashboardNotFoundError): + cmd.run() + + +def test_restore_active_dashboard_raises_not_found(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardNotFoundError for non-deleted dashboard.""" # noqa: E501 + from superset.commands.dashboard.exceptions import DashboardNotFoundError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + dashboard = MagicMock() + dashboard.deleted_at = None # not soft-deleted + + with patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ): + cmd = RestoreDashboardCommand("1") + with pytest.raises(DashboardNotFoundError): + cmd.run() + + +def test_restore_dashboard_forbidden_raises(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardForbiddenError on permission check.""" + from superset.commands.dashboard.exceptions import DashboardForbiddenError + from superset.commands.dashboard.restore import RestoreDashboardCommand + from superset.exceptions import SupersetSecurityException + + dashboard = MagicMock() Review Comment: **Suggestion:** Add an explicit type annotation for this local mock variable in this test case as well. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is another unannotated local variable in modified Python code, so it is a real instance of the type-hint rule violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9add043402744773ab0132e09e6f4e58&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=9add043402744773ab0132e09e6f4e58&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/unit_tests/commands/dashboard/restore_test.py **Line:** 60:60 **Comment:** *Custom Rule: Add an explicit type annotation for this local mock variable in this test case as well. 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%2F40128&comment_hash=4f15f76a629cb48a657b5d11b35b72ce8333009ecd776a4a6b46e188ec692d47&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=4f15f76a629cb48a657b5d11b35b72ce8333009ecd776a4a6b46e188ec692d47&reaction=dislike'>👎</a> ########## tests/unit_tests/commands/dashboard/restore_test.py: ########## @@ -0,0 +1,201 @@ +# 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 RestoreDashboardCommand.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest + + +def test_restore_dashboard_not_found_raises(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardNotFoundError for missing dashboard.""" + from superset.commands.dashboard.exceptions import DashboardNotFoundError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + with patch("superset.daos.dashboard.DashboardDAO.find_by_id", return_value=None): + cmd = RestoreDashboardCommand("999") + with pytest.raises(DashboardNotFoundError): + cmd.run() + + +def test_restore_active_dashboard_raises_not_found(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardNotFoundError for non-deleted dashboard.""" # noqa: E501 + from superset.commands.dashboard.exceptions import DashboardNotFoundError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + dashboard = MagicMock() + dashboard.deleted_at = None # not soft-deleted + + with patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ): + cmd = RestoreDashboardCommand("1") + with pytest.raises(DashboardNotFoundError): + cmd.run() + + +def test_restore_dashboard_forbidden_raises(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardForbiddenError on permission check.""" + from superset.commands.dashboard.exceptions import DashboardForbiddenError + from superset.commands.dashboard.restore import RestoreDashboardCommand + from superset.exceptions import SupersetSecurityException + + dashboard = MagicMock() + dashboard.deleted_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + + def raise_security(*args: object, **kwargs: object) -> None: + raise SupersetSecurityException(MagicMock()) + + with ( + patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ), + patch("superset.commands.restore.security_manager") as mock_sec, + ): + mock_sec.raise_for_ownership = raise_security + + cmd = RestoreDashboardCommand("1") + with pytest.raises(DashboardForbiddenError): + cmd.run() + + +def test_restore_dashboard_slug_conflict_raises(app_context: None) -> None: + """Restore raises DashboardSlugConflictError when slug is now claimed by an active dashboard. + + The partial unique index ``WHERE deleted_at IS NULL`` allows another + dashboard to claim the slug while this one was soft-deleted. The + command catches that case before flushing so the operator sees a + domain-specific error instead of an opaque unique-index violation. + """ # noqa: E501 + from superset.commands.dashboard.exceptions import DashboardSlugConflictError + from superset.commands.dashboard.restore import RestoreDashboardCommand + Review Comment: **Suggestion:** Add an explicit type annotation for this local mock variable to satisfy the rule on annotating relevant variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The test creates a local mock variable without a type hint, which is exactly what the Python type-hint rule flags for relevant variables. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=80d45c745b6a4bbdbf71cae7e2a4d4da&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=80d45c745b6a4bbdbf71cae7e2a4d4da&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/unit_tests/commands/dashboard/restore_test.py **Line:** 89:89 **Comment:** *Custom Rule: Add an explicit type annotation for this local mock variable to satisfy the rule on annotating 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%2F40128&comment_hash=59df9f0f04252d60262c92a278c7ae92ed66360d3effd3e30ddbc141fbe075a6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=59df9f0f04252d60262c92a278c7ae92ed66360d3effd3e30ddbc141fbe075a6&reaction=dislike'>👎</a> ########## tests/unit_tests/migrations/test_add_deleted_at_to_dashboards.py: ########## @@ -0,0 +1,241 @@ +# 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 ``9e1f3b8c4d2a_add_deleted_at_to_dashboards``. + +Runs the migration's ``upgrade()`` and ``downgrade()`` against an +in-memory SQLite engine with a real Alembic ``Operations`` context. + +The migration has two responsibilities: + +1. Add ``deleted_at`` column + ``ix_dashboards_deleted_at`` index — the + shared soft-delete schema. +2. Swap the legacy full unique constraint on ``slug`` for a partial + index (Postgres) or functional index (MySQL 8.0.13+). On SQLite + and older MySQL the slug-constraint swap is a documented no-op + (the original full constraint stays in place). + +The tests pin both responsibilities. The slug-swap path is exercised +only on the no-op branch (the SQLite engine the unit tests run +against); the Postgres / MySQL functional-index paths are covered by +the integration tests against those backends. +""" + +from __future__ import annotations + +from datetime import datetime, timezone +from importlib import import_module + +import pytest +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import ( + Column, + create_engine, + Index, + insert, + inspect, + Integer, + MetaData, + select, + String, + Table, +) +from sqlalchemy.engine import Engine + +migration = import_module( + "superset.migrations.versions." + "2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards" +) + +TABLE_NAME = migration.TABLE_NAME # "dashboards" +DELETED_AT_INDEX_NAME = migration.DELETED_AT_INDEX_NAME +LEGACY_SLUG_INDEX_NAME = migration.LEGACY_SLUG_INDEX_NAME Review Comment: **Suggestion:** Add explicit type annotations to these module-level constants to satisfy the type-hints requirement for relevant variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> These module-level constants are newly added and have no explicit type annotations. Since they are relevant variables that can be annotated, the code violates the type-hints rule. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=058e69ab830f415faf93ee8f57234a4d&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=058e69ab830f415faf93ee8f57234a4d&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/unit_tests/migrations/test_add_deleted_at_to_dashboards.py **Line:** 64:66 **Comment:** *Custom Rule: Add explicit type annotations to these module-level constants to satisfy the type-hints 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%2F40128&comment_hash=a21d6581e38a050817d9f0db107b8a9f7b9f371424bdf064839246d7cf1f7e5d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=a21d6581e38a050817d9f0db107b8a9f7b9f371424bdf064839246d7cf1f7e5d&reaction=dislike'>👎</a> ########## tests/unit_tests/migrations/test_add_deleted_at_to_dashboards.py: ########## @@ -0,0 +1,241 @@ +# 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 ``9e1f3b8c4d2a_add_deleted_at_to_dashboards``. + +Runs the migration's ``upgrade()`` and ``downgrade()`` against an +in-memory SQLite engine with a real Alembic ``Operations`` context. + +The migration has two responsibilities: + +1. Add ``deleted_at`` column + ``ix_dashboards_deleted_at`` index — the + shared soft-delete schema. +2. Swap the legacy full unique constraint on ``slug`` for a partial + index (Postgres) or functional index (MySQL 8.0.13+). On SQLite + and older MySQL the slug-constraint swap is a documented no-op + (the original full constraint stays in place). + +The tests pin both responsibilities. The slug-swap path is exercised +only on the no-op branch (the SQLite engine the unit tests run +against); the Postgres / MySQL functional-index paths are covered by +the integration tests against those backends. +""" + +from __future__ import annotations + +from datetime import datetime, timezone +from importlib import import_module + +import pytest +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import ( + Column, + create_engine, + Index, + insert, + inspect, + Integer, + MetaData, + select, + String, + Table, +) +from sqlalchemy.engine import Engine + +migration = import_module( + "superset.migrations.versions." + "2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards" +) Review Comment: **Suggestion:** Add an explicit type annotation for the imported migration module variable so this new module-level variable is type hinted. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This new module-level variable is assigned in Python code without any type annotation. The custom rule requires type hints on relevant variables that can be annotated, so this is a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aa649005f9c243d79be4836a10853b4c&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=aa649005f9c243d79be4836a10853b4c&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/unit_tests/migrations/test_add_deleted_at_to_dashboards.py **Line:** 59:62 **Comment:** *Custom Rule: Add an explicit type annotation for the imported migration module variable so this new module-level variable is type hinted. 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%2F40128&comment_hash=d4528cb565d66d5c150748662847689817271a6c473775ac3a4b62b518cfe5ae&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=d4528cb565d66d5c150748662847689817271a6c473775ac3a4b62b518cfe5ae&reaction=dislike'>👎</a> ########## tests/unit_tests/commands/dashboard/restore_test.py: ########## @@ -0,0 +1,201 @@ +# 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 RestoreDashboardCommand.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest + + +def test_restore_dashboard_not_found_raises(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardNotFoundError for missing dashboard.""" + from superset.commands.dashboard.exceptions import DashboardNotFoundError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + with patch("superset.daos.dashboard.DashboardDAO.find_by_id", return_value=None): + cmd = RestoreDashboardCommand("999") + with pytest.raises(DashboardNotFoundError): + cmd.run() + + +def test_restore_active_dashboard_raises_not_found(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardNotFoundError for non-deleted dashboard.""" # noqa: E501 + from superset.commands.dashboard.exceptions import DashboardNotFoundError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + dashboard = MagicMock() + dashboard.deleted_at = None # not soft-deleted + + with patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ): + cmd = RestoreDashboardCommand("1") + with pytest.raises(DashboardNotFoundError): + cmd.run() + + +def test_restore_dashboard_forbidden_raises(app_context: None) -> None: + """RestoreDashboardCommand raises DashboardForbiddenError on permission check.""" + from superset.commands.dashboard.exceptions import DashboardForbiddenError + from superset.commands.dashboard.restore import RestoreDashboardCommand + from superset.exceptions import SupersetSecurityException + + dashboard = MagicMock() + dashboard.deleted_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + + def raise_security(*args: object, **kwargs: object) -> None: + raise SupersetSecurityException(MagicMock()) + + with ( + patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ), + patch("superset.commands.restore.security_manager") as mock_sec, + ): + mock_sec.raise_for_ownership = raise_security + + cmd = RestoreDashboardCommand("1") + with pytest.raises(DashboardForbiddenError): + cmd.run() + + +def test_restore_dashboard_slug_conflict_raises(app_context: None) -> None: + """Restore raises DashboardSlugConflictError when slug is now claimed by an active dashboard. + + The partial unique index ``WHERE deleted_at IS NULL`` allows another + dashboard to claim the slug while this one was soft-deleted. The + command catches that case before flushing so the operator sees a + domain-specific error instead of an opaque unique-index violation. + """ # noqa: E501 + from superset.commands.dashboard.exceptions import DashboardSlugConflictError + from superset.commands.dashboard.restore import RestoreDashboardCommand + + dashboard = MagicMock() + dashboard.deleted_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + dashboard.slug = "q1-report" + dashboard.id = 42 + + with ( + patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ), + patch("superset.commands.restore.security_manager") as mock_sec, + patch.object( + RestoreDashboardCommand, "_has_active_slug_twin", return_value=True + ) as mock_twin_check, + ): + mock_sec.raise_for_ownership.return_value = None + + cmd = RestoreDashboardCommand("1") + with pytest.raises(DashboardSlugConflictError): + cmd.run() + + mock_twin_check.assert_called_once_with(dashboard) + + +def test_restore_dashboard_no_slug_conflict_when_no_active_collision( + app_context: None, +) -> None: + """No collision check fires when no other active dashboard has the same slug.""" + from superset.commands.dashboard.restore import RestoreDashboardCommand + + dashboard = MagicMock() Review Comment: **Suggestion:** Add an explicit type annotation for this local mock variable rather than relying on implicit typing. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This modified Python code includes another untyped local variable that can be annotated, so the rule violation is real. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a8a78814b7a14f5e9c09c3335ea217f3&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=a8a78814b7a14f5e9c09c3335ea217f3&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/unit_tests/commands/dashboard/restore_test.py **Line:** 119:119 **Comment:** *Custom Rule: Add an explicit type annotation for this local mock variable rather than relying on implicit typing. 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%2F40128&comment_hash=a164d054f015de49dc916dd9af39872e06b2d690a6b41c1f231a88e0571116c5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=a164d054f015de49dc916dd9af39872e06b2d690a6b41c1f231a88e0571116c5&reaction=dislike'>👎</a> ########## superset/models/dashboard.py: ########## @@ -192,6 +202,27 @@ class Dashboard(CoreDashboard, AuditMixinNullable, ImportExportMixin): ] extra_import_fields = ["is_managed_externally", "external_url", "theme_id"] + @classmethod + def _unique_constraints(cls) -> list[set[str]]: Review Comment: **Suggestion:** Add an explicit type hint for the class parameter in this classmethod signature to satisfy the type-hint requirement for modified methods. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The new classmethod omits a type hint for the `cls` parameter even though the rule requires type hints on modified Python methods when annotatable parameters are present. This is a real violation in the current code. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=18b0236303af41b9a01916070257cd71&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=18b0236303af41b9a01916070257cd71&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/models/dashboard.py **Line:** 205:206 **Comment:** *Custom Rule: Add an explicit type hint for the class parameter in this classmethod signature to satisfy the type-hint requirement for modified methods. 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%2F40128&comment_hash=7065deb3fa152d86e6aa3c935716b98a3d0929947d2332f87f68cd94fc9b88fd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=7065deb3fa152d86e6aa3c935716b98a3d0929947d2332f87f68cd94fc9b88fd&reaction=dislike'>👎</a> ########## superset/models/dashboard.py: ########## @@ -192,6 +202,27 @@ class Dashboard(CoreDashboard, AuditMixinNullable, ImportExportMixin): ] extra_import_fields = ["is_managed_externally", "external_url", "theme_id"] + @classmethod + def _unique_constraints(cls) -> list[set[str]]: + """Import identity keys for ``import_from_dict``. + + ``slug`` lost its column-level ``unique=True`` when the full unique + constraint was replaced by a partial (active-rows-only) index for + soft-delete. ``ImportExportMixin._unique_constraints`` derives import + lookup keys from unique columns/constraints, so without this override a + re-import whose UUID differs but whose ``slug`` matches an existing + active dashboard would no longer be matched-and-updated by slug — it + would fall through to an insert and collide on the partial active-slug + index at flush. Re-add ``{"slug"}`` here so import keeps matching by + slug (the pre-soft-delete behaviour) while DB-level uniqueness stays + partial. A ``NULL`` slug is skipped by the importer's filter builder, + so this only adds a real lookup when the imported config carries a slug. + """ + constraints = super()._unique_constraints() Review Comment: **Suggestion:** Add an explicit variable type annotation for this new local variable to comply with the rule requiring type hints on relevant annotatable variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The variable `constraints` is newly introduced in modified code and its type is readily inferable as `list[set[str]]`, so it is a relevant annotatable variable under the rule. The code omits that annotation, making the suggestion valid. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=46913adddd9d41db90be44f68722b35d&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=46913adddd9d41db90be44f68722b35d&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/models/dashboard.py **Line:** 221:221 **Comment:** *Custom Rule: Add an explicit variable type annotation for this new local variable to comply with the rule requiring type hints on 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%2F40128&comment_hash=322a23f4674dabb11a87cae5ab23ad312be401b90813158dd9a83a339c2f3b12&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=322a23f4674dabb11a87cae5ab23ad312be401b90813158dd9a83a339c2f3b12&reaction=dislike'>👎</a> ########## tests/integration_tests/base_tests.py: ########## @@ -595,10 +596,17 @@ def insert_dashboard( role_obj = db.session.query(security_manager.role_model).get(role) obj_roles.append(role_obj) - # Defensive cleanup: remove any existing dashboard with the same slug + # Defensive cleanup: remove any existing dashboard with the same slug. + # Bypass the soft-delete visibility filter so a soft-deleted row from + # a prior test still gets cleared — without the bypass the lookup + # returns ``None`` and the INSERT below trips the unique constraint + # on ``slug`` against the soft-deleted (but hidden) row. if slug: existing_dashboard = ( - db.session.query(Dashboard).filter_by(slug=slug).first() + db.session.query(Dashboard) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}}) + .filter_by(slug=slug) + .first() ) Review Comment: **Suggestion:** Add an explicit type annotation for the newly introduced local variable that stores the dashboard query result (for example as an optional dashboard type) to satisfy the type-hint requirement. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The new local variable is introduced without a type annotation, and its type is readily inferable as an optional Dashboard result from `.first()`. This 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=a67ef2e1ceac409e9b66853f4906a7ad&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=a67ef2e1ceac409e9b66853f4906a7ad&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/base_tests.py **Line:** 605:610 **Comment:** *Custom Rule: Add an explicit type annotation for the newly introduced local variable that stores the dashboard query result (for example as an optional dashboard type) to satisfy the type-hint requirement. 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%2F40128&comment_hash=54d65cfbb05d8d25923b4a7b5e5b3f452c5a3371348179e012994fe88ea45d88&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=54d65cfbb05d8d25923b4a7b5e5b3f452c5a3371348179e012994fe88ea45d88&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]
