Closed dmckeone closed 9 years ago
Great example. This helped me a lot! The only thing I had to change to get this working was:
from models import User, Base
Glad to hear! I've updated the example with your change.
Wouldn't it be possible to support the Model.query by executing https://github.com/mitsuhiko/flask-sqlalchemy/blob/master/flask_sqlalchemy/__init__.py#L675 for each declarative base during the setup step?
Can you send a pull request for the docs for this?
Ya definitely, I'll try and get something together in the next little while.
@GaretJax You probably could do that, but I know that SQLAlchemy also documents a way to add a similar .query()
property with ScopedSession.query_property()
: http://docs.sqlalchemy.org/en/rel_0_7/orm/session.html#contextual-session-api. Not really sure how this varies from the current solution in Flask-SQLAlchemy, but they seemly conceptually similar.
I'm all for reusing as much as standard SQL alchemy stuff as possible, but consistency with the current flask-sqlalchemy API is good to have as well.
All in all I'm not convinced about some of the design choices of the current version of flask-sqlalchemy (notably the dependency on a single base), but this is probably a different issue.
I'm trying out some stuff in the next days, I have still to make up my mind about all that stuff, I just began using flask.
On Thu, Aug 1, 2013 at 11:10 PM, David McKeone notifications@github.com wrote:
@GaretJax You probably could do that, but I know that SQLAlchemy also documents a way to add a similar .query property with
ScopedSession.query_property()
: http://docs.sqlalchemy.org/en/rel_0_7/orm/session.html#contextual-session-api. Not really sure how this varies from the current solution in Flask-SQLAlchemy, but they seemly conceptually similar.Reply to this email directly or view it on GitHub: https://github.com/mitsuhiko/flask-sqlalchemy/issues/98#issuecomment-21984037
All in all I'm not convinced about some of the design choices of the current version of flask-sqlalchemy (notably the dependency on a single base), but this is probably a different issue.
Agreed. That's also why I'm currently reconsidering how I can fix the library without making the migration process painful for everybody.
I started experimenting on this, the approach I am taking is available here: https://github.com/wsfcomp/flask-sqlalchemy/compare/mitsuhiko:master...master (no working code, just an idea of the API). @mitsuhiko is there an issue or a more appropriate place to discuss the changes for the next release?
I'll hang on for the better place to discuss as well, but I will say that in my ideal world Flask-SQLAlchemy should not dictate the base Model
class at all. Sure it can provide some helpers if the user doesn't care and just wants the Flask-SQLAlchemy session, signals and db.Model paradigm, but if I already have a Declarative Base, and I just want to wire it in to Flask-SQLAlchemy, then it should be possible to just add .query()
and listen for SQLAlchemy events.
I know that we can definitely add .query()
to any existing SQLAlchemy model via ScopedSession.query_property(query_cls=)
, and since you only have to inspect the old db.Model's query_cls
attribute to get the needed argument, I suspect it could be made backwards compatible as well. Additionally, signals could be easily backed in to an existing Model via the SQLAlchemy event system (which wasn't available when Flask-SQLAlchemy was first written).
So it would end being something like below if I wanted to get all the default flask stuff:
flask_app.py
app = Flask(__name__)
db = SQLAlchemy(app)
class User(db.Model):
""" Represents a User """
query_cls = CachingQuery
@app.route("/")
def root():
users = User.query.all()
and something like below if I wanted to just inject my existing stuff into an app.
models.py
Base = declarative_base(cls=CustomBase)
class User(Base):
""" Represents a User """
# etc...
flask_app.py
from models import User, Page, SecretFeature
app = Flask(__name__)
# User.query() would throw an AttributeException here
db = SQLAlchemy(app, query_models=[User, Page, SecretFeature])
@app.route("/")
def root():
users = User.query.all() #Work with Flask-SQLAlchemy's session
Now, this is probably half-baked to some degree, but it does prevent some problems that I had with Flask-SQLAlchemy that eventually forced me to not use it any more.
Advantages:
Disadvantages:
db.Model
-- but then if you need the kind of behaviour that I'm talking about, your aren't going to care, because the only alternative is to forego Flask-SQLAlchemy altogether (like I had to). Also, this proposal wouldn't need to remove the db.Model behaviour -- for those that just want easy, or don't want to dive into the SQLAlchemy deep end.Anyway, food for thought. Happy to carry the conversation on somewhere else, and very happy to see that the discussion is happening. I'd love to bring my stuff back to Flask-SQLAlchemy so that I can integrate with other extensions that expect it.
@dmckeone I agree on almost everything! Maybe instead of registering single models, we can register the declarative base we created (and thus all models at once), as I proposed in the example linked above.
One thing I think is important is to be able to "register" models or bases outside of the constructor and offer an API similar to the one exposed for blueprints (see: http://flask.pocoo.org/docs/blueprints/#registering-blueprints).
P.S.: I would gladly take the time to implement those changes in the next couple of weeks.
@GaretJax Registering the declarative base could be good as well (certainly cleaner to look at, and may even avoid monkey-patch issues). If you register a .query
property on the declarative base would that be used by all the children as well? Does that require using the @declared_attr
behaviours? (http://docs.sqlalchemy.org/en/rel_0_8/orm/extensions/declarative.html#mixing-in-deferred-column-property-and-other-mapperproperty-classes)
Normally it is inherited by the children, but I didn't run the code yet, it's just an example of the API.
I don't know if the models would be usable (but I know for sure that the .metadata attribute changed and with it the different create/drop methods stop working).
@GaretJax Missed part of your comment. Totally agree about blueprints and registering:
from models import Base
db = SQLAlchemy(app)
db.register_base(Base)
Actually it would be really awesome if the .query
attribute used the application context to decide which session was used, so that you could use separate sessions within Blueprints if you wanted to, while using the same model classes.
@dmckeone I like the idea of register_base
.
I think I want to see if i can find the time tomorrow to write up all the problems that people have mentioned with the extension and then figure out a plan how to solve them. In any case I believe by now it makes sense to start from scratch and try to find a migration path. The API exposed is very minimal and should be easy to match for the most part.
There is definitely too much magic in the extension currently.
Any news on this? I'm planning on using Flask + Flask-SQLAlchemy for a new project, but reading this makes me kind of vary on using this extension.
@mitsuhiko any news as to whether or not the refactoring process is going to happen? Is there some way to contribute?
I couldn't use the extension yet. Are there plans on this? I had to implement my own pagination function. :-(
@mitsuhiko, et al: Do you think something like register_base(Base)
is more pythonic than the current form? Would it make db.table
, db.relationship
, db.Integer
, etc. obsolete?
Is there any news about this issue?
@mozillazg I came across this today and dirty patched it for my own usage. Not heavily tested, but would really like this to be eventually merged back into Flask-SQLAlchemy since it's useful if you're using the models in non flask-apps at the same time (or wish to modularise them).
My patch simply adds the method db.register_base(Base)
.
For Example
from models import Base
app = Flask(__name__)
db = SQLAlchemy(app)
db.register_base(Base)
This appears to correctly patch and create_all() seems to work correctly. It also patches the convenience Class.query()
method onto your SQLA classes too.
Closing, inroads being made in #250
Thanks!
The application I was writing required that I have models that were portable (not just for use with Flask), but I still wanted to be able to take advantage of the sessions that Flask-SQLAlchemy provided.
It would be nice if it were documented somewhere that, if you are willing to forego some of the convenience methods on the Model (like Model.query), then you can use standard SQLAlchemy models with the Flask-SQLAlchemy session.
Incidentally this approach also helps with avoiding circular imports when passing the SQLAlchemy() instance, db, around to each model.
Here is an example: