flask-sqlalchemy

flask-sqlalchemy copied to clipboard

Basic CLI support for Flask-SQLAlchemy, modeled after the CLI support in Flask-Security. Supports the following command:

• flask db create

I don't know how I feel about this. I'd rather people use Flask-Alembic or Flask-Migrate to manage creating/dropping tables.

Also, using db here is problematic as there is no way to say this is lower priority than Alembic/Migrate, so might overwrite those commands.

FWIW, even when using Flask-Migrate and Alembic it's usually a good idea to let SQLAlchemy deal with creating the initial revision. However, I think adding support to call create_all (and at the same time stamp the alembic state to 'head') through the CLI exposed in there would make more sense.

@davidism: It shouldn't be too hard to have Flask-Alembic or Flask-Migrate add commands to the db command group. Just import it and modify it. For example:

try:
    from flask_sqlalchemy.cli import db
except ImportError:
    @click.group()
    def db():
        pass

@db.command()
def migrate():
    # implementation here...

@davidism: I created an example repository that demonstrates how I think we could resolve this problem. It turns out that if you set up both Flask-SQLAlchemy and Flask-Alembic, and they both try to create a command group named db, the second extension will clobber the first. This is actually a good thing: we can re-implement these Flask-SQLAlchemy commands in Flask-Alembic, and do them the Alembic way.

Example repo: https://github.com/singingwolfboy/flask-sqlalchemy-alembic-test

Can you take a look, and tell me what you think of this approach?

But is that order guaranteed? There's no requirement that Flask-Alembic/Migrate is initialized after Flask-SQLAlchemy.

It kind of makes sense to initialize Flask-SQLAlchemy before an extension that depends on it. Doesn't seem to be a hard requirement though (at least not when using init_app())

I guarantee we will get users doing it in the "wrong" order unless Flask-Alembic/Migrate enforce it in init_app.

While I recognize that create/drop commands would be useful for quick apps, for pretty much any serious development I want to encourage people to use Alembic to manage their database. This is partially just a documentation issue. We should have a page that says "create your tables. If you're doing this a lot, use Alembic to manage migrations."

@miguelgrinberg any comments?

Keep in mind that "quick apps" are a major use case for Flask. Many people who want database support in Flask aren't interested in setting up database migrations, because it's too much overhead for what they're looking for.

The argument that this is useful for quick apps isn't strong in my opinion. A quick app will likely use a SQLite database. If I need to drop all tables in a SQLite database, I delete the database file, as that is fully accurate, unlike drop_all() which may miss tables if the database metadata changed from when the tables were created with create_all(). I honestly don't like drop_all() because it isn't implemented to do what you would expect, so I would not expose it more by adding a command for it.

I guess I can see a benefit in having a create command, but I suggest not implementing drop. Instead, I would add a --drop-tables-first option to create for example, which I think makes it a bit more obvious that you are dropping the tables that you are about to create, not the tables that were created long ago.

Would you be willing to update Flask-Migrate so it ensures it's initialized after Flask-SQLAlchemy? That way we guarantee the correct commands are used.

@davidism I assume you mean doing this, right? That's not a problem I can add that.

@miguelgrinberg I hadn't thought about that argument against drop_all(). Between that, and @ThiefMaster's vote against it, I'm OK with removing the db drop command.

@miguelgrinberg I'm going with

def init_app(self, app):
    if 'sqlalchemy' not in app.extensions:
        raise RuntimeError('you need to initialize Flask-SQLAlchemy first')

and adding a create command that just calls upgrade, so that users can't mistakenly call create when Alembic should be managing the tables.

Although overriding the create command on the import probably works too.

Don't some people use SQLAlchemy directly, without using the Flask-SQLAlchemy extension? Do you think that any of them use Flask-Alembic or Flask-Migrate? If so, they probably won't be happy with a mandatory dependency on Flask-SQLAlchemy (although it's easy enough to work around, just by shoving some random object in app.extensions['sqlalchemy']...)

Not sure about Flask-Migrate, Flask-Alembic only works if Flask-SQLAlchemy is installed, I don't currently support other setups.

@davidism If Flask-Alembic already has a hard dependency on Flask-SQLAlchemy, then enforcing the ordering is definitely a good idea.

Flask-Migrate also requires Flask-SQLAlchemy.

@davidism is it necessary to force the init of Flask-SQLalchemy first? Importing the db command should get things in the correct order, I think? And that way I don't have to force users to upgrade Flask-SQLAlchemy if they don't want to, specially considering some deprecated items might be going away soon.

Either way will work, neither requires updating Flask-SQLAlchemy.

I pushed a commit to remove the flask db drop command. Is there anything else that needs to be done here, or can this be merged?

@singingwolfboy The db_drop import in test_cli.py failed the build for some Python versions.

Thanks for the PR ;) it seems quite handy!

The pull request is now passing tests. Is there anything else that needs to be done here, or can this be merged?

Bump here? Also, needs rebase :)

@gaffney thanks for the reminder! I've rebased the pull request.

Note that Travis CI is only failing this pull request because it's trying to build on Python 2.6, which SQLAlchemy no longer supports. #621 should resolve this issue.

Closing for now, this is too outdated. Opened #1090 to summarize and discuss a bit more.