From 54d8b8677cc1d66d05767f840c4b752345b4c50c Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Mon, 22 Nov 2021 16:54:51 +0000 Subject: [PATCH] Improve API4 count methods. --- Civi/Api4/Generic/DAOGetAction.php | 15 ++- Civi/Api4/Generic/Result.php | 60 ++++++++- .../Generic/Traits/ArrayQueryActionTrait.php | 17 ++- tests/phpunit/api/v4/Action/ResultTest.php | 116 ++++++++++++++++++ 4 files changed, 202 insertions(+), 6 deletions(-) diff --git a/Civi/Api4/Generic/DAOGetAction.php b/Civi/Api4/Generic/DAOGetAction.php index 94f1402f52..0c33d2acc0 100644 --- a/Civi/Api4/Generic/DAOGetAction.php +++ b/Civi/Api4/Generic/DAOGetAction.php @@ -109,19 +109,28 @@ class DAOGetAction extends AbstractGetAction { $onlyCount = $this->getSelect() === ['row_count']; if (!$onlyCount) { + // Typical case: fetch various fields. $query = new Api4SelectQuery($this); $rows = $query->run(); \CRM_Utils_API_HTMLInputCoder::singleton()->decodeRows($rows); $result->exchangeArray($rows); + // No need to fetch count if we got a result set below the limit if (!$this->getLimit() || count($rows) < $this->getLimit()) { - $result->rowCount = count($rows) + $this->getOffset(); - $getCount = FALSE; + if ($getCount) { + $result->setCountMatched(count($rows) + $this->getOffset()); + $getCount = FALSE; + } + else { + // Set rowCount for backward compatibility. + $result->rowCount = count($rows) + $this->getOffset(); + } } } + if ($getCount) { $query = new Api4SelectQuery($this); - $result->rowCount = $query->getCount(); + $result->setCountMatched($query->getCount()); } } diff --git a/Civi/Api4/Generic/Result.php b/Civi/Api4/Generic/Result.php index 77720c9685..3689bee311 100644 --- a/Civi/Api4/Generic/Result.php +++ b/Civi/Api4/Generic/Result.php @@ -41,10 +41,21 @@ class Result extends \ArrayObject implements \JsonSerializable { */ public $version = 4; /** + * Not for public use. Instead, please use countFetched(), countMatched() and count(). + * * @var int */ public $rowCount; + /** + * How many entities matched the query, regardless of LIMIT clauses. + * + * This requires that row_count is included in the SELECT. + * + * @var int + */ + protected $matchedCount; + private $indexedBy; /** @@ -132,7 +143,17 @@ class Result extends \ArrayObject implements \JsonSerializable { } /** - * Returns the number of results + * Returns the number of results. + * + * If row_count was included in the select fields, then this will be the + * number of matched entities, even if this differs from the number of + * entities fetched. + * + * If row_count was not included, then this returns the number of entities + * fetched, which may or may not be the number of matches. + * + * Your code might be easier to reason about if you use countFetched() or + * countMatched() instead. * * @return int */ @@ -140,6 +161,43 @@ class Result extends \ArrayObject implements \JsonSerializable { return $this->rowCount ?? parent::count(); } + /** + * Returns the number of results fetched. + * + * If a limit was used, this will be a number up to that limit. + * + * In the case that *only* the row_count was fetched, this will be zero, since no *entities* were fetched. + * + * @return int + */ + public function countFetched() :int { + return parent::count(); + } + + /** + * Returns the number of results + * + * @return int + */ + public function countMatched() :int { + if (!isset($this->matchedCount)) { + throw new \API_Exception("countMatched can only be used if there was no limit set or if row_count was included in the select fields."); + } + return $this->matchedCount; + } + + /** + * Provides a way for API implementations to set the *matched* count. + * + * The matched count is the number of matching entities, regardless of any imposed limit clause. + */ + public function setCountMatched(int $c) { + $this->matchedCount = $c; + + // Set rowCount for backward compatibility. + $this->rowCount = $c; + } + /** * Reduce each result to one field * diff --git a/Civi/Api4/Generic/Traits/ArrayQueryActionTrait.php b/Civi/Api4/Generic/Traits/ArrayQueryActionTrait.php index 70a53af8b3..b740f5e72b 100644 --- a/Civi/Api4/Generic/Traits/ArrayQueryActionTrait.php +++ b/Civi/Api4/Generic/Traits/ArrayQueryActionTrait.php @@ -30,8 +30,21 @@ trait ArrayQueryActionTrait { protected function queryArray($values, $result) { $values = $this->filterArray($values); $values = $this->sortArray($values); - // Set total count before applying limit - $result->rowCount = count($values); + + if (in_array('row_count', $this->getSelect())) { + $result->setCountMatched(count($values)); + } + else { + // Set total count before applying limit + // + // This is kept here for backward compatibility, but could be confusing because + // the API behaviour is different with ArrayQueryActionTrait than with DAO + // queries. With DAO queries, the rowCount is only the same as the total + // matched count in specific cases, whereas with the implementation here we are + // setting rowCount explicitly to the matches count, before we apply limit. + $result->rowCount = count($values); + } + $values = $this->limitArray($values); $values = $this->selectArray($values); $result->exchangeArray($values); diff --git a/tests/phpunit/api/v4/Action/ResultTest.php b/tests/phpunit/api/v4/Action/ResultTest.php index c46547b6fb..2a462f9f93 100644 --- a/tests/phpunit/api/v4/Action/ResultTest.php +++ b/tests/phpunit/api/v4/Action/ResultTest.php @@ -67,4 +67,120 @@ class ResultTest extends UnitTestCase { ); } + /** + * There are various ways to get the count of an API result. Some have particular behaviour, documented here. + * + * @dataProvider dataForTestCounts + */ + public function testCounts( + string $comment, + int $matches, + int $limit, + array $selects, + ?int $expectedRowCount, + int $expectedCount, + int $expectedCountFetched, + ?int $expectedCountMatches + ) { + + $expectedExceptionFromCountMatches = $expectedCountMatches === NULL; + + // Create $matches contacts. + $records = []; + for ($i = 0; $i < $matches; $i++) { + $records[] = [ + 'contact_type' => 'Individual', + 'first_name' => "testCounts$i", + 'last_name' => 'testCounts', + ]; + } + \Civi\Api4\Contact::save(FALSE)->setRecords($records)->execute(); + + // Do a fetch. + $result = \Civi\Api4\Contact::get(FALSE) + ->setSelect($selects) + ->addWhere('last_name', '=', 'testCounts') + ->setLimit($limit) + ->execute(); + + // Test direct access to rowCount property (naughty) for backwards compatibility. + $this->assertEquals($expectedRowCount, $result->rowCount, "$comment: Public rowCount failed"); + + // Test quirks of count() which sometimes returns the fetched count and sometimes the matched count. + $this->assertEquals($expectedCount, $result->count(), "$comment: count() method failed"); + + // We always have countFetched() available, + $this->assertEquals($expectedCountFetched, $result->countFetched(), "$comment: countFetched() method failed"); + + // We only have countMatched() available if row_count appears in the select count. + $exceptionThrown = FALSE; + try { + $countMatchResult = $result->countMatched(); + } + catch (\Exception $exceptionThrown) { + if (!$expectedExceptionFromCountMatches) { + // Did not expect this! + throw $exceptionThrown; + } + } + + if ($expectedCountMatches === NULL) { + // We expect this to throw an exception. + if (!$exceptionThrown) { + $this->fail("$comment: expected an exception but did not get one."); + } + } + else { + $this->assertEquals($expectedCountMatches, $countMatchResult, "$comment: countMatched() method failed"); + } + } + + /** + * + */ + public function dataForTestCounts() { + + $withoutRowCount = ['id']; + $withRowCount = ['id', 'row_count']; + $rowCountOnly = ['row_count']; + $expectExceptionFromCountMatches = NULL; + + return [ + ['Limited, with row_count', + 1, 1, $withRowCount, 1, 1, 1, 1, + ], + ['Limited, only row_count', + 1, 1, $rowCountOnly, 1, 1, 0, 1, + ], + ['Unlimited, no row_count', + 1, 0, $withoutRowCount, 1, 1, 1, $expectExceptionFromCountMatches, + ], + ['Unlimited, with row_count', + 1, 0, $withRowCount, 1, 1, 1, 1, + ], + ['Unlimited, only row_count', + 1, 0, $rowCountOnly, 1, 1, 0, 1, + ], + ['Limit effective, and without row_count', + 2, 1, $withoutRowCount, NULL, 1, 1, $expectExceptionFromCountMatches, + ], + ['Limit effective, with row_count', + 2, 1, $withRowCount, 2, 2, 1, 2, + ], + ['Limit effective, only row_count', + 2, 1, $rowCountOnly, 2, 2, 0, 2, + ], + ['Limit ineffective (fewer rows than limit), without row count', + 2, 10, $withoutRowCount, 2, 2, 2, $expectExceptionFromCountMatches, + ], + ['Limit ineffective (fewer rows than limit), with row count', + 2, 10, $withRowCount, 2, 2, 2, 2, + ], + ['Limit ineffective (fewer rows than limit), only row count', + 2, 10, $rowCountOnly, 2, 2, 0, 2, + ], + + ]; + } + } -- 2.25.1