Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughArray-backed JSON attributes are now NOT NULL with empty-array defaults; array append/prepend operators stop coalescing columns; adapters expose GIN-index capability; upsert/encode coerce null optional arrays to empty arrays; tests updated and PDO accessors added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Adapter/Postgres.php (1)
576-626:⚠️ Potential issue | 🟠 Major
getSQLTypeincludes NOT NULL/DEFAULT constraints unsuitable forALTER COLUMN TYPEsyntax.When
array=true,getSQLType()returns"JSONB NOT NULL DEFAULT '[]'::jsonb". PostgreSQL'sALTER COLUMN ... TYPEclause does not accept constraint modifiers—attempting to use this string directly inupdateAttribute()generates invalid SQL and fails at runtime.Separate the type from constraints: extract just
JSONBfor theTYPEclause, then applyDEFAULTandNOT NULLas separateALTER COLUMNsubcommands (backfilling NULLs before enforcingSET NOT NULLto avoid violations).🛠️ Proposed fix
- $type = $this->getSQLType( + $isArray = $array === true; + $type = $this->getSQLType( $type, $size, $signed, $array, $required, ); + if ($isArray) { + // ALTER COLUMN TYPE cannot include NOT NULL/DEFAULT in Postgres + $type = 'JSONB'; + } if ($type == 'TIMESTAMP(3)') { $type = "TIMESTAMP(3) without time zone USING TO_TIMESTAMP(\"$id\", 'YYYY-MM-DD HH24:MI:SS.MS')"; } @@ -624,6 +634,19 @@ $result = $this->execute($this->getPDO() ->prepare($sql)); + if ($isArray) { + // Backfill NULLs then enforce NOT NULL + DEFAULT + $table = $this->getSQLTable($name); + $this->execute($this->getPDO()->prepare( + "UPDATE {$table} SET \"{$id}\" = '[]'::jsonb WHERE \"{$id}\" IS NULL" + )); + $this->execute($this->getPDO()->prepare( + "ALTER TABLE {$table} ALTER COLUMN \"{$id}\" SET DEFAULT '[]'::jsonb" + )); + $this->execute($this->getPDO()->prepare( + "ALTER TABLE {$table} ALTER COLUMN \"{$id}\" SET NOT NULL" + )); + } if (!$result) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Postgres.php` around lines 576 - 626, The SQL returned by getSQLType(...) can include modifiers (e.g., "JSONB NOT NULL DEFAULT '[]'::jsonb") which break the ALTER COLUMN ... TYPE statement in updateAttribute; modify updateAttribute to parse the string returned by getSQLType($type, $size, $signed, $array, $required) into a base SQL type token (e.g., "JSONB") and separate constraint pieces (NOT NULL and DEFAULT expressions), then execute ALTER TABLE ... ALTER COLUMN "col" TYPE <baseType> first, and afterwards apply constraints with separate ALTER COLUMN subcommands: if DEFAULT present run ALTER COLUMN ... SET DEFAULT <expr>, if NOT NULL present first run an UPDATE <table> SET "col" = <defaultExpr> WHERE "col" IS NULL to backfill NULLs (when needed, e.g., JSONB default '[]'::jsonb), then run ALTER COLUMN ... SET NOT NULL; reference updateAttribute and getSQLType to find where to split the type string and apply these ordered ALTER statements.
🧹 Nitpick comments (2)
tests/e2e/Adapter/PoolTest.php (1)
82-92: Optional:deleteColumn,deleteIndex, andexecRawSQLcan reusegetPDO()now that it exists.All three methods still inline the same five-line reflection block. Since the extracted
getPDO()returns the same PDO object, they can be simplified:♻️ Example simplification for
deleteColumnprotected function deleteColumn(string $collection, string $column): bool { $sqlTable = "`" . $this->getDatabase()->getDatabase() . "`.`" . $this->getDatabase()->getNamespace() . "_" . $collection . "`"; $sql = "ALTER TABLE {$sqlTable} DROP COLUMN `{$column}`"; - - self::$pool->use(function (Adapter $adapter) use ($sql) { - // Hack to get adapter PDO reference - $class = new ReflectionClass($adapter); - $property = $class->getProperty('pdo'); - $property->setAccessible(true); - $pdo = $property->getValue($adapter); - $pdo->exec($sql); - }); - + $this->getPDO()->exec($sql); return true; }Apply the same pattern to
deleteIndexandexecRawSQL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/PoolTest.php` around lines 82 - 92, Replace the duplicated five-line reflection block in deleteColumn, deleteIndex, and execRawSQL with a call to the existing getPDO() helper to obtain the PDO instance; specifically, remove the ReflectionClass/->getProperty('pdo') logic from those methods and use $this->getPDO() (or getPDO() if static context) to retrieve the PDO, then run the existing SQL operations against that returned PDO object, leaving Adapter and self::$pool usage only inside getPDO().src/Database/Adapter/MariaDB.php (1)
2082-2090: IFNULL guards inconsistently removed — onlyAPPEND/PREPENDcleaned up.
TYPE_ARRAY_REMOVE(line 2106),TYPE_ARRAY_UNIQUE(2113),TYPE_ARRAY_INTERSECT(2121),TYPE_ARRAY_DIFF(2133), andTYPE_ARRAY_FILTER(2147) all still wrap{$quotedColumn}withIFNULL(..., JSON_ARRAY()), whileAPPEND/PREPENDno longer do. Since the column is nowNOT NULL, all theseIFNULLguards are dead code. Consider removing them in the same pass for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/MariaDB.php` around lines 2082 - 2090, The ARRAY operator handlers are inconsistent: APPEND/PREPEND now use {$quotedColumn} directly but TYPE_ARRAY_REMOVE, TYPE_ARRAY_UNIQUE, TYPE_ARRAY_INTERSECT, TYPE_ARRAY_DIFF and TYPE_ARRAY_FILTER still wrap {$quotedColumn} with IFNULL(..., JSON_ARRAY()); remove those dead IFNULL wrappers so each case uses {$quotedColumn} directly (keep the existing bind key logic and JSON functions for each case), updating the branches for Operator::TYPE_ARRAY_REMOVE, Operator::TYPE_ARRAY_UNIQUE, Operator::TYPE_ARRAY_INTERSECT, Operator::TYPE_ARRAY_DIFF and Operator::TYPE_ARRAY_FILTER to match the APPEND/PREPEND style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Adapter/MariaDB.php`:
- Around line 1686-1688: The ALTER path that emits "JSON NOT NULL DEFAULT
(JSON_ARRAY())" will fail when existing rows have NULLs — modify the MariaDB
adapter's updateAttribute() flow to run an UPDATE table SET column =
JSON_ARRAY() WHERE column IS NULL (or equivalent) before executing ALTER TABLE
MODIFY/CHANGE using getSQLType() so NULLs are backfilled; also restore
consistent IFNULL() guarding for array operators by wrapping TYPE_ARRAY_APPEND
and TYPE_ARRAY_PREPEND expressions with IFNULL(column, JSON_ARRAY()) to match
the other operators (e.g., TYPE_ARRAY_REMOVE/UNIQUE/INTERSECT/DIFF/FILTER) so
behavior is uniform and safe.
In `@src/Database/Database.php`:
- Around line 8202-8208: The code currently coerces null to [] for optional
array attributes (when $value and $default are null) and unconditionally calls
$document->setAttribute($key, []) which causes optional arrays to be cleared
during partial/bulk updates (e.g., when encode(..., applyDefaults: false)).
Change the condition to only coerce when defaults should be applied or the
attribute was explicitly provided: add a check for the $applyDefaults flag (or
the presence of the attribute in the incoming payload) alongside $array &&
!$required before calling $document->setAttribute($key, []); keep the existing
variables ($value, $default, $attribute, $array, $required, $applyDefaults) and
the document->setAttribute($key, []) call but wrap it so it only runs when
applying defaults or when the attribute was explicitly set.
In `@tests/e2e/Adapter/Scopes/IndexTests.php`:
- Around line 1156-1164: The PHPStan error arises because $stmt->fetchColumn()
can return int|string|false and assertNotFalse() doesn't narrow it to string for
subsequent strtolower(); update the block around fetchColumn() and assertions to
keep the assertNotFalse($indexDef, ...) check on the raw $indexDef, then cast to
string before calling strtolower (e.g. $indexDefStr = (string) $indexDef) and
pass $indexDefStr to strtolower in the assertStringContainsString call;
reference symbols: $stmt->fetchColumn(), $indexDef, assertNotFalse, strtolower,
assertStringContainsString.
---
Outside diff comments:
In `@src/Database/Adapter/Postgres.php`:
- Around line 576-626: The SQL returned by getSQLType(...) can include modifiers
(e.g., "JSONB NOT NULL DEFAULT '[]'::jsonb") which break the ALTER COLUMN ...
TYPE statement in updateAttribute; modify updateAttribute to parse the string
returned by getSQLType($type, $size, $signed, $array, $required) into a base SQL
type token (e.g., "JSONB") and separate constraint pieces (NOT NULL and DEFAULT
expressions), then execute ALTER TABLE ... ALTER COLUMN "col" TYPE <baseType>
first, and afterwards apply constraints with separate ALTER COLUMN subcommands:
if DEFAULT present run ALTER COLUMN ... SET DEFAULT <expr>, if NOT NULL present
first run an UPDATE <table> SET "col" = <defaultExpr> WHERE "col" IS NULL to
backfill NULLs (when needed, e.g., JSONB default '[]'::jsonb), then run ALTER
COLUMN ... SET NOT NULL; reference updateAttribute and getSQLType to find where
to split the type string and apply these ordered ALTER statements.
---
Nitpick comments:
In `@src/Database/Adapter/MariaDB.php`:
- Around line 2082-2090: The ARRAY operator handlers are inconsistent:
APPEND/PREPEND now use {$quotedColumn} directly but TYPE_ARRAY_REMOVE,
TYPE_ARRAY_UNIQUE, TYPE_ARRAY_INTERSECT, TYPE_ARRAY_DIFF and TYPE_ARRAY_FILTER
still wrap {$quotedColumn} with IFNULL(..., JSON_ARRAY()); remove those dead
IFNULL wrappers so each case uses {$quotedColumn} directly (keep the existing
bind key logic and JSON functions for each case), updating the branches for
Operator::TYPE_ARRAY_REMOVE, Operator::TYPE_ARRAY_UNIQUE,
Operator::TYPE_ARRAY_INTERSECT, Operator::TYPE_ARRAY_DIFF and
Operator::TYPE_ARRAY_FILTER to match the APPEND/PREPEND style.
In `@tests/e2e/Adapter/PoolTest.php`:
- Around line 82-92: Replace the duplicated five-line reflection block in
deleteColumn, deleteIndex, and execRawSQL with a call to the existing getPDO()
helper to obtain the PDO instance; specifically, remove the
ReflectionClass/->getProperty('pdo') logic from those methods and use
$this->getPDO() (or getPDO() if static context) to retrieve the PDO, then run
the existing SQL operations against that returned PDO object, leaving Adapter
and self::$pool usage only inside getPDO().
| if ($array === true) { | ||
| return 'JSON'; | ||
| return 'JSON NOT NULL DEFAULT (JSON_ARRAY())'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the updateAttribute method in MariaDB.php
head -n 450 src/Database/Adapter/MariaDB.php | tail -n 80Repository: utopia-php/database
Length of output: 2845
🏁 Script executed:
# Check the full context of getSQLType for array handling
rg -n "getSQLType" src/Database/Adapter/MariaDB.php -A 3 -B 3Repository: utopia-php/database
Length of output: 1486
🏁 Script executed:
# Look for any UPDATE or backfill logic related to array columns
rg -n "JSON_ARRAY\(\)" src/Database/Adapter/MariaDB.php -B 5 -A 5Repository: utopia-php/database
Length of output: 3009
🏁 Script executed:
# Check if there are any migration or backfill mechanisms in Database.php or SQL.php
rg -n "updateAttribute" src/Database/ -A 5 -B 2 --type=phpRepository: utopia-php/database
Length of output: 16758
🏁 Script executed:
# Check for NULL backfill patterns in the codebase
rg -n "IS NULL.*JSON_ARRAY\|JSON_ARRAY.*IS NULL" src/Database/ --type=phpRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Get the full updateAttribute method in Database.php around line 2616-2920
sed -n '2616,2935p' src/Database/Database.php | cat -nRepository: utopia-php/database
Length of output: 16616
🏁 Script executed:
# Specifically look for any UPDATE statements in the updateAttribute flow
rg -n "UPDATE" src/Database/Database.php | grep -A 5 -B 5 "2616\|2700\|2800\|2900"Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check for any JSON_ARRAY backfill patterns in the entire Database.php
rg -n "JSON_ARRAY" src/Database/Database.php -B 5 -A 5Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look for migration files or migration strategy
find . -type f -name "*migration*" -o -name "*migrate*" | head -20Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check if there are tests for updateAttribute with arrays
rg -n "updateAttribute.*array|array.*updateAttribute" tests/ --type=php -B 3 -A 3Repository: utopia-php/database
Length of output: 707
🏁 Script executed:
# Look for any test that validates updating array attributes on existing tables
rg -n "testUpdateAttribute" tests/ --type=php -lRepository: utopia-php/database
Length of output: 279
🏁 Script executed:
# Check what the test file mentioned in learnings says about array attributes
sed -n '6418,6439p' tests/e2e/Adapter/Scopes/DocumentTests.phpRepository: utopia-php/database
Length of output: 1068
🏁 Script executed:
# Examine the AttributeTests.php for array attribute tests
rg -n "updateAttribute" tests/e2e/Adapter/Scopes/AttributeTests.php -B 5 -A 10 | head -100Repository: utopia-php/database
Length of output: 4513
🏁 Script executed:
# Look for tests that create data with NULL or then update attribute to NOT NULL
rg -n "createDocument\|updateAttribute" tests/e2e/Adapter/Scopes/AttributeTests.php -B 2 -A 2 | head -80Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check if getSQLType array behavior is consistent across adapters
rg -n "array === true" src/Database/Adapter/ --type=php -B 2 -A 2Repository: utopia-php/database
Length of output: 860
🏁 Script executed:
# Look for any schemaless adapter handling
rg -n "getSupportForAttributes\|schemaless" src/Database/Adapter/ --type=php -B 3 -A 3 | head -80Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check if there's a specific test for updating array attributes on existing collections
rg -n "array.*true|createAttribute.*array" tests/e2e/Adapter/Scopes/AttributeTests.php -B 3 -A 8Repository: utopia-php/database
Length of output: 3855
🏁 Script executed:
# Check what createAttribute does for array types
rg -n "createAttribute" src/Database/Adapter/MariaDB.php -A 30 | head -60Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look at parent SQL adapter for array handling
rg -n "array === true|JSON NOT NULL" src/Database/Adapter/SQL.php -B 5 -A 5Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check if there's a difference between initial creation and update for NOT NULL handling
git log --oneline -20 -- src/Database/Adapter/MariaDB.phpRepository: utopia-php/database
Length of output: 159
🏁 Script executed:
# Look at the recent commit that mentions NOT NULL arrays
git show 4f02d63f --stat | head -40Repository: utopia-php/database
Length of output: 2426
🏁 Script executed:
# Check the parent class SQL.php to see if createAttribute creates with NOT NULL
rg -n "createAttribute\|getSQLType" src/Database/Adapter/SQL.php -A 15 | head -100Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look for the createAttribute implementation in MariaDB or parent
rg -n "public function createAttribute" src/Database/Adapter/ -A 20 | head -80Repository: utopia-php/database
Length of output: 5879
🏁 Script executed:
# Check if there's any test that actually calls updateAttribute on array fields
rg -n "updateAttribute.*array.*true|array.*true.*updateAttribute" tests/ -B 5 -A 10Repository: utopia-php/database
Length of output: 1608
🏁 Script executed:
# Verify when createAttribute is used vs updateAttribute for array fields
rg -n "createAttribute|updateAttribute" tests/e2e/Adapter/Scopes/AttributeTests.php | grep -i arrayRepository: utopia-php/database
Length of output: 364
🏁 Script executed:
# Check if Postgres adapter has the same issue
cat src/Database/Adapter/Postgres.php | grep -A 5 "array === true"Repository: utopia-php/database
Length of output: 221
🏁 Script executed:
# Check if there's a test that specifically tests updating an existing array column
# that might have NULL values (e.g., converting non-array to array)
rg -n "createDocument\|createAttribute" tests/e2e/Adapter/Scopes/AttributeTests.php -B 2 -A 5 | grep -A 20 "non-array\|convert"Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look for any migration or update scenario in tests where data exists before updateAttribute
rg -n "createDocument.*\$|createDocument" tests/e2e/Adapter/Scopes/AttributeTests.php | head -20Repository: utopia-php/database
Length of output: 1508
🏁 Script executed:
# Specifically check if there's a test scenario where documents are created,
# then an attribute is changed to array=true
rg -n "array.*true" tests/e2e/Adapter/Scopes/AttributeTests.php -B 20 | grep -A 20 "createDocument"Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check if there's validation preventing changing array from false to true
rg -n "originalArray\|array.*change\|array.*update" src/Database/Database.php -B 3 -A 3Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look for what happens when originalArray != array (is this even allowed?)
sed -n '2616,2650p' src/Database/Database.phpRepository: utopia-php/database
Length of output: 1706
🏁 Script executed:
# Check if there's a scenario in tests where array is changed
rg -n "originalArray" src/Database/ --type=php -B 5 -A 5Repository: utopia-php/database
Length of output: 1632
🏁 Script executed:
# Check TYPE_ARRAY_APPEND and TYPE_ARRAY_PREPEND implementations
rg -n "TYPE_ARRAY_APPEND|TYPE_ARRAY_PREPEND" src/Database/Adapter/MariaDB.php -B 2 -A 15Repository: utopia-php/database
Length of output: 1066
🏁 Script executed:
# Verify the issue: check if array attribute can be changed from false to true on existing table
rg -n "array.*false\|array.*true" tests/e2e/Adapter/Scopes/AttributeTests.php -B 5 -A 5 | head -50Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check if there's any constraint preventing changing array=false to array=true
rg -n "array.*originalArray\|changing.*array\|convert.*array" src/Database/ --type=php -B 3 -A 3Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Final check: look for validation that prevents this specific change
sed -n '2642,2750p' src/Database/Database.php | cat -nRepository: utopia-php/database
Length of output: 5648
🏁 Script executed:
# Confirm: check if there are other operators that still have IFNULL
rg -n "TYPE_ARRAY_UNIQUE|TYPE_ARRAY_REMOVE|TYPE_ARRAY_INTERSECT|TYPE_ARRAY_DIFF|TYPE_ARRAY_FILTER" src/Database/Adapter/MariaDB.php -A 8 | grep -E "IFNULL|TYPE_ARRAY"Repository: utopia-php/database
Length of output: 598
🏁 Script executed:
# Final verification: confirm IFNULL was removed from APPEND/PREPEND but kept elsewhere
git show HEAD:src/Database/Adapter/MariaDB.php | grep -A 5 "TYPE_ARRAY_APPEND\|TYPE_ARRAY_PREPEND" | head -20Repository: utopia-php/database
Length of output: 539
MODIFY/CHANGE COLUMN … NOT NULL will fail for existing tables with any NULL array values.
updateAttribute() (Database.php, line 2915) passes getSQLType() output directly to the adapter's updateAttribute() method, which executes ALTER TABLE … MODIFY/CHANGE COLUMN with no preceding UPDATE to backfill NULLs. Any production collection with existing NULL values in an array column will cause the ALTER TABLE to fail with a data-integrity error when this code path is triggered.
The PHP-side null-to-[] coercion only governs future writes and does not retroactively fix existing DB NULLs.
A safe approach is to issue an UPDATE to coerce NULLs to JSON_ARRAY() before the ALTER TABLE:
🛡️ Proposed fix: backfill NULLs before MODIFY
public function updateAttribute(string $collection, string $id, string $type, int $size, bool $signed = true, bool $array = false, ?string $newKey = null, bool $required = false): bool
{
$name = $this->filter($collection);
$id = $this->filter($id);
$newKey = empty($newKey) ? null : $this->filter($newKey);
$type = $this->getSQLType($type, $size, $signed, $array, $required);
+
+ // Backfill NULLs before adding NOT NULL constraint on array columns
+ if ($array) {
+ $backfill = "UPDATE {$this->getSQLTable($name)} SET `{$id}` = JSON_ARRAY() WHERE `{$id}` IS NULL";
+ $this->getPDO()->prepare($backfill)->execute();
+ }
+
if (!empty($newKey)) {
$sql = "ALTER TABLE {$this->getSQLTable($name)} CHANGE COLUMN `{$id}` `{$newKey}` {$type};";
} else {
$sql = "ALTER TABLE {$this->getSQLTable($name)} MODIFY `{$id}` {$type};";
}Additionally, TYPE_ARRAY_APPEND and TYPE_ARRAY_PREPEND (lines 2085, 2090) dropped their IFNULL guards, while TYPE_ARRAY_REMOVE, TYPE_ARRAY_UNIQUE, TYPE_ARRAY_INTERSECT, TYPE_ARRAY_DIFF, and TYPE_ARRAY_FILTER (lines 2103–2161) retain them. This asymmetry is harmless with the NOT NULL constraint but creates inconsistency that will mislead future maintainers. Consider applying IFNULL consistently across all array operators for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/MariaDB.php` around lines 1686 - 1688, The ALTER path
that emits "JSON NOT NULL DEFAULT (JSON_ARRAY())" will fail when existing rows
have NULLs — modify the MariaDB adapter's updateAttribute() flow to run an
UPDATE table SET column = JSON_ARRAY() WHERE column IS NULL (or equivalent)
before executing ALTER TABLE MODIFY/CHANGE using getSQLType() so NULLs are
backfilled; also restore consistent IFNULL() guarding for array operators by
wrapping TYPE_ARRAY_APPEND and TYPE_ARRAY_PREPEND expressions with
IFNULL(column, JSON_ARRAY()) to match the other operators (e.g.,
TYPE_ARRAY_REMOVE/UNIQUE/INTERSECT/DIFF/FILTER) so behavior is uniform and safe.
| // Verify GIN index exists | ||
| $stmt = $pdo->prepare( | ||
| "SELECT indexdef FROM pg_indexes WHERE tablename = ? AND indexname LIKE ?" | ||
| ); | ||
| $stmt->execute(["{$ns}_{$collection}", '%idx_tags%']); | ||
| $indexDef = $stmt->fetchColumn(); | ||
| $this->assertNotFalse($indexDef, 'GIN index should exist for array column'); | ||
| $this->assertStringContainsString('gin', \strtolower($indexDef), 'Index on array column should be a GIN index'); | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if file exists and get line count
wc -l tests/e2e/Adapter/Scopes/IndexTests.php
# View the exact lines mentioned in the review comment
sed -n '1150,1170p' tests/e2e/Adapter/Scopes/IndexTests.phpRepository: utopia-php/database
Length of output: 1098
🏁 Script executed:
#!/bin/bash
# Search for fetchColumn and strtolower usage in the file
rg -n "fetchColumn" tests/e2e/Adapter/Scopes/IndexTests.php -B2 -A5Repository: utopia-php/database
Length of output: 516
🏁 Script executed:
#!/bin/bash
# Check for similar fetchColumn patterns in the codebase to understand typical handling
rg "fetchColumn\(\)" tests/e2e/Adapter/Scopes/ -B2 -A2
# Check if there are other places where strtolower is used with fetchColumn results
rg "fetchColumn.*strtolower|strtolower.*fetchColumn" tests/e2e/Adapter/Scopes/ -B3 -A3Repository: utopia-php/database
Length of output: 604
🏁 Script executed:
#!/bin/bash
# Check the PDO documentation by looking at type hints or casts in the codebase
rg "fetchColumn" src/ -B1 -A1 | head -50Repository: utopia-php/database
Length of output: 2084
🏁 Script executed:
#!/bin/bash
# Check if there are other similar fetchColumn patterns in IndexTests.php that might have same issue
rg "fetchColumn\(\)" tests/e2e/Adapter/Scopes/IndexTests.php -nRepository: utopia-php/database
Length of output: 113
Fix PHPStan type error by casting fetchColumn() result to string.
fetchColumn() returns int|string|false, and assertNotFalse() does not narrow the type to string in PHPStan's analysis. Cast the result to string before passing it to strtolower():
🛠️ Proposed fix
$indexDef = $stmt->fetchColumn();
$this->assertNotFalse($indexDef, 'GIN index should exist for array column');
+ $indexDef = (string) $indexDef;
$this->assertStringContainsString('gin', \strtolower($indexDef), 'Index on array column should be a GIN index');📝 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.
| // Verify GIN index exists | |
| $stmt = $pdo->prepare( | |
| "SELECT indexdef FROM pg_indexes WHERE tablename = ? AND indexname LIKE ?" | |
| ); | |
| $stmt->execute(["{$ns}_{$collection}", '%idx_tags%']); | |
| $indexDef = $stmt->fetchColumn(); | |
| $this->assertNotFalse($indexDef, 'GIN index should exist for array column'); | |
| $this->assertStringContainsString('gin', \strtolower($indexDef), 'Index on array column should be a GIN index'); | |
| } else { | |
| // Verify GIN index exists | |
| $stmt = $pdo->prepare( | |
| "SELECT indexdef FROM pg_indexes WHERE tablename = ? AND indexname LIKE ?" | |
| ); | |
| $stmt->execute(["{$ns}_{$collection}", '%idx_tags%']); | |
| $indexDef = $stmt->fetchColumn(); | |
| $this->assertNotFalse($indexDef, 'GIN index should exist for array column'); | |
| $indexDef = (string) $indexDef; | |
| $this->assertStringContainsString('gin', \strtolower($indexDef), 'Index on array column should be a GIN index'); | |
| } else { |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 1163-1163: PHPStan error: Parameter #1 $string of function strtolower expects string, int|string|null given. Command: ./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Adapter/Scopes/IndexTests.php` around lines 1156 - 1164, The
PHPStan error arises because $stmt->fetchColumn() can return int|string|false
and assertNotFalse() doesn't narrow it to string for subsequent strtolower();
update the block around fetchColumn() and assertions to keep the
assertNotFalse($indexDef, ...) check on the raw $indexDef, then cast to string
before calling strtolower (e.g. $indexDefStr = (string) $indexDef) and pass
$indexDefStr to strtolower in the assertStringContainsString call; reference
symbols: $stmt->fetchColumn(), $indexDef, assertNotFalse, strtolower,
assertStringContainsString.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Database/Adapter/Postgres.php (2)
2718-2727: RemovingCOALESCEis correct forNOT NULLcolumns, but note the migration risk.Dropping
COALESCE(col, '[]'::jsonb)is safe once all rows have been backfilled to non-null. However, for any existing row where the column is stillNULL(e.g., before a backfill migration runs),NULL::jsonb || :op::jsonbevaluates toNULLin PostgreSQL — the append/prepend silently becomes a no-op instead of raising an error.Ensure the schema migration and data backfill (
UPDATE … SET col = '[]'::jsonb WHERE col IS NULL) run before this code is deployed, or consider temporarily keeping theCOALESCEuntil migration is confirmed complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Postgres.php` around lines 2718 - 2727, The change removing COALESCE from the array append/prepend SQL (cases for Operator::TYPE_ARRAY_APPEND and Operator::TYPE_ARRAY_PREPEND that return "{$quotedColumn} = {$columnRef} || :$bindKey::jsonb" and "{$quotedColumn} = :$bindKey::jsonb || {$columnRef}") can silently no-op if existing rows contain NULL; ensure you either run the migration/backfill (UPDATE … SET col = '[]'::jsonb WHERE col IS NULL) before deploying this code or revert to using COALESCE(columnRef, '[]'::jsonb) in these branches (or both) so that quotedColumn/columnRef operations behave correctly until all rows are non-null.
895-913: Extra DB round-trip perINDEX_KEYindex creation.
createCollectionloops over indexes and callscreateIndexfor each; everyINDEX_KEYindex now incurs an additionalgetDocumentcall to fetch collection metadata. For collections with many array indexes, this multiplies the number of DB round-trips.Consider enriching the existing
$indexAttributeTypesparameter (or adding an$indexAttributeFlagsparallel map) with thearrayflag so callers can pass this information in, eliminating the query entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Postgres.php` around lines 895 - 913, createIndex currently does an extra DB round-trip by calling getDocument on Database::METADATA to detect array-typed attributes when handling Database::INDEX_KEY; to fix this, add the attribute "array" flag to the existing $indexAttributeTypes parameter (or create a parallel $indexAttributeFlags map) at the call site (e.g., where createCollection assembles index metadata) and change createIndex in Postgres.php to read the array flag from that parameter instead of constructing a metadata Document and calling getDocument; update createCollection to pass the new array-aware map when iterating indexes so createIndex can compute $isArrayIndex from the provided flags and remove the whole getDocument/collectionAttributes loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Adapter/Postgres.php`:
- Around line 1957-1961: getSQLType currently returns "JSONB NOT NULL DEFAULT
'[]'::jsonb" for array columns which is invalid inside an ALTER COLUMN ... TYPE
clause; change getSQLType(array=true) to return only "JSONB" and update
updateAttribute to emit separate ALTER COLUMN subcommands for NOT NULL and
DEFAULT (e.g., ALTER TABLE ... ALTER COLUMN "col" TYPE JSONB, ALTER TABLE ...
ALTER COLUMN "col" SET DEFAULT '[]'::jsonb, and ALTER TABLE ... ALTER COLUMN
"col" SET NOT NULL / DROP NOT NULL as needed). Adjust logic in updateAttribute
to detect array columns (or call to getSQLType) and append separate ALTER
clauses for defaults and nullability rather than embedding them in the TYPE
string.
In `@src/Database/Adapter/SQL.php`:
- Around line 2683-2686: createDocuments is missing the null-to-'[]' coercion
for array-typed attributes which causes NULL to be bound for NOT NULL DEFAULT
'[]' columns; mirror the fix in upsertDocuments by building the $arrayAttributes
map just before the loop that iterates over documents (same approach used in
upsertDocuments) and, in the value-binding loop inside createDocuments where you
currently use $attributes[$key] ?? null, coerce $attrValue === null &&
isset($arrayAttributes[$key]) to '[]' before binding; reference createDocuments,
upsertDocuments, $attributeKeys, $attributes, $arrayAttributes and the binding
loop when applying the change.
---
Nitpick comments:
In `@src/Database/Adapter/Postgres.php`:
- Around line 2718-2727: The change removing COALESCE from the array
append/prepend SQL (cases for Operator::TYPE_ARRAY_APPEND and
Operator::TYPE_ARRAY_PREPEND that return "{$quotedColumn} = {$columnRef} ||
:$bindKey::jsonb" and "{$quotedColumn} = :$bindKey::jsonb || {$columnRef}") can
silently no-op if existing rows contain NULL; ensure you either run the
migration/backfill (UPDATE … SET col = '[]'::jsonb WHERE col IS NULL) before
deploying this code or revert to using COALESCE(columnRef, '[]'::jsonb) in these
branches (or both) so that quotedColumn/columnRef operations behave correctly
until all rows are non-null.
- Around line 895-913: createIndex currently does an extra DB round-trip by
calling getDocument on Database::METADATA to detect array-typed attributes when
handling Database::INDEX_KEY; to fix this, add the attribute "array" flag to the
existing $indexAttributeTypes parameter (or create a parallel
$indexAttributeFlags map) at the call site (e.g., where createCollection
assembles index metadata) and change createIndex in Postgres.php to read the
array flag from that parameter instead of constructing a metadata Document and
calling getDocument; update createCollection to pass the new array-aware map
when iterating indexes so createIndex can compute $isArrayIndex from the
provided flags and remove the whole getDocument/collectionAttributes loop.
| protected function getSQLType(string $type, int $size, bool $signed = true, bool $array = false, bool $required = false): string | ||
| { | ||
| if ($array === true) { | ||
| return 'JSONB'; | ||
| return "JSONB NOT NULL DEFAULT '[]'::jsonb"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the getSQLType method implementation to confirm the behavior with arrays
rg -A 30 "protected function getSQLType" src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 1197
🏁 Script executed:
# Search for updateAttribute method to see how it uses getSQLType
rg -A 50 "protected function updateAttribute" src/Database/Adapter/Postgres.php | head -80Repository: utopia-php/database
Length of output: 45
🌐 Web query:
PostgreSQL ALTER TABLE ALTER COLUMN TYPE syntax - does it support NOT NULL DEFAULT modifiers
💡 Result:
ALTER TABLE ... ALTER COLUMN ... TYPE (aka SET DATA TYPE) does not accept inline column-definition modifiers like NOT NULL or DEFAULT. It only changes the data type (optionally with USING/COLLATE). [1][2]
To change those properties, use separate subcommands:
ALTER TABLE t
ALTER COLUMN c TYPE new_type USING ...,
ALTER COLUMN c SET DEFAULT new_default,
ALTER COLUMN c SET NOT NULL;PostgreSQL will also try to cast the existing column default (if any) and constraints that reference the column when you change the type, which is why doing SET DEFAULT explicitly after a type change is sometimes clearer/safer. [1]
Sources:
[1] PostgreSQL docs (current): “Modifying Tables” (ALTER COLUMN TYPE / SET DEFAULT / SET NOT NULL) (postgresql.org)
[2] PostgreSQL ALTER TABLE reference (Postgres Pro mirror): SET DATA TYPE, SET/DROP DEFAULT, SET/DROP NOT NULL (postgrespro.com)
Citations:
- 1: https://www.postgresql.org/docs/current/ddl-alter.html?utm_source=openai
- 2: https://postgrespro.com/docs/postgresql/14/sql-altertable?utm_source=openai
🏁 Script executed:
# Search for updateAttribute method with more context
rg -A 100 "protected function updateAttribute" src/Database/Adapter/Postgres.php | head -120Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Also search for where getSQLType is called within updateAttribute context
rg -B 5 -A 15 "getSQLType.*\$array" src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for ALTER COLUMN in Postgres.php to find where updateAttribute uses the type
rg -B 10 "ALTER COLUMN" src/Database/Adapter/Postgres.php | head -100Repository: utopia-php/database
Length of output: 299
🏁 Script executed:
# Try to find updateAttribute by searching for the method name more broadly
rg "updateAttribute" src/Database/Adapter/Postgres.phpRepository: utopia-php/database
Length of output: 251
🏁 Script executed:
# Get the full updateAttribute implementation
rg -A 150 "public function updateAttribute" src/Database/Adapter/Postgres.php | head -200Repository: utopia-php/database
Length of output: 5027
updateAttribute will generate invalid SQL for array attributes.
getSQLType now returns "JSONB NOT NULL DEFAULT '[]'::jsonb" when $array === true. This string is fed directly into the ALTER COLUMN TYPE clause:
ALTER TABLE schema.table ALTER COLUMN "col" TYPE JSONB NOT NULL DEFAULT '[]'::jsonbPostgreSQL's ALTER COLUMN TYPE grammar accepts only data_type [COLLATE …] [USING …]. The NOT NULL and DEFAULT clauses must be issued as separate ALTER COLUMN sub-commands; their presence in the TYPE clause causes a syntax error.
This is a regression: the previous return value (JSONB alone) was valid for the TYPE clause.
Proposed fix in updateAttribute
$type = $this->getSQLType(
$type,
$size,
$signed,
$array,
$required,
);
+ // For ALTER COLUMN TYPE the NOT NULL / DEFAULT modifiers must be issued
+ // as separate ALTER COLUMN sub-commands; strip them from the type string.
+ $alterType = preg_replace('/\s+NOT NULL.*$/i', '', $type);
+
if ($type == 'TIMESTAMP(3)') {
- $type = "TIMESTAMP(3) without time zone USING TO_TIMESTAMP(\"$id\", 'YYYY-MM-DD HH24:MI:SS.MS')";
+ $alterType = "TIMESTAMP(3) without time zone USING TO_TIMESTAMP(\"$id\", 'YYYY-MM-DD HH24:MI:SS.MS')";
}
// ... (use $alterType in the ALTER COLUMN sql, keep $type for other uses)
$sql = "
ALTER TABLE {$this->getSQLTable($name)}
- ALTER COLUMN \"{$id}\" TYPE {$type}
+ ALTER COLUMN \"{$id}\" TYPE {$alterType}
";🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 1957-1957: Avoid unused parameters such as '$signed'. (undefined)
(UnusedFormalParameter)
[warning] 1957-1957: Avoid unused parameters such as '$required'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/Postgres.php` around lines 1957 - 1961, getSQLType
currently returns "JSONB NOT NULL DEFAULT '[]'::jsonb" for array columns which
is invalid inside an ALTER COLUMN ... TYPE clause; change getSQLType(array=true)
to return only "JSONB" and update updateAttribute to emit separate ALTER COLUMN
subcommands for NOT NULL and DEFAULT (e.g., ALTER TABLE ... ALTER COLUMN "col"
TYPE JSONB, ALTER TABLE ... ALTER COLUMN "col" SET DEFAULT '[]'::jsonb, and
ALTER TABLE ... ALTER COLUMN "col" SET NOT NULL / DROP NOT NULL as needed).
Adjust logic in updateAttribute to detect array columns (or call to getSQLType)
and append separate ALTER clauses for defaults and nullability rather than
embedding them in the TYPE string.
| // Array columns are NOT NULL DEFAULT '[]', so coerce null to empty array | ||
| if ($attrValue === null && isset($arrayAttributes[$attributeKey])) { | ||
| $attrValue = '[]'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and locate the relevant methods
fd "SQL.php" --type f | head -5Repository: utopia-php/database
Length of output: 122
🏁 Script executed:
# Get line count for SQL.php to understand the file size
wc -l src/Database/Adapter/SQL.phpRepository: utopia-php/database
Length of output: 96
🏁 Script executed:
# Check the createDocuments method around line 2451-2586
sed -n '2451,2500p' src/Database/Adapter/SQL.php | cat -nRepository: utopia-php/database
Length of output: 2189
🏁 Script executed:
# Check the $arrayAttributes collection and coercion in upsertDocuments (lines 2607-2686)
sed -n '2607,2700p' src/Database/Adapter/SQL.php | cat -nRepository: utopia-php/database
Length of output: 4999
🏁 Script executed:
# Check the second coercion block mentioned (lines 2815-2818)
sed -n '2800,2830p' src/Database/Adapter/SQL.php | cat -nRepository: utopia-php/database
Length of output: 1895
🏁 Script executed:
# Search for encode method in Database.php to check for upstream coercion
rg -n "public function encode|private function encode" src/Database/Database.php -A 15 | head -80Repository: utopia-php/database
Length of output: 899
🏁 Script executed:
# See the rest of createDocuments to find the value binding logic
sed -n '2500,2586p' src/Database/Adapter/SQL.php | cat -nRepository: utopia-php/database
Length of output: 4231
🏁 Script executed:
# See the full Database::encode() method to check for array coercion
sed -n '8177,8250p' src/Database/Database.php | cat -nRepository: utopia-php/database
Length of output: 3322
🏁 Script executed:
# Verify that createDocuments is called in context where encode() may not be applied first
rg -n "createDocuments" src/Database/Database.php -B 3 -A 3 | head -50Repository: utopia-php/database
Length of output: 662
🏁 Script executed:
# Check if Database::createDocuments calls encode() before adapter->createDocuments()
sed -n '5118,5200p' src/Database/Database.php | cat -nRepository: utopia-php/database
Length of output: 4233
createDocuments is missing the same null-to-'[]' guard added here.
upsertDocuments correctly coerces null → '[]' for array-typed attributes (lines 2683–2686 and 2815–2818). However, createDocuments builds $attributeKeys as the union of all documents in the batch, then falls back to $attributes[$key] ?? null for each key. When document A has an optional array attribute and document B does not, document B binds NULL for a NOT NULL DEFAULT '[]' column → constraint violation.
🛡️ Proposed fix in createDocuments (SQL.php ~line 2456)
Collect the $arrayAttributes map the same way as upsertDocuments does, just before the loop that iterates over documents:
$spatialAttributes = $this->getSpatialAttributes($collection);
$collection = $collection->getId();
+
+ $arrayAttributes = [];
+ foreach ($collection->getAttribute('attributes', []) as $attr) {
+ if ($attr['array'] ?? false) {
+ $arrayAttributes[$attr['$id']] = true;
+ }
+ }Then apply the coercion when binding values:
foreach ($attributeKeys as $key) {
$value = $attributes[$key] ?? null;
+ // Array columns are NOT NULL DEFAULT '[]', so coerce null to empty array
+ if ($value === null && isset($arrayAttributes[$key])) {
+ $value = '[]';
+ }
if (\is_array($value)) {
$value = \json_encode($value);
}🤖 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 2683 - 2686, createDocuments is
missing the null-to-'[]' coercion for array-typed attributes which causes NULL
to be bound for NOT NULL DEFAULT '[]' columns; mirror the fix in upsertDocuments
by building the $arrayAttributes map just before the loop that iterates over
documents (same approach used in upsertDocuments) and, in the value-binding loop
inside createDocuments where you currently use $attributes[$key] ?? null, coerce
$attrValue === null && isset($arrayAttributes[$key]) to '[]' before binding;
reference createDocuments, upsertDocuments, $attributeKeys, $attributes,
$arrayAttributes and the binding loop when applying the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/IndexTests.php (2)
1064-1109: Missingtry/finally— collection leaks on assertion failure.Both new tests call
deleteCollectionunconditionally at the end. If any assertion fires before that line, the collection is never cleaned up, which can cause spurious failures in subsequent test runs. Every other cleanup-bearing test in this file wraps the body intry { … } finally { $database->deleteCollection(…); }.🛠️ Proposed fix for `testArrayAttributeNotNullSchema`
$database->createAttribute($collection, 'tags', Database::VAR_STRING, 255, false, array: true); + try { // Verify the physical column is NOT NULL via INFORMATION_SCHEMA $pdo = $this->getPDO(); // ... (rest of the test body) $this->assertEquals('NO', $col['IS_NULLABLE'], /* ... */); + } finally { - $database->deleteCollection($collection); + $database->deleteCollection($collection); + } }Apply the same pattern to
testArrayIndexUsedInQuery(wrap everything aftercreateCollectionintry { … } finally { $database->deleteCollection($collection); }).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/IndexTests.php` around lines 1064 - 1109, Wrap the post-createCollection test bodies in a try { … } finally { $database->deleteCollection($collection); } to guarantee cleanup on assertion failures: in testArrayAttributeNotNullSchema and testArrayIndexUsedInQuery, move all code after $database->createCollection(...) into a try block and place the $database->deleteCollection($collection) call inside the finally block so the collection is always removed even if assertions fail.
1091-1106:COLUMN_DEFAULTis fetched but never asserted.The
SELECTretrieves bothIS_NULLABLEandCOLUMN_DEFAULT, but onlyIS_NULLABLEis checked. The stated motivation (MySQL optimizer bug#111037andJSON_CONTAINS(NULL, …)returningNULL) requires both NOT NULL and a default empty-array. Asserting the default would catch adapters that setNOT NULLwithout a proper default, leaving inserts without thetagsfield to fail.💡 Suggested addition
$this->assertEquals('NO', $col['IS_NULLABLE'], /* ... */); + $this->assertNotNull( + $col['COLUMN_DEFAULT'], + 'Array (JSON) column must have a NOT NULL default (e.g. "[]" or "(json_array())")' + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/IndexTests.php` around lines 1091 - 1106, The test selects IS_NULLABLE and COLUMN_DEFAULT for the "tags" column but never asserts the default; update the test around the $stmt/$col handling to also assert that $col['COLUMN_DEFAULT'] equals the expected empty-array default (e.g. '[]' or the DB-specific literal your adapter uses) so that adapters setting NOT NULL without a default fail the test; locate the block using $stmt->execute([$db, $tableName, 'tags']) and $col = $stmt->fetch(...) and add an assertion checking COLUMN_DEFAULT on $col immediately after the IS_NULLABLE assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/Adapter/Scopes/IndexTests.php`:
- Around line 1199-1204: The test fails on the Postgres/GIN path because
Database::find() applies a default limit (25), so when getSupportForGinIndex()
yields $total = 5000 and $expectedCount = 100 the result in $results is
truncated; update the query passed to Database::find() to include an explicit
limit by adding Query::limit($expectedCount) alongside
Query::containsAny('tags', ['tag_1']) so the find call returns the full
expectedCount before asserting $this->assertCount($expectedCount, $results).
---
Duplicate comments:
In `@tests/e2e/Adapter/Scopes/IndexTests.php`:
- Around line 1162-1164: PHPStan complains because $indexDef (from
$stmt->fetchColumn()) is typed mixed and assertNotFalse() doesn't narrow it to
string before calling strtolower; update the test in IndexTests.php to ensure
$indexDef is a string before using strtolower by adding an explicit assertion
like $this->assertIsString($indexDef, 'Index definition must be a string') or
alternatively cast $indexDef to string when calling strtolower in the
assertStringContainsString call; reference $stmt->fetchColumn(), $indexDef,
assertNotFalse(), assertIsString(), assertStringContainsString(), and
strtolower() so the change is applied to the same assertion block.
---
Nitpick comments:
In `@tests/e2e/Adapter/Scopes/IndexTests.php`:
- Around line 1064-1109: Wrap the post-createCollection test bodies in a try { …
} finally { $database->deleteCollection($collection); } to guarantee cleanup on
assertion failures: in testArrayAttributeNotNullSchema and
testArrayIndexUsedInQuery, move all code after $database->createCollection(...)
into a try block and place the $database->deleteCollection($collection) call
inside the finally block so the collection is always removed even if assertions
fail.
- Around line 1091-1106: The test selects IS_NULLABLE and COLUMN_DEFAULT for the
"tags" column but never asserts the default; update the test around the
$stmt/$col handling to also assert that $col['COLUMN_DEFAULT'] equals the
expected empty-array default (e.g. '[]' or the DB-specific literal your adapter
uses) so that adapters setting NOT NULL without a default fail the test; locate
the block using $stmt->execute([$db, $tableName, 'tags']) and $col =
$stmt->fetch(...) and add an assertion checking COLUMN_DEFAULT on $col
immediately after the IS_NULLABLE assertion.
| // Verify the query also returns correct functional results | ||
| $expectedCount = $total / 50; // each tag_N appears once per 50 documents | ||
| $results = $database->find($collection, [ | ||
| Query::containsAny('tags', ['tag_1']), | ||
| ]); | ||
| $this->assertCount($expectedCount, $results); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file and surrounding context
cat -n tests/e2e/Adapter/Scopes/IndexTests.php | sed -n '1170,1210p'Repository: utopia-php/database
Length of output: 1889
🏁 Script executed:
# Find the Database class and its find() method signature
fd "Database.php" --type f | grep -v node_modules | head -5Repository: utopia-php/database
Length of output: 88
🏁 Script executed:
# Look for the default limit in Database class
rg -n "DEFAULT_LIMIT|limit.*=.*25|getLimitForQuery" --type php | head -20Repository: utopia-php/database
Length of output: 1538
🏁 Script executed:
# Read broader test context to see where $total is set
cat -n tests/e2e/Adapter/Scopes/IndexTests.php | sed -n '1100,1210p'Repository: utopia-php/database
Length of output: 5222
🏁 Script executed:
# Find the test method definition to understand the full scope
rg -n "function test.*\{|public function" tests/e2e/Adapter/Scopes/IndexTests.php | grep -B5 -A5 "1100\|1150\|1200"Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Verify that Query::limit() is available and works correctly
rg -n "class Query|public static function limit" --type php | head -20Repository: utopia-php/database
Length of output: 374
🏁 Script executed:
# Check if there are any existing calls in the test file using Query::limit()
rg -n "Query::limit" tests/e2e/Adapter/Scopes/IndexTests.phpRepository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Check how Query::limit() is used in other parts of the codebase/tests
rg -n "find\(.*Query::limit" tests/ --type php -A2 -B2 | head -30Repository: utopia-php/database
Length of output: 2482
find() default limit will cause assertCount to fail on the Postgres/GIN path.
When getSupportForGinIndex() is true, $total = 5000 and $expectedCount = 100, but Database::find() applies a default limit of 25. The assertion assertCount(100, $results) will fail because the result set is capped at 25 items.
The MySQL path (500 documents, $expectedCount = 10) is unaffected since 10 is within the default limit.
Add an explicit Query::limit($expectedCount) to override the default:
Proposed fix
$results = $database->find($collection, [
Query::containsAny('tags', ['tag_1']),
+ Query::limit($expectedCount),
]);
$this->assertCount($expectedCount, $results);📝 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.
| // Verify the query also returns correct functional results | |
| $expectedCount = $total / 50; // each tag_N appears once per 50 documents | |
| $results = $database->find($collection, [ | |
| Query::containsAny('tags', ['tag_1']), | |
| ]); | |
| $this->assertCount($expectedCount, $results); | |
| // Verify the query also returns correct functional results | |
| $expectedCount = $total / 50; // each tag_N appears once per 50 documents | |
| $results = $database->find($collection, [ | |
| Query::containsAny('tags', ['tag_1']), | |
| Query::limit($expectedCount), | |
| ]); | |
| $this->assertCount($expectedCount, $results); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Adapter/Scopes/IndexTests.php` around lines 1199 - 1204, The test
fails on the Postgres/GIN path because Database::find() applies a default limit
(25), so when getSupportForGinIndex() yields $total = 5000 and $expectedCount =
100 the result in $results is truncated; update the query passed to
Database::find() to include an explicit limit by adding
Query::limit($expectedCount) alongside Query::containsAny('tags', ['tag_1']) so
the find call returns the full expectedCount before asserting
$this->assertCount($expectedCount, $results).
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests