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

jimleroyer · 2025-02-18T19:56:31Z

Summary | Résumé

Delayed signing of notification from processing to the redis saving
Refactored many types as well to reflect the notification's lifecycle, as well as removed unnecessary fields.

Related Issues | Cartes liées

Add X-Ray Trace ID to Redis Cluster

Test instructions | Instructions pour tester la modification

TODO: Fill in test instructions for the reviewer.

Release Instructions | Instructions pour le déploiement

None.

Reviewer checklist | Liste de vérification du réviseur

This PR does not break existing functionality.
This PR does not violate GCNotify's privacy policies.
This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive.
This PR does not significantly alter performance.
Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).

⚠ If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected.

Refactored many types as well to reflect the notification's lifecycle, as well as removed unnecessary fields.

jimleroyer · 2025-02-18T19:58:15Z

app/encryption.py

-SignedNotifications = NewType("SignedNotifications", List[SignedNotification])
-
-
-class NotificationDictToSign(TypedDict):


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).

jimleroyer · 2025-02-18T19:58:24Z

app/encryption.py

-from typing_extensions import NotRequired  # type: ignore
-
-SignedNotification = NewType("SignedNotification", str)
-SignedNotifications = NewType("SignedNotifications", List[SignedNotification])


This was moved into the types module.

app/annotations.py

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


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:

Move the relevant decorator to app.queue (and import other needed pieces there) if only code in app.queue uses it.

Use duck typing or string comparisons instead of the direct class reference—for example, check if param.annotation's name or module matches.

Delay all logic involving QueueMessage until runtime and fetch the class from the module dynamically using importlib, which avoids static import cycles.

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.

app/annotations.py

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


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).

app/queue.py

 from flask import current_app
 from redis import Redis

+from app.annotations import sign_param


The ideal fix is to break the import cycle by moving the dependency (the import of sign_param from app.annotations) to where it is actually needed, or refactor the code such that the cycle is avoided completely. The most common approach is to defer imports—that is, move the import statement inside the function(s) or method(s) that actually use sign_param, rather than having it at the top-level of the module.

This ensures that the import only occurs when the particular function using it runs, which in most cases is after all modules are initialized. Alternatively, if just one or two functions need sign_param, consider moving them to another module or to app.annotations itself. However, as we are restricted to only making changes within the code that is shown (i.e., only within app/queue.py), the best approach here is to move the line from app.annotations import sign_param from the global scope to inside the specific function(s) where sign_param is actually used.

Therefore:

Locate all usages of sign_param—only those functions call it.

Remove the from app.annotations import sign_param line from the top-level imports.

Insert the import statement immediately before its first use in each relevant function.

This change breaks the cyclic dependency at import time without changing any functional behavior.

app/types.py


-from app.encryption import NotificationDictToSign
 from app.models import Job, NotificationType, Service
+from app.queue import QueueMessage


To break the cycle, we must remove the import of QueueMessage from app.types, while maintaining the functionality of having PendingNotification inherit from QueueMessage. One standard pattern is to move shared base or interface classes to a new module that is not part of the cycle, then have both app.types and app.queue import this base.

Since you have only shown code from app/types.py, we can only make changes within that file. The best fix, therefore (without access to app.queue), is to use type hints in string form or type checking-only imports, or to import QueueMessage inside the class after definition (if only used within method annotations). However, since PendingNotification subclasses QueueMessage, the base class must be available at class creation time. Lacking the ability to edit app.queue, the only way within this file to break the runtime cycle is to remove the inheritance (PendingNotification(QueueMessage) → PendingNotification), at the cost of eliminating functionality derived from QueueMessage.

Less destructive is the use of typing.TYPE_CHECKING:

Move the import of QueueMessage inside an if TYPE_CHECKING block, so it is only imported for static analysis, not at runtime.

Change PendingNotification to inherit from object, and (optionally) add a comment to clarify the intended base class for static analysis.

This approach avoids the runtime import and breaks the cycle, at the cost of lost runtime inheritance but preserves type analysis. This is the best fix possible within app/types.py.

tests/app/test_annotations.py

        assert signed == 1
+
+    def test_unsign_and_return(self, mocker):
+        from tests.app import test_annotations


To fix the problem, remove the line from tests.app import test_annotations from tests/app/test_annotations.py. Any references in the code of the style test_annotations.func_to_unsign_and_return should be replaced with a direct reference to func_to_unsign_and_return, as that function is already in the same module scope. Specifically, in the test test_unsign_and_return, update mocker.spy(test_annotations, "func_to_unsign_and_return") to mocker.spy("tests.app.test_annotations", "func_to_unsign_and_return"), as mocker.spy can accept the fully qualified string path of the target function. If a direct function object is needed, use func_to_unsign_and_return itself.

