Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manage external declarative bases #250

Closed
wants to merge 1 commit into from

Conversation

nickw444
Copy link
Contributor

@nickw444 nickw444 commented Jan 4, 2015

Fix for Issue #98

@svenstaro
Copy link
Contributor

Can you make a test for this can perhaps document it a little?

@davidism
Copy link
Member

Overwriting self.Model here isn't correct, as this method will be called after the original Model has been created and could potentially have been used. Instead, the correct way to implement this is probably to support an arbitrary number of bases.

@immunda
Copy link
Contributor

immunda commented Feb 9, 2015

@nickw444 Any word on documentation or dealing with this in the way @davidism suggests?

@davidism
Copy link
Member

davidism commented Feb 9, 2015

It might be acceptable to just not overwrite self.Model, since references to the external bases are (by definition) external.

Another issue is that this will blindly set query_class and query, rather than checking if they each exist on the target model first.

Overall the idea is fine, it just needs some more work first.

@nickw444
Copy link
Contributor Author

nickw444 commented Mar 6, 2015

Hello All, I have been on an extended break, Over the coming days I will correctly implement this patch, write the tests, and make the required modifications for this to be pulled.

@immunda
Copy link
Contributor

immunda commented Mar 6, 2015

Great, look forward to it! 👍

@nickw444
Copy link
Contributor Author

nickw444 commented Mar 6, 2015

Good progress, taking @davidism's advice, I no longer override self.Model, since indeed, this should be left for Flask-SQLA's internal model.

Instead, registering a base will add it to reference in self.external_bases. Any requirement to access the base should indeed, be performed by doing it directly, rather than via flask-sqla.

For convenience, I have added a property called bases which returns a list, containing the internal self.Model and any additional registered bases.

This pull requests supports an arbitrary number of bases, since all bases, including the internal are available as an iter property (self.bases). Operations such as _execute_for_all_tables now perform for each base registered, including the internal self.Model.

I have added some additional test cases for this pull request, however I have disabled one, as I cannot find a solution to fix this minor problem when using raw SQLA. - When using a relationship (dynamically mapped), the query class is SQLA's own (obviously), however I am unable to re-construct this at runtime to patch it to use Flask-SQLA's own patched BaseQuery with convenience methods (pagination, etc). Only minor for now, but is something worth pointing out. I believe a solution for this is to set the session's default query class, however I did not want to do this as this may have unwanted side affects.

Hopefully we can get this merged soon, cloning my fork via pip makes me nervous every time :P 👍

@gawry
Copy link

gawry commented Mar 15, 2015

Is this going to be merged? It would be great to be able to use raw sqlalchemy models with flask

@pahaz
Copy link

pahaz commented Apr 17, 2015

Hi, find some problem with this patch:

ExtraSQLAlchemy class - is the same as your patch.

# https://github.com/omab/python-social-auth/blob/master/examples/flask_example/models/user.py
from social.apps.flask_app.default.models import init_social
from social.apps.flask_app.default.models import PSABase
from flask.ext.sqlalchemy import SQLAlchemy, itervalues, string_types, \
    BaseQuery, _QueryProperty
from pprint import pprint


class ExtraSQLAlchemy(SQLAlchemy):
    def __init__(self, app=None, use_native_unicode=True,
                 session_options=None):
        super(ExtraSQLAlchemy, self).__init__(app, use_native_unicode,
                                              session_options)
        self.external_bases = []

    def get_tables_for_bind(self, bind=None):
        """Returns a list of all tables relevant for a bind."""
        result = []
        for Base in self.bases:
            for table in itervalues(Base.metadata.tables):
                if table.info.get('bind_key') == bind:
                    result.append(table)
        return result

    def _execute_for_all_tables(self, app, bind, operation, skip_tables=False):
        app = self.get_app(app)

        for Base in self.bases:
            if bind == '__all__':
                binds = [None] + list(app.config.get('SQLALCHEMY_BINDS') or ())
            elif isinstance(bind, string_types) or bind is None:
                binds = [bind]
            else:
                binds = bind

            for bind in binds:
                extra = {}
                if not skip_tables:
                    tables = self.get_tables_for_bind(bind)
                    extra['tables'] = tables
                op = getattr(Base.metadata, operation)
                op(bind=self.get_engine(app, bind), **extra)

    @property
    def bases(self):
        return [self.Model] + self.external_bases

    def register_base(self, Base):
        """Register an external raw SQLAlchemy declarative base.
        Allows usage of the base with our session management and
        adds convenience query property using BaseQuery by default.
        """
        self.external_bases.append(Base)
        for c in Base._decl_class_registry.values():
            if isinstance(c, type):
                if not hasattr(c, 'query') and not hasattr(c, 'query_class'):
                    c.query_class = BaseQuery

                if not hasattr(c, 'query'):
                    c.query = _QueryProperty(self)


app = Flask(__name__)
app.config.from_object('config')                        

db = ExtraSQLAlchemy(app)
init_social(app, db.session)
db.register_base(PSABase)

pprint([x.name for x in db.metadata.sorted_tables])
pprint([x.name for x in PSABase.metadata.sorted_tables])

# 
# output:
# 
# ['user']
# ['social_auth_nonce',
#  'social_auth_association',
#  'social_auth_usersocialauth',
#  'social_auth_code']


# will work incorrect!
from flask.ext.migrate import Migrate
migrate = Migrate(app, db)

As you can see, the db.metadata.sorted_tables is different for PSABase and db.Model classes.
Because of this if I use the Flask-Migrate or something witch work with db.metadata I will get an unexpected behaviour.

@nickw444
Copy link
Contributor Author

@pahaz, The way this patch is implemented is that it works together with the Flask-Sqlalchemy's builtin declarative base - It does not actually replace the original base. This means you can use a hybrid of both SQLA models and Flask-SQLA models. Additionally, you can register multiple bases.

Hence when needing to access the sorted_tables property, you must do this via each of your declarative bases individually. Alternatively, a property which returns all the bases registered can be found on db.bases.

@davidism
Copy link
Member

I think migrations need to be split up per metadata for Alembic anyway. That should probably be something Flask-Migrate handles, like it handles tying in the one metadata right now.

@nickw444
Copy link
Contributor Author

I agree with @davidism on that point

@tony
Copy link

tony commented May 12, 2015

Yep Houston we have a problem. I need this.

I love flask, but I'm missing the ability to scan and inject the connection data into the objects...

@nickw444 You still on this? I'm going to take a look at how django / django orm does it, then see how this compares.

@nickw444
Copy link
Contributor Author

@tony I'm still on this, we're just waiting for this to be pulled back. For the time being my fork works flawlessly, I'm using it in about 5 projects

@tony
Copy link

tony commented May 12, 2015

@nickw444 Can you rebase / squash?

I'm going to take a deeper look and give it a QA

@tony
Copy link

tony commented May 12, 2015

@immunda , @davidism : This is working great for me.

@nickw444: Now it's being used on another project. Thanks for adding tests too.

@makmanalp
Copy link

@tony thanks for the heads up! @nickw444 thanks for the patch! I'll also try running with this flask version and see what happens.

@nickw444
Copy link
Contributor Author

@pahaz I recently needed to use Flask-Migrate with a project with this setup with models. In the alembic env.py you need to change the target_metadata variable to point to your actual Base's metadata. By default it's set to current_app.extensions['migrate'].db.metadata, which is just the builtin Flask-SQLA metadata.

@nickw444
Copy link
Contributor Author

nickw444 commented Jun 9, 2015

@tony Are you reviewing on behalf of @mitsuhiko / other devs of this repo? I'll squash now if that's the case.

Thanks!

@nickw444
Copy link
Contributor Author

