diff --git a/src/Controller/PartListsController.php b/src/Controller/PartListsController.php index 2210fc186..ce2eb2e94 100644 --- a/src/Controller/PartListsController.php +++ b/src/Controller/PartListsController.php @@ -37,6 +37,7 @@ use App\Services\Parts\PartsTableActionHandler; use App\Services\Trees\NodesListBuilder; use App\Settings\BehaviorSettings\SidebarSettings; +use App\Settings\BehaviorSettings\SearchSettings; use App\Settings\BehaviorSettings\TableSettings; use Doctrine\DBAL\Exception\DriverException; use Doctrine\ORM\EntityManagerInterface; @@ -59,6 +60,7 @@ public function __construct(private readonly EntityManagerInterface $entityManag private readonly TranslatorInterface $translator, private readonly TableSettings $tableSettings, private readonly SidebarSettings $sidebarSettings, + private readonly SearchSettings $searchSettings, ) { } @@ -315,7 +317,7 @@ function (PartFilter $filter) use ($tag) { private function searchRequestToFilter(Request $request): PartSearchFilter { - $filter = new PartSearchFilter($request->query->get('keyword', '')); + $filter = new PartSearchFilter($request->query->get('keyword', ''), $this->searchSettings); //As an unchecked checkbox is not set in the query, the default value for all bools have to be false (which is the default argument value)! $filter->setName($request->query->getBoolean('name')); diff --git a/src/DataTables/Filters/PartSearchFilter.php b/src/DataTables/Filters/PartSearchFilter.php index 9f6734e56..9d8ee394c 100644 --- a/src/DataTables/Filters/PartSearchFilter.php +++ b/src/DataTables/Filters/PartSearchFilter.php @@ -22,8 +22,11 @@ */ namespace App\DataTables\Filters; use App\DataTables\Filters\Constraints\AbstractConstraint; +use Doctrine\Common\Collections\ArrayCollection; use Doctrine\ORM\QueryBuilder; +use Doctrine\ORM\Query\Parameter; use Doctrine\DBAL\ParameterType; +use App\Settings\BehaviorSettings\SearchSettings; class PartSearchFilter implements FilterInterface { @@ -70,11 +73,16 @@ class PartSearchFilter implements FilterInterface /** @var bool Use Internal Part number for searching */ protected bool $ipn = true; + /** @var int array_map iteration helper variable */ + protected int $it = 0; + public function __construct( /** @var string The string to query for */ - protected string $keyword - ) - { + protected string $keyword, + /** @var SearchSettings The settings that control how the search operates */ + private readonly SearchSettings $searchSettings, + ) { + } protected function getFieldsToSearch(): array @@ -124,52 +132,90 @@ protected function getFieldsToSearch(): array public function apply(QueryBuilder $queryBuilder): void { $fields_to_search = $this->getFieldsToSearch(); - $is_numeric = preg_match('/^\d+$/', $this->keyword) === 1; + $is_numeric = preg_match('/^\d+$/', trim($this->keyword)) === 1; // Add exact ID match only when the keyword is numeric $search_dbId = $is_numeric && (bool)$this->dbId; - //If we have nothing to search for, do nothing - if (($fields_to_search === [] && !$search_dbId) || $this->keyword === '') { + $tokens = []; + if ($this->searchSettings->enableAdvancedSearch) { + //Transform keyword and trim excess spaces + $this->keyword = trim(str_replace('+', ' ', $this->keyword)); + //Split keyword on spaces, but limit token count (default is 3) + $tokens = explode(' ', $this->keyword, $this->searchSettings->searchTokenLimit); + //Throw away array elements which are null or have zero length + $tokens = array_filter($tokens, fn($x) => (strlen($x) > 0)); + } + else { + //Pass the whole keyword into the (empty) tokens array as is, + //retaining the original search behavior + $tokens[] = $this->keyword; + } + + //If we have nothing to search for... + if (($fields_to_search === [] && !$search_dbId) || $this->keyword === '' || empty($tokens)) { + // ...enforce returning no results + $queryBuilder->add('where','1 = 0'); return; } $expressions = []; - - if($fields_to_search !== []) { - //Convert the fields to search to a list of expressions - $expressions = array_map(function (string $field): string { - if ($this->regex) { - return sprintf("REGEXP(%s, :search_query) = TRUE", $field); - } + $expressions2 = []; + $params = []; - return sprintf("ILIKE(%s, :search_query) = TRUE", $field); - }, $fields_to_search); - - //For regex, we pass the query as is, for like we add % to the start and end as wildcards + //Search in selected fields, either based on regex or on tokenized keyword + if ($fields_to_search !== []) { + //For regex, we pass the query as is if ($this->regex) { - $queryBuilder->setParameter('search_query', $this->keyword); + //Convert the fields to search to a list of expressions + $expressions = array_merge($expressions, array_map(function (string $field): string { + return sprintf("REGEXP(%s, :search_query) = TRUE", $field); + }, $fields_to_search)); + $params[] = new Parameter('search_query', $this->keyword); } else { - //Escape % and _ characters in the keyword - $this->keyword = str_replace(['%', '_'], ['\%', '\_'], $this->keyword); - $queryBuilder->setParameter('search_query', '%' . $this->keyword . '%'); + //Add a new expression and parameter set to the query for each token + foreach ($tokens as $i => $token) { + //Conditionally escape % and _ characters + if ($this->searchSettings->escapeSQLWildcards) + $token = str_replace(['%', '_'], ['\%', '\_'], $token); + + //Convert the fields to search to a list of expressions + $tmp = array_fill_keys($fields_to_search, $i); + $expressions2 = array_map(function (string $field, int $idx): string { + return sprintf("ILIKE(%s, :search_query%u) = TRUE", $field, $idx); + }, array_keys($tmp), array_values($tmp)); + + //Aggregate the parameters for consolidated commission at the end + //For like, we add % to the start and end as wildcards + $params[] = new Parameter('search_query' . $i, '%' . $token . '%'); + + //Guard condition + if (!empty($expressions2)) { + //Add Or concatenation of the expressions to our query + $queryBuilder->andWhere( + $queryBuilder->expr()->orX(...$expressions2) + ); + } + } } } - //Use equal expression to just search for exact numeric matches - if ($search_dbId) { - $expressions[] = $queryBuilder->expr()->eq('part.id', ':id_exact'); - $queryBuilder->setParameter('id_exact', (int) $this->keyword, - ParameterType::INTEGER); - } - //Guard condition if (!empty($expressions)) { //Add Or concatenation of the expressions to our query $queryBuilder->andWhere( $queryBuilder->expr()->orX(...$expressions) ); + } + //Use equal expression to search for exact numeric matches + if ($search_dbId) { + $queryBuilder->orWhere($queryBuilder->expr()->eq('part.id', ':id_exact')); + $params[] = new Parameter('id_exact', (int)$this->keyword, + ParameterType::INTEGER); } + $queryBuilder->setParameters( + new ArrayCollection($params) + ); } public function getKeyword(): string diff --git a/src/Settings/BehaviorSettings/BehaviorSettings.php b/src/Settings/BehaviorSettings/BehaviorSettings.php index ec849db3c..2740466f8 100644 --- a/src/Settings/BehaviorSettings/BehaviorSettings.php +++ b/src/Settings/BehaviorSettings/BehaviorSettings.php @@ -44,4 +44,7 @@ class BehaviorSettings #[EmbeddedSettings] public ?KeybindingsSettings $keybindings = null; + + #[EmbeddedSettings] + public ?SearchSettings $search = null; } diff --git a/src/Settings/BehaviorSettings/SearchSettings.php b/src/Settings/BehaviorSettings/SearchSettings.php new file mode 100644 index 000000000..dd67d51c1 --- /dev/null +++ b/src/Settings/BehaviorSettings/SearchSettings.php @@ -0,0 +1,74 @@ +. + */ + +declare(strict_types=1); + + +namespace App\Settings\BehaviorSettings; + +use App\Settings\SettingsIcon; +use Jbtronics\SettingsBundle\Metadata\EnvVarMode; +use Jbtronics\SettingsBundle\Settings\Settings; +use Jbtronics\SettingsBundle\Settings\SettingsParameter; +use Symfony\Component\Translation\TranslatableMessage as TM; +use Symfony\Component\Validator\Constraints as Assert; + +#[Settings(name: "search", label: new TM("settings.behavior.search"))] +#[SettingsIcon('fa-magnifying-glass')] +class SearchSettings +{ + /** + * Whether to enable advanced search + * @var bool + */ + #[SettingsParameter( + label: new TM("settings.behavior.search.enable_advanced_search"), + description: new TM("settings.behavior.search.enable_advanced_search.help"), + envVar: "bool:ENABLE_ADVANCED_SEARCH", + envVarMode: EnvVarMode::OVERWRITE + )] + public bool $enableAdvancedSearch = false; + + /** + * Defines the maximum number of tokens the keyword can be split into + * @var int + */ + #[SettingsParameter( + label: new TM("settings.behavior.search.token_limit"), + description: new TM("settings.behavior.search.token_limit.help"), + envVar: "int:SEARCH_TOKEN_LIMIT", + envVarMode: EnvVarMode::OVERWRITE, + formOptions: ['attr' => ['min' => 2, 'max' => 5]], + )] + #[Assert\Range(min: 2, max: 5)] + public int $searchTokenLimit = 3; + + /** + * Whether to escape sql wildcards + * @var bool + */ + #[SettingsParameter( + label: new TM("settings.behavior.search.escape_sql_wildcards"), + description: new TM("settings.behavior.search.escape_sql_wildcards.help"), + envVar: "bool:ESCAPE_SQL_WILDCARDS", + envVarMode: EnvVarMode::OVERWRITE + )] + public bool $escapeSQLWildcards = true; +} diff --git a/tests/DataTables/Filters/PartSearchFilterTest.php b/tests/DataTables/Filters/PartSearchFilterTest.php new file mode 100644 index 000000000..91d04c741 --- /dev/null +++ b/tests/DataTables/Filters/PartSearchFilterTest.php @@ -0,0 +1,242 @@ +. + */ + +namespace App\Tests\DataTables\Filters; + +use App\DataTables\Filters\PartSearchFilter; +use App\Settings\BehaviorSettings\SearchSettings; +use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\DBAL\ParameterType; +use Doctrine\ORM\Query\Expr; +use Doctrine\ORM\Query\Expr\Comparison; +use Doctrine\ORM\Query\Expr\Orx; +use Doctrine\ORM\Query\Parameter; +use Doctrine\ORM\QueryBuilder; +use PHPUnit\Framework\TestCase; + +final class PartSearchFilterTest extends TestCase +{ + private function makeSearchSettings( + bool $enableAdvancedSearch = false, + int $searchTokenLimit = 3, + bool $escapeSQLWildcards = true, + ): SearchSettings { + $settings = $this->createMock(SearchSettings::class); + $settings->enableAdvancedSearch = $enableAdvancedSearch; + $settings->searchTokenLimit = $searchTokenLimit; + $settings->escapeSQLWildcards = $escapeSQLWildcards; + + return $settings; + } + + public function testApplyEnforcesNoResultsWhenKeywordEmpty(): void + { + $filter = new PartSearchFilter('', $this->makeSearchSettings()); + + $qb = $this->createMock(QueryBuilder::class); + $qb->expects($this->once()) + ->method('add') + ->with('where', '1 = 0'); + $qb->expects($this->never())->method('andWhere'); + $qb->expects($this->never())->method('setParameters'); + + $filter->apply($qb); + } + + public function testApplyEnforcesNoResultsWhenNothingToSearchForAndNoExactIdSearch(): void + { + $filter = (new PartSearchFilter('foo', $this->makeSearchSettings())) + ->setName(false) + ->setCategory(false) + ->setDescription(false) + ->setComment(false) + ->setTags(false) + ->setStorelocation(false) + ->setOrdernr(false) + ->setMpn(false) + ->setSupplier(false) + ->setManufacturer(false) + ->setFootprint(false) + ->setIPN(false) + ->setDbId(false); + + $qb = $this->createMock(QueryBuilder::class); + $qb->expects($this->once()) + ->method('add') + ->with('where', '1 = 0'); + $qb->expects($this->never())->method('andWhere'); + $qb->expects($this->never())->method('setParameters'); + + $filter->apply($qb); + } + + public function testApplyUsesRegexExpressionAndRawParameterWhenRegexEnabled(): void + { + $filter = (new PartSearchFilter('foo.*bar', $this->makeSearchSettings())) + ->setRegex(true); + + $expr = $this->createStub(Expr::class); + + $qb = $this->createMock(QueryBuilder::class); + $qb->method('expr')->willReturn($expr); + + $qb->expects($this->never())->method('setParameter'); + + // In PR #1406 the filter uses setParameters(ArrayCollection) instead of setParameter() in regex mode. + $qb->expects($this->once()) + ->method('setParameters') + ->with($this->callback(function ($params): bool { + $this->assertInstanceOf(\Doctrine\Common\Collections\ArrayCollection::class, $params); + + /** @var \Doctrine\ORM\Query\Parameter|null $p */ + $p = $params->get(0); + $this->assertNotNull($p); + $this->assertSame('search_query', $p->getName()); + $this->assertSame('foo.*bar', $p->getValue()); + + return true; + })); + + // We don't assert the exact expression object (Doctrine internals), only that a WHERE is added. + $qb->expects($this->once())->method('andWhere'); + + $filter->apply($qb); + } + + public function testApplyEscapesSqlWildcardsAndWrapsLikeParameterWhenRegexDisabled(): void + { + $filter = (new PartSearchFilter('10%_off', $this->makeSearchSettings(escapeSQLWildcards: true))) + ->setRegex(false); + + $expr = $this->createMock(Expr::class); + $expr->method('orX')->willReturn(new Orx()); + + $qb = $this->createMock(QueryBuilder::class); + $qb->method('expr')->willReturn($expr); + + $qb->expects($this->never())->method('setParameter'); + + $qb->expects($this->once()) + ->method('setParameters') + ->with($this->callback(function ($params): bool { + $this->assertInstanceOf(ArrayCollection::class, $params); + + /** @var Parameter|null $p */ + $p = $params->get(0); + $this->assertNotNull($p); + $this->assertSame('search_query0', $p->getName()); + $this->assertSame('%10\\%\\_off%', $p->getValue()); + + return true; + })); + + $qb->expects($this->once()) + ->method('andWhere') + ->with($this->isInstanceOf(Orx::class)); + + $filter->apply($qb); + } + + public function testApplyAddsExactIdExpressionWhenDbIdSearchEnabledAndKeywordNumeric(): void + { + $filter = (new PartSearchFilter('123', $this->makeSearchSettings())) + ->setDbId(true); + + $expr = $this->createMock(Expr::class); + $expr->expects($this->once()) + ->method('eq') + ->with('part.id', ':id_exact') + ->willReturn(new Comparison('part.id', '=', ':id_exact')); + + $expr->method('orX')->willReturn(new Orx()); + + $qb = $this->createMock(QueryBuilder::class); + $qb->method('expr')->willReturn($expr); + + $qb->expects($this->never())->method('setParameter'); + + // New structure: LIKE token search is added via andWhere(...), exact ID match via orWhere(...), + // and all parameters are passed in one consolidated setParameters() call. + $qb->expects($this->once()) + ->method('setParameters') + ->with($this->callback(function ($params): bool { + $this->assertInstanceOf(ArrayCollection::class, $params); + + /** @var Parameter|null $p0 */ + $p0 = $params->get(0); + $this->assertNotNull($p0); + $this->assertSame('search_query0', $p0->getName()); + $this->assertSame('%123%', $p0->getValue()); + + /** @var Parameter|null $p1 */ + $p1 = $params->get(1); + $this->assertNotNull($p1); + $this->assertSame('id_exact', $p1->getName()); + $this->assertSame(123, $p1->getValue()); + $this->assertSame(ParameterType::INTEGER, $p1->getType()); + + return true; + })); + + $qb->expects($this->once()) + ->method('andWhere') + ->with($this->isInstanceOf(Orx::class)); + + $qb->expects($this->once()) + ->method('orWhere') + ->with($this->isInstanceOf(Comparison::class)); + + $filter->apply($qb); + } + + public function testApplyDoesNotAddExactIdExpressionWhenKeywordNotNumeric(): void + { + $filter = (new PartSearchFilter('123abc', $this->makeSearchSettings())) + ->setDbId(true); + + $expr = $this->createMock(Expr::class); + $expr->expects($this->never())->method('eq'); + $expr->method('orX')->willReturn(new Orx()); + + $qb = $this->createMock(QueryBuilder::class); + $qb->method('expr')->willReturn($expr); + + $qb->expects($this->never())->method('setParameter'); + + $qb->expects($this->once()) + ->method('setParameters') + ->with($this->callback(function ($params): bool { + $this->assertInstanceOf(ArrayCollection::class, $params); + + /** @var Parameter|null $p */ + $p = $params->get(0); + $this->assertNotNull($p); + $this->assertSame('search_query0', $p->getName()); + $this->assertSame('%123abc%', $p->getValue()); + + return true; + })); + + $qb->expects($this->once())->method('andWhere'); + + $filter->apply($qb); + } +}