Re: [PR] fix: update tag unique constraint to (name, type) [superset]

["mistercrunch (via GitHub)" <gi...@apache.org>]

mistercrunch commented on PR #35542:
URL: https://github.com/apache/superset/pull/35542#issuecomment-3392681267

   reviewed with claude, main thing in my prompt was asking whether we should use the migration utils..
   ---
   
   The migration looks good overall. A few suggestions:
   
   ### 1. Use migration utilities
   
   Consider using `migration_utils.create_unique_constraint()` and `migration_utils.drop_unique_constraint()` instead of raw Alembic operations. These handle SQLite compatibility and idempotency.
   
   Reference: `superset/migrations/versions/2024-01-17_13-09_96164e3017c6_tagged_object_unique_constraint.py:84-88`
   
   ```python
   import migration_utils as utils
   
   def upgrade() -> None:
       utils.drop_unique_constraint(op, "tag_name_key", "tag")
       utils.create_unique_constraint(op, "uix_tag_name_type", "tag", ["name", "type"])
   
   def downgrade() -> None:
       utils.drop_unique_constraint(op, "uix_tag_name_type", "tag")
       utils.create_unique_constraint(op, "tag_name_key", "tag", ["name"])
   ```
   
   ### 2. Consider downgrade edge case
   
   If duplicate tag names exist after upgrade, downgrade will fail when recreating the single-column constraint. Consider checking for duplicates first or documenting this limitation.
   
   Otherwise looks good to merge once these are addressed.
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org