recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

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

epriestley 0a3093ef 10162ad4

+18 -6
+18 -6
src/applications/notification/query/PhabricatorNotificationQuery.php
··· 63 63 $this->buildWhereClause($conn), 64 64 $this->buildLimitClause($conn)); 65 65 66 - $viewed_map = ipull($data, 'hasViewed', 'chronologicalKey'); 66 + // See T13623. Although most queries for notifications return unique 67 + // stories, this isn't a guarantee. 68 + $story_map = ipull($data, null, 'chronologicalKey'); 67 69 68 70 $stories = PhabricatorFeedStory::loadAllFromRows( 69 - $data, 71 + $story_map, 70 72 $this->getViewer()); 73 + $stories = mpull($stories, null, 'getChronologicalKey'); 71 74 72 - foreach ($stories as $key => $story) { 73 - $story->setHasViewed($viewed_map[$key]); 75 + $results = array(); 76 + foreach ($data as $row) { 77 + $story_key = $row['chronologicalKey']; 78 + $has_viewed = $row['hasViewed']; 79 + 80 + $results[] = id(clone $stories[$story_key]) 81 + ->setHasViewed($has_viewed); 74 82 } 75 83 76 - return $stories; 84 + return $results; 77 85 } 78 86 79 87 protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { ··· 145 153 protected function applyExternalCursorConstraintsToQuery( 146 154 PhabricatorCursorPagedPolicyAwareQuery $subquery, 147 155 $cursor) { 148 - $subquery->withKeys(array($cursor)); 156 + 157 + $subquery 158 + ->withKeys(array($cursor)) 159 + ->setLimit(1); 160 + 149 161 } 150 162 151 163 protected function newExternalCursorStringForResult($object) {