From d1e0e1e305d3276b8f2b1dfe8ed6af97f1274c44 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Thu, 19 Jan 2023 00:09:36 +1300 Subject: [PATCH 1/3] ENH Make limit method return no results when zero is provided --- src/ORM/ArrayList.php | 32 +++++++++------- src/ORM/DataList.php | 18 +++++---- src/ORM/DataQuery.php | 6 +-- src/ORM/Limitable.php | 8 ++-- src/ORM/ListDecorator.php | 17 +++------ src/ORM/Queries/SQLSelect.php | 2 +- src/ORM/Search/SearchContext.php | 8 ++-- tests/php/ORM/ArrayListTest.php | 57 +++++++++++++++++++++++------ tests/php/ORM/DataListTest.php | 5 ++- tests/php/ORM/ListDecoratorTest.php | 5 ++- 10 files changed, 99 insertions(+), 59 deletions(-) diff --git a/src/ORM/ArrayList.php b/src/ORM/ArrayList.php index 3d3c98910..ae45dac2d 100644 --- a/src/ORM/ArrayList.php +++ b/src/ORM/ArrayList.php @@ -171,23 +171,29 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L return $result; } - /** - * Get a sub-range of this dataobjectset as an array - * Pass null to "remove the limit" - this is for consistency with DataList::limit(null) which itself will - * call SQLSelect::setLimit(null) - * - * @param int|null $length - * @param int $offset - * @return static - */ - public function limit($length, $offset = 0) + public function limit(?int $length, int $offset = 0): static { - if (!$length) { - $length = count($this->items ?? []); + if ($length === null) { + // If we unset the limit, we set the length to the size of the list. We still want the offset to be picked up + $length = count($this->items); + } + + if ($length < 0) { + throw new InvalidArgumentException("\$length can not be negative. $length was provided."); + } + + if ($offset < 0) { + throw new InvalidArgumentException("\$offset can not be negative. $offset was provided."); } $list = clone $this; - $list->items = array_slice($this->items ?? [], $offset ?? 0, $length); + + if ($length === 0) { + // If we set the limit to 0, we return an empty list. + $list->items = []; + } else { + $list->items = array_slice($this->items ?? [], $offset ?? 0, $length); + } return $list; } diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index 8fe1911c1..158039445 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -287,15 +287,19 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li /** * Return a new DataList instance with the records returned in this query * restricted by a limit clause. - * - * @param int $limit - * @param int $offset - * @return static */ - public function limit($limit, $offset = 0) + public function limit(?int $length, int $offset = 0): static { - return $this->alterDataQuery(function (DataQuery $query) use ($limit, $offset) { - $query->limit($limit, $offset); + if ($length !== null && $length < 0) { + throw new InvalidArgumentException("\$length can not be negative. $length was provided."); + } + + if ($offset < 0) { + throw new InvalidArgumentException("\$offset can not be negative. $offset was provided."); + } + + return $this->alterDataQuery(function (DataQuery $query) use ($length, $offset) { + $query->limit($length, $offset); }); } diff --git a/src/ORM/DataQuery.php b/src/ORM/DataQuery.php index 06acea92a..18951ecae 100644 --- a/src/ORM/DataQuery.php +++ b/src/ORM/DataQuery.php @@ -752,12 +752,8 @@ class DataQuery /** * Set the limit of this query. - * - * @param int $limit - * @param int $offset - * @return $this */ - public function limit($limit, $offset = 0) + public function limit(?int $limit, int $offset = 0): static { $this->query->setLimit($limit, $offset); return $this; diff --git a/src/ORM/Limitable.php b/src/ORM/Limitable.php index 120b104ba..f30083272 100644 --- a/src/ORM/Limitable.php +++ b/src/ORM/Limitable.php @@ -20,9 +20,9 @@ interface Limitable extends SS_List * If $offset is specified, then that many records at the beginning of the list will be skipped. * This matches the behaviour of the SQL LIMIT clause. * - * @param int $limit - * @param int $offset - * @return static + * If `$length` is null, then no limit is applied. If `$length` is 0, then an empty list is returned. + * + * @throws InvalidArgumentException if $length or offset are negative */ - public function limit($limit, $offset = 0); + public function limit(?int $length, int $offset = 0): Limitable; } diff --git a/src/ORM/ListDecorator.php b/src/ORM/ListDecorator.php index 7a3226dd4..5a682964a 100644 --- a/src/ORM/ListDecorator.php +++ b/src/ORM/ListDecorator.php @@ -17,21 +17,16 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable, /** * @var SS_List */ - protected $list; + protected SS_List&Sortable&Filterable&Limitable $list; - public function __construct(SS_List $list) + public function __construct(SS_List&Sortable&Filterable&Limitable $list) { $this->setList($list); parent::__construct(); } - /** - * Returns the list this decorator wraps around. - * - * @return SS_List - */ - public function getList() + public function getList(): SS_List&Sortable&Filterable&Limitable { return $this->list; } @@ -44,7 +39,7 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable, * * @return SS_List */ - public function setList($list) + public function setList(SS_List&Sortable&Filterable&Limitable $list): self { $this->list = $list; $this->failover = $this->list; @@ -246,9 +241,9 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable, return $output; } - public function limit($limit, $offset = 0) + public function limit(?int $length, int $offset = 0): SS_List&Sortable&Filterable&Limitable { - return $this->list->limit($limit, $offset); + return $this->list->limit($length, $offset); } /** diff --git a/src/ORM/Queries/SQLSelect.php b/src/ORM/Queries/SQLSelect.php index 9b2f3c440..fa38005aa 100644 --- a/src/ORM/Queries/SQLSelect.php +++ b/src/ORM/Queries/SQLSelect.php @@ -249,7 +249,7 @@ class SQLSelect extends SQLConditionalExpression throw new InvalidArgumentException("SQLSelect::setLimit() only takes positive values"); } - if (is_numeric($limit) && ($limit || $offset)) { + if (is_numeric($limit)) { $this->limit = [ 'start' => (int)$offset, 'limit' => (int)$limit, diff --git a/src/ORM/Search/SearchContext.php b/src/ORM/Search/SearchContext.php index ca374ff69..3dae7058b 100644 --- a/src/ORM/Search/SearchContext.php +++ b/src/ORM/Search/SearchContext.php @@ -133,7 +133,7 @@ class SearchContext * for example "Comments__Name" instead of the filter name "Comments.Name". * @param array|bool|string $sort Database column to sort on. * Falls back to {@link DataObject::$default_sort} if not provided. - * @param array|bool|string $limit + * @param array|null|string $limit * @param DataList $existingQuery * @return DataList * @throws Exception @@ -168,7 +168,7 @@ class SearchContext * Prepare the query to begin searching * * @param array|bool|string $sort Database column to sort on. - * @param array|bool|string $limit + * @param array|null|string $limit */ private function prepareQuery($sort, $limit, ?DataList $existingQuery): DataList { @@ -320,11 +320,11 @@ class SearchContext * * @param array $searchParams * @param array|bool|string $sort - * @param array|bool|string $limit + * @param array|null|string $limit * @return DataList * @throws Exception */ - public function getResults($searchParams, $sort = false, $limit = false) + public function getResults($searchParams, $sort = false, $limit = null) { $searchParams = array_filter((array)$searchParams, [$this, 'clearEmptySearchFields']); diff --git a/tests/php/ORM/ArrayListTest.php b/tests/php/ORM/ArrayListTest.php index 0a8040c19..8a13ee084 100644 --- a/tests/php/ORM/ArrayListTest.php +++ b/tests/php/ORM/ArrayListTest.php @@ -117,34 +117,69 @@ class ArrayListTest extends SapphireTest $this->assertEquals($list->Count(), $count); } - public function testLimit() + public function limitDataProvider(): array { - $list = new ArrayList( - [ + $all = [ ['Key' => 1], ['Key' => 2], ['Key' => 3] ]; + list($one, $two, $three) = $all; + + return [ + 'smaller limit' => [2, 0, [$one, $two]], + 'limit equal to array' => [3, 0, $all], + 'limit bigger than array' => [4, 0, $all], + 'zero limit' => [0, 0, []], + 'false limit' => [0, 0, []], + 'null limit' => [null, 0, $all], + + 'smaller limit with offset' => [1, 1, [$two]], + 'limit to end with offset' => [2, 1, [$two, $three]], + 'bigger limit with offset' => [3, 1, [$two, $three]], + 'offset beyond end of list' => [4, 3, []], + 'zero limit with offset' => [0, 1, []], + 'null limit with offset' => [null, 2, [$three]], + + ]; + } + + /** + * @dataProvider limitDataProvider + */ + public function testLimit($length, $offset, array $expected) + { + $data = [ ['Key' => 1], ['Key' => 2], ['Key' => 3] - ] + ]; + $list = new ArrayList($data); + $this->assertEquals( + $list->limit($length, $offset)->toArray(), + $expected ); $this->assertEquals( - $list->limit(2, 1)->toArray(), - [ - ['Key' => 2], ['Key' => 3] - ] + $list->toArray(), + $data, + 'limit is immutable and does not affect the original list' ); } - public function testLimitNull() + public function testLimitNegative() { + $this->expectException(\InvalidArgumentException::class, 'Calling limit with a negative length throws exception'); $list = new ArrayList( [ ['Key' => 1], ['Key' => 2], ['Key' => 3] ] ); - $this->assertEquals( - $list->limit(null, 0)->toArray(), + $list->limit(-1); + } + + public function testLimitNegativeOffset() + { + $this->expectException(\InvalidArgumentException::class, 'Calling limit with a negative offset throws exception'); + $list = new ArrayList( [ ['Key' => 1], ['Key' => 2], ['Key' => 3] ] ); + $list->limit(1, -1); } public function testAddRemove() diff --git a/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index ebb13003a..94c12b3e3 100755 --- a/tests/php/ORM/DataListTest.php +++ b/tests/php/ORM/DataListTest.php @@ -134,8 +134,11 @@ class DataListTest extends SapphireTest $check = $list->limit(1, 1); $this->assertEquals(1, $check->count()); + $check = $list->limit(0); + $this->assertEquals(0, $check->count()); + $check = $list->limit(false); - $this->assertEquals(3, $check->count()); + $this->assertEquals(0, $check->count()); $check = $list->limit(null); $this->assertEquals(3, $check->count()); diff --git a/tests/php/ORM/ListDecoratorTest.php b/tests/php/ORM/ListDecoratorTest.php index 213857aa1..e0c011a90 100644 --- a/tests/php/ORM/ListDecoratorTest.php +++ b/tests/php/ORM/ListDecoratorTest.php @@ -243,8 +243,9 @@ class ListDecoratorTest extends SapphireTest public function testLimit() { - $this->list->expects($this->once())->method('limit')->with(5, 3)->willReturn('mock'); - $this->assertSame('mock', $this->decorator->limit(5, 3)); + $mockLimitedList = $this->list; + $this->list->expects($this->once())->method('limit')->with(5, 3)->willReturn($mockLimitedList); + $this->assertSame($mockLimitedList, $this->decorator->limit(5, 3)); } public function testTotalItems() From 5090f49ecc5f4f07ef8eda107c483658760c58f1 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Wed, 25 Jan 2023 16:44:13 +1300 Subject: [PATCH 2/3] MNT Add more test coverage to DataList::limit() --- tests/php/ORM/DataListTest.php | 52 ++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index 94c12b3e3..643a169a8 100755 --- a/tests/php/ORM/DataListTest.php +++ b/tests/php/ORM/DataListTest.php @@ -121,34 +121,42 @@ class DataListTest extends SapphireTest $this->assertEquals(['Joe', 'Phil'], $list->limit(2, 1)->column('Name')); } - public function testLimitAndOffset() + public function limitDataProvider(): array { + return [ + 'no limit' => [null, 0, 3], + 'smaller limit' => [2, 0, 2], + 'greater limit' => [4, 0, 3], + 'one limit' => [1, 0, 1], + 'zero limit' => [0, 0, 0], + 'limit and offset' => [1, 1, 1], + 'false limit equivalent to 0' => [false, 0, 0], + 'offset only' => [null, 2, 1], + 'offset greater than list length' => [null, 3, 0], + 'negative length' => [-1, 0, 0, true], + 'negative offset' => [0, -1, 0, true], + ]; + } + + /** + * @dataProvider limitDataProvider + */ + public function testLimitAndOffset($length, $offset, $expectedCount, $expectException = false) { $list = TeamComment::get(); - $check = $list->limit(3); - $this->assertEquals(3, $check->count()); + if ($expectException) { + $this->expectException(\InvalidArgumentException::class); + } - $check = $list->limit(1); - $this->assertEquals(1, $check->count()); - - $check = $list->limit(1, 1); - $this->assertEquals(1, $check->count()); - - $check = $list->limit(0); - $this->assertEquals(0, $check->count()); - - $check = $list->limit(false); - $this->assertEquals(0, $check->count()); - - $check = $list->limit(null); - $this->assertEquals(3, $check->count()); - - $check = $list->limit(null, 2); - $this->assertEquals(1, $check->count()); + $this->assertCount($expectedCount, $list->limit($length, $offset)); + $this->assertCount( + $expectedCount, + $list->limit(0,9999)->limit($length, $offset), + 'Follow up limit calls unset previous ones' + ); // count()/first()/last() methods may alter limit/offset, so run the query and manually check the count - $check = $list->limit(null, 1)->toArray(); - $this->assertEquals(2, count($check ?? [])); + $this->assertCount($expectedCount, $list->limit($length, $offset)->toArray()); } public function testDistinct() From fc6c45df57093e32d57ad882913028457e64c3f8 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Wed, 25 Jan 2023 17:19:30 +1300 Subject: [PATCH 3/3] MNT Add test to SQLSelect for Zero limit --- tests/php/ORM/DataListTest.php | 5 +++-- tests/php/ORM/SQLSelectTest.php | 13 +++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index 643a169a8..a50bb6598 100755 --- a/tests/php/ORM/DataListTest.php +++ b/tests/php/ORM/DataListTest.php @@ -121,7 +121,8 @@ class DataListTest extends SapphireTest $this->assertEquals(['Joe', 'Phil'], $list->limit(2, 1)->column('Name')); } - public function limitDataProvider(): array { + public function limitDataProvider(): array + { return [ 'no limit' => [null, 0, 3], 'smaller limit' => [2, 0, 2], @@ -151,7 +152,7 @@ class DataListTest extends SapphireTest $this->assertCount($expectedCount, $list->limit($length, $offset)); $this->assertCount( $expectedCount, - $list->limit(0,9999)->limit($length, $offset), + $list->limit(0, 9999)->limit($length, $offset), 'Follow up limit calls unset previous ones' ); diff --git a/tests/php/ORM/SQLSelectTest.php b/tests/php/ORM/SQLSelectTest.php index ce372e50a..be70ccb3a 100755 --- a/tests/php/ORM/SQLSelectTest.php +++ b/tests/php/ORM/SQLSelectTest.php @@ -173,6 +173,7 @@ class SQLSelectTest extends SapphireTest $this->assertSQLEquals("SELECT * FROM MyTable LIMIT 99 OFFSET 97", $query->sql($parameters)); } + public function testSelectWithOrderbyClause() { $query = new SQLSelect(); @@ -255,6 +256,18 @@ class SQLSelectTest extends SapphireTest ); } + public function testZeroLimit() + { + $query = new SQLSelect(); + $query->setFrom("MyTable"); + $query->setLimit(0); + + $this->assertSQLEquals( + 'SELECT * FROM MyTable LIMIT 0', + $query->sql($parameters) + ); + } + public function testNegativeLimit() { $this->expectException(\InvalidArgumentException::class);