Closed EliAndrewC closed 8 years ago
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.
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.
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.)
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.
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.
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.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 theitertools
module itself which we can apply like so: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
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 sayso 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 likeI 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.
Which is okay but a little verbose. I'd like to be able to just say
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 returnsNone
if the object is unattached) to do this, so I implemented this in the MAGFest code:7) This is related to 2 above, but Django has a
get_FIELD_display
option, so that if you have achoice
column named shirt, you can sayattendee.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.