@immunda ETA on this being merged/v3? Also am I squashing the changes?

@makmanalp
Copy link

Just an update: have been using this for a while now and no trouble so far.

@sterwill
Copy link

sterwill commented Aug 6, 2015

Any update on when this might get merged? I'd love to be able to remove my models' dependencies on a single module-level "db" (and all the initialization and testing headaches that it brings) and just use plain SQLAlchemy models.

@charliewolf
Copy link

I would also like to know when this is being merged

@immunda
Copy link
Contributor

immunda commented Sep 21, 2015

I'll be releasing 2.1 this week. After which, I'll merge this into master and review what else needs to be included for 3.

@makmanalp
Copy link

Thank you!

@EliFinkelshteyn
Copy link

+1 on this. I currently have to keep two copies of the same models-- one for SQLAlchemy and one for Flask-SQLAlchemy. Merging this would remove a huge headache. Thanks for doing this @nickw444.

@jrhite
Copy link

jrhite commented Oct 11, 2015

@immunda any news on the 2.1 release?

i've also recently run into this problem. i'm using scrapy to scrape data and store models into db via sqlalchemy. later i use flask to serve up the models. scrapy shouldn't/doesn't need to be tied into flask. i want to use the sqlalchemy models in both places.

poked around, but haven't seen any 2.1 yet.

thanks!

makmanalp added a commit to cid-harvard/atlas-subnational-api that referenced this pull request Jul 11, 2016
makmanalp added a commit to cid-harvard/atlas-subnational-api that referenced this pull request Jul 11, 2016
makmanalp added a commit to cid-harvard/atlas-subnational-api that referenced this pull request Jul 11, 2016
@nzjrs
Copy link

nzjrs commented Aug 11, 2016

I'd like to use this but am wary of jumping in if the migration / alembic story isn't trustworthy. Is there an Eta for merge and maybe the next flask-sqlalchemy release?

@davidism
Copy link
Member

davidism commented Aug 11, 2016

There is no ETA. There is also nothing "untrustworthy" about using Flask-SQLAlchemy / Alembic / other base models right now, you're just not going to get automatic integration with Flask-Migrate (which we don't control anyway) or some of the helpers from Flask-SQLAlchemy in your external base.

External bases can already be patched in to the env.py script from Alembic and used with db.session. Flask-SQLAlchemy and this patch aren't standing in your way in that regard.

@nickw444
Copy link
Contributor Author

@nzjrs from my experience (running this PR for over a year and a half now in production), I've never had any issues with Alembic. I use flask-migrate the normal way, and just add import models (or whatever your models python module is) to the .mako template.

@mitsuhiko
Copy link
Contributor

@nickw444 sadly this PR will not work with multiple applications safely due the query class set to an application specific one. Same applies to the query attribute.

@davidism
Copy link
Member

@mitsuhiko this PR doesn't change the query class, unless I'm overlooking something.

@mitsuhiko
Copy link
Contributor

@davidism looking at the code register_base sets the model query_class and query to self.Query and _QueryProperty(self).

@davidism
Copy link
Member

But those are extension specific, not app specific. Same with the model, metadata, session class, etc.

@mitsuhiko
Copy link
Contributor

That however then means that you have to use the model with that instance of the extension only. If that is intended than a security check with an assertion should be added. People should not assume that this change means they can make one instance of this extension per app then.

@davidism
Copy link
Member

Ah, ok. There's probably a way to make this work.

@devyash
Copy link

devyash commented Mar 24, 2017

Hello, I am facing this exact problem. I auto generated an existing Mysql schema using Sqlacodegen, but the thing is, I wish to use Flask-SQLAchemy instead pure SQLAlchemy. This patch would be really helpful. I will try to look at it and see if I can contribute.

@melchoir55
Copy link

If you need it today, you can manually patch it in by overriding one class. I have posted an example to SO here: http://stackoverflow.com/questions/28789063/associate-external-class-model-with-flask-sqlalchemy