The changes are strictly limited to removing the self-import line and fixing how the spy is set. All changes are to be made within tests/app/test_annotations.py, lines 139-141.

jimleroyer · 2025-02-18T19:59:17Z

app/encryption.py

        self.serializer = URLSafeSerializer(secret_key)
        self.salt = salt

-    def sign(self, to_sign: str | NotificationDictToSign) -> str | bytes:


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

jimleroyer · 2025-02-18T19:59:58Z

app/notifications/process_notifications.py

-            template_id=notification.get("template_id"),
-            template_version=notification.get("template_version"),
+            template_id=verifiedNotification.template_id,
+            template_version=verifiedNotification.template_version,


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

jimleroyer · 2025-02-18T20:01:50Z

app/notifications/process_notifications.py

+            #   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


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.

jimleroyer · 2025-02-18T20:03:53Z

app/notifications/process_notifications.py

+            # reference=verifiedNotification.reference,  # type: ignore
+            # created_by_id=verifiedNotification.created_by_id,  # type: ignore
+            # billable_units=verifiedNotification.billable_units,  # type: ignore
+            status=NOTIFICATION_CREATED,  # type: ignore


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.

jimleroyer · 2025-02-18T20:05:15Z

app/notifications/process_notifications.py

+            # created_by_id=verifiedNotification.created_by_id,  # type: ignore
+            # billable_units=verifiedNotification.billable_units,  # type: ignore
+            status=NOTIFICATION_CREATED,  # type: ignore
+            reply_to_text=verifiedNotification.reply_to_text,


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.

jimleroyer · 2025-02-18T20:05:39Z

app/notifications/process_notifications.py

+            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


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

jimleroyer · 2025-02-18T20:06:11Z

app/types.py

+    api_key: str
+    key_type: str  # should be ApiKeyType but can't import that here
+    client_reference: Optional[str]
+    reply_to_text: Optional[str]


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.

jimleroyer · 2025-02-18T20:06:28Z

app/types.py

-class VerifiedNotification(NotificationDictToSign):
+
+@dataclass
+class PendingNotification(QueueMessage):


Previously known as NotificationDictToSign.

jimleroyer · 2025-02-18T20:08:37Z

app/v2/notifications/post_notifications.py

+        }
+    )

-    signed_notification_data = signer_notification.sign(_notification)


This layer does not signed the notification anymore: we want to delay the signing in the Redis queue because we need to add extra metadata in there, some coupled to queue management. Hence we removed the signing and Redis will do it itself, via the new signing annotations.

jimleroyer · 2025-02-18T20:09:28Z

app/queue.py


-    def publish(self, message: str):
-        self._redis_client.rpush(self._inbox, message)
+    @sign_param


This will sign the parameter automatically, using the app.signer_notification object.

smcmurtry

I like these changes so far!

smcmurtry · 2025-02-19T20:02:55Z

app/celery/tasks.py

