Re: [Savannah-hackers-public] attempt at savannah web interface in python/flask

[Michal Grochmal]

Hello Assaf,

> Many thanks for looking at the code and writing suggestions.
> Much appreciated. Please do feel free to write more,
> and write publicly to address@hidden

Ooops, sorry for that.  That was actually a mistake, I've typed 'r' instead of
'g' (group reply).

> I welcome the discussion and suggestions.
> 
> > On Mar 30, 2017, at 17:03, Michal Grochmal <address@hidden> wrote:
> > 
> > I'd do two things a little differently.  One is the search form.  I believe
> > that:
> > 
> >    SearchForm(request.args,csrf_enabled=False)
> > 
> > Breaks in some cases (at least had issues with csrf_enabled=False several 
> > times
> > myself).  Instead I always use the Meta class as in:
> > 
> >   class SearchForm(Form):
> >       # (...)
> >       class Meta:
> >           csrf = False
> 
> Good tip. Is this documented somewhere? I did see "@csrf.exempt", but haven't
> encountered your method before.

That is more-or-less how I use it but I did found the documentation too.  The
WTForms actually use a `SearchForm` as an example in their docs about CSRF
protection:
http://wtforms.readthedocs.io/en/latest/csrf.html

You can even overwrite it in a subclass (I didn't know that, learned something
new today)

> > A search should be completely idempotent so there should never be a need for
> > CSRF protection.  Unless you are monitoring users, but, as far as I'm aware,
> > monitoring users is against FSF philosophy :)
> 
> Agreed.
> 
> > # Comment 2
> > 
> > This is more a suggestion.
> > 
> > Since the reason for this spin-off project is that savane code is quite 
> > cranky
> > in some places and is a single big bulk of code that is hard modernise.  
> > i.e.
> > you cannot pick a part of savane code and change only that.
> 
> and worse: to have any chance of actually succeeding with this python/flask
> version, keeping the database structure exactly as-is is required. :(
> So while I can rewrite the code from scratch, I can't modify the tables
> or the relationships (or silly things like "group_id=100" being the equivalent
> of NULL).
> 
> > I'd avoid flask-sqlachemy (the package that glues both libs together) for 
> > that
> > reason.  flask-sqlalchemy forces a couple of conventions that make the glue
> > between both libs nicer but also make the code bulkier.  To not fall in the
> > same problem as the current savane I'd make two packages, one for the 
> > database
> > and one for flask.  Similar to what the falsk docs do in:
> > http://flask.pocoo.org/docs/0.12/patterns/sqlalchemy/#declarative
> > 
> > Say, varanusex-db and varanusex-web.
> > 
> > Then you import the models into the views (in varanusex-web) from 
> > varanusex-db.
> > This will allow, for example, placing a different framework on top of the
> > database or even (with some effort) changing the ORM implementation.
> 
> Can you elaborate about this ?
> What would be included in 'varanusex-db' , and what in 'varanusex-web' ?
> 
> Currently, all ORM classes are under "./varanusex/models/*.py",
> and they all inherit from "db.Model" .
> 
> There's one unused file called './varanusex/models/savane-model.py' - that
> file contains the basic ORM classes, and I generated it automatically using
> 'sqlacodegen' (which reads an existing MySQL database and creates the boiler
> plate ORM classes, instead of having to type them by hand). But again, this
> file is unused - it is just there so I can copy&paste the classes and adjust
> as needed.
> 
> I'm following the structure that's recommended in the book "Flask Web
> Development":
>  http://shop.oreilly.com/product/0636920031116.do
> 
> What would you recommend to change ?

That's an interesting book actually, in my time I needed to learn from the
docs.  Will be useful to me to train some people through, thanks for that.  I
have looked through the ToC there and saw that it explains `pip`, so, i guess,
i'm free to elaborate the above using some `pip`.

On topic: in the aforementioned `varanusex-db` I'd place the session (database
session) management (instead of relying on `flask-sqlalchemy` which simply uses
`scoped_session` anyway).  In other words inside `models/savane-models.py` I'd
start with

    # this is already there
    Base = declarative_base()
    metadata = Base.metadata
    # this is added
    db_session = scoped_session(sessionmaker())
    Base.query = db_session.query_property()

Now the models directory would be the `varanusex-db` package (and the rest,
views, filters, static and templates stay in the current package) with its own
`setup.py` e.g. it would contain:

    README
    setup.py
    varanusex-db/
    varanusex-db/savane-models.py
    varanusex-db/(...)  # the other files in models

And I'd avoid the `db.Models` altogether in the other files.  In other words,
I'd avoid the following (from __init__.py):

    db = SQLAlchemy()  # since this comes from flask-sqlalchemy
    db.init_app(app)

Instead an:

    from .savane-models import Base, db_session

Would pick the declarative class out of where we defined it, with the metadata
but without a db engine.  Then, defining a model, say:

    class BugCC(<some mixins perhaps>, Base):

Would allow it to be also taken from the file using similar imports.  And
moreover, would allow for binding the engine much, much later.  Imagine that we
have all that inside `varanusex-db` and have a `setip.py` akin of:

    from setuptools import setup
    setup(name='varanusex-db', packages=['varanusex-db'])

So we install it with say:

    pip install --user -e .

And in a *different* package (say `varanusex-web` or the current `varanusex`
without the models directory) we can do:

    from flask import Flask
    from sqlalchemy import create_engine
    from varanusex-db.savane-models import BugCC, db_session

    app = Flask(__name__)
    # ...
    engine = create_engine(app.config.get('DATABASE_URI'))  # or other name
    db_session.configure(bind=engine)
    BugCc.query.filter(group_id=3).one()  # extra note about this below

Note that the engine has been defined only in the package that deploys the WSGI
object.  This means that `varanusex-db` is independent from database and from
web framework.  So substituting the above `app = Flask()` for, say, Plone would
allow to have a different framework using the same ORM configuration.

And since `scope_session` uses thread ids as the default session separator, and
on Linux TIDs are always unique, the session would not intermingle themselves.
(That is, unless you do something very esoteric like Tornado framework).

That last line there (`BugCc.query`) is the same trick that the
`flask-sqlalchemy` package does when you subclass from `db.Model`.  Therefore
you can use it that same way as, for example, `Group.query` is used in the
views.

# Summary

Laying it like the above disengages sqlalchemy from flask, by dumping all
sqlalchemy code (but the engine) into a package of its own (varanusex-db) which
can then be called from flask, from a different framework if the need arises,
or from CLI scripts.  And, thanks to the fact that the database session
management is in that package, there is no way that, even if you have several
frameworks and cli script running concurrently and calling the same package,
you corrupt a session/transaction.

If a new framework needs to be added to savane later, the DB layer can be kept
and only the flask part needs to be changed.  This should be a good step
forward compared to the current PHP monolith.

On the other hand I'm a pretty bad PHP programmer, so I probably shouldn't talk
much about the PHP code.

Apologies for the long digression.  Don't feel obliged in any way into
following this.  This, again, is more a design opinion from someone that uses
flask for some time.

Best,
-- 
Mike Grochmal
GPG key ID 0xC840C4F6