経路検索関係のバグを修正#1323
Conversation
Walkthroughline_group_cd を NULL から実値（COALESCE）に切り替える変更が SQL とリポジトリに反映され、結果列の並びが更新された。さらに、get_routes の実装がルート単位で逐次処理し都度 tt_lines を取得する形に改められ、停車駅生成とルート絞り込みの流れが組み直された。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UC as UseCase:get_routes
participant Repo as StationRepository
participant TT as TTLinesProvider
UC->>Repo: route_row_tree_map を取得
loop 各 RouteEntry
UC->>UC: route ごとに line_group_id_vec を算出
UC->>TT: 該当 group の tt_lines を取得
loop 各 Row
UC->>UC: tt_line を照合し line_symbols を更新
UC->>UC: TrainType を構築（nullable は既定値補完）
UC->>UC: build_station_from_row で Station を生成
end
UC->>UC: from/to を含むか判定（含まなければ破棄）
UC->>UC: Route を集約して追加
end
UC-->>UC: ルート一覧を返却
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
stationapi/src/use_case/interactor/query.rs (1)
726-732: TODOコメントへの対応SQLで同等の処理を行うというTODOコメントがあります。パフォーマンス向上のため、このフィルタリングをSQL側で実装することを検討してください。
このTODOの実装をお手伝いしましょうか？新しいIssueを作成して、SQL側でのフィルタリング実装を追跡できます。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.sqlx/query-323d5e8a03e7c905970e1787748510402a5d36c1166f2d21cf14a100779048d6.json(3 hunks).sqlx/query-422bdec51028a86db49d9e2b0babd7f58066d3a2433f5b6416972dfc359540fa.json(3 hunks)stationapi/src/infrastructure/station_repository.rs(2 hunks)stationapi/src/use_case/interactor/query.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
stationapi/src/use_case/interactor/query.rs (3)
stationapi/src/domain/entity/train_type.rs (1)
new(26-58)stationapi/src/domain/entity/station.rs (1)
new(80-212)stationapi/src/domain/entity/line.rs (1)
new(44-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy
- GitHub Check: Test Suite
- GitHub Check: Check
- GitHub Check: Test Suite
🔇 Additional comments (8)
stationapi/src/infrastructure/station_repository.rs (2)
1009-1009:line_group_cdの実装が良好です
from_sst.line_group_cdから実際の値を取得するよう変更され、has_train_typesの判定が正確になりました。素晴らしい修正です。
1279-1279: 同様のline_group_cd実装です
get_route_stopsメソッドでも同じくsst.line_group_cdから値を取得しています。一貫性のある実装で良いですね。.sqlx/query-422bdec51028a86db49d9e2b0babd7f58066d3a2433f5b6416972dfc359540fa.json (2)
3-3: クエリ内のコメントが良いです
-- has_train_types用というコメントが追加され、カラムの用途が明確になりました。保守性が向上しています。
233-249: カラム順序の一貫性を確認この変更は
query-323d5e8a...jsonと同じカラム順序の変更パターンです。両ファイルで一貫した変更が行われています。.sqlx/query-323d5e8a03e7c905970e1787748510402a5d36c1166f2d21cf14a100779048d6.json (2)
3-3: 長大クエリ（107行 / 134行）につき、実行計画で性能検証をああ、此の長さは保守と性能の試金石なり。現状（実測）: .sqlx/query-323… = 107行、query-422… = 134行。CTE多用で可読性と実行時挙動（マテリアライズ）の懸念あり。以下を実施して結果を提示せよ。
- 代表パラメータで EXPLAIN (ANALYZE, BUFFERS) を取得し、プランと実行時間を共有すること。
- 使用中の PostgreSQL バージョンを提示すること（CTEのインライン化／マテリアライズ挙動確認用）。
- クエリ内の LEFT JOIN from_stations AS fs ON fs.station_cd IS NOT NULL の意図を明示すること（結合条件が実体参照を含んでいるか確認）。
- 必要なら CTE 削減・クエリ分割・一時テーブル化・インデックス見直しの案を出すこと。
233-249: 問題なし — カラムは名前でマッピングされていますStationRowは#[derive(sqlx::FromRow)]でフィールド名をキーにマッピングされ、sqlx::query_as!／query_as::<_, StationRow>の使用を確認したため、カラム順序の入れ替えはstation_repository.rsに影響しません。位置依存のrow.get(index)は該当ファイルに見当たらず（data_validator/src/main.rs にインデックス参照あり）。列は移ろえど、名にて結ばるれば安心なり。
stationapi/src/use_case/interactor/query.rs (2)
541-543: DBクエリ増加の憂い — 計測／バッチ化の確認をああ、ルート毎の処理でクエリが膨らむ懸念あり。stationapi/src/use_case/interactor/query.rs の for ループ内で get_by_line_group_id_vec_for_routes(&line_group_id_vec).await の呼び出しを確認しました。インフラ実装は渡したID配列ごとに単一クエリを発行するため、ルート数に比例してDBクエリ回数が増えます。負荷測定を行い、必要なら全ルートをまとめて一回の照会にするか、キャッシュ導入などでクエリ削減を検討すること。
571-576: unwrap_or_default()でNoneが空文字になる挙動、意図はいかにstationapi/src/use_case/interactor/query.rs（571–576）にて、StationRowのtype_name/type_name_k/colorはOptionであるのに対し、TrainTypeはStringを要求するためunwrap_or_default()でNone→""にしています。空文字で良いのか、あるいはNoneを保持する（TrainTypeをOption化する／TrainType生成をスキップする等）べきか指示を願います、いかが致しましょう。
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stationapi/src/infrastructure/station_repository.rs (1)
472-479: パラメータ二重束縛の恐れあり
ORDER BY CASE … WHEN $i THEN …で同じ位置パラメータを再利用しているにも関わらず、同じ値を二度bindしておられる。実行時に「過剰なバインド」となる懸念あり。後段のforは除くべし。- for id in ids { - query = query.bind(*id as i32); - }
🧹 Nitpick comments (9)
stationapi/src/infrastructure/station_repository.rs (3)
1009-1010: COALESCE(…, NULL) は蛇足に候
COALESCE(from_sst.line_group_cd, NULL)は実質from_sst.line_group_cdと等しく、読みも冗漫。型注釈のための::intはそのままに、式を簡潔に致したく候。- COALESCE(from_sst.line_group_cd, NULL)::int AS line_group_cd, -- has_train_types用 + from_sst.line_group_cd::int AS line_group_cd, -- has_train_types用
1279-1291: ここも同断：冗語を削り、軽やかに同趣旨につき、
COALESCE(sst.line_group_cd, NULL)も削減されるがよろしかろう。- COALESCE(sst.line_group_cd, NULL)::int AS line_group_cd, -- has_train_types用 + sst.line_group_cd::int AS line_group_cd, -- has_train_types用
1296-1299: 未使用の結合は退け、行間を軽く当該 SELECT では
tt.*を参照せず。LEFT JOIN types AS ttは徒に計算量を増すのみ。落とすを宜しと存ず。- LEFT JOIN types AS tt ON tt.type_cd = sst.type_cdstationapi/src/use_case/interactor/query.rs (4)
544-553: line_group の重複を祓い、無駄な往還を避く
line_group_id_vecはHashSetで一度去来を祓い、空なら照会を省略するがよろしかろう。往復の無益、ここに断つべし。- let line_group_id_vec = stops - .iter() - .filter_map(|row| row.line_group_cd.map(|id| id as u32)) - .collect::<Vec<u32>>(); + let line_group_id_vec: Vec<u32> = stops + .iter() + .filter_map(|row| row.line_group_cd.map(|id| id as u32)) + .collect::<HashSet<u32>>() + .into_iter() + .collect(); + + // 空なら問い合わせ不要 + if line_group_id_vec.is_empty() { + let stops = stops.iter().map(|row| { + // 既存ロジック（train_type なし）に準ず + // （下段と同趣旨の生成部を共通化できれば尚可） + let mut extracted_line = self.extract_line_from_station(row); + extracted_line.line_symbols = self.get_line_symbols(&extracted_line); + let mut stop = row.clone(); + stop.line = Some(Box::new(extracted_line)); + stop.into() + }).collect::<Vec<proto::Station>>(); + routes.push(Route { id: *id as u32, stops }); + continue; + }
557-563: 駅側の Line にも標章を
extracted_lineをそのまま据えるとline_symbolsが空のまま残る由。表示上の不整合を避けるため、駅側の Line にも標章を与えるべし。- let extracted_line = self.extract_line_from_station(row); + let mut extracted_line = self.extract_line_from_station(row); + extracted_line.line_symbols = self.get_line_symbols(&extracted_line);
655-672: 無標の径にも標章をtrain_type 無しの分岐でも、上と同様に
extracted_lineへ標章を付すことをお勧め申す。- let stop = Station { + let mut extracted_line = self.extract_line_from_station(row); + extracted_line.line_symbols = self.get_line_symbols(&extracted_line); + let stop = Station { … - line: Some(Box::new(extracted_line.clone())), + line: Some(Box::new(extracted_line.clone())), … };
726-733: 経路包含の判定、要件を糺す
anyにて「出発か到着のいずれかを含む」で足るのか、「双方を含む」ことを要件とすべきか。要件の明文化を請う。後者なら次の如く。- let includes_requested_station = stops - .iter() - .any(|stop| stop.group_id == from_station_id || stop.group_id == to_station_id); + let includes_requested_station = + stops.iter().any(|s| s.group_id == from_station_id) + && stops.iter().any(|s| s.group_id == to_station_id);.sqlx/query-323d5e8a03e7c905970e1787748510402a5d36c1166f2d21cf14a100779048d6.json (1)
3-3: 此処も冗語、潔く省くべし
COALESCE(from_sst.line_group_cd, NULL)は単にfrom_sst.line_group_cdと同義。元 SQL を簡潔に正し、cargo sqlx prepareの再生成をお願いいたく。.sqlx/query-422bdec51028a86db49d9e2b0babd7f58066d3a2433f5b6416972dfc359540fa.json (1)
3-3: 式の簡素は徳なり — COALESCE(..., NULL) を除けCOALESCE(x, NULL) は冗長なり。単に x を用いよ。修正後に sqlx prepare を再生成せよ。
- 既検出箇所（要修正）:
- stationapi/src/infrastructure/station_repository.rs （およそ行 1009, 1279）
- .sqlx/query-323d5e8a03e7c905970e1787748510402a5d36c1166f2d21cf14a100779048d6.json
- .sqlx/query-422bdec51028a86db49d9e2b0babd7f58066d3a2433f5b6416972dfc359540fa.json
- 置換例: COALESCE(sst.line_group_cd, NULL)::int AS line_group_cd → sst.line_group_cd::int AS line_group_cd
- 全件検出コマンド: rg -nPi --hidden --no-ignore 'COALESCE(\s*[^,]+,\sNULL\s)'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.sqlx/query-323d5e8a03e7c905970e1787748510402a5d36c1166f2d21cf14a100779048d6.json(3 hunks).sqlx/query-422bdec51028a86db49d9e2b0babd7f58066d3a2433f5b6416972dfc359540fa.json(3 hunks)stationapi/src/infrastructure/station_repository.rs(2 hunks)stationapi/src/use_case/interactor/query.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
stationapi/src/use_case/interactor/query.rs (5)
stationapi/src/infrastructure/line_repository.rs (3)
new(87-89)line_group_id_vec(511-513)line_group_id_vec(574-576)stationapi/src/domain/entity/train_type.rs (1)
new(26-58)stationapi/src/domain/entity/station.rs (1)
new(80-212)stationapi/src/domain/entity/line.rs (1)
new(44-114)stationapi/src/domain/entity/line_symbol.rs (1)
new(11-17)
🔇 Additional comments (4)
.sqlx/query-422bdec51028a86db49d9e2b0babd7f58066d3a2433f5b6416972dfc359540fa.json (1)
232-250: 列順更新の整合確認
line_group_cd→type_id→sst_id→type_cdへの並び替えは StationRow と一致すべし。該当 SELECT 群の全てで同順を守れているか、準備済みメタデータと乖離なきや、cargo sqlx prepare --checkにてご確認願いたい。stationapi/src/use_case/interactor/query.rs (2)
564-583: TrainType 構築は端正、可とす
unwrap_or_defaultにて空字を充てる作法、当座の実装としては妥当。後段の i18n 文言統合方針に即し、将来は None のまま返す可否を検討されたい。
529-539: 鍵衝突の憂 — 検証要すstationapi/src/use_case/interactor/query.rs（L529–539）にて、BTreeMap<i32, Vec> の鍵として line_group_cd 又は line_cd を直に用ゐておる。両者は意味を異にし得、同一の i32 値を取り得るときに別意義のエントリが同一鍵へ集まり上書きされる虞れあり。
対処案：鍵へ種別を折り込め（例： (is_group:bool, id:i32) のタプル、或いは上位ビットで区別する符号化、または新規ラップ型の導入）。
リポジトリ全体で line_group_cd と line_cd の値域・重複有無を検証し、影響範囲を確認せよ。
.sqlx/query-323d5e8a03e7c905970e1787748510402a5d36c1166f2d21cf14a100779048d6.json (1)
232-250: 列位の更改、型と順の符合を再点検
line_group_cdを前倒しにした新配列が、全 SELECT 群・StationRow 双方で矛盾なきや、準備ファイルの検査を。
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
stationapi/src/use_case/interactor/query.rs (1)
730-802: 重複の抽出、見事にて候Station 構築の冗長を一処へ纏め、可読性と保守性を高められました。先のレビュー指摘にも合致しております。
🧹 Nitpick comments (3)
stationapi/src/use_case/interactor/query.rs (3)
579-581: TrainType.lines の複製コストを控え目に各停車駅ごとに全 tt_lines を複写しており、無用の肥大化を招きます。同一 line_group のみを格納するのが程よしと見ます。
- line: Some(Box::new(tt_line.clone())), - lines: tt_lines.to_vec(), + line: Some(Box::new(tt_line.clone())), + lines: tt_lines + .iter() + .filter(|l| l.line_group_cd == row.line_group_cd) + .cloned() + .collect(),
730-736: extracted_line は参照より所有で受け取り、clone を避けませうここは値受けに改めることで余計な clone を省けます。呼び出し側に副作用はございません。
- fn build_station_from_row( + fn build_station_from_row( &self, row: &Station, - extracted_line: &Line, + extracted_line: Line, train_type: Option<Box<TrainType>>, ) -> Station { @@ - line: Some(Box::new(extracted_line.clone())), + line: Some(Box::new(extracted_line)),Also applies to: 751-751
585-586: 上記の値受け変更に伴う呼び出し側の修整参照を外し、所有で手渡すのみで済みます。
- let stop = self.build_station_from_row(row, &extracted_line, train_type); + let stop = self.build_station_from_row(row, extracted_line, train_type); @@ - let stop = self.build_station_from_row(row, &extracted_line, None); + let stop = self.build_station_from_row(row, extracted_line, None);Also applies to: 590-592
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stationapi/src/use_case/interactor/query.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
stationapi/src/use_case/interactor/query.rs (5)
stationapi/src/infrastructure/line_repository.rs (3)
new(87-89)line_group_id_vec(511-513)line_group_id_vec(574-576)stationapi/src/infrastructure/train_type_repository.rs (2)
new(68-70)line_group_id_vec(372-374)stationapi/src/domain/entity/train_type.rs (1)
new(26-58)stationapi/src/domain/entity/station.rs (1)
new(80-212)stationapi/src/domain/entity/line.rs (1)
new(44-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Clippy
- GitHub Check: Test Suite
- GitHub Check: Check
- GitHub Check: Test Suite
- GitHub Check: Clippy
🔇 Additional comments (1)
stationapi/src/use_case/interactor/query.rs (1)
544-553: ```shell
#!/bin/bash
set -euo pipefail
echo "=== 1) 検索: get_by_line_group_id_vec_for_routes ==="
rg -n --hidden --no-ignore -S 'get_by_line_group_id_vec_for_routes' || trueecho
echo "=== 2) マッチの周辺コンテキスト ==="
rg -n --hidden --no-ignore -S 'get_by_line_group_id_vec_for_routes' -C5 || trueecho
echo "=== 3) 検索: LineRepository 実装／トレイト ==="
rg -n --hidden --no-ignore -S 'trait\s+.*Line.*Repository|struct\s+.*Line.*Repository|impl\s+.*Line.*Repository' || trueecho
echo "=== 4) 検索: line_group_id / IN句 に関するパターン ==="
rg -n --hidden --no-ignore -S 'line_group(_)?id.IN|IN\s(|= ANY|= ANY(|ANY\s*(' || trueecho
echo "=== 5) 検索: line_group_id_vec / line_group_cd の利用箇所 ==="
rg -n --hidden --no-ignore -S 'line_group_id_vec|line_group_cd' -C3 || trueecho
echo "=== 6) 対象ファイルの該当箇所表示: stationapi/src/use_case/interactor/query.rs (520-580行) ==="
sed -n '520,580p' stationapi/src/use_case/interactor/query.rs || true</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
1 participant
Summary by CodeRabbit
バグ修正
改善