The only complication I have had is with flask-migrate. I had to change the generated env script to pull from the metadata on the external base.

diegodelemos pushed a commit to diegodelemos/reana-workflow-engine-yadage that referenced this pull request Oct 19, 2017
* Adds models using plain SQLAlchemy since
  pallets-eco/flask-sqlalchemy#250 looks like it won't be merged soon.

Signed-off-by: Diego Rodriguez <diego.rodriguez@cern.ch>
diegodelemos pushed a commit to diegodelemos/reana-workflow-engine-yadage that referenced this pull request Oct 19, 2017
* Adds models using plain SQLAlchemy since
  pallets-eco/flask-sqlalchemy#250 looks like it won't be merged soon.

Signed-off-by: Diego Rodriguez <diego.rodriguez@cern.ch>
diegodelemos pushed a commit to diegodelemos/reana-workflow-engine-yadage that referenced this pull request Oct 19, 2017
* Adds models using plain SQLAlchemy since
  pallets-eco/flask-sqlalchemy#250 looks like it won't be merged soon.

Signed-off-by: Diego Rodriguez <diego.rodriguez@cern.ch>
diegodelemos pushed a commit to diegodelemos/reana-workflow-engine-yadage that referenced this pull request Oct 19, 2017
* Adds models using plain SQLAlchemy since
  pallets-eco/flask-sqlalchemy#250 looks like it won't be merged soon.

Signed-off-by: Diego Rodriguez <diego.rodriguez@cern.ch>
@daviddpark
Copy link

My team would also like to be able to define SQLAlchemy models in a separate package from the flask app and import the package into both Flask based and applications without a Flask dependency. This seems to be a commonly requested feature.

@mitsuhiko, is there a better approach that you can recommend to enable using SQLAlchemy models that were not defined extending a Flask session's base model?

@gtalarico
Copy link

gtalarico commented Mar 29, 2018

@daviddpark

It's possible to use SqlAlchemy models outside a flask app; I have been using it!

I used sqlalchemy declarative_base() as a Base Class instead of db.Model and pass this object as a model_class to the Flask-SqlAlchemy constructor.

Maybe we can extend the documentation to include this?

It took me some time to get this to work,
but I arrived at the solution below after stitching together everything I could find on this topic.
PS: Sample code not tested, there might be some mistakes : )

models.py - common models to be used by flask app and outside application

from sqlalchemy.ext.declarative import declarative_base
Base = declarative_base()

class User(Base)
    pass

flask_app.py - Flask-SqlAlchemy Handles all necessary setup

from flask import Flask
from flask_sqlalchemy import SQLAlchemy

from models import User

db = SQLAlchemy(model_class=Base)
app = Flask(__name__)
db.init_app(app)
# 

@app.route('/add')
def add_user():
    user = User()
    db.session.add(user)
    db.session.commit()

Using outside of flask requires a few simple steps. I keep those in a dedicated create_session function

  1. Create Engine + Session
  2. Add Query Property to Models

external_application.py

from sqlalchemy import create_engine
from sqlalchemy.orm import scoped_session, sessionmaker

from models import Base

engine = create_engine('postgres://url')
session = sessionmaker(bind=engine)
db_session = scoped_session(session)

# Adds Query Property to Models - enables `User.query.query_method()`
Base.query = db_session.query_property()

# Create Tables
Base.metadata.create_all(bind=engine)

user = User()
db_session.add(user)
db_session.commit()
print(User.query.all())

@reritom
Copy link

reritom commented Jul 31, 2020

Did anything come from this? Three years of development and discussion and then it was closed?

@melchoir55
Copy link

Did anything come from this? Three years of development and discussion and then it was closed?

@gtalarico's comment above is a code example for how to do this today

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2020
@davidism davidism removed this from the 3.0 milestone Sep 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet