From 01fbe8929517c36a13176e7698ee94cfc294de57 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sat, 17 Oct 2020 08:53:32 +0200 Subject: [PATCH] Better catch for long queries, #3903 --- .../Controllers/Rule/CreateController.php | 3 -- app/Http/Controllers/Rule/IndexController.php | 4 +-- app/Http/Controllers/SearchController.php | 18 ++++++---- app/Support/Search/OperatorQuerySearch.php | 24 +++++++------ .../Engine/SearchRuleEngine.php | 36 ++++++++----------- config/firefly.php | 1 + resources/lang/en_US/firefly.php | 1 + resources/views/v1/search/index.twig | 7 +++- 8 files changed, 47 insertions(+), 47 deletions(-) diff --git a/app/Http/Controllers/Rule/CreateController.php b/app/Http/Controllers/Rule/CreateController.php index 744198e36a..f12449f0f0 100644 --- a/app/Http/Controllers/Rule/CreateController.php +++ b/app/Http/Controllers/Rule/CreateController.php @@ -32,15 +32,12 @@ use FireflyIII\Models\TransactionJournal; use FireflyIII\Repositories\Rule\RuleRepositoryInterface; use FireflyIII\Support\Http\Controllers\ModelInformation; use FireflyIII\Support\Http\Controllers\RuleManagement; -use FireflyIII\Support\Search\OperatorQuerySearch; use FireflyIII\Support\Search\SearchInterface; use Illuminate\Contracts\View\Factory; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Routing\Redirector; use Illuminate\View\View; -use Log; -use Throwable; /** * Class CreateController diff --git a/app/Http/Controllers/Rule/IndexController.php b/app/Http/Controllers/Rule/IndexController.php index 6f2e020184..b068004aeb 100644 --- a/app/Http/Controllers/Rule/IndexController.php +++ b/app/Http/Controllers/Rule/IndexController.php @@ -25,11 +25,9 @@ namespace FireflyIII\Http\Controllers\Rule; use FireflyIII\Http\Controllers\Controller; use FireflyIII\Models\Rule; use FireflyIII\Models\RuleGroup; -use FireflyIII\Models\RuleTrigger; use FireflyIII\Repositories\Rule\RuleRepositoryInterface; use FireflyIII\Repositories\RuleGroup\RuleGroupRepositoryInterface; use FireflyIII\Support\Http\Controllers\RuleManagement; -use FireflyIII\Support\Search\OperatorQuerySearch; use FireflyIII\User; use Illuminate\Contracts\View\Factory; use Illuminate\Http\JsonResponse; @@ -91,7 +89,7 @@ class IndexController extends Controller */ public function search(Rule $rule): RedirectResponse { - $route = route('search.index'); + $route = route('search.index'); $query = $this->ruleRepos->getSearchQuery($rule); $route = sprintf('%s?%s', $route, http_build_query(['search' => $query, 'rule' => $rule->id])); diff --git a/app/Http/Controllers/SearchController.php b/app/Http/Controllers/SearchController.php index 8e180c7290..55ce4a8148 100644 --- a/app/Http/Controllers/SearchController.php +++ b/app/Http/Controllers/SearchController.php @@ -64,11 +64,12 @@ class SearchController extends Controller public function index(Request $request, SearchInterface $searcher) { // search params: - $fullQuery = (string) $request->get('search'); - $page = 0 === (int) $request->get('page') ? 1 : (int) $request->get('page'); - $ruleId = (int) $request->get('rule'); - $rule = null; - $ruleChanged = false; + $fullQuery = (string) $request->get('search'); + $page = 0 === (int) $request->get('page') ? 1 : (int) $request->get('page'); + $ruleId = (int) $request->get('rule'); + $rule = null; + $ruleChanged = false; + $longQueryWarning = false; // find rule, check if query is different, offer to update. $ruleRepository = app(RuleRepositoryInterface::class); @@ -79,7 +80,9 @@ class SearchController extends Controller $ruleChanged = true; } } - + if (strlen($fullQuery) > 250) { + $longQueryWarning = true; + } // parse search terms: $searcher->parseQuery($fullQuery); @@ -89,7 +92,7 @@ class SearchController extends Controller $subTitle = (string) trans('breadcrumbs.search_result', ['query' => $fullQuery]); - return view('search.index', compact('query', 'operators', 'page', 'rule', 'fullQuery', 'subTitle', 'ruleId', 'ruleChanged')); + return view('search.index', compact('query', 'longQueryWarning', 'operators', 'page', 'rule', 'fullQuery', 'subTitle', 'ruleId', 'ruleChanged')); } /** @@ -106,6 +109,7 @@ class SearchController extends Controller $page = 0 === (int) $request->get('page') ? 1 : (int) $request->get('page'); $searcher->parseQuery($fullQuery); + $searcher->setPage($page); $groups = $searcher->searchTransactions(); $hasPages = $groups->hasPages(); diff --git a/app/Support/Search/OperatorQuerySearch.php b/app/Support/Search/OperatorQuerySearch.php index 744f8e05dd..1f6ee522b0 100644 --- a/app/Support/Search/OperatorQuerySearch.php +++ b/app/Support/Search/OperatorQuerySearch.php @@ -77,6 +77,7 @@ class OperatorQuerySearch implements SearchInterface private float $startTime; private Collection $modifiers; // obsolete private Collection $operators; + private string $originalQuery; /** * OperatorQuerySearch constructor. @@ -90,6 +91,7 @@ class OperatorQuerySearch implements SearchInterface $this->page = 1; $this->words = []; $this->limit = 25; + $this->originalQuery = ''; $this->validOperators = array_keys(config('firefly.search.operators')); $this->startTime = microtime(true); $this->accountRepository = app(AccountRepositoryInterface::class); @@ -143,6 +145,7 @@ class OperatorQuerySearch implements SearchInterface public function setPage(int $page): void { $this->page = $page; + $this->collector->setPage($this->page); } /** @@ -161,22 +164,16 @@ class OperatorQuerySearch implements SearchInterface public function parseQuery(string $query) { Log::debug(sprintf('Now in parseQuery(%s)', $query)); - $parser = new QueryParser(); - $this->query = $parser->parse($query); - - $this->collector = app(GroupCollectorInterface::class); - $this->collector->setUser($this->user); - $this->collector->setLimit($this->limit)->setPage($this->page); - $this->collector->withAccountInformation()->withCategoryInformation()->withBudgetInformation(); + $parser = new QueryParser(); + $this->query = $parser->parse($query); + $this->originalQuery = $query; Log::debug(sprintf('Found %d node(s)', count($this->query->getNodes()))); - foreach ($this->query->getNodes() as $searchNode) { $this->handleSearchNode($searchNode); } $this->collector->setSearchWords($this->words); - } /** @@ -211,8 +208,12 @@ class OperatorQuerySearch implements SearchInterface $this->billRepository->setUser($user); $this->categoryRepository->setUser($user); $this->budgetRepository->setUser($user); + $this->collector = app(GroupCollectorInterface::class); + $this->collector->setUser($this->user); + $this->collector->withAccountInformation()->withCategoryInformation()->withBudgetInformation(); $this->setLimit((int) app('preferences')->getForUser($user, 'listPageSize', 50)->data); + } /** @@ -268,7 +269,7 @@ class OperatorQuerySearch implements SearchInterface */ private function updateCollector(string $operator, string $value): bool { - Log::debug(sprintf('updateCollector(%s, %s)', $operator, $value)); + Log::debug(sprintf('updateCollector("%s", "%s")', $operator, $value)); // check if alias, replace if necessary: $operator = self::getRootOperator($operator); @@ -579,7 +580,7 @@ class OperatorQuerySearch implements SearchInterface */ private function searchAccount(string $value, int $searchDirection, int $stringPosition): void { - Log::debug(sprintf('searchAccount(%s, %d, %d)', $value, $stringPosition, $searchDirection)); + Log::debug(sprintf('searchAccount("%s", %d, %d)', $value, $stringPosition, $searchDirection)); // search direction (default): for source accounts $searchTypes = [AccountType::ASSET, AccountType::MORTGAGE, AccountType::LOAN, AccountType::DEBT, AccountType::REVENUE]; @@ -767,6 +768,7 @@ class OperatorQuerySearch implements SearchInterface public function setLimit(int $limit): void { $this->limit = $limit; + $this->collector->setLimit($this->limit); } /** diff --git a/app/TransactionRules/Engine/SearchRuleEngine.php b/app/TransactionRules/Engine/SearchRuleEngine.php index e3d023a109..22cd561695 100644 --- a/app/TransactionRules/Engine/SearchRuleEngine.php +++ b/app/TransactionRules/Engine/SearchRuleEngine.php @@ -234,11 +234,11 @@ class SearchRuleEngine implements RuleEngineInterface foreach ($rule->ruleTriggers as $ruleTrigger) { // if needs no context, value is different: $needsContext = config(sprintf('firefly.search.operators.%s.needs_context', $ruleTrigger->trigger_type)) ?? true; - if(false === $needsContext) { + if (false === $needsContext) { Log::debug(sprintf('SearchRuleEngine:: add a rule trigger: %s:true', $ruleTrigger->trigger_type)); $searchArray[$ruleTrigger->trigger_type] = 'true'; } - if(true === $needsContext) { + if (true === $needsContext) { Log::debug(sprintf('SearchRuleEngine:: add a rule trigger: %s:"%s"', $ruleTrigger->trigger_type, $ruleTrigger->trigger_value)); $searchArray[$ruleTrigger->trigger_type] = sprintf('"%s"', $ruleTrigger->trigger_value); } @@ -249,20 +249,17 @@ class SearchRuleEngine implements RuleEngineInterface Log::debug(sprintf('SearchRuleEngine:: add local added operator: %s:"%s"', $operator['type'], $operator['value'])); $searchArray[$operator['type']] = sprintf('"%s"', $operator['value']); } - $toJoin = []; - foreach ($searchArray as $type => $value) { - $toJoin[] = sprintf('%s:%s', $type, $value); - } - - $searchQuery = join(' ', $toJoin); - Log::debug(sprintf('SearchRuleEngine:: Search strict query for rule #%d ("%s") = %s', $rule->id, $rule->title, $searchQuery)); // build and run the search engine. $searchEngine = app(SearchInterface::class); $searchEngine->setUser($this->user); $searchEngine->setPage(1); $searchEngine->setLimit(31337); - $searchEngine->parseQuery($searchQuery); + + foreach ($searchArray as $type => $value) { + $searchEngine->parseQuery(sprintf('%s:%s', $type, $value)); + } + $result = $searchEngine->searchTransactions(); $collection = $result->getCollection(); @@ -286,13 +283,13 @@ class SearchRuleEngine implements RuleEngineInterface Log::debug('Skip trigger type.'); continue; } - $searchArray = []; + $searchArray = []; $needsContext = config(sprintf('firefly.search.operators.%s.needs_context', $ruleTrigger->trigger_type)) ?? true; - if(false === $needsContext) { + if (false === $needsContext) { Log::debug(sprintf('SearchRuleEngine:: non strict, will search for: %s:true', $ruleTrigger->trigger_type)); $searchArray[$ruleTrigger->trigger_type] = 'true'; } - if(true === $needsContext) { + if (true === $needsContext) { Log::debug(sprintf('SearchRuleEngine:: non strict, will search for: %s:"%s"', $ruleTrigger->trigger_type, $ruleTrigger->trigger_value)); $searchArray[$ruleTrigger->trigger_type] = sprintf('"%s"', $ruleTrigger->trigger_value); } @@ -302,21 +299,16 @@ class SearchRuleEngine implements RuleEngineInterface Log::debug(sprintf('SearchRuleEngine:: add local added operator: %s:"%s"', $operator['type'], $operator['value'])); $searchArray[$operator['type']] = sprintf('"%s"', $operator['value']); } - $toJoin = []; - foreach ($searchArray as $type => $value) { - $toJoin[] = sprintf('%s:%s', $type, $value); - } - $searchQuery = join(' ', $toJoin); - Log::debug(sprintf('SearchRuleEngine:: Search strict query for non-strict rule #%d ("%s") = %s', $rule->id, $rule->title, $searchQuery)); - - // build and run a search: // build and run the search engine. $searchEngine = app(SearchInterface::class); $searchEngine->setUser($this->user); $searchEngine->setPage(1); $searchEngine->setLimit(31337); - $searchEngine->parseQuery($searchQuery); + + foreach ($searchArray as $type => $value) { + $searchEngine->parseQuery(sprintf('%s:%s', $type, $value)); + } $result = $searchEngine->searchTransactions(); $collection = $result->getCollection(); diff --git a/config/firefly.php b/config/firefly.php index 80b8d26de4..41b46001ca 100644 --- a/config/firefly.php +++ b/config/firefly.php @@ -459,6 +459,7 @@ return [ 'description_ends' => ['alias' => false, 'needs_context' => true,], 'description_contains' => ['alias' => false, 'needs_context' => true,], 'description_is' => ['alias' => false, 'needs_context' => true,], + 'description' => ['alias' => true, 'alias_for' => 'description_contains', 'needs_context' => true,], 'currency_is' => ['alias' => false, 'needs_context' => true,], 'foreign_currency_is' => ['alias' => false, 'needs_context' => true,], diff --git a/resources/lang/en_US/firefly.php b/resources/lang/en_US/firefly.php index 115c5dc60b..830e4a6b68 100644 --- a/resources/lang/en_US/firefly.php +++ b/resources/lang/en_US/firefly.php @@ -260,6 +260,7 @@ return [ // search 'search' => 'Search', + 'long_query_warning' => 'Your search query is very long, and may not work as expected.', 'search_query' => 'Query', 'search_found_transactions' => 'Firefly III found :count transaction in :time seconds.|Firefly III found :count transactions in :time seconds.', 'search_found_more_transactions' => 'Firefly III found more than :count transactions in :time seconds.', diff --git a/resources/views/v1/search/index.twig b/resources/views/v1/search/index.twig index 3448d8270e..b3617e7884 100644 --- a/resources/views/v1/search/index.twig +++ b/resources/views/v1/search/index.twig @@ -20,7 +20,7 @@
-
@@ -37,6 +37,11 @@ {% endif %} + {% if longQueryWarning %} +

+ {{ 'long_query_warning'|_ }} +

+ {% endif %} {% if '' != query %}

{{ trans('firefly.search_for_query', {query: query|escape})|raw}}