-            "job_id": _notification.get("job", None),
-            "job_row_number": _notification.get("row_number", None),
-        }
+        notification = VerifiedNotification.from_dict(


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

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

smcmurtry · 2025-02-19T20:21:23Z

app/notifications/process_notifications.py

            "{} {} created at {}".format(
-                notification.get("notification_type"),
-                notification.get("notification_id"),
-                notification.get("notification_created_at"),  # type: ignore


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

smcmurtry · 2025-02-19T20:29:48Z

app/types.py

+    @classmethod
+    def from_dict(cls, data: dict):
+        """Create an object from a dictionary."""
+        return cls(**data)


Looking at the doc for dataclasses: https://docs.python.org/3/library/dataclasses.html

It looks like that decorater will add an __init__ method where you can pass in all the data you have defined above. So you could create an instance like this:

pending_notification = PendingNotification(id=x.id, template=x.template, etc )

instead of:

pending_notification = PendingNotification.from_dict({"id": x.id, "template": x.template, etc })

if you want.

But we do often pass in a dict that looks like {**notification, "id": new_id} so having a from_dict method is probably best

smcmurtry · 2025-02-19T20:31:40Z

app/v2/notifications/post_notifications.py

-        "client_reference": form.get("reference", None),
-        "reply_to_text": reply_to_text,
-    }
+    _notification = PendingNotification.from_dict(


could rename to

Suggested change

_notification = PendingNotification.from_dict(

pending_notification = PendingNotification.from_dict(

smcmurtry · 2025-02-19T20:42:31Z

app/annotations.py

-from app import signer_notification
-from app.encryption import SignedNotification, SignedNotifications
+
+def sign_param(func):


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?

…er-msg

app/v2/notifications/post_notifications.py

 from app.dao.notifications_dao import update_notification_status_by_reference
 from app.dao.templates_dao import get_precompiled_letter_template
 from app.email_limit_utils import fetch_todays_email_count
+from app.letters.utils import upload_letter_pdf


The best way to fix the problem is to remove the unused import statement. This will clean up the code and remove the unnecessary dependency. Specifically, we need to delete the line that imports upload_letter_pdf from app.letters.utils.

…er-msg

Delayed signing of notification from processing to the redis saving.

a111b12

Refactored many types as well to reflect the notification's lifecycle, as well as removed unnecessary fields.

jimleroyer requested review from sastels and smcmurtry February 18, 2025 19:56

jimleroyer self-assigned this Feb 18, 2025

jimleroyer commented Feb 18, 2025

View reviewed changes

github-advanced-security bot found potential problems Feb 18, 2025

View reviewed changes

jimleroyer commented Feb 18, 2025

View reviewed changes

jimleroyer mentioned this pull request Feb 19, 2025

Add X-Ray Trace ID to Redis Cluster cds-snc/notification-planning-core#476

Open

10 tasks

smcmurtry reviewed Feb 19, 2025

View reviewed changes

jimleroyer added 3 commits February 28, 2025 14:35

Added redis-tool to the devcontainer

a8ddcb7

Merge remote-tracking branch 'origin/main' into feat/metadata-on-buff…

448bc48

…er-msg

Merge remote-tracking branch 'origin/main' into feat/metadata-on-buff…

9f4d30f

…er-msg

github-advanced-security bot found potential problems Apr 1, 2025

View reviewed changes

jimleroyer added 4 commits May 15, 2025 17:12

Merge remote-tracking branch 'origin/main' into feat/metadata-on-buff…

eee4b25

…er-msg

Merge branch 'main' into feat/metadata-on-buffer-msg

68e7007

Merge branch 'main' into feat/metadata-on-buffer-msg

afade45

Merge branch 'main' into feat/metadata-on-buffer-msg

b1955c5

@@ -8,7 +8,7 @@
             from flask import current_app
             from redis import Redis
-            from app.annotations import sign_param
+            # NOTE: Moved 'from app.annotations import sign_param' into functions where 'sign_param' is used, to break cyclic import.
             from app.aws.metrics import (
                 put_batch_saving_expiry_metric,
                 put_batch_saving_inflight_metric,

@@ -3,14 +3,18 @@
             from typing import List, NewType, Optional
             from app.models import Job, NotificationType, Service
-            from app.queue import QueueMessage
+            from typing import TYPE_CHECKING
+            if TYPE_CHECKING:
+                from app.queue import QueueMessage
             SignedNotification = NewType("SignedNotification", str)
             SignedNotifications = NewType("SignedNotifications", List[SignedNotification])
             @dataclass
-            class PendingNotification(QueueMessage):
+            class PendingNotification:
+                # NOTE: Was intended to inherit from 'QueueMessage', but direct inheritance has been removed to avoid import cycle.
                 # todo: remove duplicate keys
                 # todo: remove all NotRequired and decide if key should be there or not
                 id: str

@@ -136,10 +136,10 @@
                     assert signed == 1
                 def test_unsign_and_return(self, mocker):
-                    from tests.app import test_annotations
-                    ann_func_spy = mocker.spy(test_annotations, "func_to_unsign_and_return")
+                    ann_func_spy = mocker.spy("tests.app.test_annotations", "func_to_unsign_and_return")
                     msg = "raw notification"
                     signed = signer_notification.sign(msg)
                     unsigned = func_to_unsign_and_return(signed)

		SignedNotifications = NewType("SignedNotifications", List[SignedNotification])


		class NotificationDictToSign(TypedDict):

	_notification = PendingNotification.from_dict(
	pending_notification = PendingNotification.from_dict(

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

jimleroyer commented Feb 18, 2025

Summary | Résumé

Related Issues | Cartes liées

Test instructions | Instructions pour tester la modification

Release Instructions | Instructions pour le déploiement

Reviewer checklist | Liste de vérification du réviseur

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

Uh oh!

Copilot Autofix

Check notice

Uh oh!

Copilot Autofix

Check notice

Uh oh!

Copilot Autofix

Check notice

Uh oh!

Copilot Autofix

Check notice

Copilot Autofix

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimleroyer Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smcmurtry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smcmurtry Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

Copilot Autofix

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jimleroyer Feb 18, 2025 •

edited

Loading

smcmurtry Feb 19, 2025 •

edited

Loading