Unify SELECT / Collection under PostQueryInterface (replaces #87)#89
Unify SELECT / Collection under PostQueryInterface (replaces #87)#89
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits 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. 📝 WalkthroughWalkthroughDbQueryInterceptor now builds a Fetch for PostQuery return types and passes it into SqlQuery::execPostQuery; SqlQuery/SqlQueryInterface accept an optional Fetch and populate PostQueryContext::$rows with fetched SELECT rows. Tests, fakes, fixtures, and docs were added to exercise associative and hydrated SELECT results via PostQueryInterface wrappers. ChangesSELECT Post-Query Flow
Sequence DiagramsequenceDiagram
participant Client
participant DbQueryInterceptor
participant FetchFactory
participant SqlQuery
participant PostQueryContext
participant Articles
Client->>DbQueryInterceptor: invoke(dbQuery, entity, returnType)
DbQueryInterceptor->>FetchFactory: factory(dbQuery, entity, returnType)
FetchFactory-->>DbQueryInterceptor: postQueryFetch
DbQueryInterceptor->>SqlQuery: execPostQuery(sqlId, values, postQueryClass, postQueryFetch)
SqlQuery->>SqlQuery: rows = perform(sqlId, values, postQueryFetch)
SqlQuery->>PostQueryContext: __construct(statement, pdo, lastValues, rows)
SqlQuery->>Articles: fromContext(PostQueryContext)
Articles-->>Client: Articles instance (uses context.rows)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
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)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/DbQuerySelectPostQueryTest.php (1)
28-28: Nit: simplify sql directory path.
dirname(__DIR__) . '/tests/sql'resolves to<repo>/tests/sql, but since the test file already lives intests/, this is equivalent to__DIR__ . '/sql'and avoids assuming the parent directory name istests.Proposed tweak
- $sqlDir = dirname(__DIR__) . '/tests/sql'; + $sqlDir = __DIR__ . '/sql';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/DbQuerySelectPostQueryTest.php` at line 28, Nit: simplify sql directory path — replace the computed dirname expression with the simpler relative path; in the test file DbQuerySelectPostQueryTest.php change the assignment to $sqlDir so it uses __DIR__ . '/sql' instead of dirname(__DIR__) . '/tests/sql' to avoid assuming the parent directory name; update the $sqlDir variable initialization (look for the line that assigns $sqlDir) and run tests to confirm paths still resolve.
🤖 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/SqlQueryInterface.php`:
- Around line 64-67: The docs currently imply a non-null $fetch always yields
entity instances; clarify that row shape depends on the fetch strategy produced
by FetchFactory (e.g., FetchAssoc yields associative arrays). Update the
sentence describing SELECT behavior to state that for SELECT statements the rows
exposed on the context's `$rows` property are shaped according to the provided
`$fetch` strategy (entity instances when the fetch returns entities, associative
arrays when using FetchAssoc, etc.), while for DML statements `$rows` remains
`[]`. Ensure references to `$fetch`, `FetchFactory`, `FetchAssoc`, and the
context's `$rows` are preserved so callers can locate the behavior.
In `@tests/DbQuerySelectPostQueryTest.php`:
- Around line 42-74: The tests testReturnsArticlesWrapperWithAssocRows and
testReturnsArticlesWrapperWithHydratedEntities assume insertion order; to fix,
make the expectation order-independent by either (A) adding a deterministic
ORDER BY id to the query used by ArticlesInterface::listAssoc and
ArticlesInterface::listHydrated (ensure the SQL that builds those results—or
todo_list.sql fixture—includes "ORDER BY id"), or (B) change the tests to
normalize/sort the returned rows before asserting (e.g., sort $result->rows by
'id' or reindex by id before comparing and before accessing
$result->rows[0]/[1]) so they no longer rely on implicit DB order. Use the
symbols ArticlesInterface::listAssoc, ArticlesInterface::listHydrated, Articles
(rows), Article (entities), and todo_list.sql to locate where to apply the
change.
---
Nitpick comments:
In `@tests/DbQuerySelectPostQueryTest.php`:
- Line 28: Nit: simplify sql directory path — replace the computed dirname
expression with the simpler relative path; in the test file
DbQuerySelectPostQueryTest.php change the assignment to $sqlDir so it uses
__DIR__ . '/sql' instead of dirname(__DIR__) . '/tests/sql' to avoid assuming
the parent directory name; update the $sqlDir variable initialization (look for
the line that assigns $sqlDir) and run tests to confirm paths still resolve.
🪄 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: 4940a3f3-5004-4891-b25a-826b4973ec06
📒 Files selected for processing (10)
src/DbQueryInterceptor.phpsrc/Result/PostQueryContext.phpsrc/ReturnEntity.phpsrc/SqlQuery.phpsrc/SqlQueryInterface.phptests/DbQuerySelectPostQueryTest.phptests/Fake/Entity/Article.phptests/Fake/FakeSqlQuery.phptests/Fake/Queries/ArticlesInterface.phptests/Fake/Result/Articles.php
SELECT return types can now implement `PostQueryInterface`. The framework pre-hydrates the result set into `PostQueryContext::$rows` (entity instances when an entity is configured, associative arrays otherwise), so a collection wrapper like `Articles<Article>` can be composed by the factory without re-fetching. - `PostQueryContext`: add `array $rows = []` (empty for DML paths). - `SqlQueryInterface::execPostQuery` / `SqlQuery::execPostQuery`: accept an optional `FetchInterface` and feed hydrated rows into the context. - `DbQueryInterceptor`: build a fetch strategy via `FetchFactory` for `PostQueryInterface` return types and forward it. - `ReturnEntity`: bypass class-name resolution for `PostQueryInterface` wrappers (same treatment as `PagesInterface`) so the docblock's `Articles<Article>` generic yields `Article` as the row entity.
koriym
force-pushed
the
collection-via-postquery
branch
from
56c9bda to
8acc97b
Compare
- SqlQueryInterface docblock: row shape follows the `$fetch` strategy (not "entity vs none"). FetchFactory may return FetchAssoc, which produces associative arrays even with $fetch non-null. - DbQuerySelectPostQueryTest: don't rely on implicit DB order. Sort rows by id before asserting; decouples the tests from todo_list.sql (which has no ORDER BY and is shared with many other tests).
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
Tip For best results, initiate chat on the files or code changes.
[resolve] |
There was a problem hiding this comment.
Pull request overview
This PR extends the existing PostQueryInterface mechanism so SELECT queries can return typed “post-query” result objects (e.g., domain collections) in the same way DML queries return AffectedRows/InsertedRow. It does so by pre-hydrating SELECT rows in the framework and exposing them via PostQueryContext::$rows, then letting the PostQueryInterface::fromContext() factory wrap/compose the result.
Changes:
- Add hydrated SELECT rows to
PostQueryContextand plumb them throughSqlQuery::execPostQuery(). - Update the interceptor to provide a
FetchInterfacewhen dispatchingPostQueryInterfacereturn types (SELECT + DML share the same dispatch). - Add new fakes + an integration test covering SELECT returning a
PostQueryInterfacewrapper with both assoc rows and hydrated entities.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Fake/Result/Articles.php | Adds a fake SELECT-path PostQueryInterface wrapper used by tests. |
| tests/Fake/Queries/ArticlesInterface.php | Defines test repo methods returning Articles with/without docblock-driven hydration. |
| tests/Fake/FakeSqlQuery.php | Updates execPostQuery signature to match the interface change. |
| tests/Fake/Entity/Article.php | Adds a simple entity used for hydration assertions. |
| tests/DbQuerySelectPostQueryTest.php | New integration test validating SELECT → PostQueryInterface wrapper and $context->rows hydration. |
| src/SqlQueryInterface.php | Extends execPostQuery to accept an optional FetchInterface for SELECT hydration. |
| src/SqlQuery.php | Passes fetched rows into PostQueryContext for SELECT; keeps DML rows as []. |
| src/ReturnEntity.php | Avoids treating PostQueryInterface return types as “entity” return types so generics can resolve entity correctly. |
| src/Result/PostQueryContext.php | Adds public readonly array $rows = [] and documents SELECT vs DML behavior. |
| src/DbQueryInterceptor.php | Builds a FetchInterface and forwards it into execPostQuery for PostQueryInterface return types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -71,7 +77,7 @@ public function exec(string $sqlId, array $values = [], FetchInterface|null $fet | |||
| * @template T of PostQueryInterface | |||
| * @psalm-taint-escape sql | |||
| */ | |||
| public function execPostQuery(string $sqlId, array $values, string $postQueryClass): PostQueryInterface; | |||
| public function execPostQuery(string $sqlId, array $values, string $postQueryClass, FetchInterface|null $fetch = null): PostQueryInterface; | |||
There was a problem hiding this comment.
The execPostQuery() PHPDoc lists $values and $postQueryClass but the method signature now also accepts $fetch. Please add a @param FetchInterface|null $fetch entry so the documentation and static-analysis hints match the actual API.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/ReturnEntity.php:53
ReturnEntitynow excludes any return type that implementsPostQueryInterfacefrom being treated as the entity class. Becausedocblock()only extracts an entity fromarray<T>/Generic<T>return annotations, a SELECT method whose return type is itself an entity implementingPostQueryInterface(i.e. no@return Wrapper<Entity>generic) will end up with$entity === nulland therefore skip entity hydration (rows become assoc arrays). Consider falling back to the concrete return type as the entity whendocblock()returns null, while still preferring the docblock generic when present (so wrapper types likeArticles<Article>keep working).
if (
class_exists($returnTypeClass)
&& ! is_a($returnTypeClass, PagesInterface::class, true)
&& ! is_a($returnTypeClass, PostQueryInterface::class, true)
) {
return $returnTypeClass;
}
return $this->docblock($method);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Builds on the SELECT-side PostQueryInterface introduced in 8acc97b: - PostQueryInterface / SqlQueryInterface / README docs now reflect the unified SELECT + DML semantics; the original DML-only wording was stale after this PR routed SELECT through the same dispatch. - New fixtures (article_list.sql / article_list_empty.sql) decouple the Articles tests from the shared todo_list.sql so the test no longer relies on implicit row order or unrelated edits. - Articles fake gains IteratorAggregate, Countable, isEmpty, plus a @template T parameter so @return Articles<Entity> propagates Entity through to $rows[N], foreach, and iterator_to_array — six runtime type checks fall away because the docblock now carries the narrow. - Three new tests cover factory: combined with PostQueryInterface, empty SELECT, and the Countable / IteratorAggregate ergonomics. - README adds a "Generic base for reuse across repositories" example showing TypedRows<T> + extends TypedRows<Article> as the pattern for sharing a wrapper across entity types. Addresses Copilot review on src/SqlQueryInterface.php (missing @param FetchInterface|null $fetch). composer test (101 / 176), composer cs, composer sa — all clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@NaokiTsuchiya 今日のcommitで /** @implements IteratorAggregate<int, Article> */と宣言することで、以下のように interface ArticleRepository
{
#[DbQuery('article_list')]
/** @return Articles<Article> */
public function list(): Articles;
}（以前はmixedでした） |
|
What this PR enables Before this, SELECT can return After this, SELECT returns ride the same final class Articles implements PostQueryInterface, IteratorAggregate, Countable
{
/** @param list<Article> $rows */
public function __construct(public readonly array $rows) {}
public static function fromContext(PostQueryContext $context): static {
/** @var list<Article> $rows */
return new static($context->rows);
}
public function published(): self { /* domain predicate */ }
public function totalWordCount(): int { /* aggregation */ }
public function getIterator(): ArrayIterator { return new ArrayIterator($this->rows); }
public function count(): int { return count($this->rows); }
}$articles->published()->totalWordCount(); // domain language
foreach ($articles as $a) { /* $a: Article via @return Articles<Article> */ }
count($articles); // CountableWhere this matters
These aren't novel patterns. The novelty is that they now have a framework-blessed shape: single-dispatch routing, generic-aware return types, and Design choices worth highlighting
Adoption note for HAL / JSON consumers Wrappers landing in private function normalize(mixed $v): mixed {
if ($v instanceof JsonSerializable) return $v;
if ($v instanceof Traversable) return array_map([$this, 'normalize'], iterator_to_array($v));
if (is_array($v)) return array_map([$this, 'normalize'], $v);
return $v;
}Existing Minor README suggestion on Lead the example with composition ( |
Show that PostQueryInterface dispatches on the last executed statement, so a single repository method can run a DML and return a typed view of the affected row via a trailing SELECT — no driver-specific RETURNING. Clarifies the meaning of $context->rows === [].
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@README.md`:
- Around line 424-428: The example uses the SQLite-specific function
last_insert_rowid() in create_article.sql which is not portable; update the
README to mark that SELECT * FROM articles WHERE id = last_insert_rowid(); is
SQLite-only and provide alternatives (e.g., use RETURNING * for
PostgreSQL/MySQL: INSERT ... RETURNING *, or call your DB driver’s
lastInsertId()/getGeneratedKeys() API) and mention that driver-specific helper
functions should be used when RETURNING is not available.
- Line 710: Update the README method signature for execPostQuery to document the
concrete fetch type by replacing or annotating `$fetch = null` with
`FetchInterface|null $fetch = null`; specifically, in the method list entry that
currently reads "execPostQuery($queryId, $params, $postQueryClass, $fetch =
null)", change it to reference FetchInterface so readers understand the typed
fetch forwarding and the accepted null value (use the symbol execPostQuery and
the FetchInterface name to locate the text).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Mark `last_insert_rowid()` in the DML+SELECT example as SQLite-specific and point to `LAST_INSERT_ID()` / `RETURNING` for other drivers. - Spell out `FetchInterface|null $fetch = null` in the execPostQuery method-list entry so the typed fetch forwarding is visible at a glance.
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
Tip For best results, initiate chat on the files or code changes.
[resolve] |
PR #89 broadens PostQueryInterface from DML-only to cover SELECT typed collection wrappers. Update the AI-oriented reference to document the new $rows field on PostQueryContext, the typed-collection pattern, the DML+SELECT use case, and the new $fetch parameter on execPostQuery.
Implements the unified design from #88 and supersedes #87 (closed).
What changed
SELECT return types can now implement
PostQueryInterfacejust like DML results. The framework pre-hydrates the row list intoPostQueryContext::$rows; the factory wraps them in a typed collection that carries domain operations on the result set as a whole — which is what a rawarray<Article>can't express.Callers get
$articles->published()->totalWordCount()— domain logic about the collection lives on the type, not scattered across services.IteratorAggregate/Countablejust give the wrapper the normal "feels like an array" ergonomics on top. Wrapping a Laravel / Illuminate / Doctrine Collection via a property is the same pattern when you want a richer base.Why this replaces #87
#87 auto-detected
Traversable+ instantiable + constructor-param-count via reflection and invokednew ReflectionClass(...)->newInstanceArgs([$rows]). The new approach:DbQueryInterceptoralready had theis_subclass_of(..., PostQueryInterface::class)branch for DML. SELECT now rides on the same check. No separate detector class, no magic constructor invocation.implements PostQueryInterface+ a small factory is the boilerplate cost. In exchange: type-safestaticreturns, alignment with the DML path, and no reflection-based magic.FetchInterface/FetchFactory/Injectorproducearray<Article>, the framework hands it to the factory via$context->rows. The wrapper never touches raw rows or DI.See #88 for the design memo and the rationale in full.
Implementation
src/Result/PostQueryContext.php— addspublic readonly array $rows = []. Hydrated for SELECT, empty for DML.src/SqlQueryInterface.php/src/SqlQuery.php—execPostQuerygainsFetchInterface|null $fetch = null.SqlQuerypasses$fetchthroughperform()(already SELECT/DML-aware) and populatesPostQueryContext::$rowswith the returned rows.src/DbQueryInterceptor.php— builds aFetchInterfacevia the existingFetchFactoryand forwards it toexecPostQueryforPostQueryInterfacereturn types. No new branches.src/ReturnEntity.php— treatsPostQueryInterfaceimplementations the same asPagesInterface: skip the class-name return so a docblock like@return Articles<Article>resolves toArticlevia the existing generic-extraction path.Follow-up commit (fa84611)
Tightens documentation, tests, and generic typing without changing the production surface beyond docblocks:
PostQueryInterface,SqlQueryInterface, and the README now reflect the unified SELECT + DML coverage. The original "DML results" wording was stale after this PR routed SELECT through the same path. Addresses Copilot review (missing@param FetchInterface|null $fetch).Articleswrapper showingpublished()/totalWordCount()/IteratorAggregate/Countable, plus shape rules for$rowsand the meaning of$rows === [].TypedRows<T>base class withextends TypedRows<Article>/extends TypedRows<User>for sharing a wrapper across entity types. The narrow happens at the@var list<T>line infromContext(); from there Psalm and PHPStan honour the parameter throughforeach,$rows[N],iterator_to_array(...), and any derived method.todo_list.sql— newarticle_list.sql(withORDER BY id) andarticle_list_empty.sqlso the Articles tests no longer share fixtures with the rest of the suite.IteratorAggregate,Countable,isEmpty, and@template T— its@return Articles<Entity>propagation is verified end-to-end. As a positive side-effect, six runtime type checks (assertContainsOnlyInstancesOf× 2,assertInstanceOf× 4) fall away because the docblock carries the narrow.factory:attribute combined withPostQueryInterface, empty SELECT, and theCountable/IteratorAggregateergonomics.Test plan
composer test— 101 tests, 176 assertions (was 98 before fa84611; +3 new tests, runtime instance assertions removed where the docblock narrow makes them redundant)composer cs— clean./vendor-bin/tools/vendor/bin/phpstan --memory-limit=1G— no errors (level max)./vendor-bin/tools/vendor/bin/psalm --no-cache— no errors (error level 1)DbQuerySelectPostQueryTestcovers:Articleswrapper returned from a SELECT$context->rowsas hydratedArticleinstances when@return Articles<Article>is declared$context->rowsasTodoConstructinstances whenfactory: TodoEntityFactory::classis supplied$context->rowsas raw assoc arrays when no entity type is declaredArticleswrapper with$rows === [],count() === 0,isEmpty() === trueiterator_to_array($articles)matches$articles->rowsAffectedRows,InsertedRow,DbQueryCustomPostQueryTest::RowCountWithQuery) still work — their$rowsstays[], other fields unchangedRefs #88 (design memo), #87 (closed, superseded).
Summary by CodeRabbit
New Features
Tests