Conversation
hikaru-0602
requested review from
a team,
Copilot,
kantacky and
masaya-osuga
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
時間割アイテム（TimetableItem）の slot について、曜日・時限が空の場合にAPI/ドメイン変換で slot オブジェクトを生成しないようにして、空データの混入を抑止するPRです。
Changes:
timetableItemToAPIでslotが空（/空白）ならnilを返すように変更toDomainTimetableItemFromRequestでリクエストのslotが空（/空白）ならnilにするように変更- 上記挙動を確認するテストを追加
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/handler/converter.go | TimetableItem の slot 変換時に空値を除外するロジックを追加 |
| internal/handler/converter_timetable_item_test.go | slot の nil/空文字/空白/正常値の変換テストを追加 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if d.Slot != nil && | ||
| strings.TrimSpace(string(d.Slot.DayOfWeek)) != "" && | ||
| strings.TrimSpace(string(d.Slot.Period)) != "" { | ||
| s := timetableSlotToAPI(*d.Slot) | ||
| slot = &s |
There was a problem hiding this comment.
PR説明では「曜日と時限が空の場合（両方空）」と読めますが、ここは DayOfWeek または Period のどちらか一方でも空（/空白）なら Slot を nil にしています。意図が「両方空のときだけ nil」なら条件を調整してください。意図が「片方欠けている Slot は無効」なら、レスポンス側で黙って落とすのではなく、どこかの層でバリデーションしてエラーにする等の方がクライアントの不具合に気づきやすいです。
| if d.Slot != nil && | ||
| strings.TrimSpace(string(d.Slot.DayOfWeek)) != "" && | ||
| strings.TrimSpace(string(d.Slot.Period)) != "" { | ||
| s := timetableSlotToAPI(*d.Slot) | ||
| slot = &s | ||
| } |
There was a problem hiding this comment.
空白チェックに TrimSpace を使っていますが、生成する Slot はトリム前の値（d.Slot.DayOfWeek / d.Slot.Period）をそのまま返しています。例えば " Monday " のように前後に空白がある場合、チェックは通る一方で空白付きの値がAPIに出てしまうので、ここでもトリム済みの値で組み立てるか、そもそもトリムせず厳密に比較するか、どちらかに揃えるのが安全です。
| if req.Slot != nil { | ||
| item.Slot = &domain.TimetableSlot{ | ||
| DayOfWeek: domain.DayOfWeek(req.Slot.DayOfWeek), | ||
| Period: domain.Period(req.Slot.Period), | ||
| dow := strings.TrimSpace(string(req.Slot.DayOfWeek)) | ||
| per := strings.TrimSpace(string(req.Slot.Period)) | ||
| if dow != "" && per != "" { | ||
| item.Slot = &domain.TimetableSlot{ | ||
| DayOfWeek: domain.DayOfWeek(dow), | ||
| Period: domain.Period(per), | ||
| } | ||
| } |
There was a problem hiding this comment.
ここも PR説明（曜日と時限が空の場合）より広い条件で、DayOfWeek または Period のどちらかが空（/空白）だと Slot を丸ごと無視して作成します。部分的な入力が来たときに黙って値が落ちるため、API仕様として「片方だけは不正」としたいなら 400 を返す等の明示的な扱いを検討してください（少なくとも意図をコメントやテストで固定すると安全です）。
| { | ||
| name: "both empty", | ||
| slot: &api.DottoFoundationV1TimetableSlot{ | ||
| DayOfWeek: "", | ||
| Period: "", | ||
| }, | ||
| wantNil: true, | ||
| }, | ||
| { | ||
| name: "whitespace only", | ||
| slot: &api.DottoFoundationV1TimetableSlot{ | ||
| DayOfWeek: " ", | ||
| Period: "\t", | ||
| }, | ||
| wantNil: true, | ||
| }, | ||
| { | ||
| name: "valid", | ||
| slot: &api.DottoFoundationV1TimetableSlot{ | ||
| DayOfWeek: api.Tuesday, | ||
| Period: api.Period2, | ||
| }, | ||
| wantNil: false, | ||
| }, | ||
| } |
There was a problem hiding this comment.
toDomainTimetableItemFromRequest 側は「DayOfWeekだけ空」「Periodだけ空」の分岐が入っていますが、このテストではその2ケースがカバーされていません（両方空/空白のみ/validのみ）。この挙動を仕様として固定するために、片方だけ空のケースも追加しておくと回帰を防げます。
やったこと
曜日と時限が空の場合にオブジェクトが作られないようにする
確認したこと
メモ