codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3425685445
########## superset/migrations/versions/2026-04-19_14-30_b7e4f2a891c3_add_auth_audit_log_table.py: ########## @@ -0,0 +1,54 @@ +# 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 auth_audit_log table + +Revision ID: b7e4f2a891c3 +Revises: ce6bd21901ab +Create Date: 2026-04-19 14:30:00.000000 + +""" + +import sqlalchemy as sa + +from superset.migrations.shared.utils import create_table, drop_table + +# revision identifiers, used by Alembic. +revision = "b7e4f2a891c3" +down_revision = "ce6bd21901ab" + + +def upgrade() -> None: + create_table( + "auth_audit_log", + sa.Column("id", sa.Integer(), nullable=False, autoincrement=True), + sa.Column("user_id", sa.Integer(), nullable=True), + sa.Column("event_type", sa.String(length=64), nullable=False), + sa.Column("ip_address", sa.String(length=256), nullable=True), + sa.Column("user_agent", sa.Text(), nullable=True), + sa.Column("metadata", sa.JSON(), nullable=True), + sa.Column("created_at", sa.DateTime(), nullable=False), + sa.ForeignKeyConstraint( + ["user_id"], + ["ab_user.id"], + name="fk_auth_audit_log_user_id_ab_user", + ), Review Comment: **Suggestion:** The foreign key to `ab_user.id` has no `ondelete` behavior, so deleting a user with audit rows will fail due to referential integrity errors. Because `user_id` is nullable and audit rows should typically survive user deletion, configure the FK with `ondelete="SET NULL"` (or explicitly cascade if that is the intended retention policy). [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Deleting users with audit rows raises integrity errors. - ⚠️ User administration delete operations can unexpectedly fail. - ⚠️ May require manual cleanup of auth_audit_log rows. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Apply migration `ce6bd21901ab` then `b7e4f2a891c3_add_auth_audit_log_table` from `superset/migrations/versions/2026-04-19_14-30_b7e4f2a891c3_add_auth_audit_log_table.py:34-50`, which creates `auth_audit_log` with `user_id` and a foreign key `sa.ForeignKeyConstraint(["user_id"], ["ab_user.id"], name="fk_auth_audit_log_user_id_ab_user")` without `ondelete`. 2. With AUTH_DB enabled, change a user's password via `PUT /api/v1/me/password`, implemented by `CurrentUserRestApi.update_my_password` in `superset/views/users/api.py:238-259`; on success it calls `AuthAuditLogDAO.create(..., user_id=g.user.id, ...)` at `superset/views/users/api.py:320-31`. 3. `AuthAuditLogDAO.create` in `superset/daos/auth_audit_log.py:28-55` persists an `AuthAuditLog` row whose `user_id` references `ab_user.id`, and the ORM model `AuthAuditLog` in `superset/models/auth_audit_log.py:35-51` maps `user_id = Column(Integer, ForeignKey("ab_user.id"), nullable=True)` to the same constraint, so the database now contains audit rows tied to that user. 4. An administrator deletes this user through the standard user API `UserRestApi` in `superset/views/users/api.py:415-423` (Flask-AppBuilder's ModelRestApi exposes DELETE `/api/v1/user/<id>` based on `resource_name = "user"`); when SQLAlchemy issues `DELETE` for the `ab_user` row, the database enforces the `auth_audit_log.user_id -> ab_user.id` foreign key without `ondelete`, raising a referential integrity error and causing the delete endpoint to fail instead of removing the user or nulling `user_id` in audit rows. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b505859c1a004fc19206a382b748b49d&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=b505859c1a004fc19206a382b748b49d&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-04-19_14-30_b7e4f2a891c3_add_auth_audit_log_table.py **Line:** 44:48 **Comment:** *Logic Error: The foreign key to `ab_user.id` has no `ondelete` behavior, so deleting a user with audit rows will fail due to referential integrity errors. Because `user_id` is nullable and audit rows should typically survive user deletion, configure the FK with `ondelete="SET NULL"` (or explicitly cascade if that is the intended retention policy). 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%2F39469&comment_hash=2262f9c0499a73821b4ebed407dafb74bcf59faf4d79a7385536a59e5f561046&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=2262f9c0499a73821b4ebed407dafb74bcf59faf4d79a7385536a59e5f561046&reaction=dislike'>👎</a> ########## superset/migrations/versions/2026-06-16_16-00_33a0aac0a26a_merging_two_heads.py: ########## @@ -0,0 +1,35 @@ +# 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. +"""merging two heads + +Revision ID: 33a0aac0a26a +Revises: ('c6219cac9270', '33d7e0e21daa') +Create Date: 2026-06-16 16:00:00.000000 + +""" + +# revision identifiers, used by Alembic. +revision = "33a0aac0a26a" +down_revision = ("c6219cac9270", "33d7e0e21daa") Review Comment: **Suggestion:** The merge revision is wired to `33d7e0e21daa`, which is not the active head of that branch anymore (there are newer descendants). This leaves multiple migration heads after applying this revision, and `alembic upgrade head` can fail with a multiple-heads error. Update `down_revision` to merge `c6219cac9270` with the current head of the other branch. [api mismatch] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Alembic upgrade head fails due to multiple migration heads. - ❌ Superset schema upgrade pipeline is blocked for affected installs. - ⚠️ Operators must manually adjust migrations to proceed. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect the migration graph around `ce6bd21901ab` using the files in `superset/migrations/versions`: `2025-11-04_11-26_33d7e0e21daa_add_semantic_layers_and_views.py` declares `revision = "33d7e0e21daa", down_revision = "ce6bd21901ab"` at lines 7-8; `2026-04-22_19-00_31dae2559c05_replay_user_favorite_tag_migration.py` declares `revision = "31dae2559c05", down_revision = "33d7e0e21daa"` at lines 11-12; `2026-06-01_00-10_c8d2e3f4a5b6_add_guest_token_revoked_before.py` and `2026-06-02_10-00_f7a1c93e0b21_add_sessions_invalidated_at.py` both have `down_revision = "31dae2559c05"` (see `f7a1c93e0b21_add_sessions_invalidated_at.py:5-7`), and `2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py:30-31` merges those two heads with `down_revision = ("c8d2e3f4a5b6", "f7a1c93e0b21")`, followed by `2026-06-03_10-00_78a40c08b4be_add_server_host_key_to_ssh_tunnels.py:3-4` which sets `revision = "78a40c08b4be", down_revision = "b7c9d1e2f3a4"`. This means the semantic-layers branch's current head is `78a40c08b4be`, not `33d7e0e21daa`. 2. In parallel, a second branch from `ce6bd21901ab` goes through the new auth audit log: `2026-04-19_14-30_b7e4f2a891c3_add_auth_audit_log_table.py:30-31` defines `revision = "b7e4f2a891c3", down_revision = "ce6bd21901ab"`, and `2026-05-13_12-00_c6219cac9270_add_user_session_auth_stamp_table.py:30-31` sets `revision = "c6219cac9270", down_revision = "b7e4f2a891c3"`, so before adding the new merge there are two Alembic heads: `78a40c08b4be` (semantic branch) and `c6219cac9270` (auth-session branch). 3. The new merge migration `2026-06-16_16-00_33a0aac0a26a_merging_two_heads.py` at `superset/migrations/versions/2026-06-16_16-00_33a0aac0a26a_merging_two_heads.py:26-27` declares `revision = "33a0aac0a26a"` and `down_revision = ("c6219cac9270", "33d7e0e21daa")`, incorrectly referencing `33d7e0e21daa`, which is an internal ancestor of the semantic branch rather than its current head `78a40c08b4be`. 4. On a database that has been upgraded to both heads `c6219cac9270` and `78a40c08b4be`, running the standard migration command (e.g. `superset db upgrade`, which uses Alembic `upgrade head`) with this PR causes Alembic to apply `33a0aac0a26a` only on top of `c6219cac9270` and `33d7e0e21daa`; the semantic branch continues past `33d7e0e21daa` to `31dae2559c05`, `b7c9d1e2f3a4`, and `78a40c08b4be`, so Alembic still sees two leaf revisions: `78a40c08b4be` and `33a0aac0a26a`. As a result, `alembic heads` shows multiple heads and `alembic upgrade head` (or `superset db upgrade`) fails with a multiple-heads CommandError instead of collapsing the branches into a single head. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b9feea289dc64cefa6c8f24af66ed6a1&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=b9feea289dc64cefa6c8f24af66ed6a1&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-06-16_16-00_33a0aac0a26a_merging_two_heads.py **Line:** 27:27 **Comment:** *Api Mismatch: The merge revision is wired to `33d7e0e21daa`, which is not the active head of that branch anymore (there are newer descendants). This leaves multiple migration heads after applying this revision, and `alembic upgrade head` can fail with a multiple-heads error. Update `down_revision` to merge `c6219cac9270` with the current head of the other branch. 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%2F39469&comment_hash=75b2bd4edd53977734ec61a29d3b3fd6ba6c2318babc5f733e6755bc3d37933c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=75b2bd4edd53977734ec61a29d3b3fd6ba6c2318babc5f733e6755bc3d37933c&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]
