Dev: #1344 Separate read and write engine bindings & AuthenticatedSer…

…viceInfo instead of "return query.one()" (#1395)

* read-db uri,  engine binding, refactored service

* add read-db for testing, formatting fixes, removed prints

* added SQLALCHEMY_DATABASE_URI_READ to tasks def-n

* adding .rate_limit and .service_sms_senders to AutheticatedServiceInfo

* lintere fixes

* added tasks for perf, prod and staging

* improve readability of docker-compose

* add message_limit

* refactoring and clean up

* lint fix

* blank line fix

* fixing unit tests init work

* fixing unit tests aug2-3 part 1

* fixing unit tests aug 4

* fixed .has_permissions and service_has_permission

* all unit tests are passing

* cleaned up dev code and prints

* additional clean up of unused code and comments

* linter fixes

* fixed "secret" violates not-null constraint" error

* removed commented out import

* add logger prints and move .close()

* reverting

* contextmanager and AuthenticatedServiceApiKey

* clean up

* clean up

* added ticket number to TODO comments

* docs for AuthenticatedServiceInfo and clean up

* changed set to frozenset

* added annotations

* pr chage requests

* new fixture, __eq__ for ServiceData, renamed AuthenticatedServiceInfo to ServiceData

* readme update, moved context to dao utils, changed 403 test case to use sample_service_data_api_key

* revert test changes

* 403 fix and clean up

* fix sample_service_data_api_key def and test_should_attach_the_current_api_key_to_current_app

* use postgres 15

.github/workflows/tests.yaml

@@ -52,6 +52,7 @@ jobs:
           NOTIFY_ENVIRONMENT: test
           RETRY_SQS_URL: RETRY_SQS_URL
           SQLALCHEMY_DATABASE_URI: postgresql://postgres:postgres@localhost/test_notification_api
+          SQLALCHEMY_DATABASE_URI_READ: postgresql://postgres:postgres@localhost/test_notification_api
           TIMEOUT: 10
           VA_PROFILE_DOMAIN: int.vaprofile.va.gov
           VETEXT_API_AUTH_SSM_PATH: test

app/__init__.py

@@ -97,8 +97,12 @@ def create_app(application, worker_id=None):
     if notify_environment == "test":
         assert worker_id is not None
         application.config["SQLALCHEMY_DATABASE_URI"] += f"_{worker_id}"
+        # set read-db to be the same as write/default instance for testing
+        application.config["SQLALCHEMY_BINDS"] = {"read-db": application.config["SQLALCHEMY_DATABASE_URI"]}
         assert "test_notification_api" in application.config["SQLALCHEMY_DATABASE_URI"], \
             "Don't run tests against the main database."
+        assert "test_notification_api" in application.config["SQLALCHEMY_DATABASE_URI_READ"], \
+            "Don't run tests against the main database."
 
     application.config["NOTIFY_APP_NAME"] = application.name
     init_app(application)

app/cloudfoundry_config.py

@@ -15,6 +15,7 @@ def extract_cloudfoundry_config():
 def set_config_env_vars(vcap_services):
     # Postgres config
     os.environ['SQLALCHEMY_DATABASE_URI'] = vcap_services['postgres'][0]['credentials']['uri']
+    os.environ['SQLALCHEMY_DATABASE_URI_READ'] = vcap_services['postgres'][0]['credentials']['uri']
 
     vcap_application = json.loads(os.environ['VCAP_APPLICATION'])
     os.environ['NOTIFY_ENVIRONMENT'] = vcap_application['space_name']

app/config.py

@@ -108,6 +108,7 @@ class Config(object):
 
     # DB conection string
     SQLALCHEMY_DATABASE_URI = os.getenv('SQLALCHEMY_DATABASE_URI')
+    SQLALCHEMY_DATABASE_URI_READ = os.getenv('SQLALCHEMY_DATABASE_URI_READ')
 
     # MMG API Key
     MMG_API_KEY = os.getenv('MMG_API_KEY')
@@ -514,6 +515,10 @@ class Development(Config):
         "SQLALCHEMY_DATABASE_URI",
         'postgresql://postgres@localhost/notification_api')
 
