Skip to content

Fulltext invalid chars #825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fogelito wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fulltext invalid chars #825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5969d92
Fail test
fogelito Feb 26, 2026
6ad0f4f
test
fogelito Mar 4, 2026
7e9eea7
Merge branch 'main' of github.com:utopia-php/database into fulltext-i…
fogelito Mar 4, 2026
4950e5e
test
fogelito Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions phpunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
convertNoticesToExceptions="true"
convertWarningsToExceptions="true"
processIsolation="false"
stopOnFailure="false"
>
stopOnFailure="true">
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid enabling global fast-fail in committed PHPUnit config.

Setting stopOnFailure="true" in shared config cuts off CI at the first failure, which hides additional failures and weakens debugging signal for contributors.

Suggested change 
-    stopOnFailure="true">
+    stopOnFailure="false">
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
stopOnFailure="true">
stopOnFailure="false">
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@phpunit.xml` at line 10, The shared PHPUnit configuration currently enables
global fast-fail via the stopOnFailure="true" attribute, which halts CI on the
first test failure; remove or set stopOnFailure to "false" in the phpunit.xml so
the test suite runs to completion and surfaces all failures; locate the
stopOnFailure attribute in the phpunit.xml root <phpunit> element and change or
remove it, and ensure any project-level CI overrides (if desired) handle
fast-fail behavior per job rather than committing a global fast-fail.

<testsuites>
<testsuite name="unit">
<directory>./tests/unit</directory>
Expand Down
4 changes: 3 additions & 1 deletion src/Database/Adapter/SQL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3148,11 +3148,13 @@ public function find(Document $collection, array $queries = [], ?int $limit = 25
";

$sql = $this->trigger(Database::EVENT_DOCUMENT_FIND, $sql);

var_dump($sql);
try {
$stmt = $this->getPDO()->prepare($sql);

foreach ($binds as $key => $value) {
var_dump($key);
var_dump($value);
Comment on lines +3151 to +3157
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Remove unconditional debug dumps from find() execution path.

Line 3151 and Lines 3156-3157 dump raw SQL and bind values on every request. This can leak sensitive data and can break consumers expecting clean response output.

Proposed fix 
-        $sql = $this->trigger(Database::EVENT_DOCUMENT_FIND, $sql);
-var_dump($sql);
+        $sql = $this->trigger(Database::EVENT_DOCUMENT_FIND, $sql);
         try {
             $stmt = $this->getPDO()->prepare($sql);

             foreach ($binds as $key => $value) {
-                var_dump($key);
-                var_dump($value);
                 if (gettype($value) === 'double') {
                     $stmt->bindValue($key, $this->getFloatPrecision($value), \PDO::PARAM_STR);
                 } else {
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Database/Adapter/SQL.php` around lines 3151 - 3157, Remove the
unconditional var_dump calls that leak SQL and bind data: delete the
var_dump($sql) and the var_dump($key)/var_dump($value) calls in the find()
execution path (the places that prepare the statement and iterate $binds), and
if you need runtime diagnostics replace them with a conditional debug log (e.g.
$this->logger->debug(...) or a check against a debug flag) so sensitive data is
not printed to output in normal operation.

