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 [email protected].
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 <[email protected]> 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