+    SQLALCHEMY_BINDS = {"read-db": os.getenv(
+        "SQLALCHEMY_DATABASE_URI_READ",
+        'postgresql://postgres@localhost/notification_api')}
+
     ANTIVIRUS_ENABLED = os.getenv('ANTIVIRUS_ENABLED') == '1'
 
     for queue in QueueNames.all_queues():
@@ -543,6 +548,9 @@ class Test(Development):
         'SQLALCHEMY_DATABASE_URI',
         'postgresql://postgres@localhost/test_notification_api'
     )
+    SQLALCHEMY_BINDS = {"read-db": os.getenv(
+        "SQLALCHEMY_DATABASE_URI_READ",
+        'postgresql://postgres@localhost/notification_api')}
 
     CELERY_SETTINGS = {
         'broker_url': 'you-forgot-to-mock-celery-in-your-tests://'

app/dao/api_key_dao.py

@@ -17,7 +17,8 @@
 def save_model_api_key(api_key):
     if not api_key.id:
         api_key.id = uuid.uuid4()  # must be set now so version history model can use same id
-    api_key.secret = uuid.uuid4()
+    if not api_key.secret:
+        api_key.secret = uuid.uuid4()
     db.session.add(api_key)
 
 

app/dao/dao_utils.py

@@ -2,6 +2,8 @@
 from app import db
 from app.history_meta import create_history
 from functools import wraps
+from contextlib import contextmanager
+from sqlalchemy.orm import scoped_session, sessionmaker
 
 
 def transactional(func):
@@ -79,3 +81,26 @@ def record_version(*args, **kwargs):
 
 def dao_rollback():
     db.session.rollback()
+
+
+@contextmanager
+def get_reader_session():
+    """
+    This context manager is used to abstract the connection to the read-only database engine
+    in order to execute read queries. By using a scoped session, it ensures that the session
+    is thread-local and can be reused if needed. It ensures proper handling of the session's
+    lifecycle by closing it when the context is exited.
+
+    Yields:
+        session (scoped_session): A session connected to the read-only database engine.
+
+    Example Usage:
+        with get_reader_session() as session:
+            result = session.query(Service).filter_by(id=service_id).one()
+    """
+    engine = db.engines['read-db']
+    session = scoped_session(sessionmaker(bind=engine))
+    try:
+        yield session
+    finally:
+        session.close()

app/dao/services_dao.py

@@ -1,15 +1,18 @@
 import uuid
 from datetime import date, datetime, timedelta
+from app.service.service_data import ServiceData, ServiceDataException
 
 from notifications_utils.statsd_decorators import statsd
 from notifications_utils.timezones import convert_utc_to_local_timezone
 from sqlalchemy.sql.expression import asc, case, and_, func
 from sqlalchemy.orm import joinedload
+from sqlalchemy.orm.exc import NoResultFound, MultipleResultsFound
 from flask import current_app
 
 from app import db
 from app.dao.date_util import get_current_financial_year
 from app.dao.dao_utils import (
+    get_reader_session,
     transactional,
     version_class,
     VersionOptions,
@@ -194,16 +197,29 @@ def dao_fetch_service_by_inbound_number(number):
 
 
 def dao_fetch_service_by_id_with_api_keys(service_id, only_active=False):
-    query = Service.query.filter_by(
-        id=service_id
-    ).options(
-        joinedload('api_keys')
-    )
-
-    if only_active:
-        query = query.filter(Service.active)
+    with get_reader_session() as session:
+        query = session.query(Service).filter_by(
+            id=service_id
+        ).options(
+            joinedload('api_keys')
+        )
 
-    return query.one()
+        if only_active:
+            query = query.filter(Service.active)
+
+        try:
+            result = query.one()
+            # instead of returning the whole model attached to read-db engine
+            # extract needed properties and return object that can be
+            # serialized for caching
+            return ServiceData(result)
+        except (ServiceDataException, NoResultFound, MultipleResultsFound) as err:
+            # we handle this failure in the parent
+            current_app.logger.error("Could not find unique service with ID %s", service_id)
+            raise NoResultFound(err)
+        except Exception:
+            # we handle this failure in the parent
+            raise
 
 
 def dao_fetch_all_services_by_user(user_id, only_active=False):

app/models.py

@@ -34,7 +34,7 @@
 from sqlalchemy.ext.associationproxy import association_proxy
 from sqlalchemy.ext.declarative import declared_attr
 from sqlalchemy.ext.hybrid import hybrid_property
-from sqlalchemy.orm.collections import attribute_mapped_collection
+from sqlalchemy.orm.collections import attribute_mapped_collection, InstrumentedList
 
 
 SMS_TYPE = 'sms'
@@ -448,8 +448,20 @@ def get_default_letter_contact(self):
         default_letter_contact = [x for x in self.letter_contacts if x.is_default]
         return default_letter_contact[0].contact_block if default_letter_contact else None
 
-    def has_permission(self, permission):
-        return permission in [p.permission for p in self.permissions]
+    def has_permissions(self, permissions_to_check_for):
+        if isinstance(permissions_to_check_for, InstrumentedList):
+            _permissions_to_check_for = [p.permission for p in permissions_to_check_for]
+        elif not isinstance(permissions_to_check_for, list):
+            _permissions_to_check_for = [permissions_to_check_for]
+        else:
+            _permissions_to_check_for = permissions_to_check_for
+
+        if isinstance(self.permissions, InstrumentedList):
+            _permissions = [p.permission for p in self.permissions]
+        else:
+            _permissions = self.permissions
+
+        return frozenset(_permissions_to_check_for).issubset(frozenset(_permissions))
 
     def serialize_for_org_dashboard(self):
         return {

app/notifications/process_notifications.py

@@ -96,7 +96,6 @@ def persist_notification(
         template_version=template_version,
         to=recipient,
         service_id=service.id,
-        service=service,
         personalisation=personalisation,
         notification_type=notification_type,
         api_key_id=api_key_id,

app/notifications/receive_notifications.py

@@ -223,7 +223,7 @@ def fetch_potential_service(inbound_number: str, provider_name: str) -> Service:
         current_app.logger.error(message)
         raise NoSuitableServiceForInboundSms(message)
 
-    elif not has_inbound_sms_permissions(service.permissions):
+    elif not service.has_permissions([INBOUND_SMS_TYPE, SMS_TYPE]):
         statsd_client.incr(f"inbound.{provider_name}.failed")
         message = f'Service "{service.id}" does not allow inbound SMS'
         current_app.logger.error(message)
@@ -233,11 +233,6 @@ def fetch_potential_service(inbound_number: str, provider_name: str) -> Service:
         return service
 
 
-def has_inbound_sms_permissions(permissions):
-    str_permissions = [p.permission for p in permissions]
-    return set([INBOUND_SMS_TYPE, SMS_TYPE]).issubset(set(str_permissions))
-
-
 def strip_leading_forty_four(number):
     if number.startswith('44'):
         return number.replace('44', '0', 1)

app/notifications/rest.py

@@ -31,8 +31,7 @@
 from app.notifications.validators import (
     check_template_is_for_notification_type,
     check_template_is_active,
-    check_rate_limiting,
-    service_has_permission
+    check_rate_limiting
 )
 from app.schemas import (
     email_notification_schema,
@@ -122,7 +121,7 @@ def send_notification(notification_type):
     template_object = create_template_object_for_notification(template, notification_form.get('personalisation', {}))
 
     _service_allowed_to_send_to(notification_form, authenticated_service)
-    if not service_has_permission(notification_type, authenticated_service.permissions):
+    if not authenticated_service.has_permissions(notification_type):
         raise InvalidRequest(
             {'service': ["Cannot send {}".format(get_public_notify_type_text(notification_type, plural=True))]},
             status_code=400
@@ -187,10 +186,9 @@ def get_notification_return_data(notification_id, notification, template):
 
 
 def _service_can_send_internationally(service, number):
-    international_phone_info = get_international_phone_info(number)
+    phone_info = get_international_phone_info(number)
 
-    if international_phone_info.international and \
-            INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]:
+    if phone_info.international and not service.has_permissions(INTERNATIONAL_SMS_TYPE):
         raise InvalidRequest(
             {'to': ["Cannot send to international mobile numbers"]},
             status_code=400

app/notifications/validators.py

@@ -23,7 +23,6 @@
 from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError
 from app import redis_store
 from app.notifications.process_notifications import create_content_for_notification
-from app.utils import get_public_notify_type_text
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
 from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id
 
@@ -108,16 +107,12 @@ def service_can_send_to_recipient(send_to, key_type, service, allow_whitelisted_
         raise BadRequestError(message=message)
 
 
+# TODO #1410 clean up and remove
 def service_has_permission(notify_type, permissions):
     return notify_type in [p.permission for p in permissions]
 
 
-def check_service_has_permission(notify_type, permissions):
-    if not service_has_permission(notify_type, permissions):
-        raise BadRequestError(message="Service is not allowed to send {}".format(
-            get_public_notify_type_text(notify_type, plural=True)))
-
-
+# TODO #1410 clean up and remove
 def check_service_can_schedule_notification(permissions, scheduled_for):
     if scheduled_for:
         if not service_has_permission(SCHEDULE_NOTIFICATIONS, permissions):
@@ -131,15 +126,14 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type,
     service_can_send_to_recipient(send_to, key_type, service, allow_whitelisted_recipients)
 
     if notification_type == SMS_TYPE:
-        international_phone_info = get_international_phone_info(send_to)
+        phone_info = get_international_phone_info(send_to)
 
-        if international_phone_info.international and \
-                INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]:
+        if phone_info.international and not service.has_permissions(INTERNATIONAL_SMS_TYPE):
             raise BadRequestError(message="Cannot send to international mobile numbers")
 
         return validate_and_format_phone_number(
             number=send_to,
-            international=international_phone_info.international
+            international=phone_info.international
         )
     elif notification_type == EMAIL_TYPE:
         return validate_and_format_email_address(email_address=send_to)

app/service/send_notification.py

@@ -8,7 +8,6 @@
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
 from app.dao.service_sms_sender_dao import dao_get_service_sms_sender_by_id
 from app.notifications.validators import (
-    check_service_has_permission,
     check_service_over_daily_message_limit,
     validate_and_format_recipient,
     validate_template
@@ -35,6 +34,7 @@
     move_uploaded_pdf_to_letters_bucket,
 )
 from app.v2.errors import BadRequestError
+from app.utils import get_public_notify_type_text
 
 
 def validate_created_by(service, created_by_id):
@@ -135,8 +135,14 @@ def get_reply_to_text(notification_type, sender_id, service, template):
 def send_pdf_letter_notification(service_id, post_data):
     service = dao_fetch_service_by_id(service_id)
 
-    check_service_has_permission(LETTER_TYPE, service.permissions)
-    check_service_has_permission(UPLOAD_LETTERS, service.permissions)
+    if not service.has_permissions(LETTER_TYPE):
+        raise BadRequestError(message="Service is not allowed to send {}".format(
+            get_public_notify_type_text(LETTER_TYPE, plural=True)))
+
+    if not service.has_permissions(UPLOAD_LETTERS):
+        raise BadRequestError(message="Service is not allowed to send {}".format(
+            get_public_notify_type_text(UPLOAD_LETTERS, plural=True)))
+
     check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service)
     validate_created_by(service, post_data['created_by'])