Implement correct behavior for min_id in boundary pagination

- The previous behavior of since_id systematically showed the most recent results
This commit is contained in:
Hypolite Petovan 2020-10-12 23:45:02 -04:00
parent e0a6b90316
commit 4427876c05
10 changed files with 101 additions and 59 deletions

View 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 * 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. * 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. * Chainable.
* *
* @param array $condition * @param array $condition
* @param array $params * @param array $params
* @param int? $max_id * @param int? $min_id Retrieve models with an id no fewer than this, as close to it as possible
* @param int? $since_id * @param int? $max_id Retrieve models with an id no greater than this, as close to it as possible
* @param int $limit * @param int $limit
* @return BaseCollection * @return BaseCollection
* @throws \Exception * @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); $totalCount = DBA::count(static::$table_name, $condition);
$boundCondition = $condition; $boundCondition = $condition;
if (isset($max_id)) { $reverseModels = false;
$boundCondition = DBA::mergeConditions($boundCondition, ['`id` < ?', $max_id]);
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)) { if (isset($max_id)) {
$boundCondition = DBA::mergeConditions($boundCondition, ['`id` > ?', $since_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; $params['limit'] = $limit;
$models = $this->selectModels($boundCondition, $params); $models = $this->selectModels($boundCondition, $params);
if ($reverseModels) {
$models = array_reverse($models);
}
return new static::$collection_class($models, $totalCount); return new static::$collection_class($models, $totalCount);
} }
@ -217,6 +236,8 @@ abstract class BaseRepository extends BaseFactory
} }
} }
$this->dba->close($result);
return $models; return $models;
} }

View File

