codeant-ai-for-open-source[bot] commented on code in PR #41994: URL: https://github.com/apache/superset/pull/41994#discussion_r3593582070
########## superset/migrations/versions/2026-07-13_11-00_5f2a8b9c4d1e_add_ondelete_for_ab_user_fks.py: ########## @@ -0,0 +1,147 @@ +# 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 ON DELETE behavior for a targeted set of ab_user foreign keys + +Revision ID: 5f2a8b9c4d1e +Revises: 8f3a1b2c4d5e +Create Date: 2026-07-13 11:00:00.000000 + +Partial fix for #38629. Deleting a user via Settings → List Users raises +IntegrityError on PostgreSQL / MySQL / MariaDB because tables that +reference ``ab_user.id`` have no ``ON DELETE`` behavior on their foreign +key constraint. Scope is intentionally narrow — only the tables where +the correct semantics are unambiguous: + +- **Pure audit trails** (``SET NULL``) — the row must survive when its + author is deleted; the audit reference is cleared. + + - ``logs.user_id`` + - ``key_value.created_by_fk`` / ``key_value.changed_by_fk`` + +- **Owner-junction tables** (``CASCADE``) — the row has no meaning + without the user. + + - ``favstar.user_id`` + - ``user_attribute.user_id`` + - ``tab_state.user_id`` + - ``user_favorite_tag.user_id`` + +**Deliberately deferred to a SIP** (per review on #41994): tables like +``saved_query``, ``query``, ``slices.last_saved_by_fk``, and everything +reached via ``AuditMixinNullable`` (``dashboards``, ``slices``, ``dbs``, +``tables``, ``report_schedule``, ...) — those represent user-owned +artifacts that should be *reassigned* to an admin rather than orphaned +with ``NULL``. The right long-term flow needs community design work. + +This partial fix still unblocks the two user-reported failures on the +issue (``logs_ibfk_1`` and ``key_value_created_by_fk_fkey``) while the +SIP determines the reassignment semantics for the rest. + +Same pattern as ``6d05b0a70c89`` (2023, owners refs) and +``32bf93dfe2a4`` (2025, FAB tables). Uses a local helper that filters +by ``local_cols`` because the shared ``redefine()`` helper matches only +by referred columns and would collide on tables with multiple FKs to +``ab_user.id`` (``created_by_fk`` + ``changed_by_fk``). +""" + +from alembic import op +from sqlalchemy.engine.reflection import Inspector + +# revision identifiers, used by Alembic. +revision = "5f2a8b9c4d1e" +down_revision = "8f3a1b2c4d5e" Review Comment: **Suggestion:** This revision is chained to `8f3a1b2c4d5e`, but there is already another migration (`d24e6b0a9c7f`) on the same parent, which creates a new parallel Alembic head instead of continuing the current branch. That will trigger migration-head conflicts in environments expecting a single head. Rebase this migration to the latest head (or add a merge migration if branching is intentional). [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Alembic upgrade detects multiple heads, aborts migration. - ❌ Migration-head CI check fails blocking deployments. - ⚠️ Developers cannot apply latest schema locally. - ⚠️ Confusing migration history complicates future merges. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect the shadow-index migration at `superset/migrations/versions/2026-06-03_12-00_8f3a1b2c4d5e_shadow_live_row_indexes.py:60-72`, which declares `revision = "8f3a1b2c4d5e"` and `down_revision = "56cd24c07170"`, making it a single head on top of `56cd24c07170_add_versioning_tables`. 2. Inspect the AG Grid cleanup migration at `superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:28-44`, which declares `revision = "d24e6b0a9c7f"` and `down_revision = "8f3a1b2c4d5e"`, establishing one branch that continues from `8f3a1b2c4d5e`. 3. Inspect this new migration at `superset/migrations/versions/2026-07-13_11-00_5f2a8b9c4d1e_add_ondelete_for_ab_user_fks.py:19-21` and `64-66`, which declares `revision = "5f2a8b9c4d1e"` and the same `down_revision = "8f3a1b2c4d5e"`, creating a second child of `8f3a1b2c4d5e` instead of chaining off `d24e6b0a9c7f`. 4. Verify via LS of `superset/migrations/versions` and Grep for `d24e6b0a9c7f` and `5f2a8b9c4d1e` that no merge migration exists yet tying these branches together; with two separate revisions both pointing at `down_revision = "8f3a1b2c4d5e"`, Alembic will detect multiple heads (`d24e6b0a9c7f` and `5f2a8b9c4d1e`) and `superset db upgrade` / Alembic head checks will fail with a “Multiple heads present” migration conflict until the DAG is rebased or merged. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7e7a92399fdb4890aa0ce43b3a892213&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=7e7a92399fdb4890aa0ce43b3a892213&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-07-13_11-00_5f2a8b9c4d1e_add_ondelete_for_ab_user_fks.py **Line:** 66:66 **Comment:** *Logic Error: This revision is chained to `8f3a1b2c4d5e`, but there is already another migration (`d24e6b0a9c7f`) on the same parent, which creates a new parallel Alembic head instead of continuing the current branch. That will trigger migration-head conflicts in environments expecting a single head. Rebase this migration to the latest head (or add a merge migration if branching is intentional). 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%2F41994&comment_hash=9950f2a759d979585c48f89a7f637eadc5560009c0702fd6481848584f0c1764&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=9950f2a759d979585c48f89a7f637eadc5560009c0702fd6481848584f0c1764&reaction=dislike'>👎</a> ########## superset/migrations/versions/2026-07-13_11-00_5f2a8b9c4d1e_add_ondelete_for_ab_user_fks.py: ########## @@ -0,0 +1,147 @@ +# 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 ON DELETE behavior for a targeted set of ab_user foreign keys + +Revision ID: 5f2a8b9c4d1e +Revises: 8f3a1b2c4d5e +Create Date: 2026-07-13 11:00:00.000000 + +Partial fix for #38629. Deleting a user via Settings → List Users raises +IntegrityError on PostgreSQL / MySQL / MariaDB because tables that +reference ``ab_user.id`` have no ``ON DELETE`` behavior on their foreign +key constraint. Scope is intentionally narrow — only the tables where +the correct semantics are unambiguous: + +- **Pure audit trails** (``SET NULL``) — the row must survive when its + author is deleted; the audit reference is cleared. + + - ``logs.user_id`` + - ``key_value.created_by_fk`` / ``key_value.changed_by_fk`` + +- **Owner-junction tables** (``CASCADE``) — the row has no meaning + without the user. + + - ``favstar.user_id`` + - ``user_attribute.user_id`` + - ``tab_state.user_id`` + - ``user_favorite_tag.user_id`` + +**Deliberately deferred to a SIP** (per review on #41994): tables like +``saved_query``, ``query``, ``slices.last_saved_by_fk``, and everything +reached via ``AuditMixinNullable`` (``dashboards``, ``slices``, ``dbs``, +``tables``, ``report_schedule``, ...) — those represent user-owned +artifacts that should be *reassigned* to an admin rather than orphaned +with ``NULL``. The right long-term flow needs community design work. + +This partial fix still unblocks the two user-reported failures on the +issue (``logs_ibfk_1`` and ``key_value_created_by_fk_fkey``) while the +SIP determines the reassignment semantics for the rest. + +Same pattern as ``6d05b0a70c89`` (2023, owners refs) and +``32bf93dfe2a4`` (2025, FAB tables). Uses a local helper that filters +by ``local_cols`` because the shared ``redefine()`` helper matches only +by referred columns and would collide on tables with multiple FKs to +``ab_user.id`` (``created_by_fk`` + ``changed_by_fk``). +""" + +from alembic import op +from sqlalchemy.engine.reflection import Inspector + +# revision identifiers, used by Alembic. +revision = "5f2a8b9c4d1e" +down_revision = "8f3a1b2c4d5e" + + +# (table, column) pairs. Semantic split: +# SET NULL — pure audit trail; row survives, reference cleared +# CASCADE — row has no meaning without the referenced user +_FKS_SET_NULL: list[tuple[str, str]] = [ + ("logs", "user_id"), + ("key_value", "created_by_fk"), + ("key_value", "changed_by_fk"), +] + +_FKS_CASCADE: list[tuple[str, str]] = [ + ("favstar", "user_id"), + ("user_attribute", "user_id"), + ("tab_state", "user_id"), + ("user_favorite_tag", "user_id"), +] Review Comment: **Suggestion:** This migration only redefines a narrow subset of `ab_user` foreign keys, but `query.user_id` and `saved_query.user_id` (and other non-covered user references) still have no `ON DELETE` behavior in models. Deleting users who have SQL Lab history will still hit FK `IntegrityError`, so this does not fully fix user deletion failures. Extend the migration scope to include the remaining user FKs that currently block user deletion or add explicit reassignment/deletion logic before deleting the user. [incomplete implementation] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Admin cannot delete users with SQL Lab history. - ❌ IntegrityError surfaces in List Users delete flow. - ⚠️ Partial fix; other user FKs still restrictive. - ⚠️ Affected on PostgreSQL, MySQL, MariaDB deployments. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create a user and run SQL Lab queries as that user so rows are inserted into the `query` table defined in `superset/migrations/versions/2016-07-25_17-48_ad82a75afd82_add_query_model.py:35-63` (note the `sa.ForeignKeyConstraint(["user_id"], ["ab_user.id"])` with no ON DELETE clause) and persisted via the `Query` ORM model at `superset/models/sql_lab.py:116-137` where `user_id = Column(Integer, ForeignKey("ab_user.id"), nullable=True)`. 2. Optionally save one of these queries so a row is created in the `saved_query` table defined in `superset/migrations/versions/2017-03-29_15-04_2fcdcb35e487_saved_queries.py:33-51`, which also declares `sa.ForeignKeyConstraint(["user_id"], ["ab_user.id"])` (again with no ON DELETE behavior). 3. From the Superset UI, navigate to the List Users page wired via `UsersListView` in `superset/views/users_list.py:7-14` and registered in the menu at `superset/initialization/__init__.py:378-387` (labelled “List Users” under the Security category). 4. Attempt to delete the user; Flask-AppBuilder’s `UserDBModelView` (referenced in `superset/security/manager.py:1104-1112`) issues a DELETE on the `ab_user` row, and because the new migration at `superset/migrations/versions/2026-07-13_11-00_5f2a8b9c4d1e_add_ondelete_for_ab_user_fks.py:69-83` only redefines FKs for `logs`, `key_value`, `favstar`, `user_attribute`, `tab_state`, and `user_favorite_tag` (and its docstring at lines 43-48 explicitly defers `saved_query` and `query`), the restrictive `query.user_id` / `saved_query.user_id` foreign keys still block deletion and the database raises an IntegrityError. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=017fcf74063e439ea3f2acaad1a617b6&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=017fcf74063e439ea3f2acaad1a617b6&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-07-13_11-00_5f2a8b9c4d1e_add_ondelete_for_ab_user_fks.py **Line:** 72:83 **Comment:** *Incomplete Implementation: This migration only redefines a narrow subset of `ab_user` foreign keys, but `query.user_id` and `saved_query.user_id` (and other non-covered user references) still have no `ON DELETE` behavior in models. Deleting users who have SQL Lab history will still hit FK `IntegrityError`, so this does not fully fix user deletion failures. Extend the migration scope to include the remaining user FKs that currently block user deletion or add explicit reassignment/deletion logic before deleting the user. 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%2F41994&comment_hash=dd341f7ec379f5ce1a548a90d7f4e1aa18cad1e1ce4ee0a9f8a4b5cd5238d950&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=dd341f7ec379f5ce1a548a90d7f4e1aa18cad1e1ce4ee0a9f8a4b5cd5238d950&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]
