codeant-ai-for-open-source[bot] commented on code in PR #41994: URL: https://github.com/apache/superset/pull/41994#discussion_r3593534708
########## 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"), +] + + +def _redefine_fk( + table: str, + local_col: str, + on_delete: str | None, +) -> None: + """DROP + RECREATE a single foreign key filtered by ``local_col``. + + Unlike ``superset.migrations.shared.constraints.redefine`` (which + matches only by referred columns and would collide when a table has + multiple FKs to the same target), this helper looks up the existing + FK whose ``constrained_columns`` includes ``local_col``, drops that + specific constraint, and creates a replacement with the requested + ``ON DELETE`` clause. + + Silent no-op when the table or column does not exist on the target + database — makes the migration safe to re-run and safe on installs + that don't have every optional table. + """ + bind = op.get_bind() + insp = Inspector.from_engine(bind) + + if table not in insp.get_table_names(): + return + + column_names = {c["name"] for c in insp.get_columns(table)} + if local_col not in column_names: + return + + existing_name: str | None = None + for fk in insp.get_foreign_keys(table): + if ( + fk["referred_table"] == "ab_user" + and fk["referred_columns"] == ["id"] + and fk["constrained_columns"] == [local_col] + ): + existing_name = fk["name"] + break + + conv = {"fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s"} + with op.batch_alter_table(table, naming_convention=conv) as batch_op: + if existing_name: + batch_op.drop_constraint(existing_name, type_="foreignkey") + batch_op.create_foreign_key( + constraint_name=f"fk_{table}_{local_col}_ab_user", + referent_table="ab_user", + local_cols=[local_col], + remote_cols=["id"], + ondelete=on_delete, + ) Review Comment: **Suggestion:** Use shared migration helpers from `superset.migrations.shared.utils` for foreign-key drop/create behavior instead of direct batch operations. [custom_rule] **Severity Level:** Major ⚠️ <details> <summary><b>Why it matters? ⭐ </b></summary> This migration performs direct Alembic batch alter/drop/create foreign-key operations instead of using helpers from `superset.migrations.shared.utils`, which matches the custom migration rule. </details> <details> <summary><b>Rule source 📖 </b></summary> .github/copilot-instructions.md (line 294) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f64094a11bb24c1eb4e6e1a5c1a75734&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=f64094a11bb24c1eb4e6e1a5c1a75734&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:** 125:134 **Comment:** *Custom Rule: Use shared migration helpers from `superset.migrations.shared.utils` for foreign-key drop/create behavior instead of direct batch operations. 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=eb23ce9c61eae977750fa70b4c3dfc4b6c36dd2497f17d74d45a98b764106161&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=eb23ce9c61eae977750fa70b4c3dfc4b6c36dd2497f17d74d45a98b764106161&reaction=dislike'>👎</a> ########## superset/models/user_attributes.py: ########## @@ -46,7 +46,7 @@ class UserAttribute(Model, AuditMixinNullable): # session-invalidation upsert depends on this for race safety. __table_args__ = (UniqueConstraint("user_id", name="uq_user_attribute_user_id"),) id = Column(Integer, primary_key=True) - user_id = Column(Integer, ForeignKey("ab_user.id")) + user_id = Column(Integer, ForeignKey("ab_user.id", ondelete="CASCADE")) Review Comment: **Suggestion:** Add an explicit type annotation to this newly introduced model attribute so the new code complies with the type-hint requirement. [custom_rule] **Severity Level:** Minor 🧹 <details> <summary><b>Why it matters? ⭐ </b></summary> The added model field is an untyped Python attribute in a modified file, and the stated rule requires type hints for new or modified variables that can be annotated. This line therefore matches the rule violation. </details> <details> <summary><b>Rule source 📖 </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2599f3d3e4dc40ccab7e872b0f3a438b&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=2599f3d3e4dc40ccab7e872b0f3a438b&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/user_attributes.py **Line:** 49:49 **Comment:** *Custom Rule: Add an explicit type annotation to this newly introduced model attribute so the new code complies with 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%2F41994&comment_hash=e4bcadda6a57d8d27caa38ea506da13ea5f59c3270f3ea79e6bc48c3f19c84af&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=e4bcadda6a57d8d27caa38ea506da13ea5f59c3270f3ea79e6bc48c3f19c84af&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"), +] + + +def _redefine_fk( + table: str, + local_col: str, + on_delete: str | None, +) -> None: + """DROP + RECREATE a single foreign key filtered by ``local_col``. + + Unlike ``superset.migrations.shared.constraints.redefine`` (which + matches only by referred columns and would collide when a table has + multiple FKs to the same target), this helper looks up the existing + FK whose ``constrained_columns`` includes ``local_col``, drops that + specific constraint, and creates a replacement with the requested + ``ON DELETE`` clause. + + Silent no-op when the table or column does not exist on the target + database — makes the migration safe to re-run and safe on installs + that don't have every optional table. + """ + bind = op.get_bind() + insp = Inspector.from_engine(bind) + + if table not in insp.get_table_names(): + return + + column_names = {c["name"] for c in insp.get_columns(table)} + if local_col not in column_names: + return + + existing_name: str | None = None + for fk in insp.get_foreign_keys(table): + if ( + fk["referred_table"] == "ab_user" + and fk["referred_columns"] == ["id"] + and fk["constrained_columns"] == [local_col] + ): + existing_name = fk["name"] + break + + conv = {"fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s"} + with op.batch_alter_table(table, naming_convention=conv) as batch_op: + if existing_name: + batch_op.drop_constraint(existing_name, type_="foreignkey") + batch_op.create_foreign_key( + constraint_name=f"fk_{table}_{local_col}_ab_user", + referent_table="ab_user", + local_cols=[local_col], + remote_cols=["id"], + ondelete=on_delete, + ) + + +def upgrade(): Review Comment: **Suggestion:** Add an explicit return type annotation to this migration function. [custom_rule] **Severity Level:** Minor 🧹 <details> <summary><b>Why it matters? ⭐ </b></summary> The rule requires Python functions to include type hints where applicable. `upgrade()` is a new function with no return annotation, so this is a real type-hint omission. </details> <details> <summary><b>Rule source 📖 </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=65d5d80042174388a774d2e61ffb79a9&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=65d5d80042174388a774d2e61ffb79a9&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:** 137:137 **Comment:** *Custom Rule: Add an explicit return type annotation to this migration function. 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=d3d7d59a37eb2295011fc3b9d3d5e7e4d4da7f485d3a4cdff3d1edc6692f18b3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=d3d7d59a37eb2295011fc3b9d3d5e7e4d4da7f485d3a4cdff3d1edc6692f18b3&reaction=dislike'>👎</a> ########## tests/unit_tests/models/test_user_delete_cascade.py: ########## @@ -0,0 +1,82 @@ +# 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. +"""Regression tests for the reduced-scope portion of #38629. + +Deleting a user via ``SecurityManager.delete_user()`` raised +``IntegrityError`` on PostgreSQL / MySQL / MariaDB whenever the user had +rows in tables that referenced ``ab_user.id`` via a foreign key with no +``ON DELETE`` clause. This PR fixes it for the narrow set of tables +where the semantics are unambiguous — pure audit trails (``SET NULL``) +and owner-junction tables (``CASCADE``). + +User-owned artifacts (``saved_query``, ``dashboards``, all +``AuditMixinNullable`` tables) are deferred to a Superset Improvement +Proposal because they should be *reassigned* to an admin rather than +silently orphaned. This test pins the reduced-scope invariant so a +future contributor cannot regress the specific FKs the fix targets. +""" + +from __future__ import annotations + +import pytest + + +def _iter_all_fks_to_ab_user() -> list[tuple[str, str, str | None]]: + """Introspect the Superset metadata and return every foreign key + that targets ``ab_user.id`` as ``(table, column, ondelete)``.""" + from superset import db # noqa: F401 (ensures app is initialized) + from superset.models.helpers import Model + + fks: list[tuple[str, str, str | None]] = [] + for table in Model.metadata.tables.values(): + for column in table.columns: + for fk in column.foreign_keys: + if fk.target_fullname == "ab_user.id": + fks.append((table.name, column.name, fk.ondelete)) + return fks + + [email protected]( + "table,column,expected", + [ + # Pure audit trails: row survives with FK cleared + ("logs", "user_id", "SET NULL"), + ("key_value", "created_by_fk", "SET NULL"), + ("key_value", "changed_by_fk", "SET NULL"), + # Owner-junction: row has no meaning without the user + ("favstar", "user_id", "CASCADE"), + ("user_attribute", "user_id", "CASCADE"), + ("tab_state", "user_id", "CASCADE"), + ("user_favorite_tag", "user_id", "CASCADE"), + ], +) +def test_targeted_fk_uses_expected_ondelete( + table: str, column: str, expected: str +) -> None: + """Each targeted FK to ``ab_user.id`` must declare the semantically + correct ``ondelete`` behavior. See #38629 for the audit vs. + ownership rationale. + """ + matches = [ + ondelete + for tbl, col, ondelete in _iter_all_fks_to_ab_user() + if tbl == table and col == column + ] Review Comment: **Suggestion:** Add an explicit type annotation for this local collection variable to comply with the type-hint requirement for relevant variables. [custom_rule] **Severity Level:** Minor 🧹 <details> <summary><b>Why it matters? ⭐ </b></summary> The local variable `matches` is newly introduced without an explicit type annotation, and it is a relevant variable that can be annotated under the Python type-hint rule. This is a real violation in the added code. </details> <details> <summary><b>Rule source 📖 </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2da6a7028124fcc803183add6001b69&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=d2da6a7028124fcc803183add6001b69&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/models/test_user_delete_cascade.py **Line:** 74:78 **Comment:** *Custom Rule: Add an explicit type annotation for this local collection 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%2F41994&comment_hash=db84058705101854ec8845f4e9bcee21d8cf5aaaf9ef98cc1b9e44ec65225d7e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=db84058705101854ec8845f4e9bcee21d8cf5aaaf9ef98cc1b9e44ec65225d7e&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]
