Skip to content

Delayed signing of notification from processing to the redis saving #2463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jimleroyer wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Delayed signing of notification from processing to the redis saving #2463

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a111b12
Delayed signing of notification from processing to the redis saving.
jimleroyer Feb 18, 2025
a8ddcb7
Added redis-tool to the devcontainer
jimleroyer Feb 28, 2025
448bc48
Merge remote-tracking branch 'origin/main' into feat/metadata-on-buff…
jimleroyer Feb 28, 2025
9f4d30f
Merge remote-tracking branch 'origin/main' into feat/metadata-on-buff…
jimleroyer Apr 1, 2025
eee4b25
Merge remote-tracking branch 'origin/main' into feat/metadata-on-buff…
jimleroyer May 15, 2025
68e7007
Merge branch 'main' into feat/metadata-on-buffer-msg
jimleroyer Jun 5, 2025
afade45
Merge branch 'main' into feat/metadata-on-buffer-msg
jimleroyer Sep 5, 2025
b1955c5
Merge branch 'main' into feat/metadata-on-buffer-msg
jimleroyer Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .devcontainer/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ RUN apt-get update \
libxss1 \
libasound2 \
libxtst6 \
redis-tools \
xauth \
xvfb \
&& apt-get autoremove -y \
Expand All @@ -66,5 +67,5 @@ COPY .devcontainer/scripts/notify-dev-entrypoint.sh /usr/local/bin/

ENV SHELL /bin/zsh

EXPOSE 8000
EXPOSE 6011
EXPOSE 8000
46 changes: 43 additions & 3 deletions app/annotations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,45 @@
from functools import wraps

# from flask import current_app
from inspect import signature

from app import signer_notification
from app.encryption import SignedNotification, SignedNotifications

def sign_param(func):
Copy link
Contributor

@smcmurtry smcmurtry Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a great idea but I can't tell if it works just by looking at the code. Could you add some instructions for how to test if this is working?

"""
A decorator that signs parameters annotated with `PendingNotification` or `VerifiedNotification`
before passing them to the decorated function.
This decorator inspects the function's signature to find parameters annotated with
`PendingNotification` or `VerifiedNotification`. It then uses `signer_notification.sign`
to sign these parameters and replaces the original values with the signed values before
calling the decorated function.
Args:
func (Callable): The function to be decorated.
Returns:
Callable: The wrapped function with signed parameters.
"""

@wraps(func)
def wrapper(*args, **kwargs):
from app import signer_notification
from app.queue import QueueMessage

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
app.queue
Loading
begins an import cycle.

Copilot Autofix

AI 4 months ago

The best way to fix the cyclic import is to prevent app/annotations.py from importing QueueMessage from app.queue. Since the only use of QueueMessage in the shown code is within the sign_param decorator, specifically for issubclass(param.annotation, QueueMessage), we need to achieve that check without directly importing QueueMessage from app.queue within this module. There are two main ways to fix this:

  1. Move the relevant decorator to app.queue (and import other needed pieces there) if only code in app.queue uses it.
  2. Use duck typing or string comparisons instead of the direct class reference—for example, check if param.annotation's name or module matches.
  3. Delay all logic involving QueueMessage until runtime and fetch the class from the module dynamically using importlib, which avoids static import cycles.
  4. Move the common bits (QueueMessage definition) to a new module that both app.queue and app.annotations can import.

In the context of this snippet (where you cannot introduce a new module), the least intrusive fix is to use importlib and retrieve QueueMessage at runtime inside sign_param.wrapper, instead of statically importing it at the top of wrapper. This breaks the static module import cycle, as the import is now purely dynamic and delayed until the decorated function is called. The usage in issubclass(param.annotation, QueueMessage) is for runtime checking and can accept dynamic imports.

Specific edit:
In sign_param, replace from app.queue import QueueMessage with a dynamic import inside the wrapper (e.g., via importlib.import_module), or, if possible, compare param.annotation.__name__ and param.annotation.__module__ strings to detect the desired class, thus removing the static import altogether.

Suggested changeset 1
app/annotations.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/annotations.py b/app/annotations.py
--- a/app/annotations.py
+++ b/app/annotations.py
@@ -20,7 +20,8 @@
     @wraps(func)
     def wrapper(*args, **kwargs):
         from app import signer_notification
-        from app.queue import QueueMessage
+        import importlib
+        QueueMessage = getattr(importlib.import_module("app.queue"), "QueueMessage")
 
         sig = signature(func)
 
