Return new seqno from reaction-modifying endpoints

This allows Session to properly avoid a race condition between adding a
reaction and getting a reaction update without the reaction applied yet
(e.g. because the poll result was generated before the reaction update).

With the seqno returned, clients can use it to detect such a case to
properly handle local display of not-yet-updated reactions applied to a
post.

Author: jagerman

sogs/model/room.py

@@ -1154,23 +1154,34 @@ def add_reaction(self, user: User, msg_id: int, reaction: str):
         The post must exist in the room (throws NoSuchPost if it does not).
 
         The user must have read permission in the room (throws BadPermission if not).
+
+        Returns a tuple of: bool indicating whether adding was successful (False = reaction already
+        present), and the new message seqno value.
         """
 
         self._check_reaction_request(user, msg_id, reaction)
 
-        try:
-            query(
-                """
-                INSERT INTO message_reactions (message, reaction, "user")
-                VALUES (:msg, :react, :user)
-                """,
-                msg=msg_id,
-                react=reaction,
-                user=user.id,
-            )
-        except sqlalchemy.exc.IntegrityError:
-            return False
-        return True
+        with db.transaction():
+            try:
+                query(
+                    """
+                    INSERT INTO message_reactions (message, reaction, "user")
+                    VALUES (:msg, :react, :user)
+                    """,
+                    msg=msg_id,
+                    react=reaction,
+                    user=user.id,
+                )
+                added = True
+                seqno = query("SELECT seqno FROM messages WHERE id = :msg", msg=msg_id).first()[0]
+
+            except sqlalchemy.exc.IntegrityError:
+                added = False
+
+        if not added:
+            seqno = query("SELECT seqno FROM messages WHERE id = :msg", msg=msg_id).first()[0]
+
+        return added, seqno
 
     def delete_reaction(self, user: User, msg_id: int, reaction: str):
         """
@@ -1181,19 +1192,23 @@ def delete_reaction(self, user: User, msg_id: int, reaction: str):
         """
 
         self._check_reaction_request(user, msg_id, reaction)
-        return (
-            query(
-                """
-                DELETE FROM user_reactions
-                WHERE reaction = (SELECT id FROM reactions WHERE message = :m AND reaction = :r)
-                    AND "user" = :u
-                """,
-                m=msg_id,
-                r=reaction,
-                u=user.id,
-            ).rowcount
-            > 0
-        )
+        with db.transaction():
+            removed = (
+                query(
+                    """
+                    DELETE FROM user_reactions
+                    WHERE reaction = (SELECT id FROM reactions WHERE message = :m AND reaction = :r)
+                        AND "user" = :u
+                    """,
+                    m=msg_id,
+                    r=reaction,
+                    u=user.id,
+                ).rowcount
+                > 0
+            )
+            seqno = query("SELECT seqno FROM messages WHERE id = :msg", msg=msg_id).first()[0]
+
+        return removed, seqno
 
     def delete_all_reactions(self, mod: User, msg_id: int, reaction: Optional[str] = None):
         """
