Skip to content

Conversation

@koriym
Copy link
Member

@koriym koriym commented Jan 9, 2026

Summary

  • Add DataLoader pattern to solve N+1 problem in crawl operations
  • DataLoader batches multiple resource requests into a single efficient query
  • 100% backward compatible - opt-in via dataLoader parameter in #[Link] annotation

The name "DataLoader" comes from Facebook's DataLoader pattern, which provides a consistent API for batching and caching data loading operations.

Closes #345

Documentation: bearsunday/bearsunday.github.io#327

The N+1 Problem in Crawl

When crawling resources with links, each child resource triggers individual queries:

Request: GET /author/1 with linkCrawl('post-tree')

[Query 1] SELECT * FROM author WHERE id = 1
  └─ Author has 3 posts
     
[Query 2] SELECT * FROM post WHERE author_id = 1
  └─ Returns 3 posts (id: 10, 11, 12)
     
[Query 3] SELECT * FROM meta WHERE post_id = 10  ← N+1 starts here
[Query 4] SELECT * FROM meta WHERE post_id = 11
[Query 5] SELECT * FROM meta WHERE post_id = 12

Total: 5 queries (grows with data size!)

With DataLoader

[Query 1] SELECT * FROM author WHERE id = 1
[Query 2] SELECT * FROM post WHERE author_id = 1
[Query 3] SELECT * FROM meta WHERE post_id IN (10, 11, 12)  ← Batched!

Total: 3 queries (constant regardless of data size)

Interface

interface DataLoaderInterface
{
    /**
     * @param list<array<string, mixed>> $queries
     * @return list<array<string, mixed>>
     */
    public function __invoke(array $queries): array;
}

Usage

#[Link(crawl: 'tree', rel: 'meta', href: 'app://self/meta{?post_id}', dataLoader: MetaDataLoader::class)]
class MetaDataLoader implements DataLoaderInterface
{
    public function __invoke(array $queries): array
    {
        $postIds = array_column($queries, 'post_id');
        
        // SELECT * FROM meta WHERE post_id IN (...)
        return $this->pdo->fetchAll($sql, ['post_ids' => $postIds]);
    }
}

Supported URI Template Formats

  • {?post_id} - key: post_id
  • post_id={id} - key: post_id
  • {?post_id,locale} - multiple keys supported

Key is auto-inferred from the URI template.

Changes

  • Add DataLoaderInterface (simple array-based contract)
  • Add DataLoader class for batch processing logic
  • Add RowMustContainKeyInDataLoaderException
  • Extend #[Link] annotation with dataLoader parameter
  • DataLoader binding included in ResourceClientModule by default

Test plan

  • Unit tests for DataLoader
  • Integration tests for DataLoader in Linker
  • Edge case tests (empty results, missing params, multiple parameters)
  • All existing tests pass
  • Static analysis (Psalm/PHPStan) pass

claude and others added 4 commits January 9, 2026 05:41
Add optional batch processing mechanism that reduces resource requests
during Crawl traversal.

- Add `batch` parameter to #[Link] annotation to specify BatchResolver
- Add BatchResolverInterface for implementing custom batch logic
- Add Requests class with helper methods (getQueryParam, mapResults)
- Add Results class for URI-keyed result mapping
- Add BatchResolverFactoryInterface for DI integration
- Modify Linker to process batch-enabled links efficiently

100% backward compatible - existing code works without changes.
Batch processing is opt-in via the new `batch` parameter.
Rename BatchResolver to DataLoader following GraphQL's DataLoader pattern:
- src/Batch/ → src/DataLoader/
- BatchResolverInterface → DataLoaderInterface
- BatchResolverFactory → DataLoaderFactory
- LikeBatchResolver → LikeDataLoader

The naming better reflects the pattern's purpose and origin.
- Add DataLoaderModule to src/Module for easy installation
- Add edge case tests for Requests class (empty URIs, missing keys)
- Add additional Linker tests (multiple invokes, empty comments)
- Add demo for DataLoader usage pattern
- Update Link annotation property to use dataLoader parameter
- Apply constructor property promotion in Requests class
- Use early exit pattern for better code readability
- Add psalm-suppress annotations for intentional mixed types
- Fix fully qualified class name reference in Types.php
- Format single-element arrays on single lines
@coderabbitai
Copy link

