fix(csv): 一覧をソートしてからCSVをダウンロードするとエラーになるのを修正 (#6713) - #7070
fix(csv): 一覧をソートしてからCSVをダウンロードするとエラーになるのを修正 (#6713)#7070ttokoro20240902 wants to merge 5 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 @@
## 4.4 #7070 +/- ##
==========================================
+ Coverage 77.67% 77.76% +0.08%
==========================================
Files 597 597
Lines 29333 29345 +12
==========================================
+ Hits 22785 22819 +34
+ Misses 6548 6526 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
受注検索・商品検索のクエリは to-many の関連を join しているため、その関連側の列で
ソートした状態で CSV を出力すると `LimitSubqueryWalker` が例外を投げ、CSV が
最後まで出力されなかった。
対象は次の 3 経路・5 キー。
- 受注CSV / 配送CSV … 出荷状況 (`s.shipping_date`) / お問い合わせ番号
(`s.tracking_number`) / お届け先 (`s.name01`)
- 商品CSV … 商品コード (`pc.code`) / 在庫数 (`pc.stock`)
受注一覧・商品一覧はどちらも `wrap-queries` を有効にして既に回避済みだったので、
CSV 出力でも同じ判定を使うようにした。一覧の判定ロジックは各コントローラの
`createPaginateOptions()` に切り出して一覧と CSV で共有する。
`order_status` (`o.OrderStatus`) と `status` (`p.Status`) は association を
ソート対象にするため、一覧と同様に対象外のままとする。
商品CSVは `wrap-queries` だけでは足りず、PostgreSQL が
`for SELECT DISTINCT, ORDER BY expressions must appear in select list` で
拒否する。`ProductClass` の列でソートしている場合は `select('p, pc')` に切り替える
(在庫切れ検索で既に使われている形)。出力行数は 469 行・重複 0 で変化しないことを
実測で確認した。
`CsvExportService::exportData()` は会員・カテゴリ・規格分類・規格名の CSV 出力でも
共有しているため、オプションは引数で受け取る形にした。既定値は `[]` で、これら 4 経路の
挙動は変わらない。
Refs #6713
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3090d5b to
aefee64
Compare
ソート対象が ProductClass の列のときに pc を fetch join していたため,
`pc.visible = true` の条件で ProductClasses が部分初期化され, 非表示の規格
(規格を登録すると非表示になる 規格なし既定の ProductClass) の行が CSV から
落ちていた。全カタログで 1149 行 → 701 行, 448 商品で各 1 行の差になる。
fetch join をやめ, ソート対象の列を HIDDEN で select 句に載せる形にした。
PostgreSQL の「ORDER BY expressions must appear in select list」は満たしつつ,
ProductClasses は遅延ロードのままなので出力行数が変わらない。5 ソートキー
すべてで 1149 行に揃うこと, 並び順が一覧画面と一致することを実測で確認した。
在庫切れ絞り込みの `select('p, pc')` は従来どおりとする。この絞り込みでは
ソートの有無で select が変わらないため, 内部で不整合は生じない。
あわせて次を整理した。
- ソートの有無で CSV の行数が変わらないことのテストを受注CSV・配送CSV・商品CSV に
追加した。項目の値に改行が含まれるため, 行数は fgetcsv でレコード単位に数える
- 到達不能な条件を削除した (`$sortKey === 'code'`。COLUMNS に 'code' キーは無く,
手前の empty() で必ず打ち切られる)
- ソートキーの正規化を extractSortKey() に寄せ, createPaginateOptions() は
string を受ける形に統一した。受注側と商品側で呼び出し方が揃う
Refs #6713
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PHP 8.4 で `fgetcsv(): the $escape parameter must be provided as its default
value will change` の deprecation が出て, failOnDeprecation により 8.4 / 8.5 の
PHPUnit が全ジョブ赤になっていた。
escape を明示するだけでなく, 出力側の設定に合わせて読むようにした。
- エンコーディング: 出力は eccube_csv_export_encoding (既定 SJIS-win) なので
UTF-8 に戻してから読む。SJIS は 2 バイト目に 0x5C を持つ文字 (表・ソ 等) があり,
生バイト列を escape '\' で読むと閉じ引用符をエスケープして行が結合される (実測)
- 区切り文字: eccube_csv_export_separator を使う
- escape: PHP 8.4 以降の既定値に合わせて '' を明示する。'\' はデータ中の
バックスラッシュで同じ行結合を起こす (実測)
あわせて次を整理した。
- 行数を preg_split('/\R/') で数えていた既存アサートを, レコード単位で数える形に
統一した。商品CSVは説明文に改行を含むため, 改行の数は行数にならない
- ソートが商品CSVの並び順に反映されることのテストを追加した。商品コードの昇順が
商品IDの昇順と一致しないよう逆順に登録し, 昇順・降順の両方で並びを検証する
Refs #6713
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
レビュー指摘 4 件のうち 3 件を反映した。
- CSV をレコード単位で読む処理を AbstractAdminWebTestCase::parseCsv() /
countCsvRows() に移動した。受注側と商品側のテストに docblock 込みで同一の
約 35 行が重複しており, 直前の $escape 漏れがまさに「同じ処理を 2 箇所に書いて
片方だけ直す」形で起きていた
- 並び順テストの期待値から規格ごとの連番を落とし, 商品間の並びだけを比較するよう
にした。商品内の行順は Product::$ProductClasses の取得順に依存し,
#[ORM\OrderBy] が無いため DB 任せになっている
- createPaginateOptions() の docblock を修正した。受注検索は select('o, s') で
Shipping のみ fetch join し, OrderItem は leftJoin だけで select に載らない
- stock_status は SearchProductType に無くコアからは設定されないため,
select('p, pc') の分岐がプラグイン等からのみ到達する旨をコメントした
extractSortKey() の重複解消 (AbstractController への移動) は見送った。
プラグインを含む全コントローラの基底に管理画面向けの 5 行を載せると公開 API が
広がるうえ, sortkey を扱うのは受注・商品の 2 箇所だけで, 隣の
createPaginateOptions() も各リポジトリの COLUMNS を見るため意図的に重複させている。
回帰テストは 22 ケースとも pass。base 4.4 では 12 件失敗する。
Refs #6713
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ソート未指定時の既定は p.update_date DESC, p.id DESC。商品を c, b, a の順に 登録していたため既定の並びが a, b, c となり, 昇順の期待値と一致していた。 sortkey を空にして実行すると昇順アサートは通り降順アサートだけが落ちる状態で, 昇順側が独立したゲートになっていなかった。 登録順を b, c, a にすると既定の並びは a, c, b になり, 昇順 (a, b, c) とも 降順 (c, b, a) とも一致しない。ソートを無効化すると昇順アサートが落ちること, 降順ブロックを昇順にすり替えると降順アサートが落ちることを実測で確認した。 期待値は変更していない。 Refs #6713 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
概要(Overview・Refs Issue)
Refs #6713
一覧をソートしてから CSV をダウンロードするとエラーになる不具合を修正します。
Issue は受注一覧の報告ですが、調査したところ同じ原因で 3 経路・5 キーが壊れていました。
s.shipping_date) / お問い合わせ番号 (s.tracking_number) / お届け先 (s.name01)pc.code) / 在庫数 (pc.stock)いずれも検索クエリが to-many の関連を join しているため、その関連側の列でソートした状態で
ページングすると
LimitSubqueryWalkerが例外を投げます。ブラウザで確認した結果、HTTP 500 になりダウンロードは発生しません(
APP_ENV=devで実測)。なお Issue には「ヘッダ行だけ書き出された壊れた CSV がダウンロードされたうえでエラー」と
報告されています。
exportHeader()がphp://outputにヘッダ行を書いた後でexportData()が例外を投げる構造なので、出力バッファの状態次第で部分的な CSV がクライアントへ届くことは起こり得ます(こちらは未実測です)。
いずれにしても CSV は最後まで出力されません。
s.shipping_dates.tracking_numbers.name01pc.codepc.stocko.*p.*受注CSVと配送CSVは同じクエリビルダ(
CsvExportService::getOrderQueryBuilder())を使うため、配送CSVも同じ不具合を抱えていました。 Issue には記載がありませんが、
追加したテストで両方の再現を確認しています。
方針(Policy)
一覧画面では既に
wrap-queriesで回避済みだったので、CSV 出力でも同じ判定を使います。OrderController::index()は以前から次のように書かれていました。この判定を
createPaginateOptions()に切り出し、一覧・受注CSV・配送CSV で共有します。商品側も同じ形にしました。新しい方針を持ち込まず、一覧の既存挙動に CSV を合わせる
形にしています。
order_status/statusを対象外にしている理由OrderRepository::COLUMNSのorder_statusは'o.OrderStatus'、ProductRepository::COLUMNSのstatusは'p.Status'で、いずれもスカラー列ではなくassociation です。一覧側が以前から明示的に除外しているので、CSV 側でも除外を維持します。
無条件に
wrap-queriesを有効にすると、現在正常に出力できているソートの挙動を変えてしまいます。商品CSVは
wrap-queriesだけでは足りない商品CSVは
wrap-queriesを有効にするとLimitSubqueryWalkerは解消しますが、今度は PostgreSQL が別のエラーで拒否します。
商品CSVの出力パスは
resetDQLPart('select')の後にselect('p')->distinct()でselect を絞るため、
ORDER BY pc.codeが select 句に無いことが原因です。そこで
ProductClassの列でソートしている場合だけ、その列をHIDDENで select 句に載せるようにしました。
HIDDENなので取得結果には含まれず、ORDER BYの対象が select 句にある状態だけを作れます。形ごとに実測した結果です(PostgreSQL・1 ケース 1 トランザクションで隔離。
商品 469 件 / 規格 1149 件、うち非表示 448 件)。
pc.code/pc.stockでソートpLimitSubqueryWalkerの例外p42P10DISTINCT/ORDER BY の例外p, pcp+HIDDENselect('p, pc')を採らなかった理由は、出力行数が変わってしまうからです。pcを fetch join すると、検索クエリが持つpc.visible = trueの条件でProductClassesコレクションが部分初期化され、
getProductClasses()が表示中の規格しか返さなくなります。商品CSVは規格ごとに 1 行を出力するため、規格を登録すると非表示になる 規格なし既定の
ProductClass(class_category_id1 IS NULL)の行が落ちます。上記データでは 448 商品で各 1 行、1149 行 → 701 行の差になりました。
ソートリンクを押したかどうかで CSV の内容が変わってしまうため採用していません。
HIDDEN方式ではProductClassesは遅延ロードのままなので、この問題が起きません。ソートキー 5 種すべて(未指定 /
product_code/stock/name/status)で1149 行に一致することを実測しました。
在庫切れ絞り込み(
$isOutOfStock)のselect('p, pc')は従来どおりです。この絞り込みではソートの有無で select が変わらないため、内部で不整合は生じません。
CsvExportService::exportData()を引数で受け取る形にした理由exportData()は 受注・商品・会員・カテゴリ・規格分類・規格名 の CSV 出力で共有されています(計 6 経路)。ここで無条件に
wrap-queriesを有効にすると、無関係な経路の挙動まで変わってしまいます。
そのため呼び出し側からオプションを渡す形にし、既定値を
[]にしました。会員・カテゴリ・規格分類・規格名の 4 経路は引数を渡していないため、挙動は変わりません。
実装に関する補足(Appendix)
sortkeyをセッション(eccube.admin.order.search/eccube.admin.product.search)から読んでいます。sortkeyはSearchOrderType/SearchProductTypeでHiddenTypeとして定義されているため、セッションの view data とsubmit 後の値は同じ文字列になります。
extractSortKey()に寄せました(文字列以外は未指定として扱う。
nullをそのまま配列オフセットに使うのは PHP 8.5 で非推奨)。createPaginateOptions()はstringを受け取るだけで、一覧・CSV のどちらからも同じ形で呼びます。$sortKey === 'code'は削除しました。ProductRepository::COLUMNSに'code'キーは無く(商品コードは'product_code')、手前のempty()で必ず打ち切られる到達不能な条件でした。判定結果は変わりません。
テスト(Test)
受注CSV・配送CSV・商品CSVそれぞれに回帰テストを追加しました(計 21 ケース)。
OrderControllerTest::testExportOrderWithSortKeyOrderControllerTest::testExportShippingWithSortKeyProductControllerTest::testExportProductWithSortKeyOrderControllerTest::testExportOrderRowCountIsNotAffectedBySortOrderControllerTest::testExportShippingRowCountIsNotAffectedBySortProductControllerTest::testExportProductRowCountIsNotAffectedBySort4.4): 21 件のうち 11 件が失敗(受注/配送 各 3 キー = 6 件、商品 2 キー = 2 件、行数テスト 3 件)order_status/status/purchase_price/product_id/name/ ソート未指定のケースは修正前後どちらもパスするため、除外条件が効いていること・従来動いていたキーが
壊れていないことを同時に担保します。
行数テストは、上記の
select('p, pc')で起きる行の欠落を検知するために入れています。行数は改行の数では数えられません(商品CSVは説明文に改行を含むため、
preg_split('/\R/')では 1149 行が 1594 行に膨らみます)。
fgetcsvでレコード単位に数えています。管理画面での動作確認(Playwright / Chrome)
実ブラウザで、一覧のソートリンクをクリックしてから CSV をダウンロードする経路を確認しました。
機能テストは
sortkeyを POST で直接渡すため、JS 駆動のソート UI(
js-listSortのクリックでjs-listSort-keyに値が入る)を通っていないので、別途確認しています。LimitSubqueryWalkerの uncaught 例外・ダウンロードなし)ブラウザ確認は
select('p, pc')方式のときに実施したものです。HIDDEN方式に変更した後は、出力行数(5 ソートキーすべて 1149 行)と並び順を実測で再確認しています。
ソートが CSV まで維持されていることも確認しました。 「お届け先」でソートした状態の
一覧の表示順(受注 25 件)と、ダウンロードした CSV の受注の並びが完全に一致しました。
order_status(wrap-queries 対象外)でも同様に一致します。商品CSVも、一覧と同じクエリで 4 ページ分(100 件)取得した商品ID列と、CSV 側クエリの
先頭 100 件が完全に一致しました。エラーが消えただけでなく、並び順が正しく保たれています。
ブラウザのコンソールエラーは 0 件です。
受注一覧のソートリンクは 8 種(
orderer/order/payment_method/order_status/purchase_price/shipping_status/tracking_number/delivery)で、テストはこのうち 5 種と「ソート未指定」を網羅しています。
未カバーの
orderer/order/payment_methodはいずれもo.*のスカラー列で、カバー済みの
purchase_priceと同じ扱いになります。商品一覧のソートリンクは 7 種で、テストは
product_code/stock/status/product_id/nameと「ソート未指定」を網羅しています。未カバーの
create_date/update_dateはp.*のスカラー列です。自動テストの実行結果(Docker / PHP 8.2.31 / PostgreSQL):
OrderControllerTest… 32 tests, 140 assertions すべてパスProductControllerTest… 57 tests。1 件失敗するが本 PR とは無関係(
testProductSearchAllの件数アサート。ローカル DB にテストが残した商品が蓄積して480 件あり、フィクスチャの期待値 13 件と合わない。修正前のコードでも同じ結果)
CsvExportServiceTest/PaginationTest/CustomerControllerTest… 26 tests パスsrcの 3 ファイル)… エラーなし相談(Discussion)
Issue では「CSV 出力時はソートを使わない方針にする」案も出ていましたが、
本 PR はソートを維持したまま解消する方針で作っています。
報告者の @ishida20284 からは、既存のソート機能を維持できることと性能面の劣化が
無いことを確認したうえで同意をいただいています。
@dotani1111 当初「出力時にページのソートを利用しない方針にしようか」とご提案いただいた
点について、この方針で問題ないかご判断をお願いします。
性能は受注 5,011 件 / 明細 30,051 件(27.5 MB)で A/B を交互に 6 往復して計測し、
中央値 42.9 秒 → 45.2 秒でレンジが重複しており、有意差は確認できませんでした。
商品CSVを本 PR に含めるかについてもご意見をください。 Issue は受注一覧の報告ですが、
原因が同一で修正パターンも共通なので、まとめて直すほうが取りこぼしが無いと判断しました。
分けたほうがよければ商品CSV分を別 PR / 別 Issue に切り出します。
マイナーバージョン互換性保持のための制限事項チェックリスト