Feat relationship updates #724
Conversation
…improvements Co-authored-by: jakeb994 <jakeb994@gmail.com>
- Update batch methods to use Document->getAttribute() instead of array access - Fix shouldSkipRelationshipFetchBatch to accept Document parameter - Convert Document to array when adding to relationship fetch stack - Maintain compatibility with existing relationship fetch stack structure
- Use array access syntax (['key']) instead of getAttribute() - Matches the original implementation pattern - Always use batch processing for all document counts - Should resolve relationship population issues in tests
- Add batchRelationshipProcessing flag to prevent infinite recursion - When already in batch processing, fall back to original single-document method - This should resolve issues with nested relationship queries returning empty arrays
- Use batch processing only for top-level calls (relationshipFetchDepth === 1) - Use original single-document processing for nested relationship calls - Remove custom flag and rely on existing relationshipFetchDepth system - This should resolve missing relationship data in nested scenarios
- Use empty relationshipFetchStack to detect top-level calls for batch processing - Use non-empty stack to detect nested calls that should use single processing - This should properly distinguish between top-level and nested relationship calls - Should resolve issues with one-to-many and other relationship types returning empty arrays
- Remove conditional logic that mixed batch and single processing - Always use batch processing for all relationship population - Eliminates N+1 problems at all nesting levels - Simplifies logic and ensures consistent behavior - Maximum performance benefits across all relationship scenarios
- Convert getDocument to use populateDocumentsRelationships with single-element array - Convert createDocument to use batch processing for relationships - Convert updateDocument to use batch processing for relationships - This ensures consistent batch processing at all levels, including nested relationships - Should resolve 2nd level relationship population issues
…e original method
…ping - Fixed TypeError: Cannot access offset of type Document in isset - Handle cases where twoWayKey returns Document objects instead of string IDs - Extract ID from Document objects when using as array keys - Re-enabled cycle detection after fixing core issue
…plex cycle detection is causing issues
…ument object equality - Replace Document == comparison with specific property matching - Compare key, collection, and relatedCollection for reflexive detection - Should fix cycle detection issues while preventing infinite recursion
- Complex cycle detection was over-blocking legitimate relationships - Simpler approach showed better results (8 errors vs 15 errors) - Focus on max depth limit for two-way relationships only - Keep back-reference removal as in original implementation
- Replace depth-first cycle detection with collection-level cycle detection - Track collection→collection paths instead of document-level relationships - Detect direct cycles (A→B, B→A) and indirect cycles (A→B→A) - Designed specifically for batch/breadth-first relationship traversal - Should prevent infinite loops while allowing legitimate nested relationships
- Only block exact same relationship (same key + collections) being processed - Only block direct back-references for two-way relationships - Remove overly broad collection-level blocking that prevented legitimate relationships - Should allow multiple different relationships to same collection
…essing - Remove complex collection-level cycle tracking that was over-blocking relationships - Use simple depth-based limits only: max depth for all, max-1 for two-way - Should allow legitimate nested relationships like veterinarians→presidents→animals - Still prevents infinite recursion through depth limits
Per user insight: In breadth-first batch processing: - Depth control prevents infinite recursion (process by levels) - Map prevents duplicate fetches (no need to track cycles) - Level-by-level processing can't create infinite loops Testing if cycle detection is unnecessary in batch scenario.
- Remove all cycle detection logic, rely only on depth control - Add map tracking to all batch methods to prevent duplicate fetches - Test theory: level-by-level processing + depth control = no infinite recursion possible - Should allow maximum relationship population with minimal restrictions
Your insight implemented: - Use map to check if relationships already processed BEFORE collecting IDs - Skip duplicate relationship processing automatically - No cycle detection needed - map + depth control is sufficient - Should eliminate redundant database queries and improve performance significantly
…ed deduplication - Skip one-to-one relationships already populated (value is Document object) - Skip array relationships already populated (array contains Document objects) - Map prevents duplicate processing across calls - No cycle detection needed - breadth-first + depth control + smart skipping - Should achieve maximum efficiency with minimal redundant work
- Add debug output in testZoo to see presidents directly - Add logging in one-to-one batch method to track ID collection and fetching - Add logging to main relationship processing loop - Should help identify why some relationships populate and others don't
- Add targeted debugging for 'animal' relationship key - Should show in test output when presidents->animal relationships are processed - Will help identify if IDs are collected and if animals are found
…ne-to-one Root cause found: When multiple animals reference same president (biden), the second animal was overwriting the first in documentsByRelatedId. Changes: - Track arrays of documents per related ID instead of single document - Set relationship for ALL documents that reference the same related ID - Should fix inconsistent relationship population (iguana vs tiger)
- Keep general error logging but remove targeted var_dumps - One-to-one fix applied, ready to tackle remaining issues - Errors reduced from 13 to 10, making progress
- Add error_log statements to track query processing - Debug nested selections flow - Track queries passed to batch methods - Monitor find() calls in relationship population
- Create testSimpleRelationshipPopulation for isolated debugging - Test basic one-to-many user->posts relationship - Add debug output to trace relationship population flow
- Add debug logs for map blocking behavior - Debug skip logic for already-populated relationships - Track relationship assignment in one-to-many - Temporarily force-include IDs that would be blocked by map - Monitor existing values during processing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/tasks/load.php (1)
120-201: Respect the requested document limit.
createDocuments()always inserts 1 000 records, and the loop runs$limit / 1000times. For any non-multiple of 1 000 (e.g., 500, 1 500) the task now inserts more data than requested, so the CLI parameter is effectively broken. Track the remaining count and pass the desired batch size intocreateDocuments()so we never overshoot.Apply this diff:
- for ($i = 0; $i < $limit / 1000; $i++) { + $remaining = $limit; + while ($remaining > 0) { + $batchCount = \min(1000, $remaining); try { - $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) + $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) ->setDatabase($name) ->setNamespace($namespace) ->setSharedTables($sharedTables); - createDocuments($database); + createDocuments($database, $batchCount); } catch (\Throwable $error) { Console::error('Coroutine error: ' . $error->getMessage()); } - } + $remaining -= $batchCount; + } @@ -function createDocuments(Database $database): void +function createDocuments(Database $database, int $count = 1000): void { global $namesPool, $genresPool, $tagsPool; $documents = []; $start = \microtime(true); - for ($i = 0; $i < 1000; $i++) { + for ($i = 0; $i < $count; $i++) { $length = \mt_rand(1000, 4000); $bytes = \random_bytes(intdiv($length + 1, 2)); $text = \substr(\bin2hex($bytes), 0, $length); $tagCount = \mt_rand(1, count($tagsPool)); @@ - Console::info("Prepared 1000 documents in {$time} seconds"); + Console::info("Prepared {$count} documents in {$time} seconds"); $start = \microtime(true); - $database->createDocuments('articles', $documents, 1000); + $database->createDocuments('articles', $documents, $count); $time = \microtime(true) - $start; - Console::success("Inserted 1000 documents in {$time} seconds"); + Console::success("Inserted {$count} documents in {$time} seconds"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bin/cli.php(2 hunks)bin/relationships(1 hunks)bin/tasks/index.php(1 hunks)bin/tasks/load.php(2 hunks)bin/tasks/query.php(3 hunks)bin/tasks/relationships.php(1 hunks)bin/view/index.php(6 hunks)src/Database/Database.php(13 hunks)src/Database/Validator/Queries/Documents.php(1 hunks)src/Database/Validator/Query/Filter.php(4 hunks)src/Database/Validator/Query/Order.php(1 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(2 hunks)tests/unit/Validator/Query/FilterTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
tests/unit/Validator/Query/FilterTest.php (1)
src/Database/Validator/Query/Filter.php (2)
Filter(15-344)getMaxValuesCount(335-338)
bin/tasks/query.php (3)
src/Database/Adapter.php (4)
Adapter(16-1318)setDatabase(145-150)setNamespace(92-97)setSharedTables(174-179)src/Database/Database.php (4)
Database(37-7895)setDatabase(762-767)setNamespace(735-740)setSharedTables(1023-1028)src/Database/Adapter/SQL.php (1)
getPDOAttributes(1739-1749)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
src/Database/Database.php (3)
Database(37-7895)createDocument(4176-4270)find(6695-6826)
src/Database/Validator/Query/Filter.php (1)
src/Database/Database.php (1)
Database(37-7895)
src/Database/Database.php (5)
src/Database/Mirror.php (1)
silent(172-175)src/Database/Document.php (9)
Document(12-470)getAttribute(224-231)setAttribute(244-261)getId(63-66)removeAttribute(287-293)getCollection(85-88)isEmpty(396-399)find(304-322)getArrayCopy(423-458)src/Database/Query.php (10)
getAttribute(159-162)setAttribute(200-205)Query(8-1116)equal(435-438)limit(623-626)getMethod(151-154)getValues(167-170)offset(634-637)groupByType(824-906)select(580-583)src/Database/Adapter/SQL.php (2)
find(2334-2513)count(2525-2587)src/Database/Validator/Authorization.php (2)
Authorization(7-225)skip(160-170)
bin/tasks/load.php (3)
src/Database/Adapter.php (6)
Adapter(16-1318)setDatabase(145-150)setNamespace(92-97)setSharedTables(174-179)createDocuments(713-713)create(488-488)src/Database/Database.php (6)
Database(37-7895)setDatabase(762-767)setNamespace(735-740)setSharedTables(1023-1028)createDocuments(4286-4385)create(1241-1260)src/Database/DateTime.php (2)
DateTime(7-86)now(19-23)
bin/tasks/index.php (4)
src/Database/Database.php (2)
Database(37-7895)setSharedTables(1023-1028)src/Database/Adapter.php (2)
Adapter(16-1318)setSharedTables(174-179)src/Database/PDO.php (1)
PDO(13-143)src/Database/Adapter/SQL.php (1)
getPDOAttributes(1739-1749)
bin/tasks/relationships.php (6)
src/Database/Database.php (10)
Database(37-7895)create(1241-1260)createCollection(1328-1462)createAttribute(1692-1747)createRelationship(2656-2833)createDocuments(4286-4385)skipRelationships(653-663)findOne(6894-6909)getDocument(3445-3566)find(6695-6826)src/Database/DateTime.php (2)
DateTime(7-86)now(19-23)src/Database/Helpers/Permission.php (3)
Permission(9-264)read(186-195)update(220-229)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/Query.php (3)
Query(8-1116)limit(623-626)cursorAfter(645-648)src/Database/Document.php (1)
getId(63-66)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/RelationshipTests.php
431-431: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
437-437: Avoid unused local variables such as '$post1'. (undefined)
(UnusedLocalVariable)
444-444: Avoid unused local variables such as '$post2'. (undefined)
(UnusedLocalVariable)
3044-3044: Avoid unused local variables such as '$author1'. (undefined)
(UnusedLocalVariable)
3051-3051: Avoid unused local variables such as '$author2'. (undefined)
(UnusedLocalVariable)
3125-3125: Avoid unused local variables such as '$user1'. (undefined)
(UnusedLocalVariable)
3131-3131: Avoid unused local variables such as '$profile1'. (undefined)
(UnusedLocalVariable)
3173-3173: Avoid unused local variables such as '$userA'. (undefined)
(UnusedLocalVariable)
3179-3179: Avoid unused local variables such as '$comment1'. (undefined)
(UnusedLocalVariable)
3186-3186: Avoid unused local variables such as '$comment2'. (undefined)
(UnusedLocalVariable)
3227-3227: Avoid unused local variables such as '$student1'. (undefined)
(UnusedLocalVariable)
3233-3233: Avoid unused local variables such as '$course1'. (undefined)
(UnusedLocalVariable)
3690-3690: Avoid unused local variables such as '$dev1'. (undefined)
(UnusedLocalVariable)
3697-3697: Avoid unused local variables such as '$dev2'. (undefined)
(UnusedLocalVariable)
3705-3705: Avoid unused local variables such as '$project1'. (undefined)
(UnusedLocalVariable)
3714-3714: Avoid unused local variables such as '$project2'. (undefined)
(UnusedLocalVariable)
bin/tasks/relationships.php
438-438: Avoid unused local variables such as '$total'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (4)
bin/cli.php (1)
17-22: Thanks for bubbling CLI errors to Console::error.src/Database/Validator/Query/Order.php (1)
31-40: Nice touch supporting dotted attributes.tests/unit/Validator/Query/FilterTest.php (1)
47-50: Good call switching the test to Filter and using the dynamic max.src/Database/Validator/Query/Filter.php (1)
31-338: Filter updates look solid—thanks for exposinggetMaxValuesCount()and relaxing nested relationship validation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Add test asserting numeric ID stays string
# Conflicts: # src/Database/Database.php # tests/e2e/Adapter/Scopes/RelationshipTests.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Database.php (1)
7885-8054: Restore original query indices when rewriting dot-path filtersLike the earlier review noted, iterating with
Query::groupByType($queries)['filters']gives you cloned, reindexed filters. The$indexvalues you record no longer correspond to the positions in$queries, sounset($queries[$index])will happily drop unrelated clauses (often the caller’sselect) instead of the rewritten dot-path filter. That regresses projections and still leaves the original relationship filter in place. Preserve the original keys by iterating over$queriesdirectly and skipping selects/non-dot attributes.Apply this diff:
- foreach (Query::groupByType($queries)['filters'] as $index => $query) { + foreach ($queries as $index => $query) { + if ($query->getMethod() === Query::TYPE_SELECT) { + continue; + } $method = $query->getMethod(); $attribute = $query->getAttribute(); if (!\str_contains($attribute, '.')) { continue; } @@ - $groupedQueries[$relationshipKey]['indices'][] = $index; + $groupedQueries[$relationshipKey]['indices'][] = $index;
🧹 Nitpick comments (1)
src/Database/Database.php (1)
7946-7955: Drop unused$deepestQuery
$deepestQueryis assigned but never read, triggering PHPMD’sUnusedLocalVariable. Removing it keeps the new helper tidy.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Database.php(13 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.543Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.543Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
🧬 Code graph analysis (3)
tests/e2e/Adapter/Scopes/DocumentTests.php (7)
src/Database/Database.php (6)
getDatabase(777-780)createCollection(1328-1462)createAttribute(1692-1747)Database(37-8114)createDocument(4176-4270)getDocument(3445-3566)src/Database/Adapter.php (5)
getDatabase(160-163)createCollection(525-525)createAttribute(557-557)createDocument(701-701)getDocument(691-691)src/Database/Adapter/MariaDB.php (2)
createCollection(76-225)createDocument(822-933)src/Database/Adapter/Postgres.php (3)
createCollection(191-333)createAttribute(444-463)createDocument(958-1058)src/Database/Adapter/SQLite.php (2)
createCollection(141-227)createDocument(522-630)src/Database/Adapter/SQL.php (2)
createAttribute(234-248)getDocument(351-417)src/Database/Document.php (3)
Document(12-470)getId(63-66)getAttribute(224-231)
src/Database/Database.php (5)
src/Database/Mirror.php (1)
silent(172-175)src/Database/Document.php (8)
Document(12-470)getAttribute(224-231)setAttribute(244-261)getId(63-66)removeAttribute(287-293)isEmpty(396-399)find(304-322)getArrayCopy(423-458)src/Database/Query.php (10)
getAttribute(159-162)setAttribute(200-205)Query(8-1116)equal(435-438)limit(623-626)getMethod(151-154)getValues(167-170)offset(634-637)select(580-583)groupByType(824-906)src/Database/Adapter.php (2)
find(802-802)count(825-825)src/Database/Adapter/SQL.php (2)
find(2334-2513)count(2525-2587)
tests/e2e/Adapter/Scopes/RelationshipTests.php (3)
src/Database/Database.php (5)
createCollection(1328-1462)Database(37-8114)createDocument(4176-4270)find(6695-6826)updateDocument(4722-4923)src/Database/Document.php (5)
Document(12-470)getAttribute(224-231)find(304-322)getId(63-66)setAttribute(244-261)src/Database/Query.php (11)
getAttribute(159-162)setAttribute(200-205)Query(8-1116)equal(435-438)lessThan(459-462)notEqual(447-450)lessThanEqual(471-474)greaterThan(483-486)greaterThanEqual(495-498)contains(507-510)limit(623-626)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 7678-7678: PHPStan error: Method Utopia\Database\Database::processNestedRelationshipPath() return type has no value type specified in iterable type array. See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
🪛 PHPMD (2.15.0)
src/Database/Database.php
7948-7948: Avoid unused local variables such as '$deepestQuery'. (undefined)
(UnusedLocalVariable)
tests/e2e/Adapter/Scopes/RelationshipTests.php
431-431: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
437-437: Avoid unused local variables such as '$post1'. (undefined)
(UnusedLocalVariable)
444-444: Avoid unused local variables such as '$post2'. (undefined)
(UnusedLocalVariable)
3044-3044: Avoid unused local variables such as '$author1'. (undefined)
(UnusedLocalVariable)
3051-3051: Avoid unused local variables such as '$author2'. (undefined)
(UnusedLocalVariable)
3125-3125: Avoid unused local variables such as '$user1'. (undefined)
(UnusedLocalVariable)
3131-3131: Avoid unused local variables such as '$profile1'. (undefined)
(UnusedLocalVariable)
3173-3173: Avoid unused local variables such as '$userA'. (undefined)
(UnusedLocalVariable)
3179-3179: Avoid unused local variables such as '$comment1'. (undefined)
(UnusedLocalVariable)
3186-3186: Avoid unused local variables such as '$comment2'. (undefined)
(UnusedLocalVariable)
3227-3227: Avoid unused local variables such as '$student1'. (undefined)
(UnusedLocalVariable)
3233-3233: Avoid unused local variables such as '$course1'. (undefined)
(UnusedLocalVariable)
3690-3690: Avoid unused local variables such as '$dev1'. (undefined)
(UnusedLocalVariable)
3697-3697: Avoid unused local variables such as '$dev2'. (undefined)
(UnusedLocalVariable)
3705-3705: Avoid unused local variables such as '$project1'. (undefined)
(UnusedLocalVariable)
3714-3714: Avoid unused local variables such as '$project2'. (undefined)
(UnusedLocalVariable)
3843-3843: Avoid unused local variables such as '$dept1'. (undefined)
(UnusedLocalVariable)
3850-3850: Avoid unused local variables such as '$dept2'. (undefined)
(UnusedLocalVariable)
3857-3857: Avoid unused local variables such as '$company1'. (undefined)
(UnusedLocalVariable)
3864-3864: Avoid unused local variables such as '$company2'. (undefined)
(UnusedLocalVariable)
3871-3871: Avoid unused local variables such as '$employee1'. (undefined)
(UnusedLocalVariable)
3880-3880: Avoid unused local variables such as '$employee2'. (undefined)
(UnusedLocalVariable)
3889-3889: Avoid unused local variables such as '$project1'. (undefined)
(UnusedLocalVariable)
3897-3897: Avoid unused local variables such as '$project2'. (undefined)
(UnusedLocalVariable)
3905-3905: Avoid unused local variables such as '$task1'. (undefined)
(UnusedLocalVariable)
3914-3914: Avoid unused local variables such as '$task2'. (undefined)
(UnusedLocalVariable)
3923-3923: Avoid unused local variables such as '$task3'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (9)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
333-360: Good coverage for numeric-string IDsThanks for adding an end-to-end case that proves digit-only IDs survive the create/get path without being coerced. This should help prevent regressions on adapters that might otherwise cast to integers. Nice work.
tests/e2e/Adapter/Scopes/RelationshipTests.php (8)
404-474: LGTM! Well-structured basic relationship test.This test provides good coverage of simple one-to-many relationship population in both directions (parent → children and child → parent). The assertions properly verify that relationships are populated as Document instances with the expected attributes.
2880-3010: LGTM! Critical test for depth-handling bug fix.This test thoroughly validates that nested document creation properly populates relationships at all depths. The comprehensive docblock clearly explains the bug scenario, and the test assertions at lines 2965-2971 directly verify that the product's store relationship is populated even though the product was created at depth 1. Testing both create and update flows provides good coverage.
3015-3257: LGTM! Comprehensive relationship filtering test.This test provides excellent coverage of dot-path filtering across all four relationship types (ONE_TO_MANY, ONE_TO_ONE, MANY_TO_ONE, MANY_TO_MANY). Each relationship type is tested in isolation with proper cleanup, and the test verifies filtering works in both directions using various query operators.
3262-3427: LGTM! Thorough query operator coverage.This test systematically validates all query operators (equal, notEqual, comparison operators, string operators, boolean) work correctly on relationship fields using dot-path notation. The final combined-conditions test at lines 3416-3422 also verifies query grouping optimization for multiple filters on the same relationship.
3432-3546: LGTM! Solid edge case coverage.This test covers important edge cases including no matches, impossible conditions, non-existent fields, empty arrays, null relationships, and combining relationship queries with regular queries. The try-catch blocks properly handle expected exceptions.
Minor observation: The assertion at line 3510 expects a specific error message containing "at least one value" which might be adapter-specific. If this becomes flaky across adapters, consider making the assertion more generic.
3551-3655: LGTM! Parent-side relationship query test.This test validates querying from the parent side of a ONE_TO_MANY relationship using virtual fields, which is a key feature for filtering collections by nested member attributes. The test scenarios cover various combinations of member properties and properly verify the results.
3660-3759: LGTM! Complex many-to-many relationship tests.This test provides comprehensive coverage of MANY_TO_MANY relationship queries with complex filtering. It tests both directions of the relationship and includes combined query conditions, validating that the junction table logic works correctly for multi-attribute filtering scenarios.
3761-4019: LGTM! Excellent deep nesting test coverage.This is the most comprehensive test in the suite, validating that relationship queries work correctly across multiple depth levels (up to 3 levels deep). The test systematically covers depth 1, 2, and 3 queries, including mixed MANY_TO_ONE relationships within the chain. The combined depth tests at the end verify that the query resolution works correctly when mixing different depths in the same query. This thoroughly exercises the
processNestedRelationshipPathfunctionality described in the code snippets.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Database.php (1)
7885-8050: Still unsetting the wrong query entries
We’re again iterating overQuery::groupByType($queries)['filters'], stashing its auto-reindexed offsets, and laterunset($queries[$index]). That index now points into the original array, so if a caller mixesQuery::select(...)with a dot-path filter, we strip the select instead of the rewritten filter, letting the originalauthor.namepredicate leak through. This is the exact bug we discussed earlier—the selects disappear and results become incorrect. Please iterate the original$querieswhile preserving their keys (skipping TYPE_SELECT entries) so the removal targets only the rewritten filters, then keep using those original keys in$indicesToRemove.
🧹 Nitpick comments (1)
src/Database/Database.php (1)
7948-7955: Drop the unused$deepestQuerytemp
$deepestQueryis assigned and never read, which PHPMD is flagging. Remove it or make use of it to keep the method clean.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (5)
src/Database/Mirror.php (1)
silent(172-175)src/Database/Document.php (9)
Document(12-470)getAttribute(224-231)setAttribute(244-261)getId(63-66)removeAttribute(287-293)getCollection(85-88)isEmpty(396-399)find(304-322)getArrayCopy(423-458)src/Database/Query.php (10)
getAttribute(159-162)setAttribute(200-205)Query(8-1116)equal(435-438)limit(623-626)getMethod(151-154)getValues(167-170)offset(634-637)select(580-583)groupByType(824-906)src/Database/Adapter/SQL.php (2)
find(2334-2513)count(2525-2587)src/Database/Adapter.php (2)
find(802-802)count(825-825)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 7678-7678: PHPStan: Method Utopia\Database\Database::processNestedRelationshipPath() return type has no value type specified in iterable type array.
🪛 PHPMD (2.15.0)
src/Database/Database.php
7948-7948: Avoid unused local variables such as '$deepestQuery'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bin/tasks/load.php (1)
71-82: Fix nested array key causing parse error.Lines 71-82 have a duplicate nested
'postgres'key that creates invalid syntax and causes the PSR-12 parse error reported in the pipeline. The outer'postgres'key should directly contain the configuration array, not another'postgres'key.Apply this diff:
'postgres' => [ - 'postgres' => [ - 'host' => 'postgres', - 'port' => 5432, - 'user' => 'postgres', - 'pass' => 'password', - 'dsn' => static fn (string $host, int $port) => "pgsql:host={$host};port={$port}", - 'driver' => 'pgsql', - 'adapter' => Postgres::class, - 'attrs' => Postgres::getPDOAttributes(), - ], + 'host' => 'postgres', + 'port' => 5432, + 'user' => 'postgres', + 'pass' => 'password', + 'dsn' => static fn (string $host, int $port) => "pgsql:host={$host};port={$port}", + 'driver' => 'pgsql', + 'adapter' => Postgres::class, + 'attrs' => Postgres::getPDOAttributes(), ],Note: This fix also addresses the postgres credentials issue flagged in the previous review.
🧹 Nitpick comments (4)
bin/tasks/load.php (4)
7-7: Remove unused import.The
Swoole\Runtimeimport is not used, as theenableCoroutine()call at line 48 is commented out.Apply this diff:
-use Swoole\Runtime;
107-117: Remove unused PDOPool.The
PDOPoolis created but never used, as the$pool->get()call at line 124 is commented out and the single$pdoinstance is reused instead.Apply this diff:
- $pool = new PDOPool( - (new PDOConfig()) - ->withDriver($cfg['driver']) - ->withHost($cfg['host']) - ->withPort($cfg['port']) - ->withDbName($name) - //->withCharset('utf8mb4') - ->withUsername($cfg['user']) - ->withPassword($cfg['pass']), - 128 - ); -
189-189: Consider varying document timestamps.All 1000 documents in each batch will have identical
createdtimestamps sinceDateTime::now()is called once outside the loop. For more realistic test data, consider calling it inside the loop or adding random offsets.+ $createdOffset = mt_rand(-86400 * 30, 0); // Random offset up to 30 days ago + $created = date('Y-m-d\TH:i:s.v\Z', time() + $createdOffset); $documents[] = new Document([ '$permissions' => [ Permission::read(Role::any()), ...array_map(fn () => Permission::read(Role::user(mt_rand(0, 999999999))), range(1, 4)), ...array_map(fn () => Permission::create(Role::user(mt_rand(0, 999999999))), range(1, 3)), ...array_map(fn () => Permission::update(Role::user(mt_rand(0, 999999999))), range(1, 3)), ...array_map(fn () => Permission::delete(Role::user(mt_rand(0, 999999999))), range(1, 3)), ], 'author' => $namesPool[\array_rand($namesPool)], - 'created' => DateTime::now(), + 'created' => $created,
199-199: Add error handling for document insertion.The
createDocumentscall at line 199 has no error handling. If the insertion fails, the script will terminate without proper cleanup or reporting.Console::info("Prepared {$count} documents in {$time} seconds"); $start = \microtime(true); - $database->createDocuments('articles', $documents, 1000); + try { + $database->createDocuments('articles', $documents, 1000); + } catch (\Throwable $e) { + Console::error("Failed to insert documents: " . $e->getMessage()); + throw $e; + } $time = \microtime(true) - $start;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/tasks/load.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bin/tasks/load.php (3)
src/Database/Adapter.php (6)
Adapter(16-1318)setDatabase(145-150)setNamespace(92-97)setSharedTables(174-179)createDocuments(713-713)create(488-488)src/Database/Database.php (6)
Database(37-8104)setDatabase(762-767)setNamespace(735-740)setSharedTables(1023-1028)createDocuments(4286-4385)create(1241-1260)src/Database/DateTime.php (2)
DateTime(7-86)now(19-23)
🪛 GitHub Actions: Linter
bin/tasks/load.php
[error] 1-1: PSR-12: Parse error in bin/tasks/load.php: syntax error, unexpected token ';' (parse error shown in lint step).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Unit Test
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (3)
bin/tasks/load.php (3)
25-29: LGTM!The global pools for document generation are well-sized and will improve performance by avoiding repeated allocations.
143-163: LGTM!The schema creation logic is correct and comprehensive.
121-137: Fix incorrect batch calculation.Line 121 uses integer division (
$limit / 1000) which truncates the result. If$limitis 1500, only one batch of 1000 documents is created instead of two batches totaling 2000 documents. If$limitis less than 1000, no documents are created at all.Apply this diff:
- for ($i = 0; $i < $limit / 1000; $i++) { + for ($i = 0; $i < ceil($limit / 1000); $i++) {Alternatively, to create exactly
$limitdocuments:- for ($i = 0; $i < $limit / 1000; $i++) { + $batchSize = 1000; + for ($i = 0; $i < ceil($limit / $batchSize); $i++) { //\go(function () use ($cfg, $pool, $name, $namespace, $sharedTables, $cache) { try { //$pdo = $pool->get(); $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) ->setDatabase($name) ->setNamespace($namespace) ->setSharedTables($sharedTables); - createDocuments($database); + $remaining = $limit - ($i * $batchSize); + $currentBatch = min($remaining, $batchSize); + createDocuments($database, $currentBatch);And update the
createDocumentssignature to accept the batch size parameter.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bin/tasks/load.php (2)
37-38: Strengthen parameter validation (duplicate concern).The validators remain too permissive:
adapterallows empty strings withText(0)limitallows negative values withInteger(true)This was flagged in a previous review but not yet addressed.
Apply this diff:
- ->param('adapter', '', new Text(0), 'Database adapter') - ->param('limit', 0, new Integer(true), 'Total number of records to add to database') + ->param('adapter', '', new Text(1), 'Database adapter') + ->param('limit', 1, new Integer(false), 'Total number of records to add to database')
166-203: Make batch size configurable (duplicate concern).The function still hardcodes 1000 documents throughout, which was flagged in a previous review. This prevents the function from handling variable batch sizes (e.g., remainders when
$limitis not a multiple of 1000).Apply this diff:
-function createDocuments(Database $database): void +function createDocuments(Database $database, int $count = 1000): void { global $namesPool, $genresPool, $tagsPool; $documents = []; $start = \microtime(true); - for ($i = 0; $i < 1000; $i++) { + for ($i = 0; $i < $count; $i++) { $length = \mt_rand(1000, 4000); $bytes = \random_bytes(intdiv($length + 1, 2)); $text = \substr(\bin2hex($bytes), 0, $length); $tagCount = \mt_rand(1, count($tagsPool)); $tagKeys = (array) \array_rand($tagsPool, $tagCount); $tags = \array_map(fn ($k) => $tagsPool[$k], $tagKeys); $documents[] = new Document([ '$permissions' => [ Permission::read(Role::any()), ...array_map(fn () => Permission::read(Role::user(mt_rand(0, 999999999))), range(1, 4)), ...array_map(fn () => Permission::create(Role::user(mt_rand(0, 999999999))), range(1, 3)), ...array_map(fn () => Permission::update(Role::user(mt_rand(0, 999999999))), range(1, 3)), ...array_map(fn () => Permission::delete(Role::user(mt_rand(0, 999999999))), range(1, 3)), ], 'author' => $namesPool[\array_rand($namesPool)], 'created' => DateTime::now(), 'text' => $text, 'genre' => $genresPool[\array_rand($genresPool)], 'views' => \mt_rand(0, 999999), 'tags' => $tags, ]); } $time = \microtime(true) - $start; - Console::info("Prepared 1000 documents in {$time} seconds"); + Console::info("Prepared {$count} documents in {$time} seconds"); $start = \microtime(true); - $database->createDocuments('articles', $documents, 1000); + $database->createDocuments('articles', $documents, $count); $time = \microtime(true) - $start; - Console::success("Inserted 1000 documents in {$time} seconds"); + Console::success("Inserted {$count} documents in {$time} seconds"); }
🧹 Nitpick comments (3)
bin/tasks/load.php (3)
7-7: Remove unused import.The
Swoole\Runtimeimport is not used sinceRuntime::enableCoroutine()is commented out at line 48. Consider removing this import to keep the code clean.Apply this diff:
-use Swoole\Runtime;
26-28: Consider using constants instead of globals.While global variables work for this CLI script, defining these pools as constants would be more maintainable and avoid potential scope issues.
Apply this diff:
-// Global pools for faster document generation -$namesPool = ['Alice', 'Bob', 'Carol', 'Dave', 'Eve', 'Frank', 'Grace', 'Heidi', 'Ivan', 'Judy', 'Mallory', 'Niaj', 'Olivia', 'Peggy', 'Quentin', 'Rupert', 'Sybil', 'Trent', 'Uma', 'Victor']; -$genresPool = ['fashion','food','travel','music','lifestyle','fitness','diy','sports','finance']; -$tagsPool = ['short','quick','easy','medium','hard']; +const NAMES_POOL = ['Alice', 'Bob', 'Carol', 'Dave', 'Eve', 'Frank', 'Grace', 'Heidi', 'Ivan', 'Judy', 'Mallory', 'Niaj', 'Olivia', 'Peggy', 'Quentin', 'Rupert', 'Sybil', 'Trent', 'Uma', 'Victor']; +const GENRES_POOL = ['fashion','food','travel','music','lifestyle','fitness','diy','sports','finance']; +const TAGS_POOL = ['short','quick','easy','medium','hard'];Then update line 168 and usage sites:
- global $namesPool, $genresPool, $tagsPool;And references like
$namesPool[\array_rand($namesPool)]toNAMES_POOL[\array_rand(NAMES_POOL)].
108-118: Remove unused PDOPool.The
PDOPoolis created but never used since all pool operations are commented out (lines 125, 133). Consider removing this code block to avoid confusion.Apply this diff:
- $pool = new PDOPool( - (new PDOConfig()) - ->withDriver($cfg['driver']) - ->withHost($cfg['host']) - ->withPort($cfg['port']) - ->withDbName($name) - //->withCharset('utf8mb4') - ->withUsername($cfg['user']) - ->withPassword($cfg['pass']), - 128 - ); -
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/tasks/load.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bin/tasks/load.php (5)
src/Database/Database.php (8)
Database(37-8104)setDatabase(762-767)setNamespace(735-740)setSharedTables(1023-1028)createDocuments(4286-4385)count(6923-6959)create(1241-1260)delete(1299-1313)src/Database/DateTime.php (2)
DateTime(7-86)now(19-23)src/Database/Document.php (1)
Document(12-470)src/Database/Helpers/Permission.php (3)
Permission(9-264)read(186-195)update(220-229)src/Database/Helpers/Role.php (3)
Role(5-178)any(159-162)user(115-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (1)
bin/tasks/load.php (1)
122-122: Integer division truncates non-multiples of 1000.Line 122 uses
$limit / 1000which performs integer division in PHP. If$limitis not a multiple of 1000, the remainder is silently discarded. For example,--limit=1500will only create 1000 documents instead of 1500.Apply this diff to use proper integer division or handle remainders:
- for ($i = 0; $i < $limit / 1000; $i++) { + for ($i = 0; $i < \intdiv($limit, 1000); $i++) {Or better, handle the remainder:
+ $batches = \intdiv($limit, 1000); + $remainder = $limit % 1000; + - for ($i = 0; $i < $limit / 1000; $i++) { + for ($i = 0; $i < $batches; $i++) { //\go(function () use ($cfg, $pool, $name, $namespace, $sharedTables, $cache) { try { //$pdo = $pool->get(); $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) ->setDatabase($name) ->setNamespace($namespace) ->setSharedTables($sharedTables); createDocuments($database); //$pool->put($pdo); } catch (\Throwable $error) { Console::error('Coroutine error: ' . $error->getMessage()); } //}); } + + if ($remainder > 0) { + try { + $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) + ->setDatabase($name) + ->setNamespace($namespace) + ->setSharedTables($sharedTables); + createDocuments($database, $remainder); + } catch (\Throwable $error) { + Console::error('Coroutine error: ' . $error->getMessage()); + } + }Note: This assumes
createDocumentsis updated to accept a count parameter (see separate comment).Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
bin/tasks/load.php (2)
170-170: Make batch size configurable.The function hardcodes 1000 documents, which doesn't align with varying batch sizes. Consider accepting a batch size parameter.
Apply this diff:
-function createDocuments(Database $database): void +function createDocuments(Database $database, int $count = 1000): void { global $namesPool, $genresPool, $tagsPool; $documents = []; $start = \microtime(true); - for ($i = 0; $i < 1000; $i++) { + for ($i = 0; $i < $count; $i++) {And update the logging and insertion calls:
- Console::info("Prepared 1000 documents in {$time} seconds"); + Console::info("Prepared {$count} documents in {$time} seconds"); $start = \microtime(true); - $database->createDocuments('articles', $documents, 1000); + $database->createDocuments('articles', $documents, $count); $time = \microtime(true) - $start; - Console::success("Inserted 1000 documents in {$time} seconds"); + Console::success("Inserted {$count} documents in {$time} seconds");
36-37: Strengthen parameter validation.The
adapterparameter allows empty strings withText(0), andlimitallows negative values withInteger(true). Consider using more restrictive validators:
adaptershould useText(1)to require a non-empty stringlimitshould use a minimum value validator or disallow negativesApply this diff:
- ->param('adapter', '', new Text(0), 'Database adapter') - ->param('limit', 0, new Integer(true), 'Total number of records to add to database') + ->param('adapter', '', new Text(1), 'Database adapter') + ->param('limit', 1, new Integer(false), 'Total number of records to add to database')
🧹 Nitpick comments (1)
bin/tasks/load.php (1)
187-187: All documents in a batch share the same timestamp.Using
DateTime::now()inside the loop (line 187) means all 1000 documents in a batch will have identicalcreatedtimestamps. If timestamp variety is needed for realistic test data, consider incrementing timestamps slightly or sampling from a range.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bin/tasks/load.php(2 hunks)src/Database/Database.php(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
bin/tasks/load.php (3)
src/Database/Database.php (4)
Database(37-8107)setSharedTables(1023-1028)createDocuments(4286-4385)create(1241-1260)src/Database/DateTime.php (2)
DateTime(7-86)now(19-23)src/Database/Adapter/SQL.php (2)
getPDOAttributes(1739-1749)createDocuments(1968-2101)
src/Database/Database.php (6)
src/Database/Mirror.php (1)
silent(172-175)src/Database/Document.php (8)
Document(12-470)getAttribute(224-231)setAttribute(244-261)getId(63-66)removeAttribute(287-293)isEmpty(396-399)find(304-322)getArrayCopy(423-458)src/Database/Query.php (8)
getAttribute(159-162)setAttribute(200-205)Query(8-1116)equal(435-438)limit(623-626)getValues(167-170)offset(634-637)select(580-583)src/Database/Adapter/SQL.php (2)
find(2334-2513)count(2525-2587)src/Database/Adapter.php (2)
find(802-802)count(825-825)src/Database/Validator/Authorization.php (2)
Authorization(7-225)skip(160-170)
🪛 PHPMD (2.15.0)
src/Database/Database.php
7941-7941: Avoid unused local variables such as '$deepestQuery'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (6)
bin/tasks/load.php (6)
24-27: LGTM: Global pools for faster document generation.The global pools provide a reasonable variety of mock data and eliminate the need for repeated initialization. This is an acceptable pattern for CLI scripts.
49-80: LGTM: Unified adapter configuration.The data-driven adapter configuration is well-structured and consistent across all three adapters (mariadb, mysql, postgres). The DSN closures and PDO attributes are appropriately defined.
82-85: LGTM: Adapter validation.The validation logic correctly prevents execution with unsupported adapters and provides a clear error message.
87-96: LGTM: PDO initialization.The PDO initialization correctly uses the adapter configuration. Exception propagation is acceptable for CLI scripts.
98-103: LGTM: Schema creation with sharedTables support.The schema creation correctly propagates the
sharedTablesparameter to the Database instance.
141-161: LGTM: Schema creation function.The schema creation logic correctly sets up the database, collection, and attributes.
Summary by CodeRabbit
New Features
Improvements
Tests