Repository navigation
[#508] Support related criteria paths in SleekDB joins - #509
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds join-path-aware related-model criteria to the SleekDB adapter: criteria are partitioned into root vs related scopes, applied at appropriate join depths, and parent rows lacking required related data are pruned via post-fetch filtering. DBAL public API unchanged. ChangesSleekDB Related-Model Criteria Path Filtering
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SDBal as SleekDbal
participant QB as QueryBuilder
participant Join as JoinTrait
participant SubQ as JoinSubquery
participant Result as ResultTrait
Caller->>SDBal: build query (getBuilder)
SDBal->>QB: getQueryBuilder() / prepareCriteriaScopesIfNeeded()
QB->>Join: applyJoins(currentPath='')
Join->>SubQ: applyJoin(joinPath)
SubQ->>SubQ: applyRelatedCriteriaToJoin(relatedCriteriasByPath[joinPath])
QB->>QB: fetch() -> raw rows
Result->>Result: fetchFilteredResultsFromBuilder() -> applyRelatedCriteriaPostFilter()
Result->>Caller: return filtered rows / mapped ORM models
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #509 +/- ##
============================================
+ Coverage 90.81% 90.87% +0.05%
- Complexity 2997 3069 +72
============================================
Files 262 263 +1
Lines 7906 8096 +190
============================================
+ Hits 7180 7357 +177
- Misses 726 739 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2196fa2c70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/Adapters/Sleekdb/Statements/Result.php (1)
62-97:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid calling
getBuilder()twice infindOne/findOneBy— it re-applies joins and criteria on the cached query builder.After this change, the flow becomes:
$this->getBuilder()->where([...]); // call 1: applies select/joins/rootCriterias/... $results = $this->fetchFilteredResults(); // call 2 (inside): applies them AGAINIn
SleekDbal::getBuilder()(lines 312–358), the QueryBuilder is cached but operations likeselect,applyJoins(),where($rootCriterias),having,groupBy,orderBy,skip, andlimitare reapplied on every call because they lack an "already applied" guard (onlycriteriaPreparedhas one). This means a second call togetBuilder()will re-invokeapplyJoins(), which appends duplicatejoin()operations to the cached builder, and re-apply$builder->where($this->rootCriterias)even if already added. The existing test suite doesn't catch this because it never combinesfindOne/findOneBywithjoinTo()or prior criteria.🛠️ Proposed fix — call
getBuilder()once and filter the raw resultpublic function findOne(int $id): DbalInterface { try { - $this->getBuilder()->where(['id', '=', $id]); - $results = $this->fetchFilteredResults(); + $rawResults = $this->getBuilder()->where(['id', '=', $id])->getQuery()->fetch(); + $results = $this->applyPostFetchFilters($rawResults); $result = $results[0] ?? []; $this->updateOrmModel($result); } finally { $this->resetBuilderState(); } ... public function findOneBy(string $column, $value): DbalInterface { try { - $this->getBuilder()->where([$column, '=', $value]); - $results = $this->fetchFilteredResults(); + $rawResults = $this->getBuilder()->where([$column, '=', $value])->getQuery()->fetch(); + $results = $this->applyPostFetchFilters($rawResults); $result = $results[0] ?? []; $this->updateOrmModel($result); } finally { $this->resetBuilderState(); }Alternatively, add an
appliedToBuilderflag togetBuilder()that mirrorscriteriaPreparedfor joins, where, having, orderBy, limit, and other operations — but the above is the smallest local fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Adapters/Sleekdb/Statements/Result.php` around lines 62 - 97, The findOne/findOneBy methods call getBuilder() twice which re-applies joins/criteria; fix by calling getBuilder() once: assign $builder = $this->getBuilder(), call $builder->where(...) on that instance, retrieve results directly from that same $builder (instead of invoking fetchFilteredResults() which calls getBuilder() again), pick the first result and pass it to updateOrmModel($result), then call resetBuilderState(); apply this change in the findOne and findOneBy methods (referencing getBuilder, fetchFilteredResults, updateOrmModel, findOne, findOneBy).
🧹 Nitpick comments (5)
src/Database/Adapters/Sleekdb/SleekDbal.php (3)
477-501: ⚖️ Poor tradeoffDefensive note:
unserialize($nextItem['model'])is invoked again here.Static analysis flags the same
unserialize()pattern that exists inapplyJoin()(Join.php Line 74). The data originates fromjoinTo()within the same process so the practical risk is low, but ifjoinsever becomes externally influenced (cached/serialized routes, queued jobs), this becomes an object-injection surface. Consider restricting withunserialize($nextItem['model'], ['allowed_classes' => [DbModel::class, /* known model subclasses */]])or refactoringjoinTo()to store the model reference withoutserialize()/unserialize()at all.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Adapters/Sleekdb/SleekDbal.php` around lines 477 - 501, collectJoinPathsRecursive currently calls unserialize($nextItem['model']) which can allow object injection; update collectJoinPathsRecursive to deserialize safely by using the allowed_classes option (restricting to DbModel and any known subclasses) or, better, refactor joinTo so joins[] stores the model reference instead of a serialized string; locate collectJoinPathsRecursive and change the unserialize call to use ['allowed_classes'=>[DbModel::class, /* other models */]] or remove serialize/unserialize usage in joinTo/applyJoin so the model is passed by reference rather than deserialized.
401-410: 💤 Low valueSet
criteriaPrepared = trueonly after the scope split completes successfully.
$this->criteriaPrepared = true;is assigned before the work that can throw (RuntimeExceptionat Line 451). IfgetBuilder()is re-entered after a caught exception (e.g. by a wrapping layer), the flag will skip re-preparation whilerootCriterias/relatedCriteriasByPath/requiredRelatedPathsmay be in a partial state. Moving the assignment to the end of the method makes the preparation atomic.♻️ Suggested move
protected function prepareCriteriaScopes(): void { $this->rootCriterias = $this->criterias; $this->relatedCriteriasByPath = []; $this->requiredRelatedPaths = []; - $this->criteriaPrepared = true; if ($this->joins === [] || $this->criterias === []) { + $this->criteriaPrepared = true; return; } @@ $this->requiredRelatedPaths = array_values(array_unique($this->requiredRelatedPaths)); $this->rootCriterias = $rootCriterias; + $this->criteriaPrepared = true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Adapters/Sleekdb/SleekDbal.php` around lines 401 - 410, The method prepareCriteriaScopes() sets $this->criteriaPrepared = true before performing work that can throw (see RuntimeException in the method and callers like getBuilder()), which can leave $this->rootCriterias, $this->relatedCriteriasByPath, and $this->requiredRelatedPaths in a partial state; move the assignment of $this->criteriaPrepared = true to the very end of prepareCriteriaScopes() after all splitting/preparation logic completes successfully so the prepared flag is only set when the scope split finishes without exceptions.
613-625: 💤 Low valueOptimize
isList()to use native PHP 8.1+array_is_list()when available.The method reimplements
array_is_list()(available since PHP 8.1). Since the project targets PHP 7.4+, usefunction_exists()to feature-detect and call the native function on 8.1+ runtimes, falling back to the manual loop on earlier versions:♻️ Suggested optimization
protected function isList(array $value): bool { + if (function_exists('array_is_list')) { + return array_is_list($value); + } + $index = 0; foreach ($value as $key => $_) { if ($key !== $index) { return false; } $index++; } return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Adapters/Sleekdb/SleekDbal.php` around lines 613 - 625, The isList method currently reimplements array_is_list; update SleekDbal::isList to feature-detect and call the native array_is_list() when available by checking function_exists('array_is_list') and otherwise fall back to the existing loop implementation in the isList(array $value): bool method so behavior remains the same on PHP 7.4 while using the native function on PHP 8.1+.CHANGELOG.md (1)
82-82: 💤 Low valueConsider moving this entry from
### Fixedto### Added/### Changed.The other entries in the
Fixedsection are bug/compat fixes (PHP 8.1/8.4 deprecations, error message assertions, magic-method type hints), whereas this is a new capability for the SleekDB adapter. It would categorize more accurately underAdded(orChanged).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` at line 82, The changelog entry "SleekDB adapter now supports related-model criteria path filtering across join depths while preserving DBAL public API (`#508`)" is a new capability and should be moved out of the "### Fixed" section into a more appropriate section such as "### Added" or "### Changed"; update CHANGELOG.md by cutting that exact line from the Fixed block and pasting it under "### Added" (or "### Changed") so the entry is categorized correctly.tests/Unit/Database/Adapters/Sleekdb/Statements/JoinSleekTest.php (1)
296-371: 💤 Low valueTest coverage matches the PR's acceptance criteria; one note about the invalid-path test.
The five new tests cover immediate, deep, AND-combined, OR-rejection, and invalid-path scenarios as outlined in the issue. One observation:
testSleekInvalidRelatedCriteriaPathReturnsNoMatchespasses becauseunknown_relation.firstnamedoesn't match any join path → the criterion falls back to root → SleekDB filters by a non-existent field → 0 rows. That's the documented behavior, but the result also holds for any unknown root column, so a follow-up assertion (e.g. that the user count without the bad criterion is > 0) would make the test explicitly distinguish "invalid related path" from "valid criterion that happens to match nothing".The OR-rejection test (Line 342) is contingent on
prepareCriteriaScopes()actually detecting OR in the nested-array form — see the separate comment onSleekDbal::prepareCriteriaScopes().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Database/Adapters/Sleekdb/Statements/JoinSleekTest.php` around lines 296 - 371, The invalid-path test (testSleekInvalidRelatedCriteriaPathReturnsNoMatches) should explicitly prove the zero-result is due to the bad related-path fallback, not just a non-matching value: after the existing query that uses ->criteria('unknown_relation.firstname', '=', 'Jane') and asserts count 0, add a second query using the same TestUserModel (without joining TestProfileModel and without the invalid related-path criterion) to assert the baseline user count is > 0 (e.g. $this->assertGreaterThan(0, $usersWithoutBadCriterion->count())). This makes clear the behavior of prepareCriteriaScopes()/criteria fallback rather than a dataset that genuinely has no users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Database/Adapters/Sleekdb/Statements/Result.php`:
- Around line 62-97: The findOne/findOneBy methods call getBuilder() twice which
re-applies joins/criteria; fix by calling getBuilder() once: assign $builder =
$this->getBuilder(), call $builder->where(...) on that instance, retrieve
results directly from that same $builder (instead of invoking
fetchFilteredResults() which calls getBuilder() again), pick the first result
and pass it to updateOrmModel($result), then call resetBuilderState(); apply
this change in the findOne and findOneBy methods (referencing getBuilder,
fetchFilteredResults, updateOrmModel, findOne, findOneBy).
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 82: The changelog entry "SleekDB adapter now supports related-model
criteria path filtering across join depths while preserving DBAL public API
(`#508`)" is a new capability and should be moved out of the "### Fixed" section
into a more appropriate section such as "### Added" or "### Changed"; update
CHANGELOG.md by cutting that exact line from the Fixed block and pasting it
under "### Added" (or "### Changed") so the entry is categorized correctly.
In `@src/Database/Adapters/Sleekdb/SleekDbal.php`:
- Around line 477-501: collectJoinPathsRecursive currently calls
unserialize($nextItem['model']) which can allow object injection; update
collectJoinPathsRecursive to deserialize safely by using the allowed_classes
option (restricting to DbModel and any known subclasses) or, better, refactor
joinTo so joins[] stores the model reference instead of a serialized string;
locate collectJoinPathsRecursive and change the unserialize call to use
['allowed_classes'=>[DbModel::class, /* other models */]] or remove
serialize/unserialize usage in joinTo/applyJoin so the model is passed by
reference rather than deserialized.
- Around line 401-410: The method prepareCriteriaScopes() sets
$this->criteriaPrepared = true before performing work that can throw (see
RuntimeException in the method and callers like getBuilder()), which can leave
$this->rootCriterias, $this->relatedCriteriasByPath, and
$this->requiredRelatedPaths in a partial state; move the assignment of
$this->criteriaPrepared = true to the very end of prepareCriteriaScopes() after
all splitting/preparation logic completes successfully so the prepared flag is
only set when the scope split finishes without exceptions.
- Around line 613-625: The isList method currently reimplements array_is_list;
update SleekDbal::isList to feature-detect and call the native array_is_list()
when available by checking function_exists('array_is_list') and otherwise fall
back to the existing loop implementation in the isList(array $value): bool
method so behavior remains the same on PHP 7.4 while using the native function
on PHP 8.1+.
In `@tests/Unit/Database/Adapters/Sleekdb/Statements/JoinSleekTest.php`:
- Around line 296-371: The invalid-path test
(testSleekInvalidRelatedCriteriaPathReturnsNoMatches) should explicitly prove
the zero-result is due to the bad related-path fallback, not just a non-matching
value: after the existing query that uses
->criteria('unknown_relation.firstname', '=', 'Jane') and asserts count 0, add a
second query using the same TestUserModel (without joining TestProfileModel and
without the invalid related-path criterion) to assert the baseline user count is
> 0 (e.g. $this->assertGreaterThan(0, $usersWithoutBadCriterion->count())). This
makes clear the behavior of prepareCriteriaScopes()/criteria fallback rather
than a dataset that genuinely has no users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a97f2d2a-f59f-4d89-b7e3-ed5d2014d175
📒 Files selected for processing (5)
CHANGELOG.mdsrc/Database/Adapters/Sleekdb/SleekDbal.phpsrc/Database/Adapters/Sleekdb/Statements/Join.phpsrc/Database/Adapters/Sleekdb/Statements/Result.phptests/Unit/Database/Adapters/Sleekdb/Statements/JoinSleekTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Database/Adapters/Sleekdb/SleekDbal.php`:
- Around line 429-461: The partition loop in SleekDbal.php can miss related-path
columns inside grouped criteria (nested arrays), allowing cross-scope ORs to
bypass the existing guard; update the foreach that processes $this->criterias to
also inspect grouped array entries (e.g., when $criteria[0] is itself an array)
by iterating that group and calling matchRelatedPath(...) for any element with a
string column, setting $foundRelated and moving related conditions into
$this->relatedCriteriasByPath[$path] (or immediately throwing the same
RuntimeException when $hasOr is true), and reject or document groups containing
dotted related columns; also add a unit test in JoinSleekTest that asserts
grouped related-path criteria cause the same RuntimeException (or are rejected)
to lock the behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75428431-46b9-454f-b36e-a42f9f23b3d2
📒 Files selected for processing (4)
CHANGELOG.mdsrc/Database/Adapters/Sleekdb/SleekDbal.phpsrc/Database/Adapters/Sleekdb/Statements/Result.phptests/Unit/Database/Adapters/Sleekdb/Statements/JoinSleekTest.php
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Database/Adapters/Sleekdb/Statements/JoinSleekTest.php
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Database/Adapters/Sleekdb/Statements/RelatedCriteria.php (1)
56-88:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGrouped related criteria still do not route by scope.
This splitter only handles flat top-level predicates. A grouped entry like
[['profiles.firstname', '=', 'John'], 'OR', ['profiles.lastname', '=', 'Doe']]hits the!is_string($criteria[0])branch, gets pushed into$rootCriterias, and never reaches$relatedCriteriasByPath. That means mixed-scope groups can still bypass the OR guard, while the blanket$foundRelated && $hasOrcheck also rejects valid ORs that stay inside a single related path. Please recurse through grouped criteria and only reject groups that actually mix scopes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Adapters/Sleekdb/Statements/RelatedCriteria.php` around lines 56 - 88, The loop over $this->criterias currently treats grouped predicates as opaque and pushes them into $rootCriterias, so grouped related predicates never get routed to $this->relatedCriteriasByPath and OR-mixing checks are wrong; update the loop to detect when a $criteria is itself a grouped array (e.g. first element is an array of predicates) and iterate that inner group, using $this->matchRelatedPath($column, $joinPaths) for each predicate to classify it into root vs related (populate $rootCriterias, $this->relatedCriteriasByPath[$path][] and $this->requiredRelatedPaths accordingly), propagate OR tokens inside the group to $hasOr, set $foundRelated only when any predicate matched a related path, and only throw the RuntimeException when a single group actually mixes scopes (i.e. when predicates within the same group resolve to both root and related paths) or when top-level mixing occurs across groups; keep using the existing symbols $this->criterias, matchRelatedPath, $rootCriterias, $this->relatedCriteriasByPath, $this->requiredRelatedPaths, $foundRelated and $hasOr to locate and modify the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Database/Adapters/Sleekdb/SleekDbal.php`:
- Around line 308-314: getBuilder() reuses a cached QueryBuilder but calls
applyBuilderModifiers($builder) every time, causing modifiers
(where/join/orderBy/pagination) to be replayed on subsequent calls; fix by
ensuring modifiers are applied only once per builder lifecycle or by creating a
fresh builder each call: modify getBuilder() to either (a) call
$this->getQueryBuilder() and then applyBuilderModifiers() only when the builder
was just created (e.g. track a flag like $this->builderPrepared or check if
$this->getQueryBuilder() returned a new instance) or (b) always clone/reset the
builder from getQueryBuilder() so applyBuilderModifiers() operates on a fresh
QueryBuilder; adjust prepareCriteriaScopesIfNeeded()/applyBuilderModifiers()
accordingly to avoid mutating the cached builder multiple times.
In `@src/Database/Adapters/Sleekdb/Statements/Result.php`:
- Around line 50-54: The cloned model returned from the array_map closure is
keeping stale query state (cached $queryBuilder and prepared criteria) — before
calling updateOrmModel on the clone, clear its query state. Modify the closure
in Result.php so each clone has its query-related caches reset (e.g. clear
$queryBuilder and prepared criteria) by calling an explicit reset method (create
one like resetQueryState() if none exists) or by nulling/emptying those
properties on $item prior to updateOrmModel($element).
---
Duplicate comments:
In `@src/Database/Adapters/Sleekdb/Statements/RelatedCriteria.php`:
- Around line 56-88: The loop over $this->criterias currently treats grouped
predicates as opaque and pushes them into $rootCriterias, so grouped related
predicates never get routed to $this->relatedCriteriasByPath and OR-mixing
checks are wrong; update the loop to detect when a $criteria is itself a grouped
array (e.g. first element is an array of predicates) and iterate that inner
group, using $this->matchRelatedPath($column, $joinPaths) for each predicate to
classify it into root vs related (populate $rootCriterias,
$this->relatedCriteriasByPath[$path][] and $this->requiredRelatedPaths
accordingly), propagate OR tokens inside the group to $hasOr, set $foundRelated
only when any predicate matched a related path, and only throw the
RuntimeException when a single group actually mixes scopes (i.e. when predicates
within the same group resolve to both root and related paths) or when top-level
mixing occurs across groups; keep using the existing symbols $this->criterias,
matchRelatedPath, $rootCriterias, $this->relatedCriteriasByPath,
$this->requiredRelatedPaths, $foundRelated and $hasOr to locate and modify the
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ff925e6-c99f-4ffd-bdab-727279c147d8
📒 Files selected for processing (4)
src/Database/Adapters/Sleekdb/SleekDbal.phpsrc/Database/Adapters/Sleekdb/Statements/Join.phpsrc/Database/Adapters/Sleekdb/Statements/RelatedCriteria.phpsrc/Database/Adapters/Sleekdb/Statements/Result.php
2 participants
Closes #508
Summary by CodeRabbit
Bug Fixes
Tests
Documentation