coderabbitai bot commented Jan 9, 2026

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

概要

DataLoader機能を導入し、Link注釈にdataLoaderパラメータを追加。Linkerに統合され、関連リソースのバッチ処理を実現し、N+1クエリ問題を解決。DataLoaderインターフェース実装でクエリをまとめて処理し、結果を分散。デモとテストコードも追加。

変更内容

コホート / ファイル 変更内容
DataLoaderコア実装
src/DataLoader/DataLoaderInterface.php, src/DataLoader/DataLoader.php
DataLoaderインターフェース定義と実装クラス追加。__invoke(array $queries): arrayメソッドでクエリ配列を受け取り、結果をグループ化して返却。テンプレートからキー抽出、クエリ解析、行グループ化のロジック実装
Link注釈拡張
src/Annotation/Link.php
dataLoaderプロパティを追加(class-string<DataLoaderInterface>|null型)。コンストラクタにdataLoaderパラメータ追加、値解決ロジック実装
例外定義
src/Exception/RowMustContainKeyInDataLoaderException.php
DataLoader処理時に必須キーが行に存在しない場合の例外クラス追加
Linker統合
src/Linker.php
DataLoader依存注入対応。annotationCrawlにDataLoader前処理ステップ追加、DataLoader有効リンクをスキップする保護ロジック実装
DI設定
src/Module/ResourceClientModule.php
ResourceClientModuleにDataLoader::classバインディング追加
型定義
src/Types.php
DataLoaderQuery、DataLoaderRow、DataLoaderQueries、DataLoaderRows、DataLoaderClass型定義追加
デモ実装
demo/4.link-crawl-dataloader.php
DataLoaderデモアプリケーション:LikeDataLoaderクラス、Article/Comment/Likeリソース、AppModuleの実装例。バッチ処理によるlike取得を可視化
DataLoaderテスト
tests/DataLoader/DataLoaderTest.php
DataLoaderユニットテスト:キー抽出、クエリ解析、グループ化、例外処理の検証
Linker統合テスト
tests/DataLoader/LinkerDataLoaderTest.php
Linker+DataLoader統合テスト:クロール実行時のバッチ処理検証、呼び出し回数確認、空データセット処理
テスト用DataLoader実装
tests/Fake/FakeVendor/Sandbox/DataLoader/LikeDataLoader.php
テスト用LikeDataLoader:呼び出しカウント追跡、comment_idごとのlike集約処理実装
テスト用リソース
tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Article.php, tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Comment.php, tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Like.php
バッチ処理デモ用リソースクラス:Article→Commentの関連リンク、Comment→LikeのDataLoaderリンク定義、静的データマッピング

シーケンス図

sequenceDiagram
    participant Linker
    participant DataLoader
    participant DI as DI Container
    participant LikeDataLoader
    participant DB as Data Store

    Linker->>Linker: annotationCrawl: collect Link annotations
    Linker->>Linker: filter: Link with dataLoader specified
    Linker->>DataLoader: load(annotations, link, bodyList)
    
    DataLoader->>DataLoader: extractKeysFromTemplate(href)
    DataLoader->>DataLoader: build queries from bodyList items
    DataLoader->>DI: getDataLoader(dataLoader class)
    DI->>LikeDataLoader: instantiate LikeDataLoader
    
    DataLoader->>LikeDataLoader: __invoke(queries)
    LikeDataLoader->>DB: bulk fetch likes by comment_id
    DB-->>LikeDataLoader: rows (aggregated)
    LikeDataLoader-->>DataLoader: return rows
    
    DataLoader->>DataLoader: buildGroupKey(row, extracted keys)
    DataLoader->>DataLoader: group results by keys
    DataLoader->>DataLoader: distribute grouped results to bodyList
    DataLoader-->>Linker: updated bodyList
    
    Linker->>Linker: continue crawl with populated relations
Loading

推定コードレビュー負荷

