sqlalchemy / alembic

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

TypeError: drop_index() takes 2 positional arguments but 3 were given #1243

Closed bobbypriam closed 1 year ago

bobbypriam commented 1 year ago

Describe the bug Upgrading from 1.10.4 to 1.11.0 breaks existing migration, possibly due to this change: https://github.com/sqlalchemy/alembic/issues/1130

Expected behavior Existing migration still works, or a deprecation warning with clear migration path is given.

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.

"""dummy revision

Revision ID: cc20a95c17c5
Revises:
Create Date: 2023-05-16 13:41:28.641085

"""
from alembic import op
import sqlalchemy as sa

# revision identifiers, used by Alembic.
revision = 'cc20a95c17c5'
down_revision = None
branch_labels = None
depends_on = None

def upgrade() -> None:
    # This worked on 1.10.4, but broke on 1.11.0
    op.drop_index("dummy_index_name", "dummy_table_name")
    pass

def downgrade() -> None:
    pass

Error

➵  alembic upgrade head
INFO  [alembic.runtime.migration] Context impl MySQLImpl.
INFO  [alembic.runtime.migration] Will assume non-transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade  -> cc20a95c17c5, dummy revision
Traceback (most recent call last):
  File "/Users/<user>/Work/alembic-test/.venv/bin/alembic", line 8, in <module>
    sys.exit(main())
  File "/Users/<user>/Work/alembic-test/.venv/lib/python3.10/site-packages/alembic/config.py", line 617, in main
    CommandLine(prog=prog).main(argv=argv)
  File "/Users/<user>/Work/alembic-test/.venv/lib/python3.10/site-packages/alembic/config.py", line 611, in main
    self.run_cmd(cfg, options)
  File "/Users/<user>/Work/alembic-test/.venv/lib/python3.10/site-packages/alembic/config.py", line 588, in run_cmd
    fn(
  File "/Users/<user>/Work/alembic-test/.venv/lib/python3.10/site-packages/alembic/command.py", line 385, in upgrade
    script.run_env()
  File "/Users/<user>/Work/alembic-test/.venv/lib/python3.10/site-packages/alembic/script/base.py", line 582, in run_env
    util.load_python_file(self.dir, "env.py")
  File "/Users/<user>/Work/alembic-test/.venv/lib/python3.10/site-packages/alembic/util/pyfiles.py", line 94, in load_python_file
    module = load_module_py(module_id, path)
  File "/Users/<user>/Work/alembic-test/.venv/lib/python3.10/site-packages/alembic/util/pyfiles.py", line 110, in load_module_py
    spec.loader.exec_module(module)  # type: ignore
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/Users/<user>/Work/alembic-test/alembic/env.py", line 78, in <module>
    run_migrations_online()
  File "/Users/<user>/Work/alembic-test/alembic/env.py", line 72, in run_migrations_online
    context.run_migrations()
  File "<string>", line 8, in run_migrations
  File "/Users/<user>/Work/alembic-test/.venv/lib/python3.10/site-packages/alembic/runtime/environment.py", line 928, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/Users/<user>/Work/alembic-test/.venv/lib/python3.10/site-packages/alembic/runtime/migration.py", line 628, in run_migrations
    step.migration_fn(**kw)
  File "/Users/<user>/Work/alembic-test/alembic/versions/cc20a95c17c5_dummy_revision.py", line 20, in upgrade
    op.drop_index("dummy_index_name", "dummy_table_name")
  File "<string>", line 8, in drop_index
TypeError: drop_index() takes 2 positional arguments but 3 were given

Versions.

Additional context

Have a nice day!

Vishal2696 commented 1 year ago

+1. Same here. The upgrade is breaking. Same error too.

zzzeek commented 1 year ago

ah i was almost going to change that one too.

zzzeek commented 1 year ago

Can I just confirm that the migration given is fairly old, op.drop_index("dummy_index_name", "dummy_table_name") should not be the format that we output now. I would need to check when we changed this.

zzzeek commented 1 year ago

hm, OK so, the correct form of this migration:

def upgrade() -> None:
    # This worked on 1.10.4, but broke on 1.11.0
    op.drop_index("dummy_index_name", "dummy_table_name")
    pass

should be this:

def upgrade() -> None:
    op.drop_index("dummy_index_name", table_name="dummy_table_name")
    pass

It looks like the previous syntax was changed in this commit: 20c08069. that was in 2014.

kind of a tossup if we just please ask users to upgrade very old migrations or not, but I'll restore it now anyway.

sqla-tester commented 1 year ago

Mike Bayer has proposed a fix for this issue in the main branch:

restore drop_index.table_name as positional https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4618

bobbypriam commented 1 year ago

Yeah, this is from an old migration. Thank you so much for looking into this.