From 4427876c05023ecf681ea9e217f163444edcda8b Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Mon, 12 Oct 2020 23:45:02 -0400 Subject: [PATCH] Implement correct behavior for min_id in boundary pagination - The previous behavior of since_id systematically showed the most recent results --- src/BaseRepository.php | 35 +++++++++++++---- src/Content/BoundariesPager.php | 10 ++--- src/Module/Api/Mastodon/FollowRequests.php | 6 +-- src/Module/Api/Twitter/ContactEndpoint.php | 11 +++++- src/Module/Conversation/Community.php | 44 ++++++++++++++-------- src/Repository/FSuggest.php | 12 +++--- src/Repository/Introduction.php | 12 +++--- src/Repository/PermissionSet.php | 12 +++--- src/Repository/ProfileField.php | 12 +++--- view/js/main.js | 6 +-- 10 files changed, 101 insertions(+), 59 deletions(-) diff --git a/src/BaseRepository.php b/src/BaseRepository.php index abec4c119b..3e67cd5b20 100644 --- a/src/BaseRepository.php +++ b/src/BaseRepository.php @@ -97,34 +97,53 @@ abstract class BaseRepository extends BaseFactory * Populates the collection according to the condition. Retrieves a limited subset of models depending on the boundaries * and the limit. The total count of rows matching the condition is stored in the collection. * + * max_id and min_id are susceptible to the query order: + * - min_id alone only reliably works with ASC order + * - max_id alone only reliably works with DESC order + * If the wrong order is detected in either case, we inverse the query order and we reverse the model array after the query + * * Chainable. * * @param array $condition * @param array $params - * @param int? $max_id - * @param int? $since_id + * @param int? $min_id Retrieve models with an id no fewer than this, as close to it as possible + * @param int? $max_id Retrieve models with an id no greater than this, as close to it as possible * @param int $limit * @return BaseCollection * @throws \Exception */ - public function selectByBoundaries(array $condition = [], array $params = [], int $max_id = null, int $since_id = null, int $limit = self::LIMIT) + public function selectByBoundaries(array $condition = [], array $params = [], int $min_id = null, int $max_id = null, int $limit = self::LIMIT) { $totalCount = DBA::count(static::$table_name, $condition); $boundCondition = $condition; - if (isset($max_id)) { - $boundCondition = DBA::mergeConditions($boundCondition, ['`id` < ?', $max_id]); + $reverseModels = false; + + if (isset($min_id)) { + $boundCondition = DBA::mergeConditions($boundCondition, ['`id` > ?', $min_id]); + if (!isset($max_id) && isset($params['order']['id']) && ($params['order']['id'] === true || $params['order']['id'] === 'DESC')) { + $reverseModels = true; + $params['order']['id'] = 'ASC'; + } } - if (isset($since_id)) { - $boundCondition = DBA::mergeConditions($boundCondition, ['`id` > ?', $since_id]); + if (isset($max_id)) { + $boundCondition = DBA::mergeConditions($boundCondition, ['`id` < ?', $max_id]); + if (!isset($min_id) && (!isset($params['order']['id']) || $params['order']['id'] === false || $params['order']['id'] === 'ASC')) { + $reverseModels = true; + $params['order']['id'] = 'DESC'; + } } $params['limit'] = $limit; $models = $this->selectModels($boundCondition, $params); + if ($reverseModels) { + $models = array_reverse($models); + } + return new static::$collection_class($models, $totalCount); } @@ -217,6 +236,8 @@ abstract class BaseRepository extends BaseFactory } } + $this->dba->close($result); + return $models; } diff --git a/src/Content/BoundariesPager.php b/src/Content/BoundariesPager.php index 8bbbde2b47..ebdd72c911 100644 --- a/src/Content/BoundariesPager.php +++ b/src/Content/BoundariesPager.php @@ -27,7 +27,7 @@ use Friendica\Util\Network; use Friendica\Util\Strings; /** - * This pager should be used by lists using the since_id†/max_id† parameters + * This pager should be used by lists using the min_id†/max_id† parameters * * This pager automatically identifies if the sorting is done increasingly or decreasingly if the first item id† * and last item id† are different. Otherwise it defaults to decreasingly like reverse chronological lists. @@ -60,9 +60,9 @@ class BoundariesPager extends Pager if (!empty($parsed['query'])) { parse_str($parsed['query'], $queryParameters); - $this->first_page = !($queryParameters['since_id'] ?? null) && !($queryParameters['max_id'] ?? null); + $this->first_page = !($queryParameters['min_id'] ?? null) && !($queryParameters['max_id'] ?? null); - unset($queryParameters['since_id']); + unset($queryParameters['min_id']); unset($queryParameters['max_id']); $parsed['query'] = http_build_query($queryParameters); @@ -111,7 +111,7 @@ class BoundariesPager extends Pager 'prev' => [ 'url' => Strings::ensureQueryParameter($this->baseQueryString . ($this->first_item_id >= $this->last_item_id ? - '&since_id=' . $this->first_item_id : '&max_id=' . $this->first_item_id) + '&min_id=' . $this->first_item_id : '&max_id=' . $this->first_item_id) ), 'text' => $this->l10n->t('newer'), 'class' => 'previous' . ($this->first_page ? ' disabled' : '') @@ -119,7 +119,7 @@ class BoundariesPager extends Pager 'next' => [ 'url' => Strings::ensureQueryParameter($this->baseQueryString . ($this->first_item_id >= $this->last_item_id ? - '&max_id=' . $this->last_item_id : '&since_id=' . $this->last_item_id) + '&max_id=' . $this->last_item_id : '&min_id=' . $this->last_item_id) ), 'text' => $this->l10n->t('older'), 'class' => 'next' . ($displayedItemCount < $this->getItemsPerPage() ? ' disabled' : '') diff --git a/src/Module/Api/Mastodon/FollowRequests.php b/src/Module/Api/Mastodon/FollowRequests.php index 3e3ffec58e..e746c135c6 100644 --- a/src/Module/Api/Mastodon/FollowRequests.php +++ b/src/Module/Api/Mastodon/FollowRequests.php @@ -91,7 +91,7 @@ class FollowRequests extends BaseApi */ public static function rawContent(array $parameters = []) { - $since_id = $_GET['since_id'] ?? null; + $min_id = $_GET['min_id'] ?? null; $max_id = $_GET['max_id'] ?? null; $limit = intval($_GET['limit'] ?? 40); @@ -100,7 +100,7 @@ class FollowRequests extends BaseApi $introductions = DI::intro()->selectByBoundaries( ['`uid` = ? AND NOT `ignore`', self::$current_user_id], ['order' => ['id' => 'DESC']], - $since_id, + $min_id, $max_id, $limit ); @@ -127,7 +127,7 @@ class FollowRequests extends BaseApi } if (count($introductions)) { - $links[] = '<' . $baseUrl->get() . '/api/v1/follow_requests?' . http_build_query($base_query + ['since_id' => $introductions[0]->id]) . '>; rel="prev"'; + $links[] = '<' . $baseUrl->get() . '/api/v1/follow_requests?' . http_build_query($base_query + ['min_id' => $introductions[0]->id]) . '>; rel="prev"'; } header('Link: ' . implode(', ', $links)); diff --git a/src/Module/Api/Twitter/ContactEndpoint.php b/src/Module/Api/Twitter/ContactEndpoint.php index 58807e6296..4ab9cc9742 100644 --- a/src/Module/Api/Twitter/ContactEndpoint.php +++ b/src/Module/Api/Twitter/ContactEndpoint.php @@ -155,15 +155,24 @@ abstract class ContactEndpoint extends BaseApi $total_count = (int)DBA::count('contact', $condition); + $params = ['limit' => $count, 'order' => ['id' => 'ASC']]; + if ($cursor !== -1) { if ($cursor > 0) { $condition = DBA::mergeConditions($condition, ['`id` > ?', $cursor]); } else { $condition = DBA::mergeConditions($condition, ['`id` < ?', -$cursor]); + // Previous page case: we want the items closest to cursor but for that we need to reverse the query order + $params['order']['id'] = 'DESC'; } } - $contacts = Contact::selectToArray(['id'], $condition, ['limit' => $count, 'order' => ['id']]); + $contacts = Contact::selectToArray(['id'], $condition, $params); + + // Previous page case: once we get the relevant items closest to cursor, we need to restore the expected display order + if ($cursor !== -1 && $cursor <= 0) { + $contacts = array_reverse($contacts); + } // Contains user-specific contact ids $ids = array_column($contacts, 'id'); diff --git a/src/Module/Conversation/Community.php b/src/Module/Conversation/Community.php index f9eeaa6fd9..5ce76bd174 100644 --- a/src/Module/Conversation/Community.php +++ b/src/Module/Conversation/Community.php @@ -44,7 +44,7 @@ class Community extends BaseModule protected static $content; protected static $accounttype; protected static $itemsPerPage; - protected static $since_id; + protected static $min_id; protected static $max_id; protected static $item_id; @@ -98,8 +98,8 @@ class Community extends BaseModule } $query_parameters = []; - if (!empty($_GET['since_id'])) { - $query_parameters['since_id'] = $_GET['since_id']; + if (!empty($_GET['min_id'])) { + $query_parameters['min_id'] = $_GET['min_id']; } if (!empty($_GET['max_id'])) { $query_parameters['max_id'] = $_GET['max_id']; @@ -247,7 +247,7 @@ class Community extends BaseModule self::$item_id = 0; } - self::$since_id = $_GET['since_id'] ?? null; + self::$min_id = $_GET['min_id'] ?? null; self::$max_id = $_GET['max_id'] ?? null; self::$max_id = $_GET['last_commented'] ?? self::$max_id; } @@ -263,7 +263,7 @@ class Community extends BaseModule */ protected static function getItems() { - $items = self::selectItems(self::$since_id, self::$max_id, self::$item_id, self::$itemsPerPage); + $items = self::selectItems(self::$min_id, self::$max_id, self::$item_id, self::$itemsPerPage); $maxpostperauthor = (int) DI::config()->get('system', 'max_author_posts_community_page'); if ($maxpostperauthor != 0 && self::$content == 'local') { @@ -288,14 +288,14 @@ class Community extends BaseModule // If we're looking at a "previous page", the lookup continues forward in time because the list is // sorted in chronologically decreasing order - if (isset(self::$since_id)) { - self::$since_id = $items[0]['commented']; + if (isset(self::$min_id)) { + self::$min_id = $items[0]['commented']; } else { // In any other case, the lookup continues backwards in time self::$max_id = $items[count($items) - 1]['commented']; } - $items = self::selectItems(self::$since_id, self::$max_id, self::$item_id, self::$itemsPerPage); + $items = self::selectItems(self::$min_id, self::$max_id, self::$item_id, self::$itemsPerPage); } } else { $selected_items = $items; @@ -307,17 +307,15 @@ class Community extends BaseModule /** * Database query for the comunity page * - * @param $since_id + * @param $min_id * @param $max_id * @param $itemspage * @return array * @throws \Exception * @TODO Move to repository/factory */ - private static function selectItems($since_id, $max_id, $item_id, $itemspage) + private static function selectItems($min_id, $max_id, $item_id, $itemspage) { - $r = false; - if (self::$content == 'local') { if (!is_null(self::$accounttype)) { $condition = ["`wall` AND `origin` AND `private` = ? AND `owner`.`contact-type` = ?", Item::PUBLIC, self::$accounttype]; @@ -334,6 +332,8 @@ class Community extends BaseModule return []; } + $params = ['order' => ['commented' => true], 'limit' => $itemspage]; + if (!empty($item_id)) { $condition[0] .= " AND `iid` = ?"; $condition[] = $item_id; @@ -348,14 +348,26 @@ class Community extends BaseModule $condition[] = $max_id; } - if (isset($since_id)) { + if (isset($min_id)) { $condition[0] .= " AND `commented` > ?"; - $condition[] = $since_id; + $condition[] = $min_id; + + // Previous page case: we want the items closest to min_id but for that we need to reverse the query order + if (!isset($max_id)) { + $params['order']['commented'] = false; + } } } - $r = Item::selectThreadForUser(0, ['uri', 'commented', 'author-link'], $condition, ['order' => ['commented' => true], 'limit' => $itemspage]); + $r = Item::selectThreadForUser(0, ['uri', 'commented', 'author-link'], $condition, $params); - return DBA::toArray($r); + $items = DBA::toArray($r); + + // Previous page case: once we get the relevant items closest to min_id, we need to restore the expected display order + if (empty($item_id) && isset($min_id) && !isset($max_id)) { + $items = array_reverse($items); + } + + return $items; } } diff --git a/src/Repository/FSuggest.php b/src/Repository/FSuggest.php index f7f6cef71b..c6c6b35601 100644 --- a/src/Repository/FSuggest.php +++ b/src/Repository/FSuggest.php @@ -78,16 +78,16 @@ class FSuggest extends BaseRepository } /** - * @param array $condition - * @param array $params + * @param array $condition + * @param array $params + * @param int|null $min_id * @param int|null $max_id - * @param int|null $since_id - * @param int $limit + * @param int $limit * @return Collection\FSuggests * @throws \Exception */ - public function selectByBoundaries(array $condition = [], array $params = [], int $max_id = null, int $since_id = null, int $limit = self::LIMIT) + public function selectByBoundaries(array $condition = [], array $params = [], int $min_id = null, int $max_id = null, int $limit = self::LIMIT) { - return parent::selectByBoundaries($condition, $params, $max_id, $since_id, $limit); + return parent::selectByBoundaries($condition, $params, $min_id, $max_id, $limit); } } diff --git a/src/Repository/Introduction.php b/src/Repository/Introduction.php index bde6edef6c..37e7637d8b 100644 --- a/src/Repository/Introduction.php +++ b/src/Repository/Introduction.php @@ -64,16 +64,16 @@ class Introduction extends BaseRepository } /** - * @param array $condition - * @param array $params + * @param array $condition + * @param array $params + * @param int|null $min_id * @param int|null $max_id - * @param int|null $since_id - * @param int $limit + * @param int $limit * @return Collection\Introductions * @throws \Exception */ - public function selectByBoundaries(array $condition = [], array $params = [], int $max_id = null, int $since_id = null, int $limit = self::LIMIT) + public function selectByBoundaries(array $condition = [], array $params = [], int $min_id = null, int $max_id = null, int $limit = self::LIMIT) { - return parent::selectByBoundaries($condition, $params, $max_id, $since_id, $limit); + return parent::selectByBoundaries($condition, $params, $min_id, $max_id, $limit); } } diff --git a/src/Repository/PermissionSet.php b/src/Repository/PermissionSet.php index ec0b91e0fe..b9d07e1669 100644 --- a/src/Repository/PermissionSet.php +++ b/src/Repository/PermissionSet.php @@ -93,17 +93,17 @@ class PermissionSet extends BaseRepository } /** - * @param array $condition - * @param array $params + * @param array $condition + * @param array $params + * @param int|null $min_id * @param int|null $max_id - * @param int|null $since_id - * @param int $limit + * @param int $limit * @return Collection\PermissionSets * @throws \Exception */ - public function selectByBoundaries(array $condition = [], array $params = [], int $max_id = null, int $since_id = null, int $limit = self::LIMIT) + public function selectByBoundaries(array $condition = [], array $params = [], int $min_id = null, int $max_id = null, int $limit = self::LIMIT) { - return parent::selectByBoundaries($condition, $params, $max_id, $since_id, $limit); + return parent::selectByBoundaries($condition, $params, $min_id, $max_id, $limit); } /** diff --git a/src/Repository/ProfileField.php b/src/Repository/ProfileField.php index 7137191684..3f8b1599b0 100644 --- a/src/Repository/ProfileField.php +++ b/src/Repository/ProfileField.php @@ -87,17 +87,17 @@ class ProfileField extends BaseRepository } /** - * @param array $condition - * @param array $params + * @param array $condition + * @param array $params + * @param int|null $min_id * @param int|null $max_id - * @param int|null $since_id - * @param int $limit + * @param int $limit * @return Collection\ProfileFields * @throws \Exception */ - public function selectByBoundaries(array $condition = [], array $params = [], int $max_id = null, int $since_id = null, int $limit = self::LIMIT) + public function selectByBoundaries(array $condition = [], array $params = [], int $min_id = null, int $max_id = null, int $limit = self::LIMIT) { - return parent::selectByBoundaries($condition, $params, $max_id, $since_id, $limit); + return parent::selectByBoundaries($condition, $params, $min_id, $max_id, $limit); } /** diff --git a/view/js/main.js b/view/js/main.js index 35a0529a0e..b523ce6e70 100644 --- a/view/js/main.js +++ b/view/js/main.js @@ -536,7 +536,7 @@ function updateConvItems(data) { if ($('#' + ident).length === 0 && (!getUrlParameter('page') && !getUrlParameter('max_id') - && !getUrlParameter('since_id') + && !getUrlParameter('min_id') || getUrlParameter('page') === '1' ) ) { @@ -609,8 +609,8 @@ function liveUpdate(src) { if (getUrlParameter('page')) { update_url += '&page=' + getUrlParameter('page'); } - if (getUrlParameter('since_id')) { - update_url += '&since_id=' + getUrlParameter('since_id'); + if (getUrlParameter('min_id')) { + update_url += '&min_id=' + getUrlParameter('min_id'); } if (getUrlParameter('max_id')) { update_url += '&max_id=' + getUrlParameter('max_id');