sqlalchemy / alembic

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

postgresql serial column detection might fail #1565

Open JabberWocky-22 opened 3 weeks ago

JabberWocky-22 commented 3 weeks ago

Describe the bug There is a probability that serial column was not detected. This can happen when the table is referenced by another table.

Expected behavior The result of autogererate should't contain server default for column id.

To Reproduce

import sqlalchemy as sa
from alembic.autogenerate.api import produce_migrations
from alembic.autogenerate.api import render_python_code
from alembic.migration import MigrationContext

def filter_by_schema(schema_name: str):
    def _filter_by_schema(name, type_, parent_names):
        if type_ == "schema":
            return name == schema_name
        else:
            return True

    return _filter_by_schema

engine = sa.create_engine("postgresql://name:pass@host:1234/tmp")
with engine.connect() as conn, conn.begin():
    conn.execute(
        sa.text(
            """
        DROP SCHEMA IF EXISTS test CASCADE;
        CREATE SCHEMA test;
        CREATE TABLE test.account1(id SERIAL PRIMARY KEY);
        CREATE TABLE test.account2(id SERIAL PRIMARY KEY, id2 int REFERENCES test.account1(id));
        """
        )
    )
metadata = sa.MetaData(schema="test")
mc = MigrationContext.configure(
    connection=engine.connect(),
    opts={"include_name": filter_by_schema("test"), "include_schemas": True},
)
migration_script = produce_migrations(mc, metadata)
downgrade_code = render_python_code(migration_script.downgrade_ops)
print(downgrade_code)

Error It will produce two outputs and the first one is incorrect.

# ### commands auto generated by Alembic - please adjust! ###
    op.create_table('account1',
    sa.Column('id', sa.INTEGER(), server_default=sa.text("nextval('test.account1_id_seq'::regclass)"), autoincrement=True, nullable=False),
    sa.PrimaryKeyConstraint('id', name='account1_pkey'),
    schema='test',
    postgresql_ignore_search_path=False
    )
    op.create_table('account2',
    sa.Column('id', sa.INTEGER(), autoincrement=True, nullable=False),
    sa.Column('id2', sa.INTEGER(), autoincrement=False, nullable=True),
    sa.ForeignKeyConstraint(['id2'], ['test.account1.id'], name='account2_id2_fkey'),
    sa.PrimaryKeyConstraint('id', name='account2_pkey'),
    schema='test'
    )
    # ### end Alembic commands ###
# ### commands auto generated by Alembic - please adjust! ###
    op.create_table('account2',
    sa.Column('id', sa.INTEGER(), autoincrement=True, nullable=False),
    sa.Column('id2', sa.INTEGER(), autoincrement=False, nullable=True),
    sa.ForeignKeyConstraint(['id2'], ['test.account1.id'], name='account2_id2_fkey'),
    sa.PrimaryKeyConstraint('id', name='account2_pkey'),
    schema='test'
    )
    op.create_table('account1',
    sa.Column('id', sa.INTEGER(), autoincrement=True, nullable=False),
    sa.PrimaryKeyConstraint('id', name='account1_pkey'),
    schema='test'
    )
    # ### end Alembic commands ###

Versions.

Additional context If table is not fererenced by other table, the result is always correct. If I make both tables reference each other, the first table in autogenerate is alway incorrect.

ALTER TABLE test.account1 ADD FOREIGN KEY (id) REFERENCES test.account2(id);

I guess the listen hook migth fail with foreign keys, not sure since havn't dig into code much.

Have a nice day!

CaselIT commented 2 weeks ago

Hi,

It seems like a bug, but I'm proposing to close as won't fix.

The reason is that they table definition that you have are highly unlikely, since it doesn't make much sense adding a fk on an auto-incrementing column. Since postgresql does not tell you if a column is a serial when reflecting a table, alembic has some heuristic to figure out when it is and whet it isn't. The presence of a fk on the column likely makes alembic think that the column is not a serial.

Thoughts @zzzeek ?

JabberWocky-22 commented 2 weeks ago

Actually, it can happen on a table with serial column and being referenced by another table(doesn't matter adding foreign key on which column), just like:

CREATE SCHEMA test;
CREATE TABLE test.account1(short_id SERIAL PRIMARY KEY, id uuid unique);
CREATE TABLE test.account2(short_id SERIAL PRIMARY KEY, id uuid unique REFERENCES test.account1(id));
ALTER TABLE test.account1 ADD FOREIGN KEY (id) REFERENCES test.account2(id);  -- not necessary

Besides two tables reference each other just make it easier to reproduce, it's not necessary.

JabberWocky-22 commented 2 weeks ago

https://github.com/sqlalchemy/alembic/blob/431fd0783e2e414cac5b0d0f002d7adc71df3f02/alembic/autogenerate/compare.py#L204-L219

Looks like when reflecting table, the referenced table will be reflected too, since the default value of resolve_fks is True. And the event hook won't take effect for the referenced table.

another similar issue: https://github.com/sqlalchemy/alembic/issues/1454