From 0528d7e5f27b4991775502dfe98a94e34b811aa7 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 7 Apr 2021 12:17:10 +0100 Subject: [PATCH] Try using nested transactions to rollback --- app/billing/rest.py | 2 ++ app/dao/annual_billing_dao.py | 4 ++-- app/dao/dao_utils.py | 13 +++++++++++++ app/dao/organisation_dao.py | 4 ++-- app/dao/services_dao.py | 4 ++-- app/organisation/rest.py | 16 +++++++--------- app/service/rest.py | 16 +++++++--------- tests/app/dao/test_organisation_dao.py | 1 + 8 files changed, 36 insertions(+), 24 deletions(-) diff --git a/app/billing/rest.py b/app/billing/rest.py index 3f5f6e08c..d18b786f7 100644 --- a/app/billing/rest.py +++ b/app/billing/rest.py @@ -83,6 +83,8 @@ def get_free_sms_fragment_limit(service_id): annual_billing = dao_create_or_update_annual_billing_for_year(service_id, annual_billing.free_sms_fragment_limit, financial_year_start) + from app import db + db.session.commit() return jsonify(annual_billing.serialize_free_sms_items()), 200 diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 5caa46011..8f64d445c 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -1,12 +1,12 @@ from flask import current_app from app import db -from app.dao.dao_utils import transactional +from app.dao.dao_utils import transactional, nested_transactional from app.dao.date_util import get_current_financial_year_start_year from app.models import AnnualBilling -@transactional +@nested_transactional def dao_create_or_update_annual_billing_for_year(service_id, free_sms_fragment_limit, financial_year_start): result = dao_get_free_sms_fragment_limit_for_year(service_id, financial_year_start) diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index f5515d8b6..0d4c0301e 100644 --- a/app/dao/dao_utils.py +++ b/app/dao/dao_utils.py @@ -4,6 +4,19 @@ from functools import wraps from app import db from app.history_meta import create_history +def nested_transactional(func): + @wraps(func) + def commit_or_rollback(*args, **kwargs): + try: + db.session.begin_nested() + res = func(*args, **kwargs) + db.session.commit() + return res + except Exception: + db.session.rollback() + raise + + return commit_or_rollback def transactional(func): @wraps(func) diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index 877f8af30..6e2c902fb 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -1,7 +1,7 @@ from sqlalchemy.sql.expression import func from app import db -from app.dao.dao_utils import VersionOptions, transactional, version_class +from app.dao.dao_utils import VersionOptions, transactional, version_class, nested_transactional from app.models import Domain, Organisation, Service, User @@ -105,7 +105,7 @@ def _update_organisation_services(organisation, attribute, only_where_none=True) db.session.add(service) -@transactional +@nested_transactional @version_class(Service) def dao_add_service_to_organisation(service, organisation_id): organisation = Organisation.query.filter_by( diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index c68092c64..3cc78887a 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -7,7 +7,7 @@ from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import and_, asc, case, func from app import db -from app.dao.dao_utils import VersionOptions, transactional, version_class +from app.dao.dao_utils import VersionOptions, transactional, version_class, nested_transactional from app.dao.date_util import get_current_financial_year from app.dao.email_branding_dao import dao_get_email_branding_by_name from app.dao.letter_branding_dao import dao_get_letter_branding_by_name @@ -284,7 +284,7 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): ).one() -@transactional +@nested_transactional @version_class(Service) def dao_create_service( service, diff --git a/app/organisation/rest.py b/app/organisation/rest.py index d745a7adb..3ec6a5f75 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -119,17 +119,15 @@ def link_service_to_organisation(organisation_id): service = dao_fetch_service_by_id(data['service_id']) service.organisation = None - dao_add_service_to_organisation(service, organisation_id) - # Need to do the annual billing update in a separate transaction because the both the - # dao_add_service_to_organisation and set_default_free_allowance_for_service are wrapped in a transaction. - # Catch and report an error if the annual billing doesn't happen - but don't rollback the service update. + from app import db + try: + dao_add_service_to_organisation(service, organisation_id) set_default_free_allowance_for_service(service, year_start=None) - except SQLAlchemyError: - # No need to worry about key errors because service.organisation_type has a foreign key to organisation_types - current_app.logger.exception( - f"Exception caught when trying to update annual billing when the organisation " - f"changed for service: {service.id} to organisation: {organisation_id}") + db.session.commit() + except Exception: + db.session.rollback() + raise return '', 204 diff --git a/app/service/rest.py b/app/service/rest.py index 03d111be9..c38b13143 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -254,18 +254,16 @@ def create_service(): # unpack valid json into service object valid_service = Service.from_json(data) - dao_create_service(valid_service, user) + from app import db - # Need to do the annual billing update in a separate transaction because the both the - # dao_add_service_to_organisation and set_default_free_allowance_for_service are wrapped in a transaction. - # Catch and report an error if the annual billing doesn't happen - but don't rollback the service update. try: + db.session.begin_nested() + dao_create_service(valid_service, user) set_default_free_allowance_for_service(valid_service, year_start=None) - except SQLAlchemyError: - # No need to worry about key errors because service.organisation_type has a foreign key to organisation_types - current_app.logger.exception( - f"Exception caught when trying to insert annual billing creating a service {valid_service.id} " - f"for organisation_type {valid_service.organisation_type}") + db.session.commit() + except Exception: + db.session.rollback() + raise return jsonify(data=service_schema.dump(valid_service).data), 201 diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py index 91136de8a..800381d7e 100644 --- a/tests/app/dao/test_organisation_dao.py +++ b/tests/app/dao/test_organisation_dao.py @@ -233,6 +233,7 @@ def test_add_service_to_organisation(sample_service, sample_organisation): sample_organisation.crown = False dao_add_service_to_organisation(sample_service, sample_organisation.id) + db.session.commit() assert len(sample_organisation.services) == 1 assert sample_organisation.services[0].id == sample_service.id