]> git.mxchange.org Git - friendica.git/commitdiff
Implement correct behavior for min_id in boundary pagination
authorHypolite Petovan <hypolite@mrpetovan.com>
Tue, 13 Oct 2020 03:45:02 +0000 (23:45 -0400)
committerHypolite Petovan <hypolite@mrpetovan.com>
Tue, 13 Oct 2020 04:11:39 +0000 (00:11 -0400)
- The previous behavior of since_id systematically showed the most recent results

src/BaseRepository.php
src/Content/BoundariesPager.php
src/Module/Api/Mastodon/FollowRequests.php
src/Module/Api/Twitter/ContactEndpoint.php
src/Module/Conversation/Community.php
src/Repository/FSuggest.php
src/Repository/Introduction.php
src/Repository/PermissionSet.php
src/Repository/ProfileField.php
view/js/main.js

index abec4c119b3f21c834cc73ba34017184ded8f94f..3e67cd5b20f5368133f1f4534ca8c57ec7cc5a56 100644 (file)
@@ -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;
        }
 
index 8bbbde2b4764eed12362073f86d6895cee940647..ebdd72c911abe2ad458080231e410a9dd810459a 100644 (file)
@@ -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' : '')
index 3e3ffec58e23fe46ad11804bf5298afbe4244e0f..e746c135c6cd7038b43f306d2c5c90602060de26 100644 (file)
@@ -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));
index 58807e62962fe951467978dacd19894ed582c4fe..4ab9cc97423537b0bb93a228c8aaca5d511386c3 100644 (file)
@@ -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');
index f9eeaa6fd944e0bcc40af4d99916865893204083..5ce76bd1747e4ae609036874d6aeda37ff1b7934 100644 (file)
@@ -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);
+
+               $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 DBA::toArray($r);
+               return $items;
        }
 }
index f7f6cef71bf92066f4c8acb053fa8385c502f952..c6c6b356016afd0ce5b3bf655be1a2cfefe1f068 100644 (file)
@@ -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);
        }
 }
index bde6edef6cd6a87f7f00d4604658e65228dd3596..37e7637d8b22dfd749680fc9e5dfdc15bd201a37 100644 (file)
@@ -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);
        }
 }
index ec0b91e0fe9c4e4e428739c424ca0f9e1009cd7e..b9d07e1669d7a88eb61cd2a36ff14ca7edcccfde 100644 (file)
@@ -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);
        }
 
        /**
index 7137191684868a51cdf46aab9fb253cf4309879b..3f8b1599b0a82c5d8a2914dbd30215bf9f82490a 100644 (file)
@@ -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);
        }
 
        /**
index 35a0529a0e0e7e118c71befc5a635a6c28349bfc..b523ce6e70f43af10b9004537ce7958306719542 100644 (file)
@@ -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');