sadpandajoe commented on code in PR #35542: URL: https://github.com/apache/superset/pull/35542#discussion_r3825413825
########## superset/migrations/versions/2025-10-06_16-05_b54f3bd8e69_update_tag_unique_constraint.py: ########## @@ -0,0 +1,116 @@ +# 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. +"""update_tag_unique_constraint + +Revision ID: b54f3bd8e69 +Revises: c233f5365c9e +Create Date: 2025-10-06 16:05:00.000000 + +""" + +import enum + +import migration_utils as utils +from alembic import op +from sqlalchemy import Column, Enum, Integer, MetaData, String, Table, Text +from sqlalchemy.sql import func, select + +# revision identifiers, used by Alembic. +revision = "b54f3bd8e69" +down_revision = "c233f5365c9e" + + +class TagType(enum.Enum): + # pylint: disable=invalid-name + custom = 1 + type = 2 + owner = 3 + favorited_by = 4 + + +# Define the tag table structure for data operations +metadata = MetaData() +tag_table = Table( + "tag", + metadata, + Column("id", Integer, primary_key=True), + Column("name", String(250)), + Column("type", Enum(TagType)), + Column("description", Text), +) + +old_constraint_name = "tag_name_key" +new_constraint_name = "uix_tag_name_type" +table_name = "tag" +new_constraint_columns = ["name", "type"] + + +def upgrade(): + """ + Change tag unique constraint from name only to (name, type) composite. + This allows the same tag name to exist with different types (e.g., 'type:dashboard' + can be both a system tag with type='type' and a custom tag with type='custom'). + """ + bind = op.get_bind() + + # Reflect the current database state to get existing tables + metadata.reflect(bind=bind) + + # Delete duplicate tags if any, keeping the one with the lowest ID + min_id_subquery = ( + select( + [ + func.min(tag_table.c.id).label("min_id"), + tag_table.c.name, + tag_table.c.type, + ] + ) + .group_by( + tag_table.c.name, + tag_table.c.type, + ) + .alias("min_ids") + ) + + delete_query = tag_table.delete().where( + tag_table.c.id.notin_(select([min_id_subquery.c.min_id])) + ) + + bind.execute(delete_query) Review Comment: This can delete an existing tag before the constraint change. `name` is nullable, and the old single-column unique constraint permits multiple NULL names; two such tags with the same type are grouped here even though the new composite constraint also permits them. If either tag is referenced, the migration either fails on the foreign key or loses its association. Could this skip NULL names (or preserve/remap references) instead of deleting them? ########## superset/migrations/versions/2025-10-06_16-05_b54f3bd8e69_update_tag_unique_constraint.py: ########## @@ -0,0 +1,62 @@ +# 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. +"""update_tag_unique_constraint + +Revision ID: b54f3bd8e69 +Revises: c233f5365c9e +Create Date: 2025-10-06 16:05:00.000000 + +""" + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "b54f3bd8e69" +down_revision = "c233f5365c9e" + + +def upgrade(): + """ + Change tag unique constraint from name only to (name, type) composite. + This allows the same tag name to exist with different types (e.g., 'type:dashboard' + can be both a system tag with type='type' and a custom tag with type='custom'). + """ + try: + # Drop the old unique constraint on name only + op.drop_constraint("tag_name_key", "tag", type_="unique") + + # Create new composite unique constraint on (name, type) + op.create_unique_constraint("uix_tag_name_type", "tag", ["name", "type"]) + except Exception: # noqa: S110 + # SQLite doesn't support constraint operations well + pass + + +def downgrade(): + """ + Revert to name-only unique constraint. + Note: This may fail if there are duplicate names with different types. + """ Review Comment: Agreed—the upgrade enables same-name tags across types, so recreating the name-only constraint during downgrade can fail and leave the migration half-applied. Should the downgrade reject that state before dropping the composite constraint? ########## superset/migrations/versions/2025-10-06_16-05_b54f3bd8e69_update_tag_unique_constraint.py: ########## @@ -0,0 +1,116 @@ +# 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. +"""update_tag_unique_constraint + +Revision ID: b54f3bd8e69 +Revises: c233f5365c9e +Create Date: 2025-10-06 16:05:00.000000 + +""" + +import enum + +import migration_utils as utils +from alembic import op +from sqlalchemy import Column, Enum, Integer, MetaData, String, Table, Text +from sqlalchemy.sql import func, select + +# revision identifiers, used by Alembic. +revision = "b54f3bd8e69" +down_revision = "c233f5365c9e" + + +class TagType(enum.Enum): + # pylint: disable=invalid-name + custom = 1 + type = 2 + owner = 3 + favorited_by = 4 + + +# Define the tag table structure for data operations +metadata = MetaData() +tag_table = Table( + "tag", + metadata, + Column("id", Integer, primary_key=True), + Column("name", String(250)), + Column("type", Enum(TagType)), + Column("description", Text), +) + +old_constraint_name = "tag_name_key" +new_constraint_name = "uix_tag_name_type" +table_name = "tag" +new_constraint_columns = ["name", "type"] + + +def upgrade(): + """ + Change tag unique constraint from name only to (name, type) composite. + This allows the same tag name to exist with different types (e.g., 'type:dashboard' + can be both a system tag with type='type' and a custom tag with type='custom'). + """ + bind = op.get_bind() + + # Reflect the current database state to get existing tables + metadata.reflect(bind=bind) + + # Delete duplicate tags if any, keeping the one with the lowest ID + min_id_subquery = ( + select( + [ + func.min(tag_table.c.id).label("min_id"), + tag_table.c.name, + tag_table.c.type, + ] + ) + .group_by( + tag_table.c.name, + tag_table.c.type, + ) + .alias("min_ids") + ) + + delete_query = tag_table.delete().where( + tag_table.c.id.notin_(select([min_id_subquery.c.min_id])) + ) + + bind.execute(delete_query) + + # Drop the old unique constraint on name only + utils.drop_unique_constraint(op, old_constraint_name, table_name) + Review Comment: Agreed—the original name-only uniqueness was unnamed, so this PostgreSQL-specific identifier is absent on MySQL and SQLite and the migration aborts before the replacement constraint is created. Could this discover the existing name-based unique constraint/index per dialect before dropping it? -- 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]