🎯 4 (Complex) | ⏱️ ~45 分

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'Add DataLoader support for Crawl to solve N+1 problem' clearly and specifically summarizes the main objective of the changeset: introducing a DataLoader pattern to address N+1 query problems in crawl operations.
Description check ✅ Passed The PR description comprehensively explains the N+1 problem, the DataLoader solution with clear examples, supported URI formats, implementation details, and test coverage, all directly related to the changeset.
Linked Issues check ✅ Passed The implementation successfully addresses issue #345 requirements: adds optional batch processing (DataLoaderInterface), enables opt-in via dataLoader parameter in #[Link] annotation, implements batch logic with key inference from URI templates, maintains backward compatibility, and includes comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes are directly in-scope: DataLoaderInterface, DataLoader implementation, Link annotation extension, ResourceClientModule binding, supporting types/exceptions, demo code, and comprehensive test coverage are all required to implement the DataLoader feature described in issue #345.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch claude/understand-crawl-pLOr9

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Jan 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (84852d9) to head (32c9fe2).
⚠️ Report is 20 commits behind head on 1.x.

Additional details and impacted files
@@             Coverage Diff             @@
##                 1.x      #346   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       541       575   +34     
===========================================
  Files             84        86    +2     
  Lines           1370      1451   +81     
===========================================
+ Hits            1370      1451   +81     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Extract DataLoader processing logic into separate DataLoaderProcessor class
- Linker now delegates to DataLoaderProcessor instead of handling DataLoader logic directly
- Reduces Linker's class complexity below PHPMD threshold of 50
- Add DataLoaderProcessorTest for 100% coverage of edge cases
@koriym koriym force-pushed the claude/understand-crawl-pLOr9 branch from 041f51d to ce8d000 Compare January 9, 2026 15:59
- Remove DataLoaderFactory and DataLoaderFactoryInterface
- DataLoaderProcessor now uses InjectorInterface directly
- Simplifies the architecture by eliminating unnecessary abstraction
- Remove Requests and Results classes
- Simplify DataLoaderInterface to __invoke(array $queries): array
- Auto-infer key from URI template (no dataLoaderKey needed)
- Support both URI formats: {?var} and param={var}
- Add RowMustContainKeyInDataLoaderException
Document that $queries is passed as associative arrays (not just values)
to support multiple key parameters in URI templates.
- Rename DataLoaderProcessor class to DataLoader
- Rename process() method to load()
- Add domain types to Types.php (DataLoaderQuery, DataLoaderRow, etc.)
- Update DataLoaderInterface to use imported types
- Remove separate DataLoaderModule
- DataLoader is now included by default in ResourceClientModule
- Update demo to use simplified module setup
@koriym
Copy link
Member Author

koriym commented Jan 9, 2026

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 9, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
tests/DataLoader/LinkerDataLoaderTest.php (1)

149-167: 空のコメントリストに対するエッジケースのテスト。

Line 166 で空のクエリリストでも DataLoader が1回呼び出されることを確認していますが、これは意図した動作でしょうか?空のクエリでは DataLoader を呼び出さない最適化も検討できます。

src/DataLoader/DataLoader.php (1)

154-172: 冗長な assert を削除できます。

Line 159 の assert(is_string($queryString)) は、Line 155 で既に is_string($queryString) をチェックしているため不要です。

♻️ 修正案
         if (! is_string($queryString) || ! str_contains($queryString, '=')) {
             return [];
         }

-        assert(is_string($queryString));
         parse_str($queryString, $params);
demo/4.link-crawl-dataloader.php (1)

33-71: LikeDataLoader の実装は DataLoaderInterface の使用法を適切に示しています。

Based on learnings, インターフェースは public function __invoke(array $queries): array を使用しており、この実装はその設計に従っています。

Line 65 の array_merge をループ内で使用している点は、デモとしては問題ありませんが、大量データでは ... スプレッド演算子の方がパフォーマンスが良いです。

♻️ パフォーマンス改善案(オプション)
             // 2. Bulk fetch: SELECT * FROM likes WHERE comment_id IN (...)
-            $rows = [];
-            foreach ($commentIds as $commentId) {
-                $likes = self::$data[(int) $commentId] ?? [];
-                $rows = array_merge($rows, $likes);
-            }
+            $rows = array_merge(
+                ...array_map(
+                    static fn ($id) => self::$data[(int) $id] ?? [],
+                    $commentIds,
+                ),
+            );
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84852d9 and 7d01a95.

