From 0a3093ef9c1898913196564435346e4daa9d2538 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 26 Feb 2021 10:41:53 -0800
Subject: [PATCH] Fix an issue where paginating notifications could fail a
 GROUP BY test

Summary:
Ref T13623. When paginating notifications, we may currently construct a query which:

  - loads from non-unique rows; and
  - returns multiple results.

In particular, `chronologicalKey` isn't unique across the whole table (only for a given viewer). We can get away with this because no user-facing view of notifications is truly "every notification for every viewer" today.

One fix would be to implicitly force the paging query to include `withUserPHIDs(viewerPHID)`, but puruse a slightly more general fix:

  - Load only unique stories.
  - Explictly limit the pagination subquery to one result.

Test Plan:
  - Set page size to 1, inserted duplicate notifications of all stories for another user, clicked "Next", got the GROUP BY error.
  - Applied the "only load unique stories" part of the change, got a "expected one row" error instead.
  - Applied the "limit 1" part of the change, got a second page of notifications.

Maniphest Tasks: T13623

Differential Revision: https://secure.phabricator.com/D21577
---
 .../query/PhabricatorNotificationQuery.php    | 24 ++++++++++++++-----
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/applications/notification/query/PhabricatorNotificationQuery.php b/src/applications/notification/query/PhabricatorNotificationQuery.php
index 6eea54a0de..4ceca2e1d4 100644
--- a/src/applications/notification/query/PhabricatorNotificationQuery.php
+++ b/src/applications/notification/query/PhabricatorNotificationQuery.php
@@ -63,17 +63,25 @@ final class PhabricatorNotificationQuery
       $this->buildWhereClause($conn),
       $this->buildLimitClause($conn));
 
-    $viewed_map = ipull($data, 'hasViewed', 'chronologicalKey');
+    // See T13623. Although most queries for notifications return unique
+    // stories, this isn't a guarantee.
+    $story_map = ipull($data, null, 'chronologicalKey');
 
     $stories = PhabricatorFeedStory::loadAllFromRows(
-      $data,
+      $story_map,
       $this->getViewer());
+    $stories = mpull($stories, null, 'getChronologicalKey');
 
-    foreach ($stories as $key => $story) {
-      $story->setHasViewed($viewed_map[$key]);
+    $results = array();
+    foreach ($data as $row) {
+      $story_key = $row['chronologicalKey'];
+      $has_viewed = $row['hasViewed'];
+
+      $results[] = id(clone $stories[$story_key])
+        ->setHasViewed($has_viewed);
     }
 
-    return $stories;
+    return $results;
   }
 
   protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
@@ -145,7 +153,11 @@ final class PhabricatorNotificationQuery
   protected function applyExternalCursorConstraintsToQuery(
     PhabricatorCursorPagedPolicyAwareQuery $subquery,
     $cursor) {
-    $subquery->withKeys(array($cursor));
+
+    $subquery
+      ->withKeys(array($cursor))
+      ->setLimit(1);
+
   }
 
   protected function newExternalCursorStringForResult($object) {