[#514] Preserve SleekDB criteria state across paginator count#515
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR fixes a SleekDB paginator regression where calling ChangesSleekDB Criteria Preservation in Pagination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #515 +/- ##
=========================================
Coverage 90.87% 90.87%
- Complexity 3069 3070 +1
=========================================
Files 263 263
Lines 8096 8100 +4
=========================================
+ Hits 7357 7361 +4
Misses 739 739 ☔ 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: 432b88d79d
ℹ️ 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.
🧹 Nitpick comments (2)
src/Database/Adapters/Sleekdb/Statements/Result.php (1)
131-135: ⚡ Quick winConsider adding an explanatory comment for the cloning approach.
The cloning strategy differs from the try/finally pattern used in
get(),findOne(),findOneBy(), andfirst(). Adding a brief inline comment explaining why cloning is necessary here (to preserve criteria state for subsequent operations like pagination) would help future maintainers understand the design decision.📝 Suggested comment addition
public function count(): int { + // Clone to preserve original criteria state for subsequent operations (e.g., paginate()->data()) + // Nulling queryBuilder ensures getBuilder() reconstructs a fresh builder with preserved criteria $counter = clone $this; $counter->queryBuilder = null; $counter->builderPrepared = false;🤖 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 131 - 135, Add a brief inline comment above the cloning block that explains why clone $this is used instead of the try/finally pattern: note that cloning preserves the original Result instance's query/criteria state while allowing $counter->queryBuilder = null and $counter->builderPrepared = false to be modified for a count operation via fetchFilteredResultsFromBuilder($counter->getBuilder()), which avoids altering state needed by methods like get(), findOne(), findOneBy(), and first() (use the symbols clone $this, $counter, fetchFilteredResultsFromBuilder, get, findOne, findOneBy, first in the comment).tests/Unit/Database/Adapters/Sleekdb/Statements/ResultSleekTest.php (1)
90-119: ⚡ Quick winConsider adding test coverage for grouped OR criteria.
The tests validate that basic
criteria()chains are preserved acrosscount()andpaginate()workflows. However, the PR objectives specifically mention testing "grouped OR criteria viacriterias([...], [...])to ensure filters persist and no stale builder side effects occur."Adding a test case that uses
criterias()with multiple grouped conditions would provide more comprehensive validation of the 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 `@tests/Unit/Database/Adapters/Sleekdb/Statements/ResultSleekTest.php` around lines 90 - 119, Add a test that exercises grouped OR criteria using the criterias([...], [...]) API and asserts those grouped filters persist across count() and paginate() calls: instantiate the model (e.g., TestEventModel or SleekDbal), apply criterias with two grouped condition arrays (one group AND, the other OR), call count() and then paginate() (or get()) and assert the returned count and paginated results match the expected filtered set and ordering (use criterias, count, paginate, orderBy, get and model/TestEventModel/SleekDbal to locate where to add the test).
🤖 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.
Nitpick comments:
In `@src/Database/Adapters/Sleekdb/Statements/Result.php`:
- Around line 131-135: Add a brief inline comment above the cloning block that
explains why clone $this is used instead of the try/finally pattern: note that
cloning preserves the original Result instance's query/criteria state while
allowing $counter->queryBuilder = null and $counter->builderPrepared = false to
be modified for a count operation via
fetchFilteredResultsFromBuilder($counter->getBuilder()), which avoids altering
state needed by methods like get(), findOne(), findOneBy(), and first() (use the
symbols clone $this, $counter, fetchFilteredResultsFromBuilder, get, findOne,
findOneBy, first in the comment).
In `@tests/Unit/Database/Adapters/Sleekdb/Statements/ResultSleekTest.php`:
- Around line 90-119: Add a test that exercises grouped OR criteria using the
criterias([...], [...]) API and asserts those grouped filters persist across
count() and paginate() calls: instantiate the model (e.g., TestEventModel or
SleekDbal), apply criterias with two grouped condition arrays (one group AND,
the other OR), call count() and then paginate() (or get()) and assert the
returned count and paginated results match the expected filtered set and
ordering (use criterias, count, paginate, orderBy, get and
model/TestEventModel/SleekDbal to locate where to add the test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac83240b-0fbf-4f7e-83ae-9bffa4ead722
📒 Files selected for processing (3)
CHANGELOG.mdsrc/Database/Adapters/Sleekdb/Statements/Result.phptests/Unit/Database/Adapters/Sleekdb/Statements/ResultSleekTest.php
2 participants
Fixes #514
Summary by CodeRabbit
Bug Fixes
Tests