From 30336293c649bc1f6389c2d4740c1cedc929a065 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sat, 25 Feb 2023 17:41:01 +0300 Subject: [PATCH 01/18] pkp/pkp-lib#8700 Updated UserGroupDAO::_getSearchSql() to sort users by ID --- classes/security/UserGroupDAO.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/classes/security/UserGroupDAO.inc.php b/classes/security/UserGroupDAO.inc.php index f86393c8b75..2932dbd7b8c 100644 --- a/classes/security/UserGroupDAO.inc.php +++ b/classes/security/UserGroupDAO.inc.php @@ -910,7 +910,7 @@ function _getSearchSql($searchType, $search, $searchMatch, &$params) { } } - $searchSql .= $this->userDao->getOrderBy(); // FIXME Add "sort field" parameter? + $searchSql .= ' ORDER BY u.user_id'; // FIXME Add "sort field" parameter? return $searchSql; } From e96b2fd2063c99e7fa623d4d712d17db2b618a51 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sat, 25 Feb 2023 17:46:15 +0300 Subject: [PATCH 02/18] pkp/pkp-lib#8700 Updated DAO::countRecords() to optimize the query before enclosing it with a SELECT --- classes/db/DAO.inc.php | 65 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 2 deletions(-) diff --git a/classes/db/DAO.inc.php b/classes/db/DAO.inc.php index 47f68502bd2..5aa48b0bbfe 100644 --- a/classes/db/DAO.inc.php +++ b/classes/db/DAO.inc.php @@ -103,8 +103,15 @@ function retrieveRange($sql, $params = [], $dbResultRange = null, $callHooks = t * @return int */ public function countRecords($sql, $params = []) { - $result = $this->retrieve('SELECT COUNT(*) AS row_count FROM (' . $sql . ') AS count_subquery', $params); - return $result->current()->row_count; + foreach ([$this->_optimizeCountQuery($sql, $params), [$sql, $params]] as [$sql, $params]) { + try { + $result = $this->retrieve("SELECT COUNT(*) AS row_count FROM ({$sql}) AS count_subquery", $params); + return $result->current()->row_count; + } catch (Exception $e) { + error_log($e); + } + } + throw $e; } /** @@ -583,4 +590,58 @@ function formatDateToDB($date, $defaultNumWeeks = null, $acceptPastDate = true) return null; } } + + /** + * Retrieves a SELECT statement without the SELECT and ORDER BY clauses for optimization purposes + * @return array The SQL query at the index 0 and the updated parameters at the index 1 + */ + private static function _optimizeCountQuery(string $s, array $params): array { + $findTopLevelExpression = static function (string $s, string $expression, int $index, ?int &$foundParams = null): int { + static + $beginLevel = '(', + $endLevel = ')', + $delimiters = ["'" => 0, '`' => 0, '"' => 0], + $escape = '\\'; + + if ($index < 0) { + return -1; + } + $levels = 0; + $delimiter = null; + for ($l = strlen($s), $i = $index; $i < $l; ) { + $c = $s[$i]; + if ($c === $beginLevel) { + ++$levels; + } elseif ($c === $endLevel) { + if (!$levels--) { + return -1; + } + } elseif (($newDelimiter = $delimiters[$c] ?? null)) { + if ($delimiter === $newDelimiter) { + $delimiter = null; + } elseif (!$delimiter) { + $delimiter = $c; + } + } else if ($c === $escape && $delimiter) { + $i += 2; + continue; + } elseif ($c === '?') { + ++$foundParams; + } elseif (!$delimiter && !$levels && preg_match("/\G{$expression}/i", $s, $m, 0, $i)) { + return $i; + } + ++$i; + } + return -1; + }; + + $selectParams = 0; + // Abort if there's a UNION clause or if there's no "FROM" + if (~$findTopLevelExpression($s, 'UNION', 0) || !~($from = $findTopLevelExpression($s, '\bFROM\b', 0, $selectParams))) { + return [$s, $params]; + } + // Slice the statement up to the ORDER BY clause + $order = ~($order = $findTopLevelExpression($s, '\bORDER\s+BY\b', $from)) ? $order - $from : null; + return ['SELECT 0 ' . substr($s, $from, $order), array_slice($params, $selectParams)]; + } } From ee9f26c46e681f19e801a26008213ebaf0d07736 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sat, 25 Feb 2023 20:26:48 +0300 Subject: [PATCH 03/18] pkp/pkp-lib#8700 Updated code to drop the params after the ORDER BY --- classes/db/DAO.inc.php | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/classes/db/DAO.inc.php b/classes/db/DAO.inc.php index 5aa48b0bbfe..9982434b649 100644 --- a/classes/db/DAO.inc.php +++ b/classes/db/DAO.inc.php @@ -595,7 +595,7 @@ function formatDateToDB($date, $defaultNumWeeks = null, $acceptPastDate = true) * Retrieves a SELECT statement without the SELECT and ORDER BY clauses for optimization purposes * @return array The SQL query at the index 0 and the updated parameters at the index 1 */ - private static function _optimizeCountQuery(string $s, array $params): array { + private static function _optimizeCountQuery(string $sql, array $params): array { $findTopLevelExpression = static function (string $s, string $expression, int $index, ?int &$foundParams = null): int { static $beginLevel = '(', @@ -606,7 +606,9 @@ private static function _optimizeCountQuery(string $s, array $params): array { if ($index < 0) { return -1; } + // Keeps the current level (0 means we are not inside a sub-query/function/etc) $levels = 0; + // Keeps the current opened delimiter $delimiter = null; for ($l = strlen($s), $i = $index; $i < $l; ) { $c = $s[$i]; @@ -614,10 +616,10 @@ private static function _optimizeCountQuery(string $s, array $params): array { ++$levels; } elseif ($c === $endLevel) { if (!$levels--) { - return -1; + throw new Exception('Unexpected ")" on the SQL statement'); } - } elseif (($newDelimiter = $delimiters[$c] ?? null)) { - if ($delimiter === $newDelimiter) { + } elseif ($delimiters[$c] ?? null) { + if ($delimiter === $c) { $delimiter = null; } elseif (!$delimiter) { $delimiter = $c; @@ -632,16 +634,31 @@ private static function _optimizeCountQuery(string $s, array $params): array { } ++$i; } + if ($levels) { + throw new Exception("Missing {$levels} closing \")\" at the end of the SQL statement"); + } + if ($delimiter) { + throw new Exception("Missing \"{$delimiter}\" at the end of the SQL statement"); + } return -1; }; - $selectParams = 0; - // Abort if there's a UNION clause or if there's no "FROM" - if (~$findTopLevelExpression($s, 'UNION', 0) || !~($from = $findTopLevelExpression($s, '\bFROM\b', 0, $selectParams))) { - return [$s, $params]; + $paramsSkippedFromTop = 0; + // Abort if there's a "UNION" clause or if there's no "FROM" + if (~$findTopLevelExpression($sql, '\bUNION\b', 0) || !~($from = $findTopLevelExpression($sql, '\bFROM\b', 0, $paramsSkippedFromTop))) { + return [$sql, $params]; } - // Slice the statement up to the ORDER BY clause - $order = ~($order = $findTopLevelExpression($s, '\bORDER\s+BY\b', $from)) ? $order - $from : null; - return ['SELECT 0 ' . substr($s, $from, $order), array_slice($params, $selectParams)]; + // Find the "ORDER BY" clause + $orderBy = $findTopLevelExpression($sql, '\bORDER\s+BY\b', $from); + $paramsSkippedFromBottom = 0; + // Look for the end of the query, just to retrieve the skipped params + $findTopLevelExpression($sql, '$', $orderBy, $paramsSkippedFromBottom); + $length = ~$orderBy ? $orderBy - $from : strlen($sql); + return [ + // Slice the statement from the "FROM" clause up to the "ORDER BY" clause + 'SELECT 0 ' . substr($sql, $from, $length), + // Slice the params which were dropped + array_slice($params, $paramsSkippedFromTop, $paramsSkippedFromBottom ? -$paramsSkippedFromBottom : count($params)) + ]; } } From 4742748332c67c64f3dd3040db9ee1be6f1ac40c Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Mon, 27 Feb 2023 16:35:09 +0300 Subject: [PATCH 04/18] pkp/pkp-lib#8700 Fixed check to ensure the user was properly created --- cypress/support/commands.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cypress/support/commands.js b/cypress/support/commands.js index 6a5a766d97e..6ab99279f7b 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -394,8 +394,10 @@ Cypress.Commands.add('createUser', user => { user.roles.forEach(role => { cy.get('form[id=userRoleForm]').contains(role).click(); }); + cy.server(); + cy.route('POST', '**/grid/settings/user/user-grid/update-user-roles*').as('rolesSaved'); cy.get('form[id=userRoleForm] button[id^=submitFormButton]').click(); - cy.get('span[id$="-username"]:contains("' + Cypress.$.escapeSelector(user.username) + '")'); + cy.wait('@rolesSaved').its('status').should('eq', 200); }); Cypress.Commands.add('flushNotifications', function() { From f0a721d658c6a63e534658280a46069d18d07c49 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Tue, 28 Feb 2023 11:03:57 +0300 Subject: [PATCH 05/18] pkp/pkp-lib#8700 Give Cypress a chance to retry --- cypress/support/commands.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cypress/support/commands.js b/cypress/support/commands.js index 6ab99279f7b..cfc33e914a3 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -396,8 +396,12 @@ Cypress.Commands.add('createUser', user => { }); cy.server(); cy.route('POST', '**/grid/settings/user/user-grid/update-user-roles*').as('rolesSaved'); - cy.get('form[id=userRoleForm] button[id^=submitFormButton]').click(); - cy.wait('@rolesSaved').its('status').should('eq', 200); + cy.get('div[id^=component-grid-settings-user-usergrid]').as('userGrid').then(staleUserGrid => { + cy.get('form[id=userRoleForm] button[id^=submitFormButton]').click(); + cy.wait('@rolesSaved').its('status').should('eq', 200); + // Wait for the interface to refresh, the "User Grid" will be recreated and get a new ID. + cy.get('@userGrid').should('not.have.attr', 'id', staleUserGrid.attr('id')); + }); }); Cypress.Commands.add('flushNotifications', function() { From d64de0ecf5e457b5b4fa1763473a595073568493 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Thu, 23 Mar 2023 09:03:51 +0300 Subject: [PATCH 06/18] pkp/pkp-lib#8700 Dropped query optimizer --- classes/db/DAO.inc.php | 82 +-------------------------- classes/security/UserGroupDAO.inc.php | 2 +- 2 files changed, 3 insertions(+), 81 deletions(-) diff --git a/classes/db/DAO.inc.php b/classes/db/DAO.inc.php index 9982434b649..47f68502bd2 100644 --- a/classes/db/DAO.inc.php +++ b/classes/db/DAO.inc.php @@ -103,15 +103,8 @@ function retrieveRange($sql, $params = [], $dbResultRange = null, $callHooks = t * @return int */ public function countRecords($sql, $params = []) { - foreach ([$this->_optimizeCountQuery($sql, $params), [$sql, $params]] as [$sql, $params]) { - try { - $result = $this->retrieve("SELECT COUNT(*) AS row_count FROM ({$sql}) AS count_subquery", $params); - return $result->current()->row_count; - } catch (Exception $e) { - error_log($e); - } - } - throw $e; + $result = $this->retrieve('SELECT COUNT(*) AS row_count FROM (' . $sql . ') AS count_subquery', $params); + return $result->current()->row_count; } /** @@ -590,75 +583,4 @@ function formatDateToDB($date, $defaultNumWeeks = null, $acceptPastDate = true) return null; } } - - /** - * Retrieves a SELECT statement without the SELECT and ORDER BY clauses for optimization purposes - * @return array The SQL query at the index 0 and the updated parameters at the index 1 - */ - private static function _optimizeCountQuery(string $sql, array $params): array { - $findTopLevelExpression = static function (string $s, string $expression, int $index, ?int &$foundParams = null): int { - static - $beginLevel = '(', - $endLevel = ')', - $delimiters = ["'" => 0, '`' => 0, '"' => 0], - $escape = '\\'; - - if ($index < 0) { - return -1; - } - // Keeps the current level (0 means we are not inside a sub-query/function/etc) - $levels = 0; - // Keeps the current opened delimiter - $delimiter = null; - for ($l = strlen($s), $i = $index; $i < $l; ) { - $c = $s[$i]; - if ($c === $beginLevel) { - ++$levels; - } elseif ($c === $endLevel) { - if (!$levels--) { - throw new Exception('Unexpected ")" on the SQL statement'); - } - } elseif ($delimiters[$c] ?? null) { - if ($delimiter === $c) { - $delimiter = null; - } elseif (!$delimiter) { - $delimiter = $c; - } - } else if ($c === $escape && $delimiter) { - $i += 2; - continue; - } elseif ($c === '?') { - ++$foundParams; - } elseif (!$delimiter && !$levels && preg_match("/\G{$expression}/i", $s, $m, 0, $i)) { - return $i; - } - ++$i; - } - if ($levels) { - throw new Exception("Missing {$levels} closing \")\" at the end of the SQL statement"); - } - if ($delimiter) { - throw new Exception("Missing \"{$delimiter}\" at the end of the SQL statement"); - } - return -1; - }; - - $paramsSkippedFromTop = 0; - // Abort if there's a "UNION" clause or if there's no "FROM" - if (~$findTopLevelExpression($sql, '\bUNION\b', 0) || !~($from = $findTopLevelExpression($sql, '\bFROM\b', 0, $paramsSkippedFromTop))) { - return [$sql, $params]; - } - // Find the "ORDER BY" clause - $orderBy = $findTopLevelExpression($sql, '\bORDER\s+BY\b', $from); - $paramsSkippedFromBottom = 0; - // Look for the end of the query, just to retrieve the skipped params - $findTopLevelExpression($sql, '$', $orderBy, $paramsSkippedFromBottom); - $length = ~$orderBy ? $orderBy - $from : strlen($sql); - return [ - // Slice the statement from the "FROM" clause up to the "ORDER BY" clause - 'SELECT 0 ' . substr($sql, $from, $length), - // Slice the params which were dropped - array_slice($params, $paramsSkippedFromTop, $paramsSkippedFromBottom ? -$paramsSkippedFromBottom : count($params)) - ]; - } } diff --git a/classes/security/UserGroupDAO.inc.php b/classes/security/UserGroupDAO.inc.php index 2932dbd7b8c..f86393c8b75 100644 --- a/classes/security/UserGroupDAO.inc.php +++ b/classes/security/UserGroupDAO.inc.php @@ -910,7 +910,7 @@ function _getSearchSql($searchType, $search, $searchMatch, &$params) { } } - $searchSql .= ' ORDER BY u.user_id'; // FIXME Add "sort field" parameter? + $searchSql .= $this->userDao->getOrderBy(); // FIXME Add "sort field" parameter? return $searchSql; } From 269a3c25acc2b080004339d93bebdf2e0a55797c Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Thu, 23 Mar 2023 15:36:17 +0300 Subject: [PATCH 07/18] pkp/pkp-lib#8700 Fixed query --- classes/user/UserDAO.inc.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/classes/user/UserDAO.inc.php b/classes/user/UserDAO.inc.php index e13dee667bf..eee24445743 100644 --- a/classes/user/UserDAO.inc.php +++ b/classes/user/UserDAO.inc.php @@ -137,7 +137,7 @@ function getUserByCredentials($username, $password, $allowDisabled = true) { * @return DAOResultFactory containing matching Users */ function getReviewersForSubmission($contextId, $submissionId, $round) { - return new DAOResultFactory($result, + return new DAOResultFactory( $this->retrieve( 'SELECT u.* , ' . $this->getFetchColumns() . ' @@ -156,8 +156,7 @@ function getReviewersForSubmission($contextId, $submissionId, $round) { ROLE_ID_REVIEWER, (int) $submissionId, (int) $round - ]), - $params + ]) ), $this, '_returnUserFromRowWithData' ); From 802899f6d299209b6ae4cd673cd9b16755fc1adb Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Thu, 23 Mar 2023 15:37:08 +0300 Subject: [PATCH 08/18] pkp/pkp-lib#8700 Updated DAO::countRecords() to accept a Laravel Builder --- classes/db/DAO.inc.php | 13 ++++++++++++- classes/db/DAOResultFactory.inc.php | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/classes/db/DAO.inc.php b/classes/db/DAO.inc.php index 47f68502bd2..faa6a1580ca 100644 --- a/classes/db/DAO.inc.php +++ b/classes/db/DAO.inc.php @@ -29,6 +29,7 @@ define('SORT_DIRECTION_DESC', 0x00002); use Illuminate\Database\Capsule\Manager as Capsule; +use Illuminate\Database\Query\Builder; class DAO { /** @@ -98,11 +99,21 @@ function retrieveRange($sql, $params = [], $dbResultRange = null, $callHooks = t /** * Count the number of records in the supplied SQL statement (with optional bind parameters parameters) - * @param $sql string SQL query to be counted + * @param $sql string|Builder SQL query to be counted * @param $params array Optional SQL query bind parameters * @return int */ public function countRecords($sql, $params = []) { + // In case a Laravel Builder has been received, drop its SELECT and ORDER BY clauses for optimization purposes + if ($sql instanceof Builder) { + // Clone query to avoid side-effects + /** @var Builder */ + $builder = clone $sql; + // Discard the SELECT and ORDER BY clauses + $builder->select(Capsule::raw('0'))->reorder(); + $params = $builder->getBindings(); + $sql = $builder->toSql(); + } $result = $this->retrieve('SELECT COUNT(*) AS row_count FROM (' . $sql . ') AS count_subquery', $params); return $result->current()->row_count; } diff --git a/classes/db/DAOResultFactory.inc.php b/classes/db/DAOResultFactory.inc.php index 15fd9345136..369733f5f1a 100644 --- a/classes/db/DAOResultFactory.inc.php +++ b/classes/db/DAOResultFactory.inc.php @@ -63,7 +63,7 @@ class DAOResultFactory extends ItemIterator { * @param $dao object DAO class for factory * @param $functionName The function to call on $dao to create an object * @param $idFields array an array of primary key field names that uniquely identify a result row in the record set. Should be data object _data array key, not database column name - * @param $sql string Optional SQL query used to generate paged result set. Necessary when total row counts will be needed (e.g. when paging). WARNING: New code should not use this. + * @param $sql string|Builder Optional SQL query used to generate paged result set. Necessary when total row counts will be needed (e.g. when paging). WARNING: New code should not use this. * @param $params array Optional parameters for SQL query used to generate paged result set. Necessary when total row counts will be needed (e.g. when paging). WARNING: New code should not use this. * @param $rangeInfo DBResultRange Optional pagination information. WARNING: New code should not use this. */ From 9ef25155a3169878ca0560024b0df80ee398555a Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Thu, 23 Mar 2023 15:38:07 +0300 Subject: [PATCH 09/18] pkp/pkp-lib#8700 Optimized DAOResultFactory instances which are built from a Laravel Builder --- api/v1/stats/publications/PKPStatsPublicationHandler.inc.php | 2 +- classes/services/PKPSubmissionService.inc.php | 4 ++-- classes/services/PKPUserService.inc.php | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/v1/stats/publications/PKPStatsPublicationHandler.inc.php b/api/v1/stats/publications/PKPStatsPublicationHandler.inc.php index 635f644a9ba..326a33f7a48 100644 --- a/api/v1/stats/publications/PKPStatsPublicationHandler.inc.php +++ b/api/v1/stats/publications/PKPStatsPublicationHandler.inc.php @@ -246,7 +246,7 @@ public function getMany($slimRequest, $response, $args) { $metricsDao = \DAORegistry::getDAO('MetricsDAO'); /** @var MetricsDAO */ return $response->withJson([ 'items' => $items, - 'itemsMax' => $metricsDao->countRecords($statsQO->toSql(), $statsQO->getBindings()), + 'itemsMax' => $metricsDao->countRecords($statsQO), ], 200); } diff --git a/classes/services/PKPSubmissionService.inc.php b/classes/services/PKPSubmissionService.inc.php index ade3ae72a38..b8fb44dad65 100644 --- a/classes/services/PKPSubmissionService.inc.php +++ b/classes/services/PKPSubmissionService.inc.php @@ -102,8 +102,8 @@ public function getMany($args = []) { if (isset($args['offset'])) unset($args['offset']); $submissionListQO = $this->getQueryBuilder($args)->getQuery(); $submissionDao = DAORegistry::getDAO('SubmissionDAO'); /* @var $submissionDao SubmissionDAO */ - $result = $submissionDao->retrieveRange($sql = $submissionListQO->toSql(), $params = $submissionListQO->getBindings(), $range); - $queryResults = new DAOResultFactory($result, $submissionDao, '_fromRow', [], $sql, $params, $range); + $result = $submissionDao->retrieveRange($submissionListQO->toSql(), $submissionListQO->getBindings(), $range); + $queryResults = new DAOResultFactory($result, $submissionDao, '_fromRow', [], $submissionListQO, [], $range); return $queryResults->toIterator(); } diff --git a/classes/services/PKPUserService.inc.php b/classes/services/PKPUserService.inc.php index f34791723ee..42f6376fa67 100644 --- a/classes/services/PKPUserService.inc.php +++ b/classes/services/PKPUserService.inc.php @@ -83,7 +83,7 @@ public function getMany($args = []) { $userListQO = $this->getQueryBuilder($args)->getQuery(); $userDao = DAORegistry::getDAO('UserDAO'); /* @var $userDao UserDAO */ $result = $userDao->retrieveRange($userListQO->toSql(), $userListQO->getBindings(), $range); - $queryResults = new DAOResultFactory($result, $userDao, '_returnUserFromRowWithData', [], $userListQO->toSql(), $userListQO->getBindings()); + $queryResults = new DAOResultFactory($result, $userDao, '_returnUserFromRowWithData', [], $userListQO); return $queryResults->toIterator(); } From 1abb51f9d6856a3947b7205d2fb071753a982857 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Thu, 23 Mar 2023 15:40:27 +0300 Subject: [PATCH 10/18] pkp/pkp-lib#8700 Optimized remaining DAOResultFactory instances --- classes/log/EmailLogDAO.inc.php | 17 +++-- classes/note/NoteDAO.inc.php | 17 ++--- classes/security/UserGroupDAO.inc.php | 71 +++++++++++-------- .../submission/SubmissionCommentDAO.inc.php | 24 +++---- classes/user/UserStageAssignmentDAO.inc.php | 49 +++++++------ 5 files changed, 100 insertions(+), 78 deletions(-) diff --git a/classes/log/EmailLogDAO.inc.php b/classes/log/EmailLogDAO.inc.php index 06a6dc615a6..08fc36d681a 100644 --- a/classes/log/EmailLogDAO.inc.php +++ b/classes/log/EmailLogDAO.inc.php @@ -59,19 +59,22 @@ function _getByEventType($assocType, $assocId, $eventType, $userId = null, $rang ]; if ($userId) $params[] = $userId; - $result = $this->retrieveRange( - $sql = 'SELECT e.* - FROM email_log e' . - ($userId ? ' LEFT JOIN email_log_users u ON e.log_id = u.email_log_id' : '') . - ' WHERE e.assoc_type = ? AND + $baseSql = ' + FROM email_log e' . + ($userId ? ' LEFT JOIN email_log_users u ON e.log_id = u.email_log_id' : '') . ' + WHERE e.assoc_type = ? AND e.assoc_id = ? AND e.event_type = ?' . - ($userId ? ' AND u.user_id = ?' : ''), + ($userId ? ' AND u.user_id = ?' : '') . ' + '; + + $result = $this->retrieveRange( + "SELECT e.* {$baseSql}", $params, $rangeInfo ); - return new DAOResultFactory($result, $this, 'build', [], $sql, $params, $rangeInfo); // Counted in submissionEmails.tpl + return new DAOResultFactory($result, $this, 'build', [], "SELECT 0 {$baseSql}", $params, $rangeInfo); // Counted in submissionEmails.tpl } /** diff --git a/classes/note/NoteDAO.inc.php b/classes/note/NoteDAO.inc.php index b867f0d2654..8788ac57eff 100644 --- a/classes/note/NoteDAO.inc.php +++ b/classes/note/NoteDAO.inc.php @@ -89,18 +89,19 @@ function getByAssoc($assocType, $assocId, $userId = null, $orderBy = NOTE_ORDER_ $directionSanitized = 'DESC'; } - $result = $this->retrieve( - $sql = 'SELECT * - FROM notes - WHERE assoc_id = ? + $baseSql = ' + FROM notes + WHERE assoc_id = ? AND assoc_type = ? ' . ($userId?' AND user_id = ?':'') . - ($isAdmin?'':' - AND (title IS NOT NULL OR contents IS NOT NULL)') . ' - ORDER BY ' . $orderSanitized . ' ' . $directionSanitized, + ($isAdmin?'':' AND (title IS NOT NULL OR contents IS NOT NULL)') . ' + '; + + $result = $this->retrieve( + "SELECT * {$baseSql} ORDER BY {$orderSanitized} {$directionSanitized}", $params ); - return new DAOResultFactory($result, $this, '_fromRow', [], $sql, $params); // Counted in QueriesGridCellProvider + return new DAOResultFactory($result, $this, '_fromRow', [], "SELECT 0 {$baseSql}", $params); // Counted in QueriesGridCellProvider } /** diff --git a/classes/security/UserGroupDAO.inc.php b/classes/security/UserGroupDAO.inc.php index f86393c8b75..d155e51ebdf 100644 --- a/classes/security/UserGroupDAO.inc.php +++ b/classes/security/UserGroupDAO.inc.php @@ -250,18 +250,21 @@ function isDefault($userGroupId) { function getByRoleId($contextId, $roleId, $default = false, $dbResultRange = null) { $params = [(int) $contextId, (int) $roleId]; if ($default) $params[] = 1; // true - $result = $this->retrieveRange( - $sql = 'SELECT * - FROM user_groups - WHERE context_id = ? AND + + $baseSql = ' + FROM user_groups + WHERE context_id = ? AND role_id = ? - ' . ($default?' AND is_default = ?':'') - . ' ORDER BY user_group_id', + ' . ($default?' AND is_default = ?':'') . ' + '; + + $result = $this->retrieveRange( + "SELECT * {$baseSql} ORDER BY user_group_id", $params, $dbResultRange ); - return new DAOResultFactory($result, $this, '_returnFromRow', [], $sql, $params, $dbResultRange); + return new DAOResultFactory($result, $this, '_returnFromRow', [], "SELECT 0 {$baseSql}", $params, $dbResultRange); } /** @@ -381,15 +384,18 @@ function getByContextId($contextId = null, $dbResultRange = null) { $params = []; if ($contextId) $params[] = (int) $contextId; + $baseSql = ' + FROM user_groups ug' . + ($contextId?' WHERE ug.context_id = ?':'') . ' + '; + $result = $this->retrieveRange( - $sql = 'SELECT ug.* - FROM user_groups ug' . - ($contextId?' WHERE ug.context_id = ?':''), + "SELECT ug.* {$baseSql}", $params, $dbResultRange ); - return new DAOResultFactory($result, $this, '_returnFromRow', [], $sql, $params, $dbResultRange); + return new DAOResultFactory($result, $this, '_returnFromRow', [], "SELECT 0 {$baseSql}", $params, $dbResultRange); } /** @@ -509,14 +515,15 @@ function getUsersById($userGroupId = null, $contextId = null, $searchType = null COALESCE(us.setting_value, '') = '', us.locale <> ? LIMIT 1 )"; - $params = [ + $selectParams = [ IDENTITY_SETTING_GIVENNAME, $locale, $primaryLocale, $locale, IDENTITY_SETTING_FAMILYNAME, $locale, $primaryLocale, $locale ]; - $sql = "SELECT u.*, $settingValue AS user_given, $settingValue AS user_family + $baseSql = ' FROM users AS u - WHERE 1 = 1"; + WHERE 1 = 1 + '; // Has user group if ($contextId || $userGroupId) { @@ -526,7 +533,7 @@ function getUsersById($userGroupId = null, $contextId = null, $searchType = null if ($userGroupId) { $params[] = (int) $userGroupId; } - $sql .= ' AND EXISTS ( + $baseSql .= ' AND EXISTS ( SELECT 0 FROM user_user_groups uug INNER JOIN user_groups ug @@ -537,12 +544,16 @@ function getUsersById($userGroupId = null, $contextId = null, $searchType = null ' . ($userGroupId ? 'AND ug.user_group_id = ?' : '') . ' )'; } - $sql .= ' ' . $this->_getSearchSql($searchType, $search, $searchMatch, $params); + $baseSql .= ' ' . $this->_getSearchSql($searchType, $search, $searchMatch, $params, ''); // Get the result set - $result = $this->retrieveRange($sql, $params, $dbResultRange); + $result = $this->retrieveRange( + "SELECT u.*, $settingValue AS user_given, $settingValue AS user_family {$baseSql} " . $this->userDao->getOrderBy(), + array_merge($selectParams, $params), + $dbResultRange + ); - return new DAOResultFactory($result, $this->userDao, '_returnUserFromRowWithData', [], $sql, $params, $dbResultRange); + return new DAOResultFactory($result, $this->userDao, '_returnUserFromRowWithData', [], "SELECT 0 {$baseSql}", $params, $dbResultRange); } // @@ -819,9 +830,10 @@ function deleteSettingsByLocale($locale) { * @param string $search the keywords to search for. * @param string $searchMatch where to match (is, contains, startsWith). * @param array $params SQL parameter array reference + * @param ?string $querySuffix When null adds an ORDER BY clause using the UserDAO::getOrderBy() * @return string SQL search snippet */ - function _getSearchSql($searchType, $search, $searchMatch, &$params) { + function _getSearchSql($searchType, $search, $searchMatch, &$params, $querySuffix = null) { $hasUserSetting = "EXISTS( SELECT 0 FROM user_settings @@ -910,7 +922,7 @@ function _getSearchSql($searchType, $search, $searchMatch, &$params) { } } - $searchSql .= $this->userDao->getOrderBy(); // FIXME Add "sort field" parameter? + $searchSql .= $querySuffix ?? $this->userDao->getOrderBy(); // FIXME Add "sort field" parameter? return $searchSql; } @@ -930,22 +942,25 @@ function _getSearchSql($searchType, $search, $searchMatch, &$params) { function getUserGroupsByStage($contextId, $stageId, $roleId = null, $dbResultRange = null) { $params = [(int) $contextId, (int) $stageId]; if ($roleId) $params[] = (int) $roleId; + + $baseSql = ' + FROM user_groups ug + JOIN user_group_stage ugs ON (ug.user_group_id = ugs.user_group_id AND ug.context_id = ugs.context_id) + WHERE ugs.context_id = ? AND + ugs.stage_id = ? + ' . ($roleId?'AND ug.role_id = ?':'') . ' + '; + return new DAOResultFactory( $this->retrieveRange( - $sql = 'SELECT ug.* - FROM user_groups ug - JOIN user_group_stage ugs ON (ug.user_group_id = ugs.user_group_id AND ug.context_id = ugs.context_id) - WHERE ugs.context_id = ? AND - ugs.stage_id = ? - ' . ($roleId?'AND ug.role_id = ?':'') . ' - ORDER BY ug.role_id ASC', + "SELECT ug.* {$baseSql} ORDER BY ug.role_id ASC", $params, $dbResultRange ), $this, '_returnFromRow', [], - $sql, $params, $dbResultRange + "SELECT 0 {$baseSql}", $params, $dbResultRange ); } diff --git a/classes/submission/SubmissionCommentDAO.inc.php b/classes/submission/SubmissionCommentDAO.inc.php index 307be399821..fd150913191 100644 --- a/classes/submission/SubmissionCommentDAO.inc.php +++ b/classes/submission/SubmissionCommentDAO.inc.php @@ -72,19 +72,19 @@ function getReviewerCommentsByReviewerId($submissionId, $reviewerId = null, $rev $params = array((int) $submissionId); if ($reviewerId) $params[] = (int) $reviewerId; if ($reviewId) $params[] = (int) $reviewId; + + $baseSql = ' + FROM submission_comments a + WHERE submission_id = ? + ' . ($reviewerId?' AND author_id = ?':'') . ' + ' . ($reviewId?' AND assoc_id = ?':'') . ' + ' . ($viewable === true?' AND viewable = 1':'') . ' + ' . ($viewable === false?' AND viewable = 0':'') . ' + '; + return new DAOResultFactory( - $this->retrieve( - $sql = 'SELECT a.* - FROM submission_comments a - WHERE submission_id = ? - ' . ($reviewerId?' AND author_id = ?':'') . ' - ' . ($reviewId?' AND assoc_id = ?':'') . ' - ' . ($viewable === true?' AND viewable = 1':'') . ' - ' . ($viewable === false?' AND viewable = 0':'') . ' - ORDER BY date_posted DESC', - $params - ), - $this, '_fromRow', [], $sql, $params // Counted in readReview.tpl and authorReadReview.tpl + $this->retrieve("SELECT a.* {$baseSql} ORDER BY date_posted DESC", $params), + $this, '_fromRow', [], "SELECT 0 {$baseSql}", $params // Counted in readReview.tpl and authorReadReview.tpl ); } diff --git a/classes/user/UserStageAssignmentDAO.inc.php b/classes/user/UserStageAssignmentDAO.inc.php index bd593340a4b..9e79fd781b5 100644 --- a/classes/user/UserStageAssignmentDAO.inc.php +++ b/classes/user/UserStageAssignmentDAO.inc.php @@ -106,31 +106,34 @@ function filterUsersNotAssignedToStageInUserGroup($submissionId, $stageId, $user if ($name !== null) { $params = array_merge($params, array_fill(0, 6, (string) $name)); } - $result = $this->retrieveRange( - $sql = 'SELECT u.* - FROM users u - LEFT JOIN user_user_groups uug ON (u.user_id = uug.user_id) - LEFT JOIN stage_assignments s ON (s.user_id = uug.user_id AND s.user_group_id = uug.user_group_id AND s.submission_id = ?) - JOIN user_group_stage ugs ON (uug.user_group_id = ugs.user_group_id AND ugs.stage_id = ?) - LEFT JOIN user_settings usgs_pl ON (usgs_pl.user_id = u.user_id AND usgs_pl.setting_name = ? AND usgs_pl.locale = ?) - LEFT JOIN user_settings usfs_pl ON (usfs_pl.user_id = u.user_id AND usfs_pl.setting_name = ? AND usfs_pl.locale = ?) - LEFT JOIN user_settings usgs_l ON (usgs_l.user_id = u.user_id AND usgs_l.setting_name = ? AND usgs_l.locale = ?) - LEFT JOIN user_settings usfs_l ON (usfs_l.user_id = u.user_id AND usfs_l.setting_name = ? AND usfs_l.locale = ?) - WHERE uug.user_group_id = ? AND + $baseSql = ' + FROM users u + LEFT JOIN user_user_groups uug ON (u.user_id = uug.user_id) + LEFT JOIN stage_assignments s ON (s.user_id = uug.user_id AND s.user_group_id = uug.user_group_id AND s.submission_id = ?) + JOIN user_group_stage ugs ON (uug.user_group_id = ugs.user_group_id AND ugs.stage_id = ?) + LEFT JOIN user_settings usgs_pl ON (usgs_pl.user_id = u.user_id AND usgs_pl.setting_name = ? AND usgs_pl.locale = ?) + LEFT JOIN user_settings usfs_pl ON (usfs_pl.user_id = u.user_id AND usfs_pl.setting_name = ? AND usfs_pl.locale = ?) + LEFT JOIN user_settings usgs_l ON (usgs_l.user_id = u.user_id AND usgs_l.setting_name = ? AND usgs_l.locale = ?) + LEFT JOIN user_settings usfs_l ON (usfs_l.user_id = u.user_id AND usfs_l.setting_name = ? AND usfs_l.locale = ?) + WHERE uug.user_group_id = ? AND s.user_group_id IS NULL' - . ($name !== null - ? " AND (LOWER(usgs_pl.setting_value) LIKE CONCAT('%', LOWER(?), '%') - OR LOWER(usgs_l.setting_value) LIKE CONCAT('%', LOWER(?), '%') - OR LOWER(usfs_pl.setting_value) LIKE CONCAT('%', LOWER(?), '%') - OR LOWER(usfs_l.setting_value) LIKE CONCAT('%', LOWER(?), '%') - OR LOWER(u.username) LIKE CONCAT('%', LOWER(?), '%') - OR LOWER(u.email) LIKE CONCAT('%', LOWER(?), '%'))" - : "") - . " ORDER BY COALESCE(usfs_l.setting_value, usfs_pl.setting_value)", - $params, - $rangeInfo); - return new DAOResultFactory($result, $this, '_returnUserFromRowWithData', [], $sql, $params, $rangeInfo); + . ($name !== null + ? " AND (LOWER(usgs_pl.setting_value) LIKE CONCAT('%', LOWER(?), '%') + OR LOWER(usgs_l.setting_value) LIKE CONCAT('%', LOWER(?), '%') + OR LOWER(usfs_pl.setting_value) LIKE CONCAT('%', LOWER(?), '%') + OR LOWER(usfs_l.setting_value) LIKE CONCAT('%', LOWER(?), '%') + OR LOWER(u.username) LIKE CONCAT('%', LOWER(?), '%') + OR LOWER(u.email) LIKE CONCAT('%', LOWER(?), '%'))" + : "") . ' + '; + + $result = $this->retrieveRange( + "SELECT u.* {$baseSql} ORDER BY COALESCE(usfs_l.setting_value, usfs_pl.setting_value)", + $params, + $rangeInfo + ); + return new DAOResultFactory($result, $this, '_returnUserFromRowWithData', [], "SELECT 0 {$baseSql}", $params, $rangeInfo); } } From ace373aefdd8ec5ab26d4d03d52c1ebc1c99eddb Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Thu, 23 Mar 2023 17:24:24 +0300 Subject: [PATCH 11/18] pkp/pkp-lib#8700 Added Builder macro to count records in a safe and performant way --- classes/core/PKPApplication.inc.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/classes/core/PKPApplication.inc.php b/classes/core/PKPApplication.inc.php index d43cd4a7905..a336ff79191 100644 --- a/classes/core/PKPApplication.inc.php +++ b/classes/core/PKPApplication.inc.php @@ -69,6 +69,7 @@ define('WORKFLOW_TYPE_AUTHOR', 'author'); use Illuminate\Database\Capsule\Manager as Capsule; +use Illuminate\Database\Query\Builder; interface iPKPApplicationInfoProvider { /** @@ -221,6 +222,21 @@ public function initializeDatabaseConnection() { error_log("Database query\n$query->sql\n" . json_encode($query->bindings));//\n Bindings: " . print_r($query->bindings, true)); }); + Builder::macro('safeCount', function (): int { + // Discard the ORDER BY and enclose the query in a sub-query to avoid miscounting rows in the presence of a GROUP BY + $run = function (Builder $query): int { + return Capsule::table(Capsule::raw("({$query->reorder()->toSql()}) AS query"))->mergeBindings($query)->count(); + }; + try { + /** @var Builder $query */ + $query = clone $this; + // Discard the SELECT if the query doesn't have a UNION + return $run(empty($query->unions) ? $query->select(Capsule::raw('0')) : $query); + } catch (Exception $e) { + // Retry using a fail-proof query, as dropping the SELECT might fail in the presence of a GROUP BY with an alias + return $run(clone $this); + } + }); // Set up Laravel queue handling $laravelContainer->bind('exception.handler', function () { From fde386f82b63050b1bded2d06f8dc9a8a1a8253a Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Thu, 23 Mar 2023 17:25:14 +0300 Subject: [PATCH 12/18] pkp/pkp-lib#8700 Updated code to make use of the new safeCount() macro --- api/v1/_email/PKPEmailHandler.inc.php | 4 ++-- classes/db/DAOResultFactory.inc.php | 3 ++- .../upgrade/PKPv3_3_0UpgradeMigration.inc.php | 2 +- classes/services/PKPUserService.inc.php | 5 +++-- .../PKPAnnouncementQueryBuilder.inc.php | 4 +--- .../queryBuilders/PKPAuthorQueryBuilder.inc.php | 4 +--- .../queryBuilders/PKPContextQueryBuilder.inc.php | 4 +--- .../PKPEmailTemplateQueryBuilder.inc.php | 2 +- .../queryBuilders/PKPPublicationQueryBuilder.inc.php | 6 ++---- .../PKPStatsEditorialQueryBuilder.inc.php | 12 ++++++------ .../PKPSubmissionFileQueryBuilder.inc.php | 4 +--- .../queryBuilders/PKPSubmissionQueryBuilder.inc.php | 4 +--- .../queryBuilders/PKPUserQueryBuilder.inc.php | 9 +++------ 13 files changed, 25 insertions(+), 38 deletions(-) diff --git a/api/v1/_email/PKPEmailHandler.inc.php b/api/v1/_email/PKPEmailHandler.inc.php index 11353154691..27d1dc70e4e 100644 --- a/api/v1/_email/PKPEmailHandler.inc.php +++ b/api/v1/_email/PKPEmailHandler.inc.php @@ -192,7 +192,7 @@ public function process(ServerRequestInterface $slimRequest, APIResponse $respon $countRunning = Capsule::table('jobs') ->where('queue', $args['queueId']) ->whereNotNull('reserved_at') - ->count(); + ->safeCount(); $countPending = $this->countPending($args['queueId']); // Don't run another job if one is already running. @@ -230,6 +230,6 @@ function() { protected function countPending(string $queueId) : int { return Capsule::table('jobs') ->where('queue', $queueId) - ->count(); + ->safeCount(); } } diff --git a/classes/db/DAOResultFactory.inc.php b/classes/db/DAOResultFactory.inc.php index 369733f5f1a..0482011abda 100644 --- a/classes/db/DAOResultFactory.inc.php +++ b/classes/db/DAOResultFactory.inc.php @@ -18,6 +18,7 @@ import('lib.pkp.classes.core.ItemIterator'); import('lib.pkp.classes.db.DAOResultIterator'); +use Illuminate\Database\Query\Builder; use Illuminate\Support\Enumerable; class DAOResultFactory extends ItemIterator { @@ -37,7 +38,7 @@ class DAOResultFactory extends ItemIterator { var $records; /** - * @var string|null Fetch SQL + * @var string|Builder|null Fetch SQL */ var $sql; diff --git a/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php b/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php index c42c5de4e18..869e889a71b 100755 --- a/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php +++ b/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php @@ -491,7 +491,7 @@ function (Builder $q) use ($row, $lastInsertedFileId) { // be unique. $count = Capsule::table('review_round_files') ->where('file_id', '=', $row->file_id) - ->count(); + ->safeCount(); if ($count > 1) { Capsule::table('review_round_files') ->where('file_id', '=', $row->file_id) diff --git a/classes/services/PKPUserService.inc.php b/classes/services/PKPUserService.inc.php index 42f6376fa67..16e9e38db16 100644 --- a/classes/services/PKPUserService.inc.php +++ b/classes/services/PKPUserService.inc.php @@ -592,8 +592,9 @@ public function getAccessibleWorkflowStages($userId, $contextId, &$submission, $ * @param array $args See self::getMany() */ public function count($args = []) { - $qb = $this->getQueryBuilder($args); - return $qb->getQuery()->get()->count(); + return $this->getQueryBuilder($args) + ->getQuery() + ->safeCount(); } /** diff --git a/classes/services/queryBuilders/PKPAnnouncementQueryBuilder.inc.php b/classes/services/queryBuilders/PKPAnnouncementQueryBuilder.inc.php index c3c98e492f6..5b83c87d7c8 100644 --- a/classes/services/queryBuilders/PKPAnnouncementQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPAnnouncementQueryBuilder.inc.php @@ -70,9 +70,7 @@ public function searchPhrase($phrase) { public function getCount() { return $this ->getQuery() - ->select('a.announcement_id') - ->get() - ->count(); + ->safeCount(); } /** diff --git a/classes/services/queryBuilders/PKPAuthorQueryBuilder.inc.php b/classes/services/queryBuilders/PKPAuthorQueryBuilder.inc.php index c7622ca2ace..2b5a486126a 100644 --- a/classes/services/queryBuilders/PKPAuthorQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPAuthorQueryBuilder.inc.php @@ -102,9 +102,7 @@ public function filterByPublicationIds($publicationIds) { public function getCount() { return $this ->getQuery() - ->select('a.author_id') - ->get() - ->count(); + ->safeCount(); } /** diff --git a/classes/services/queryBuilders/PKPContextQueryBuilder.inc.php b/classes/services/queryBuilders/PKPContextQueryBuilder.inc.php index acc9643e235..0e7a0c8d4c5 100644 --- a/classes/services/queryBuilders/PKPContextQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPContextQueryBuilder.inc.php @@ -85,9 +85,7 @@ public function searchPhrase($phrase) { public function getCount() { return $this ->getQuery() - ->select('c.' . $this->dbIdColumn) - ->get() - ->count(); + ->safeCount(); } /** diff --git a/classes/services/queryBuilders/PKPEmailTemplateQueryBuilder.inc.php b/classes/services/queryBuilders/PKPEmailTemplateQueryBuilder.inc.php index ddfcfc684cd..035c2f41b7a 100644 --- a/classes/services/queryBuilders/PKPEmailTemplateQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPEmailTemplateQueryBuilder.inc.php @@ -179,7 +179,7 @@ public function getCount() { $compiledQuery = $this->getCompiledQuery(); return Capsule::table(Capsule::raw('(' . $compiledQuery[0] . ') as email_template_count')) ->setBindings($compiledQuery[1]) - ->count(); + ->safeCount(); } /** diff --git a/classes/services/queryBuilders/PKPPublicationQueryBuilder.inc.php b/classes/services/queryBuilders/PKPPublicationQueryBuilder.inc.php index 3495be43a50..a5d696a8f21 100644 --- a/classes/services/queryBuilders/PKPPublicationQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPPublicationQueryBuilder.inc.php @@ -85,9 +85,7 @@ public function offsetBy($offset) { public function getCount() { return $this ->getQuery() - ->select('p.publication_id') - ->get() - ->count(); + ->safeCount(); } /** @@ -188,6 +186,6 @@ public function isDuplicateUrlPath($urlPath, $submissionId, $contextId) { ->where('url_path', '=' , $urlPath) ->where('p.submission_id', '!=', $submissionId) ->where('s.context_id', '=', $contextId) - ->count(); + ->safeCount(); } } diff --git a/classes/services/queryBuilders/PKPStatsEditorialQueryBuilder.inc.php b/classes/services/queryBuilders/PKPStatsEditorialQueryBuilder.inc.php index 283dc150909..0aab1c5342b 100644 --- a/classes/services/queryBuilders/PKPStatsEditorialQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPStatsEditorialQueryBuilder.inc.php @@ -95,7 +95,7 @@ public function countSubmissionsReceived() { $q->where('s.date_submitted', '<=', $this->dateEnd); } - return $q->count(); + return $q->safeCount(); } /** @@ -165,7 +165,7 @@ public function countByDecisions($decisions, $forSubmittedDate = false) { public function countByStatus($status) { return $this->_getObject() ->whereIn('s.status', (array) $status) - ->count(); + ->safeCount(); } /** @@ -180,7 +180,7 @@ public function countActiveByStages($stages) { return $this->_getObject() ->where('s.status', '=', STATUS_QUEUED) ->whereIn('s.stage_id', $stages) - ->count(); + ->safeCount(); } /** @@ -214,7 +214,7 @@ public function countPublished() { } } - return $q->count(); + return $q->safeCount(); } /** @@ -433,7 +433,7 @@ public function countImported() { ->when($this->dateEnd, function (Builder $q) { $q->where('s.date_submitted', '<=', $this->dateEnd); }) - ->count(); + ->safeCount(); } /** @@ -451,7 +451,7 @@ public function countInProgress() { ->when($this->dateEnd, function (Builder $q) { $q->where('s.date_submitted', '<=', $this->dateEnd); }) - ->count(); + ->safeCount(); } /** diff --git a/classes/services/queryBuilders/PKPSubmissionFileQueryBuilder.inc.php b/classes/services/queryBuilders/PKPSubmissionFileQueryBuilder.inc.php index be70fba8ffa..76ef719c315 100644 --- a/classes/services/queryBuilders/PKPSubmissionFileQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPSubmissionFileQueryBuilder.inc.php @@ -159,9 +159,7 @@ public function includeDependentFiles($includeDependentFiles) { public function getCount() { return $this ->getQuery() - ->select('sf.submission_file_id') - ->get() - ->count(); + ->safeCount(); } /** diff --git a/classes/services/queryBuilders/PKPSubmissionQueryBuilder.inc.php b/classes/services/queryBuilders/PKPSubmissionQueryBuilder.inc.php index a528edc8b57..30bda53ac28 100644 --- a/classes/services/queryBuilders/PKPSubmissionQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPSubmissionQueryBuilder.inc.php @@ -239,9 +239,7 @@ public function offsetBy($offset) { public function getCount() { return $this ->getQuery() - ->select('s.submission_id') - ->get() - ->count(); + ->safeCount(); } /** diff --git a/classes/services/queryBuilders/PKPUserQueryBuilder.inc.php b/classes/services/queryBuilders/PKPUserQueryBuilder.inc.php index a6c5e9588a8..22d2af34710 100644 --- a/classes/services/queryBuilders/PKPUserQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPUserQueryBuilder.inc.php @@ -424,12 +424,9 @@ public function offsetBy($offset) { * @copydoc PKP\Services\QueryBuilders\Interfaces\EntityQueryBuilderInterface::getCount() */ public function getCount() { - $q = $this->getQuery(); - // Reset the orderBy - $q->orders = []; - return $q->select('u.user_id') - ->get() - ->count(); + return $this + ->getQuery() + ->safeCount(); } /** From 9d9fad40ff8be9ec1bc5ad5e21b9fd0ec4021206 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sun, 26 Mar 2023 20:51:36 +0300 Subject: [PATCH 13/18] pkp/pkp-lib#8700 Fixed flaky test --- cypress/support/commands.js | 1 + 1 file changed, 1 insertion(+) diff --git a/cypress/support/commands.js b/cypress/support/commands.js index cfc33e914a3..f502784818c 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -315,6 +315,7 @@ Cypress.Commands.add('assignParticipant', (role, name, recommendOnly) => { cy.get('select[name=filterUserGroupId').select(role); cy.get('input[id^="namegrid-users-userselect-userselectgrid-"]').type(names[1], {delay: 0}); cy.get('form[id="searchUserFilter-grid-users-userselect-userselectgrid"]').find('button[id^="submitFormButton-"]').click(); + cy.waitJQuery(); cy.get('input[name="userId"]').click(); // Assume only one user results from the search. if (recommendOnly) cy.get('input[name="recommendOnly"]').click(); cy.flushNotifications(); From fe5ee56526a13eb7e7fc9ec1c6bf0e5efd772912 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sun, 26 Mar 2023 21:23:23 +0300 Subject: [PATCH 14/18] pkp/pkp-lib#8700 Updated countRecords() to use the Builder::safeCount() --- classes/db/DAO.inc.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/classes/db/DAO.inc.php b/classes/db/DAO.inc.php index faa6a1580ca..489249586b6 100644 --- a/classes/db/DAO.inc.php +++ b/classes/db/DAO.inc.php @@ -106,13 +106,7 @@ function retrieveRange($sql, $params = [], $dbResultRange = null, $callHooks = t public function countRecords($sql, $params = []) { // In case a Laravel Builder has been received, drop its SELECT and ORDER BY clauses for optimization purposes if ($sql instanceof Builder) { - // Clone query to avoid side-effects - /** @var Builder */ - $builder = clone $sql; - // Discard the SELECT and ORDER BY clauses - $builder->select(Capsule::raw('0'))->reorder(); - $params = $builder->getBindings(); - $sql = $builder->toSql(); + return $sql->safeCount(); } $result = $this->retrieve('SELECT COUNT(*) AS row_count FROM (' . $sql . ') AS count_subquery', $params); return $result->current()->row_count; From 30bc59a9cd9e5558a4d9ce471b20ffd7fbe16a63 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Thu, 23 May 2024 17:01:20 +0300 Subject: [PATCH 15/18] pkp/pkp-lib#8700 Dropped safeCount() workaround in favor of getCountForPagination() --- api/v1/_email/PKPEmailHandler.inc.php | 4 ++-- classes/core/PKPApplication.inc.php | 16 ---------------- classes/db/DAO.inc.php | 2 +- .../upgrade/PKPv3_3_0UpgradeMigration.inc.php | 2 +- classes/services/PKPUserService.inc.php | 2 +- .../PKPAnnouncementQueryBuilder.inc.php | 2 +- .../queryBuilders/PKPAuthorQueryBuilder.inc.php | 2 +- .../queryBuilders/PKPContextQueryBuilder.inc.php | 2 +- .../PKPEmailTemplateQueryBuilder.inc.php | 2 +- .../PKPPublicationQueryBuilder.inc.php | 4 ++-- .../PKPStatsEditorialQueryBuilder.inc.php | 12 ++++++------ .../PKPSubmissionFileQueryBuilder.inc.php | 2 +- .../PKPSubmissionQueryBuilder.inc.php | 2 +- .../queryBuilders/PKPUserQueryBuilder.inc.php | 2 +- 14 files changed, 20 insertions(+), 36 deletions(-) diff --git a/api/v1/_email/PKPEmailHandler.inc.php b/api/v1/_email/PKPEmailHandler.inc.php index 27d1dc70e4e..44e21b6abf9 100644 --- a/api/v1/_email/PKPEmailHandler.inc.php +++ b/api/v1/_email/PKPEmailHandler.inc.php @@ -192,7 +192,7 @@ public function process(ServerRequestInterface $slimRequest, APIResponse $respon $countRunning = Capsule::table('jobs') ->where('queue', $args['queueId']) ->whereNotNull('reserved_at') - ->safeCount(); + ->getCountForPagination(); $countPending = $this->countPending($args['queueId']); // Don't run another job if one is already running. @@ -230,6 +230,6 @@ function() { protected function countPending(string $queueId) : int { return Capsule::table('jobs') ->where('queue', $queueId) - ->safeCount(); + ->getCountForPagination(); } } diff --git a/classes/core/PKPApplication.inc.php b/classes/core/PKPApplication.inc.php index a336ff79191..e22ec92c9fb 100644 --- a/classes/core/PKPApplication.inc.php +++ b/classes/core/PKPApplication.inc.php @@ -222,22 +222,6 @@ public function initializeDatabaseConnection() { error_log("Database query\n$query->sql\n" . json_encode($query->bindings));//\n Bindings: " . print_r($query->bindings, true)); }); - Builder::macro('safeCount', function (): int { - // Discard the ORDER BY and enclose the query in a sub-query to avoid miscounting rows in the presence of a GROUP BY - $run = function (Builder $query): int { - return Capsule::table(Capsule::raw("({$query->reorder()->toSql()}) AS query"))->mergeBindings($query)->count(); - }; - try { - /** @var Builder $query */ - $query = clone $this; - // Discard the SELECT if the query doesn't have a UNION - return $run(empty($query->unions) ? $query->select(Capsule::raw('0')) : $query); - } catch (Exception $e) { - // Retry using a fail-proof query, as dropping the SELECT might fail in the presence of a GROUP BY with an alias - return $run(clone $this); - } - }); - // Set up Laravel queue handling $laravelContainer->bind('exception.handler', function () { return new class implements Illuminate\Contracts\Debug\ExceptionHandler { diff --git a/classes/db/DAO.inc.php b/classes/db/DAO.inc.php index 489249586b6..25233a5d1aa 100644 --- a/classes/db/DAO.inc.php +++ b/classes/db/DAO.inc.php @@ -106,7 +106,7 @@ function retrieveRange($sql, $params = [], $dbResultRange = null, $callHooks = t public function countRecords($sql, $params = []) { // In case a Laravel Builder has been received, drop its SELECT and ORDER BY clauses for optimization purposes if ($sql instanceof Builder) { - return $sql->safeCount(); + return $sql->getCountForPagination(); } $result = $this->retrieve('SELECT COUNT(*) AS row_count FROM (' . $sql . ') AS count_subquery', $params); return $result->current()->row_count; diff --git a/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php b/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php index 869e889a71b..0e30761791a 100755 --- a/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php +++ b/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php @@ -491,7 +491,7 @@ function (Builder $q) use ($row, $lastInsertedFileId) { // be unique. $count = Capsule::table('review_round_files') ->where('file_id', '=', $row->file_id) - ->safeCount(); + ->getCountForPagination(); if ($count > 1) { Capsule::table('review_round_files') ->where('file_id', '=', $row->file_id) diff --git a/classes/services/PKPUserService.inc.php b/classes/services/PKPUserService.inc.php index 16e9e38db16..b49ac050db5 100644 --- a/classes/services/PKPUserService.inc.php +++ b/classes/services/PKPUserService.inc.php @@ -594,7 +594,7 @@ public function getAccessibleWorkflowStages($userId, $contextId, &$submission, $ public function count($args = []) { return $this->getQueryBuilder($args) ->getQuery() - ->safeCount(); + ->getCountForPagination(); } /** diff --git a/classes/services/queryBuilders/PKPAnnouncementQueryBuilder.inc.php b/classes/services/queryBuilders/PKPAnnouncementQueryBuilder.inc.php index 5b83c87d7c8..dbefdc1aa1c 100644 --- a/classes/services/queryBuilders/PKPAnnouncementQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPAnnouncementQueryBuilder.inc.php @@ -70,7 +70,7 @@ public function searchPhrase($phrase) { public function getCount() { return $this ->getQuery() - ->safeCount(); + ->getCountForPagination(); } /** diff --git a/classes/services/queryBuilders/PKPAuthorQueryBuilder.inc.php b/classes/services/queryBuilders/PKPAuthorQueryBuilder.inc.php index 2b5a486126a..43c87233597 100644 --- a/classes/services/queryBuilders/PKPAuthorQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPAuthorQueryBuilder.inc.php @@ -102,7 +102,7 @@ public function filterByPublicationIds($publicationIds) { public function getCount() { return $this ->getQuery() - ->safeCount(); + ->getCountForPagination(); } /** diff --git a/classes/services/queryBuilders/PKPContextQueryBuilder.inc.php b/classes/services/queryBuilders/PKPContextQueryBuilder.inc.php index 0e7a0c8d4c5..ff9a348caad 100644 --- a/classes/services/queryBuilders/PKPContextQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPContextQueryBuilder.inc.php @@ -85,7 +85,7 @@ public function searchPhrase($phrase) { public function getCount() { return $this ->getQuery() - ->safeCount(); + ->getCountForPagination(); } /** diff --git a/classes/services/queryBuilders/PKPEmailTemplateQueryBuilder.inc.php b/classes/services/queryBuilders/PKPEmailTemplateQueryBuilder.inc.php index 035c2f41b7a..c664b8d79a8 100644 --- a/classes/services/queryBuilders/PKPEmailTemplateQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPEmailTemplateQueryBuilder.inc.php @@ -179,7 +179,7 @@ public function getCount() { $compiledQuery = $this->getCompiledQuery(); return Capsule::table(Capsule::raw('(' . $compiledQuery[0] . ') as email_template_count')) ->setBindings($compiledQuery[1]) - ->safeCount(); + ->getCountForPagination(); } /** diff --git a/classes/services/queryBuilders/PKPPublicationQueryBuilder.inc.php b/classes/services/queryBuilders/PKPPublicationQueryBuilder.inc.php index a5d696a8f21..0682a8d1e73 100644 --- a/classes/services/queryBuilders/PKPPublicationQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPPublicationQueryBuilder.inc.php @@ -85,7 +85,7 @@ public function offsetBy($offset) { public function getCount() { return $this ->getQuery() - ->safeCount(); + ->getCountForPagination(); } /** @@ -186,6 +186,6 @@ public function isDuplicateUrlPath($urlPath, $submissionId, $contextId) { ->where('url_path', '=' , $urlPath) ->where('p.submission_id', '!=', $submissionId) ->where('s.context_id', '=', $contextId) - ->safeCount(); + ->getCountForPagination(); } } diff --git a/classes/services/queryBuilders/PKPStatsEditorialQueryBuilder.inc.php b/classes/services/queryBuilders/PKPStatsEditorialQueryBuilder.inc.php index 0aab1c5342b..0d08c3ebe82 100644 --- a/classes/services/queryBuilders/PKPStatsEditorialQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPStatsEditorialQueryBuilder.inc.php @@ -95,7 +95,7 @@ public function countSubmissionsReceived() { $q->where('s.date_submitted', '<=', $this->dateEnd); } - return $q->safeCount(); + return $q->getCountForPagination(); } /** @@ -165,7 +165,7 @@ public function countByDecisions($decisions, $forSubmittedDate = false) { public function countByStatus($status) { return $this->_getObject() ->whereIn('s.status', (array) $status) - ->safeCount(); + ->getCountForPagination(); } /** @@ -180,7 +180,7 @@ public function countActiveByStages($stages) { return $this->_getObject() ->where('s.status', '=', STATUS_QUEUED) ->whereIn('s.stage_id', $stages) - ->safeCount(); + ->getCountForPagination(); } /** @@ -214,7 +214,7 @@ public function countPublished() { } } - return $q->safeCount(); + return $q->getCountForPagination(); } /** @@ -433,7 +433,7 @@ public function countImported() { ->when($this->dateEnd, function (Builder $q) { $q->where('s.date_submitted', '<=', $this->dateEnd); }) - ->safeCount(); + ->getCountForPagination(); } /** @@ -451,7 +451,7 @@ public function countInProgress() { ->when($this->dateEnd, function (Builder $q) { $q->where('s.date_submitted', '<=', $this->dateEnd); }) - ->safeCount(); + ->getCountForPagination(); } /** diff --git a/classes/services/queryBuilders/PKPSubmissionFileQueryBuilder.inc.php b/classes/services/queryBuilders/PKPSubmissionFileQueryBuilder.inc.php index 76ef719c315..2ce0f53df3f 100644 --- a/classes/services/queryBuilders/PKPSubmissionFileQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPSubmissionFileQueryBuilder.inc.php @@ -159,7 +159,7 @@ public function includeDependentFiles($includeDependentFiles) { public function getCount() { return $this ->getQuery() - ->safeCount(); + ->getCountForPagination(); } /** diff --git a/classes/services/queryBuilders/PKPSubmissionQueryBuilder.inc.php b/classes/services/queryBuilders/PKPSubmissionQueryBuilder.inc.php index 30bda53ac28..eebf365e2b9 100644 --- a/classes/services/queryBuilders/PKPSubmissionQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPSubmissionQueryBuilder.inc.php @@ -239,7 +239,7 @@ public function offsetBy($offset) { public function getCount() { return $this ->getQuery() - ->safeCount(); + ->getCountForPagination(); } /** diff --git a/classes/services/queryBuilders/PKPUserQueryBuilder.inc.php b/classes/services/queryBuilders/PKPUserQueryBuilder.inc.php index 22d2af34710..6455ac36ad9 100644 --- a/classes/services/queryBuilders/PKPUserQueryBuilder.inc.php +++ b/classes/services/queryBuilders/PKPUserQueryBuilder.inc.php @@ -426,7 +426,7 @@ public function offsetBy($offset) { public function getCount() { return $this ->getQuery() - ->safeCount(); + ->getCountForPagination(); } /** From 88a287e231bca1f12361a2d4747d303824dc8926 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Tue, 28 May 2024 12:24:09 +0300 Subject: [PATCH 16/18] pkp/pkp-lib#8700 Removed not-needed using --- classes/core/PKPApplication.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/classes/core/PKPApplication.inc.php b/classes/core/PKPApplication.inc.php index e22ec92c9fb..d43cd4a7905 100644 --- a/classes/core/PKPApplication.inc.php +++ b/classes/core/PKPApplication.inc.php @@ -69,7 +69,6 @@ define('WORKFLOW_TYPE_AUTHOR', 'author'); use Illuminate\Database\Capsule\Manager as Capsule; -use Illuminate\Database\Query\Builder; interface iPKPApplicationInfoProvider { /** @@ -222,6 +221,7 @@ public function initializeDatabaseConnection() { error_log("Database query\n$query->sql\n" . json_encode($query->bindings));//\n Bindings: " . print_r($query->bindings, true)); }); + // Set up Laravel queue handling $laravelContainer->bind('exception.handler', function () { return new class implements Illuminate\Contracts\Debug\ExceptionHandler { From b6759cc5b1441ea7f8432ee22248818453ce1627 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Tue, 28 May 2024 12:29:00 +0300 Subject: [PATCH 17/18] pkp/pkp-lib#8700 Improved documentation --- classes/db/DAO.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/classes/db/DAO.inc.php b/classes/db/DAO.inc.php index 25233a5d1aa..6902f0304ba 100644 --- a/classes/db/DAO.inc.php +++ b/classes/db/DAO.inc.php @@ -100,7 +100,7 @@ function retrieveRange($sql, $params = [], $dbResultRange = null, $callHooks = t /** * Count the number of records in the supplied SQL statement (with optional bind parameters parameters) * @param $sql string|Builder SQL query to be counted - * @param $params array Optional SQL query bind parameters + * @param $params array Optional SQL query bind parameters, only used when the $sql argument is a string * @return int */ public function countRecords($sql, $params = []) { From b790311163e48f7545b50c2e94b853a7f5988073 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Tue, 28 May 2024 13:29:57 +0300 Subject: [PATCH 18/18] pkp/pkp-lib#8700 Updated _getSearchSql() method not to generate a SQL with an order by clause --- classes/security/UserGroupDAO.inc.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/classes/security/UserGroupDAO.inc.php b/classes/security/UserGroupDAO.inc.php index d155e51ebdf..e782f98a2b4 100644 --- a/classes/security/UserGroupDAO.inc.php +++ b/classes/security/UserGroupDAO.inc.php @@ -544,7 +544,7 @@ function getUsersById($userGroupId = null, $contextId = null, $searchType = null ' . ($userGroupId ? 'AND ug.user_group_id = ?' : '') . ' )'; } - $baseSql .= ' ' . $this->_getSearchSql($searchType, $search, $searchMatch, $params, ''); + $baseSql .= ' ' . $this->_getSearchSql($searchType, $search, $searchMatch, $params); // Get the result set $result = $this->retrieveRange( @@ -830,10 +830,9 @@ function deleteSettingsByLocale($locale) { * @param string $search the keywords to search for. * @param string $searchMatch where to match (is, contains, startsWith). * @param array $params SQL parameter array reference - * @param ?string $querySuffix When null adds an ORDER BY clause using the UserDAO::getOrderBy() * @return string SQL search snippet */ - function _getSearchSql($searchType, $search, $searchMatch, &$params, $querySuffix = null) { + function _getSearchSql($searchType, $search, $searchMatch, &$params) { $hasUserSetting = "EXISTS( SELECT 0 FROM user_settings @@ -922,8 +921,6 @@ function _getSearchSql($searchType, $search, $searchMatch, &$params, $querySuffi } } - $searchSql .= $querySuffix ?? $this->userDao->getOrderBy(); // FIXME Add "sort field" parameter? - return $searchSql; }