Fix message nesting when retrying Firebase v1 notifications (#387)

* Move fcm v1 body building outside of retry loop

* Add changelog entry

* Add test for retrying notification content

* Remove unnecessary test lines

changelog.d/387.bugfix

@@ -0,0 +1 @@
+Fixes an issue where retry attempts using the Firebase v1 API would fail due to nested `messages`.

sygnal/gcmpushkin.py

@@ -579,17 +579,20 @@ async def _dispatch_notification_unlimited(
                 else:
                     body["android"] = priority
 
+            if self.api_version is APIVersion.V1:
+                body["token"] = device.pushkey
+                new_body = body
+                body = {}
+                body["message"] = new_body
+
             for retry_number in range(0, MAX_TRIES):
+                # This has to happen inside the retry loop since `pushkeys` can be modified in the
+                # event of a failure that warrants a retry.
                 if self.api_version is APIVersion.Legacy:
                     if len(pushkeys) == 1:
                         body["to"] = pushkeys[0]
                     else:
                         body["registration_ids"] = pushkeys
-                elif self.api_version is APIVersion.V1:
-                    body["token"] = device.pushkey
-                    new_body = body
-                    body = {}
-                    body["message"] = new_body
 
                 log.info(
                     "Sending (attempt %i) => %r room:%s, event:%s",

tests/test_gcm.py

@@ -17,6 +17,7 @@
 from typing import TYPE_CHECKING, Any, AnyStr, Dict, List, Tuple
 from unittest.mock import MagicMock
 
+from sygnal.exceptions import TemporaryNotificationDispatchException
 from sygnal.gcmpushkin import APIVersion, GcmPushkin
 
 from tests import testutils
@@ -259,9 +260,6 @@ def test_expected_api_v1(self) -> None:
         """
         self.apns_pushkin_snotif = MagicMock()
         gcm = self.get_test_pushkin("com.example.gcm.apiv1")
-        gcm.preload_with_response(
-            200, {"results": [{"message_id": "msg42", "registration_id": "spqr"}]}
-        )
 
         # type safety: using ignore here due to mypy not handling monkeypatching,
         # see https://github.com/python/mypy/issues/2427
@@ -464,6 +462,74 @@ def test_batching_individual_failure(self) -> None:
         self.assertEqual(gcm.last_request_body["registration_ids"], ["spqr", "spqr2"])
         self.assertEqual(gcm.num_requests, 1)
 
+    def test_api_v1_retry(self) -> None:
+        """
+        Tests that a Firebase response of 502 results in Sygnal retrying.
+        Also checks the notification message to ensure it is sane after retrying
+        multiple times.
+        """
+        self.gcm_pushkin_snotif = MagicMock()
+
+        gcm = self.get_test_pushkin("com.example.gcm.apiv1")
+
+        # type safety: using ignore here due to mypy not handling monkeypatching,
+        # see https://github.com/python/mypy/issues/2427
+        gcm._request_dispatch = self.gcm_pushkin_snotif  # type: ignore[assignment] # noqa: E501
+
+        async def side_effect(*_args: Any, **_kwargs: Any) -> None:
+            raise TemporaryNotificationDispatchException(
+                "GCM server error, hopefully temporary.", custom_retry_delay=None
+            )
+
+        method = self.gcm_pushkin_snotif
+        method.side_effect = side_effect
+
+        _resp = self._request(self._make_dummy_notification([DEVICE_EXAMPLE_APIV1]))
+
+        self.assertEqual(3, method.call_count)
+        notification_req = method.call_args.args
+
+        self.assertEqual(
+            {
+                "message": {
+                    "data": {
+                        "event_id": "$qTOWWTEL48yPm3uT-gdNhFcoHxfKbZuqRVnnWWSkGBs",
+                        "type": "m.room.message",
+                        "sender": "@exampleuser:matrix.org",
+                        "room_name": "Mission Control",
+                        "room_alias": "#exampleroom:matrix.org",
+                        "membership": None,
+                        "sender_display_name": "Major Tom",
+                        "content_msgtype": "m.text",
+                        "content_body": "I'm floating in a most peculiar way.",
+                        "room_id": "!slw48wfj34rtnrf:example.com",
+                        "prio": "high",
+                        "unread": "2",
+                        "missed_calls": "1",
+                    },
+                    "android": {
+                        "notification": {
+                            "body": {
+                                "test body",
+                            },
+                        },
+                        "priority": "high",
+                    },
+                    "apns": {
+                        "payload": {
+                            "aps": {
+                                "content-available": 1,
+                                "mutable-content": 1,
+                                "alert": "",
+                            },
+                        },
+                    },
+                    "token": "spqr",
+                }
+            },
+            notification_req[2],
+        )
+
     def test_fcm_options(self) -> None:
         """
         Tests that the config option `fcm_options` allows setting a base layer

tests/testutils.py

@@ -286,6 +286,7 @@ def wait_for_work(self, early_stop=lambda: False):
 class DummyResponse:
     def __init__(self, code):
         self.code = code
+        self.headers = Headers()
 
 
 def make_async_magic_mock(ret_val):