codeant-ai-for-open-source[bot] commented on code in PR #40129: URL: https://github.com/apache/superset/pull/40129#discussion_r3492980949
########## 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: **Suggestion:** The migration is anchored to an unrelated newer revision, which creates an unintended dependency chain for chart soft-delete and can produce an unexpected Alembic branch/head layout. Repoint `down_revision` to the intended soft-delete rollout base revision so this migration composes correctly with sibling entity rollouts and doesn’t force unrelated migrations first. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Chart soft-delete migration depends on SSH tunnel migration. - ⚠️ Alembic upgrade path includes unnecessary SSH host key change. - ⚠️ Soft-delete entity rollouts cannot share a common base. - ⚠️ Future migration rebases risk extra heads and merges. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect the chart soft-delete migration at `superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py`. At lines 23–25 and 37–39, the docstring and Alembic identifiers show `Revision ID: 7c4a8d09ca37` and `down_revision = "78a40c08b4be"`, meaning this migration depends on revision `78a40c08b4be`. 2. Inspect the referenced parent migration at `superset/migrations/versions/2026-06-03_10-00_78a40c08b4be_add_server_host_key_to_ssh_tunnels.py`. At lines 17–27 and 35–37, it is clearly an SSH tunnel feature migration (`add server_host_key to ssh_tunnels`) with `revision = "78a40c08b4be"` and `down_revision = "b7c9d1e2f3a4"`, unrelated to charts or soft-delete infrastructure. 3. Inspect the intended rollout base on the existing migration chain at `superset/migrations/versions/2026-04-22_19-00_31dae2559c05_replay_user_favorite_tag_migration.py` and its parent `2025-11-04_11-26_33d7e0e21daa_add_semantic_layers_and_views.py`. These files (lines 35–46 and 19–41 respectively) show a separate lineage `ce6bd21901ab → 33d7e0e21daa → 31dae2559c05` that the PR description states is the anchor for the soft-delete rollout, but the chart migration is not attached to this chain. 4. Run `superset db upgrade` (or Alembic `upgrade head`) in an environment where migrations are applied up to `31dae2559c05` but not yet to `b7c9d1e2f3a4`/`78a40c08b4be`. Alembic will resolve the graph such that `7c4a8d09ca37_add_deleted_at_to_slices` cannot be applied until the unrelated SSH tunnel migrations `b7c9d1e2f3a4_add_password_must_change.py` and `78a40c08b4be_add_server_host_key_to_ssh_tunnels.py` are also applied, coupling chart soft-delete rollout to SSH tunnel changes and preventing the chart migration from sharing the intended common base with sibling soft-delete migrations. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2a8ae812e4ad4503b505e859bdf4fc6b&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=2a8ae812e4ad4503b505e859bdf4fc6b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py **Line:** 39:39 **Comment:** *Logic Error: The migration is anchored to an unrelated newer revision, which creates an unintended dependency chain for chart soft-delete and can produce an unexpected Alembic branch/head layout. Repoint `down_revision` to the intended soft-delete rollout base revision so this migration composes correctly with sibling entity rollouts and doesn’t force unrelated migrations first. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=b4d5c3ab3d0fbbe9e2e31cb07f4d6802dd0b0d38cb510f557a5636a412b7780b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=b4d5c3ab3d0fbbe9e2e31cb07f4d6802dd0b0d38cb510f557a5636a412b7780b&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]