EOF
@@ -20,7 +20,8 @@
@wraps(func)
def wrapper(*args, **kwargs):
from app import signer_notification
from app.queue import QueueMessage
import importlib
QueueMessage = getattr(importlib.import_module("app.queue"), "QueueMessage")

sig = signature(func)

Copilot is powered by AI and may make mistakes. Always verify output.

sig = signature(func)

# Find the parameter annotated with VerifyAndSign
bound_args = sig.bind(*args, **kwargs)
bound_args.apply_defaults()

for param_name, param in sig.parameters.items():
if issubclass(param.annotation, QueueMessage):
unsigned: QueueMessage = bound_args.arguments[param_name] # type: ignore
signed_param = signer_notification.sign(unsigned.to_dict())
# Replace the signed value with the verified value
bound_args.arguments[param_name] = signed_param

# Call the decorated function with the signed value
result = func(*bound_args.args, **bound_args.kwargs)
return result

return wrapper


def unsign_params(func):
Expand All @@ -22,6 +57,9 @@

@wraps(func)
def wrapper(*args, **kwargs):
from app import signer_notification
from app.types import SignedNotification, SignedNotifications

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
app.types
Loading
begins an import cycle.

Copilot Autofix

AI 4 months ago

The best way to break this cycle is to refactor: move the types SignedNotification and SignedNotifications (and any other common types referred to by both annotations and types) into a new module—for example, app.notification_types.py. Both app/annotations.py and app/types.py can then import the required types from this shared "types only" module, with neither importing the other.

Only modify the code snippet shown in the app/annotations.py file. That means replacing the from app.types import SignedNotification, SignedNotifications import on line 61 with an import from the new module: from app.notification_types import SignedNotification, SignedNotifications. This requires minimal changes in annotations.py and does not affect the logic or behavior.

This solution is valid as long as SignedNotification and SignedNotifications are not otherwise locally defined in app/types.py (which we must assume, as they're imported), and the new module is purely a collection of type definitions. You would need to move the relevant type definitions into app/notification_types.py (not shown here, as you are only to edit the code you've provided).

Suggested changeset 1
app/annotations.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/annotations.py b/app/annotations.py
--- a/app/annotations.py
+++ b/app/annotations.py
@@ -58,7 +58,7 @@
     @wraps(func)
     def wrapper(*args, **kwargs):
         from app import signer_notification
-        from app.types import SignedNotification, SignedNotifications
+        from app.notification_types import SignedNotification, SignedNotifications
 
         sig = signature(func)
 
EOF
@@ -58,7 +58,7 @@
@wraps(func)
def wrapper(*args, **kwargs):
from app import signer_notification
from app.types import SignedNotification, SignedNotifications
from app.notification_types import SignedNotification, SignedNotifications

sig = signature(func)

Copilot is powered by AI and may make mistakes. Always verify output.

sig = signature(func)

# Find the parameter annotated with VerifyAndSign
Expand Down Expand Up @@ -59,6 +97,8 @@

@wraps(func)
def wrapper(*args, **kwargs):
from app import signer_notification

# Call the decorated function with the verified value
result = func(*args, **kwargs)