@ -27,7 +27,7 @@ use Friendica\Util\Network;
use Friendica\Util\Strings; 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† * 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. * 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'])) { if (!empty($parsed['query'])) {
parse_str($parsed['query'], $queryParameters); 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']); unset($queryParameters['max_id']);
$parsed['query'] = http_build_query($queryParameters); $parsed['query'] = http_build_query($queryParameters);
@ -111,7 +111,7 @@ class BoundariesPager extends Pager
'prev' => [ 'prev' => [
'url' => Strings::ensureQueryParameter($this->baseQueryString . 'url' => Strings::ensureQueryParameter($this->baseQueryString .
($this->first_item_id >= $this->last_item_id ? ($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'), 'text' => $this->l10n->t('newer'),
'class' => 'previous' . ($this->first_page ? ' disabled' : '') 'class' => 'previous' . ($this->first_page ? ' disabled' : '')
@ -119,7 +119,7 @@ class BoundariesPager extends Pager
'next' => [ 'next' => [
'url' => Strings::ensureQueryParameter($this->baseQueryString . 'url' => Strings::ensureQueryParameter($this->baseQueryString .
($this->first_item_id >= $this->last_item_id ? ($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'), 'text' => $this->l10n->t('older'),
'class' => 'next' . ($displayedItemCount < $this->getItemsPerPage() ? ' disabled' : '') 'class' => 'next' . ($displayedItemCount < $this->getItemsPerPage() ? ' disabled' : '')

View File

@ -91,7 +91,7 @@ class FollowRequests extends BaseApi
*/ */
public static function rawContent(array $parameters = []) public static function rawContent(array $parameters = [])
{ {
$since_id = $_GET['since_id'] ?? null; $min_id = $_GET['min_id'] ?? null;
$max_id = $_GET['max_id'] ?? null; $max_id = $_GET['max_id'] ?? null;
$limit = intval($_GET['limit'] ?? 40); $limit = intval($_GET['limit'] ?? 40);
@ -100,7 +100,7 @@ class FollowRequests extends BaseApi
$introductions = DI::intro()->selectByBoundaries( $introductions = DI::intro()->selectByBoundaries(
['`uid` = ? AND NOT `ignore`', self::$current_user_id], ['`uid` = ? AND NOT `ignore`', self::$current_user_id],
['order' => ['id' => 'DESC']], ['order' => ['id' => 'DESC']],
$since_id, $min_id,
$max_id, $max_id,
$limit $limit
); );
@ -127,7 +127,7 @@ class FollowRequests extends BaseApi
} }
if (count($introductions)) { 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)); header('Link: ' . implode(', ', $links));

View File

@ -155,15 +155,24 @@ abstract class ContactEndpoint extends BaseApi
$total_count = (int)DBA::count('contact', $condition); $total_count = (int)DBA::count('contact', $condition);
$params = ['limit' => $count, 'order' => ['id' => 'ASC']];
if ($cursor !== -1) { if ($cursor !== -1) {
if ($cursor > 0) { if ($cursor > 0) {
$condition = DBA::mergeConditions($condition, ['`id` > ?', $cursor]); $condition = DBA::mergeConditions($condition, ['`id` > ?', $cursor]);
} else { } else {
$condition = DBA::mergeConditions($condition, ['`id` < ?', -$cursor]); $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 // Contains user-specific contact ids
$ids = array_column($contacts, 'id'); $ids = array_column($contacts, 'id');

View File

@ -44,7 +44,7 @@ class Community extends BaseModule
protected static $content; protected static $content;
protected static $accounttype; protected static $accounttype;
protected static $itemsPerPage; protected static $itemsPerPage;
protected static $since_id; protected static $min_id;
protected static $max_id; protected static $max_id;
protected static $item_id; protected static $item_id;
@ -98,8 +98,8 @@ class Community extends BaseModule
} }
$query_parameters = []; $query_parameters = [];
if (!empty($_GET['since_id'])) { if (!empty($_GET['min_id'])) {
$query_parameters['since_id'] = $_GET['since_id']; $query_parameters['min_id'] = $_GET['min_id'];
} }
if (!empty($_GET['max_id'])) { if (!empty($_GET['max_id'])) {
$query_parameters['max_id'] = $_GET['max_id']; $query_parameters['max_id'] = $_GET['max_id'];
@ -247,7 +247,7 @@ class Community extends BaseModule
self::$item_id = 0; 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['max_id'] ?? null;
self::$max_id = $_GET['last_commented'] ?? self::$max_id; self::$max_id = $_GET['last_commented'] ?? self::$max_id;
} }
@ -263,7 +263,7 @@ class Community extends BaseModule
*/ */
protected static function getItems() 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'); $maxpostperauthor = (int) DI::config()->get('system', 'max_author_posts_community_page');
if ($maxpostperauthor != 0 && self::$content == 'local') { 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 // If we're looking at a "previous page", the lookup continues forward in time because the list is
// sorted in chronologically decreasing order // sorted in chronologically decreasing order
if (isset(self::$since_id)) { if (isset(self::$min_id)) {
self::$since_id = $items[0]['commented']; self::$min_id = $items[0]['commented'];
} else { } else {
// In any other case, the lookup continues backwards in time // In any other case, the lookup continues backwards in time
self::$max_id = $items[count($items) - 1]['commented']; 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 { } else {
$selected_items = $items; $selected_items = $items;
@ -307,17 +307,15 @@ class Community extends BaseModule
/** /**
* Database query for the comunity page * Database query for the comunity page
* *
* @param $since_id * @param $min_id
* @param $max_id * @param $max_id
* @param $itemspage * @param $itemspage
* @return array * @return array
* @throws \Exception * @throws \Exception
* @TODO Move to repository/factory * @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 (self::$content == 'local') {
if (!is_null(self::$accounttype)) { if (!is_null(self::$accounttype)) {
$condition = ["`wall` AND `origin` AND `private` = ? AND `owner`.`contact-type` = ?", Item::PUBLIC, 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 []; return [];
} }
$params = ['order' => ['commented' => true], 'limit' => $itemspage];
if (!empty($item_id)) { if (!empty($item_id)) {
$condition[0] .= " AND `iid` = ?"; $condition[0] .= " AND `iid` = ?";
$condition[] = $item_id; $condition[] = $item_id;
@ -348,14 +348,26 @@ class Community extends BaseModule
$condition[] = $max_id; $condition[] = $max_id;
} }
if (isset($since_id)) { if (isset($min_id)) {
$condition[0] .= " AND `commented` > ?"; $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;
} }
} }

View File

@ -80,14 +80,14 @@ class FSuggest extends BaseRepository
/** /**
* @param array $condition * @param array $condition
* @param array $params * @param array $params
* @param int|null $min_id
* @param int|null $max_id * @param int|null $max_id
* @param int|null $since_id
* @param int $limit * @param int $limit
* @return Collection\FSuggests * @return Collection\FSuggests
* @throws \Exception * @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);
} }
} }

View File

@ -66,14 +66,14 @@ class Introduction extends BaseRepository
/** /**
* @param array $condition * @param array $condition
* @param array $params * @param array $params
* @param int|null $min_id
* @param int|null $max_id * @param int|null $max_id
* @param int|null $since_id
* @param int $limit * @param int $limit
* @return Collection\Introductions * @return Collection\Introductions
* @throws \Exception * @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);
} }
} }

View File

@ -95,15 +95,15 @@ class PermissionSet extends BaseRepository
/** /**
* @param array $condition * @param array $condition
* @param array $params * @param array $params
* @param int|null $min_id
* @param int|null $max_id * @param int|null $max_id
* @param int|null $since_id
* @param int $limit * @param int $limit
* @return Collection\PermissionSets * @return Collection\PermissionSets
* @throws \Exception * @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);
} }
/** /**

View File

@ -89,15 +89,15 @@ class ProfileField extends BaseRepository
/** /**
* @param array $condition * @param array $condition
* @param array $params * @param array $params
* @param int|null $min_id
* @param int|null $max_id * @param int|null $max_id
* @param int|null $since_id
* @param int $limit * @param int $limit
* @return Collection\ProfileFields * @return Collection\ProfileFields
* @throws \Exception * @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);
} }
/** /**

View File

@ -536,7 +536,7 @@ function updateConvItems(data) {
if ($('#' + ident).length === 0 if ($('#' + ident).length === 0
&& (!getUrlParameter('page') && (!getUrlParameter('page')
&& !getUrlParameter('max_id') && !getUrlParameter('max_id')
&& !getUrlParameter('since_id') && !getUrlParameter('min_id')
|| getUrlParameter('page') === '1' || getUrlParameter('page') === '1'
) )
) { ) {
@ -609,8 +609,8 @@ function liveUpdate(src) {
if (getUrlParameter('page')) { if (getUrlParameter('page')) {
update_url += '&page=' + getUrlParameter('page'); update_url += '&page=' + getUrlParameter('page');
} }
if (getUrlParameter('since_id')) { if (getUrlParameter('min_id')) {
update_url += '&since_id=' + getUrlParameter('since_id'); update_url += '&min_id=' + getUrlParameter('min_id');
} }
if (getUrlParameter('max_id')) { if (getUrlParameter('max_id')) {
update_url += '&max_id=' + getUrlParameter('max_id'); update_url += '&max_id=' + getUrlParameter('max_id');