Skip to content

Conversation

@nobuhiko
Copy link
Contributor

概要

商品詳細ページで発生していたN+1クエリ問題を解決しました。

問題

  • findWithSortedClassCategories()がProductStockとTaxRuleをEager Loadingしていなかった
  • Product::_calc()が各ProductClassに対してlazy loadをトリガーし、N+1クエリが発生
  • 352個のProductClassを持つ商品で987クエリ(2300ms)を記録

解決策

  • ProductStockとTaxRuleのleftJoinによるEager Loadingを追加
  • eccube_result_cache_lifetime_shortによる結果キャッシュを有効化
  • 商品一覧ページで既に使用されている最適化パターンを適用

期待される効果

  • クエリ削減: 987クエリ → 3クエリ (99.7%削減)
  • 初回表示: 約60-70%改善
  • 2回目以降: キャッシュヒットでさらに高速化

変更内容

1. ProductRepository::findWithSortedClassCategories()

  • ProductStockとTaxRuleをEager Loadingに追加
  • 結果キャッシュの設定を追加

2. ProductRepositoryTest::testFindWithSortedClassCategoriesWithManyProductClasses()

  • 100個のProductClassを持つ商品でN+1問題が解決されているかを検証
  • Doctrineクエリロガーで_calc()実行前後のクエリ数をカウント
  • 追加クエリが実行されないことをアサート

セキュリティレビュー結果

セキュリティチェック: 合格

  • SQLインジェクション: 問題なし(パラメータバインディング使用)
  • XSS: 問題なし(データ取得のみ)
  • 認証・認可: 問題なし(既存の仕組みを維持)
  • 情報漏洩: 問題なし(公開情報のみ)
  • キャッシュセキュリティ: 問題なし(10秒短期キャッシュ)
  • DoS対策: 改善(リソース消費削減)

詳細なセキュリティレポートはコメントを参照してください。

テスト

bin/phpunit tests/Eccube/Tests/Repository/ProductRepositoryTest.php::testFindWithSortedClassCategoriesWithManyProductClasses

参考

EC-CUBE 4.3既存実装(findProductsWithSortedClassCategories)のパターンを商品詳細にも適用


🤖 Generated with Claude Code

## Problem
- findWithSortedClassCategories() for product detail page was not eagerly loading ProductStock and TaxRule
- Product::_calc() triggered lazy loading for each ProductClass, causing N+1 queries
- For products with 352 ProductClasses, this resulted in 987 queries (2300ms)

## Solution
- Added eager loading of ProductStock and TaxRule via leftJoin
- Enabled query result cache with eccube_result_cache_lifetime_short
- Applied the same optimization pattern already used in findProductsWithSortedClassCategories() for product list page

## Expected Impact (based on helmet.jp optimization memo)
- Query reduction: 987 queries → 3 queries (99.7% reduction)
- Initial page load: ~60-70% improvement
- Second access: Further improvement with cache hit

## Changes
1. ProductRepository::findWithSortedClassCategories()
   - Added ProductStock and TaxRule to eager loading
   - Added result cache configuration

2. ProductRepositoryTest::testFindWithSortedClassCategoriesWithManyProductClasses()
   - Added test with 100 ProductClasses to verify N+1 problem is solved
   - Uses Doctrine query logger to count queries before/after _calc()
   - Asserts no additional queries are executed (N+1 solved)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@dotani1111
Copy link
Contributor

@nobuhiko
PRありがとうございます!
確認を進めたいと思います!

テストが落ちているため、修正のほどお願いいたします。

@dotani1111 dotani1111 added the improvement 機能改善 label Jan 7, 2026
@dotani1111 dotani1111 added this to the 4.4.0 milestone Jan 7, 2026
nobuhiko and others added 2 commits January 7, 2026 16:50
## Problem
The test `testFindWithSortedClassCategoriesWithManyProductClasses` was failing on MySQL and SQLite3 (but passing on PostgreSQL) because:
- Product::_calc() calls getClassName()->getName() at lines 103 and 106
- ClassName was not eagerly loaded, causing lazy loading queries
- This resulted in 2-3 additional queries being detected by the test

## Solution
Added ClassName eager loading to both methods:
- findWithSortedClassCategories() - for product detail page
- findProductsWithSortedClassCategories() - for product list page

Added leftJoin for cc1.ClassName and cc2.ClassName to prevent lazy loading when Product::_calc() accesses ClassName.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Rector requires strict type comparison in tests.
Changed assertEquals to assertSame for string and integer comparisons.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.71%. Comparing base (fba6352) to head (3ce8335).
⚠️ Report is 15 commits behind head on 4.3.

Additional details and impacted files
@@           Coverage Diff           @@
##              4.3    #6547   +/-   ##
=======================================
  Coverage   82.70%   82.71%           
=======================================
  Files         480      480           
  Lines       26507    26517   +10     
=======================================
+ Hits        21923    21933   +10     
  Misses       4584     4584           
Flag Coverage Δ
E2E 82.71% <100.00%> (+<0.01%) ⬆️
Unit 82.71% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nanasess
Copy link
Contributor

nanasess commented Jan 8, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Review Summary:

  • CLAUDE.md compliance: Verified against coding standards (PHP-CS-Fixer, PHPStan, license headers, directory structure)
  • Bug scan: No obvious bugs found in the eager loading implementation
  • Historical context: Changes align with established patterns (ProductStock/TaxRule eager loading since 2018)
  • Past PR comments: No conflicting guidance from previous reviews (PR 受注登録画面の商品検索で発生することがあるエラーを修正 #4695 discussion does not apply here due to ManyToOne relation)
  • Code comments compliance: All changes properly documented with inline comments explaining the purpose

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement 機能改善

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants