From c257ec105c2313f1cf5dc3c56af38a5bce6a4ce8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 6 Mar 2019 08:37:55 +0000 Subject: [PATCH 1/2] =?UTF-8?q?Raise=20exception=20if=20history=20can?= =?UTF-8?q?=E2=80=99t=20be=20written?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 14 +++++++++++++- app/dao/services_dao.py | 6 +++--- tests/app/dao/test_services_dao.py | 10 ++++++++++ 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index 96628be06..c7af24845 100644 --- a/app/dao/dao_utils.py +++ b/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 diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 0685699a8..db9d8325e 100644 --- a/app/dao/services_dao.py +++ b/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 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index a8665554f..0c8fe8257 100644 --- a/tests/app/dao/test_services_dao.py +++ b/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() From eeb90bed573c7b3f04cc02f425f292833ac12a48 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 7 Mar 2019 17:39:38 +0000 Subject: [PATCH 2/2] Do all version table writes in one commit The behaviour of stacking the version decorators does not work as expected. What you would expect to happen is that each decorator causes a history row to be written for its respective model object. What actually happens is that the first decorator adds history records to the database session, but then causes the database session to commit. This means that subsequent uses of this decorator find a clean session, and therefore no changes to copy to their respective history tables. This commit changes the intended use of the decorator so that it is only used once per function, and accepts multiple definitions of what to record history for. This way it can record everything that needs to go into the history before doing anything that would risk flushing the session. --- app/dao/dao_utils.py | 55 +++++++++++++++++++++++++++++----------- app/dao/services_dao.py | 21 +++++++++------ app/dao/templates_dao.py | 11 +++++--- 3 files changed, 61 insertions(+), 26 deletions(-) diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index c7af24845..fa038b55c 100644 --- a/app/dao/dao_utils.py +++ b/app/dao/dao_utils.py @@ -1,5 +1,5 @@ import itertools -from functools import wraps, partial +from functools import wraps from app import db from app.history_meta import create_history @@ -20,8 +20,18 @@ def transactional(func): return commit_or_rollback -def version_class(model_class, history_cls=None, must_write_history=True): - create_hist = partial(create_history, history_cls=history_cls) +class VersionOptions(): + + def __init__(self, model_class, history_class=None, must_write_history=True): + self.model_class = model_class + self.history_class = history_class + self.must_write_history = must_write_history + + +def version_class(*version_options): + + if len(version_options) == 1 and not isinstance(version_options[0], VersionOptions): + version_options = (VersionOptions(version_options[0]),) def versioned(func): @wraps(func) @@ -29,20 +39,35 @@ def version_class(model_class, history_cls=None, must_write_history=True): func(*args, **kwargs) - history_objects = [create_hist(obj) for obj in - itertools.chain(db.session.new, db.session.dirty) - if isinstance(obj, model_class)] + session_objects = [] - 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 version_option in version_options: + tmp_session_objects = [ + ( + session_object, version_option.history_class + ) + for session_object in itertools.chain( + db.session.new, db.session.dirty + ) + if isinstance( + session_object, version_option.model_class + ) + ] - for h_obj in history_objects: - db.session.add(h_obj) + if tmp_session_objects == [] and version_option.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(version_option.model_class.__name__)) + + session_objects += tmp_session_objects + + for session_object, history_class in session_objects: + db.session.add( + create_history(session_object, history_cls=history_class) + ) return record_version return versioned diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index db9d8325e..c71d01851 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -9,7 +9,8 @@ from flask import current_app from app import db from app.dao.dao_utils import ( transactional, - version_class + version_class, + VersionOptions, ) from app.dao.organisation_dao import dao_get_organisation_by_email_address from app.dao.service_sms_sender_dao import insert_service_sms_sender @@ -117,9 +118,11 @@ def dao_fetch_all_services_by_user(user_id, only_active=False): @transactional -@version_class(Service) -@version_class(Template, TemplateHistory, must_write_history=False) -@version_class(ApiKey, must_write_history=False) +@version_class( + VersionOptions(ApiKey, must_write_history=False), + VersionOptions(Service), + VersionOptions(Template, history_class=TemplateHistory, 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 @@ -372,8 +375,10 @@ def dao_fetch_todays_stats_for_all_services(include_from_test_key=True, only_act @transactional -@version_class(Service) -@version_class(ApiKey, must_write_history=False) +@version_class( + VersionOptions(ApiKey, must_write_history=False), + VersionOptions(Service), +) 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 @@ -381,12 +386,12 @@ def dao_suspend_service(service_id): joinedload('api_keys'), ).filter(Service.id == service_id).one() - service.active = False - for api_key in service.api_keys: if not api_key.expiry_date: api_key.expiry_date = datetime.utcnow() + service.active = False + @transactional @version_class(Service) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 7861e20e1..63079ef68 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -11,12 +11,15 @@ from app.models import ( ) from app.dao.dao_utils import ( transactional, - version_class + version_class, + VersionOptions, ) @transactional -@version_class(Template, TemplateHistory) +@version_class( + VersionOptions(Template, history_class=TemplateHistory) +) def dao_create_template(template): template.id = uuid.uuid4() # must be set now so version history model can use same id template.archived = False @@ -36,7 +39,9 @@ def dao_create_template(template): @transactional -@version_class(Template, TemplateHistory) +@version_class( + VersionOptions(Template, history_class=TemplateHistory) +) def dao_update_template(template): if template.archived: template.folder = None