sqlalchemy / alembic

A database migrations tool for SQLAlchemy.
MIT License
2.91k stars 250 forks source link

Example change for branched connection still doesn't work #922

Open RazerM opened 3 years ago

RazerM commented 3 years ago

Describe the bug From https://alembic.sqlalchemy.org/en/latest/cookbook.html#sharing-a-connection-with-a-series-of-migration-commands-and-environments there is this snippet:

def run_migrations_online():
    connectable = config.attributes.get('connection', None)

    if connectable is None:
        # only create Engine if we don't have a Connection
        # from the outside
        connectable = engine_from_config(
            config.get_section(config.config_ini_section),
            prefix='sqlalchemy.',
            poolclass=pool.NullPool)

    context.configure(
        connection=connectable,
        target_metadata=target_metadata
    )

    with context.begin_transaction():
        context.run_migrations()

but connectable is then an engine when there isn't one in the config. The error is:

'connection' argument to configure() is expected to be a sqlalchemy.engine.Connection instance, got Engine(postgresql://pydiscos3test:***@postgres/discos3)

Versions.

Additional context

Have a nice day!

zzzeek commented 3 years ago

sure. can you assist with an update? I believe we can make this smooth using https://docs.python.org/3/library/contextlib.html#contextlib.closing :

import contextlib

def run_migrations_online():
    connectable = config.attributes.get('connection', None)

    if connectable is None:
        # only create Engine if we don't have a Connection
        # from the outside
        connectable = engine_from_config(
            config.get_section(config.config_ini_section),
            prefix='sqlalchemy.',
            poolclass=pool.NullPool).connect()
    else:
        connectable = contextlib.closing(connectable)

    with connectable:
        context.configure(
            connection=connectable,
            target_metadata=target_metadata
        )

        with context.begin_transaction():
            context.run_migrations()
RazerM commented 3 years ago

Wouldn't it need to call .begin() on the result from engine_from_config?

zzzeek commented 3 years ago

context.begin_transaction() should do that

it goes here: https://github.com/sqlalchemy/alembic/blob/master/alembic/runtime/migration.py#L437 then here: https://github.com/sqlalchemy/alembic/blob/master/alembic/util/sqla_compat.py#L114

so it is approximating some of "branched" there