codeant-ai-for-open-source[bot] commented on code in PR #40128: URL: https://github.com/apache/superset/pull/40128#discussion_r3492947098
########## superset/commands/dashboard/restore.py: ########## @@ -0,0 +1,84 @@ +# 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. +"""Command to restore a soft-deleted dashboard.""" + +from superset.commands.dashboard.exceptions import ( + DashboardForbiddenError, + DashboardNotFoundError, + DashboardRestoreFailedError, + DashboardSlugConflictError, +) +from superset.commands.restore import BaseRestoreCommand +from superset.daos.dashboard import DashboardDAO +from superset.models.dashboard import Dashboard + + +class RestoreDashboardCommand(BaseRestoreCommand[Dashboard]): + """Restore a soft-deleted dashboard by clearing its ``deleted_at`` field. + + Most behaviour is inherited from ``BaseRestoreCommand``. The override + here adds the slug-conflict check: with the partial unique index on + ``slug WHERE deleted_at IS NULL``, slug reuse during the soft-deleted + window is allowed, so a restore may now collide with an active row + that claimed the slug while this one was deleted. Raise a clean + domain error in that case instead of letting the unique-index + violation surface as an opaque ``IntegrityError`` at flush time. + """ + + dao = DashboardDAO + not_found_exc = DashboardNotFoundError + forbidden_exc = DashboardForbiddenError + restore_failed_exc = DashboardRestoreFailedError + + def validate(self) -> Dashboard: # type: ignore[override] + """Extend ``BaseRestoreCommand.validate`` with a slug-conflict pre-check. + + Raises ``DashboardSlugConflictError`` when the dashboard has a + ``slug`` that has been claimed by another active dashboard while + this one was soft-deleted. Surfacing the conflict as a domain + error here keeps callers from seeing an opaque ``IntegrityError`` + at flush time on dialects with the partial index, and a + constraint-violation 500 on dialects without it. + """ + model = super().validate() Review Comment: **Suggestion:** Annotate the local variable with an explicit type to comply with the requirement for type hints on relevant annotatable variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The local variable `model` is introduced without an explicit type annotation even though its type is knowable from the return value of `validate()`. Under the stated rule, this is an omission of a relevant type hint. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4a781df32c344f15a84acdc33efa9645&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=4a781df32c344f15a84acdc33efa9645&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/commands/dashboard/restore.py **Line:** 57:57 **Comment:** *Custom Rule: Annotate the local variable with an explicit type to comply with the requirement for 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=27d1bdac5e8818407eb0d46b5762c89653f7a2a10a81969a42d21389c1e4ff97&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=27d1bdac5e8818407eb0d46b5762c89653f7a2a10a81969a42d21389c1e4ff97&reaction=dislike'>👎</a> ########## superset/commands/dashboard/restore.py: ########## @@ -0,0 +1,84 @@ +# 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. +"""Command to restore a soft-deleted dashboard.""" + +from superset.commands.dashboard.exceptions import ( + DashboardForbiddenError, + DashboardNotFoundError, + DashboardRestoreFailedError, + DashboardSlugConflictError, +) +from superset.commands.restore import BaseRestoreCommand +from superset.daos.dashboard import DashboardDAO +from superset.models.dashboard import Dashboard + + +class RestoreDashboardCommand(BaseRestoreCommand[Dashboard]): + """Restore a soft-deleted dashboard by clearing its ``deleted_at`` field. + + Most behaviour is inherited from ``BaseRestoreCommand``. The override + here adds the slug-conflict check: with the partial unique index on + ``slug WHERE deleted_at IS NULL``, slug reuse during the soft-deleted + window is allowed, so a restore may now collide with an active row + that claimed the slug while this one was deleted. Raise a clean + domain error in that case instead of letting the unique-index + violation surface as an opaque ``IntegrityError`` at flush time. + """ + + dao = DashboardDAO + not_found_exc = DashboardNotFoundError + forbidden_exc = DashboardForbiddenError + restore_failed_exc = DashboardRestoreFailedError Review Comment: **Suggestion:** Add explicit class-variable type hints for the command configuration attributes so the subclass satisfies the typed contract defined by the base restore command. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> These newly added class attributes are assignable, annotatable variables and currently lack type hints. The custom rule requires type hints on relevant variables in new or modified Python code, so this is a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a023fb6bf32b48659c161b0a155369de&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=a023fb6bf32b48659c161b0a155369de&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/commands/dashboard/restore.py **Line:** 42:45 **Comment:** *Custom Rule: Add explicit class-variable type hints for the command configuration attributes so the subclass satisfies the typed contract defined by the base restore command. 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=b721cbb8ce539e367fe54d6c86ea9ef76b34a13bcedac9374c945ebef100ffee&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=b721cbb8ce539e367fe54d6c86ea9ef76b34a13bcedac9374c945ebef100ffee&reaction=dislike'>👎</a> ########## superset/dashboards/filters.py: ########## @@ -265,3 +266,38 @@ def apply(self, query: Query, value: Any) -> Query: if value is False: return query.filter(and_(Dashboard.created_by_fk.is_(None))) return query + + +class DashboardDeletedStateFilter( # pylint: disable=too-few-public-methods + BaseDeletedStateFilter +): + """Rison filter for the GET list that exposes soft-deleted dashboards. + + Soft-deleted rows are additionally scoped to the **restore audience**: + only the dashboard's owners (or admins) may enumerate them. This mirrors + ``RestoreDashboardCommand``'s ``raise_for_ownership`` check, so a + read-access non-owner (who can see the dashboard via published-datasource + access or dashboard RBAC) cannot list soft-deleted dashboards they could + never restore. Live rows are unaffected — they keep their normal + ``DashboardAccessFilter`` visibility. + """ + + arg_name = "dashboard_deleted_state" + model = Dashboard + + 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 derived local value to comply with the type-hint requirement for relevant new variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is a newly added local variable in Python code and it has no type annotation, which matches the custom rule requiring type hints on relevant variables that can be annotated. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fd77bbe399364ee49a24358610edd89f&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=fd77bbe399364ee49a24358610edd89f&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/dashboards/filters.py **Line:** 290:290 **Comment:** *Custom Rule: Add an explicit type annotation for this derived local value to comply with the type-hint requirement for relevant new 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=a3a03f961f1c84391f2b754e446c9173ec169c174bb3349005209a2b7cac6d1c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=a3a03f961f1c84391f2b754e446c9173ec169c174bb3349005209a2b7cac6d1c&reaction=dislike'>👎</a> ########## superset/dashboards/filters.py: ########## @@ -265,3 +266,38 @@ def apply(self, query: Query, value: Any) -> Query: if value is False: return query.filter(and_(Dashboard.created_by_fk.is_(None))) return query + + +class DashboardDeletedStateFilter( # pylint: disable=too-few-public-methods + BaseDeletedStateFilter +): + """Rison filter for the GET list that exposes soft-deleted dashboards. + + Soft-deleted rows are additionally scoped to the **restore audience**: + only the dashboard's owners (or admins) may enumerate them. This mirrors + ``RestoreDashboardCommand``'s ``raise_for_ownership`` check, so a + read-access non-owner (who can see the dashboard via published-datasource + access or dashboard RBAC) cannot list soft-deleted dashboards they could + never restore. Live rows are unaffected — they keep their normal + ``DashboardAccessFilter`` visibility. + """ + + arg_name = "dashboard_deleted_state" + model = Dashboard + + 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 dashboards they own. ``any()`` + # emits an EXISTS subquery so it composes with the base access filter + # without producing duplicate rows from a join. + owned = Dashboard.owners.any(security_manager.user_model.id == get_user_id()) Review Comment: **Suggestion:** Add a type annotation for this SQLAlchemy expression variable so the new logic remains fully typed. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This newly introduced SQLAlchemy expression variable is unannotated in Python code, so it falls under the type-hint rule for annotatable variables. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7273a69809ac424680e5efe9a52770d6&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=7273a69809ac424680e5efe9a52770d6&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/dashboards/filters.py **Line:** 297:297 **Comment:** *Custom Rule: Add a type annotation for this SQLAlchemy expression variable so the new logic remains 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%2F40128&comment_hash=25d80430735fb9d9cb1c4974d814bb20e3a2f4dc9c305729f710c649c6a3aab8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=25d80430735fb9d9cb1c4974d814bb20e3a2f4dc9c305729f710c649c6a3aab8&reaction=dislike'>👎</a> ########## superset/dashboards/api.py: ########## @@ -257,7 +265,17 @@ class DashboardRestApi(CustomTagsOptimizationMixin, BaseSupersetModelRestApi): allow_browser_login = True class_permission_name = "Dashboard" - 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_Dashboard``) 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 to satisfy the type-hint requirement for relevant variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This new class-level mapping is introduced without any type annotation, and it is a clearly annotatable variable. That matches the rule requiring type hints on relevant variables in modified Python code. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=826d912ef36442b89c6c9b265d92cae5&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=826d912ef36442b89c6c9b265d92cae5&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/dashboards/api.py **Line:** 275:278 **Comment:** *Custom Rule: Add an explicit type annotation to this new class-level mapping 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%2F40128&comment_hash=e32426f427cadbfcda21681f40e3da14c1f761d3a42abe1f6e594f8c8368fe7b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=e32426f427cadbfcda21681f40e3da14c1f761d3a42abe1f6e594f8c8368fe7b&reaction=dislike'>👎</a> ########## superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.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. +"""Add deleted_at + partial unique slug index for soft-delete. + +Adds to the ``dashboards`` table: +- a nullable ``deleted_at`` column for soft-delete state +- an index ``ix_dashboards_deleted_at`` for the visibility-filter listener +- a partial unique index ``ix_dashboards_active_slug`` enforcing slug + uniqueness only among active (non-soft-deleted) rows + +Drops: +- the existing full unique constraint on ``slug`` (named + ``idx_unique_slug``, created in migration 1a48a5411020) + +The constraint change makes the ``slug`` field reusable after soft-delete: +soft-deleted rows no longer reserve their slug for the lifetime of the +row. ``RestoreDashboardCommand`` handles the reverse case (restoring a +dashboard whose slug has since been claimed by another active row) with +an explicit conflict error. See UPDATING.md for the user-facing change. + +Dialect support for the partial index: +- PostgreSQL: native ``WHERE deleted_at IS NULL`` partial index +- MySQL 8.0+: functional index over + ``(CASE WHEN deleted_at IS NULL THEN slug END)`` +- MySQL <8.0: keeps the original full unique constraint (documented + limitation; functional indexes are not supported on these versions) +- SQLite: keeps the original full unique constraint (column-level + ``UNIQUE`` cannot be dropped without recreating the table, which is + not worth the migration complexity for a test-only dialect). Tests + that need to verify the partial-index behaviour run only on + PostgreSQL and MySQL 8+. + +Revision ID: 9e1f3b8c4d2a +Revises: 78a40c08b4be +Create Date: 2026-05-08 12:05:00.000000 +""" + +from alembic import op +from sqlalchemy import Column, DateTime +from sqlalchemy.engine import Connection + +from superset.migrations.shared.utils import ( + add_columns, + create_index, + drop_columns, + drop_index, + table_has_index, +) + +# revision identifiers, used by Alembic. +revision = "9e1f3b8c4d2a" +down_revision = "78a40c08b4be" + +TABLE_NAME = "dashboards" +DELETED_AT_INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at" +PARTIAL_SLUG_INDEX_NAME = f"ix_{TABLE_NAME}_active_slug" +# The original full unique constraint on ``slug`` was created with an +# explicit name in migration 1a48a5411020 (2015-12-04). Same name on +# PostgreSQL (constraint) and MySQL (index). +LEGACY_SLUG_INDEX_NAME = "idx_unique_slug" + + +def _mysql_supports_functional_index(bind: Connection) -> bool: + """Return True iff the connected MySQL is 8.0.13+ (supports functional indexes). + + MySQL added functional key parts in 8.0.13; 8.0.0–8.0.12 reject the + ``(CASE WHEN deleted_at IS NULL THEN slug END)`` expression at index + creation time, so deployments on those patch releases must keep the + original full slug constraint. See + https://dev.mysql.com/doc/mysql/8.0/en/create-index.html for the + 8.0.13 minimum. + + Excludes MariaDB even at server version ``>= (10, x)`` because MariaDB + reports through the same ``server_version_info`` attribute but uses + different functional-index semantics around ``CASE`` expressions. + Uses SQLAlchemy's parsed ``server_version_info`` rather than ``SELECT + VERSION()`` to avoid an extra round-trip and brittle string parsing. + """ + if getattr(bind.dialect, "is_mariadb", False): + return False + return (bind.dialect.server_version_info or ()) >= (8, 0, 13) + + +def upgrade() -> None: + bind = op.get_bind() + _add_deleted_at_column() + _replace_slug_constraint_with_partial_index(bind) + + +def downgrade() -> None: + bind = op.get_bind() + _restore_slug_constraint(bind) + _drop_deleted_at_column() + + +def _add_deleted_at_column() -> None: + add_columns(TABLE_NAME, Column("deleted_at", DateTime(), nullable=True)) + create_index(TABLE_NAME, DELETED_AT_INDEX_NAME, ["deleted_at"]) + + +def _drop_deleted_at_column() -> None: + drop_index(TABLE_NAME, DELETED_AT_INDEX_NAME) + drop_columns(TABLE_NAME, "deleted_at") + + +def _replace_slug_constraint_with_partial_index(bind: Connection) -> None: + """Swap the full UNIQUE on ``slug`` for a partial index where supported. + + The original constraint is named ``idx_unique_slug`` from migration + 1a48a5411020 — same name on PostgreSQL (constraint) and MySQL (index). + + SQLite and MySQL <8.0 are no-ops here: they keep the original full + unique constraint. See the module docstring for the rationale. + """ + dialect = bind.dialect.name + if dialect == "postgresql": + op.execute( + f"ALTER TABLE {TABLE_NAME} " + f"DROP CONSTRAINT IF EXISTS {LEGACY_SLUG_INDEX_NAME}" + ) + # Some installations may have the unique enforced as a plain + # index rather than a constraint. Both DROPs are IF EXISTS, so + # whichever path applies cleans up. + op.execute(f"DROP INDEX IF EXISTS {LEGACY_SLUG_INDEX_NAME}") Review Comment: **Suggestion:** Replace this raw index drop SQL with the shared migration helper for dropping indexes so database-compatibility behavior stays centralized. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The shared migration utility `drop_index` exists in `superset.migrations.shared.utils` and is designed for cross-database index removal. This code uses raw SQL `op.execute(...)` for dropping an index instead of the helper, so it matches the migration-helper rule violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a49c1877c56c4dcfaa9bf03d39c11ecd&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=a49c1877c56c4dcfaa9bf03d39c11ecd&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-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py **Line:** 138:138 **Comment:** *Custom Rule: Replace this raw index drop SQL with the shared migration helper for dropping indexes so database-compatibility behavior stays centralized. 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=380270d183ac62a4af308383e48a01e36e6320cbb5307bfc1ef33a27e04662b1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=380270d183ac62a4af308383e48a01e36e6320cbb5307bfc1ef33a27e04662b1&reaction=dislike'>👎</a> ########## superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.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. +"""Add deleted_at + partial unique slug index for soft-delete. + +Adds to the ``dashboards`` table: +- a nullable ``deleted_at`` column for soft-delete state +- an index ``ix_dashboards_deleted_at`` for the visibility-filter listener +- a partial unique index ``ix_dashboards_active_slug`` enforcing slug + uniqueness only among active (non-soft-deleted) rows + +Drops: +- the existing full unique constraint on ``slug`` (named + ``idx_unique_slug``, created in migration 1a48a5411020) + +The constraint change makes the ``slug`` field reusable after soft-delete: +soft-deleted rows no longer reserve their slug for the lifetime of the +row. ``RestoreDashboardCommand`` handles the reverse case (restoring a +dashboard whose slug has since been claimed by another active row) with +an explicit conflict error. See UPDATING.md for the user-facing change. + +Dialect support for the partial index: +- PostgreSQL: native ``WHERE deleted_at IS NULL`` partial index +- MySQL 8.0+: functional index over + ``(CASE WHEN deleted_at IS NULL THEN slug END)`` +- MySQL <8.0: keeps the original full unique constraint (documented + limitation; functional indexes are not supported on these versions) +- SQLite: keeps the original full unique constraint (column-level + ``UNIQUE`` cannot be dropped without recreating the table, which is + not worth the migration complexity for a test-only dialect). Tests + that need to verify the partial-index behaviour run only on + PostgreSQL and MySQL 8+. + +Revision ID: 9e1f3b8c4d2a +Revises: 78a40c08b4be +Create Date: 2026-05-08 12:05:00.000000 +""" + +from alembic import op +from sqlalchemy import Column, DateTime +from sqlalchemy.engine import Connection + +from superset.migrations.shared.utils import ( + add_columns, + create_index, + drop_columns, + drop_index, + table_has_index, +) + +# revision identifiers, used by Alembic. +revision = "9e1f3b8c4d2a" +down_revision = "78a40c08b4be" + +TABLE_NAME = "dashboards" +DELETED_AT_INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at" +PARTIAL_SLUG_INDEX_NAME = f"ix_{TABLE_NAME}_active_slug" +# The original full unique constraint on ``slug`` was created with an +# explicit name in migration 1a48a5411020 (2015-12-04). Same name on +# PostgreSQL (constraint) and MySQL (index). +LEGACY_SLUG_INDEX_NAME = "idx_unique_slug" + + +def _mysql_supports_functional_index(bind: Connection) -> bool: + """Return True iff the connected MySQL is 8.0.13+ (supports functional indexes). + + MySQL added functional key parts in 8.0.13; 8.0.0–8.0.12 reject the + ``(CASE WHEN deleted_at IS NULL THEN slug END)`` expression at index + creation time, so deployments on those patch releases must keep the + original full slug constraint. See + https://dev.mysql.com/doc/mysql/8.0/en/create-index.html for the + 8.0.13 minimum. + + Excludes MariaDB even at server version ``>= (10, x)`` because MariaDB + reports through the same ``server_version_info`` attribute but uses + different functional-index semantics around ``CASE`` expressions. + Uses SQLAlchemy's parsed ``server_version_info`` rather than ``SELECT + VERSION()`` to avoid an extra round-trip and brittle string parsing. + """ + if getattr(bind.dialect, "is_mariadb", False): + return False + return (bind.dialect.server_version_info or ()) >= (8, 0, 13) + + +def upgrade() -> None: + bind = op.get_bind() + _add_deleted_at_column() + _replace_slug_constraint_with_partial_index(bind) + + +def downgrade() -> None: + bind = op.get_bind() + _restore_slug_constraint(bind) + _drop_deleted_at_column() + + +def _add_deleted_at_column() -> None: + add_columns(TABLE_NAME, Column("deleted_at", DateTime(), nullable=True)) + create_index(TABLE_NAME, DELETED_AT_INDEX_NAME, ["deleted_at"]) + + +def _drop_deleted_at_column() -> None: + drop_index(TABLE_NAME, DELETED_AT_INDEX_NAME) + drop_columns(TABLE_NAME, "deleted_at") + + +def _replace_slug_constraint_with_partial_index(bind: Connection) -> None: + """Swap the full UNIQUE on ``slug`` for a partial index where supported. + + The original constraint is named ``idx_unique_slug`` from migration + 1a48a5411020 — same name on PostgreSQL (constraint) and MySQL (index). + + SQLite and MySQL <8.0 are no-ops here: they keep the original full + unique constraint. See the module docstring for the rationale. + """ + dialect = bind.dialect.name + if dialect == "postgresql": + op.execute( + f"ALTER TABLE {TABLE_NAME} " + f"DROP CONSTRAINT IF EXISTS {LEGACY_SLUG_INDEX_NAME}" + ) + # Some installations may have the unique enforced as a plain + # index rather than a constraint. Both DROPs are IF EXISTS, so + # whichever path applies cleans up. + op.execute(f"DROP INDEX IF EXISTS {LEGACY_SLUG_INDEX_NAME}") + op.execute( + f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} " + f"ON {TABLE_NAME} (slug) WHERE deleted_at IS NULL" + ) + elif dialect == "mysql" and _mysql_supports_functional_index(bind): + # Create the functional replacement BEFORE dropping the legacy unique + # index. MySQL autocommits each DDL statement (unlike PostgreSQL's + # transactional DDL above, where a failed CREATE rolls back the DROP), + # so a drop-then-create ordering would leave the table with no slug + # uniqueness if the CREATE failed. Creating first keeps the stricter + # existing uniqueness in place until the replacement is confirmed. + # Both statements are guarded by ``table_has_index`` because MySQL has + # no ``IF [NOT] EXISTS`` for indexes and DDL autocommits: an unguarded + # run on a table missing the legacy index (it was created inside + # ``try/except: pass`` in 2015's ``1a48a5411020``) would fail AFTER + # the partial index was committed, wedging the migration — the re-run + # would then die on the duplicate partial index. The guards make the + # migration re-runnable from any partial state. + if not table_has_index(TABLE_NAME, PARTIAL_SLUG_INDEX_NAME): + op.execute( + f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} " + f"ON {TABLE_NAME} ((CASE WHEN deleted_at IS NULL THEN slug END))" + ) + if table_has_index(TABLE_NAME, LEGACY_SLUG_INDEX_NAME): + op.execute(f"ALTER TABLE {TABLE_NAME} DROP INDEX {LEGACY_SLUG_INDEX_NAME}") + + +def _restore_slug_constraint(bind: Connection) -> None: + """Restore the full UNIQUE on ``slug`` from the partial index. + + Symmetric counterpart to ``_replace_slug_constraint_with_partial_index``. + No-op on dialects that never received the partial index. + + Pre-condition: each value of ``slug`` (other than NULL) must appear at + most once across the entire ``dashboards`` table. The partial-index + window allowed an active row and a soft-deleted row to share a slug; + rebuilding the full unique constraint will abort with a + ``UniqueViolation`` if any such pair still exists. Before downgrading, + hard-delete the soft-deleted duplicates (or rename one side of each + pair) so the constraint can be added cleanly. + """ + dialect = bind.dialect.name + if dialect == "postgresql": + op.execute(f"DROP INDEX IF EXISTS {PARTIAL_SLUG_INDEX_NAME}") Review Comment: **Suggestion:** Use the shared helper for dropping this index instead of executing raw SQL directly. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The shared `drop_index` helper exists specifically to centralize index removal behavior. This line uses direct SQL execution to drop an index in the PostgreSQL downgrade path, so the migration-helper rule is genuinely violated here. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2da2c4678565434a97683063ad228f9f&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=2da2c4678565434a97683063ad228f9f&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-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py **Line:** 182:182 **Comment:** *Custom Rule: Use the shared helper for dropping this index instead of executing raw SQL directly. 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=abb3e4a6711c348defe24c18d924d9dc308c24dafdaada5ae0a9d1bac8ef4ec7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=abb3e4a6711c348defe24c18d924d9dc308c24dafdaada5ae0a9d1bac8ef4ec7&reaction=dislike'>👎</a> ########## superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.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. +"""Add deleted_at + partial unique slug index for soft-delete. + +Adds to the ``dashboards`` table: +- a nullable ``deleted_at`` column for soft-delete state +- an index ``ix_dashboards_deleted_at`` for the visibility-filter listener +- a partial unique index ``ix_dashboards_active_slug`` enforcing slug + uniqueness only among active (non-soft-deleted) rows + +Drops: +- the existing full unique constraint on ``slug`` (named + ``idx_unique_slug``, created in migration 1a48a5411020) + +The constraint change makes the ``slug`` field reusable after soft-delete: +soft-deleted rows no longer reserve their slug for the lifetime of the +row. ``RestoreDashboardCommand`` handles the reverse case (restoring a +dashboard whose slug has since been claimed by another active row) with +an explicit conflict error. See UPDATING.md for the user-facing change. + +Dialect support for the partial index: +- PostgreSQL: native ``WHERE deleted_at IS NULL`` partial index +- MySQL 8.0+: functional index over + ``(CASE WHEN deleted_at IS NULL THEN slug END)`` +- MySQL <8.0: keeps the original full unique constraint (documented + limitation; functional indexes are not supported on these versions) +- SQLite: keeps the original full unique constraint (column-level + ``UNIQUE`` cannot be dropped without recreating the table, which is + not worth the migration complexity for a test-only dialect). Tests + that need to verify the partial-index behaviour run only on + PostgreSQL and MySQL 8+. + +Revision ID: 9e1f3b8c4d2a +Revises: 78a40c08b4be +Create Date: 2026-05-08 12:05:00.000000 +""" + +from alembic import op +from sqlalchemy import Column, DateTime +from sqlalchemy.engine import Connection + +from superset.migrations.shared.utils import ( + add_columns, + create_index, + drop_columns, + drop_index, + table_has_index, +) + +# revision identifiers, used by Alembic. +revision = "9e1f3b8c4d2a" +down_revision = "78a40c08b4be" + +TABLE_NAME = "dashboards" +DELETED_AT_INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at" +PARTIAL_SLUG_INDEX_NAME = f"ix_{TABLE_NAME}_active_slug" +# The original full unique constraint on ``slug`` was created with an +# explicit name in migration 1a48a5411020 (2015-12-04). Same name on +# PostgreSQL (constraint) and MySQL (index). +LEGACY_SLUG_INDEX_NAME = "idx_unique_slug" + + +def _mysql_supports_functional_index(bind: Connection) -> bool: + """Return True iff the connected MySQL is 8.0.13+ (supports functional indexes). + + MySQL added functional key parts in 8.0.13; 8.0.0–8.0.12 reject the + ``(CASE WHEN deleted_at IS NULL THEN slug END)`` expression at index + creation time, so deployments on those patch releases must keep the + original full slug constraint. See + https://dev.mysql.com/doc/mysql/8.0/en/create-index.html for the + 8.0.13 minimum. + + Excludes MariaDB even at server version ``>= (10, x)`` because MariaDB + reports through the same ``server_version_info`` attribute but uses + different functional-index semantics around ``CASE`` expressions. + Uses SQLAlchemy's parsed ``server_version_info`` rather than ``SELECT + VERSION()`` to avoid an extra round-trip and brittle string parsing. + """ + if getattr(bind.dialect, "is_mariadb", False): + return False + return (bind.dialect.server_version_info or ()) >= (8, 0, 13) + + +def upgrade() -> None: + bind = op.get_bind() + _add_deleted_at_column() + _replace_slug_constraint_with_partial_index(bind) + + +def downgrade() -> None: + bind = op.get_bind() + _restore_slug_constraint(bind) + _drop_deleted_at_column() + + +def _add_deleted_at_column() -> None: + add_columns(TABLE_NAME, Column("deleted_at", DateTime(), nullable=True)) + create_index(TABLE_NAME, DELETED_AT_INDEX_NAME, ["deleted_at"]) + + +def _drop_deleted_at_column() -> None: + drop_index(TABLE_NAME, DELETED_AT_INDEX_NAME) + drop_columns(TABLE_NAME, "deleted_at") + + +def _replace_slug_constraint_with_partial_index(bind: Connection) -> None: + """Swap the full UNIQUE on ``slug`` for a partial index where supported. + + The original constraint is named ``idx_unique_slug`` from migration + 1a48a5411020 — same name on PostgreSQL (constraint) and MySQL (index). + + SQLite and MySQL <8.0 are no-ops here: they keep the original full + unique constraint. See the module docstring for the rationale. + """ + dialect = bind.dialect.name + if dialect == "postgresql": + op.execute( + f"ALTER TABLE {TABLE_NAME} " + f"DROP CONSTRAINT IF EXISTS {LEGACY_SLUG_INDEX_NAME}" + ) + # Some installations may have the unique enforced as a plain + # index rather than a constraint. Both DROPs are IF EXISTS, so + # whichever path applies cleans up. + op.execute(f"DROP INDEX IF EXISTS {LEGACY_SLUG_INDEX_NAME}") + op.execute( + f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} " + f"ON {TABLE_NAME} (slug) WHERE deleted_at IS NULL" + ) + elif dialect == "mysql" and _mysql_supports_functional_index(bind): + # Create the functional replacement BEFORE dropping the legacy unique + # index. MySQL autocommits each DDL statement (unlike PostgreSQL's + # transactional DDL above, where a failed CREATE rolls back the DROP), + # so a drop-then-create ordering would leave the table with no slug + # uniqueness if the CREATE failed. Creating first keeps the stricter + # existing uniqueness in place until the replacement is confirmed. + # Both statements are guarded by ``table_has_index`` because MySQL has + # no ``IF [NOT] EXISTS`` for indexes and DDL autocommits: an unguarded + # run on a table missing the legacy index (it was created inside + # ``try/except: pass`` in 2015's ``1a48a5411020``) would fail AFTER + # the partial index was committed, wedging the migration — the re-run + # would then die on the duplicate partial index. The guards make the + # migration re-runnable from any partial state. + if not table_has_index(TABLE_NAME, PARTIAL_SLUG_INDEX_NAME): + op.execute( + f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} " + f"ON {TABLE_NAME} ((CASE WHEN deleted_at IS NULL THEN slug END))" + ) + if table_has_index(TABLE_NAME, LEGACY_SLUG_INDEX_NAME): + op.execute(f"ALTER TABLE {TABLE_NAME} DROP INDEX {LEGACY_SLUG_INDEX_NAME}") Review Comment: **Suggestion:** Use the shared `drop_index` helper instead of raw MySQL DDL for removing the legacy index. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The migration helper module provides `drop_index` for this exact operation. Here the MySQL branch uses raw DDL to drop an index instead of the shared helper, which is a real instance of the migration-helper rule violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d166afe7fd8940a6823c1e9025d42df7&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=d166afe7fd8940a6823c1e9025d42df7&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-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py **Line:** 163:163 **Comment:** *Custom Rule: Use the shared `drop_index` helper instead of raw MySQL DDL for removing the legacy index. 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=85ce7e44793ac3d37a4bfae88ab8dc82bdd216a84100f4ba5bfcc9c9edd18cb3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=85ce7e44793ac3d37a4bfae88ab8dc82bdd216a84100f4ba5bfcc9c9edd18cb3&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]
