Phorge full-text search extension for Elasticsearch 7.x / 8.x / OpenSearch via the typeless API (mirror of https://github.com/vyos/phorge-elasticsearch-modern.git) https://git.amelek.net/vyos/phorge-elasticsearch-modern.git/atom?h=production 2026-05-23T14:16:51+00:00 2026-05-23T14:16:51+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-23T14:16:51+00:00 urn:sha1:e5fbde8bb9b21d264f71f25881b44db1af3d03c6 Two correctness bugs surfaced by CR / Phase 0, both inherited from the bundled PhabricatorElasticFulltextStorageEngine pattern: 1. executeSearch() did not validate that $offset / $limit were non-negative. PHP's (int) cast on a negative string yields a negative int, which would be passed unchecked to Elasticsearch. Add an explicit non-negative check before the existing upper- bound (offset + limit > 10000) check. 2. reindexAbstractDocument()'s relationship-data loop assigned the '_ts' field unconditionally instead of accumulating. For multi- valued relationships (multiple authorPHID, projectPHID, etc. entries on the same doc), only the last timestamp survived. Change to array-append, parallel to the PHID array. Both fixes diverge from the bundled engine; bundled retains the faithful-but-broken behavior. Worth fixing in this extension since we control the wire format. 🤖 Generated by [robots](https://vyos.io) 2026-05-23T12:48:18+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-23T12:48:18+00:00 urn:sha1:011eec12f9176a6a8b8d8b9133bc00107a7855c0 A recent fixup cascade declared VyOSElasticModernFulltextStorageEngine as 'abstract class' to enable test-doubling subclasses (VyOSElasticModernHostTestEngine, VyOSElasticModernFulltextStorageEngineTestDouble). That change broke production dispatch: Phorge's PhutilClassMapQuery selects engines via PhutilSymbolLoader::loadObjects(), which calls setConcreteOnly(true) and unsets any class where reflection reports isAbstract(). The engine class would no longer be discoverable by cluster.search[].type:elasticsearch-modern, and the test-fixture subclasses live in src/__tests__/ which libphutil excludes from runtime class-map loading. Revert to plain 'class' (not 'final', not 'abstract'). Plain 'class' is discoverable by PhutilClassMapQuery; existing test fixtures keep working without rewrite; operators can subclass if they need to (harmless). Also update VyOSElasticModernHostTestCase::testPlaceholderEngineIsAbstract → testEngineIsConcreteForDispatch, inverting the assertion to document the correct invariant and its rationale (setConcreteOnly reference). All 22 tests pass. 🤖 Generated by [robots](https://vyos.io) 2026-05-23T12:35:17+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-23T12:35:17+00:00 urn:sha1:a19f5c077c0ed8ee4d88c0c8d704995b155d3dc2 - README cluster.search example uses 'phabricator/' to match host default (was '/phabricator'; both work for index name but the host uses path verbatim for the base URI). - Add VERIFICATION.md placeholder with the planned smoke-test matrix template; status notes v0.1.0 is pending IS-474. - Engine reindex: replace 'if ($time)' falsy check with 'if ($time !== null)' so a Unix epoch timestamp (0) is not silently dropped. 🤖 Generated by [robots](https://vyos.io) 2026-05-23T12:14:02+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-23T12:14:02+00:00 urn:sha1:4d27ac74aff1ca47db53510bf07e9d083635175b 1. TestDouble signatures: indexExists() and getIndexStats() in VyOSElasticModernFulltextStorageEngineTestDouble were missing the nullable host parameter added in E7/E8, causing a PHP fatal on 'arc unit --everything'. 2. buildSpec() zero-query sort bug: the falsy check (!$query->getParameter('query')) incorrectly treats the string '0' as empty (PHP falsy), enabling date-sorted path for a real search term. Changed to explicit null/empty-string checks to match the existing must-clause guard on line 316. All 22 unit tests now pass. 🤖 Generated by [robots](https://vyos.io) 2026-05-23T12:08:18+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-23T12:08:18+00:00 urn:sha1:e36bce7dee75eccf67ae4737452275a6a01d7bb9 Phase 0 CodeRabbit caught a typo — HTTPFutureHTTPResponseStatus does not exist in the libphutil type hierarchy; the correct class is HTTPFutureResponseStatus (consistent with line 257 in executeRequest()). 🤖 Generated by [robots](https://vyos.io) 2026-05-23T10:55:22+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-23T10:54:24+00:00 urn:sha1:4d836601cb2f602c0089134eda1604f605fb39da 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) 2026-05-23T10:48:42+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-22T09:00:28+00:00 urn:sha1:51ceab16c95efc5527445788cb83f0289675d0bf - 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) 2026-05-23T10:48:42+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-22T08:52:43+00:00 urn:sha1:58ab9f6cacf8735adec2413e7404b34fc129f8a5 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) 2026-05-23T10:47:45+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-23T10:47:45+00:00 urn:sha1:3a4580e35a0c7a6480aeee9ec4acf90d0fa85326 Previously the success health-check was called inside the JSON-decode try block. This was wrong: a valid HTTP 200 response with non-JSON body (e.g. ES maintenance page) would mark the host as *unhealthy* because JSON parse failure triggered didHealthCheck(false). The host is demonstrably reachable and responding; only the application layer failed. Move didHealthCheck(true) to immediately after list($body) = resolvex() succeeds, before the GET/non-GET branch. Remove the now-unnecessary didHealthCheck(false) from the JSON catch block — only network/timeout failures belong in the health-check false path. 🤖 Generated by [robots](https://vyos.io) 2026-05-23T10:44:49+00:00 Yuriy Andamasov yuriy@vyos.io 2026-05-22T08:48:30+00:00 urn:sha1:e0eb6dd6b21c129c775d8bb2bec203ae0d756d0f reindexAbstractDocument builds the document body with documentType as a real field (replacing the bundled engine's per-type URL segment), then PUTs to the typeless /_doc/{phid} URL via the new helper. executeRequest mirrors the bundled engine's HTTPSFuture pattern; it's private and not test-targeted directly (smoke tests in V1-V3 exercise the full wire path). 🤖 Generated by [robots](https://vyos.io)