Merge New Models

[gouthambs]

@slippers Lets discuss your PR here.

[slippers]

excellent.

[gouthambs]

Here are some points of discussion:

PROS

• The ORM representation of tables is cleaner
• Flask-Admin support. Though, we should see if it can be currently done with what we have as well.

CONS

• Modifying a whole lot of core code base, and the uncertainty that surrounds the new
• Do we know if things like code migration works (which is not currently tested)
• Do we know if there will be any issues with some one moving from old model to new?

I am generally in favor of improvements, as long as we know we won't break any existing features. So before you change much, lets take a look at what you have currently and make sure we understand it.

[gouthambs]

@slippers
I came across this link that lets you map classes from existing metadata:

http://docs.sqlalchemy.org/en/latest/orm/extensions/automap.html

In principle it seems like we can have models mapped using automap. Perhaps, these models can be readily made available from the existing tables. These models can be used in Flask-Admin.

You mentioned in the email you have some improvements to some queries. I would be happy to bring any improvements over. My preference is to bring small changes, that is easy to track -- only from a stability and maintenance perspective.

Thanks for thinking about the Flask-Admin aspect. It did not strike me when I originally designed it.

[slippers]

@gouthambs thanks for thinking about this feature.

• at this point the only reason to implement this new storage class is if there were a bunch of new tables to add.
• code could be migrated by pointing the current sqlastorage to the new ones. i will have to try it out.
• I will try out a migration. how to test actual query performance improvements?

models from tables

without any changes to existing code the automap_base will extract models. a simple insert into post works via session. i will try this with flask_admin and assume it will work fine.

from sqlalchemy.ext.automap import automap_base
from sqlalchemy.orm import Session

self.storage = SQLAStorage(self._engine, metadata=self._meta)
self._meta.create_all(bind=self._engine)
Base = automap_base()
Base.prepare(self._engine, reflect=True)
Post = Base.classes.post
session = Session(self._engine)
session.add(Post(title='hello'))
session.commit()

improvements to queries

• using session.bulk_save_objects
• doing a single select and then selectively do a update/insert

I tried to simplify this method SQLAStorage._save_tags by making it two methods _save_tag_posts and _save_tags. doing one select query to grab all the existing tags being requested. then use the python set() to find the new ones. at that point a set of Tag models were created and session.bulk_save_objects called. skipped the old tag cleanup step you have.

[gouthambs]

@slippers The changes you have made to the _save_tags looks much cleaner. Do you think you can provide smaller pull requests? The reason is, that makes it easier to comprehend and track should something break in the future. When there are large changes, I always have to weigh about the pros of the new features vs the unknown from a support point of view.

[gouthambs]

@slippers You are right, the automap feature will take care of mapping the tables to models. We could perhaps add a default mapping so users can use that to expose the Models to use with flask-admin. I would prefer to have smaller additions than major changes to the database core implementation.

A more direct approach to test performance of database queries, would be to use jupyter notebook or ipython, and directly see how many queries or inserts are achieved in a second. ipython has magic commands like %%time% or %%prun that you may want to try. But remember all the performance measurements, are database and database configuration specific. For a blog application, reads typically outnumber the writes. So optimizing reads is important. Which is what led me to implement caching in the blog which helps to scale up well. With writes, typically there is only so many in a day, but there can be exceptions to this rule.

[gouthambs]

One thing to keep in mind is that here we allow the database table names to have a prefix attached. The mapping to models would perhaps have to be cognizant of that prefix as well.

[mbrookes]

I'm using MySQL with the database shared between the app and Flask-Blogging, and the above config wouldn't work for me. I had to do this:

    # automap models for Flask-Blogging tables.
    engine = create_engine('mysql+mysqlconnector://root:@localhost/lflask')
    metadata.reflect(engine, only=['post'])
    Base = automap_base(metadata=metadata)
    Base.prepare()
    Post = Base.classes.post

The only issue is that you can't edit tags for posts in Flask-Admin.

[gouthambs]

@mbrookes @slippers
I finally added the automap_base feature to sqlastorage. Now you can import thePost and Tag classes as models and use them in the code.
a3c5266

The usage would be something like this:

storage = SQLAStorage(...)  # configure the connection etc
from flask_blogging.sqlastorage import Post, Tag # Important. This call has to be after the initializing the storage

[gouthambs]

@mbrookes @slippers
Version 0.9.0 adds this feature. Check it out when you get a chance.

[mbrookes]

Oh, nice! I'll be checking that out pronto!

Edit: Hmm... I seem to have the same (or similar) problem with 0.9.0 to that which I had when using this approach: #81 (comment)

sqlalchemy.exc.ArgumentError
ArgumentError: Error creating backref 'purchase_collection' on relationship 'purchase.user_collection': property of that name exists on mapper 'Mapper|user|user'

When I face the before, I had to use this approach from the docs:
#81 (comment)

[gouthambs]

@mbrookes
You can also do this:

storage = SQLAStorage(...)  # configure the connection etc
Post = storage.post_model # the Post model class
Tag = storage.tag_model  # the Tag model class

[mbrookes]

Actually, even just upgrading to 0.9.0 causes that error (with or without my automap code in place).

[gouthambs]

Can you post the stacktrace?

[mbrookes]

Sure:

