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

Decide what new SQLAlchemy features should be rolled into Sideboard #42

Open
EliAndrewC opened this issue Sep 14, 2016 · 0 comments
Open

Comments

@EliAndrewC
Copy link
Contributor

EliAndrewC commented Sep 14, 2016

MAGFest implements a ton of SQLAlchemy utilities, including a lot of Django idioms we ported over from older versions of our codebase. 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. We can split some of these out into separate issues as we decide to tackle them.

  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 plugins might not care whether it's a client default or a server default.

  1. We used to 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(c.SHIRT_OPTS), default=c.NO_SHIRT)

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

  1. 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(c.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.

  1. 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.

  2. 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.

  1. 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)
  1. 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.

  2. 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.

  3. 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.

  1. 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.)

  1. 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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant