Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use cases for finer control over SQLALCHEMY_BINDS (compared to current --multidb flag) #438

Closed
tgross35 opened this issue Sep 28, 2021 · 13 comments
Labels

Comments

@tgross35
Copy link

tgross35 commented Sep 28, 2021

Problem overview

I would like to propose somehow being able to specify the specific bind(s) that Flask-Migrate migrates. Within SQLALCHEMY_BINDS there may be multiple databases; it is possible that Flask-Migrate doesn't have to handle all of them.

A related issue I have is that it migrations start to fail if there is no SQLALCHEMY_DATABASE_URI set for the app. This is acceptable in Flask if all databases are specified in SQLALCHEMY_BINDS, and all models include a __bind_key__. (This is actually preferred in my opinion for multiple binds - by forcing inclusion of a __bind_key__, it lowers risk of a model defaulting to the wrong SQLAlchemy engine and causing problems).

Solution Proposal

I would propose adding a syntax such as flask db init --bind=my_bind_1 --name=bind1, flask migrate bind1, and flask upgrade bind1. This would allow the user to address only needed binds - current behavior of --multidb could still exist for cases where all binds are handled at once.

Solution Notes

If this were implemented, it would make Flask-Migrate more applicable for the following use cases:

  • Multiple binds, some databases managed by another app / DBA outside of Flask
  • No use of SQLALCHEMY_DATABASE_URI for better code integrity/clarity
  • Ability to ignore local Sqlite databases that are created/destroyed at app setup/teardown
  • Allow usage of read-only database URIs where the user doesn't have create table access (for alembic_version table)

I'm curious if anyone else has run into similar issues - I have come across almost all of the use cases at least once.

A current workaround is just to comment out the BINDS item and all imports for unwanted database tracking, but this is not always straightforward.

@miguelgrinberg
Copy link
Owner

You are making several suggestions here. Some I think are valid, while some others can be handled with a custom env.py file.

Multiple binds, some databases managed by another app / DBA outside of Flask

I think this is easily handled by modifying the env.py file, after flask init --multidb generates it. You can remove any binds that you do not want Flask-Migrate to handle there.

No use of SQLALCHEMY_DATABASE_URI for better code integrity/clarity

This one I agree. Flask-SQLAlchemy supports that, so Flask-Migrate should as well. I will look into that.

Ability to ignore local Sqlite databases that are created/destroyed at app setup/teardown

How is this different than the first item? You have a database that you want Flask-Migrate to skip, right? So the env.py can just remove the bind.

Allow usage of read-only database URIs where the user doesn't have create table access (for alembic_version table)

I'm not sure I understand this one. If you are saying that you want to manage migrations in a database where the alembic_version table cannot be written (i.e. this info needs to be written elsewhere), then this is a change that needs to be proposed to Alembic. If you want to skip these databases, then we are back at item 1, where you can tell env.py to skip any databases.

@tgross35
Copy link
Author

Honestly, it did not even occur to me to edit env.py. That essentially solves the issue,

The four items you mentioned - I might not have been clear in my original post, but those are just four use cases I have come across that would be made easier by allowing migration for individual binds. They weren't meant as four unique problems. So yes, editing the env.py solves all of them.

While I would still vouch for some better integration with the command line (maybe the ability to create per-bind env files), it looks like this is pretty much a nonissue.

@tgross35
Copy link
Author

Hm, there seem to be further issues using a custom env.py. flask db heads, flask db show heads, flask db current and flask db show all show the same revision. However, flask db migrate insists that the target database is not up to date. stamp head doesn't change it at all.

Current workaround is to manually manage the revisions files, but this is tedious. I will dig more into where this problem might be coming from.

@miguelgrinberg
Copy link
Owner

@tgross35 What you call a "custom env.py" is no different than a "stock env.py". My guess is that the difference is that the changes that you made to the env.py file are incorrect and are affecting the operating of Alembic.

@xmikew
Copy link

xmikew commented Sep 13, 2023

With using multiple databases, how is one to call stamp, current etc and specify the bind to use? There are no parameter options to these. When using upgrade and downgrade, I can pass in -x/-x-args and have a custom parameter to specify the binds I want to upgrade or downgrade.

I'm sure I'm doing something wrong. I manage one core database that is a different metadata, all those mapped classes specify bind_key as core. The other metadata does not specify a bind_key. This is a multi-tenant app. The tenants all have the same schema. I can't stamp a particular tenant database with a revision without manual update to alembic_version in that particular database(schema)

- Mike

@miguelgrinberg
Copy link
Owner

@xmikew this really is a question that applies to Alembic, not to Flask-Migrate, which just implements the integration between Flask, Flask-SQLAlchemy and Alembic.

Alembic does not natively support migrations for a subset of the databases, nor it supports multi-tenancy. These advanced usages can be implemented with a custom env.py file and the -x option if you need to pass custom arguments. If you Google for multi-tenancy in Alembic you may be able to find some examples of what people have implemented. There is also a recipe for multi-tenancy with Postgres schemas in the official Alembic docs from where you may be able to get some ideas.

@xmikew
Copy link

xmikew commented Sep 14, 2023

Thanks @miguelgrinberg. I do have an functional env.py that parses -x to get the tenant to upgrade/downgrade. However, seems like env.py is not loaded when you run some of the other commands (current etc).

Thanks much for the pointers!

- Mike

@xmikew
Copy link

xmikew commented Sep 19, 2023

Just to follow-up here. Alembic does allow -x to be passed to all commands. So if you run alembic -x db=testdb current the x args will be passed along to env.py. The parser for flask db does not appear to allow this except on upgrade and migrate.

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented Sep 19, 2023

@xmikew I have made a small change to accept the -x argument in the db group instead of in the migrate and upgrade/downgrade commands. Please install the main branch of this repository to test. For example, try flask db -x db=testdb current.

@adityatoshniwal
Copy link

@miguelgrinberg With the release of 4.0.6, this is breaking with error:

Traceback (most recent call last):
  File "C:\pgadmin4\web\pgadmin\__init__.py", line 373, in backup_db_file
    db_upgrade(app)
  File "C:\pgadmin4\web\pgadmin\setup\db_upgrade.py", line 22, in db_upgrade
    flask_migrate.upgrade(migration_folder)
  File "C:\venv\pypg312\Lib\site-packages\flask_migrate\__init__.py", line 111, in wrapped
    f(*args, **kwargs)
  File "C:\venv\pypg312\Lib\site-packages\flask_migrate\__init__.py", line 198, in upgrade
    config = current_app.extensions['migrate'].migrate.get_config(directory,
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\venv\pypg312\Lib\site-packages\flask_migrate\__init__.py", line 96, in get_config
    for x in g.x_arg:
             ^^^^^^^
  File "C:\venv\pypg312\Lib\site-packages\flask\ctx.py", line 54, in __getattr__
    raise AttributeError(name) from None
AttributeError: x_arg

@hfp-ak
Copy link

hfp-ak commented Mar 11, 2024

I can confirm the problem with pgAdmin4 7.8. The update from 4.0.5 to 4.0.6 breaks it as shown above
@miguelgrinberg @adityatoshniwal

@miguelgrinberg
Copy link
Owner

Fix is released. Please report back if there are any remaining problems.

@adityatoshniwal
Copy link

@miguelgrinberg Thank you for the quick fix. I can confirm the error is gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants