Should the SQL Storage be executing db.create_all()?

[ashcrow]

From #22 (comment) by @gouthambs

Another thing is I feel the tables should not be created by this storage class.
Explicit invocation of db.create_all() should take care of that. Thats a small
change in usage.

This is consistent with how Flask-SQLAlchemy models just create the table
instances in the meta, but the explicit invocation of db.create_all() creates
the tables.

My initial thought is it does make sense to execute db.create_all() if the tables to not already exist. If they do exist the plugin should step back and let the admin/programmer do what needs to be done.

Right now if a user passes in a db instance they could easily execute db.create_all() when they are ready. However, if they are using conn_str method of setting up they wouldn't have access to do so.

However, I'm open to changing it as long as we can do it in a way that doesn't force the SQL Storage to be somewhat special. We could add meta as a public variable which could be used.

@gouthambs Thoughts?

[gouthambs]

@ashcrow
My feeling is that SQL Storage should behave like any Flask-SQLAlchemy model. In a Flask-SQLAlchemy ORM model, the db.create_all() explicit call is needed to create the tables. In case the tables already exist, then nothing is created.

Another thing to keep in mind is that people use Alembic to explicitly manage migrations. Here is a very good example:
http://blog.miguelgrinberg.com/post/flask-migrate-alembic-database-migration-wrapper-for-flask

Here you will see that migrate call generates the table difference. An explicit upgrade call is needed to modify the tables.

While using migrations, I was having trouble with SQLStorage because the existence of the tracking tables was not recognized by Alembic. The metadata fix that we did earlier will address that. However, the expectation that upgrade call is when tables are modified no longer holds true. The scariest thing is now that we share the metadata, not only track tables are created, but everything else that user may have initialized would get created. The table modification is not in the user control anymore as is expected because SQL Storage goes and creates it behind the scene. This is a very inconsistent behavior. I wish I had known better! :)

[gouthambs]

Another suggestion, is to remove connection string argument, and instead allow engine, metadata, and db as arguments. If someone passes SQLAlchemy db object, then we take care of using the metadata from it. Other wise, the user has to pass engine and metadata created from SQLAlchemy directly.

We shouldn't create metadata at all. This is the most flexible. Because, someone may not have the SQLAlchemy db object, because they are using SQLAlchemy directly. In which case, the user would maintain a global metadata object for their tables. This modification allows the user to pick the SQLAlchemy approach (pass engine and metadata) or Flask-SQLAlchemy approach (pass db).

[ashcrow]

I was starting to think along the same lines. What if the we make the db input assume that the user will manually take care of creating tables, etc.. . But if user only passes in the connection string we assume they want the automatic creation and ownership? Would that end up working for your use case (while still retaining some backwards compatibility)?

Do it for me example

# ...
# I want the Storage to handle all the internals for me
SS.set_up(conn_str="slite:///tmp/db.sqlite"):
# Done ..

I want control example*

# ...
# I want more control
SS.set_up(db=db)
# Do things here
db.create_all()
# Done

[gouthambs]

@ashcrow If compatibility is very important to you then we can try along the lines of what you have.

I have noticed that connection string alone is insufficient to create an engine (some dbs take extra args which have to passed into create_engine method). I am leaning towards breaking compatibility in favor of better design. We can supplement docs by adding compatibility notes.

What do you think?

[ashcrow]

Okay you convinced me :-)

[gouthambs]

Awesome! Will give you a PR.

[ashcrow]

@gouthambs sounds good!