Expand Down
81 changes: 43 additions & 38 deletions app/celery/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
from app.dao.services_dao import dao_fetch_service_by_id
from app.dao.templates_dao import dao_get_template_by_id
from app.email_limit_utils import fetch_todays_email_count
from app.encryption import SignedNotification
from app.exceptions import DVLAException
from app.models import (
BULK,
Expand All @@ -84,7 +83,7 @@
send_notification_to_queue,
)
from app.sms_fragment_utils import fetch_todays_requested_sms_count
from app.types import VerifiedNotification
from app.types import SignedNotification, VerifiedNotification
from app.utils import get_csv_max_rows, get_delivery_queue_for_template, get_fiscal_year
from app.v2.errors import (
LiveServiceTooManyRequestsError,
Expand Down Expand Up @@ -289,23 +288,25 @@ def save_smss(self, service_id: Optional[str], signed_notifications: List[Signed
else:
reply_to_text = service.get_default_sms_sender() # type: ignore

notification: VerifiedNotification = {
**_notification, # type: ignore
"notification_id": notification_id,
"reply_to_text": reply_to_text,
"service": service,
"key_type": _notification.get("key_type", KEY_TYPE_NORMAL),
"template_id": template.id,
"template_version": template.version,
"recipient": _notification.get("to"),
"personalisation": _notification.get("personalisation"),
"notification_type": SMS_TYPE, # type: ignore
"simulated": _notification.get("simulated", None),
"api_key_id": _notification.get("api_key", None),
"created_at": datetime.utcnow(),
"job_id": _notification.get("job", None),
"job_row_number": _notification.get("row_number", None),
}
notification = VerifiedNotification.from_dict(
Copy link
Contributor

@smcmurtry smcmurtry Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! having a from_dict method is a much better way to do this.

Copy link
Contributor

@smcmurtry smcmurtry Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename notification -> verified_notification to make this consistent with the code below?

{
**_notification, # type: ignore
"notification_id": notification_id,
"reply_to_text": reply_to_text,
"service": service,
"key_type": _notification.get("key_type", KEY_TYPE_NORMAL),
"template_id": template.id,
"template_version": template.version,
"recipient": _notification.get("to"),
"personalisation": _notification.get("personalisation"),
"notification_type": SMS_TYPE, # type: ignore
"simulated": _notification.get("simulated", None),
"api_key_id": _notification.get("api_key", None),
"created_at": datetime.utcnow(),
"job_id": _notification.get("job", None),
"job_row_number": _notification.get("row_number", None),
}
)

verified_notifications.append(notification)
notification_id_queue[notification_id] = notification.get("queue") # type: ignore
Expand Down Expand Up @@ -404,26 +405,29 @@ def save_emails(self, _service_id: Optional[str], signed_notifications: List[Sig
else:
reply_to_text = service.get_default_reply_to_email_address()

notification: VerifiedNotification = {
**_notification, # type: ignore
"notification_id": notification_id,
"reply_to_text": reply_to_text,
"service": service,
"key_type": _notification.get("key_type", KEY_TYPE_NORMAL),
"template_id": template.id,
"template_version": template.version,
"recipient": _notification.get("to"),
"personalisation": _notification.get("personalisation"),
"notification_type": EMAIL_TYPE, # type: ignore
"simulated": _notification.get("simulated", None),
"api_key_id": _notification.get("api_key", None),
"created_at": datetime.utcnow(),
"job_id": _notification.get("job", None),
"job_row_number": _notification.get("row_number", None),
}
verified_notification = VerifiedNotification.from_dict(
{
**_notification, # type: ignore
"notification_id": notification_id,
"reply_to_text": reply_to_text,
"service": service,
"key_type": _notification.get("key_type", KEY_TYPE_NORMAL),
"template_id": template.id,
"template_version": template.version,
"recipient": _notification.get("to"),
"personalisation": _notification.get("personalisation"),
"notification_type": EMAIL_TYPE, # type: ignore
"simulated": _notification.get("simulated", None),
"api_key_id": _notification.get("api_key", None),
"created_at": datetime.utcnow(),
"job_id": _notification.get("job", None),
"job_row_number": _notification.get("row_number", None),
"queue": _notification.get("queue", None),
}
)

verified_notifications.append(notification)
notification_id_queue[notification_id] = notification.get("queue") # type: ignore
verified_notifications.append(verified_notification)
notification_id_queue[notification_id] = verified_notification.queue # type: ignore
process_type = template.process_type

try:
Expand Down Expand Up @@ -748,6 +752,7 @@ def send_notify_no_reply(self, data):
template = dao_get_template_by_id(current_app.config["NO_REPLY_TEMPLATE_ID"])

try:
# TODO: replace dict creation with VerifiedNotification.from_dict
data_to_send = [
dict(
template_id=template.id,
Expand Down
34 changes: 5 additions & 29 deletions app/encryption.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,7 @@
from typing import Any, List, NewType, Optional, TypedDict, cast
from typing import Any, List, cast

from flask_bcrypt import check_password_hash, generate_password_hash
from itsdangerous import URLSafeSerializer
from typing_extensions import NotRequired # type: ignore

SignedNotification = NewType("SignedNotification", str)
SignedNotifications = NewType("SignedNotifications", List[SignedNotification])
Copy link
Member Author

@jimleroyer jimleroyer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved into the types module.



class NotificationDictToSign(TypedDict):
Copy link
Member Author

@jimleroyer jimleroyer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved into the types module and became PendingNotification to represent the early lifecycle stage of the notification processing (when it hits the API and prior to be saved into the DB).

# todo: remove duplicate keys
# todo: remove all NotRequired and decide if key should be there or not
id: NotRequired[str]
template: str # actually template_id
service_id: NotRequired[str]
template_version: int
to: str # recipient
reply_to_text: NotRequired[str]
personalisation: Optional[dict]
simulated: NotRequired[bool]
api_key: str
key_type: str # should be ApiKeyType but I can't import that here
client_reference: Optional[str]
queue: Optional[str]
sender_id: Optional[str]
job: NotRequired[str] # actually job_id
row_number: Optional[Any] # should this be int or str?


class CryptoSigner:
Expand All @@ -42,22 +18,22 @@ def init_app(self, app: Any, secret_key: str | List[str], salt: str) -> None:
self.serializer = URLSafeSerializer(secret_key)
self.salt = salt

def sign(self, to_sign: str | NotificationDictToSign) -> str | bytes:
Copy link
Member Author

@jimleroyer jimleroyer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncoupling the notification model from the CryptoSigner. It will only have a str to sign when that is called.

def sign(self, to_sign: str) -> str | bytes:
"""Sign a string or dict with the class secret key and salt.

Args:
to_sign (str | NotificationDictToSign): The string or dict to sign.
to_sign (str): The string or dict to sign.

Returns:
str | bytes: The signed string or bytes.
"""
return self.serializer.dumps(to_sign, salt=self.salt)

def sign_with_all_keys(self, to_sign: str | NotificationDictToSign) -> List[str | bytes]:
def sign_with_all_keys(self, to_sign: str) -> List[str | bytes]:
"""Sign a string or dict with all the individual keys in the class secret key list, and the class salt.

Args:
to_sign (str | NotificationDictToSign): The string or dict to sign.
to_sign (str): The string or dict to sign.

Returns:
List[str | bytes]: A list of signed values.
Expand Down
80 changes: 43 additions & 37 deletions app/notifications/process_notifications.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
)
from notifications_utils.timezones import convert_local_timezone_to_utc

from app import redis_store
from app import models, redis_store
from app.celery import provider_tasks
from app.celery.letters_pdf_tasks import create_letters_pdf
from app.config import QueueNames
Expand Down Expand Up @@ -295,70 +295,76 @@ def send_notification_to_queue(notification, research_mode, queue=None):
)


def persist_notifications(notifications: List[VerifiedNotification]) -> List[Notification]:
def persist_notifications(verifiedNotifications: List[VerifiedNotification]) -> List[Notification]:
"""
Persist Notifications takes a list of json objects and creates a list of Notifications
that gets bulk inserted into the DB.
"""

lofnotifications = []

for notification in notifications:
notification_created_at = notification.get("created_at") or datetime.utcnow()
notification_id = notification.get("notification_id", uuid.uuid4())
notification_recipient = notification.get("recipient") or notification.get("to")
service_id = notification.get("service").id if notification.get("service") else None # type: ignore
for verifiedNotification in verifiedNotifications:
notification_created_at = verifiedNotification.created_at or datetime.utcnow()
notification_id = verifiedNotification.notification_id or uuid.uuid4()
notification_recipient = verifiedNotification.recipient or verifiedNotification.to
service_id = verifiedNotification.service.id if verifiedNotification.service else None # type: ignore
# todo: potential bug. notification_obj is being created using some keys that don't exist on notification
# reference, created_by_id, status, billable_units aren't keys on notification at this point
notification_obj = Notification(
id=notification_id,
template_id=notification.get("template_id"),
template_version=notification.get("template_version"),
template_id=verifiedNotification.template_id,
template_version=verifiedNotification.template_version,
Copy link
Member Author

@jimleroyer jimleroyer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now a data class so it has proper fields instead of dictionary entries.

to=notification_recipient,
service_id=service_id,
personalisation=notification.get("personalisation"),
notification_type=notification.get("notification_type"),
api_key_id=notification.get("api_key_id"),
key_type=notification.get("key_type"),
personalisation=verifiedNotification.personalisation,
notification_type=verifiedNotification.notification_type,
api_key_id=verifiedNotification.api_key_id,
key_type=verifiedNotification.key_type,
created_at=notification_created_at,
job_id=notification.get("job_id"),
job_row_number=notification.get("job_row_number"),
client_reference=notification.get("client_reference"),
reference=notification.get("reference"), # type: ignore
created_by_id=notification.get("created_by_id"), # type: ignore
status=notification.get("status"), # type: ignore
reply_to_text=notification.get("reply_to_text"),
billable_units=notification.get("billable_units"), # type: ignore
job_id=verifiedNotification.job_id,
job_row_number=verifiedNotification.job_row_number,
client_reference=verifiedNotification.client_reference,
# REVIEW: We can remove these ones if possible, as these will be set later in the process:
# reference: this is the provider's reference and will be set on sending time
# created_by_id: this is the user who created the notification and will be set on sending time, used by one off or admin UI uploads
# reference=verifiedNotification.reference, # type: ignore
# created_by_id=verifiedNotification.created_by_id, # type: ignore
# billable_units=verifiedNotification.billable_units, # type: ignore
Copy link
Member Author

@jimleroyer jimleroyer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove these:

  • reference: a notification will only have a reference once SES or Pinpoint is called. The exception is letters where a random reference will be generated but this function does not handle these, not GCNotify at large.
  • created_by_id: Not set when received by the API AFAIK. Other code paths are used by the one-off send or bulk send, where the created_by_id might be set.
  • billable_units: most likely not set at this stage yet.

status=NOTIFICATION_CREATED, # type: ignore
Copy link
Member Author

@jimleroyer jimleroyer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not receive a status from the notification, and should we? We can safely say the notification is created at this point, prior to save it into the database. I checked the other code paths for the persist_notification function (the one off save -- this one is for multiple) and it sets the notification to created status by default.

reply_to_text=verifiedNotification.reply_to_text,
Copy link
Member Author

@jimleroyer jimleroyer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reply_to_text field can be set via the API by providing the email_reply_to_id JSON field in the POST request. So that should be present in the PendingNotification | VerifiedNotification type and passed down the stream to the database.

)
template = dao_get_template_by_id(notification_obj.template_id, notification_obj.template_version, use_cache=True)
service = dao_fetch_service_by_id(service_id, use_cache=True)
notification_obj.queue_name = choose_queue(
notification=notification_obj, research_mode=service.research_mode, queue=get_delivery_queue_for_template(template)
)

if notification.get("notification_type") == SMS_TYPE:
formatted_recipient = validate_and_format_phone_number(notification_recipient, international=True)
recipient_info = get_international_phone_info(formatted_recipient)
notification_obj.normalised_to = formatted_recipient
notification_obj.international = recipient_info.international
notification_obj.phone_prefix = recipient_info.country_prefix
notification_obj.rate_multiplier = recipient_info.billable_units
elif notification.get("notification_type") == EMAIL_TYPE:
notification_obj.normalised_to = format_email_address(notification_recipient)
elif notification.get("notification_type") == LETTER_TYPE:
notification_obj.postage = notification.get("postage") or notification.get("template_postage") # type: ignore
match verifiedNotification.notification_type:
case models.SMS_TYPE:
formatted_recipient = validate_and_format_phone_number(notification_recipient, international=True)
recipient_info = get_international_phone_info(formatted_recipient)
notification_obj.normalised_to = formatted_recipient
notification_obj.international = recipient_info.international
notification_obj.phone_prefix = recipient_info.country_prefix
notification_obj.rate_multiplier = recipient_info.billable_units
case models.EMAIL_TYPE:
notification_obj.normalised_to = format_email_address(notification_recipient)
# case models.LETTER_TYPE:
# notification_obj.postage = verifiedNotification.postage # or verifiedNotification.template_postage
Copy link
Member Author

@jimleroyer jimleroyer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah.. I thought this is so useless and annoying..

case _:
current_app.logger.debug(f"Notification type {verifiedNotification.notification_type} not handled")

lofnotifications.append(notification_obj)
if notification.get("key_type") != KEY_TYPE_TEST:
service_id = notification.get("service").id # type: ignore
if verifiedNotification.key_type != KEY_TYPE_TEST:
service_id = verifiedNotification.service.id # type: ignore
if redis_store.get(redis.daily_limit_cache_key(service_id)):
redis_store.incr(redis.daily_limit_cache_key(service_id))

current_app.logger.info(
"{} {} created at {}".format(
notification.get("notification_type"),
notification.get("notification_id"),
notification.get("notification_created_at"), # type: ignore
Copy link
Contributor

@smcmurtry smcmurtry Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this just a bug? I don't think this key was actually there.

verifiedNotification.notification_type,
verifiedNotification.notification_id,
verifiedNotification.created_at,
)
)
bulk_insert_notifications(lofnotifications)
Expand Down
Loading
Loading