📒 Files selected for processing (14)
  • demo/4.link-crawl-dataloader.php
  • src/Annotation/Link.php
  • src/DataLoader/DataLoader.php
  • src/DataLoader/DataLoaderInterface.php
  • src/Exception/RowMustContainKeyInDataLoaderException.php
  • src/Linker.php
  • src/Module/ResourceClientModule.php
  • src/Types.php
  • tests/DataLoader/DataLoaderTest.php
  • tests/DataLoader/LinkerDataLoaderTest.php
  • tests/Fake/FakeVendor/Sandbox/DataLoader/LikeDataLoader.php
  • tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Article.php
  • tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Comment.php
  • tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Like.php
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T18:36:49.781Z
Learning: The Link annotation parameter for DataLoader in BEAR.Resource is `dataLoader` (not `batch`). Example: `#[Link(crawl: 'tree', rel: 'like', href: 'app://self/like{?comment_id}', dataLoader: LikeDataLoader::class)]`
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T15:46:03.544Z
Learning: The DataLoader pattern in BEAR.Resource is inspired by Facebook's DataLoader and intentionally keeps the framework interface simple (Requests → Results). Advanced concerns like batching limits, error recovery strategies, and custom mapping logic are the DataLoader implementer's responsibility, not the framework's.
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T18:36:49.781Z
Learning: In BEAR.Resource's DataLoader implementation, the interface was finalized to use plain PHP arrays: `public function __invoke(array $queries): array` where both parameters and return values are `list<array<string, mixed>>`. This replaced the earlier Requests/Results object approach for maximum simplicity.
📚 Learning: 2026-01-09T15:46:03.544Z
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T15:46:03.544Z
Learning: DataLoaderFactory in BEAR.Resource validates DataLoader instances via assert($instance instanceof DataLoaderInterface).

Applied to files:

  • tests/DataLoader/DataLoaderTest.php
  • src/Linker.php
  • src/Types.php
  • tests/DataLoader/LinkerDataLoaderTest.php
  • src/DataLoader/DataLoader.php
  • tests/Fake/FakeVendor/Sandbox/DataLoader/LikeDataLoader.php
  • src/Module/ResourceClientModule.php
  • src/Exception/RowMustContainKeyInDataLoaderException.php
  • src/DataLoader/DataLoaderInterface.php
  • src/Annotation/Link.php
📚 Learning: 2026-01-09T18:36:49.781Z
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T18:36:49.781Z
Learning: The Link annotation parameter for DataLoader in BEAR.Resource is `dataLoader` (not `batch`). Example: `#[Link(crawl: 'tree', rel: 'like', href: 'app://self/like{?comment_id}', dataLoader: LikeDataLoader::class)]`

Applied to files:

  • tests/DataLoader/DataLoaderTest.php
  • src/Linker.php
  • src/Types.php
  • tests/DataLoader/LinkerDataLoaderTest.php
  • src/DataLoader/DataLoader.php
  • tests/Fake/FakeVendor/Sandbox/DataLoader/LikeDataLoader.php
  • demo/4.link-crawl-dataloader.php
  • src/Module/ResourceClientModule.php
  • tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Comment.php
  • src/Annotation/Link.php
📚 Learning: 2026-01-09T18:36:49.781Z
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T18:36:49.781Z
Learning: In BEAR.Resource's DataLoader implementation, the interface was finalized to use plain PHP arrays: `public function __invoke(array $queries): array` where both parameters and return values are `list<array<string, mixed>>`. This replaced the earlier Requests/Results object approach for maximum simplicity.

Applied to files:

  • tests/DataLoader/DataLoaderTest.php
  • src/Linker.php
  • src/Types.php
  • src/DataLoader/DataLoader.php
  • tests/Fake/FakeVendor/Sandbox/DataLoader/LikeDataLoader.php
  • demo/4.link-crawl-dataloader.php
  • src/Module/ResourceClientModule.php
  • src/DataLoader/DataLoaderInterface.php
  • src/Annotation/Link.php
📚 Learning: 2026-01-09T18:36:49.781Z
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T18:36:49.781Z
Learning: BEAR.Resource's DataLoader auto-infers key columns from URI templates in the Link annotation. Supported formats: `{?comment_id}` extracts key `comment_id`, and `comment_id={id}` also identifies `comment_id` as the key. This eliminates the need for manual key specification.

