Decide what new SQLAlchemy features should be rolled into Sideboard

[EliAndrewC]

Over the long weekend I made a lot more progress migrating the MAGFest code to use SQLAlchemy. Since it was previously a Django app, there were a lot of Django idioms I needed to port over. Some of these might be generally useful for Sideboard, others should probably be left up to plugins to define for themselves.

For the sake of having a record of these things, I'm going to keep a running list in this ticket. Anything that seems like a good idea to include in Sideboard itself should have a separate issue created for it.

1. Sideboard ships with a UTCDateTime class, and our documentation explains that you can just set a client-side default, e.g.

created = Column(UTCDateTime, default=lambda: datetime.now(UTC))``

The problem with this approach is that the timestamp is set when the model is instantiated rather than when it's saved. There's no easy way to set a server-side default. SQLAlchemy actually documents an approach for this (http://docs.sqlalchemy.org/en/rel_0_9/core/compiler.html#utc-timestamp-function), in the same way that itertools has a ton of recipes in their docs without actually including them in the itertools module itself which we can apply like so:

class utcnow(FunctionElement):
    type = UTCDateTime()

@compiles(utcnow, 'postgresql')
def pg_utcnow(element, compiler, **kw):
    return "timezone('utc', current_timestamp)"

@compiles(utcnow, 'sqlite')
def sqlite_utcnow(element, compiler, **kw):
    return "(datetime('now', 'utc'))"

We might want to extend this further and include it in Sideboard. Or not, since most plugins probably don't care whether it's a client default or a server default.

2. I have a lot of columns which were defined in Django like

shirt = IntegerField(choices=SHIRT_OPTS, default=NO_SHIRT)

And this seems like a reasonable thing to want to do; in our original app we sometimes implemented this as (blech) a plain old UnicodeText column and just didn't allow the UI code to set a bad value. This seems bad, so maybe we want to include a Choice type; in the MAGFest code I just say

shirt = Column(Choice(SHIRT_OPTS), default=NO_SHIRT)

so if we want to then we could roll the Choice type into Sideboard.

3. This one is probably a more specific to MAGFest, but I have a lot of columns which represent checkbox groups. I previously used the CommaSeparatedIntegerField in Django for this, so in the MAGFest code now I have code like

assigned_depts = Column(MultiChoice(DEPARTMENT_OPTS))

I have no idea if this is something that would be generally useful, so it's probably not a good candidate for inclusion in Sideboard at this time. If we end up writing more plugins that could use this then maybe we can roll it in.

4. SQLAlchemy models by default do not implement the __hash__ method, which I would always want in any of my apps. It also doesn't implement a sensible __eq__ so we should probably roll that into what our @declarative_base provides.

5. It should always be easy to look up an object by id, since that's the most common thing I think I do in any of my ORM code. The default way to do that in Sideboard is e.g.

with Session() as session:
    attendee = session.query(Attendee).filter_by(id=id).one()

Which is okay but a little verbose. I'd like to be able to just say

with Session() as session:
    attendee = session.attendee(id)

Which I'm doing in the MAGFest code by adding a lookup method to the session for each model which takes an id. We might want this in Sideboard core, or maybe it's too magicky even for Sideboard.

6. When dealing with an object, it's often useful to get the session which that object is attached to. Otherwise, when passing a model object to a function, you have to pass both it and the session object. Fortunately, you can use the object_session method which SQLAlchemy provides (which just returns None if the object is unattached) to do this, so I implemented this in the MAGFest code:

    @property
    def session(self):
        return Session.session_factory.object_session(self)

7. This is related to 2 above, but Django has a get_FIELD_display option, so that if you have a choice column named shirt, you can say attendee.get_shirt_display() to get the textual description. I faked this with some code in __getattr__ in my base class, but it might do to make this more generic. Or not - we'll see how often we end up wanting to do something like this.

[EliAndrewC]

8. sideboard.tests.patch_session creates a new session_factory, which means that the SQLAlchemy ORM events which were defined will still work, but the SQLAlchemy core events will not. For example, MAGFest defines before_flush and after_flush events. We could make these automatically get listened-for on the patched session object. For now I'm just going to do it manually, and until we implement this, we should just document this limitation.

[EliAndrewC]

9. In the MAGFest tests, I use the same idiom we use in our Sideboard tests for testing database interactions: generate a test.db SQLite file once, then basically just do a cp to restore it on every test. Since this is something I want to happen for every test module, it makes sense to put this fixture in conftest.py, since that will make it available everywhere. (If you specify autouse=True for a fixture in a module, it's still only auto-used within that module or other modules its imported into, but if you specify a fixture with autouse=True in conftest.py it's available and automatically used everywhere.)

So the conftest.py we'd add would look something like this:

import shutil
import pytest
from sideboard.tests import patch_session
from {{ module }} import sa

@pytest.fixture(scope='session', autouse=True)
def init_db(request):
    patch_session(sa.Session, request)
    with sa.Session() as session:
       pass  # make insertions here

@pytest.fixture(autouse=True)
def db(request, init_db):
    shutil.copy('/tmp/test.db', '/tmp/test.db.backup')
    request.addfinalizer(lambda: shutil.copy('/tmp/test.db.backup', '/tmp/test.db'))

We'd also need some documentation explaining why we're doing this and how to use it effectively.

[EliAndrewC]

10. This is almost certainly not something we'd want to generally do; this seems more like a "weird Eli idiom" than anything, and it almost certainly would not apply to a rich Javascript application, which is probably what most people will be writing anyway.

I have a ton of page handlers which render templates. When someone submits the form, these typically run validations and then either return the rendered form again with an error message or save and redirect somewhere with a success message.

So in order to avoid saying with Session() as session at the top of every function, I use my all-time favorite thing (a class decorator) to automatically apply a sessionized decorator to each method, which checks for the presence of a session argument and then does the with Session() as session part automatically and passes that. Since I typically redirect on success and render on error, the decorator codifies that convention:

def _get_innermost(func):
    return _get_innermost(func.__wrapped__) if hasattr(func, '__wrapped__') else func

def sessionized(func):
    @wraps(func)
    def with_session(*args, **kwargs):
        innermost = _get_innermost(func)
        if 'session' not in inspect.getfullargspec(innermost).args:
            return func(*args, **kwargs)
        else:
            with Session() as session:
                try:
                    retval = func(*args, session=session, **kwargs)
                    session.expunge_all()
                    return retval
                except HTTPRedirect:
                    session.commit()
                    raise
    return with_session

This doesn't seem worth including in Sideboard itself, but I figured I'd mention it here since it's a thing I'm doing with the Sideboard code. (Rob Ruana might argue that this implies that our with Session() as session idiom is considered harmful since I'm writing decorators to work around it. I don't agree, but that seemed like another reason to document what I'm doing here.)

[EliAndrewC]

11. I'm doing a customized-for-MAGFest version of something that we might want to be able to turn on a generic version of for any Sideboard plugin with a database. Basically, for every database module, I want to be able to track all changes made to each row over time; when and how it was added, updated, and deleted, as well as who it was who made the changes.

Here's what the code looks like in the MAGFest codebase:

class Tracking(MagModel):
    fk_id  = Column(UUID)
    model  = Column(UnicodeText)
    when   = Column(UTCDateTime, default=lambda: datetime.now(UTC))
    who    = Column(UnicodeText)
    which  = Column(UnicodeText)
    links  = Column(UnicodeText)
    action = Column(Choice(TRACKING_OPTS))
    data   = Column(UnicodeText)

    @classmethod
    def format(cls, values):
        return ', '.join('{}={}'.format(k, v) for k,v in values.items())

    @classmethod
    def differences(cls, instance):
        diff = {}
        for attr, column in instance.__table__.columns.items():
            new_val = getattr(instance, attr)
            old_val = instance.orig_value_of(attr)
            if old_val != new_val:
                diff[attr] = "'{} -> {}'".format(cls.repr(column, old_val), cls.repr(column, new_val))
        return diff

    @classmethod
    def track(cls, action, instance):
        if action in [CREATED, UNPAID_PREREG, EDITED_PREREG]:
            vals = {attr: column_repr(column, getattr(instance, attr)) for attr, column in instance.__table__.columns.items()}
            data = cls.format(vals)
        elif action == UPDATED:
            diff = cls.differences(instance)
            data = cls.format(diff)
            if len(diff) == 1 and 'badge_num' in diff:
                action = AUTO_BADGE_SHIFT
            elif not data:
                return
        else:
            data = 'id={}'.format(instance.id)

        links = ', '.join(
            '{}({})'.format(list(column.foreign_keys)[0].table.name, getattr(instance, val))
            for name, column in instance.__table__.columns.items()
            if column.foreign_keys and getattr(instance, name)
        )
        who = AdminAccount.admin_name() or (current_thread().name if current_thread().daemon else 'non-admin')

        def _insert(session):
            session.add(Tracking(
                model = instance.__class__.__name__,
                fk_id = 0 if action in [UNPAID_PREREG, EDITED_PREREG] else instance.id,
                which = repr(instance),
                who = who,
                links = links,
                action = action,
                data = data
            ))
        if instance.session:
            _insert(instance.session)
        else:
            with Session() as session:
                _insert(session)

Tracking.UNTRACKED = [Tracking, Email]

And then it's super-easy to hook it up with a SQLAlchemy pre_flush listener:

def _track_changes(session, context, instances='deprecated'):
    for action, instances in {CREATED: session.new, UPDATED: session.dirty, DELETED: session.deleted}.items():
        for instance in instances:
            if instance.__class__ not in Tracking.UNTRACKED:
                Tracking.track(action, instance)

So the question is whether a generic version of this would be generally applicable. Given how much "audit logging" we've done on a lot of our apps, this seems like a good thing. On the other hand, the specifics might well be so different between projects that it's not worth having a base implementation if everyone will have to customize it so heavily they might as well roll one from scratch.

[EliAndrewC]

12. SQLAlchemy allows you to implement custom query subclasses by passing the query_cls parameter to the sessionmaker. Since by default we define the sessionmaker for our plugins if it's not already specified, it would be nice if there was a declarative way to implement custom query methods. So it would be nice if you could say something like:

class Session(SessionManager):
    class QuerySubclass(Query):
        def custom_method(self, ...): ...

I'm using this in the MAGFest codebase and it's nice and seems acceptably magicky.

[EliAndrewC]

Moved to magfest/sideboard#42