From e8b6db20a26809bc77804bb35f5850a08cc21550 Mon Sep 17 00:00:00 2001 From: Yuri Kuznetsov Date: Fri, 3 Mar 2023 09:50:02 +0200 Subject: [PATCH] orm discard aggregation --- application/Espo/ORM/Mapper/BaseMapper.php | 177 +++++++++++------- .../ORM/QueryComposer/BaseQueryComposer.php | 174 +++-------------- tests/unit/Espo/ORM/MapperTest.php | 30 ++- 3 files changed, 158 insertions(+), 223 deletions(-) diff --git a/application/Espo/ORM/Mapper/BaseMapper.php b/application/Espo/ORM/Mapper/BaseMapper.php index 67d8000cef..2351b17cae 100644 --- a/application/Espo/ORM/Mapper/BaseMapper.php +++ b/application/Espo/ORM/Mapper/BaseMapper.php @@ -56,7 +56,8 @@ use RuntimeException; */ class BaseMapper implements RDBMapper { - protected const ATTRIBUTE_DELETED = 'deleted'; + protected const ATTR_DELETED = 'deleted'; + private const FUNC_COUNT = 'COUNT'; protected Helper $helper; @@ -115,7 +116,7 @@ class BaseMapper implements RDBMapper */ public function count(Select $select): int { - return (int) $this->aggregate($select, 'COUNT', 'id'); + return (int) $this->aggregate($select, self::FUNC_COUNT, 'id'); } public function max(Select $select, string $attribute): int|float @@ -186,23 +187,16 @@ class BaseMapper implements RDBMapper throw new RuntimeException("No entity type."); } - $select = $this->addFromAliasToSelectQuery($select); - $entity = $this->entityFactory->create($entityType); if (!$aggregation || !$entity->hasAttribute($aggregationBy)) { throw new RuntimeException(); } - $raw = $select->getRaw(); - - $raw['aggregation'] = $aggregation; - $raw['aggregationBy'] = $aggregationBy; - - $select = Select::fromRaw($raw); - - $sth = $this->queryExecutor->execute($select); + $select = $this->addFromAliasToSelectQuery($select); + $selectAggregation = $this->convertSelectQueryToAggregation($select, $aggregation, $aggregationBy); + $sth = $this->queryExecutor->execute($selectAggregation); $row = $sth->fetch(); if (!$row) { @@ -212,6 +206,56 @@ class BaseMapper implements RDBMapper return $row['value'] ?? null; } + private function convertSelectQueryToAggregation( + Select $select, + string $aggregation, + string $aggregationBy = 'id' + ): Select { + + $expression = "{$aggregation}:({$aggregationBy})"; + + $raw = $select->getRaw(); + + unset($raw['select']); + unset($raw['orderBy']); + unset($raw['order']); + unset($raw['offset']); + unset($raw['limit']); + unset($raw['distinct']); + unset($raw['forShare']); + unset($raw['forUpdate']); + + $selectAggregation = SelectBuilder::create() + ->clone(Select::fromRaw($raw)) + ->select($expression, 'value') + ->build(); + + $wrap = $aggregation === self::FUNC_COUNT && ( + $select->isDistinct() || + $select->getGroup() && $select->getHaving() + ); + + if (!$wrap) { + return $selectAggregation; + } + + $expression = "{$aggregation}:(asq.{$aggregationBy})"; + + $subQueryBuilder = SelectBuilder::create() + ->clone($selectAggregation) + ->select([]) + ->select('id'); + + if ($select->isDistinct()) { + $subQueryBuilder->distinct(); + } + + return SelectBuilder::create() + ->select($expression, 'value') + ->fromQuery($subQueryBuilder->build(), 'asq') + ->build(); + } + /** * Select related entities from DB. * @@ -259,14 +303,7 @@ class BaseMapper implements RDBMapper $relEntity = $this->entityFactory->create($relEntityType); } - if ($returnTotalCount) { - $params['aggregation'] = 'COUNT'; - $params['aggregationBy'] = 'id'; - } - - if (empty($params['whereClause'])) { - $params['whereClause'] = []; - } + $params['whereClause'] ??= []; $keySet = $this->helper->getRelationKeys($entity, $relationName); @@ -283,11 +320,14 @@ class BaseMapper implements RDBMapper $params['from'] = $relEntity->getEntityType(); $params['fromAlias'] ??= lcfirst($relEntity->getEntityType()); - $sth = $this->queryExecutor->execute(Select::fromRaw($params)); - - $row = $sth->fetch(); + $select = Select::fromRaw($params); if ($returnTotalCount) { + $select = $this->convertSelectQueryToAggregation($select, self::FUNC_COUNT); + + $sth = $this->queryExecutor->execute($select); + $row = $sth->fetch(); + if (!$row) { return 0; } @@ -295,6 +335,9 @@ class BaseMapper implements RDBMapper return (int) $row['value']; } + $sth = $this->queryExecutor->execute($select); + $row = $sth->fetch(); + if (!$row) { return null; } @@ -307,12 +350,10 @@ class BaseMapper implements RDBMapper case Entity::HAS_MANY: case Entity::HAS_CHILDREN: case Entity::HAS_ONE: - /** @var Entity $relEntity */ $params['from'] = $relEntity->getEntityType(); $params['fromAlias'] ??= lcfirst($relEntity->getEntityType()); - $params['whereClause'][$foreignKey] = $entity->get($key); if ($relType == Entity::HAS_CHILDREN) { @@ -336,10 +377,12 @@ class BaseMapper implements RDBMapper $params['limit'] = 1; } - $query = Select::fromRaw($params); + $select = Select::fromRaw($params); if ($returnTotalCount) { - $sth = $this->queryExecutor->execute($query); + $select = $this->convertSelectQueryToAggregation($select, self::FUNC_COUNT); + + $sth = $this->queryExecutor->execute($select); $row = $sth->fetch(); if (!$row) { @@ -350,7 +393,7 @@ class BaseMapper implements RDBMapper } if ($relType == Entity::HAS_ONE) { - $sth = $this->queryExecutor->execute($query); + $sth = $this->queryExecutor->execute($select); $row = $sth->fetch(); if (!$row) { @@ -363,28 +406,27 @@ class BaseMapper implements RDBMapper return $relEntity; } - return $this->collectionFactory->createFromQuery($query); + return $this->collectionFactory->createFromQuery($select); case Entity::MANY_MANY: - /** @var Entity $relEntity */ $params['from'] = $relEntity->getEntityType(); $params['fromAlias'] ??= lcfirst($relEntity->getEntityType()); - $params['joins'] ??= []; $params['joins'][] = $this->getManyManyJoin($entity, $relationName); - $params['select'] = $this->getModifiedSelectForManyToMany( $entity, $relationName, $params['select'] ?? [] ); - $query = Select::fromRaw($params); + $select = Select::fromRaw($params); if ($returnTotalCount) { - $sth = $this->queryExecutor->execute($query); + $select = $this->convertSelectQueryToAggregation($select, self::FUNC_COUNT); + + $sth = $this->queryExecutor->execute($select); $row = $sth->fetch(); if (!$row) { @@ -394,7 +436,7 @@ class BaseMapper implements RDBMapper return (int) $row['value']; } - return $this->collectionFactory->createFromQuery($query); + return $this->collectionFactory->createFromQuery($select); case Entity::BELONGS_TO_PARENT: $typeKey = $keySet['typeKey'] ?? null; @@ -419,12 +461,14 @@ class BaseMapper implements RDBMapper $params['from'] = $foreignEntityType; $params['fromAlias'] ??= lcfirst($foreignEntityType); - $query = Select::fromRaw($params); - - $sth = $this->queryExecutor->execute($query); - $row = $sth->fetch(); + $select = Select::fromRaw($params); if ($returnTotalCount) { + $select = $this->convertSelectQueryToAggregation($select, self::FUNC_COUNT); + + $sth = $this->queryExecutor->execute($select); + $row = $sth->fetch(); + if (!$row) { return 0; } @@ -432,6 +476,9 @@ class BaseMapper implements RDBMapper return (int) $row['value']; } + $sth = $this->queryExecutor->execute($select); + $row = $sth->fetch(); + if (!$row) { return null; } @@ -550,7 +597,7 @@ class BaseMapper implements RDBMapper $where = [ $nearKey => $entity->getId(), $distantKey => $id, - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ]; $conditions = $this->getRelationParam($entity, $relationName, 'conditions') ?? []; @@ -612,7 +659,7 @@ class BaseMapper implements RDBMapper $where = [ $nearKey => $entity->getId(), $distantKey => $id, - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ]; $conditions = $this->getRelationParam($entity, $relationName, 'conditions') ?? []; @@ -722,7 +769,7 @@ class BaseMapper implements RDBMapper 'columns' => $columns, 'valuesQuery' => Select::fromRaw($params), 'updateSet' => [ - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ], ]); @@ -794,7 +841,7 @@ class BaseMapper implements RDBMapper 'whereClause' => [ 'id!=' => $entity->getId(), $key => $id, - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ], 'set' => [ $key => null, @@ -811,7 +858,7 @@ class BaseMapper implements RDBMapper 'from' => $entityType, 'whereClause' => [ 'id' => $entity->getId(), - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ], 'set' => [ $key => $relEntity->getId(), @@ -835,7 +882,7 @@ class BaseMapper implements RDBMapper 'from' => $entityType, 'whereClause' => [ 'id' => $entity->getId(), - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ], 'set' => [ $key => $relEntity->getId(), @@ -863,7 +910,7 @@ class BaseMapper implements RDBMapper 'from' => $relEntity->getEntityType(), 'whereClause' => [ $foreignKey => $entity->getId(), - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ], 'set' => [ $foreignKey => null, @@ -874,7 +921,7 @@ class BaseMapper implements RDBMapper 'from' => $relEntity->getEntityType(), 'whereClause' => [ 'id' => $id, - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ], 'set' => [ $foreignKey => $entity->getId(), @@ -918,7 +965,7 @@ class BaseMapper implements RDBMapper 'from' => $relEntity->getEntityType(), 'whereClause' => [ 'id' => $id, - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ], 'set' => $set, ]); @@ -978,7 +1025,7 @@ class BaseMapper implements RDBMapper $columns = array_keys($values); $update = [ - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ]; foreach ($data as $column => $value) { @@ -1000,7 +1047,7 @@ class BaseMapper implements RDBMapper return true; } - $update = [static::ATTRIBUTE_DELETED => false]; + $update = [static::ATTR_DELETED => false]; foreach ($data as $column => $value) { $update[$column] = $value; @@ -1100,7 +1147,7 @@ class BaseMapper implements RDBMapper $entity->setFetched($typeKey, null); } - $where[static::ATTRIBUTE_DELETED] = false; + $where[static::ATTR_DELETED] = false; $query = Update::fromRaw([ 'from' => $entityType, @@ -1140,7 +1187,7 @@ class BaseMapper implements RDBMapper $update[$foreignType] = null; } - $where[static::ATTRIBUTE_DELETED] = false; + $where[static::ATTR_DELETED] = false; /** @var Entity $relEntity */ @@ -1182,7 +1229,7 @@ class BaseMapper implements RDBMapper $query = Update::fromRaw([ 'from' => $middleName, 'whereClause' => $where, - 'set' => [static::ATTRIBUTE_DELETED => true], + 'set' => [static::ATTR_DELETED => true], ]); $this->queryExecutor->execute($query); @@ -1340,7 +1387,7 @@ class BaseMapper implements RDBMapper } /** - * @return array + * @return array */ protected function getValueMapForUpdate(Entity $entity): array { @@ -1382,7 +1429,7 @@ class BaseMapper implements RDBMapper 'from' => $entity->getEntityType(), 'whereClause' => [ 'id' => $entity->getId(), - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ], 'set' => $valueMap, ]); @@ -1390,12 +1437,7 @@ class BaseMapper implements RDBMapper $this->queryExecutor->execute($query); } - /** - * @param ?string $type - * @param mixed $value - * @return mixed - */ - protected function prepareValueForInsert($type, $value) + protected function prepareValueForInsert(?string $type, mixed $value): mixed { if ($type == Entity::JSON_ARRAY && is_array($value)) { $value = json_encode($value, \JSON_UNESCAPED_UNICODE); @@ -1424,7 +1466,7 @@ class BaseMapper implements RDBMapper $whereClause = ['id' => $id]; if ($onlyDeleted) { - $whereClause[static::ATTRIBUTE_DELETED] = true; + $whereClause[static::ATTR_DELETED] = true; } $query = Delete::fromRaw([ @@ -1447,7 +1489,7 @@ class BaseMapper implements RDBMapper $query = Update::fromRaw([ 'from' => $entityType, 'whereClause' => ['id' => $id], - 'set' => [static::ATTRIBUTE_DELETED => false], + 'set' => [static::ATTR_DELETED => false], ]); $this->queryExecutor->execute($query); @@ -1458,7 +1500,7 @@ class BaseMapper implements RDBMapper */ public function delete(Entity $entity): void { - $entity->set(static::ATTRIBUTE_DELETED, true); + $entity->set(static::ATTR_DELETED, true); $this->update($entity); } @@ -1569,7 +1611,7 @@ class BaseMapper implements RDBMapper [ "{$distantKey}:" => $foreignKey, "{$nearKey}" => $entity->get($key), - static::ATTRIBUTE_DELETED => false, + static::ATTR_DELETED => false, ], ]; @@ -1636,10 +1678,7 @@ class BaseMapper implements RDBMapper return $entityDefs->getAttribute($attribute)->getParam($param); } - /** - * @return mixed - */ - protected function getRelationParam(Entity $entity, string $relation, string $param) + protected function getRelationParam(Entity $entity, string $relation, string $param): mixed { if ($entity instanceof BaseEntity) { return $entity->getRelationParam($relation, $param); diff --git a/application/Espo/ORM/QueryComposer/BaseQueryComposer.php b/application/Espo/ORM/QueryComposer/BaseQueryComposer.php index 15f886b1e0..f4fe98e3a4 100644 --- a/application/Espo/ORM/QueryComposer/BaseQueryComposer.php +++ b/application/Espo/ORM/QueryComposer/BaseQueryComposer.php @@ -72,8 +72,6 @@ abstract class BaseQueryComposer implements QueryComposer 'leftJoins', 'distinct', 'joinConditions', - 'aggregation', - 'aggregationBy', 'groupBy', 'havingClause', 'customHaving', @@ -562,10 +560,6 @@ abstract class BaseQueryComposer implements QueryComposer $params['leftJoins'] = $params['leftJoins'] ?? []; if ($method !== self::SELECT_METHOD) { - if (isset($params['aggregation'])) { - throw new RuntimeException("ORM Query: Param 'aggregation' is not allowed for '{$method}'."); - } - if (isset($params['offset'])) { throw new RuntimeException("ORM Query: Param 'offset' is not allowed for '{$method}'."); } @@ -593,6 +587,9 @@ abstract class BaseQueryComposer implements QueryComposer $entityType = $params['from'] ?? null; $fromQuery = $params['fromQuery'] ?? null; + $fromAlias = $params['fromAlias'] ?? null; + $whereClause = $params['whereClause'] ?? []; + $havingClause = $params['havingClause'] ?? []; if ($entityType === null && !$fromQuery) { return $this->createSelectQueryNoFrom($params); @@ -600,57 +597,18 @@ abstract class BaseQueryComposer implements QueryComposer $entity = $this->getSeed($entityType); - $isAggregation = (bool) ($params['aggregation'] ?? null); - - $whereClause = $params['whereClause'] ?? []; - $havingClause = $params['havingClause'] ?? []; - if (!$params['withDeleted'] && $entity->hasAttribute('deleted')) { $whereClause = $whereClause + ['deleted' => false]; } - $selectPart = null; - $orderPart = null; - $havingPart = null; - $tailPart = null; - $wherePart = $this->getWherePart($entity, $whereClause, 'AND', $params); - - if (!empty($havingClause)) { - $havingPart = $this->getWherePart($entity, $havingClause, 'AND', $params); - } - - if (!$isAggregation) { - $orderPart = $this->getOrderPart($entity, $params['orderBy'], $params['order'], $params); - - $selectPart = $this->getSelectPart($entity, $params); - - $additionalSelectPart = $this->getAdditionalSelect($entity, $params); - - if ($additionalSelectPart) { - $selectPart .= $additionalSelectPart; - } - - $tailPart = $this->getSelectTailPart($params); - } - - if ($isAggregation) { - $aggregationDistinct = false; - - if ($params['distinct'] && $params['aggregation'] == 'COUNT') { - $aggregationDistinct = true; - } - - $params['select'] = []; - - $selectPart = $this->getAggregationSelectPart( - $entity, - $params['aggregation'], - $params['aggregationBy'], - $aggregationDistinct, - $params - ); - } + $havingPart = $havingClause ? $this->getWherePart($entity, $havingClause, 'AND', $params) : null; + $orderPart = $this->getOrderPart($entity, $params['orderBy'], $params['order'], $params); + $selectPart = $this->getSelectPart($entity, $params); + $selectPart .= $this->getAdditionalSelect($entity, $params) ?? ''; + $tailPart = $this->getSelectTailPart($params); + $joinsPart = $this->getJoinsPart($entity, $params, true); + $groupByPart = $this->getGroupByPart($entity, $params); // @todo remove 'customWhere' support if (!empty($params['customWhere'])) { @@ -670,61 +628,23 @@ abstract class BaseQueryComposer implements QueryComposer $havingPart .= $params['customHaving']; } - $joinsPart = $this->getJoinsPart($entity, $params, !$isAggregation); + $indexKeyList = $entityType ? + $this->getIndexKeyList($entityType, $params) : null; - $groupByPart = $this->getGroupByPart($entity, $params); + $fromAlias = $fromAlias ? + $this->sanitize($fromAlias) : null; - $indexKeyList = []; - - if ($entityType) { - $indexKeyList = $this->getIndexKeyList($entityType, $params); - } - - $fromAlias = $params['fromAlias'] ?? null; - - if ($fromAlias) { - $fromAlias = $this->sanitize($fromAlias); - } - - $fromPart = null; - - if ($entityType) { - $fromPart = $this->quoteIdentifier( - $this->toDb($entityType) + $fromPart = $fromQuery ? + '(' . $this->composeSelecting($fromQuery) . ')' : + ( + $entityType ? + $this->quoteIdentifier($this->toDb($entityType)) : null ); - } - - if ($fromQuery) { - $fromPart = '(' . $this->composeSelecting($fromQuery) . ')'; - } /** @var string $selectPart */ /** @var string $fromAlias */ - if ($isAggregation) { - $sql = $this->composeSelectQuery( - $fromPart, - $selectPart, - $fromAlias, - $joinsPart, - $wherePart, - null, - null, - null, - false, - $groupByPart, - $havingPart, - $indexKeyList - ); - - if ($params['aggregation'] === 'COUNT' && $groupByPart && $havingPart) { - return $this->wrapCountSql($sql); - } - - return $sql; - } - - $sql = $this->composeSelectQuery( + return $this->composeSelectQuery( $fromPart, $selectPart, $fromAlias, @@ -739,15 +659,6 @@ abstract class BaseQueryComposer implements QueryComposer $indexKeyList, $tailPart ); - - return $sql; - } - - protected function wrapCountSql(string $sql): string - { - return - "SELECT COUNT(*) AS " . $this->quoteIdentifier('value') . " ". - "FROM ({$sql}) AS " . $this->quoteIdentifier('countAlias'); } /** @@ -757,12 +668,7 @@ abstract class BaseQueryComposer implements QueryComposer { $selectPart = $this->getSelectPart(null, $params); - $sql = $this->composeSelectQuery( - null, - $selectPart - ); - - return $sql; + return $this->composeSelectQuery(null, $selectPart); } /** @@ -1128,17 +1034,6 @@ abstract class BaseQueryComposer implements QueryComposer return 'FIELD(' . implode(', ', $argumentPartList) . ')'; } - if ($distinct) { - $fromAlias = $this->getFromAlias($params, $entityType); - - $idPart = $fromAlias . ".id"; - - switch ($function) { - case 'COUNT': - return $function . "({$part}) * COUNT(DISTINCT {$idPart}) / COUNT({$idPart})"; - } - } - return $function . '(' . $part . ')'; } @@ -2230,33 +2125,6 @@ abstract class BaseQueryComposer implements QueryComposer return $this->getAttributePath($entity, $orderBy, $params); } - /** - * @param array $params - */ - protected function getAggregationSelectPart( - Entity $entity, - string $aggregation, - string $aggregationBy, - bool $distinct, - array $params - ): ?string { - - if (!$entity->hasAttribute($aggregationBy)) { - return null; - } - - $aggregationPart = $this->sanitize(strtoupper($aggregation)); - - $distinctPart = $distinct ? 'DISTINCT ' : ''; - - $fromAlias = $this->getFromAlias($params, $entity->getEntityType()); - - $columnPart = "{$fromAlias}." . $this->toDb($this->sanitize($aggregationBy)); - $columnPart = $this->quoteColumn($columnPart); - - return "{$aggregationPart}({$distinctPart}{$columnPart}) AS " . $this->quoteIdentifier('value'); - } - /** * Quote a value (if needed). * @deprecated As of v6.0. Not meant to be used outside as Query Builder should be used to diff --git a/tests/unit/Espo/ORM/MapperTest.php b/tests/unit/Espo/ORM/MapperTest.php index 5adcc3e996..fdc8f978bb 100644 --- a/tests/unit/Espo/ORM/MapperTest.php +++ b/tests/unit/Espo/ORM/MapperTest.php @@ -41,7 +41,6 @@ use Espo\ORM\Query\Select; use Espo\ORM\Query\SelectBuilder; use Espo\ORM\QueryComposer\MysqlQueryComposer as QueryComposer; use Espo\ORM\QueryComposer\QueryComposerWrapper; -use Espo\ORM\Executor\QueryExecutor; use Espo\ORM\Executor\SqlExecutor; use Espo\ORM\SthCollection; @@ -605,6 +604,35 @@ class MapperTest extends \PHPUnit\Framework\TestCase $this->assertEquals($count, 1); } + public function testCount1(): void + { + $sql = "SELECT COUNT(post.id) AS `value` FROM `post` AS `post` WHERE post.deleted = 0"; + + $select = SelectBuilder::create() + ->from('Post') + ->build(); + + $this->mockQuery($sql, ['value' => 1]); + + $this->db->count($select); + } + + public function testCountWithDistinct(): void + { + $sql = "SELECT COUNT(asq.id) AS `value` FROM (" . + "SELECT DISTINCT post.id AS `id` FROM `post` AS `post` WHERE post.deleted = 0" . + ") AS `asq`"; + + $select = SelectBuilder::create() + ->from('Post') + ->distinct() + ->build(); + + $this->mockQuery($sql, ['value' => 1]); + + $this->db->count($select); + } + public function testInsert() { $query = "INSERT INTO `post` (`id`, `name`) VALUES ('1', 'test')";