if (gettype($value) === 'double') {
$stmt->bindValue($key, $this->getFloatPrecision($value), \PDO::PARAM_STR);
} else {
Expand Down
24 changes: 12 additions & 12 deletions tests/e2e/Adapter/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,19 @@

abstract class Base extends TestCase
{
use CollectionTests;
use CustomDocumentTypeTests;
// use CollectionTests;
// use CustomDocumentTypeTests;
use DocumentTests;
use AttributeTests;
use IndexTests;
use OperatorTests;
use PermissionTests;
use RelationshipTests;
use SpatialTests;
use SchemalessTests;
use ObjectAttributeTests;
use VectorTests;
use GeneralTests;
// use AttributeTests;
// use IndexTests;
// use OperatorTests;
// use PermissionTests;
// use RelationshipTests;
// use SpatialTests;
// use SchemalessTests;
// use ObjectAttributeTests;
// use VectorTests;
// use GeneralTests;
Comment on lines +26 to +38
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Re-enable the disabled e2e trait suites in base test harness.

Commenting out most trait use lines here significantly reduces CI signal and can hide regressions outside DocumentTests, which is high-risk for a production PR.

Proposed fix 
-//    use CollectionTests;
-//    use CustomDocumentTypeTests;
+    use CollectionTests;
+    use CustomDocumentTypeTests;
     use DocumentTests;
-//    use AttributeTests;
-//    use IndexTests;
-//    use OperatorTests;
-//    use PermissionTests;
-//    use RelationshipTests;
-//    use SpatialTests;
-//    use SchemalessTests;
-//    use ObjectAttributeTests;
-//    use VectorTests;
-//    use GeneralTests;
+    use AttributeTests;
+    use IndexTests;
+    use OperatorTests;
+    use PermissionTests;
+    use RelationshipTests;
+    use SpatialTests;
+    use SchemalessTests;
+    use ObjectAttributeTests;
+    use VectorTests;
+    use GeneralTests;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// use CollectionTests;
// use CustomDocumentTypeTests;
use DocumentTests;
use AttributeTests;
use IndexTests;
use OperatorTests;
use PermissionTests;
use RelationshipTests;
use SpatialTests;
use SchemalessTests;
use ObjectAttributeTests;
use VectorTests;
use GeneralTests;
// use AttributeTests;
// use IndexTests;
// use OperatorTests;
// use PermissionTests;
// use RelationshipTests;
// use SpatialTests;
// use SchemalessTests;
// use ObjectAttributeTests;
// use VectorTests;
// use GeneralTests;
use CollectionTests;
use CustomDocumentTypeTests;
use DocumentTests;
use AttributeTests;
use IndexTests;
use OperatorTests;
use PermissionTests;
use RelationshipTests;
use SpatialTests;
use SchemalessTests;
use ObjectAttributeTests;
use VectorTests;
use GeneralTests;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/Adapter/Base.php` around lines 26 - 38, The Base e2e test harness
currently only uses DocumentTests and has commented out the other trait suites
(CollectionTests, CustomDocumentTypeTests, AttributeTests, IndexTests,
OperatorTests, PermissionTests, RelationshipTests, SpatialTests,
SchemalessTests, ObjectAttributeTests, VectorTests, GeneralTests); restore CI
coverage by uncommenting those `use` lines in tests/e2e/Adapter/Base.php so the
full set of trait test suites are included again, ensuring the test class
imports all previously-disabled traits and runs their tests alongside
DocumentTests.


protected static string $namespace;

Expand Down
10 changes: 10 additions & 0 deletions tests/e2e/Adapter/Scopes/DocumentTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2177,6 +2177,16 @@ public function testFindFulltextSpecialChars(): void
]);

$this->assertEquals(1, count($documents));

$phrases = ["Álvaro"];

foreach ($phrases as $phrase) {
$database->find($collection, [
Query::search('ft', $phrase),
]);
}

$this->assertEquals(999, 999999);
Comment on lines +2181 to +2189
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

This block introduces a guaranteed test failure and no meaningful verification.

Line 2189 always fails (999 !== 999999), and the loop currently performs queries without asserting behavior.

Suggested fix 
-        $phrases = ["Álvaro"];
-
-        foreach ($phrases as $phrase) {
-            $database->find($collection, [
-                Query::search('ft', $phrase),
-            ]);
-        }
-
-        $this->assertEquals(999, 999999);
+        $phrases = ['Álvaro'];
+
+        foreach ($phrases as $phrase) {
+            $documents = $database->find($collection, [
+                Query::search('ft', $phrase),
+            ]);
+            $this->assertIsArray($documents);
+        }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 2181 - 2189, The
test currently contains a guaranteed failing assertion (999 vs 999999) and no
real checks for the queries; replace the bogus assert with meaningful assertions
that validate the results from $database->find when using Query::search('ft',
$phrase). For each $phrase in $phrases, capture the result of
$database->find($collection, [ Query::search('ft', $phrase) ]) and assert
expected behavior (e.g., assert that the returned result is not null,
assertCount > 0, or assert the returned documents contain the expected value
"Álvaro"); update the loop to perform these assertions instead of the hardcoded
failing check so the test verifies actual query output.

}

public function testFindMultipleConditions(): void
Expand Down
Loading