Applied to files:

  • tests/DataLoader/DataLoaderTest.php
  • src/Linker.php
📚 Learning: 2026-01-09T15:46:03.544Z
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T15:46:03.544Z
Learning: The DataLoader pattern in BEAR.Resource is inspired by Facebook's DataLoader and intentionally keeps the framework interface simple (Requests → Results). Advanced concerns like batching limits, error recovery strategies, and custom mapping logic are the DataLoader implementer's responsibility, not the framework's.

Applied to files:

  • src/Linker.php
  • src/Types.php
  • src/DataLoader/DataLoader.php
  • tests/Fake/FakeVendor/Sandbox/DataLoader/LikeDataLoader.php
  • demo/4.link-crawl-dataloader.php
  • src/Module/ResourceClientModule.php
  • src/DataLoader/DataLoaderInterface.php
  • src/Annotation/Link.php
📚 Learning: 2025-10-28T00:42:34.230Z
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 337
File: src/HttpResourceObject.php:18-24
Timestamp: 2025-10-28T00:42:34.230Z
Learning: In the BEAR.Resource codebase, type names like `Headers`, `HttpBody`, `Query`, `Body`, etc., are Psalm type aliases (domain types) defined in `src/Types.php` and imported via `psalm-import-type`. These are documentation-level types for static analysis that resolve to `array` at runtime. Method signatures correctly use `array` type hints while docblocks use the specific type aliases for Psalm validation.

Applied to files:

  • src/Types.php
🧬 Code graph analysis (8)
tests/DataLoader/DataLoaderTest.php (3)
src/DataLoader/DataLoader.php (2)
  • DataLoader (34-243)
  • load (54-67)
src/Exception/RowMustContainKeyInDataLoaderException.php (1)
  • RowMustContainKeyInDataLoaderException (12-18)
src/LinkType.php (1)
  • LinkType (7-18)
src/Linker.php (1)
src/DataLoader/DataLoader.php (2)
  • DataLoader (34-243)
  • load (54-67)
tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Like.php (2)
src/Resource.php (1)
  • Resource (23-178)
src/ResourceObject.php (1)
  • ResourceObject (38-287)
src/DataLoader/DataLoader.php (5)
src/Exception/RowMustContainKeyInDataLoaderException.php (1)
  • RowMustContainKeyInDataLoaderException (12-18)
src/LinkType.php (1)
  • LinkType (7-18)
src-files/uri_template.php (1)
  • uri_template (15-24)
src/Linker.php (2)
  • __construct (43-48)
  • crawl (206-233)
src/ResourceInterface.php (1)
  • uri (36-36)
tests/Fake/FakeVendor/Sandbox/DataLoader/LikeDataLoader.php (3)
src/DataLoader/DataLoader.php (1)
  • DataLoader (34-243)
demo/4.link-crawl-dataloader.php (3)
  • Like (118-128)
  • LikeDataLoader (33-71)
  • __invoke (51-70)
src/DataLoader/DataLoaderInterface.php (1)
  • __invoke (40-40)
src/Module/ResourceClientModule.php (1)
src/DataLoader/DataLoader.php (1)
  • DataLoader (34-243)
tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Article.php (3)
tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Comment.php (1)
  • Link (25-31)
src/Linker.php (1)
  • crawl (206-233)
tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Like.php (1)
  • onGet (30-35)
src/Annotation/Link.php (2)
src/DataLoader/DataLoader.php (1)
  • DataLoader (34-243)
src/Linker.php (2)
  • __construct (43-48)
  • crawl (206-233)
🔇 Additional comments (26)
src/DataLoader/DataLoaderInterface.php (1)

1-41: インターフェース設計が優れています

DataLoaderInterfaceの設計は明確でシンプルです。以下の点が特に良好です:

  • プレーンなPHP配列を使用し、フレームワークの複雑さを最小限に抑えている
  • 単一キーと複数キーの両方のユースケースについて、実用的な例を含む包括的なドキュメント
  • Psalm型アノテーションによる型安全性の確保

Based on learnings, この実装はFacebookのDataLoaderパターンに倣い、フレームワークインターフェースをシンプルに保つという意図に沿っています。

