From bfb7d88b79cf77b35d57c7b7f450269646797867 Mon Sep 17 00:00:00 2001 From: Yuri Kuznetsov Date: Wed, 13 Apr 2022 11:18:12 +0300 Subject: [PATCH] refactoring --- .../Select/Applier/Appliers/TextFilter.php | 120 +++++++++--------- .../Core/Select/Text/FullTextSearchData.php | 40 +----- .../Text/FullTextSearchDataComposer.php | 28 ++-- .../Appliers/TextFilterApplierTest.php | 5 +- .../Text/FullTextSearchDataComposerTest.php | 7 +- .../Select/Text/FullTextSearchDataTest.php | 39 +----- 6 files changed, 94 insertions(+), 145 deletions(-) diff --git a/application/Espo/Core/Select/Applier/Appliers/TextFilter.php b/application/Espo/Core/Select/Applier/Appliers/TextFilter.php index 73460fd50a..7d9b0979c6 100644 --- a/application/Espo/Core/Select/Applier/Appliers/TextFilter.php +++ b/application/Espo/Core/Select/Applier/Appliers/TextFilter.php @@ -39,7 +39,8 @@ use Espo\Core\Select\Text\FilterFactory; use Espo\ORM\Query\SelectBuilder as QueryBuilder; use Espo\ORM\Query\Part\Order as OrderExpr; -use Espo\ORM\Query\Part\Where\AndGroup; +use Espo\ORM\Query\Part\Expression as Expr; +use Espo\ORM\Query\Part\WhereItem; use Espo\Entities\User; @@ -134,65 +135,11 @@ class TextFilter ); } - $fullTextGroup = []; + $fullTextWhere = $fullTextSearchData ? + $this->processFullTextSearch($queryBuilder, $fullTextSearchData) : + null; - $fullTextSearchFieldList = []; - - $hasFullTextSearch = false; - - if ($fullTextSearchData) { - if ($this->fullTextRelevanceThreshold) { - $fullTextGroup[] = [ - $fullTextSearchData->getExpression() . '>=' => $this->fullTextRelevanceThreshold - ]; - } - else { - $fullTextGroup[] = $fullTextSearchData->getExpression(); - } - - $fullTextSearchFieldList = $fullTextSearchData->getFieldList(); - $relevanceExpression = $fullTextSearchData->getExpression(); - - $fullTextOrderType = self::DEFAULT_FT_ORDER; - - $orderTypeMap = [ - 'combined' => self::FT_ORDER_COMBINTED, - 'relevance' => self::FT_ORDER_RELEVANCE, - 'original' => self::FT_ORDER_ORIGINAL, - ]; - - $mOrderType = $this->metadataProvider->getFullTextSearchOrderType($this->entityType); - - if ($mOrderType) { - $fullTextOrderType = $orderTypeMap[$mOrderType]; - } - - $previousOrderBy = $queryBuilder->build()->getOrder(); - - $hasOrderBy = !empty($previousOrderBy); - - if (!$hasOrderBy || $fullTextOrderType === self::FT_ORDER_RELEVANCE) { - $queryBuilder->order([ - OrderExpr::fromString($relevanceExpression)->withDesc() - ]); - } - else if ($fullTextOrderType === self::FT_ORDER_COMBINTED) { - $relevanceExpression = - 'ROUND:(DIV:(' . $fullTextSearchData->getExpression() . ',' . - $this->fullTextOrderRelevanceDivider . '))'; - - $newOrderBy = array_merge( - [ - OrderExpr::fromString($relevanceExpression)->withDesc() - ], - $previousOrderBy - ); - - $queryBuilder->order($newOrderBy); - } - - $hasFullTextSearch = true; - } + $fullTextSearchFieldList = $fullTextSearchData ? $fullTextSearchData->getFieldList() : []; $fieldList = array_filter( $this->metadataProvider->getTextFilterAttributeList($this->entityType) ?? ['name'], @@ -213,9 +160,7 @@ class TextFilter $filterData = FilterData::create($filter, $fieldList) ->withSkipWildcards($skipWildcards) ->withForceFullTextSearch($forceFullTextSearch) - ->withFullTextSearchWhereItem( - $hasFullTextSearch ? AndGroup::fromRaw($fullTextGroup) : null - ); + ->withFullTextSearchWhereItem($fullTextWhere); $this->filterFactory ->create($this->entityType, $this->user) @@ -232,4 +177,55 @@ class TextFilter return $composer->compose($filter, $params); } + + private function processFullTextSearch(QueryBuilder $queryBuilder, FullTextSearchData $data): WhereItem + { + $expression = $data->getExpression(); + + $fullTextOrderType = self::DEFAULT_FT_ORDER; + + $orderTypeMap = [ + 'combined' => self::FT_ORDER_COMBINTED, + 'relevance' => self::FT_ORDER_RELEVANCE, + 'original' => self::FT_ORDER_ORIGINAL, + ]; + + $mOrderType = $this->metadataProvider->getFullTextSearchOrderType($this->entityType); + + if ($mOrderType) { + $fullTextOrderType = $orderTypeMap[$mOrderType]; + } + + $previousOrderBy = $queryBuilder->build()->getOrder(); + + $hasOrderBy = !empty($previousOrderBy); + + if (!$hasOrderBy || $fullTextOrderType === self::FT_ORDER_RELEVANCE) { + $queryBuilder->order([ + OrderExpr::create($expression)->withDesc() + ]); + } + else if ($fullTextOrderType === self::FT_ORDER_COMBINTED) { + $orderExpression = + Expr::round( + Expr::divide($expression, $this->fullTextOrderRelevanceDivider) + ); + + $newOrderBy = array_merge( + [OrderExpr::create($orderExpression)->withDesc()], + $previousOrderBy + ); + + $queryBuilder->order($newOrderBy); + } + + if ($this->fullTextRelevanceThreshold) { + return Expr::greaterOrEqual( + $expression, + $this->fullTextRelevanceThreshold + ); + } + + return $expression; + } } diff --git a/application/Espo/Core/Select/Text/FullTextSearchData.php b/application/Espo/Core/Select/Text/FullTextSearchData.php index 044ec35ebc..37e7beb288 100644 --- a/application/Espo/Core/Select/Text/FullTextSearchData.php +++ b/application/Espo/Core/Select/Text/FullTextSearchData.php @@ -29,11 +29,12 @@ namespace Espo\Core\Select\Text; -use InvalidArgumentException; +use Espo\ORM\Query\Part\Expression; + class FullTextSearchData { - private string $expression; + private Expression $expression; /** * @var string[] @@ -46,48 +47,17 @@ class FullTextSearchData private array $columnList; /** - * @param string $expression * @param string[] $fieldList * @param string[] $columnList */ - public function __construct(string $expression, array $fieldList, array $columnList) + public function __construct(Expression $expression, array $fieldList, array $columnList) { $this->expression = $expression; $this->fieldList = $fieldList; $this->columnList = $columnList; } - /** - * @param array $params - */ - public static function fromArray(array $params): self - { - $expression = $params['expression'] ?? null; - - if (!$expression || !is_string($expression)) { - throw new InvalidArgumentException("Bad expression."); - } - - $object = new self( - $expression, - $params['fieldList'] ?? [], - $params['columnList'] ?? [] - ); - - $object->expression = $expression; - $object->fieldList = $params['fieldList'] ?? []; - $object->columnList = $params['columnList'] ?? []; - - foreach ($params as $key => $value) { - if (!property_exists($object, $key)) { - throw new InvalidArgumentException("Unknown parameter '{$key}'."); - } - } - - return $object; - } - - public function getExpression(): string + public function getExpression(): Expression { return $this->expression; } diff --git a/application/Espo/Core/Select/Text/FullTextSearchDataComposer.php b/application/Espo/Core/Select/Text/FullTextSearchDataComposer.php index 9db44c7c65..b84c60b6f9 100644 --- a/application/Espo/Core/Select/Text/FullTextSearchDataComposer.php +++ b/application/Espo/Core/Select/Text/FullTextSearchDataComposer.php @@ -31,15 +31,18 @@ namespace Espo\Core\Select\Text; use Espo\Core\Utils\Config; +use Espo\ORM\Query\Part\Expression\Util as ExpressionUtil; +use Espo\ORM\Query\Part\Expression; + class FullTextSearchDataComposer { private const MIN_LENGTH = 4; - protected string $entityType; + private string $entityType; - protected Config $config; + private Config $config; - protected MetadataProvider $metadataProvider; + private MetadataProvider $metadataProvider; public function __construct( string $entityType, @@ -154,7 +157,6 @@ class FullTextSearchDataComposer $filter = str_replace('"*', '"', $filter); $filter = str_replace('*"', '"', $filter); - $filter = str_replace('\'', '\'\'', $filter); while (strpos($filter, '**')) { $filter = str_replace('**', '*', $filter); @@ -168,13 +170,19 @@ class FullTextSearchDataComposer $filter = trim($filter); } - $expression = $function . ':(' . implode(', ', $fullTextSearchColumnList ?? []) . ', ' . "'{$filter}'" . ')'; + $argumentList = array_merge( + array_map( + function ($item) { + return Expression::column($item); + }, + $fullTextSearchColumnList ?? [] + ), + [$filter] + ); - return FullTextSearchData::fromArray([ - 'expression' => $expression, - 'fieldList' => $fullTextSearchFieldList, - 'columnList' => $fullTextSearchColumnList, - ]); + $expression = ExpressionUtil::composeFunction($function, ...$argumentList); + + return new FullTextSearchData($expression, $fullTextSearchFieldList, $fullTextSearchColumnList ?? []); } /** diff --git a/tests/unit/Espo/Core/Select/Applier/Appliers/TextFilterApplierTest.php b/tests/unit/Espo/Core/Select/Applier/Appliers/TextFilterApplierTest.php index 561d5e7f65..41f54efc8c 100644 --- a/tests/unit/Espo/Core/Select/Applier/Appliers/TextFilterApplierTest.php +++ b/tests/unit/Espo/Core/Select/Applier/Appliers/TextFilterApplierTest.php @@ -46,6 +46,7 @@ use Espo\Core\{ use Espo\{ ORM\Query\SelectBuilder as QueryBuilder, ORM\Query\Part\Where\OrGroup, + ORM\Query\Part\Expression as Expr, ORM\Entity, Entities\User, }; @@ -203,7 +204,7 @@ class TextFilterApplierTest extends \PHPUnit\Framework\TestCase if (!$noFullTextSearch) { - $expectedWhere[] = 'TEST:(test)'; + $expectedWhere[] = ['TEST:(test):' => null]; } $this->queryBuilder @@ -250,7 +251,7 @@ class TextFilterApplierTest extends \PHPUnit\Framework\TestCase $fullTextSearchData ->expects($this->any()) ->method('getExpression') - ->willReturn($expression); + ->willReturn(Expr::create($expression)); $fullTextSearchDataComposer ->expects($this->any()) diff --git a/tests/unit/Espo/Core/Select/Text/FullTextSearchDataComposerTest.php b/tests/unit/Espo/Core/Select/Text/FullTextSearchDataComposerTest.php index 57c4cd347b..bf778a7ba5 100644 --- a/tests/unit/Espo/Core/Select/Text/FullTextSearchDataComposerTest.php +++ b/tests/unit/Espo/Core/Select/Text/FullTextSearchDataComposerTest.php @@ -38,7 +38,7 @@ use Espo\Core\{ class FullTextSearchDataComposerTest extends \PHPUnit\Framework\TestCase { - protected function setUp() : void + protected function setUp(): void { $this->config = $this->createMock(Config::class); $this->metadataProvider = $this->createMock(MetadataProvider::class); @@ -125,7 +125,10 @@ class FullTextSearchDataComposerTest extends \PHPUnit\Framework\TestCase $this->assertEquals(['field1A', 'field1B', 'field2'], $data->getColumnList()); $this->assertEquals(['field1', 'field2'], $data->getFieldList()); - $this->assertEquals('MATCH_BOOLEAN:(field1A, field1B, field2, \'test filter\')', $data->getExpression()); + $this->assertEquals( + 'MATCH_BOOLEAN:(field1A, field1B, field2, \'test filter\')', + $data->getExpression()->getValue() + ); } public function testCompose2() diff --git a/tests/unit/Espo/Core/Select/Text/FullTextSearchDataTest.php b/tests/unit/Espo/Core/Select/Text/FullTextSearchDataTest.php index 3f66b9cbe5..e57f2190fc 100644 --- a/tests/unit/Espo/Core/Select/Text/FullTextSearchDataTest.php +++ b/tests/unit/Espo/Core/Select/Text/FullTextSearchDataTest.php @@ -29,47 +29,18 @@ namespace tests\unit\Espo\Core\Select\Text; -use Espo\Core\{ - Select\Text\FullTextSearchData, -}; +use Espo\ORM\Query\Part\Expression as Expr; -use InvalidArgumentException; +use Espo\Core\Select\Text\FullTextSearchData; class FullTextSearchDataTest extends \PHPUnit\Framework\TestCase { - protected function setUp() : void + public function testGet1() { - } + $item = new FullTextSearchData(Expr::create('TEST:()'), ['test'], ['column']); - public function testFromArray() - { - $item = FullTextSearchData::fromArray([ - 'expression' => 'TEST', - 'fieldList' => ['test'], - 'columnList' => ['column'], - ]); - - $this->assertEquals('TEST', $item->getExpression()); + $this->assertEquals('TEST:()', $item->getExpression()->getValue()); $this->assertEquals(['test'], $item->getFieldList()); $this->assertEquals(['column'], $item->getColumnList()); } - - public function testEmpty() - { - $item = FullTextSearchData::fromArray([ - 'expression' => 'TEST', - ]); - - $this->assertEquals([], $item->getFieldList()); - $this->assertEquals([], $item->getColumnList()); - } - - public function testNonExistingParam() - { - $this->expectException(InvalidArgumentException::class); - - $params = FullTextSearchData::fromArray([ - 'bad' => 'd', - ]); - } }