codeant-ai-for-open-source[bot] commented on code in PR #41994:
URL: https://github.com/apache/superset/pull/41994#discussion_r3569061876
##########
superset/key_value/models.py:
##########
@@ -34,10 +34,14 @@ class KeyValueEntry(CoreKeyValue, AuditMixinNullable,
ImportExportMixin):
resource = Column(String(32), nullable=False)
value = Column(LargeBinary(length=VALUE_MAX_SIZE), nullable=False)
created_on = Column(DateTime, nullable=True)
- created_by_fk = Column(Integer, ForeignKey("ab_user.id"), nullable=True)
+ created_by_fk = Column(
+ Integer, ForeignKey("ab_user.id", ondelete="SET NULL"), nullable=True
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this new model attribute
so the foreign-key field is typed in addition to being assigned a SQLAlchemy
column. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This newly added SQLAlchemy model attribute is a relevant variable that can
be annotated, but it is declared without a Python type hint. That matches the
rule requiring type hints on new or modified Python code where applicable.
</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=c7d3073b7e3e488188442627dc55beef&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=c7d3073b7e3e488188442627dc55beef&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/key_value/models.py
**Line:** 37:39
**Comment:**
*Custom Rule: Add an explicit type annotation for this new model
attribute so the foreign-key field is typed in addition to being assigned a
SQLAlchemy column.
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=201044f715c696acb17c372d42114c2804c8d32d0da673e1ac4a0f66e5d1a0fd&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=201044f715c696acb17c372d42114c2804c8d32d0da673e1ac4a0f66e5d1a0fd&reaction=dislike'>๐</a>
##########
superset/models/core.py:
##########
@@ -1547,7 +1547,7 @@ class Log(Model): # pylint:
disable=too-few-public-methods
id = Column(Integer, primary_key=True)
action = Column(String(512))
- user_id = Column(Integer, ForeignKey("ab_user.id"))
+ user_id = Column(Integer, ForeignKey("ab_user.id", ondelete="SET NULL"))
Review Comment:
**Suggestion:** Add an explicit type annotation for this model attribute so
the newly introduced field is type-hinted consistently with the typing rule.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a newly added ORM class attribute in modified Python code, and it
lacks an explicit type annotation even though the custom rule requires type
hints on relevant variables that can be annotated.
</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=67aed3fb970546c7941686d04d4b1960&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=67aed3fb970546c7941686d04d4b1960&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/core.py
**Line:** 1550:1550
**Comment:**
*Custom Rule: Add an explicit type annotation for this model attribute
so the newly introduced field is type-hinted consistently with the typing rule.
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=8fda98f3ce31a367db6c5b0ad4088745a970f15cc68a25da2e4c5cdc956a50ff&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=8fda98f3ce31a367db6c5b0ad4088745a970f15cc68a25da2e4c5cdc956a50ff&reaction=dislike'>๐</a>
##########
superset/models/core.py:
##########
@@ -1568,7 +1568,7 @@ class FavStar(UUIDMixin, Model):
__tablename__ = "favstar"
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 for this newly added ORM
field to comply with the required type-hinting rule for relevant variables.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is also a newly added ORM class attribute in modified Python code, and
it has no explicit type annotation. That matches the type-hinting rule
violation described by the suggestion.
</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=eb748de7b3ce4dfc83cf4038b0a24910&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=eb748de7b3ce4dfc83cf4038b0a24910&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/core.py
**Line:** 1571:1571
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly added ORM
field to comply with the required type-hinting rule 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=d668f603f9d1ec112ce0cdeacdb6af6565e071fd08787ecf70132fc42040dfca&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=d668f603f9d1ec112ce0cdeacdb6af6565e071fd08787ecf70132fc42040dfca&reaction=dislike'>๐</a>
##########
tests/unit_tests/models/test_user_delete_cascade.py:
##########
@@ -0,0 +1,140 @@
+# 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 #38629.
+
+Deleting a user via ``SecurityManager.delete_user()`` (which issues a
+bare ``DELETE FROM ab_user``) raised ``IntegrityError`` on
+PostgreSQL / MySQL / MariaDB whenever the user had dependent rows in
+tables that referenced ``ab_user.id`` via a foreign key with no
+``ON DELETE`` clause. This module pins two invariants at the model
+metadata layer:
+
+1. Every ``ForeignKey`` targeting ``ab_user.id`` declares an ``ondelete``
+ clause. A future contributor cannot add a new FK to ``ab_user`` and
+ silently reintroduce the bug.
+2. The ``AuditMixinNullable`` mixin's ``created_by_fk`` / ``changed_by_fk``
+ columns use ``ON DELETE SET NULL`` โ audit trails survive user
+ deletion with the reference cleared, rather than blocking the delete.
+"""
+
+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``.
+
+ Returns tuples of ``(table_name, column_name, ondelete)`` โ the
+ ``ondelete`` is the string captured by the ``ForeignKey`` constructor
+ (``"SET NULL"``, ``"CASCADE"``, or ``None``).
+ """
+ # Importing this module pulls in the whole Superset model graph, which
+ # in turn populates ``Model.metadata`` with every mapped table. Kept
+ # inside the function so that collection failures during pytest
+ # collection surface as test failures rather than import errors.
+ 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:
+ # ``fk.target_fullname`` looks like ``"ab_user.id"``
+ if fk.target_fullname == "ab_user.id":
+ fks.append((table.name, column.name, fk.ondelete))
+ return fks
+
+
+def test_every_superset_fk_to_ab_user_declares_ondelete() -> None:
+ """Every Superset-owned foreign key targeting ``ab_user.id`` must
+ declare an ``ondelete`` behavior so deleting a user does not blow up
+ with ``IntegrityError``. See #38629.
+
+ FAB-owned tables (``ab_user`` self-references, ``ab_user_role``,
+ ``ab_permission_view_role``, etc.) are excluded from this
+ model-level assertion because Superset does not own their model
+ declarations. The companion Alembic migration fixes them at the
+ database schema level via ``redefine()``.
+ """
+ missing = [
+ (table, col)
+ for table, col, ondelete in _iter_all_fks_to_ab_user()
+ if not ondelete and not table.startswith("ab_")
+ ]
+ assert not missing, (
+ f"Found {len(missing)} Superset ForeignKey(s) to ab_user.id "
+ f"without an ondelete clause: {missing}. Every FK to ab_user.id "
+ "must set ondelete='SET NULL' (audit-trail columns) or 'CASCADE' "
+ "(ownership columns) to avoid IntegrityError when a user is "
+ "deleted โ see #38629."
+ )
+
+
+def test_audit_mixin_fks_use_set_null() -> None:
+ """``AuditMixinNullable.created_by_fk`` and ``changed_by_fk`` must
+ use ``ON DELETE SET NULL``. Any other behavior would silently drop
+ the audited row when its author is deleted, which we do not want.
+ See #38629.
+ """
+ wrong_behavior = [
+ (table, col, ondelete)
+ for table, col, ondelete in _iter_all_fks_to_ab_user()
+ if col in {"created_by_fk", "changed_by_fk"}
+ and ondelete != "SET NULL"
+ and not table.startswith("ab_")
+ ]
+ assert not wrong_behavior, (
+ f"AuditMixinNullable columns must use ondelete='SET NULL'. "
+ f"Found: {wrong_behavior}."
+ )
+
+
[email protected](
+ "table,column,expected",
+ [
+ # Audit-trail: row should survive user deletion with FK nulled
+ ("logs", "user_id", "SET NULL"),
+ ("slices", "last_saved_by_fk", "SET NULL"),
+ ("query", "user_id", "SET NULL"),
+ ("saved_query", "user_id", "SET NULL"),
+ ("key_value", "created_by_fk", "SET NULL"),
+ ("key_value", "changed_by_fk", "SET NULL"),
+ # Ownership: 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_direct_fk_uses_expected_ondelete(
+ table: str, column: str, expected: str
+) -> None:
+ """Individual model FKs to ``ab_user.id`` must use 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:** Declare the type of this local list so the expected optional
string element type is explicitly enforced. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is another newly introduced local list whose element type is
straightforward to annotate, but no type hint is present. The suggestion
correctly points out a missing type annotation under the stated rule.
</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=be548847d58941b39c3305e6da4f989e&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=be548847d58941b39c3305e6da4f989e&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:** 132:136
**Comment:**
*Custom Rule: Declare the type of this local list so the expected
optional string element type is explicitly enforced.
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=0e16c32b9bf873baa6e4ae4705eae8be2db046601c26b7bca5429011c1e6960d&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=0e16c32b9bf873baa6e4ae4705eae8be2db046601c26b7bca5429011c1e6960d&reaction=dislike'>๐</a>
##########
tests/unit_tests/models/test_user_delete_cascade.py:
##########
@@ -0,0 +1,140 @@
+# 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 #38629.
+
+Deleting a user via ``SecurityManager.delete_user()`` (which issues a
+bare ``DELETE FROM ab_user``) raised ``IntegrityError`` on
+PostgreSQL / MySQL / MariaDB whenever the user had dependent rows in
+tables that referenced ``ab_user.id`` via a foreign key with no
+``ON DELETE`` clause. This module pins two invariants at the model
+metadata layer:
+
+1. Every ``ForeignKey`` targeting ``ab_user.id`` declares an ``ondelete``
+ clause. A future contributor cannot add a new FK to ``ab_user`` and
+ silently reintroduce the bug.
+2. The ``AuditMixinNullable`` mixin's ``created_by_fk`` / ``changed_by_fk``
+ columns use ``ON DELETE SET NULL`` โ audit trails survive user
+ deletion with the reference cleared, rather than blocking the delete.
+"""
+
+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``.
+
+ Returns tuples of ``(table_name, column_name, ondelete)`` โ the
+ ``ondelete`` is the string captured by the ``ForeignKey`` constructor
+ (``"SET NULL"``, ``"CASCADE"``, or ``None``).
+ """
+ # Importing this module pulls in the whole Superset model graph, which
+ # in turn populates ``Model.metadata`` with every mapped table. Kept
+ # inside the function so that collection failures during pytest
+ # collection surface as test failures rather than import errors.
+ 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:
+ # ``fk.target_fullname`` looks like ``"ab_user.id"``
+ if fk.target_fullname == "ab_user.id":
+ fks.append((table.name, column.name, fk.ondelete))
+ return fks
+
+
+def test_every_superset_fk_to_ab_user_declares_ondelete() -> None:
+ """Every Superset-owned foreign key targeting ``ab_user.id`` must
+ declare an ``ondelete`` behavior so deleting a user does not blow up
+ with ``IntegrityError``. See #38629.
+
+ FAB-owned tables (``ab_user`` self-references, ``ab_user_role``,
+ ``ab_permission_view_role``, etc.) are excluded from this
+ model-level assertion because Superset does not own their model
+ declarations. The companion Alembic migration fixes them at the
+ database schema level via ``redefine()``.
+ """
+ missing = [
+ (table, col)
+ for table, col, ondelete in _iter_all_fks_to_ab_user()
+ if not ondelete and not table.startswith("ab_")
+ ]
+ assert not missing, (
+ f"Found {len(missing)} Superset ForeignKey(s) to ab_user.id "
+ f"without an ondelete clause: {missing}. Every FK to ab_user.id "
+ "must set ondelete='SET NULL' (audit-trail columns) or 'CASCADE' "
+ "(ownership columns) to avoid IntegrityError when a user is "
+ "deleted โ see #38629."
+ )
+
+
+def test_audit_mixin_fks_use_set_null() -> None:
+ """``AuditMixinNullable.created_by_fk`` and ``changed_by_fk`` must
+ use ``ON DELETE SET NULL``. Any other behavior would silently drop
+ the audited row when its author is deleted, which we do not want.
+ See #38629.
+ """
+ wrong_behavior = [
+ (table, col, ondelete)
+ for table, col, ondelete in _iter_all_fks_to_ab_user()
+ if col in {"created_by_fk", "changed_by_fk"}
+ and ondelete != "SET NULL"
+ and not table.startswith("ab_")
+ ]
Review Comment:
**Suggestion:** Add a type annotation to this local variable to make the
expected tuple fields and optional value explicit. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This local variable is a newly added computed list with a predictable tuple
shape, but it has no explicit type annotation. That matches the custom rule
about omitting type hints on relevant annotatable variables.
</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=13180ca342e24d5e86b39c4e005b6c6c&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=13180ca342e24d5e86b39c4e005b6c6c&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:** 95:101
**Comment:**
*Custom Rule: Add a type annotation to this local variable to make the
expected tuple fields and optional value explicit.
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=29176a937de8007a58d85f22fae8d3d04239d0b43d2c7c3ab2237e5200d2b803&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=29176a937de8007a58d85f22fae8d3d04239d0b43d2c7c3ab2237e5200d2b803&reaction=dislike'>๐</a>
##########
tests/unit_tests/models/test_user_delete_cascade.py:
##########
@@ -0,0 +1,140 @@
+# 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 #38629.
+
+Deleting a user via ``SecurityManager.delete_user()`` (which issues a
+bare ``DELETE FROM ab_user``) raised ``IntegrityError`` on
+PostgreSQL / MySQL / MariaDB whenever the user had dependent rows in
+tables that referenced ``ab_user.id`` via a foreign key with no
+``ON DELETE`` clause. This module pins two invariants at the model
+metadata layer:
+
+1. Every ``ForeignKey`` targeting ``ab_user.id`` declares an ``ondelete``
+ clause. A future contributor cannot add a new FK to ``ab_user`` and
+ silently reintroduce the bug.
+2. The ``AuditMixinNullable`` mixin's ``created_by_fk`` / ``changed_by_fk``
+ columns use ``ON DELETE SET NULL`` โ audit trails survive user
+ deletion with the reference cleared, rather than blocking the delete.
+"""
+
+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``.
+
+ Returns tuples of ``(table_name, column_name, ondelete)`` โ the
+ ``ondelete`` is the string captured by the ``ForeignKey`` constructor
+ (``"SET NULL"``, ``"CASCADE"``, or ``None``).
+ """
+ # Importing this module pulls in the whole Superset model graph, which
+ # in turn populates ``Model.metadata`` with every mapped table. Kept
+ # inside the function so that collection failures during pytest
+ # collection surface as test failures rather than import errors.
+ 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:
+ # ``fk.target_fullname`` looks like ``"ab_user.id"``
+ if fk.target_fullname == "ab_user.id":
+ fks.append((table.name, column.name, fk.ondelete))
+ return fks
+
+
+def test_every_superset_fk_to_ab_user_declares_ondelete() -> None:
+ """Every Superset-owned foreign key targeting ``ab_user.id`` must
+ declare an ``ondelete`` behavior so deleting a user does not blow up
+ with ``IntegrityError``. See #38629.
+
+ FAB-owned tables (``ab_user`` self-references, ``ab_user_role``,
+ ``ab_permission_view_role``, etc.) are excluded from this
+ model-level assertion because Superset does not own their model
+ declarations. The companion Alembic migration fixes them at the
+ database schema level via ``redefine()``.
+ """
+ missing = [
+ (table, col)
+ for table, col, ondelete in _iter_all_fks_to_ab_user()
+ if not ondelete and not table.startswith("ab_")
+ ]
Review Comment:
**Suggestion:** Add an explicit type annotation for this local collection so
the inferred tuple structure is declared. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The rule requires type hints on relevant variables that can be annotated.
This local list is newly introduced and has a clear tuple element structure,
but it is left unannotated, so the suggestion identifies 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=69fa860838cd4deba58c5ce302d00670&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=69fa860838cd4deba58c5ce302d00670&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:** 75:79
**Comment:**
*Custom Rule: Add an explicit type annotation for this local collection
so the inferred tuple structure is declared.
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=1e21c82082a668b4c9a79b97600354252b1a26fabad271dc85ec7aace4882e48&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41994&comment_hash=1e21c82082a668b4c9a79b97600354252b1a26fabad271dc85ec7aace4882e48&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]
