[codex] Implement stub handlers and split handler files#22
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces previously stubbed handler endpoints with working proxy implementations to upstream services (Academic, Announcement, and newly wired User API), and reorganizes handler code/tests into resource-focused files to match the project’s per-resource layout.
Changes:
- Implemented real handler logic for previously
not implementedroutes (rooms, course registrations, timetable items, users, etc.) with claim checks and upstream proxying. - Added
user_apiexternal client wiring (USER_API_URL) and passed it through to the handler soUsersV1Detailcan proxy upstream. - Split former stub/test files into per-resource handler and test files; added several new handler tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/infrastructure/clients.go |
Adds user_api client creation and includes it in ExternalClients. |
cmd/server/main.go |
Wires the new user client into handler.NewHandler(...). |
internal/handler/handler.go |
Extends Handler to hold userClient and updates constructor signature/validation. |
internal/handler/user.go |
Implements UsersV1Detail as an authenticated upstream proxy. |
internal/handler/course_registration.go |
Implements course registration list/create/delete proxy handlers. |
internal/handler/timetable_item.go |
Implements timetable list/create/delete proxy handlers and adds a slice conversion helper. |
internal/handler/room.go |
Implements rooms CRUD and reservations list proxy handlers. |
internal/handler/test_helper_test.go |
Centralizes test handler construction and admin-claim setup for handler tests. |
internal/handler/course_registration_test.go |
Adds proxy test for course registration creation. |
internal/handler/personal_calendar_item_test.go |
Adds proxy test for personal calendar items list. |
internal/handler/user_test.go |
Adds proxy test for user detail. |
internal/handler/subject_test.go |
Removes now-duplicated local helpers/tests after test split. |
internal/handler/stub.go |
Removes the old not implemented handler stubs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| gotMethod = r.Method | ||
| gotPath = r.URL.Path | ||
|
|
||
| if err := json.NewDecoder(r.Body).Decode(&gotBody); err != nil { | ||
| t.Fatalf("decode request body: %v", err) | ||
| } |
There was a problem hiding this comment.
The httptest server handler calls t.Fatalf from its own goroutine (net/http serves requests concurrently). In Go tests, Fatal/FailNow should be called only from the test goroutine; this can lead to confusing behavior and makes the test harder to reason about. Capture assertion failures via a channel (or store them and assert after the handler call) instead of calling t.Fatalf inside the server handler.
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| query := r.URL.Query() | ||
| if got := query.Get("userId"); got != "user-1" { | ||
| t.Fatalf("userId = %q, want %q", got, "user-1") | ||
| } | ||
| if got := query.Get("dates"); got == "" { | ||
| t.Fatal("dates query parameter is empty") | ||
| } |
There was a problem hiding this comment.
The httptest server handler calls t.Fatalf/t.Fatal from its own goroutine. In Go tests, Fatal/FailNow should be called only from the test goroutine; consider reporting failures back to the main test goroutine via a channel and asserting after the request completes.
| // RoomsV1List 教室一覧を取得する | ||
| func (h *Handler) RoomsV1List(c *gin.Context, params api.RoomsV1ListParams) { | ||
| if !middleware.RequireAnyClaim(c, "admin", "developer") { | ||
| return | ||
| } | ||
|
|
||
| response, err := h.academicClient.RoomsV1ListWithResponse(c.Request.Context(), &academic_api.RoomsV1ListParams{ | ||
| Floor: convertSlicePtr[api.DottoFoundationV1Floor, academic_api.DottoFoundationV1Floor](params.Floor), | ||
| }) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| if response.JSON200 == nil { | ||
| c.JSON(response.StatusCode(), gin.H{"error": "unexpected response from upstream"}) | ||
| return | ||
| } | ||
|
|
||
| c.JSON(http.StatusOK, response.JSON200) | ||
| } |
There was a problem hiding this comment.
These handlers implement previously-stubbed room endpoints, but there are no corresponding tests to verify request/response proxying (query/body pass-through, status codes, auth claim enforcement). Adding at least a minimal httptest-based proxy test for one read and one write path would help prevent regressions.
| // TimetableItemsV1List 時間割を取得する | ||
| func (h *Handler) TimetableItemsV1List(c *gin.Context, params api.TimetableItemsV1ListParams) { | ||
| if !middleware.RequireAnyClaim(c, "admin", "developer") { | ||
| return | ||
| } | ||
|
|
||
| response, err := h.academicClient.TimetableItemsV1ListWithResponse(c.Request.Context(), &academic_api.TimetableItemsV1ListParams{ | ||
| Year: params.Year, | ||
| Semesters: convertSlice[api.DottoFoundationV1CourseSemester, academic_api.DottoFoundationV1CourseSemester](params.Semesters), | ||
| }) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| if response.JSON200 == nil { | ||
| c.JSON(response.StatusCode(), gin.H{"error": "unexpected response from upstream"}) | ||
| return | ||
| } | ||
|
|
||
| c.JSON(http.StatusOK, gin.H{ | ||
| "items": response.JSON200.TimetableItems, | ||
| }) | ||
| } | ||
|
|
||
| // TimetableItemsV1Create 時間割に追加する | ||
| func (h *Handler) TimetableItemsV1Create(c *gin.Context) { | ||
| if !middleware.RequireAnyClaim(c, "admin", "developer") { | ||
| return | ||
| } | ||
|
|
||
| var req academic_api.TimetableItemRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| response, err := h.academicClient.TimetableItemsV1CreateWithResponse(c.Request.Context(), req) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| if response.JSON201 == nil { | ||
| c.JSON(response.StatusCode(), gin.H{"error": "unexpected response from upstream"}) | ||
| return | ||
| } | ||
|
|
||
| c.JSON(http.StatusCreated, gin.H{ | ||
| "item": response.JSON201.TimetableItem, | ||
| }) | ||
| } |
There was a problem hiding this comment.
These handlers implement previously-stubbed timetable endpoints, but there are no tests covering the proxy behavior (semester param mapping, request body binding, status code handling, and auth claim enforcement). Adding at least one list/create/delete test using httptest would improve confidence and catch upstream contract changes.
What changed
not implementeduser_apiclient wiring soUsersV1Detailcan proxy upstreamstub.goandhandler_test.gointo resource-focused filesWhy
These endpoints were still stubbed, so the admin BFF could not serve those routes. The follow-up file split keeps handler code and tests aligned with the existing per-resource layout and makes further changes easier to isolate.
Impact
USER_API_URLin the external client configurationValidation
GOROOT=/Users/kantacky/.local/share/mise/installs/go/1.25.7 GOCACHE=/Users/kantacky/Developer/dotto-admin-bff-api/.cache/go-build go test -run ^ ./internal/handler ./internal/infrastructure ./cmd/servergo testwas not run in this environment because sandbox restrictions block the local listener used by the tests