sadpandajoe commented on code in PR #41994: URL: https://github.com/apache/superset/pull/41994#discussion_r3829225090
########## 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( Review Comment: This only checks ORM metadata, so it still passes if the migration fails to replace an existing database constraint. Could this cover a pre-migration schema, apply the migration, and delete a seeded user to verify the reflected `SET NULL`/`CASCADE` behavior? ########## 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: The normal user-delete path uses `session.delete(user)`, and this `extra_attributes` backref is not passive, so SQLAlchemy nulls `user_attribute.user_id` before the database can apply this CASCADE. That leaves the supposedly user-owned attribute row orphaned (and its audit FKs can still block deletion). Could the relationship use the database delete semantics instead? ########## 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: Deleting a user now removes `FavStar` rows through the database cascade, which bypasses `FavStarUpdater.after_delete`; with `TAGGING_SYSTEM` enabled, the corresponding `favorited_by:<user>` `TaggedObject` rows therefore remain and make search/filter results disagree with `favstar`. Could this deletion clean up the derived tags as well? ########## 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: Agreed—the parent revision already has a different child, so this migration creates a second Alembic head and upgrades will abort. Could this be rebased onto the current migration lineage? -- 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]
