flask-security
flask-security copied to clipboard
remove dependency on flask-sqlalchemy
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.
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.
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.
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 ;-)
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.
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"?