olirice / alembic_utils

An alembic/sqlalchemy extension for migrating sql views, functions, triggers, and policies
MIT License
193 stars 42 forks source link

gracefully handle when a migrator needs to modify a column that a view depends on #89

Open nstrong-scw opened 2 years ago

nstrong-scw commented 2 years ago

Alembic version: 1.7.7 Alembic_utils version: 0.7.7 db: postgres 12

Here's a basic setup:

-- pretend I have SQLAlchemy classes set up to create these
CREATE TABLE A (
  id integer not null primary_key,
  some_field varchar(255)
);

CREATE VIEW A_VIEW(id, some_field) AS
  SELECT id, some_field FROM A;

Now, suppose you wish to change some_field to a text column. I change the class, run alembic revision -m 'update table' --autogenerate.

Expected behavior: the column should upgrade successfully. Actual behavior: Postgres throws an error: "cannot alter type of a column used by a view or rule"

So, in other words, alembic_utils will successfully recreate views if the view definition is changed, but it doesn't catch when table columns associated with those views get modified.

I've devised a workaround, but it would be nice if it could be done automatically somehow.

Here is my (admittedly naive) workaround to solve this problem in the current code:

# - dependent_view_names is a list of view names that my migration conflicts with
# - lambda_func: a function that does the actual migration steps
def _wrap(dependent_view_names, lambda_func):
    # start by retrieving the views that currently exist at migration time
    existing_views = {v.signature: v for v in PGView.from_database(op.get_bind(), 'public')}
    for view in dependent_view_names:
        if view in existing_views:
            # drop the view and any dependent views
            op.drop_entity(existing_views[view], cascade=True)
    # do the thing
    lambda_func()
    # we replace all views, because the cascade delete may have deleted multiple views.
    for view in existing_views:
        op.replace_entity(existing_views[view])

def upgrade():
    _wrap(['A_VIEW'], op.alter_column('A', 'some_field', existing_type=sa.VARCHAR(length=255), type_=sa.Text(), existing_nullable=True))

def downgrade():
    _wrap(['A_VIEW'], op.alter_column('A', 'some_field', existing_type=sa.TEXT(), type_=sa.VARCHAR(length=255), existing_nullable=True))
olirice commented 2 years ago

There is an experimental context manager for this use case:

from alembic_utils.depends import recreate_dropped

     def upgrade() -> None: 

         my_view = PGView(...) 

         with recreate_dropped(op.get_bind()) as conn: 

             op.drop_entity(my_view) 
             # you could also do `op.execute("drop view myview")` here

             # change an integer column to a bigint 
             op.alter_column( 
                 table_name="account", 
                 column_name="id", 
                 schema="public" 
                 type_=sa.BIGINT() 
                 existing_type=sa.Integer(), 
             ) 

when the context manager goes out of scope, anything you drop gets recreated

Please note that it is not currently part of the public API so it is use-at-your-own-risk

nstrong-scw commented 2 years ago

Thanks for this!

I looked over the code behind recreate_dropped and it looks good to me. It's certainly a nicer way to handle the issue, although it'd still be cool if the need for it could be auto-detected at generate time.

The logging tends to be a little chatty (the "Resolving entities with dependencies" message gets repeated across each round), but that's a very minor complaint.

olirice commented 2 years ago

it'd still be cool if the need for it could be auto-detected at generate time.

the tricky part w/ that is that the alembic migrations need to be executed for the schema diffing alembic_utils uses to work. That would be fine 80% of the time but some failure cases are:

olirice commented 2 years ago

The logging tends to be a little chatty (the "Resolving entities with dependencies" message gets repeated across each round), but that's a very minor complaint

If you'd like to open a PR I'm happy for that log line to more outside the loop

https://github.com/olirice/alembic_utils/blob/5f670039a9a10722c0e5522370d8591831855e54/src/alembic_utils/depends.py#L34

nstrong-scw commented 2 years ago

I took a stab at optimizing solve_resolution_order little bit:

https://github.com/olirice/alembic_utils/pull/90

I'm getting an error KeyError: 'formatters' when I try to run tests. The error is coming from trying to parse the config file (alembic.ini), but I'm not sure why it can't find it. So, I don't know if the tests still pass or not--I'd appreciate guidance.

olirice commented 2 years ago

Here are the steps to run the tests

Requirements:

git clone git@github.com:olirice/alembic_utils.git
cd alembic_utils
python -m venv venv
source venv/bin/activate
pip install -U pip
pip install -e ".[dev]"
pytest

If that fails, please add a comment with:

so i can try to reproduce

nstrong-scw commented 2 years ago

No dice.

pytest.log pip.freeze.txt

As a side note, I'm not sure if it is relevant to the tests failing, but I had to re-pin pypy to the latest version because the old version pulls in a version of check-ast that doesn't build against python 3.9 and later.

olirice commented 2 years ago

unfortunately i can not reproduce that error with 3.10.3 and the lib versions from pip freeze

It looks like its failing to find this key https://github.com/olirice/alembic_utils/blob/master/alembic.ini#L59 in this function https://github.com/olirice/alembic_utils/blob/5f670039a9a10722c0e5522370d8591831855e54/src/alembic_utils/testbase.py#L36

if you track it down I'd be happy to fix. In the meantime, please feel free to open PRs and let CI run the test suite for you as needed (mark as draft)

thanks