flask-security icon indicating copy to clipboard operation
flask-security copied to clipboard

remove dependency on flask-sqlalchemy

Open nitishr opened this issue 9 years ago • 5 comments

It seems better to remove the dependency on flask-sqlalchemy if we can achieve exactly the same functionality without it, using plain old sqlalchemy.

Additionally, the tests now use sqlite in memory instead of file, making the test suite many times faster.

This change is currently backward incompatible, since SQLAlchemyUserDatastore now takes a session from sqlalchemy instead of SQLAlchemy from flask-sqlalchemy. So, SQLAlchemyUserDatastore(db, User, Role) now becomes SQLAlchemyUserDatastore(db.session, User, Role). Can be made easily backward compatible, though, by adding an isinstance check (preferable), or creating a new SQLAlchemySessionUserDatastore.

nitishr avatar Sep 24 '15 18:09 nitishr

I like this idea - I don't really see any upside to depending on Flask-SQLAlchemy, although maybe it brings something to the table here that I'm not aware of.

At any rate, better to leave things decoupled and make fewer assumptions when possible.

fuhrysteve avatar Sep 29 '15 16:09 fuhrysteve

Flask-SQLAlchemy removes a lot of SQLAlchemy boilerplate that I personally know the majority of developers using Flask-Security appreciate. If you want plain-old SQLAlchemy, then there's nothing stopping you from implementing this on your own and providing Flask-Security with your own datastore.

mattupstate avatar Oct 06 '15 17:10 mattupstate

Can't disagree with the benefits of Flask-SQLAlchemy, @mattupstate. And the good thing is, this change has no net-negative or net-positive impact on developers who already use Flask-SQLAlchemy (except for a really small avoidable incompatibility that I'll get to in a moment). It does, however, have a net positive effect for those who don't (want to) use Flask-SQLAlchemy (they won't need to implement their own datastore), and for Flask-Security itself (one less dependency).

Now, about the only difference this makes for existing Flask-Security users: SQLAlchemyUserDatastore needs to be initialized as:

SQLAlchemyUserDatastore(db.session, User, Role)

instead of:

SQLAlchemyUserDatastore(db, User, Role)

I have a fix for this in mind as well, which I'm happy to push as an update to this PR if you think this is useful.

P.S. We can still advertise close integration with Flask-SQLAlchemy, except that it's a no-op ;-)

nitishr avatar Oct 07 '15 05:10 nitishr

This is an excellent idea.

The boilerplate caused by plain SQLAlchemy in a Flask app is really negligible compared to the separation benefits it gives. Thus a lot of people use plain SQLAlchemy. I think it would be beneficial to support it out of the box.

However, backwards compatibility for users of Flask-SQLAlchemy should definitely be maintained at the call site.

raiskila avatar Jun 22 '16 18:06 raiskila

Just pushed a change to maintain call-site backwards compatibility. However, the Travis CI build seems to not have been triggered. Is that because "This branch has conflicts that must be resolved"?

nitishr avatar Jun 30 '16 04:06 nitishr