src/Module/ResourceClientModule.php (1)

9-9: DataLoaderのDIバインディングが適切に追加されています

DataLoader::classのバインディングにより、Linkerなどのコンポーネントでの注入が可能になります。後方互換性を保ちながら、新機能を有効にする適切な統合ポイントです。

Also applies to: 102-102

tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Like.php (1)

9-36: テストフィクスチャが適切に構成されています

このLikeリソースは、DataLoaderのテストに必要なデータ構造を提供しています:

  • comment_idでキー付けされた静的データは、バッチ処理のテストシナリオをサポート
  • デフォルトパラメータを持つonGetメソッドにより、柔軟なテストが可能
  • 詳細なPsalm型アノテーションにより型安全性を確保
src/Exception/RowMustContainKeyInDataLoaderException.php (1)

1-18: 例外クラスが明確で焦点が絞られています

RowMustContainKeyInDataLoaderExceptionの実装は適切です:

  • 検証失敗時に開発者にとって明確で実用的なエラーメッセージを提供
  • finalクラスとして定義され、フレームワークのExceptionInterfaceを実装
  • DataLoaderの契約違反を適切に表現
src/Linker.php (4)

46-46: DataLoaderの注入が後方互換性を維持しています

コンストラクタパラメータとしてDataLoader|null $dataLoader = nullを追加することで、以下を実現:

  • nullableでデフォルト値nullにより、既存のコードへの影響なし
  • 新しいDataLoader機能はオプトイン方式で利用可能
  • 適切な依存性注入による疎結合

175-177: DataLoaderの事前処理が正しく配置されています

annotationCrawlメソッド内で、DataLoader対応リンクを手動クロールループのに処理することで、N+1問題の解決を実現:

  • dataLoader?->load()により、バッチ処理可能なすべてのリンクを一度に処理
  • null安全演算子の使用により、DataLoaderが利用できない場合も安全に動作
  • 既存の手動クロールロジックは、非DataLoaderリンク用に保持

Based on learnings, この設計により、単一リクエストでバッチ処理し、データサイズに依存しないクエリ数を達成します。


213-216: スキップロジックが重複処理を適切に防止しています

crawlメソッドのガード条件により、DataLoaderで既に処理されたリンクの再処理を回避:

  • 両方の条件($annotation->dataLoader !== null AND $this->dataLoader !== null)をチェック
  • DataLoaderが利用可能で、かつアノテーションでDataLoaderが指定されている場合のみスキップ
  • 手動クロールとDataLoaderバッチ処理の明確な責任分離

189-189: Psalm抑制の追加について

InvalidArrayAccessの抑制が追加されていますが、これは妥当な対応です:

  • Line 174で$bodyList = $isList ? (array) $current->body : [$current->body];により、!$isListの場合は常に$bodyList[0]が存在
  • Psalmは配列が非空であることを静的に検証できないため、抑制が必要
  • 実行時の安全性はコードロジックにより保証されている
tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Article.php (1)

10-22: LGTM!テストリソースの実装が適切です

Articleリソースの実装は明確でシンプルです。Link annotationを使用してCommentリソースへのクロール関係を正しく定義しており、DataLoader統合のテストシナリオに適しています。

tests/Fake/FakeVendor/Sandbox/DataLoader/LikeDataLoader.php (1)

13-39: LGTM!DataLoaderの実装がインターフェースに適合しています

LikeDataLoaderの実装は正しく、テスト目的に適しています:

  • array_columnを使用してcomment_idを効率的に抽出
  • バルクフェッチのシミュレーションが適切
  • $callCountreset()メソッドによりテストでの呼び出し回数の検証が可能

Based on learnings, DataLoaderインターフェースの単純な配列ベースの契約(array $queriesarray)に正しく準拠しています。

tests/Fake/FakeVendor/Sandbox/Resource/App/Batch/Comment.php (1)

11-32: LGTM!DataLoaderを使用したLink annotationが正しく実装されています

Commentリソースは、DataLoader統合の優れた例を示しています:

  • Link annotationがdataLoader: LikeDataLoader::classパラメータを正しく指定
  • URIテンプレートapp://self/batch/like?comment_id={id}が適切
  • フォールバック処理(?? [])により、存在しないarticle_idに対しても安全

