From 58ab9f6cacf8735adec2413e7404b34fc129f8a5 Mon Sep 17 00:00:00 2001 From: Yuriy Andamasov Date: Fri, 22 May 2026 11:52:43 +0300 Subject: Engine: Wire executeSearch, index lifecycle, buildSpec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full public surface lands. executeSearch normalizes \$types before both URL building and body filter construction. buildSpec uses the bundled PhabricatorElasticsearchQueryBuilder (final, by composition) and emits bool.must_not.ids for the exclude parameter from day one. buildSpec also injects the body-level documentType filter via buildTypeFilter. indexExists/indexIsSane/initIndex/getIndexStats mirror the bundled engine but without the version-branching dead code. The private check() and normalizeConfigValue() helpers are copied verbatim from the bundled engine (no behavior difference). catch (Exception) widened to catch (Throwable) per XHP132. normalizeConfigValue() one-liners braced per XHP24. Lint clean. 🤖 Generated by [robots](https://vyos.io) --- .../VyOSElasticModernFulltextStorageEngine.php | 291 ++++++++++++++++++++- 1 file changed, 283 insertions(+), 8 deletions(-) (limited to 'src/engine/VyOSElasticModernFulltextStorageEngine.php') diff --git a/src/engine/VyOSElasticModernFulltextStorageEngine.php b/src/engine/VyOSElasticModernFulltextStorageEngine.php index e036082..199f2c8 100644 --- a/src/engine/VyOSElasticModernFulltextStorageEngine.php +++ b/src/engine/VyOSElasticModernFulltextStorageEngine.php @@ -281,18 +281,293 @@ abstract class VyOSElasticModernFulltextStorageEngine } public function executeSearch(PhabricatorSavedQuery $query) { - throw new Exception(pht( - 'executeSearch() is not yet implemented; lands in Task E8.')); + $types = $query->getParameter('types'); + if (!$types) { + $types = array_keys( + PhabricatorSearchApplicationSearchEngine::getIndexableDocumentTypes()); + } + + $uri = $this->getSearchUri($types); + $spec = $this->buildSpec($query, $types); + + $exceptions = array(); + foreach ($this->service->getAllHostsForRole('read') as $host) { + try { + $response = $this->executeRequest($host, $uri, $spec); + $phids = ipull($response['hits']['hits'], '_id'); + return $phids; + } catch (Throwable $e) { + $exceptions[] = $e; + } + } + throw new PhutilAggregateException( + pht('All Fulltext Search hosts failed:'), + $exceptions); } - public function indexExists() { - throw new Exception(pht( - 'indexExists() is not yet implemented; lands in Task E8.')); + private function buildSpec(PhabricatorSavedQuery $query, array $types) { + $q = new PhabricatorElasticsearchQueryBuilder(); + $query_string = $query->getParameter('query'); + if (strlen($query_string)) { + $fields = $this->getTypeConstants('PhabricatorSearchDocumentFieldType'); + $q->addMustClause(array( + 'simple_query_string' => array( + 'query' => $query_string, + 'fields' => array( + PhabricatorSearchDocumentFieldType::FIELD_TITLE.'.*', + PhabricatorSearchDocumentFieldType::FIELD_BODY.'.*', + PhabricatorSearchDocumentFieldType::FIELD_COMMENT.'.*', + ), + 'default_operator' => 'AND', + ), + )); + $q->addShouldClause(array( + 'simple_query_string' => array( + 'query' => $query_string, + 'fields' => array( + '*.raw', + PhabricatorSearchDocumentFieldType::FIELD_TITLE.'^4', + PhabricatorSearchDocumentFieldType::FIELD_BODY.'^3', + PhabricatorSearchDocumentFieldType::FIELD_COMMENT.'^1.2', + ), + 'analyzer' => 'english_exact', + 'default_operator' => 'and', + ), + )); + } + + $exclude = $query->getParameter('exclude'); + if ($exclude) { + // Correct from day one: bool.must_not, not the obsolete 'not' clause. + $q->addMustNotClause(array( + 'ids' => array( + 'values' => array($exclude), + ), + )); + } + + $relationship_map = array( + PhabricatorSearchRelationship::RELATIONSHIP_AUTHOR => + $query->getParameter('authorPHIDs', array()), + PhabricatorSearchRelationship::RELATIONSHIP_SUBSCRIBER => + $query->getParameter('subscriberPHIDs', array()), + PhabricatorSearchRelationship::RELATIONSHIP_PROJECT => + $query->getParameter('projectPHIDs', array()), + PhabricatorSearchRelationship::RELATIONSHIP_REPOSITORY => + $query->getParameter('repositoryPHIDs', array()), + ); + + $statuses = $query->getParameter('statuses', array()); + $statuses = array_fuse($statuses); + $rel_open = PhabricatorSearchRelationship::RELATIONSHIP_OPEN; + $rel_closed = PhabricatorSearchRelationship::RELATIONSHIP_CLOSED; + $rel_unowned = PhabricatorSearchRelationship::RELATIONSHIP_UNOWNED; + $include_open = !empty($statuses[$rel_open]); + $include_closed = !empty($statuses[$rel_closed]); + if ($include_open && !$include_closed) { + $q->addExistsClause($rel_open); + } else if (!$include_open && $include_closed) { + $q->addExistsClause($rel_closed); + } + + if ($query->getParameter('withUnowned')) { + $q->addExistsClause($rel_unowned); + } + + $rel_owner = PhabricatorSearchRelationship::RELATIONSHIP_OWNER; + if ($query->getParameter('withAnyOwner')) { + $q->addExistsClause($rel_owner); + } else { + $owner_phids = $query->getParameter('ownerPHIDs', array()); + if (count($owner_phids)) { + $q->addTermsClause($rel_owner, $owner_phids); + } + } + + foreach ($relationship_map as $field => $phids) { + if (is_array($phids) && !empty($phids)) { + $q->addTermsClause($field, $phids); + } + } + + // Body-level type filter (typeless API has no per-type URL segment). + $q->addFilterClause($this->buildTypeFilter($types)); + + if (!$q->getClauseCount('must')) { + $q->addMustClause(array('match_all' => array('boost' => 1))); + } + + $spec = array( + '_source' => false, + 'query' => array( + 'bool' => $q->toArray(), + ), + ); + + if (!$query->getParameter('query')) { + $spec['sort'] = array( + array('dateCreated' => 'desc'), + ); + } + + $offset = (int)$query->getParameter('offset', 0); + $limit = (int)$query->getParameter('limit', 101); + if ($offset + $limit > 10000) { + throw new Exception(pht( + 'Query offset is too large. offset+limit=%s (max=%s)', + $offset + $limit, 10000)); + } + $spec['from'] = $offset; + $spec['size'] = $limit; + + return $spec; } - public function getIndexStats() { - throw new Exception(pht( - 'getIndexStats() is not yet implemented; lands in Task E8.')); + public function indexExists(?VyOSElasticModernHost $host = null) { + if (!$host) { + $host = $this->getHostForRead(); + } + try { + $res = $this->executeRequest($host, '/_stats/', array()); + return isset($res['indices'][$this->index]); + } catch (HTTPFutureHTTPResponseStatus $e) { + if ($e->getStatusCode() == 404) { + return false; + } + throw $e; + } + } + + public function indexIsSane(?VyOSElasticModernHost $host = null) { + if (!$host) { + $host = $this->getHostForRead(); + } + if (!$this->indexExists($host)) { + return false; + } + $cur_mapping = $this->executeRequest($host, '/_mapping/', array()); + $cur_settings = $this->executeRequest($host, '/_settings/', array()); + $actual = array_merge( + $cur_settings[$this->index], + $cur_mapping[$this->index]); + + return $this->check($actual, $this->getIndexConfiguration()); + } + + public function initIndex() { + $host = $this->getHostForWrite(); + if ($this->indexExists()) { + $this->executeRequest($host, '/', array(), 'DELETE'); + } + $data = $this->getIndexConfiguration(); + $this->executeRequest($host, '/', $data, 'PUT'); + } + + public function getIndexStats(?VyOSElasticModernHost $host = null) { + if (!$host) { + $host = $this->getHostForRead(); + } + $res = $this->executeRequest($host, '/_stats/', array()); + $stats = $res['indices'][$this->index]; + return array( + pht('Queries') => + idxv($stats, array('primaries', 'search', 'query_total')), + pht('Documents') => + idxv($stats, array('total', 'docs', 'count')), + pht('Deleted') => + idxv($stats, array('total', 'docs', 'deleted')), + pht('Storage Used') => + phutil_format_bytes( + idxv($stats, array('total', 'store', 'size_in_bytes'))), + ); + } + + private function getIndexConfiguration() { + $data = array(); + $data['settings'] = array( + 'index' => array( + 'auto_expand_replicas' => '0-2', + 'analysis' => array( + 'filter' => array( + 'english_stop' => array( + 'type' => 'stop', + 'stopwords' => '_english_', + ), + 'english_stemmer' => array( + 'type' => 'stemmer', + 'language' => 'english', + ), + 'english_possessive_stemmer' => array( + 'type' => 'stemmer', + 'language' => 'possessive_english', + ), + ), + 'analyzer' => array( + 'english_exact' => array( + 'tokenizer' => 'standard', + 'filter' => array('lowercase'), + ), + 'letter_stop' => array( + 'tokenizer' => 'letter', + 'filter' => array('lowercase', 'english_stop'), + ), + 'english_stem' => array( + 'tokenizer' => 'standard', + 'filter' => array( + 'english_possessive_stemmer', + 'lowercase', + 'english_stop', + 'english_stemmer', + ), + ), + ), + ), + ), + ); + + $fields = $this->getTypeConstants('PhabricatorSearchDocumentFieldType'); + $relationships = $this->getTypeConstants('PhabricatorSearchRelationship'); + $doc_types = array_keys( + PhabricatorSearchApplicationSearchEngine::getIndexableDocumentTypes()); + $text_type = $this->getTextFieldType(); + + $data['mappings'] = $this->buildIndexMappings( + $doc_types, $fields, $relationships, $text_type); + + return $data; + } + + private function check($actual, $required, $path = '') { + foreach ($required as $key => $value) { + if (!array_key_exists($key, $actual)) { + return false; + } + if (is_array($value)) { + if (!is_array($actual[$key])) { + return false; + } + if (!$this->check($actual[$key], $value, $path.'.'.$key)) { + return false; + } + continue; + } + $actual[$key] = self::normalizeConfigValue($actual[$key]); + $value = self::normalizeConfigValue($value); + if ($actual[$key] != $value) { + return false; + } + } + return true; + } + + private static function normalizeConfigValue($value) { + if ($value === true) { + return 'true'; + } + if ($value === false) { + return 'false'; + } + return $value; } } -- cgit v1.2.3 From 51ceab16c95efc5527445788cb83f0289675d0bf Mon Sep 17 00:00:00 2001 From: Yuriy Andamasov Date: Fri, 22 May 2026 12:00:28 +0300 Subject: E6-E8: Apply code-quality fixes from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove dead $fields assignment in buildSpec() (copy-paste artifact from bundled engine; never actually used). - Revert executeSearch catch from Throwable back to Exception. The earlier lint-driven widening was over-broad: Error subclasses (TypeError, etc.) should propagate immediately, not be aggregated into the per-host failover loop. Comment added explaining the intent. 🤖 Generated by [robots](https://vyos.io) --- src/engine/VyOSElasticModernFulltextStorageEngine.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'src/engine/VyOSElasticModernFulltextStorageEngine.php') diff --git a/src/engine/VyOSElasticModernFulltextStorageEngine.php b/src/engine/VyOSElasticModernFulltextStorageEngine.php index 199f2c8..c49ff5e 100644 --- a/src/engine/VyOSElasticModernFulltextStorageEngine.php +++ b/src/engine/VyOSElasticModernFulltextStorageEngine.php @@ -296,7 +296,12 @@ abstract class VyOSElasticModernFulltextStorageEngine $response = $this->executeRequest($host, $uri, $spec); $phids = ipull($response['hits']['hits'], '_id'); return $phids; - } catch (Throwable $e) { + } catch (Exception $e) { + // Catches HTTPFutureResponseStatus and other network/HTTP exceptions + // raised by executeRequest(). PHP Error subclasses (TypeError, etc.) + // are intentionally NOT caught -- they indicate programming bugs that + // should propagate immediately, not get aggregated into the per-host + // failover. $exceptions[] = $e; } } @@ -309,7 +314,6 @@ abstract class VyOSElasticModernFulltextStorageEngine $q = new PhabricatorElasticsearchQueryBuilder(); $query_string = $query->getParameter('query'); if (strlen($query_string)) { - $fields = $this->getTypeConstants('PhabricatorSearchDocumentFieldType'); $q->addMustClause(array( 'simple_query_string' => array( 'query' => $query_string, -- cgit v1.2.3 From 4d836601cb2f602c0089134eda1604f605fb39da Mon Sep 17 00:00:00 2001 From: Yuriy Andamasov Date: Sat, 23 May 2026 13:54:24 +0300 Subject: E8 fixup: four correctness fixes from Phase 0 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. strlen(\$query_string) → null-safe check \$query->getParameter('query') may return null; strlen(null) is deprecated in PHP 8.1+. Switch to explicit !== null && !== '' check. 2. ids.values nesting fix in buildSpec() 'values' => array(\$exclude) wraps an already-array \$exclude in a nested array, producing invalid Elasticsearch ids syntax. Cast to (array) then re-index with array_values() so both scalar PHID and array-of-PHIDs produce a flat list. 3. initIndex() host consistency: pass write host to indexExists() initIndex() called indexExists() without a host argument, which fell back to the read host, then deleted via getHostForWrite(). A read/write split lag window could cause false-positive "index doesn't exist" results. Pass the already-resolved write host. 4. getIndexStats() unchecked array access \$res['indices'][\$this->index] was accessed without verifying the key exists. If the index was just deleted or the name drifted, this throws an undefined-offset PHP notice. Add an explicit isset() guard with a clear exception message naming the index. 🤖 Generated by [robots](https://vyos.io) --- ...yOSElasticModernFulltextStorageEngineTestCase.php | 20 ++++++++++++++++++++ .../VyOSElasticModernFulltextStorageEngine.php | 14 +++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) (limited to 'src/engine/VyOSElasticModernFulltextStorageEngine.php') diff --git a/src/__tests__/VyOSElasticModernFulltextStorageEngineTestCase.php b/src/__tests__/VyOSElasticModernFulltextStorageEngineTestCase.php index 9f08f8c..8a8b29c 100644 --- a/src/__tests__/VyOSElasticModernFulltextStorageEngineTestCase.php +++ b/src/__tests__/VyOSElasticModernFulltextStorageEngineTestCase.php @@ -257,4 +257,24 @@ final class VyOSElasticModernFulltextStorageEngineTestCase $this->assertEqual('lastModified', $engine->getTimestampField()); } + public function testBuildSpecTreatsZeroQueryAsSearchTerm() { + // '0' is falsy in PHP but must be treated as a non-empty search term. + // strlen('0') == 1, so the query clause should be present and the + // default date-sorted path should NOT fire. + $engine = $this->newEngine()->setVersion(7); + $query = id(new PhabricatorSavedQuery()) + ->setParameter('query', '0'); + + $method = new ReflectionMethod($engine, 'buildSpec'); + $method->setAccessible(true); + $spec = $method->invoke($engine, $query, array('TASK')); + + $this->assertFalse(isset($spec['sort'])); + $this->assertEqual( + '0', + idxv( + $spec, + array('query', 'bool', 'must', 0, 'simple_query_string', 'query'))); + } + } diff --git a/src/engine/VyOSElasticModernFulltextStorageEngine.php b/src/engine/VyOSElasticModernFulltextStorageEngine.php index c49ff5e..70844f9 100644 --- a/src/engine/VyOSElasticModernFulltextStorageEngine.php +++ b/src/engine/VyOSElasticModernFulltextStorageEngine.php @@ -313,7 +313,7 @@ abstract class VyOSElasticModernFulltextStorageEngine private function buildSpec(PhabricatorSavedQuery $query, array $types) { $q = new PhabricatorElasticsearchQueryBuilder(); $query_string = $query->getParameter('query'); - if (strlen($query_string)) { + if ($query_string !== null && $query_string !== '') { $q->addMustClause(array( 'simple_query_string' => array( 'query' => $query_string, @@ -343,9 +343,11 @@ abstract class VyOSElasticModernFulltextStorageEngine $exclude = $query->getParameter('exclude'); if ($exclude) { // Correct from day one: bool.must_not, not the obsolete 'not' clause. + // Cast to array so a single PHID scalar and an already-array of PHIDs + // both produce a flat list for the Elasticsearch ids.values field. $q->addMustNotClause(array( 'ids' => array( - 'values' => array($exclude), + 'values' => array_values((array)$exclude), ), )); } @@ -460,7 +462,7 @@ abstract class VyOSElasticModernFulltextStorageEngine public function initIndex() { $host = $this->getHostForWrite(); - if ($this->indexExists()) { + if ($this->indexExists($host)) { $this->executeRequest($host, '/', array(), 'DELETE'); } $data = $this->getIndexConfiguration(); @@ -472,6 +474,12 @@ abstract class VyOSElasticModernFulltextStorageEngine $host = $this->getHostForRead(); } $res = $this->executeRequest($host, '/_stats/', array()); + if (!isset($res['indices'][$this->index])) { + throw new Exception( + pht( + 'Index "%s" not found in Elasticsearch _stats response.', + $this->index)); + } $stats = $res['indices'][$this->index]; return array( pht('Queries') => -- cgit v1.2.3