CLI support

[singingwolfboy]

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

• flask db create

[ThiefMaster]

IMO it'd be better to just drop support for anclient flask versions

[singingwolfboy]

Flask-CLI supports Flask 0.10, and CLI support was added to Flask in 0.11. Flask 0.10 was released in June 2013, while Flask 0.11 was released in June 2016. Three years is hardly ancient history, even in software development.

Besides, the cost of supporting Flask-CLI is so low. Is it really a problem to keep this in?

[ThiefMaster]

No, just my opinion :) And I'd say using a 3y old version from a 0.x line is kind of ancient.

[ThiefMaster]

I would go a step further and completely disallow running this unless the app is in debug mode. It's something you pretty much never need outside development.

[singingwolfboy]

I see where you're coming from -- this is a potential footgun, and we should be careful with it. At the same time, I can imagine circumstances where it has a use with debug mode turned off. For example, it's reasonable to have an automated test flow where the code is automatically checked out, setup, run, and torn down, and where debug mode is specifically turned off, in order to make the test more consistent with a production environment. There's no reason not to support that flow, and I think that having a confirmation step will deter most human error.

[ThiefMaster]

You can always call it with FLASK_DEBUG=1 in that case

[ThiefMaster]

Besides that, creating/dropping database tables for tests should usually be part of the test setup (e.g. pytest fixtures).

[singingwolfboy]

I'm talking about a testing flow that isn't necessarily in Python. For example, a Jenkins worker that sets up the codebase and then runs Selenium tests. That sort of scenario would generally be scripted in bash, not in Python.

I hadn't thought about FLASK_DEBUG=1 -- but if anything, that makes me even less keen to make this change. The confirmation option can be bypassed by running flask db drop --yes -- what makes you think it's any more difficult to just run FLASK_DEBUG=1 flask db drop --yes instead?

I'm all for avoiding footguns, but this smacks of child-proofing that is too easily disabled to be useful. I believe we should make sure that people are aware of the risks of what they're doing (hence the confirmation option), and then trust that they're smart enough to do the right thing.

[davidism]

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.

[ThiefMaster]

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.

[singingwolfboy]

@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...

[singingwolfboy]

@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?

[davidism]

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

[ThiefMaster]

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())

[davidism]

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?

[singingwolfboy]

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.

[miguelgrinberg]

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.

[davidism]

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.

[miguelgrinberg]

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

[singingwolfboy]

@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.

[davidism]

@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.

[singingwolfboy]

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']...)

[davidism]

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

[singingwolfboy]

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

[miguelgrinberg]

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.

[davidism]

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

[singingwolfboy]

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?

[pavelpascari]

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

Thanks for the PR ;) it seems quite handy!

[singingwolfboy]

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

[gaffney]

Bump here? Also, needs rebase :)

[singingwolfboy]

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

[singingwolfboy]

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.

[davidism]

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