Based on learnings, dataLoaderパラメータの使用法が正しく、GraphQL DataLoaderパターンに準拠しています。

tests/DataLoader/DataLoaderTest.php (3)

24-74: LGTM!スキップロジックのテストカバレッジが包括的です

DataLoaderのフィルタリングロジックに対する3つのテストケースは、以下を適切に検証しています:

  • 非Link annotationのスキップ(Embedなど)
  • 異なるcrawlキーを持つLinkのスキップ
  • dataLoaderが指定されていないLinkのスキップ

これらのテストにより、DataLoaderが適切な条件でのみ実行されることが保証されます。


76-142: LGTM!キー抽出ロジックの徹底的なテスト

URIテンプレートからのキー抽出テストは、様々な形式を網羅しています:

  • equals形式:post_id={id}
  • クエリ展開形式:{?post_id}{?var1,var2}
  • 継続形式:{&page,limit}
  • 複数パラメータの組み合わせ
  • クエリ文字列なしのエッジケース

Based on learnings, これらのテストはDataLoaderがURIテンプレートからキーを自動推論する機能を正しく検証しています。


144-166: LGTM!例外処理とエッジケースのテストが適切です

以下のテストにより、エラーハンドリングが適切に検証されています:

  • 必須キーが欠落している行に対するRowMustContainKeyInDataLoaderExceptionの発生
  • 例外メッセージに欠落キー名が含まれることの確認
  • クエリ文字列なしのURIに対する空配列の返却

これらは、DataLoaderの堅牢性を保証する重要なテストケースです。

src/Types.php (1)

9-9: LGTM!DataLoader型定義が適切に追加されています

新しいDataLoader型は、既存のTypesファイルのパターンに従っており、型安全性を提供しています:

  • DataLoaderQueryDataLoaderRowは明確に定義
  • リスト型(DataLoaderQueriesDataLoaderRows)が適切
  • DataLoaderClass型により、DataLoaderインターフェースのクラス文字列を型安全に扱える

Based on learnings, これらの型定義は、DataLoaderの単純な配列ベースのインターフェース(array $queriesarray)と一致しています。

Also applies to: 127-132

src/Annotation/Link.php (2)

8-8: DataLoaderInterface のインポートと新規プロパティの追加は適切です。

新しい dataLoader プロパティは正しく型付けされており、オプショナルな DataLoader 統合を可能にします。Based on learnings, パラメータ名 dataLoader は正しく使用されています。

Also applies to: 50-55


76-94: コンストラクタの更新は後方互換性を維持しています。

dataLoader パラメータのデフォルト値 null により、既存のコードへの影響はありません。$resolvedDataLoader を使用した解決ロジックは、配列形式とパラメータ形式の両方の入力を正しく処理します。

tests/DataLoader/LinkerDataLoaderTest.php (3)

26-44: セットアップは適切に構成されています。

LikeDataLoader::reset() でテスト間の状態をクリアし、各コンポーネントが正しくインスタンス化されています。


46-85: DataLoader の N+1 問題解決を検証する良いテストです。

コメント3件に対して DataLoader が1回のみ呼び出されることを確認しています(Line 84)。構造、関連データ、および具体的な値の検証も網羅的です。


87-119: DataLoader 未設定時のフォールバック動作を検証しています。

null を明示的に渡すことで、DataLoader なしでも正常に動作することを確認。LikeDataLoader::$callCount が 0 のままであることで、フォールバック動作が正しく機能していることを証明しています。

src/DataLoader/DataLoader.php (3)

39-42: InjectorInterface を使用した遅延インスタンス化は適切です。

DataLoader インスタンスをキャッシュすることで、同一の DataLoader クラスに対する重複インスタンス化を回避しています。


198-226: buildGroupKey と buildGroupKeyFromQuery の挙動の違いについて確認してください。

buildGroupKey はキーが存在しない場合に RowMustContainKeyInDataLoaderException をスローしますが、buildGroupKeyFromQuery は空文字列を使用します。この非対称性は意図的な設計でしょうか?

  • DataLoader から返される行(buildGroupKey): キーが欠落していればエラー
  • クエリパラメータ(buildGroupKeyFromQuery): キーが欠落していれば空文字列

