Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds internal subquery support and batched many-to-many relationship resolution across the database layer, extends adapters/Pool with subquery and junction-mapping APIs, implements SQL subquery rendering and junction retrieval, refactors Database for batch casting/subquery paths, and adds M2M benchmarking scripts. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant DB as Database
participant Pool as Adapter/Pool
participant SQL as SQL Engine
participant Junction as JunctionCollection
Note over Client,DB: High-level M2M resolution (subquery vs mapping)
Client->>DB: find(collection, filters with M2M)
DB->>DB: convertRelationshipQueries()
DB->>Pool: getSupportForSubqueries()
alt Pool supports subqueries
DB->>Pool: emit inSubquery / inSubqueryDirect descriptor
Pool->>SQL: execute inline subquery
SQL-->>Pool: matching IDs
Pool-->>DB: subquery-backed condition
else Pool does not support subqueries
DB->>Pool: findJunctionMapping(junction, parentCol, childCol, parentIds)
Pool->>Junction: SELECT junction rows (chunked)
Junction-->>Pool: junction mappings
Pool-->>DB: mapping rows
DB->>DB: build filters from mappings
end
DB->>Pool: final find on target collection
Pool-->>DB: raw documents
DB->>DB: castAndDecodeBatch()
DB-->>Client: returned documents
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 (2)
src/Database/Database.php (2)
9781-9857:⚠️ Potential issue | 🔴 CriticalFix phpstan failures: make
resolveRelationshipGroupToIds()return shapes explicit before indexingids.CodeQL/phpstan is right:
$resultis typed with optionalids, but you index it at Line 9824 and Line 9932. Even though your control-flow intends “non-subquery => ids exist”, the type doesn’t guarantee it.This should be fixed before merge because it’s already breaking CI.
Proposed fix (tighten typing + guard before indexing)
@@ - foreach ($query->getValues() as $value) { + foreach ($query->getValues() as $value) { $relatedQuery = Query::equal($nestedAttribute, [$value]); $result = $this->resolveRelationshipGroupToIds($relationship, [$relatedQuery], $collection); @@ if (isset($result['subquery'])) { $additionalQueries[] = $this->createSubqueryFromResult($result); $usedSubqueries = true; } else { + if (!isset($result['ids'])) { + throw new \LogicException('Expected "ids" for non-subquery relationship resolution result.'); + } $resolvedAttribute = $result['attribute']; $parentIdSets[] = $result['ids']; } } @@ - if (isset($result['subquery'])) { + if (isset($result['subquery'])) { $additionalQueries[] = $this->createSubqueryFromResult($result); } else { + if (!isset($result['ids'])) { + throw new \LogicException('Expected "ids" for non-subquery relationship resolution result.'); + } $additionalQueries[] = Query::equal($result['attribute'], $result['ids']); }Also applies to: 9929-9934
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 9781 - 9857, The phpstan error is caused by indexing $result['ids'] without guaranteeing the 'ids' key exists from resolveRelationshipGroupToIds(); update resolveRelationshipGroupToIds() PHPDoc/return annotation to an explicit shape (e.g. array{ids?: array, attribute?: string, subquery?: array}|null) and add guards wherever $result is used (in the loop inside the method handling $query->getValues()) — specifically before accessing $result['ids'] (replace direct indexing at the spots using $result['ids'] with a check like if (!isset($result['ids'])) { return null; } or handle the subquery case first) so the code only indexes 'ids' when present and phpstan's typing matches the runtime checks.
9997-10176:⚠️ Potential issue | 🟠 MajorSubquery path bypasses
find()conversion/typing — convert relationship queries against the related collection before embedding in SQL.Right now, when
$canUseSubqueryis true you return subquery “conditions” built from$relatedQueries(Line 10063-10066, 10112-10116, 10159-10162), but those$relatedQuerieswere constructed from the parent query set and have not been run throughconvertQueries()using the related collection’s attributes/indexes/type rules.This can change results for at least:
- datetime queries (timezone normalization done in
convertQuery()forVAR_DATETIME)- object nested queries (where
convertQuery()sets attribute type / object handling)- any adapter-specific query typing needed to render correct SQL conditions
You already special-case spatial queries (Line 9997-10015). Datetime/object typing needs similar care, but can likely be handled by converting the query objects rather than disabling subqueries.
Proposed fix (convert queries for related collection when using subqueries)
@@ - $canUseSubquery = $this->adapter->getSupportForSubqueries(); + $canUseSubquery = $this->adapter->getSupportForSubqueries(); if ($canUseSubquery) { @@ foreach ($relatedQueries as $rq) { if (\in_array($rq->getMethod(), $spatialMethods)) { $canUseSubquery = false; break; } } } + + // If we will emit a subquery, ensure conditions are converted/typed against the related collection schema. + // This mirrors the normal find() pipeline for value normalization (e.g. datetime) and attribute typing. + $relatedCollectionDoc = null; + if ($canUseSubquery) { + $relatedCollectionDoc = $this->silent(fn () => $this->getCollection($relatedCollection)); + $relatedQueries = $this->convertQueries($relatedCollectionDoc, $relatedQueries); + } @@ - $relatedCollectionDoc = $this->silent(fn () => $this->getCollection($relatedCollection)); + $relatedCollectionDoc ??= $this->silent(fn () => $this->getCollection($relatedCollection)); $junction = $this->getJunctionCollection($collection, $relatedCollectionDoc, $side);Based on learnings: “Relationship selects are always evaluated in the main alias context… preserve the root of dotted paths and any projection alias…”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 9997 - 10176, The subquery branches embed $relatedQueries directly without running them through convertQueries/convertQuery for the related collection, so convert the related queries before building any subquery conditions: call the conversion helper (e.g. $convertedRelated = $this->convertQueries($relatedCollection, $relatedQueries) or per-query $this->convertQuery(...)) and use $convertedRelated in the three places that return a 'subquery' with 'conditions' (the nested subquery with Query::inSubqueryDirect, the many-to-many junction subquery branch returning 'conditions', and the simple FK resolution branch returning 'conditions') so that datetime/object typing and adapter-specific typing are applied while preserving dotted-path roots/projection aliases.
🧹 Nitpick comments (3)
bin/tasks/relationships.php (1)
461-569: Consider isolating query-filter performance from relationship hydration.
These M2M timings may be dominated by relationship expansion. If the intent is to measure the subquery filter path, wrap the timedfind()calls inskipRelationships()to reduce noise.♻️ Suggested adjustment (pattern example)
- $docs = $database->find('authors', [ - Query::equal('articles.$id', [$singleId]), - Query::limit(5000), - ]); + $docs = $database->skipRelationships(fn () => $database->find('authors', [ + Query::equal('articles.$id', [$singleId]), + Query::limit(5000), + ]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/tasks/relationships.php` around lines 461 - 569, The benchmarkM2M timings include relationship hydration, so wrap the timed and warmup Query::find() calls in $database->skipRelationships(...) to isolate filter/subquery cost: update the blocks that call $database->find(...) for the equal_single, equal_multi, containsAll and reverse_equal tests (both warmup and timed loops) to use $database->skipRelationships(fn () => $database->find(...)) while leaving the initial ID-selection calls as-is; reference the benchmarkM2M function and the Query::equal/Query::containsAll invocations to locate each find() to change.src/Database/Database.php (2)
10179-10206:createSubqueryFromResult()is a nice consolidation; consider hardening the subquery payload schema.Given how dynamic the
$subarray is, it may be worth adding minimal validation (e.g., required keys present) to fail fast with a clearer error if a caller accidentally returns a malformedsubquerypayload (especially as more subquery “modes” get added over time).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 10179 - 10206, The createSubqueryFromResult function trusts a dynamic $sub payload; add minimal validation at the top of createSubqueryFromResult(array $result): Query to assert required keys exist and have expected types before calling Query::inSubqueryDirect or Query::inSubquery. Specifically, validate that $result['attribute'] is set and that $sub contains 'table' and 'column'; if using the 'conditions' branch ensure 'conditions' is present and is an array, and for the other branch ensure 'filterColumn' and 'filterValues' exist (and that 'filterValues' is iterable); throw a clear InvalidArgumentException (or similar) with a message including the missing key(s) so callers fail fast and debugging is easier.
8806-9000:castAndDecodeBatch()is a correctness hotspot — ensure it matchescasting()+decode()semantics across adapters.A few specific risks in this implementation:
castingAfter()is always called (Line 8906) even when$supportsCastingis false; ifcastingAfter()does more than casting, that’s fine, but if it assumes a priorcastingBefore()/DB-specific shape, this could be adapter-sensitive.- The method selectively processes only “work attributes” (Line 8878-8887). That’s great for perf, but it increases the chance that some transformation previously done in
casting()/decode()is skipped for certain types/filters/edge cases (especially where adapters return filtered column names or where attributes have implicit filters).Given how central this is (it now gates all
find()results), I’d strongly recommend adding targeted tests for:
- filtered key fallback (
$this->adapter->filter($key))- array attributes stored as JSON strings
- selections containing dotted paths (relationship/object nested selection patterns)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 8806 - 9000, castAndDecodeBatch can diverge from the original casting()/decode() semantics (castingAfter called unconditionally and selective "workAttributes" may skip transformations), so update it to preserve adapter semantics: only call $this->adapter->castingAfter(...) when $this->adapter->getSupportForCasting() is true or when adapter docs state castingAfter must always run; ensure keys filtered by $this->adapter->filter($key) are handled exactly like casting()/decode() by re-checking attributes omitted from $workAttributes (especially attributes with implicit filters or adapter-mapped names) and by reusing decodeAttribute(...) for all filtered-key fallbacks and JSON-array handling (string->json) so behavior matches decode(); add a clear pass to apply casting rules from getSupportForCasting() and to run decodeAttribute for dotted/relationship selections before createDocumentInstance(...) so selections and relationship renames match previous find() behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/bench_m2m.php`:
- Around line 146-151: The printf call and other multiline function calls
(notably bench(...)) violate PSR-12 method_argument_space by placing multiple
arguments on a single line; update each multiline invocation (e.g., the printf
at the shown diff and all bench(...) calls) so each argument is on its own line,
with the opening parenthesis on the same line as the function name and the
closing parenthesis and semicolon on their own line; ensure alignment matches
PSR-12 (one argument per line) for all affected calls in the file.
In `@src/Database/Adapter.php`:
- Around line 1574-1608: The docblock for findJunctionMapping promises
array<array{string, string}> but getAttribute() returns mixed, causing phpstan
errors; update the method to coerce/validate both values to string before
pushing into $mappings (use the $parentColumn/$childColumn keys and the $results
from find(), e.g. retrieve $doc->getAttribute(...) and cast or explicitly
validate and handle nulls/other types) so the returned arrays always contain
string values, and keep or adjust the docblock accordingly if you choose to
allow nulls instead of casting.
In `@src/Database/Adapter/SQL.php`:
- Around line 2353-2379: The HAVING COUNT(DISTINCT ...) comparison uses the raw
$filterValues length which breaks when inputs contain duplicates; before
building placeholders for the Query::TYPE_IN_SUBQUERY_ALL branch deduplicate and
reindex $filterValues (e.g. array_unique + array_values), handle the case it
becomes empty (return '1 = 0'), then build $valuePlaceholders and $binds from
the deduplicated list and set $binds[$countKey] to count($filterValues) so the
HAVING uses the unique count.
---
Outside diff comments:
In `@src/Database/Database.php`:
- Around line 9781-9857: The phpstan error is caused by indexing $result['ids']
without guaranteeing the 'ids' key exists from resolveRelationshipGroupToIds();
update resolveRelationshipGroupToIds() PHPDoc/return annotation to an explicit
shape (e.g. array{ids?: array, attribute?: string, subquery?: array}|null) and
add guards wherever $result is used (in the loop inside the method handling
$query->getValues()) — specifically before accessing $result['ids'] (replace
direct indexing at the spots using $result['ids'] with a check like if
(!isset($result['ids'])) { return null; } or handle the subquery case first) so
the code only indexes 'ids' when present and phpstan's typing matches the
runtime checks.
- Around line 9997-10176: The subquery branches embed $relatedQueries directly
without running them through convertQueries/convertQuery for the related
collection, so convert the related queries before building any subquery
conditions: call the conversion helper (e.g. $convertedRelated =
$this->convertQueries($relatedCollection, $relatedQueries) or per-query
$this->convertQuery(...)) and use $convertedRelated in the three places that
return a 'subquery' with 'conditions' (the nested subquery with
Query::inSubqueryDirect, the many-to-many junction subquery branch returning
'conditions', and the simple FK resolution branch returning 'conditions') so
that datetime/object typing and adapter-specific typing are applied while
preserving dotted-path roots/projection aliases.
---
Nitpick comments:
In `@bin/tasks/relationships.php`:
- Around line 461-569: The benchmarkM2M timings include relationship hydration,
so wrap the timed and warmup Query::find() calls in
$database->skipRelationships(...) to isolate filter/subquery cost: update the
blocks that call $database->find(...) for the equal_single, equal_multi,
containsAll and reverse_equal tests (both warmup and timed loops) to use
$database->skipRelationships(fn () => $database->find(...)) while leaving the
initial ID-selection calls as-is; reference the benchmarkM2M function and the
Query::equal/Query::containsAll invocations to locate each find() to change.
In `@src/Database/Database.php`:
- Around line 10179-10206: The createSubqueryFromResult function trusts a
dynamic $sub payload; add minimal validation at the top of
createSubqueryFromResult(array $result): Query to assert required keys exist and
have expected types before calling Query::inSubqueryDirect or Query::inSubquery.
Specifically, validate that $result['attribute'] is set and that $sub contains
'table' and 'column'; if using the 'conditions' branch ensure 'conditions' is
present and is an array, and for the other branch ensure 'filterColumn' and
'filterValues' exist (and that 'filterValues' is iterable); throw a clear
InvalidArgumentException (or similar) with a message including the missing
key(s) so callers fail fast and debugging is easier.
- Around line 8806-9000: castAndDecodeBatch can diverge from the original
casting()/decode() semantics (castingAfter called unconditionally and selective
"workAttributes" may skip transformations), so update it to preserve adapter
semantics: only call $this->adapter->castingAfter(...) when
$this->adapter->getSupportForCasting() is true or when adapter docs state
castingAfter must always run; ensure keys filtered by
$this->adapter->filter($key) are handled exactly like casting()/decode() by
re-checking attributes omitted from $workAttributes (especially attributes with
implicit filters or adapter-mapped names) and by reusing decodeAttribute(...)
for all filtered-key fallbacks and JSON-array handling (string->json) so
behavior matches decode(); add a clear pass to apply casting rules from
getSupportForCasting() and to run decodeAttribute for dotted/relationship
selections before createDocumentInstance(...) so selections and relationship
renames match previous find() behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bin/bench_m2m.phpbin/tasks/relationships.phpsrc/Database/Adapter.phpsrc/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Pool.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Database.phpsrc/Database/Query.php
| /** | ||
| * Fetch junction table mappings as lightweight arrays (no Document hydration). | ||
| * | ||
| * Returns array of [parentColumn => parentId, childColumn => childId] rows. | ||
| * Override in SQL adapters for maximum performance; default falls back to find(). | ||
| * | ||
| * @param Document $collection Junction collection document | ||
| * @param string $parentColumn Column name for parent FK | ||
| * @param string $childColumn Column name for child FK | ||
| * @param array<string> $parentIds Parent document IDs to look up | ||
| * @return array<array{string, string}> | ||
| */ | ||
| public function findJunctionMapping(Document $collection, string $parentColumn, string $childColumn, array $parentIds): array | ||
| { | ||
| // Default: use find() with select (adapters can override for raw performance) | ||
| $results = $this->find( | ||
| $collection, | ||
| [ | ||
| Query::equal($parentColumn, $parentIds), | ||
| Query::select([$parentColumn, $childColumn]), | ||
| Query::limit(PHP_INT_MAX), | ||
| ], | ||
| PHP_INT_MAX | ||
| ); | ||
|
|
||
| $mappings = []; | ||
| foreach ($results as $doc) { | ||
| $mappings[] = [ | ||
| $parentColumn => $doc->getAttribute($parentColumn), | ||
| $childColumn => $doc->getAttribute($childColumn), | ||
| ]; | ||
| } | ||
|
|
||
| return $mappings; | ||
| } |
There was a problem hiding this comment.
Fix phpstan return-type mismatch in findJunctionMapping().
The new default implementation returns array<array<string, mixed>> (because getAttribute() is mixed), but the docblock requires array<array{string, string}>, and CI already flags this. Please align the return type by validating/casting to string (or relaxing the docblock if null is valid).
🛠️ Suggested fix (type-safe mapping + docblock)
- * `@return` array<array{string, string}>
+ * `@return` array<array<string, string>>
...
- foreach ($results as $doc) {
- $mappings[] = [
- $parentColumn => $doc->getAttribute($parentColumn),
- $childColumn => $doc->getAttribute($childColumn),
- ];
- }
+ foreach ($results as $doc) {
+ $parentId = $doc->getAttribute($parentColumn);
+ $childId = $doc->getAttribute($childColumn);
+ if ($parentId === null || $childId === null) {
+ continue;
+ }
+ $mappings[] = [
+ $parentColumn => (string) $parentId,
+ $childColumn => (string) $childId,
+ ];
+ }🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 1607-1607: phpstan: Method Utopia\Database\Adapter::findJunctionMapping() should return array<array{string, string}> but returns array<int<0, max>, non-empty-array<string, mixed>>.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter.php` around lines 1574 - 1608, The docblock for
findJunctionMapping promises array<array{string, string}> but getAttribute()
returns mixed, causing phpstan errors; update the method to coerce/validate both
values to string before pushing into $mappings (use the
$parentColumn/$childColumn keys and the $results from find(), e.g. retrieve
$doc->getAttribute(...) and cast or explicitly validate and handle nulls/other
types) so the returned arrays always contain string values, and keep or adjust
the docblock accordingly if you choose to allow nulls instead of casting.
| // Literal filter values | ||
| $filterCol = $this->quote($this->filter($meta['filterColumn'])); | ||
| $filterValues = $meta['filterValues']; | ||
|
|
||
| if (empty($filterValues)) { | ||
| return '1 = 0'; | ||
| } | ||
|
|
||
| $valuePlaceholders = []; | ||
| foreach ($filterValues as $i => $val) { | ||
| $key = ":{$placeholder}_sub_{$i}"; | ||
| $binds[$key] = $val; | ||
| $valuePlaceholders[] = $key; | ||
| } | ||
|
|
||
| $inClause = implode(', ', $valuePlaceholders); | ||
|
|
||
| if ($query->getMethod() === Query::TYPE_IN_SUBQUERY_ALL) { | ||
| $countKey = ":{$placeholder}_sub_count"; | ||
| $binds[$countKey] = count($filterValues); | ||
| return "{$alias}.{$attribute} IN (" . | ||
| "SELECT {$selectCol}" . | ||
| " FROM {$subqueryTable}" . | ||
| " WHERE {$filterCol} IN ({$inClause}){$tenantCondition}" . | ||
| " GROUP BY {$selectCol}" . | ||
| " HAVING COUNT(DISTINCT {$filterCol}) = {$countKey}" . | ||
| ")"; |
There was a problem hiding this comment.
Deduplicate filter values for TYPE_IN_SUBQUERY_ALL to avoid false negatives.
COUNT(DISTINCT ...) is compared to the raw filterValues count; duplicates in the input will make the HAVING clause fail even when matches exist. Consider normalizing the list before building placeholders and the count.
🛠️ Suggested fix
- $filterValues = $meta['filterValues'];
+ $filterValues = array_values(array_unique($meta['filterValues'], SORT_REGULAR));📝 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.
| // Literal filter values | |
| $filterCol = $this->quote($this->filter($meta['filterColumn'])); | |
| $filterValues = $meta['filterValues']; | |
| if (empty($filterValues)) { | |
| return '1 = 0'; | |
| } | |
| $valuePlaceholders = []; | |
| foreach ($filterValues as $i => $val) { | |
| $key = ":{$placeholder}_sub_{$i}"; | |
| $binds[$key] = $val; | |
| $valuePlaceholders[] = $key; | |
| } | |
| $inClause = implode(', ', $valuePlaceholders); | |
| if ($query->getMethod() === Query::TYPE_IN_SUBQUERY_ALL) { | |
| $countKey = ":{$placeholder}_sub_count"; | |
| $binds[$countKey] = count($filterValues); | |
| return "{$alias}.{$attribute} IN (" . | |
| "SELECT {$selectCol}" . | |
| " FROM {$subqueryTable}" . | |
| " WHERE {$filterCol} IN ({$inClause}){$tenantCondition}" . | |
| " GROUP BY {$selectCol}" . | |
| " HAVING COUNT(DISTINCT {$filterCol}) = {$countKey}" . | |
| ")"; | |
| // Literal filter values | |
| $filterCol = $this->quote($this->filter($meta['filterColumn'])); | |
| $filterValues = array_values(array_unique($meta['filterValues'], SORT_REGULAR)); | |
| if (empty($filterValues)) { | |
| return '1 = 0'; | |
| } | |
| $valuePlaceholders = []; | |
| foreach ($filterValues as $i => $val) { | |
| $key = ":{$placeholder}_sub_{$i}"; | |
| $binds[$key] = $val; | |
| $valuePlaceholders[] = $key; | |
| } | |
| $inClause = implode(', ', $valuePlaceholders); | |
| if ($query->getMethod() === Query::TYPE_IN_SUBQUERY_ALL) { | |
| $countKey = ":{$placeholder}_sub_count"; | |
| $binds[$countKey] = count($filterValues); | |
| return "{$alias}.{$attribute} IN (" . | |
| "SELECT {$selectCol}" . | |
| " FROM {$subqueryTable}" . | |
| " WHERE {$filterCol} IN ({$inClause}){$tenantCondition}" . | |
| " GROUP BY {$selectCol}" . | |
| " HAVING COUNT(DISTINCT {$filterCol}) = {$countKey}" . | |
| ")"; |
🤖 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 2353 - 2379, The HAVING
COUNT(DISTINCT ...) comparison uses the raw $filterValues length which breaks
when inputs contain duplicates; before building placeholders for the
Query::TYPE_IN_SUBQUERY_ALL branch deduplicate and reindex $filterValues (e.g.
array_unique + array_values), handle the case it becomes empty (return '1 = 0'),
then build $valuePlaceholders and $binds from the deduplicated list and set
$binds[$countKey] to count($filterValues) so the HAVING uses the unique count.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Dockerfile (1)
67-68:⚠️ Potential issue | 🔴 CriticalMissing
COPY --from=pcovdirective prevents pcov extension from reaching the final stage.The pcov extension is built in a separate
FROM compile AS pcovstage (lines 49–55) usingmake install, which installs the.soto that stage's filesystem. However, there is noCOPY --from=pcovdirective to transfer it to the finalFROM compile AS finalstage. The final stage is a fresh instance of the base image and will not include the pcov binary. Line 67 attempts to enable a non-existent extension, causing the container to fail on startup.Add
COPY --from=pcov /usr/local/lib/php/extensions/ /usr/local/lib/php/extensions/(or equivalent path matching your build output) to the final stage before line 67.The xdebug extension (line 68) is assumed to come from the base image
appwrite/utopia-base:8.4-1.0.0. Verify it is included there; if not, a similar build stage and copy is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 67 - 68, The final stage enables pcov with "RUN echo extension=pcov.so >> /usr/local/etc/php/conf.d/pcov.ini" but the built pcov .so from the build stage named "pcov" isn't copied into the final image; add a COPY --from=pcov directive to copy the built extension files (e.g., from /usr/local/lib/php/extensions/) into the final stage's extension directory before the RUN that enables pcov so the extension actually exists at runtime; verify xdebug is present in the base image or apply the same copy pattern if it was also built separately.src/Database/Database.php (1)
9997-10164:⚠️ Potential issue | 🟠 MajorConvert related-collection queries before embedding them in subqueries.
Right now the subquery path uses$relatedQuerieswithout runningconvertQueries()for the related collection, so attribute typing/UTC normalization (e.g., datetime/object casting) can diverge from the non-subquery path that goes throughfind(). This can yield incorrect results when subqueries are enabled. Please normalize the related queries against the related collection before returning subquery conditions.Suggested fix
- $canUseSubquery = $this->adapter->getSupportForSubqueries(); + $canUseSubquery = $this->adapter->getSupportForSubqueries(); + if ($canUseSubquery) { + $relatedCollectionDoc = $this->silent(fn () => $this->getCollection($relatedCollection)); + $relatedQueries = $this->convertQueries($relatedCollectionDoc, $relatedQueries); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 9997 - 10164, The subquery branches use $relatedQueries directly without normalizing types; call convertQueries() for the related collection and replace $relatedQueries with the converted queries before constructing any 'subquery' arrays. Retrieve the related collection document via $this->getCollection($relatedCollection) (as in $relatedCollectionDoc), run $this->convertQueries($relatedCollectionDoc, $relatedQueries) (wrapped in $this->silent() like other lookups), and use the returned/converted queries in the Query::inSubqueryDirect call and in all places that set 'conditions' for subqueries (the nested M2M subquery, the O2M/M2O parent-resolution subquery, and the simple FK resolution subquery).
🧹 Nitpick comments (2)
Dockerfile (2)
1-1: Consider pinning the Composer image tag for reproducible builds.Switching from
composer:2.8tocomposer:2means builds will silently pick up new minor/patch versions. For CI reproducibility, a pinned minor (e.g.,composer:2.8) is preferable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 1, The Dockerfile currently uses an unpinned base image "FROM composer:2 AS composer" which allows automatic minor/patch updates; update that FROM directive to a pinned minor tag (for example "composer:2.8") to ensure reproducible builds, and document the chosen pinned tag in the Dockerfile or repo release notes so CI and local builds use the exact Composer version.
58-88: No non-rootUSERdirective in the final stage.Static analysis (Trivy DS-0002) flags that the container runs as root. Since this appears to be a development/testing Dockerfile (
CMD ["tail", "-f", "/dev/null"]), this is likely acceptable — but worth noting if this image is ever used in less trusted environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 58 - 88, Add a non-root user in the final stage and switch to it so the container does not run as root: create a user (e.g., appuser) and group with a fixed UID/GID, chown the workdir and important copied paths (referenced by WORKDIR /usr/src/code and the copied paths COPY --from=composer ... /usr/src/code/vendor, COPY ./bin, ./src, ./dev) to that user, then add a USER appuser line before the CMD [ "tail", "-f", "/dev/null" ]; ensure the chown and USER commands are conditional/placed after files are copied and that any debug-file removal logic (the DEBUG-related RUN lines) still runs as root if needed before switching users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Line 15: Update the Dockerfile's base image tag in the FROM instruction:
replace the incorrect image tag "appwrite/utopia-base:8.4-1.0.0" with the
correct tag "appwrite/utopia-base:php-8.4-1.0.0" so the FROM line uses the
existing image name.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 67-68: The final stage enables pcov with "RUN echo
extension=pcov.so >> /usr/local/etc/php/conf.d/pcov.ini" but the built pcov .so
from the build stage named "pcov" isn't copied into the final image; add a COPY
--from=pcov directive to copy the built extension files (e.g., from
/usr/local/lib/php/extensions/) into the final stage's extension directory
before the RUN that enables pcov so the extension actually exists at runtime;
verify xdebug is present in the base image or apply the same copy pattern if it
was also built separately.
In `@src/Database/Database.php`:
- Around line 9997-10164: The subquery branches use $relatedQueries directly
without normalizing types; call convertQueries() for the related collection and
replace $relatedQueries with the converted queries before constructing any
'subquery' arrays. Retrieve the related collection document via
$this->getCollection($relatedCollection) (as in $relatedCollectionDoc), run
$this->convertQueries($relatedCollectionDoc, $relatedQueries) (wrapped in
$this->silent() like other lookups), and use the returned/converted queries in
the Query::inSubqueryDirect call and in all places that set 'conditions' for
subqueries (the nested M2M subquery, the O2M/M2O parent-resolution subquery, and
the simple FK resolution subquery).
---
Nitpick comments:
In `@Dockerfile`:
- Line 1: The Dockerfile currently uses an unpinned base image "FROM composer:2
AS composer" which allows automatic minor/patch updates; update that FROM
directive to a pinned minor tag (for example "composer:2.8") to ensure
reproducible builds, and document the chosen pinned tag in the Dockerfile or
repo release notes so CI and local builds use the exact Composer version.
- Around line 58-88: Add a non-root user in the final stage and switch to it so
the container does not run as root: create a user (e.g., appuser) and group with
a fixed UID/GID, chown the workdir and important copied paths (referenced by
WORKDIR /usr/src/code and the copied paths COPY --from=composer ...
/usr/src/code/vendor, COPY ./bin, ./src, ./dev) to that user, then add a USER
appuser line before the CMD [ "tail", "-f", "/dev/null" ]; ensure the chown and
USER commands are conditional/placed after files are copied and that any
debug-file removal logic (the DEBUG-related RUN lines) still runs as root if
needed before switching users.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfilebin/bench_m2m.phpsrc/Database/Adapter.phpsrc/Database/Database.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Database/Adapter.php
- bin/bench_m2m.php
The tag format is php-8.4-1.0.0, not 8.4-1.0.0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Dockerfile (2)
32-41:⚠️ Potential issue | 🔴 Critical
$PHP_MONGODB_VERSIONis undefined — this breaks the build.The old Dockerfile declared
ENV PHP_MONGODB_VERSION=...explicitly; that block was removed when switching toappwrite/utopia-base, but the base image does not export this variable. As confirmed by the pipeline failure,pecl install mongodb-$PHP_MONGODB_VERSIONexpands topecl install mongodb-, which is an invalid package name and halts the build.Pin the version directly or introduce a build-arg with a default:
🔧 Proposed fix
+ARG PHP_MONGODB_VERSION=1.21.0 + RUN apk update && apk add --no-cache \ ... && (pecl install mongodb-$PHP_MONGODB_VERSION \Or pin inline to remove the variable dependency entirely:
- && (pecl install mongodb-$PHP_MONGODB_VERSION \ - || (git clone --depth 1 --branch $PHP_MONGODB_VERSION --recurse-submodules https://github.com/mongodb/mongo-php-driver.git /tmp/mongodb \ + && (pecl install mongodb-1.21.0 \ + || (git clone --depth 1 --branch 1.21.0 --recurse-submodules https://github.com/mongodb/mongo-php-driver.git /tmp/mongodb \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 32 - 41, The Dockerfile uses $PHP_MONGODB_VERSION in the pecl install command but that build ARG/ENV is not defined, causing pecl install to run with an empty suffix; fix by adding a default build argument or environment variable (e.g., declare ARG PHP_MONGODB_VERSION=<version> or ENV PHP_MONGODB_VERSION=<version> at top of the Dockerfile) or by pinning the driver inline (replace pecl install mongodb-$PHP_MONGODB_VERSION with pecl install mongodb-<fixed-version>); update the Dockerfile near the build-stage declarations so the pecl install mongodb-$PHP_MONGODB_VERSION invocation (and the fallback git clone branch name) use the defined value.
49-55:⚠️ Potential issue | 🟠 MajorOrphaned
pcovbuild stage causes PHP startup failure — missing artifact is never copied to final image.The
FROM compile AS pcovstage (lines 49–55) buildspcov.so, but the final stage contains noCOPY --from=pcovinstruction. Combined with the unconditionalextension=pcov.soon line 67, this causes a runtime failure: PHP will attempt to load a.sofile that does not exist in the final image.Solution: Either remove the build stage entirely, or add the missing
COPYinstruction to transfer the compiled extension:Option A — Remove the build stage (recommended if unused)
-## PCOV Extension -FROM compile AS pcov -RUN \ - git clone --depth 1 https://github.com/krakjoe/pcov.git \ - && cd pcov \ - && phpize \ - && ./configure --enable-pcov \ - && make && make install - FROM compile AS finalOption B — Copy the built extension
FROM compile AS final +COPY --from=pcov /usr/local/lib/php/extensions/ /usr/local/lib/php/extensions/ + LABEL maintainer="team@appwrite.io"Note: A similar issue exists with
xdebug.ini(line 68) — whenDEBUG=false, the.sofile is removed (line 87) but the.iniremains, also causing startup errors. Both should be conditionally created.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 49 - 55, The Dockerfile builds a pcov extension in the "pcov" build stage but never copies the resulting pcov.so into the final image while still unconditionally enabling "extension=pcov.so", causing PHP startup failures; fix by either removing the "FROM compile AS pcov" stage if pcov isn't used, or add a COPY --from=pcov to copy the compiled pcov.so into the final image and ensure the php ini enabling it is only written when the .so exists; likewise make creation of "xdebug.ini" conditional on DEBUG=true (or mirror removal of the .so) so you don't leave an .ini that references a missing .so.
🧹 Nitpick comments (1)
Dockerfile (1)
1-1:composer:2is a floating tag — consider re-pinning to a minor version.Switching from
composer:2.8tocomposer:2means any new2.xComposer release will be silently picked up on the next build, potentially causing unexpected behaviour. Pinning to at least a minor version (e.g.composer:2.8) restores reproducibility.♻️ Proposed fix
-FROM composer:2 AS composer +FROM composer:2.8 AS composer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 1, The Dockerfile uses a floating image tag "FROM composer:2"; update this to a pinned minor version (e.g., change "FROM composer:2" to "FROM composer:2.8" or another specific 2.x minor you have tested) to ensure reproducible builds and avoid silently pulling new 2.x Composer releases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Dockerfile`:
- Around line 32-41: The Dockerfile uses $PHP_MONGODB_VERSION in the pecl
install command but that build ARG/ENV is not defined, causing pecl install to
run with an empty suffix; fix by adding a default build argument or environment
variable (e.g., declare ARG PHP_MONGODB_VERSION=<version> or ENV
PHP_MONGODB_VERSION=<version> at top of the Dockerfile) or by pinning the driver
inline (replace pecl install mongodb-$PHP_MONGODB_VERSION with pecl install
mongodb-<fixed-version>); update the Dockerfile near the build-stage
declarations so the pecl install mongodb-$PHP_MONGODB_VERSION invocation (and
the fallback git clone branch name) use the defined value.
- Around line 49-55: The Dockerfile builds a pcov extension in the "pcov" build
stage but never copies the resulting pcov.so into the final image while still
unconditionally enabling "extension=pcov.so", causing PHP startup failures; fix
by either removing the "FROM compile AS pcov" stage if pcov isn't used, or add a
COPY --from=pcov to copy the compiled pcov.so into the final image and ensure
the php ini enabling it is only written when the .so exists; likewise make
creation of "xdebug.ini" conditional on DEBUG=true (or mirror removal of the
.so) so you don't leave an .ini that references a missing .so.
---
Nitpick comments:
In `@Dockerfile`:
- Line 1: The Dockerfile uses a floating image tag "FROM composer:2"; update
this to a pinned minor version (e.g., change "FROM composer:2" to "FROM
composer:2.8" or another specific 2.x minor you have tested) to ensure
reproducible builds and avoid silently pulling new 2.x Composer releases.
The utopia-base image already includes mongodb, pdo_mysql, pdo_pgsql, docker-cli, and docker-cli-compose. Only opcache and pgsql still need to be installed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The base image may not include these drivers in the tagged version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Dockerfile (3)
52-52:⚠️ Potential issue | 🟡 Minor
xdebug.iniretainsextension=xdebug.sowhenDEBUG=false, causing a PHP startup warning.Line 52 unconditionally writes
extension=xdebug.sointoxdebug.ini. WhenDEBUG=false, line 70 removesxdebug.sobut does not touchxdebug.ini, so every PHP invocation emits a startup warning about the missing library. Clean up the ini when running without debug:🔧 Suggested fix
-RUN if [ "$DEBUG" = "false" ]; then rm -f /usr/local/lib/php/extensions/no-debug-non-zts-20240924/xdebug.so; fi +RUN if [ "$DEBUG" = "false" ]; then \ + rm -f /usr/local/lib/php/extensions/no-debug-non-zts-20240924/xdebug.so \ + && rm -f /usr/local/etc/php/conf.d/xdebug.ini; \ + fiAlso applies to: 67-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 52, The Dockerfile unconditionally appends "RUN echo extension=xdebug.so >> /usr/local/etc/php/conf.d/xdebug.ini" which leaves xdebug.ini present when DEBUG=false and causes startup warnings after the xdebug extension is removed; change the Dockerfile so that the xdebug.ini file is only created when DEBUG=true (wrap the echo in the same conditional that installs/enables xdebug) or ensure the ini is removed when DEBUG=false (delete /usr/local/etc/php/conf.d/xdebug.ini alongside the existing removal of the xdebug .so), keeping the existing DEBUG check and the commands that remove the extension in sync.
42-72:⚠️ Potential issue | 🟡 MinorContainer runs as root — no
USERdirective present.The final stage has no
USERinstruction; the container process runs as root. While acceptable for a CI/test environment, adding a non-root user is a security best practice (flagged by Trivy DS-0002).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 42 - 72, The Dockerfile final stage leaves the container running as root; add a non-root user and switch to it to satisfy DS-0002: create a dedicated system user/group (e.g., app/app) in the final stage (use RUN to add group and user), ensure ownership of the application dirs (chown -R /usr/src/code) after COPY, and then set USER to that non-root user (USER app) before CMD; keep the DEBUG-related file ops in a place where the non-root user has access (either perform chown after those RUNs or create the user earlier and adjust permissions) so builds still succeed while avoiding root runtime.
32-42:⚠️ Potential issue | 🟠 MajorRemove the orphaned
pcovbuild stage.Lines 33–39 build and install
pcov.so, but thefinalstage (line 42) derives fromcompile, not frompcov, and there is noCOPY --from=pcovanywhere. The evidence thatxdebug.sois removed at line 70 without being copied in indicates both extensions come from the base imageappwrite/utopia-base:php-8.4-1.0.0. The entire pcov build stage is dead code.Remove lines 32–40
&& rm -rf /var/cache/apk/* -## PCOV Extension -FROM compile AS pcov -RUN \ - git clone --depth 1 https://github.com/krakjoe/pcov.git \ - && cd pcov \ - && phpize \ - && ./configure --enable-pcov \ - && make && make install - FROM compile AS final🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 32 - 42, Remove the orphaned pcov build stage: delete the "FROM compile AS pcov" stage and its block that clones/builds pcov (the git clone, cd pcov, phpize, ./configure --enable-pcov, make && make install) because the final image is built from "FROM compile AS final" and there is no COPY --from=pcov; ensure only the "FROM compile AS final" stage remains so there is no dead pcov build stage left in the Dockerfile.
🧹 Nitpick comments (1)
Dockerfile (1)
1-1: Pin the Composer image to a specific version for reproducible builds.
composer:2is a floating tag; future 2.x releases could silently change behaviour. The previouscomposer:2.8was more reproducible.🔧 Suggested fix
-FROM composer:2 AS composer +FROM composer:2.8 AS composer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 1, The Dockerfile uses a floating image tag "composer:2" which can introduce unreproducible builds; change the base image reference in the Dockerfile's FROM instruction (the "FROM composer:2 AS composer" line) to a specific pinned Composer 2.x tag (e.g., composer:2.8) to ensure reproducible builds and prevent unexpected behaviour from future 2.x releases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Dockerfile`:
- Line 52: The Dockerfile unconditionally appends "RUN echo extension=xdebug.so
>> /usr/local/etc/php/conf.d/xdebug.ini" which leaves xdebug.ini present when
DEBUG=false and causes startup warnings after the xdebug extension is removed;
change the Dockerfile so that the xdebug.ini file is only created when
DEBUG=true (wrap the echo in the same conditional that installs/enables xdebug)
or ensure the ini is removed when DEBUG=false (delete
/usr/local/etc/php/conf.d/xdebug.ini alongside the existing removal of the
xdebug .so), keeping the existing DEBUG check and the commands that remove the
extension in sync.
- Around line 42-72: The Dockerfile final stage leaves the container running as
root; add a non-root user and switch to it to satisfy DS-0002: create a
dedicated system user/group (e.g., app/app) in the final stage (use RUN to add
group and user), ensure ownership of the application dirs (chown -R
/usr/src/code) after COPY, and then set USER to that non-root user (USER app)
before CMD; keep the DEBUG-related file ops in a place where the non-root user
has access (either perform chown after those RUNs or create the user earlier and
adjust permissions) so builds still succeed while avoiding root runtime.
- Around line 32-42: Remove the orphaned pcov build stage: delete the "FROM
compile AS pcov" stage and its block that clones/builds pcov (the git clone, cd
pcov, phpize, ./configure --enable-pcov, make && make install) because the final
image is built from "FROM compile AS final" and there is no COPY --from=pcov;
ensure only the "FROM compile AS final" stage remains so there is no dead pcov
build stage left in the Dockerfile.
---
Nitpick comments:
In `@Dockerfile`:
- Line 1: The Dockerfile uses a floating image tag "composer:2" which can
introduce unreproducible builds; change the base image reference in the
Dockerfile's FROM instruction (the "FROM composer:2 AS composer" line) to a
specific pinned Composer 2.x tag (e.g., composer:2.8) to ensure reproducible
builds and prevent unexpected behaviour from future 2.x releases.
Add back libpq, git, brotli-dev, docker-cli, and docker-cli-compose which may not be present in the base image tagged version. docker-cli is needed at runtime for cache fallback tests that stop/start Redis. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…atial queries TYPE_CONTAINS/TYPE_NOT_CONTAINS and TYPE_EQUAL/TYPE_NOT_EQUAL are dual-purpose query methods used for both regular and spatial queries. The spatial methods list only checked explicitly-spatial method types, missing these shared methods. When a spatial CONTAINS query wasn't caught, it flowed through the subquery path to escapeWildcards() which can't handle array values (coordinates). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The reconstructed Query objects in relationship resolution don't have attributeType set (validateQuery doesn't resolve dot-notation relationship attributes to spatial types), so isSpatialAttribute() always returns false. For dual-purpose methods like TYPE_CONTAINS and TYPE_EQUAL that are used for both string/array and spatial queries, detect spatial usage by checking if the values are arrays (coordinates) rather than strings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Dockerfile (1)
57-57:⚠️ Potential issue | 🔴 Criticalxdebug.so does not exist in the base image — this line will fail at runtime.
The base image
appwrite/utopia-base:php-8.4-1.0.0does not ship with xdebug pre-installed. According to the official xdebug documentation, xdebug must be compiled and enabled separately viapecl install xdebug && docker-php-ext-enable xdebug. Line 57 writes a config referencing a non-existent extension, and line 75's attempt to remove it will silently fail. This is the same critical issue as pcov on line 56.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 57, The Dockerfile currently writes "extension=xdebug.so" but xdebug is not present in the base image; either install and enable xdebug before referencing it or remove the config line — specifically, run a proper installation step (pecl install xdebug and docker-php-ext-enable xdebug) prior to the RUN echo that appends xdebug to php conf (the line that writes extension=xdebug.so) and ensure any later cleanup that tries to remove xdebug (the removal referencing xdebug.so) is only executed if installation succeeded; alternatively, delete the RUN echo for xdebug.so and the corresponding removal to avoid referencing a non-existent extension.src/Database/Database.php (1)
9781-9857:⚠️ Potential issue | 🟠 MajorSubquery path bypasses related-collection permission filters.
At Line 9784 and Line 10056, the subquery branches avoid
find()on the related collection, which is where documentSecurity/permission filtering is applied. This can cause parent documents to match via related documents the caller cannot read. Please either (a) inject the related-collection permission predicates into the subquery, or (b) gate subquery usage when the related collection has documentSecurity enabled (or when collection-level read isn’t granted). Based on learnings: “In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).”Also applies to: 9929-9933, 10056-10176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 9781 - 9857, The subquery branch in Database::convertRelationshipQueries (where it builds Query::inSubqueryAll via getJunctionCollection and createSubqueryFromResult) bypasses related-collection permission filtering; update the logic to check the related collection's documentSecurity and collection-level read permission (use the $relatedCollectionDoc from getCollection and the collection permission API) and if documentSecurity is enabled or read is not granted, do NOT generate the subquery — instead resolve IDs via the existing non-subquery path (use resolveRelationshipGroupToIds or perform a find() on the related collection so permission predicates are applied). Ensure this check is performed wherever subqueries are created (see createSubqueryFromResult usage and the many-to-many early-return branch) and fall back to the permission-aware resolution to avoid returning IDs from unreadable related documents.
🧹 Nitpick comments (1)
Dockerfile (1)
77-77: Consider adding a non-rootUSERfor defense-in-depth.Trivy flags DS-0002: no non-root user specified. Since this is a dev/test container, running as root is common practice and not blocking. If you'd like to harden it, adding a
USERdirective beforeCMDis straightforward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 77, Trivy warns the image runs as root because no non-root USER is set; create a non-root user and switch to it before CMD to reduce risk. Add steps to create a user/group (e.g., with addgroup/adduser or useradd), chown any runtime directories/files the container needs (so the non-root user can access them), and include a USER <username> directive before the existing CMD ["tail","-f","/dev/null"]; ensure any necessary setup commands that require root remain earlier in the Dockerfile so the final image drops privileges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Dockerfile`:
- Line 57: The Dockerfile currently writes "extension=xdebug.so" but xdebug is
not present in the base image; either install and enable xdebug before
referencing it or remove the config line — specifically, run a proper
installation step (pecl install xdebug and docker-php-ext-enable xdebug) prior
to the RUN echo that appends xdebug to php conf (the line that writes
extension=xdebug.so) and ensure any later cleanup that tries to remove xdebug
(the removal referencing xdebug.so) is only executed if installation succeeded;
alternatively, delete the RUN echo for xdebug.so and the corresponding removal
to avoid referencing a non-existent extension.
In `@src/Database/Database.php`:
- Around line 9781-9857: The subquery branch in
Database::convertRelationshipQueries (where it builds Query::inSubqueryAll via
getJunctionCollection and createSubqueryFromResult) bypasses related-collection
permission filtering; update the logic to check the related collection's
documentSecurity and collection-level read permission (use the
$relatedCollectionDoc from getCollection and the collection permission API) and
if documentSecurity is enabled or read is not granted, do NOT generate the
subquery — instead resolve IDs via the existing non-subquery path (use
resolveRelationshipGroupToIds or perform a find() on the related collection so
permission predicates are applied). Ensure this check is performed wherever
subqueries are created (see createSubqueryFromResult usage and the many-to-many
early-return branch) and fall back to the permission-aware resolution to avoid
returning IDs from unreadable related documents.
---
Nitpick comments:
In `@Dockerfile`:
- Line 77: Trivy warns the image runs as root because no non-root USER is set;
create a non-root user and switch to it before CMD to reduce risk. Add steps to
create a user/group (e.g., with addgroup/adduser or useradd), chown any runtime
directories/files the container needs (so the non-root user can access them),
and include a USER <username> directive before the existing CMD
["tail","-f","/dev/null"]; ensure any necessary setup commands that require root
remain earlier in the Dockerfile so the final image drops privileges.
skipValidation() called $this->disableValidation() which, in Mirror, delegates to source and destination, permanently disabling their validation. The finally block only restored $this->validate directly without re-enabling source/destination. Use direct property assignment instead to avoid unwanted delegation in subclasses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit