From e658820e19a045cd6e08b79d1800805b47d6c011 Mon Sep 17 00:00:00 2001 From: Sean Molenaar Date: Tue, 13 Apr 2021 13:50:31 +0200 Subject: DB: only sort on item IDs Signed-off-by: Sean Molenaar --- AUTHORS.md | 16 +++++- CHANGELOG.md | 1 + lib/Db/ItemMapperV2.php | 87 ++++++++++++++++--------------- tests/Unit/Db/ItemMapperPaginatedTest.php | 44 ++++++---------- tests/Unit/Db/ItemMapperTest.php | 6 +-- 5 files changed, 79 insertions(+), 75 deletions(-) diff --git a/AUTHORS.md b/AUTHORS.md index e6d7dca80..afeda70f2 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -2,23 +2,28 @@ * [Bernhard Posselt](mailto:dev@bernhard-posselt.com) * [Alessandro Cosentino](mailto:cosenal@gmail.com) * [Benjamin Brahmer](mailto:info@b-brahmer.de) +* [Marco Nassabain](mailto:marco.nassabain@hotmail.com) * [Robin Appelman](mailto:icewind@owncloud.com) * [Sean Molenaar](mailto:sean@seanmolenaar.eu) * [Gregor Tätzner](mailto:gregor@freenet.de) * [Sean Molenaar](mailto:SMillerDev@users.noreply.github.com) * [Morris Jobke](mailto:hey@morrisjobke.de) * [anoy](mailto:anoymouserver+github@mailbox.org) +* [Jimmy Huynh](mailto:jimmy.huynh@etu.unistra.fr) +* [Aurélien](mailto:dav.aurelien@gmail.com) * [Jan-Christoph Borchardt](mailto:hey@jancborchardt.net) +* [Paul Tirk](mailto:paultirk@paultirk.com) * [Daniel Schaal](mailto:daniel@schaal.email) * [Davide Saurino](mailto:davide.saurino@alcacoop.it) * [raghunayyar](mailto:me@iraghu.com) +* [WENDLING NICOLAS](mailto:nicolas.wendling2@etu.unistra.fr) * [bastei](mailto:bastei@users.noreply.github.com) * [Bernhard Posselt](mailto:bep@foryouandyourcustomers.com) * [Thomas Müller](mailto:thomas.mueller@tmit.eu) * [Hoàng Đức Hiếu](mailto:hdhoang@zahe.me) +* [Marco Nassabain](mailto:marco.nassabain@etu.unistra.fr) * [rakekniven](mailto:mark.ziegler@rakekniven.de) * [Daniel Opitz](mailto:git@copynpaste.de) -* [Marco Nassabain](mailto:marco.nassabain@hotmail.com) * [Sean Molenaar](mailto:sean@m2mobi.com) * [David-Development](mailto:david-dev@live.de) * [IBBoard](mailto:dev@ibboard.co.uk) @@ -26,7 +31,7 @@ * [Lukas Reschke](mailto:lukas@owncloud.com) * [Bart Visscher](mailto:bartv@thisnet.nl) * [Christian Elmer](mailto:christian@keinkurt.de) -* [Jimmy Huynh](mailto:jimmy.huynh@etu.unistra.fr) +* [Nicolas Wendling](mailto:nicolas.wendling1011@gmail.com) * [Thomas Tanghus](mailto:thomas@tanghus.net) * [Volkan Gezer](mailto:volkangezer@gmail.com) * [Xéfir Destiny](mailto:xefir@crystalyx.net) @@ -39,11 +44,13 @@ * [Brice Maron](mailto:brice@bmaron.net) * [Christoph Stenglein](mailto:christoph@christophstenglein.com) * [Daniel Kesselberg](mailto:mail@danielkesselberg.de) +* [ELHADDAD Hamza](mailto:elhaddadhamza49@gmail.com) * [Jakob Sack](mailto:mail@jakobsack.de) * [Qingping Hou](mailto:dave2008713@gmail.com) * [Roman](mailto:reverse@jamm.me) * [b_b](mailto:bruno@eliaz.fr) * [heyarne](mailto:arne@schlueter.is) +* [marco.nassabain@etu.unistra.fr](mailto:marco.nassabain@hotmail.com) * [Andreas Fischer](mailto:bantu@owncloud.com) * [David Guillot](mailto:david@guillot.me) * [Gioele Falcetti](mailto:thegio.f@gmail.com) @@ -73,6 +80,8 @@ * [Andrea Boero](mailto:mail@tsumi.it) * [Andreas Demmelbauer](mailto:git@notice.at) * [Arthur Schiwon](mailto:blizzz@arthur-schiwon.de) +* [Aurelien DAVID](mailto:aurelien.david@etu.unistra.fr) +* [Aurelien DAVID](mailto:dav.aurelien@gmail.com) * [Benedikt Geißler](mailto:benedikt@g5r.eu) * [Bernhard Posselt](mailto:bernhard@desktop.localdomain) * [Candid Dauth](mailto:cdauth@cdauth.eu) @@ -132,6 +141,7 @@ * [Tilo Spannagel](mailto:development@tilosp.de) * [Timo Schmidt](mailto:timo@xinterchange.net) * [Tucker McKnight](mailto:tucker.mcknight@gmail.com) +* [WENDLING NICOLAS](mailto:nicolas.wendling1011@gmail.com) * [Welling Guzmán](mailto:WellingGuzman@users.noreply.github.com) * [Xaver Maierhofer](mailto:xaver.maierhofer@xwissen.info) * [Xemle](mailto:xemle@phtagr.org) @@ -141,6 +151,7 @@ * [b_b](mailto:brunobergot@gmail.com) * [bjoerns1983](mailto:bjoern@sengotta.net) * [blackcrack](mailto:blackcrack@blackysgate.de) +* [cherguimalih](mailto:ilyes.chergui-malih@etu.unistra.fr) * [comradekingu](mailto:epost@anotheragency.no) * [derritter88](mailto:derritter88@users.noreply.github.com) * [e-alfred](mailto:e-alfred@users.noreply.github.com) @@ -149,6 +160,7 @@ * [kesselb](mailto:mail@danielkesselberg.de) * [kondou](mailto:kondou@ts.unde.re) * [markusj](mailto:markusj@users.noreply.github.com) +* [mnassabain](mailto:34754819+mnassabain@users.noreply.github.com) * [nexus-uw](mailto:you@example.com) * [repat](mailto:repat@repat.de) * [ritchiewilson](mailto:rawilson52@gmail.com) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf5f29691..bc44acd27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The format is almost based on [Keep a Changelog](https://keepachangelog.com/en/1 ### Fixed - Check category label for null (#1282) - Do not return non-matching search items +- Resolve an issue with webservices missing items ## [15.4.0-beta3] - 2021-04-03 ### Fixed diff --git a/lib/Db/ItemMapperV2.php b/lib/Db/ItemMapperV2.php index 44d1740cd..6e50a5fb5 100644 --- a/lib/Db/ItemMapperV2.php +++ b/lib/Db/ItemMapperV2.php @@ -62,7 +62,7 @@ class ItemMapperV2 extends NewsMapperV2 ->setParameter('user_id', $userId, IQueryBuilder::PARAM_STR); foreach ($params as $key => $value) { - $builder->andWhere("${key} = " . $builder->createNamedParameter($value)); + $builder->andWhere("$key = " . $builder->createNamedParameter($value)); } return $this->findEntities($builder); @@ -201,7 +201,6 @@ class ItemMapperV2 extends NewsMapperV2 ->from($this->tableName) ->where('feed_id = :feedId') ->andWhere('starred = false') - ->orderBy('last_modified', 'DESC') ->addOrderBy('id', 'DESC'); if ($removeUnread === false) { @@ -425,22 +424,6 @@ class ItemMapperV2 extends NewsMapperV2 return $this->findEntities($builder); } - /** - * Generate an expression for the offset. - * - * @param bool $oldestFirst Sorting direction - * - * @return string - */ - private function offsetWhere(bool $oldestFirst): string - { - if ($oldestFirst === true) { - return 'items.id > :offset'; - } - - return 'items.id < :offset'; - } - /** * @param string $userId User identifier * @param int $feedId Feed identifier @@ -471,16 +454,9 @@ class ItemMapperV2 extends NewsMapperV2 ->andWhere('items.feed_id = :feedId') ->setParameter('userId', $userId) ->setParameter('feedId', $feedId) - ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC')) ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC')); - if ($search !== []) { - foreach ($search as $key => $term) { - $term = $this->db->escapeLikeParameter($term); - $builder->andWhere("items.search_index LIKE :term${key}") - ->setParameter("term${key}", "%$term%"); - } - } + $builder = $this->addSearch($builder, $search); if ($limit >= 1) { $builder->setMaxResults($limit); @@ -534,16 +510,9 @@ class ItemMapperV2 extends NewsMapperV2 ->andWhere('feeds.deleted_at = 0') ->andWhere($folderWhere) ->setParameter('userId', $userId) - ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC')) ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC')); - if ($search !== []) { - foreach ($search as $key => $term) { - $term = $this->db->escapeLikeParameter($term); - $builder->andWhere("items.search_index LIKE :term${key}") - ->setParameter("term${key}", "%$term%"); - } - } + $builder = $this->addSearch($builder, $search); if ($limit >= 1) { $builder->setMaxResults($limit); @@ -589,17 +558,8 @@ class ItemMapperV2 extends NewsMapperV2 ->andWhere('feeds.user_id = :userId') ->andWhere('feeds.deleted_at = 0') ->setParameter('userId', $userId) - ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC')) ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC')); - if ($search !== []) { - foreach ($search as $key => $term) { - $term = $this->db->escapeLikeParameter($term); - $builder->andWhere("items.search_index LIKE :term${key}") - ->setParameter("term${key}", "%$term%"); - } - } - if ($limit >= 1) { $builder->setMaxResults($limit); } @@ -609,6 +569,8 @@ class ItemMapperV2 extends NewsMapperV2 ->setParameter('offset', $offset); } + $builder = $this->addSearch($builder, $search); + switch ($type) { case ListType::STARRED: $builder->andWhere('items.starred = :starred') @@ -626,4 +588,43 @@ class ItemMapperV2 extends NewsMapperV2 return $this->findEntities($builder); } + + /** + * Add search parameters. + * + * @param IQueryBuilder $builder + * @param array $terms + * + * @return IQueryBuilder + */ + private function addSearch(IQueryBuilder $builder, array $terms): IQueryBuilder + { + if ($terms === []) { + return $builder; + } + + foreach ($terms as $key => $term) { + $term = $this->db->escapeLikeParameter($term); + $builder->andWhere("items.search_index LIKE :term$key") + ->setParameter("term$key", "%$term%"); + } + + return $builder; + } + + /** + * Generate an expression for the offset. + * + * @param bool $oldestFirst Sorting direction + * + * @return string + */ + private function offsetWhere(bool $oldestFirst): string + { + if ($oldestFirst === true) { + return 'items.id > :offset'; + } + + return 'items.id < :offset'; + } } diff --git a/tests/Unit/Db/ItemMapperPaginatedTest.php b/tests/Unit/Db/ItemMapperPaginatedTest.php index 448622280..652e2fa98 100644 --- a/tests/Unit/Db/ItemMapperPaginatedTest.php +++ b/tests/Unit/Db/ItemMapperPaginatedTest.php @@ -92,11 +92,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) - ->method('orderBy') - ->with('items.last_modified', 'DESC') - ->will($this->returnSelf()); - $this->builder->expects($this->once()) ->method('addOrderBy') ->with('items.id', 'DESC') @@ -163,11 +158,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) - ->method('orderBy') - ->with('items.last_modified', 'ASC') - ->will($this->returnSelf()); - $this->builder->expects($this->once()) ->method('addOrderBy') ->with('items.id', 'ASC') @@ -234,7 +224,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -303,7 +293,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -375,7 +365,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -429,9 +419,9 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive( ['feeds.user_id = :userId'], ['feeds.deleted_at = 0'], + ['items.id < :offset'], ['items.search_index LIKE :term0'], ['items.search_index LIKE :term1'], - ['items.id < :offset'], ['items.starred = :starred'] ) ->will($this->returnSelf()); @@ -440,9 +430,9 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('setParameter') ->withConsecutive( ['userId', 'jack'], + ['offset', 10], ['term0', '%key%'], ['term1', '%word%'], - ['offset', 10], ['starred', true] ) ->will($this->returnSelf()); @@ -459,7 +449,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -532,7 +522,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -603,7 +593,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -674,7 +664,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'ASC') ->will($this->returnSelf()); @@ -748,7 +738,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -832,7 +822,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -917,7 +907,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -991,7 +981,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->will($this->returnSelf()); - $this->builder->expects($this->never(1)) + $this->builder->expects($this->never()) ->method('setMaxResults'); @@ -1000,7 +990,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -1086,7 +1076,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); @@ -1172,7 +1162,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'ASC') ->will($this->returnSelf()); @@ -1262,7 +1252,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->once()) + $this->builder->expects($this->never()) ->method('orderBy') ->with('items.last_modified', 'DESC') ->will($this->returnSelf()); diff --git a/tests/Unit/Db/ItemMapperTest.php b/tests/Unit/Db/ItemMapperTest.php index 5cb42c3fa..12c022893 100644 --- a/tests/Unit/Db/ItemMapperTest.php +++ b/tests/Unit/Db/ItemMapperTest.php @@ -752,7 +752,7 @@ class ItemMapperTest extends MapperTestUtility ->with('starred = false') ->willReturnSelf(); - $builder2->expects($this->once()) + $builder2->expects($this->never()) ->method('orderBy') ->with('last_modified', 'DESC') ->willReturnSelf(); @@ -910,7 +910,7 @@ class ItemMapperTest extends MapperTestUtility ->withConsecutive(['starred = false'], ['unread = false']) ->willReturnSelf(); - $builder2->expects($this->once()) + $builder2->expects($this->never()) ->method('orderBy') ->with('last_modified', 'DESC') ->willReturnSelf(); @@ -1067,7 +1067,7 @@ class ItemMapperTest extends MapperTestUtility ->withConsecutive(['starred = false'], ['unread = false']) ->willReturnSelf(); - $builder2->expects($this->once()) + $builder2->expects($this->never()) ->method('orderBy') ->with('last_modified', 'DESC') ->willReturnSelf(); -- cgit v1.2.3