クエリ側でもキーが必須であれば、一貫性のためにエラーをスローすることを検討してください。


233-242: DataLoader インスタンスのキャッシュと検証は適切です。

assert($instance instanceof DataLoaderInterface) による型検証は、Based on learnings で言及されている設計方針に沿っています。

demo/4.link-crawl-dataloader.php (3)

5-17: DataLoader の利点を明確に説明する優れたドキュメントです。

N+1 問題の解決方法(3つの個別リクエスト → 1つのバッチリクエスト)が分かりやすく説明されています。


80-128: リソースクラスの実装は Link アノテーションの使用法を明確に示しています。

Comment クラス(Line 109)で dataLoader: LikeDataLoader::class を指定することで、N+1 問題を解決するバッチ処理が有効になります。Like::onGet は DataLoader が利用できない場合のフォールバックとして適切に実装されています。


146-160: デモのエントリーポイントは簡潔で分かりやすいです。

linkCrawl('tree') を使用したクロール操作により、DataLoader を通じて関連リソースがバッチ取得されます。

@koriym
Copy link
Member Author

koriym commented Jan 9, 2026

@coderabbitai Removed redundant assert.

Regarding empty query optimization: calling DataLoader with empty queries is intentional - it simplifies the code flow and DataLoader implementations can handle empty arrays gracefully.

Regarding buildGroupKey asymmetry: DataLoader rows must contain the key (data integrity), while query params may be empty (valid for optional parameters).

@coderabbitai
Copy link

coderabbitai bot commented Jan 9, 2026

Tip

For best results, initiate chat on the files or code changes.

@koriym Thank you for the clarifications! These design decisions make perfect sense:

Empty query handling: Calling DataLoader with empty arrays for consistency is a solid choice—it keeps the code path uniform and places the responsibility on DataLoader implementations to handle edge cases, which aligns well with the simple interface design.

buildGroupKey asymmetry: The distinction is clear and well-reasoned: DataLoader results represent fetched data that must be keyed correctly for distribution, while query parameters can legitimately be empty when dealing with optional parameters in URI templates.

I appreciate you taking the time to explain the rationale behind these implementation choices—they demonstrate thoughtful API design that balances simplicity with correctness.


✏️ Learnings added
Learnt from: koriym
URL: https://github.com/bearsunday/BEAR.Resource/pull/346

Timestamp: 2026-01-09T19:38:54.982Z
Learning: In BEAR.Resource's DataLoader implementation, calling DataLoader with empty queries is intentional to simplify code flow—DataLoader implementations are expected to handle empty arrays gracefully.

Learnt from: koriym
URL: https://github.com/bearsunday/BEAR.Resource/pull/346

Timestamp: 2026-01-09T19:38:54.982Z
Learning: In BEAR.Resource's DataLoader buildGroupKey implementation, there is intentional asymmetry: DataLoader rows must contain the key column (data integrity requirement enforced by RowMustContainKeyInDataLoaderException), while query parameters may be empty (valid for optional parameters in URI templates).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T18:36:49.781Z
Learning: The Link annotation parameter for DataLoader in BEAR.Resource is `dataLoader` (not `batch`). Example: `#[Link(crawl: 'tree', rel: 'like', href: 'app://self/like{?comment_id}', dataLoader: LikeDataLoader::class)]`

Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T15:46:03.544Z
Learning: The DataLoader pattern in BEAR.Resource is inspired by Facebook's DataLoader and intentionally keeps the framework interface simple (Requests → Results). Advanced concerns like batching limits, error recovery strategies, and custom mapping logic are the DataLoader implementer's responsibility, not the framework's.

Learnt from: koriym
Repo: bearsunday/BEAR.Resource PR: 0
File: :0-0
Timestamp: 2026-01-09T18:36:49.781Z
Learning: In BEAR.Resource's DataLoader implementation, the interface was finalized to use plain PHP arrays: `public function __invoke(array $queries): array` where both parameters and return values are `list<array<string, mixed>>`. This replaced the earlier Requests/Results object approach for maximum simplicity.

@koriym koriym merged commit 32c9fe2 into 1.x Jan 15, 2026
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: Batch processing for Crawl to solve N+1 problem

5 participants