Raise exception if history can’t be written

This is fiendishly difficult error to discover on your own.

It’s caused when, during the creation of a row in the database, you run
a query on the same table, or a table that joins to the table you’re
inserting into. What I think is happening is that the database is forced
to flush the session before running the query in order to maintain
consistency.

This means that the session is clean by the time the history stuff comes
to do its work, so there’s nothing for it to copy into the history
table, and it silently fails to record history.

Hopefully raising an exception will:
- prevent this from failing silently
- save whoever comes across this issue in the future a whole load of
  time

app/dao/dao_utils.py

@@ -20,18 +20,30 @@ def transactional(func):
     return commit_or_rollback
 
 
-def version_class(model_class, history_cls=None):
+def version_class(model_class, history_cls=None, must_write_history=True):
     create_hist = partial(create_history, history_cls=history_cls)
 
     def versioned(func):
         @wraps(func)
         def record_version(*args, **kwargs):
+
             func(*args, **kwargs)
+
             history_objects = [create_hist(obj) for obj in
                                itertools.chain(db.session.new, db.session.dirty)
                                if isinstance(obj, model_class)]
+
+            if history_objects == [] and must_write_history:
+                raise RuntimeError((
+                    'Can\'t record history for {} '
+                    '(something in your code has casued the database to '
+                    'flush the session early so there\'s nothing to '
+                    'copy into the history table)'
+                ).format(model_class.__name__))
+
             for h_obj in history_objects:
                 db.session.add(h_obj)
+
         return record_version
     return versioned
 

app/dao/services_dao.py

@@ -118,8 +118,8 @@ def dao_fetch_all_services_by_user(user_id, only_active=False):
 
 @transactional
 @version_class(Service)
-@version_class(Template, TemplateHistory)
-@version_class(ApiKey)
+@version_class(Template, TemplateHistory, must_write_history=False)
+@version_class(ApiKey, must_write_history=False)
 def dao_archive_service(service_id):
     # have to eager load templates and api keys so that we don't flush when we loop through them
     # to ensure that db.session still contains the models when it comes to creating history objects
@@ -373,7 +373,7 @@ def dao_fetch_todays_stats_for_all_services(include_from_test_key=True, only_act
 
 @transactional
 @version_class(Service)
-@version_class(ApiKey)
+@version_class(ApiKey, must_write_history=False)
 def dao_suspend_service(service_id):
     # have to eager load api keys so that we don't flush when we loop through them
     # to ensure that db.session still contains the models when it comes to creating history objects

tests/app/dao/test_services_dao.py

@@ -838,6 +838,16 @@ def test_dao_fetch_todays_stats_for_all_services_can_exclude_from_test_key(notif
     assert stats[0].count == 2
 
 
+@freeze_time('2001-01-01T23:59:00')
+def test_dao_suspend_service_with_no_api_keys(notify_db_session):
+    service = create_service()
+    dao_suspend_service(service.id)
+    service = Service.query.get(service.id)
+    assert not service.active
+    assert service.name == service.name
+    assert service.api_keys == []
+
+
 @freeze_time('2001-01-01T23:59:00')
 def test_dao_suspend_service_marks_service_as_inactive_and_expires_api_keys(notify_db_session):
     service = create_service()