diff --git a/CHANGELOG.md b/CHANGELOG.md index 58286bf..fc623cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`fetch()` no longer emits a PHP warning for named-argument forms** (#205) + - `fetch(pks: ['doc1'])` previously triggered `Undefined array key 0` before the validation error — `is_array($args[0])` is now guarded with `isset()`. + - The named-argument form `pks:` is now rejected with a clear `ZVecException` in all forms, including next to a positional array (`fetch(['doc1'], pks: [...])` previously silently ignored the named argument). + - Added regression test `test_fetch_validation.phpt`. + +- **`fetch()` rejects mixing scalar PKs with an outputFields array** (#206) + - `fetch('doc1', ['name'])` previously failed with the confusing `PKs must be non-empty strings`; it now throws with a hint to use `fetch(['doc1'], ['name'])` instead (DX part of #206; accepting the form outright is left as a separate enhancement). + - Covered in `test_fetch_validation.phpt`. + +- **`fetch()` hardening: string-keyed arrays and unknown named arguments** (review of #205) + - String-keyed arrays (`fetch(['a' => 'pk1', 'b' => 'pk2'])`) previously corrupted memory and segfaulted — `toCStringArray()` now reindexes with `array_values()` and rejects non-string elements (protects all FFI string-array call sites). + - Unknown named arguments (`fetch('pk1', foo: 'bar')`) previously segfaulted the same way; they now throw with a hint listing supported named arguments. + - `outputFields:` is now accepted as a named argument in both forms (`fetch('pk1', outputFields: ['name'])`, `fetch(['pk1'], outputFields: ['name'])`) instead of being silently ignored. + - Note: `fetch()` result order is not guaranteed by the engine, even for positional PK arrays. + - **`queryVector()` honors radius/linear/refiner set before `set*Params()`** (#197) - Calling `setRadius()`, `setLinear()`, or `setUsingRefiner()` on a `ZVecVectorQuery` *before* any `set*Params()` method previously dropped those settings silently (the param setters replaced `query_params_` wholesale) — queries ran with wrong settings (e.g. radius 0.0) and no error. - FFI: `merge_stored_query_settings()` now applies stored radius/linear/refiner onto the params object right after each param setter (`set_hnsw_ef`, `set_hnsw_rabitq_ef`, `set_vamana_ef_search`, `set_ivf_nprobe`, `set_flat_mode`), so setter order no longer matters for HNSW, IVF, Vamana, RaBitQ and Flat. diff --git a/README.md b/README.md index 0f10e0e..4b2da6d 100644 --- a/README.md +++ b/README.md @@ -237,6 +237,7 @@ $collection->updateBatch(ZVecDoc ...$docs): array // Returns per-doc status $collection->delete(string ...$pks): void $collection->deleteByFilter(string $filter): void $collection->fetch(string ...$pks): ZVecDoc[] // also fetch(array $pks, ?array $outputFields = null, includeVector: bool = true) + // named `outputFields:` supported; named-arg `pks:` and mixing scalar PKs with an array are rejected with a hint // Search $collection->query(string|ZVecVectorQuery $fieldName, array $queryVector = [], int $topk = 10, ...): ZVecDoc[] diff --git a/src/ZVec.php b/src/ZVec.php index e71fee6..2a5fd26 100644 --- a/src/ZVec.php +++ b/src/ZVec.php @@ -121,7 +121,10 @@ public static function toCStringArray(FFI $ffi, array $strings): array return [null, 0, $cStrings]; } $arr = $ffi->new("char*[$count]", false); - foreach ($strings as $i => $s) { + foreach (array_values($strings) as $i => $s) { + if (!is_string($s)) { + throw new ZVecException('String arrays must contain only strings'); + } $len = strlen($s) + 1; $cStr = $ffi->new("char[$len]", false); FFI::memcpy($cStr, $s, strlen($s)); @@ -757,15 +760,29 @@ public function fetch(array|string|bool ...$args): array if (empty($args)) { throw new ZVecException('At least one PK is required'); } - if (is_array($args[0])) { + if (array_key_exists('pks', $args)) { + throw new ZVecException('Named argument pks: is not supported — use fetch(\'pk1\', \'pk2\') or fetch([\'pk1\', \'pk2\'])'); + } + if (isset($args[0]) && is_array($args[0])) { + if (isset($args[1]) && array_key_exists('outputFields', $args)) { + throw new ZVecException('outputFields passed both positionally and as a named argument'); + } if (count($args) > 2) { throw new ZVecException('Unexpected extra arguments: expected pks array and optional outputFields'); } - $pks = $args[0]; - $outputFields = $args[1] ?? null; + $pks = array_values($args[0]); + $outputFields = $args[1] ?? $args['outputFields'] ?? null; } else { - $pks = $args; - $outputFields = null; + $outputFields = $args['outputFields'] ?? null; + $pks = array_diff_key($args, ['outputFields' => true]); + foreach ($pks as $key => $pk) { + if (!is_int($key)) { + throw new ZVecException(sprintf('Unknown named argument $%s — supported named arguments: outputFields, includeVector', $key)); + } + } + if (in_array(true, array_map(is_array(...), $pks), true)) { + throw new ZVecException('Mixing scalar PKs with an outputFields array is not supported — use fetch([\'pk1\', \'pk2\'], [\'name\']) instead'); + } } if (empty($pks)) { throw new ZVecException('At least one PK is required'); diff --git a/tests/test_fetch_validation.phpt b/tests/test_fetch_validation.phpt new file mode 100644 index 0000000..ca5be26 --- /dev/null +++ b/tests/test_fetch_validation.phpt @@ -0,0 +1,134 @@ +--TEST-- +fetch() validation: named-arg pks: rejected without PHP warning (#205), mixing scalar PK with outputFields array rejected with hint, named args cannot bypass validation, string-keyed arrays reindexed +--SKIPIF-- + +--FILE-- +addInt64('id', nullable: false, withInvertIndex: true) + ->addString('name', nullable: true) + ->addVectorFp32('v', dimension: 4, metricType: ZVecSchema::METRIC_IP); + + $c = ZVec::create($path, $schema); + $c->createHnswIndex('v', metricType: ZVecSchema::METRIC_IP, m: 16, efConstruction: 200); + + for ($i = 1; $i <= 3; $i++) { + $doc = new ZVecDoc("doc$i"); + $doc->setInt64('id', $i)->setString('name', "User$i")->setVectorFp32('v', [1.0 * $i, 0.0, 0.0, 0.0]); + $c->insert($doc); + } + $c->flush(); + + $case = static function (string $label, callable $fn) use (&$warnings): void { + $warnings = []; + try { + $fn(); + echo "$label: NO EXCEPTION\n"; + } catch (ZVecException $e) { + if (str_contains($e->getMessage(), 'Named argument pks')) { + echo "$label: ZVecException (named-arg hint)\n"; + } elseif (str_contains($e->getMessage(), 'Mixing scalar PKs')) { + echo "$label: ZVecException (mixing hint)\n"; + } elseif (str_contains($e->getMessage(), 'Unknown named argument')) { + echo "$label: ZVecException (unknown named arg)\n"; + } else { + echo "$label: ZVecException (other: {$e->getMessage()})\n"; + } + } + echo $warnings === [] ? "$label: no PHP warning\n" : "$label: UNEXPECTED WARNING: " . implode(' | ', $warnings) . "\n"; + }; + + // #205: named-arg pks: must throw a clear ZVecException, not warn + $case('fetch(pks: [doc1])', fn() => $c->fetch(pks: ['doc1'])); + // hoisted guard: pks: cannot bypass validation next to a positional array + $case('fetch([doc1], pks: [x])', fn() => $c->fetch(['doc1'], pks: ['nonexistent'])); + $case('fetch(pks: [doc1], outputFields: [name])', fn() => $c->fetch(pks: ['doc1'], outputFields: ['name'])); + + // #206 (DX part): scalar PK followed by an outputFields array + $case('fetch(doc1, [name])', fn() => $c->fetch('doc1', ['name'])); + + // unknown named argument in variadic form + $case('fetch(doc1, foo: bar)', fn() => $c->fetch('doc1', foo: 'bar')); + + // outputFields must still be validated as an array of non-empty strings + $case('fetch([doc1], name)', fn() => $c->fetch(['doc1'], 'name')); + + // valid forms + if (count($c->fetch('doc1')) !== 1) { + throw new RuntimeException('variadic form should work'); + } + if (count($c->fetch(['doc1'], ['name'])) !== 1) { + throw new RuntimeException('array + outputFields form should work'); + } + echo "valid forms OK\n"; + + // string-keyed arrays are reindexed (previously: native segfault); + // result order is not guaranteed by the engine, so compare as sets + $fetched = $c->fetch(['a' => 'doc1', 'b' => 'doc2']); + $pks = array_map(fn($d) => $d->getPk(), $fetched); + sort($pks); + if (count($fetched) !== 2 || $pks !== ['doc1', 'doc2']) { + throw new RuntimeException('string-keyed PK array should fetch doc1 and doc2'); + } + echo "string-keyed PK array OK\n"; + + $fetched = $c->fetch(['doc1', 'doc2'], ['a' => 'name', 'b' => 'id']); + $byPk = []; + foreach ($fetched as $d) { + $byPk[$d->getPk()] = $d; + } + if (count($fetched) !== 2 || ($byPk['doc1'] ?? null)?->getString('name') !== 'User1' || ($byPk['doc1'] ?? null)?->getInt64('id') !== 1) { + throw new RuntimeException('string-keyed outputFields should be applied'); + } + echo "string-keyed outputFields OK\n"; + + // named outputFields argument (scalar and array PK forms) + $fetched = $c->fetch('doc1', outputFields: ['name']); + if (count($fetched) !== 1 || $fetched[0]->getString('name') !== 'User1' || $fetched[0]->getInt64('id') !== null) { + throw new RuntimeException('scalar PK + named outputFields should return only name'); + } + $fetched = $c->fetch(['doc1'], outputFields: ['name']); + if (count($fetched) !== 1 || $fetched[0]->getString('name') !== 'User1' || $fetched[0]->getInt64('id') !== null) { + throw new RuntimeException('array PK + named outputFields should return only name'); + } + echo "named outputFields OK\n"; +} finally { + if (isset($c)) { + $c->destroy(); + } + exec('rm -rf ' . escapeshellarg($path)); +} +?> +--EXPECT-- +fetch(pks: [doc1]): ZVecException (named-arg hint) +fetch(pks: [doc1]): no PHP warning +fetch([doc1], pks: [x]): ZVecException (named-arg hint) +fetch([doc1], pks: [x]): no PHP warning +fetch(pks: [doc1], outputFields: [name]): ZVecException (named-arg hint) +fetch(pks: [doc1], outputFields: [name]): no PHP warning +fetch(doc1, [name]): ZVecException (mixing hint) +fetch(doc1, [name]): no PHP warning +fetch(doc1, foo: bar): ZVecException (unknown named arg) +fetch(doc1, foo: bar): no PHP warning +fetch([doc1], name): ZVecException (other: outputFields must be an array of non-empty strings) +fetch([doc1], name): no PHP warning +valid forms OK +string-keyed PK array OK +string-keyed outputFields OK +named outputFields OK