@@ -1215,16 +1230,20 @@ def delete_all_reactions(self, mod: User, msg_id: int, reaction: Optional[str] =
 
         self._check_reaction_request(mod, msg_id, reaction, allow_none=True, mod_required=True)
 
-        return query(
-            f"""
-            DELETE FROM user_reactions WHERE
-                reaction IN (SELECT id FROM reactions WHERE
-                    message = :m
-                    {'AND reaction = :r' if reaction is not None else ''})
-            """,
-            m=msg_id,
-            r=reaction,
-        ).rowcount
+        with db.transaction():
+            removed = query(
+                f"""
+                DELETE FROM user_reactions WHERE
+                    reaction IN (SELECT id FROM reactions WHERE
+                        message = :m
+                        {'AND reaction = :r' if reaction is not None else ''})
+                """,
+                m=msg_id,
+                r=reaction,
+            ).rowcount
+            seqno = query("SELECT seqno FROM messages WHERE id = :msg", msg=msg_id).first()[0]
+
+        return removed, seqno
 
     def get_reactors(
         self,

sogs/routes/messages.py

@@ -572,6 +572,10 @@ def message_react(room, msg_id, reaction):
 
     - `"added"` — boolean value indicating whether the reaction was added (true) or already present
       (false).
+    - `"seqno"` — the message's new seqno value.  This can be used to identify stale reaction
+      updates when polling and reactions can race: if an in-progress poll returns a reaction update
+      for the message with a seqno less than this value then the client can know that that reaction
+      update won't yet have the reaction added here.
 
     # Error status codes
 
@@ -583,8 +587,8 @@ def message_react(room, msg_id, reaction):
     (instead in such a case the success response return value includes `"added": false`).
     """
 
-    added = room.add_reaction(g.user, msg_id, reaction)
-    return jsonify({"added": added})
+    added, seqno = room.add_reaction(g.user, msg_id, reaction)
+    return jsonify({"added": added, "seqno": seqno})
 
 
 @messages.delete("/room/<Room:room>/reaction/<int:msg_id>/<path:reaction>")
@@ -608,6 +612,7 @@ def message_unreact(room, msg_id, reaction):
 
     - `"removed"` — boolean value indicating whether the reaction was removed (true) or was not
       present to begin with (false).
+    - `"seqno"` — the message's new seqno value.  (See description in the put reaction endpoint).
 
     # Error status codes
 
@@ -618,8 +623,8 @@ def message_unreact(room, msg_id, reaction):
     Note that it is *not* an error to attempt to remove a reaction that does not exist (instead in
     such a case the success response return value includes `"removed": false`).
     """
-    removed = room.delete_reaction(g.user, msg_id, reaction)
-    return jsonify({"removed": removed})
+    removed, seqno = room.delete_reaction(g.user, msg_id, reaction)
+    return jsonify({"removed": removed, "seqno": seqno})
 
 
 @messages.delete("/room/<Room:room>/reactions/<int:msg_id>/<path:reaction>")
@@ -643,18 +648,19 @@ def message_delete_reactions(room, msg_id, reaction=None):
 
     # Return value
 
-    On success returns a 200 status code and a JSON object response body with key:
+    On success returns a 200 status code and a JSON object response body with keys:
 
     - `"removed"` — the total number of reactions that were deleted.
+    - `"seqno"` — the message's new seqno value.  (See description in the put reaction endpoint).
 
     # Error status codes
 
     - 403 Forbidden — if not a moderator
     - 404 Not Found — if the referenced post does not exist or is not a regular message
     - 400 Bad Request — if the input does not contain a valid reaction *or* `"all": true`.
     """
-    removed = room.delete_all_reactions(g.user, msg_id, reaction)
-    return jsonify({"removed": removed})
+    removed, seqno = room.delete_all_reactions(g.user, msg_id, reaction)
+    return jsonify({"removed": removed, "seqno": seqno})
 
 
 @messages.get("/room/<Room:room>/reactors/<int:msg_id>/<path:reaction>")

tests/test_reactions.py

@@ -18,10 +18,12 @@ def test_reactions(client, room, room2, user, user2, mod, admin, global_mod, glo
 
     seqno = r.json[-1]["seqno"]
 
+    new_seqno = seqno
     for x in ("🖕", "🍆", "f", "y/n", "abcdefghijkl"):
         r = sogs_put(client, f"/room/test-room/reaction/4/{x}", {}, user)
         assert r.status_code == 200
-        assert r.json["added"]
+        new_seqno += 1
+        assert r.json == {"added": True, "seqno": new_seqno}
 
     # Without the ?t=r flag, we don't get reaction-only updates:
     r = sogs_get(client, f"/room/test-room/messages/since/{seqno}", user2)
@@ -37,7 +39,7 @@ def test_reactions(client, room, room2, user, user2, mod, admin, global_mod, glo
     # Already present:
     r = sogs_put(client, "/room/test-room/reaction/4/🖕", {}, user)
     assert r.status_code == 200
-    assert not r.json["added"]
+    assert r.json == {"added": False, "seqno": seqno}
     assert sogs_get(client, f"/room/test-room/messages/since/{seqno}?t=r", user2).json == []
 
     r = sogs_get(client, "/room/test-room/messages/since/0?t=r", user2)
@@ -46,14 +48,15 @@ def test_reactions(client, room, room2, user, user2, mod, admin, global_mod, glo
     assert r.json[-1]["seqno"] == seqno
 
     r = sogs_put(client, "/room/test-room/reaction/10/🍍", {}, user)
-    assert r.json["added"]
+    assert r.json == {"added": True, "seqno": seqno + 1}
     r = sogs_put(client, "/room/test-room/reaction/4/🖕", {}, user2)
-    assert r.json["added"]
+    assert r.json == {"added": True, "seqno": seqno + 2}
     r = sogs_put(client, "/room/test-room/reaction/4/🍍", {}, user)
-    assert r.json["added"]
+    assert r.json == {"added": True, "seqno": seqno + 3}
 
     r = sogs_get(client, f"/room/test-room/messages/since/{seqno}?t=r", user2)
     assert {x['id']: x['seqno'] for x in r.json} == {4: seqno + 3, 10: seqno + 1}
+    seqno_10 = seqno + 1
     seqno += 3
 
     r = sogs_get(client, "/room/room2/messages/since/0?t=r", user2)
@@ -62,12 +65,17 @@ def test_reactions(client, room, room2, user, user2, mod, admin, global_mod, glo
     # If there is both an edit and new reactions, we should get the full message including reactions
     # and *not* a separate reactions row.
     room.edit_post(mod, 4, data=b'edited fake data 4', sig=pad64(b'fake sig 4b'))
+    new_seqno = seqno + 1
     for u in (user2, global_admin, mod, global_mod, admin):
         r = sogs_put(client, "/room/test-room/reaction/4/🍍", {}, u)
-        assert r.json['added']
-    assert not sogs_put(client, "/room/test-room/reaction/4/🍍", {}, user).json['added']
+        new_seqno += 1
+        assert r.json == {'added': True, 'seqno': new_seqno}
+    assert sogs_put(client, "/room/test-room/reaction/4/🍍", {}, user).json == {
+        'added': False,
+        "seqno": new_seqno,
+    }
     r = sogs_put(client, "/room/test-room/reaction/4/🦒🦍🐍🐊🦢🦁🦎", {}, user)
-    assert r.json['added']
+    assert r.json == {'added': True, "seqno": new_seqno + 1}
 
     exp_reactions = {
         'abcdefghijkl': {'index': 4, 'count': 1, 'reactors': [user.session_id]},
@@ -147,9 +155,8 @@ def test_reactions(client, room, room2, user, user2, mod, admin, global_mod, glo
 
     r = sogs_delete(client, "/room/test-room/reactions/4/🍍", global_admin)
     assert r.status_code == 200
-    n_pineapples = exp_reactions["🍍"]["count"]
-    assert r.json["removed"] == n_pineapples
-    assert r.json["removed"] == 5
+    assert exp_reactions["🍍"]["count"] == 5
+    assert r.json == {"removed": 5, "seqno": seqno + 5}
     del exp_reactions["🍍"]
     exp_reactions["🦒🦍🐍🐊🦢🦁🦎"]["index"] -= 1
 
@@ -160,7 +167,7 @@ def test_reactions(client, room, room2, user, user2, mod, admin, global_mod, glo
     n_other = sum(x["count"] for x in exp_reactions.values())
     r = sogs_delete(client, "/room/test-room/reactions/4", mod)
     assert r.status_code == 200
-    assert r.json["removed"] == n_other
+    assert r.json == {"removed": n_other, "seqno": seqno + n_other}
 
     r = sogs_get(client, f"/room/test-room/messages/since/{seqno}?t=r&reactors=0", user2)
     assert r.json == [{'id': 4, 'reactions': {}, 'seqno': seqno + n_other}]
@@ -172,11 +179,23 @@ def test_reactions(client, room, room2, user, user2, mod, admin, global_mod, glo
     assert [x['id'] for x in r if x['reactions']] == [10]
     assert r[7]['reactions'] == {'🍍': {'count': 1, 'index': 0}}
 
-    assert not sogs_delete(client, "/room/test-room/reaction/10/🍍", global_mod).json['removed']
-    assert sogs_delete(client, "/room/test-room/reaction/10/🍍", user).json['removed']
+    assert sogs_delete(client, "/room/test-room/reaction/10/🍍", global_mod).json == {
+        'removed': False,
+        'seqno': seqno_10,
+    }
+    assert sogs_delete(client, "/room/test-room/reaction/10/🍍", user).json == {
+        'removed': True,
+        'seqno': seqno + 1,
+    }
 
-    assert sogs_put(client, "/room/test-room/reaction/9/🍍", {}, user).json['added']
-    assert sogs_put(client, "/room/test-room/reaction/9/🍍", {}, user2).json['added']
+    assert sogs_put(client, "/room/test-room/reaction/9/🍍", {}, user).json == {
+        'added': True,
+        "seqno": seqno + 2,
+    }
+    assert sogs_put(client, "/room/test-room/reaction/9/🍍", {}, user2).json == {
+        'added': True,
+        "seqno": seqno + 3,
+    }
     r = sogs_get(client, "/room/test-room/message/9", mod).json
     assert 'reactions' in r
     assert r.get('reactions') == {
@@ -263,27 +282,27 @@ def test_reaction_ordering(client, room, user, user2):
     for x in ("🖕", "f", "🍆", "y/n", "abcdefghijkl", "🍍"):
         r = sogs_put(client, f"/room/test-room/reaction/1/{x}", {}, user)
         assert r.status_code == 200
-        assert r.json["added"]
         seqno += 1
+        assert r.json == {"added": True, "seqno": seqno}
 
     for x in ("‽", "abcdefghijkl", "f", "🍍", "🖕"):
         r = sogs_put(client, f"/room/test-room/reaction/2/{x}", {}, user2)
         assert r.status_code == 200
-        assert r.json["added"]
         seqno += 1
+        assert r.json == {"added": True, "seqno": seqno}
 
     for x in ("🖕", "f", "🍆", "y/n", "abcdefghijkl", "🍍", "🫑"):
         r = sogs_put(client, f"/room/test-room/reaction/2/{x}", {}, user)
         assert r.status_code == 200
-        assert r.json["added"]
         seqno += 1
+        assert r.json == {"added": True, "seqno": seqno}
     seqno_2 = seqno
 
     for x in ("abcdefghijkl", "f", "🍍", "🖕", "🎂"):
         r = sogs_put(client, f"/room/test-room/reaction/1/{x}", {}, user2)
         assert r.status_code == 200
-        assert r.json["added"]
         seqno += 1
+        assert r.json == {"added": True, "seqno": seqno}
 
     u1 = [user.session_id]
     u2 = [user2.session_id]
@@ -316,7 +335,10 @@ def test_reaction_ordering(client, room, user, user2):
     ]
 
     # Deleting a user reaction while the post has other user reactions should not affect the order:
-    assert sogs_delete(client, "/room/test-room/reaction/1/f", user).json['removed']
+    assert sogs_delete(client, "/room/test-room/reaction/1/f", user).json == {
+        'removed': True,
+        'seqno': seqno + 1,
+    }
     seqno += 1
     exp_reacts_1["f"]["count"] -= 1
     exp_reacts_1["f"]["reactors"] = u2
@@ -329,8 +351,14 @@ def test_reaction_ordering(client, room, user, user2):
 
     # Deleting the last reaction and then adding it again should put it back at the *end*, not in
     # its original position:
-    assert sogs_delete(client, "/room/test-room/reaction/1/f", user2).json['removed']
-    assert sogs_put(client, "/room/test-room/reaction/1/f", {}, user2).json['added']
+    assert sogs_delete(client, "/room/test-room/reaction/1/f", user2).json == {
+        'removed': True,
+        'seqno': seqno + 1,
+    }
+    assert sogs_put(client, "/room/test-room/reaction/1/f", {}, user2).json == {
+        'added': True,
+        'seqno': seqno + 2,
+    }
     seqno += 2
 
     for v in exp_reacts_1.values():