fix: fetch() validation hardening — named args, string-keyed arrays (closes #205, refs #206)

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.

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[]

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');

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--
+<?php if (!extension_loaded('ffi')) die('skip FFI extension not available'); ?>
+--FILE--
+<?php
+declare(strict_types=1);
+require_once __DIR__ . '/../src/ZVec.php';
+
+$warnings = [];
+set_error_handler(static function (int $errno, string $message) use (&$warnings): bool {
+    if ($errno === E_USER_DEPRECATED && str_contains($message, ' is deprecated, use createIndex()')) {
+        return true;
+    }
+    $warnings[] = "[$errno] $message";
+    return true;
+});
+
+ZVec::init(logType: ZVec::LOG_CONSOLE, logLevel: ZVec::LOG_WARN);
+
+$path = __DIR__ . '/../test_dbs/fetch_validation_' . uniqid();
+try {
+    $schema = new ZVecSchema('fetch_validation');
+    $schema->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