korbit-ai[bot] commented on code in PR #32759: URL: https://github.com/apache/superset/pull/32759#discussion_r2003934136
########## superset/migrations/versions/2025-03-19_17-46_32bf93dfe2a4_add_on_cascade_in_fab_tables.py: ########## @@ -0,0 +1,102 @@ +# 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. +from alembic import op + +"""Add on cascade to foreign keys in ab_permission_view_role and ab_user_role + +Revision ID: 32bf93dfe2a4 +Revises: 94e7a3499973 +Create Date: 2025-03-19 17:46:25.702610 + +""" + +# revision identifiers, used by Alembic. +revision = "32bf93dfe2a4" +down_revision = "94e7a3499973" + + +def upgrade(): + op.drop_constraint( + "ab_permission_view_role_permission_view_id_fkey", + "ab_permission_view_role", + type_="foreignkey", + ) + op.drop_constraint( + "ab_permission_view_role_role_id_fkey", + "ab_permission_view_role", + type_="foreignkey", + ) + op.create_foreign_key( + None, + "ab_permission_view_role", + "ab_role", + ["role_id"], + ["id"], + ondelete="CASCADE", + ) Review Comment: ### Cascading Deletions in RBAC Tables <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Adding CASCADE deletion behavior to role-based access control (RBAC) foreign keys could potentially lead to uncontrolled permission changes if a parent record is deleted. ###### Why this matters Automatic cascading deletions in security-related tables may cause unintended removal of permission assignments, potentially creating security gaps or unauthorized access situations if not carefully managed at the application level. ###### Suggested change ∙ *Feature Preview* Consider using `ondelete="RESTRICT"` or handling deletions explicitly at the application level with proper security checks: ```python op.create_foreign_key( None, "ab_permission_view_role", "ab_role", ["role_id"], ["id"], ondelete="RESTRICT", ) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9ca8294-c6e7-4d43-9312-6ca5734b658e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9ca8294-c6e7-4d43-9312-6ca5734b658e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9ca8294-c6e7-4d43-9312-6ca5734b658e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9ca8294-c6e7-4d43-9312-6ca5734b658e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9ca8294-c6e7-4d43-9312-6ca5734b658e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:2762a7d7-be92-4842-a85c-2d9ac33a10db --> [](2762a7d7-be92-4842-a85c-2d9ac33a10db) ########## superset/migrations/versions/2025-03-19_17-46_32bf93dfe2a4_add_on_cascade_in_fab_tables.py: ########## @@ -0,0 +1,102 @@ +# 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. +from alembic import op + +"""Add on cascade to foreign keys in ab_permission_view_role and ab_user_role + +Revision ID: 32bf93dfe2a4 +Revises: 94e7a3499973 +Create Date: 2025-03-19 17:46:25.702610 + +""" + +# revision identifiers, used by Alembic. +revision = "32bf93dfe2a4" +down_revision = "94e7a3499973" + + +def upgrade(): + op.drop_constraint( + "ab_permission_view_role_permission_view_id_fkey", + "ab_permission_view_role", + type_="foreignkey", + ) + op.drop_constraint( + "ab_permission_view_role_role_id_fkey", + "ab_permission_view_role", + type_="foreignkey", + ) + op.create_foreign_key( + None, + "ab_permission_view_role", + "ab_role", + ["role_id"], + ["id"], + ondelete="CASCADE", + ) + op.create_foreign_key( + None, + "ab_permission_view_role", + "ab_permission_view", + ["permission_view_id"], + ["id"], + ondelete="CASCADE", + ) + op.drop_constraint("ab_user_role_user_id_fkey", "ab_user_role", type_="foreignkey") + op.drop_constraint("ab_user_role_role_id_fkey", "ab_user_role", type_="foreignkey") + op.create_foreign_key( + None, "ab_user_role", "ab_user", ["user_id"], ["id"], ondelete="CASCADE" + ) + op.create_foreign_key( + None, "ab_user_role", "ab_role", ["role_id"], ["id"], ondelete="CASCADE" + ) + + +def downgrade(): + op.drop_constraint("ab_user_role_role_id_fkey", "ab_user_role", type_="foreignkey") + op.drop_constraint("ab_user_role_user_id_fkey", "ab_user_role", type_="foreignkey") + op.create_foreign_key( + "ab_user_role_role_id_fkey", "ab_user_role", "ab_role", ["role_id"], ["id"] + ) Review Comment: ### Incorrect constraint drop order in downgrade <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The downgrade function drops constraints in a different order than they were created in the upgrade function, which could lead to foreign key violations during the downgrade process. ###### Why this matters If there are existing records in these tables, dropping constraints in a different order than they were created might cause the migration to fail due to referential integrity issues. ###### Suggested change ∙ *Feature Preview* Modify the downgrade function to drop and create constraints in the reverse order of the upgrade function: ```python def downgrade(): # First handle ab_permission_view_role constraints op.drop_constraint( "ab_permission_view_role_permission_view_id_fkey", "ab_permission_view_role", type_="foreignkey", ) op.drop_constraint( "ab_permission_view_role_role_id_fkey", "ab_permission_view_role", type_="foreignkey", ) op.create_foreign_key( "ab_permission_view_role_permission_view_id_fkey", "ab_permission_view_role", "ab_permission_view", ["permission_view_id"], ["id"], ) op.create_foreign_key( "ab_permission_view_role_role_id_fkey", "ab_permission_view_role", "ab_role", ["role_id"], ["id"], ) # Then handle ab_user_role constraints op.drop_constraint("ab_user_role_user_id_fkey", "ab_user_role", type_="foreignkey") op.drop_constraint("ab_user_role_role_id_fkey", "ab_user_role", type_="foreignkey") op.create_foreign_key( "ab_user_role_user_id_fkey", "ab_user_role", "ab_user", ["user_id"], ["id"] ) op.create_foreign_key( "ab_user_role_role_id_fkey", "ab_user_role", "ab_role", ["role_id"], ["id"] ) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22672018-5df6-41e5-b0a6-c9259ee155bb/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22672018-5df6-41e5-b0a6-c9259ee155bb?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22672018-5df6-41e5-b0a6-c9259ee155bb?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22672018-5df6-41e5-b0a6-c9259ee155bb?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22672018-5df6-41e5-b0a6-c9259ee155bb) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:5dd48da8-a6e0-4e84-b6cd-bd920fdef4c7 --> [](5dd48da8-a6e0-4e84-b6cd-bd920fdef4c7) ########## superset/migrations/versions/2025-03-19_17-46_32bf93dfe2a4_add_on_cascade_in_fab_tables.py: ########## @@ -0,0 +1,102 @@ +# 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. +from alembic import op + +"""Add on cascade to foreign keys in ab_permission_view_role and ab_user_role + +Revision ID: 32bf93dfe2a4 +Revises: 94e7a3499973 +Create Date: 2025-03-19 17:46:25.702610 + +""" Review Comment: ### Missing Migration Purpose and Impact <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The migration docstring does not explain the reasoning behind adding CASCADE constraints or its implications. ###### Why this matters Without understanding the purpose and impact of CASCADE constraints, future maintainers might accidentally modify or remove them, potentially leading to data integrity issues. ###### Suggested change ∙ *Feature Preview* """Add on cascade to foreign keys in ab_permission_view_role and ab_user_role Revision ID: 32bf93dfe2a4 Revises: 94e7a3499973 Create Date: 2025-03-19 17:46:25.702610 Adds CASCADE constraints to ensure automatic cleanup of related permission_view_role and user_role entries when parent records are deleted, preventing orphaned records and maintaining referential integrity. """ ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38d8b5d2-becc-458b-b33b-ab5ba6ff33c6/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38d8b5d2-becc-458b-b33b-ab5ba6ff33c6?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38d8b5d2-becc-458b-b33b-ab5ba6ff33c6?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38d8b5d2-becc-458b-b33b-ab5ba6ff33c6?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38d8b5d2-becc-458b-b33b-ab5ba6ff33c6) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:3e0f14e4-29f0-4287-af69-c96066d71e54 --> [](3e0f14e4-29f0-4287-af69-c96066d71e54) -- 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]