Traceback (most recent call last):
  File "/Users/Matt/Projects/flask/main.py", line 6, in <module>
    app = create_app(environ.get('FLASK_ENV') or 'development')
  File "/Users/Matt/Projects/flask/app/__init__.py", line 164, in create_app
    admin.add_view(AuthenticatedModelView(User, db.session))
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/flask_admin/contrib/sqla/view.py", line 329, in __init__
    menu_icon_value=menu_icon_value)
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/flask_admin/model/base.py", line 782, in __init__
    self._refresh_cache()
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/flask_admin/model/base.py", line 859, in _refresh_cache
    self._list_columns = self.get_list_columns()
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/flask_admin/model/base.py", line 993, in get_list_columns
    only_columns=self.column_list or self.scaffold_list_columns(),
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/flask_admin/contrib/sqla/view.py", line 415, in scaffold_list_columns
    for p in self._get_model_iterator():
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/flask_admin/contrib/sqla/view.py", line 351, in _get_model_iterator
    return model._sa_class_manager.mapper.iterate_properties
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/sqlalchemy/orm/mapper.py", line 1874, in iterate_properties
    configure_mappers()
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/sqlalchemy/orm/mapper.py", line 2872, in configure_mappers
    mapper._post_configure_properties()
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/sqlalchemy/orm/mapper.py", line 1765, in _post_configure_properties
    prop.init()
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/sqlalchemy/orm/interfaces.py", line 184, in init
    self.do_init()
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/sqlalchemy/orm/relationships.py", line 1656, in do_init
    self._generate_backref()
  File "/Users/Matt/Projects/flask/venv/lib/python2.7/site-packages/sqlalchemy/orm/relationships.py", line 1837, in _generate_backref
    (backref_key, self, m))
ArgumentError: Error creating backref 'purchase_collection' on relationship 'purchase.user_collection': property of that name exists on mapper 'Mapper|user|user'

[gouthambs]

@mbrookes
Thanks. Do you have the flask_admin call after the SQLAStorage? Are you able to post a code snippet or a reproducible example?

[mbrookes]

Do you have the flask_admin call after the SQLAStorage?

Yes.

Are you able to post a code snippet or a reproducible example?

Here you go, sorry it's a bit long, I stripped it back as much as I could whilst keeping the relevant code and enough of the rest for context:

https://gist.github.com/mbrookes/05577989d101fda275c9d5732e5b6303

[gouthambs]

@mbrookes Thanks for providing the gist. I have added a full example to test the Flask-Admin feature in example\blog_admin.py on master.

One thing that I notice is that you automap code in your snippet while the SQLAStorage also has a similar mapping internally. I wonder if there is some sort of clash between the two. One suggestion I have is that you remove all automap in your code and just use the ones from SQLAStorage to see if it works.

[mbrookes]

One thing that I notice is that you automap code in your snippet while the SQLAStorage also has a similar mapping internally.

That's the code that works with 0.8.0. When I said "even without my automap code in place", I meant using the document approach.

Thanks for the example, the only difference was using this:

    # Flask-Blogging database config
    with app.app_context():
        sql_storage = SQLAStorage(db=db)
        db.create_all()
    blog_engine = BloggingEngine(app, sql_storage)

instead of that:

    # Flask-Blogging database config
    with app.app_context():
        sql_storage = SQLAStorage(db=db)
        db.create_all()
        blog_engine = BloggingEngine()
        blog_engine.init_app(app, sql_storage)

(What's the difference between those two incantations anyway?)

The error is the same either way (although strangely on a different table this time.

[mbrookes]

Here's a minimal example that exhibits the problem: https://github.com/mbrookes/blogging-example

(I stripped back a bigger app, so it's incomplete in places, but enough to arrive at the error...)

[gouthambs]

@mbrookes
Thanks for providing this example. This was helpful. I have a fix in version 0.9.1 I just released. Feel free to check it out to see if it works.

[mbrookes]

@gouthambs Thanks for looking at this, and for working with an incomplete app!

So, how do I now access the Post and Tag models?

Neither:

 from flask_blogging.sqlastorage import Post, Tag

nor

Post = storage.post_model  # the Post model class
Tag = storage.tag_model  # the Tag model class

seem to work.

ImportError: cannot import name Post and
AttributeError: 'SQLAStorage' object has no attribute 'post_model' errors respectively.

I've tried both ways of initialising BloggingEngine, combined with both ways of assigning / importing the models, but no difference in outcome.

Thanks!

[gouthambs]

@mbrookes
There is a example in example\blog_admin.py that shows the usage. Can you check you have the latest version (0.9.1)?

[mbrookes]

@gouthambs Okay, that's really strange - I would swear I was running 0.9.1, but when I tied again today, rather than the import / attribute errors, I'm back to the same error as 0.9.0.

I've updated the example to make it complete and simpler to run (uses sqlite).

It works as is. Upgrade flask-blogging to 0.9.1 and it will break, downgrade, it will work agian...

[gouthambs]

@mbrookes Thanks for creating an updated example. I have committed a fix to the master also available as version 0.9.2. I tested with your example and it worked with the new fixes. Hopefully this works out on your end as well!

[mbrookes]

@gouthambs Fantastic, that's working perfectly now. Thanks for your perseverance and quick turnaround!

This release does seem to break the tag cloud plugin, but I'll open a separate issue for that.