Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds TYPE_CONTAINS_ANY and TYPE_CONTAINS_ALL query methods across Query, validators, and adapters; refactors nested relationship traversal into resolveRelationshipGroupToIds() and processNestedRelationshipPath(); and adds e2e tests for containsAny/containsAll and many-to-many relationship containsAll scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Query
participant Database
participant Adapter
Client->>Query: build containsAny/containsAll query
Query->>Database: execute(query)
Database->>Database: group dot-path relationship queries
Database->>Database: processNestedRelationshipPath(...) / resolveRelationshipGroupToIds(...)
Database->>Adapter: fetch related IDs (forward/reverse, nested)
Adapter-->>Database: return ids
Database->>Database: integrate resolved IDs into main query criteria
Database->>Adapter: run final query with resolved criteria
Adapter-->>Client: return results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
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 (4)
src/Database/Adapter/Mongo.php (1)
2762-2771:⚠️ Potential issue | 🔴 Critical
TYPE_CONTAINS_ALLon object attributes incorrectly uses$ininstead of$all.In
handleObjectFilters,TYPE_CONTAINS_ALLfalls through to the same case asTYPE_CONTAINSandTYPE_CONTAINS_ANY, which sets$operator = '$in'. This gives "any" semantics, not "all" semantics. ForTYPE_CONTAINS_ALL, the operator should be$all.Proposed fix
case Query::TYPE_CONTAINS: case Query::TYPE_CONTAINS_ANY: case Query::TYPE_CONTAINS_ALL: case Query::TYPE_NOT_CONTAINS: { $arrayValue = \is_array($queryValue) ? $queryValue : [$queryValue]; - $operator = $isNot ? '$nin' : '$in'; + if ($query->getMethod() === Query::TYPE_CONTAINS_ALL) { + $operator = '$all'; + } else { + $operator = $isNot ? '$nin' : '$in'; + } $conditions[] = [ $flattenedObjectKey => [ $operator => $arrayValue] ]; break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Mongo.php` around lines 2762 - 2771, In handleObjectFilters, Query::TYPE_CONTAINS_ALL is currently grouped with TYPE_CONTAINS and TYPE_CONTAINS_ANY so it uses the '$in' operator; update the switch so that when $query->getMethod() === Query::TYPE_CONTAINS_ALL you set $operator = $isNot ? '$nin' : '$all' (or otherwise use '$all' for positive semantics) and keep TYPE_CONTAINS/TYPE_CONTAINS_ANY using '$in'; modify the case labels so TYPE_CONTAINS_ALL has its own branch (or conditional) to choose '$all' instead of '$in' when constructing $conditions for $flattenedObjectKey.src/Database/Adapter/Postgres.php (2)
1714-1738:⚠️ Potential issue | 🔴 Critical
TYPE_CONTAINS_ALLon object attributes uses OR separator, giving "any" semantics instead of "all".The
$separatoris' OR 'for non-NOT queries (line 1737), so each@>condition is joined with OR. ForCONTAINS_ALL, the conditions should be joined with AND so that all values must be present.Proposed fix
case Query::TYPE_CONTAINS: case Query::TYPE_CONTAINS_ANY: case Query::TYPE_CONTAINS_ALL: case Query::TYPE_NOT_CONTAINS: { $isNot = $query->getMethod() === Query::TYPE_NOT_CONTAINS; $conditions = []; foreach ($query->getValues() as $key => $value) { // ...binding logic... $fragment = "{$alias}.{$attribute} @> :{$placeholder}_{$key}::jsonb"; $conditions[] = $isNot ? "NOT (" . $fragment . ")" : $fragment; } - $separator = $isNot ? ' AND ' : ' OR '; + $separator = ($isNot || $query->getMethod() === Query::TYPE_CONTAINS_ALL) ? ' AND ' : ' OR '; return empty($conditions) ? '' : '(' . implode($separator, $conditions) . ')'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Postgres.php` around lines 1714 - 1738, The current branch that builds JSON containment conditions always uses ' OR ' for non-NOT queries, which makes Query::TYPE_CONTAINS_ALL behave like "any" instead of "all"; change the separator logic in the Postgres adapter case handling (the block that inspects Query::TYPE_CONTAINS, TYPE_CONTAINS_ANY, TYPE_CONTAINS_ALL, TYPE_NOT_CONTAINS) so that if $query->getMethod() === Query::TYPE_CONTAINS_ALL you use ' AND ' to join $conditions, if $isNot is true keep ' AND ' (negated fragments must all hold), otherwise keep ' OR ' for TYPE_CONTAINS/TYPE_CONTAINS_ANY; update the code that sets $separator (currently $separator = $isNot ? ' AND ' : ' OR ') to reflect these three cases.
1817-1848:⚠️ Potential issue | 🟠 Major
TYPE_CONTAINS_ALLfalls through to the default path for non-array attributes, but is missing from the value-matching arm and uses wrong separator.When
onArray()is false,TYPE_CONTAINS_ALLfalls through to the default block. Two issues:
- The value-matching arm lists
Query::TYPE_CONTAINS, Query::TYPE_CONTAINS_ANYbut notQuery::TYPE_CONTAINS_ALL, so values won't get%LIKE wildcards.- The separator will be
' OR 'instead of' AND ', giving "any" semantics instead of "all" semantics.The parent
SQLclass correctly handlesTYPE_CONTAINS_ALLingetSQLOperator(), returning the LIKE operator, so the operator mapping is correct. However, both the value transformation and separator logic need fixing when non-array containsAll is used with multiple values.Proposed fix
- Query::TYPE_CONTAINS, Query::TYPE_CONTAINS_ANY => ($query->onArray()) ? \json_encode($value) : '%' . $this->escapeWildcards($value) . '%', + Query::TYPE_CONTAINS, Query::TYPE_CONTAINS_ANY, Query::TYPE_CONTAINS_ALL => ($query->onArray()) ? \json_encode($value) : '%' . $this->escapeWildcards($value) . '%',And for the separator:
- $separator = $isNotQuery ? ' AND ' : ' OR '; + $isAllQuery = $query->getMethod() === Query::TYPE_CONTAINS_ALL; + $separator = ($isNotQuery || $isAllQuery) ? ' AND ' : ' OR ';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Postgres.php` around lines 1817 - 1848, TYPE_CONTAINS_ALL currently falls through to the default path for non-array attributes but isn’t handled in the value transformation or separator logic; update the default block to include Query::TYPE_CONTAINS_ALL in the match arm that applies LIKE/wildcard transformations (alongside TYPE_CONTAINS and TYPE_CONTAINS_ANY) so values get '%' wrappers via escapeWildcards, and set the join separator to ' AND ' (instead of the default ' OR ') when $query->getMethod() === Query::TYPE_CONTAINS_ALL and !$query->onArray(); keep using $this->getSQLOperator($query->getMethod()) for the operator and preserve existing array handling for onArray().src/Database/Adapter/MariaDB.php (1)
1621-1636:⚠️ Potential issue | 🟠 Major
TYPE_CONTAINS_ALLon non-array attributes produces incorrectORlogic instead ofAND.When
$query->onArray()isfalse, theTYPE_CONTAINS_ALLcase falls through to thedefaultblock (line 1655). There, it is not explicitly handled in thematchstatement and gets processed as a generic value. The default block then loops through values and joins them with' OR '(line 1665), producing(attr LIKE ...) OR (attr LIKE ...)semantics, which is equivalent tocontainsAny, notcontainsAll.For example,
containsAll(['foo', 'bar'])on a non-array string should mean "contains both 'foo' AND 'bar'", but instead produces "(contains 'foo') OR (contains 'bar')".Array attributes correctly use
JSON_CONTAINSfor subset semantics (line 1622), but non-array strings need explicit handling withANDlogic or should be explicitly disallowed at validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/MariaDB.php` around lines 1621 - 1636, The TYPE_CONTAINS_ALL branch falls through for non-array attributes and ends up using the default logic which joins per-value LIKE clauses with OR; change the handling so that when Query::TYPE_CONTAINS_ALL and $query->onArray() is false you explicitly build per-value binds from $query->getValues() (using :{$placeholder}_n keys into $binds) and return a combined expression joining each "({$alias}.{$attribute} LIKE :{$placeholder}_n)" with AND, or alternatively throw/validate to disallow containsAll on non-array attributes; update the match/switch to include this explicit case (referencing TYPE_CONTAINS_ALL, $query->onArray(), $query->getValues(), $binds, $placeholder, $alias and $attribute) instead of falling through to the default OR logic.
🧹 Nitpick comments (5)
src/Database/Validator/Query/Filter.php (1)
377-390: Same grouping inconsistency as inQueries.php—TYPE_CONTAINS_ALLis separated fromTYPE_CONTAINS_ANY.Consider grouping all contains variants together in the switch for consistency.
Suggested grouping
case Query::TYPE_EQUAL: case Query::TYPE_CONTAINS: case Query::TYPE_CONTAINS_ANY: - case Query::TYPE_NOT_CONTAINS: case Query::TYPE_CONTAINS_ALL: + case Query::TYPE_NOT_CONTAINS: case Query::TYPE_EXISTS: case Query::TYPE_NOT_EXISTS:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Validator/Query/Filter.php` around lines 377 - 390, The switch in Filter.php groups Query::TYPE_CONTAINS_ALL separately from the other contains variants; update the case grouping so TYPE_CONTAINS_ALL is adjacent to TYPE_CONTAINS and TYPE_CONTAINS_ANY (i.e., include TYPE_CONTAINS_ALL in the same case block that checks $this->isEmpty($value->getValues()) and then calls $this->isValidAttributeAndValues($attribute, $value->getValues(), $method)), keeping the existing $this->message assignment and return behavior and preserving use of $method and $value->getValues().src/Database/Validator/Queries.php (1)
106-112: GroupTYPE_CONTAINS_ALLnext to the other contains variants for consistency.
TYPE_CONTAINS_ANYis placed next toTYPE_CONTAINS(Line 107), butTYPE_CONTAINS_ALLis placed afterTYPE_OR(Line 111). Both are contains variants and should be co-located for readability and maintainability.Suggested grouping
Query::TYPE_CONTAINS, Query::TYPE_CONTAINS_ANY, + Query::TYPE_CONTAINS_ALL, Query::TYPE_NOT_CONTAINS, Query::TYPE_AND, Query::TYPE_OR, - Query::TYPE_CONTAINS_ALL, Query::TYPE_ELEM_MATCH,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Validator/Queries.php` around lines 106 - 112, The enum-like ordering mixes contains variants: move Query::TYPE_CONTAINS_ALL so it sits with the other contains entries (Query::TYPE_CONTAINS, Query::TYPE_CONTAINS_ANY, Query::TYPE_NOT_CONTAINS, Query::TYPE_CONTAINS_ALL, Query::TYPE_ELEM_MATCH) instead of after Query::TYPE_OR; update the list in the code block where these constants are declared so all "contains" types are co-located for consistency (refer to Query::TYPE_CONTAINS_ALL, Query::TYPE_CONTAINS_ANY, and Query::TYPE_OR to find the items to reorder).src/Database/Query.php (3)
842-850: NewcontainsAll()factory method looks good.Consistent with the
containsAny()pattern. Missing a@returnin the docblock saysQuery, correct.One minor note: the docblock for
containsAlllacks a description (unlikecontainsAnywhich has one). Consider adding a brief description for consistency.Suggested docblock improvement
/** + * Helper method to create Query with containsAll method. + * For array attributes, matches documents where the attribute contains ALL of the given values. + * * `@param` string $attribute * `@param` array<mixed> $values * `@return` Query */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 842 - 850, Add a brief description to the containsAll() docblock consistent with containsAny() (e.g., "Create a Query that matches items containing all given values.") and ensure the `@param` and `@return` annotations match the existing style used by containsAny() (keep `@return` Query if that is the project's convention) so the docblock is consistent and descriptive for the containsAll() factory method.
69-69:TYPE_CONTAINS_ALLis declared in the "Logical methods" section instead of with the other filter/contains constants.This constant is a filter type (like
TYPE_CONTAINSat line 17 andTYPE_CONTAINS_ANYat line 18), not a logical combinator likeTYPE_AND/TYPE_OR. Placing it here is misleading. Move it next to line 18 for consistency.Proposed relocation
public const TYPE_CONTAINS = 'contains'; public const TYPE_CONTAINS_ANY = 'containsAny'; + public const TYPE_CONTAINS_ALL = 'containsAll'; public const TYPE_NOT_CONTAINS = 'notContains';And remove the declaration from line 69.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` at line 69, The constant TYPE_CONTAINS_ALL is misplaced in the "Logical methods" section; move its declaration next to the other filter/contains constants (near TYPE_CONTAINS and TYPE_CONTAINS_ANY) and remove the duplicate declaration from the logical section (where TYPE_AND/TYPE_OR live) so all filter/contains constants are grouped together and the logical combinators remain separate.
540-550: Add#[Deprecated]attribute to thecontains()method for runtime deprecation notices.The project targets PHP 8.4+, which supports the native
#[Deprecated]attribute. Using it alongside the@deprecateddocblock will generate runtime deprecation notices in addition to static analysis coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 540 - 550, Add the native #[Deprecated] attribute to the contains method so it emits runtime deprecation notices: update the Query::contains method declaration to include a #[Deprecated(...)] attribute (optionally include a message like "Use containsAny() for arrays or contains() for strings" and a since version) while keeping the existing `@deprecated` docblock and returning the same signature and 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 `@src/Database/Database.php`:
- Around line 9111-9133: The containsAll branch always adds Query::equal('$id',
$ids) which ignores the attribute returned by resolveRelationshipGroupToIds
(e.g., relationshipKey) and breaks parent‑side relationship filtering; change
the logic that appends to $additionalQueries to use the attribute from the
resolver result (falling back to '$id' if not provided) — i.e., extract the
returned attribute (from the $result returned by resolveRelationshipGroupToIds,
e.g., 'relationshipKey' or similar) and call Query::equal($attribute, $ids)
instead of Query::equal('$id', $ids) so containsAll filters the correct field
for one‑to‑one/many‑to‑one relationships.
---
Outside diff comments:
In `@src/Database/Adapter/MariaDB.php`:
- Around line 1621-1636: The TYPE_CONTAINS_ALL branch falls through for
non-array attributes and ends up using the default logic which joins per-value
LIKE clauses with OR; change the handling so that when Query::TYPE_CONTAINS_ALL
and $query->onArray() is false you explicitly build per-value binds from
$query->getValues() (using :{$placeholder}_n keys into $binds) and return a
combined expression joining each "({$alias}.{$attribute} LIKE
:{$placeholder}_n)" with AND, or alternatively throw/validate to disallow
containsAll on non-array attributes; update the match/switch to include this
explicit case (referencing TYPE_CONTAINS_ALL, $query->onArray(),
$query->getValues(), $binds, $placeholder, $alias and $attribute) instead of
falling through to the default OR logic.
In `@src/Database/Adapter/Mongo.php`:
- Around line 2762-2771: In handleObjectFilters, Query::TYPE_CONTAINS_ALL is
currently grouped with TYPE_CONTAINS and TYPE_CONTAINS_ANY so it uses the '$in'
operator; update the switch so that when $query->getMethod() ===
Query::TYPE_CONTAINS_ALL you set $operator = $isNot ? '$nin' : '$all' (or
otherwise use '$all' for positive semantics) and keep
TYPE_CONTAINS/TYPE_CONTAINS_ANY using '$in'; modify the case labels so
TYPE_CONTAINS_ALL has its own branch (or conditional) to choose '$all' instead
of '$in' when constructing $conditions for $flattenedObjectKey.
In `@src/Database/Adapter/Postgres.php`:
- Around line 1714-1738: The current branch that builds JSON containment
conditions always uses ' OR ' for non-NOT queries, which makes
Query::TYPE_CONTAINS_ALL behave like "any" instead of "all"; change the
separator logic in the Postgres adapter case handling (the block that inspects
Query::TYPE_CONTAINS, TYPE_CONTAINS_ANY, TYPE_CONTAINS_ALL, TYPE_NOT_CONTAINS)
so that if $query->getMethod() === Query::TYPE_CONTAINS_ALL you use ' AND ' to
join $conditions, if $isNot is true keep ' AND ' (negated fragments must all
hold), otherwise keep ' OR ' for TYPE_CONTAINS/TYPE_CONTAINS_ANY; update the
code that sets $separator (currently $separator = $isNot ? ' AND ' : ' OR ') to
reflect these three cases.
- Around line 1817-1848: TYPE_CONTAINS_ALL currently falls through to the
default path for non-array attributes but isn’t handled in the value
transformation or separator logic; update the default block to include
Query::TYPE_CONTAINS_ALL in the match arm that applies LIKE/wildcard
transformations (alongside TYPE_CONTAINS and TYPE_CONTAINS_ANY) so values get
'%' wrappers via escapeWildcards, and set the join separator to ' AND ' (instead
of the default ' OR ') when $query->getMethod() === Query::TYPE_CONTAINS_ALL and
!$query->onArray(); keep using $this->getSQLOperator($query->getMethod()) for
the operator and preserve existing array handling for onArray().
---
Nitpick comments:
In `@src/Database/Query.php`:
- Around line 842-850: Add a brief description to the containsAll() docblock
consistent with containsAny() (e.g., "Create a Query that matches items
containing all given values.") and ensure the `@param` and `@return` annotations
match the existing style used by containsAny() (keep `@return` Query if that is
the project's convention) so the docblock is consistent and descriptive for the
containsAll() factory method.
- Line 69: The constant TYPE_CONTAINS_ALL is misplaced in the "Logical methods"
section; move its declaration next to the other filter/contains constants (near
TYPE_CONTAINS and TYPE_CONTAINS_ANY) and remove the duplicate declaration from
the logical section (where TYPE_AND/TYPE_OR live) so all filter/contains
constants are grouped together and the logical combinators remain separate.
- Around line 540-550: Add the native #[Deprecated] attribute to the contains
method so it emits runtime deprecation notices: update the Query::contains
method declaration to include a #[Deprecated(...)] attribute (optionally include
a message like "Use containsAny() for arrays or contains() for strings" and a
since version) while keeping the existing `@deprecated` docblock and returning the
same signature and behavior.
In `@src/Database/Validator/Queries.php`:
- Around line 106-112: The enum-like ordering mixes contains variants: move
Query::TYPE_CONTAINS_ALL so it sits with the other contains entries
(Query::TYPE_CONTAINS, Query::TYPE_CONTAINS_ANY, Query::TYPE_NOT_CONTAINS,
Query::TYPE_CONTAINS_ALL, Query::TYPE_ELEM_MATCH) instead of after
Query::TYPE_OR; update the list in the code block where these constants are
declared so all "contains" types are co-located for consistency (refer to
Query::TYPE_CONTAINS_ALL, Query::TYPE_CONTAINS_ANY, and Query::TYPE_OR to find
the items to reorder).
In `@src/Database/Validator/Query/Filter.php`:
- Around line 377-390: The switch in Filter.php groups Query::TYPE_CONTAINS_ALL
separately from the other contains variants; update the case grouping so
TYPE_CONTAINS_ALL is adjacent to TYPE_CONTAINS and TYPE_CONTAINS_ANY (i.e.,
include TYPE_CONTAINS_ALL in the same case block that checks
$this->isEmpty($value->getValues()) and then calls
$this->isValidAttributeAndValues($attribute, $value->getValues(), $method)),
keeping the existing $this->message assignment and return behavior and
preserving use of $method and $value->getValues().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 9246-9265: The code currently replaces $relatedQueries with
Query::equal('$id', $matchingIds) when nested paths are found, which drops
direct filters; instead, keep any non-nested filters and append the $id
equality: after obtaining $matchingIds from processNestedRelationshipPath, if
not null/empty build a new array that includes the original $relatedQuery
entries whose getAttribute() does NOT contain '.' and then add
Query::equal('$id', $matchingIds) (preserving the early return when $matchingIds
is null/empty); update the $relatedQueries variable to this merged list so both
direct filters and the id filter are applied.
fogelito
reviewed
Feb 17, 2026
fogelito
approved these changes
Feb 17, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/AttributeTests.php (1)
1687-1723: Consider addingcontainsAlltests for string attributes and error paths.The array-attribute tests are correct. Two gaps worth considering:
- No
containsAlltest on non-array string attributes (tv_show,pref) — thecontainsAnysection covers these butcontainsAlldoesn't. IfcontainsAllon a plain string should behave like a multi-substring check (all substrings must be present), it would be good to have an explicit test.- No error-handling test for
containsAny/containsAllon a non-array, non-string attribute likeage— there's already such a test forcontainsat Line 1614, and parity would prevent silent regressions if validation differs across query types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/AttributeTests.php` around lines 1687 - 1723, Add tests that cover containsAll behavior on string attributes and error paths for non-array/string attributes: add cases calling Query::containsAll('tv_show', [...]) and Query::containsAll('pref', [...]) verifying expected matches when all substrings are present and non-matches when any substring is missing; and add negative tests invoking Query::containsAll('age', [...]) and Query::containsAny('age', [...]) that assert an error/validation failure (mirror the existing contains('age', ...) test at Line 1614) so query validation for containsAny/containsAll on non-array/non-string attributes is enforced consistently with Query::contains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 9254-9267: The reverse lookup in processNestedRelationshipPath is
incorrectly skipping relationship resolution for many-to-many links, causing
twoWayKey attributes to be absent and empty matchingIds; modify
processNestedRelationshipPath (or the caller convertRelationshipQueries where it
invokes processNestedRelationshipPath) to detect many-to-many relationships and
avoid calling skipRelationships() for those cases (or alternatively perform the
lookup against the junction collection), so that relationship attributes
(including the twoWayKey) are populated when calling find() on relatedCollection
and matchingIds are correctly returned.
---
Nitpick comments:
In `@tests/e2e/Adapter/Scopes/AttributeTests.php`:
- Around line 1687-1723: Add tests that cover containsAll behavior on string
attributes and error paths for non-array/string attributes: add cases calling
Query::containsAll('tv_show', [...]) and Query::containsAll('pref', [...])
verifying expected matches when all substrings are present and non-matches when
any substring is missing; and add negative tests invoking
Query::containsAll('age', [...]) and Query::containsAny('age', [...]) that
assert an error/validation failure (mirror the existing contains('age', ...)
test at Line 1614) so query validation for containsAny/containsAll on
non-array/non-string attributes is enforced consistently with Query::contains.
Contributor
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 (1)
src/Database/Database.php (1)
8913-9057:⚠️ Potential issue | 🟠 MajorIntersect nested-path matches instead of unioning them.
$allMatchingIdsis currently built viaarray_merge, which ORs different nested paths. That makes combined filters likeemployee.company.name = AANDemployee.department.name = Breturn parents that satisfy either branch. Intersect the ID sets per path group and early-return when the intersection is empty.💡 Proposed fix
- $allMatchingIds = []; + $allMatchingIds = null; foreach ($pathGroups as $path => $queryGroup) { @@ - $allMatchingIds = \array_merge($allMatchingIds, $matchingIds); + $allMatchingIds = $allMatchingIds === null + ? $matchingIds + : \array_values(\array_intersect($allMatchingIds, $matchingIds)); + + if (empty($allMatchingIds)) { + return null; + } } - return \array_unique($allMatchingIds); + return $allMatchingIds === null ? null : \array_values(\array_unique($allMatchingIds));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 8913 - 9057, The bug is that $allMatchingIds is built with array_merge (union) across $pathGroups causing OR semantics; change the logic so for the first path set $allMatchingIds = array_values(array_unique($matchingIds)), and for each subsequent path do $allMatchingIds = array_values(array_unique(array_intersect($allMatchingIds, $matchingIds))) to perform an intersection, and if at any point $allMatchingIds becomes empty return null early; update references where $allMatchingIds is used later to assume a reindexed array of unique IDs.
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)
2270-2272: Consider adding defensive cleanup for pre-existing collections.The generic names
brands,products, andtagsrisk colliding with leftover state from a failed prior run or fromtestPartialUpdateManyToManyWithStringIdsAndDocuments(which also usestags). Other tests in this trait (e.g.,testManyToManyRelationshipWithArrayOperatorsat Line 2113) already use try/catch guards for this.Proposed fix
// 3-level many-to-many chain: brands <-> products <-> tags + try { $database->deleteCollection('brands'); } catch (\Throwable $e) {} + try { $database->deleteCollection('products'); } catch (\Throwable $e) {} + try { $database->deleteCollection('tags'); } catch (\Throwable $e) {} + $database->createCollection('brands'); $database->createCollection('products'); $database->createCollection('tags');Alternatively, use more specific prefixes (e.g.,
nested_m2m_brands) to avoid name collisions entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php` around lines 2270 - 2272, The test creates collections named 'brands', 'products', and 'tags' which can collide with leftover state; modify the setup in ManyToManyTests (around the createCollection calls) to defensively clean up or avoid collisions by either (a) attempting to drop those collections first inside try/catch guards before calling $database->createCollection('brands'), $database->createCollection('products'), $database->createCollection('tags') or (b) rename them to unique test-scoped names (e.g., 'nested_m2m_brands', 'nested_m2m_products', 'nested_m2m_tags') so they don't clash with other tests like testPartialUpdateManyToManyWithStringIdsAndDocuments or testManyToManyRelationshipWithArrayOperators.
🤖 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 `@src/Database/Database.php`:
- Around line 8913-9057: The bug is that $allMatchingIds is built with
array_merge (union) across $pathGroups causing OR semantics; change the logic so
for the first path set $allMatchingIds =
array_values(array_unique($matchingIds)), and for each subsequent path do
$allMatchingIds = array_values(array_unique(array_intersect($allMatchingIds,
$matchingIds))) to perform an intersection, and if at any point $allMatchingIds
becomes empty return null early; update references where $allMatchingIds is used
later to assume a reindexed array of unique IDs.
---
Nitpick comments:
In `@tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php`:
- Around line 2270-2272: The test creates collections named 'brands',
'products', and 'tags' which can collide with leftover state; modify the setup
in ManyToManyTests (around the createCollection calls) to defensively clean up
or avoid collisions by either (a) attempting to drop those collections first
inside try/catch guards before calling $database->createCollection('brands'),
$database->createCollection('products'), $database->createCollection('tags') or
(b) rename them to unique test-scoped names (e.g., 'nested_m2m_brands',
'nested_m2m_products', 'nested_m2m_tags') so they don't clash with other tests
like testPartialUpdateManyToManyWithStringIdsAndDocuments or
testManyToManyRelationshipWithArrayOperators.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Deprecated
Tests