sqlalchemy / alembic

A database migrations tool for SQLAlchemy.
MIT License
2.89k stars 247 forks source link

Unique constraint missing name/duplicated create_unique/drop_constraint #1531

Closed gaborfekete-greehill closed 2 months ago

gaborfekete-greehill commented 2 months ago

Describe the bug

I'm using SQLAlchemy with postgresql+psycopg2 as driver on localhost. I created a simple SQLAlchemy model using ORM:

# models.py
from sqlalchemy import Column, Integer, String, UniqueConstraint
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()

class User(Base):
    __tablename__ = 'users'

    id = Column(Integer, primary_key=True)
    username = Column(String, unique=True, nullable=False)
    email = Column(String, unique=True, nullable=False)

I created an autogenerated alembic revision:

def upgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.create_table('users',
    sa.Column('id', sa.Integer(), nullable=False),
    sa.Column('username', sa.String(), nullable=False),
    sa.Column('email', sa.String(), nullable=False),
    sa.PrimaryKeyConstraint('id'),
    sa.UniqueConstraint('email'),
    sa.UniqueConstraint('username')
    )
    # ### end Alembic commands ###

def downgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.drop_table('users')
    # ### end Alembic commands ###

Until now this is the expected behaviour. I wanted to add a new column nickname to the users table with a unique contraint:

class User(Base):
    __tablename__ = 'users'

    id = Column(Integer, primary_key=True)
    username = Column(String, unique=True, nullable=False)
    email = Column(String, unique=True, nullable=False)
    nickname = Column(String, unique=True, nullable=False)

The resulting generated revision file then looks like this:

def upgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.add_column('users', sa.Column('nickname', sa.String(), nullable=False))
    op.create_unique_constraint(None, 'users', ['nickname'])
    # ### end Alembic commands ###

def downgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.drop_constraint(None, 'users', type_='unique')
    op.drop_column('users', 'nickname')
    # ### end Alembic commands ###

The users table now has a unique constraint without a name. It cannot even be downgraded anymore as it can't drop the unnamed constraint. I found a solution to add the unique contraint to the __table_args__ class member:

class User(Base):
    __tablename__ = 'users'
    __table_args__ = (
        UniqueConstraint('nickname', name='uq_user_nickname'),
    )

    id = Column(Integer, primary_key=True)
    username = Column(String, unique=True, nullable=False)
    email = Column(String, unique=True, nullable=False)
    nickname = Column(String, unique=True, nullable=False)

This now generated this revision:

def upgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.add_column('users', sa.Column('nickname', sa.String(), nullable=False))
    op.create_unique_constraint('uq_user_nickname', 'users', ['nickname'])
    op.create_unique_constraint(None, 'users', ['nickname'])
    # ### end Alembic commands ###

def downgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.drop_constraint(None, 'users', type_='unique')
    op.drop_constraint('uq_user_nickname', 'users', type_='unique')
    op.drop_column('users', 'nickname')
    # ### end Alembic commands ###

Note the duplicated create/drop constraint lines. Then as an extra case to include in this issue I re-did the previous step but removed the unique=True from the nickname column. This then generated this revision:

def upgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.add_column('users', sa.Column('nickname', sa.String(), nullable=False))
    op.create_unique_constraint('uq_user_nickname', 'users', ['nickname'])
    # ### end Alembic commands ###

def downgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.drop_constraint('uq_user_nickname', 'users', type_='unique')
    op.drop_column('users', 'nickname')
    # ### end Alembic commands ###

Which seems to be the correct behaviour in this case but it's a bit ugly ORM-wise. Modifying the models.py file then to this:

class User(Base):
    __tablename__ = 'users'

    id = Column(Integer, primary_key=True)
    username = Column(String, unique=True, nullable=False)
    email = Column(String, unique=True, nullable=False)
    nickname = Column(String, unique=True, nullable=False)

makes alembic happy too, it can correctly up/downgrade and the ORM model looks much better!

Expected behavior

I'm not that familiar with ORM based database models and what the usual practices are with naming the constraints but some standard way to name the constraint(s) could be an expected behaviour in this case. Also not duplicating the create/drop lines could be somehow detected and omitted as part of the solution for this issue.

To Reproduce Please try to provide a Minimal, Complete, and Verifiable example, with the migration script and/or the SQLAlchemy tables or models involved. See also Reporting Bugs on the website.

I wrote every step at the description of the bug

Error No particular error it just can't create a proper constraint for the column as it won't have a name. Downgrading causes an error when the anonymous constraint is present as it can't drop it.

Versions.

Additional context

Have a nice day!