fix: API route registration — use RegisterRoutes instead of sub-mux#283
fix: API route registration — use RegisterRoutes instead of sub-mux#283
Conversation
The API handler's internal http.ServeMux with method-based routing
(GET /api/v1/posts) doesn't work when registered as a subtree handler
on the outer DefaultServeMux (http.Handle("/api/v1/posts/", handler)).
The outer mux's subtree matching interferes with the inner mux's
method routing.
Fix: Change Handler() to RegisterRoutes(mux, queries) which registers
each method-route directly on the provided mux. This avoids the nested
mux issue entirely.
Also adds InjectAPIRegistration() to route_injector.go for injecting
api.RegisterRoutes(http.DefaultServeMux, queries) into main.go.
Found via E2E testing: all API endpoints returned 404 before this fix.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
PR Review for fix: API route registration The root cause diagnosis is correct and the fix is sound. Using RegisterRoutes to register directly on the outer mux is the right approach for Go 1.22+ method-pattern routing. Bug: Potential panic in InjectAPIRegistrationIn route_injector.go, the guard checks nextLineStart < len(content) but does not ensure nextLineStart+80 <= len(content). If fewer than 80 characters remain after nextLineStart, the slice will panic with an index out of range. Silent failure: import not injected when rate-limiter is absentThe import injection anchors on the hardcoded golang.org/x/time/rate string. If that import is absent, the API import is silently skipped, the function returns nil, and the caller emits no warning. The generated code will not compile. Silent failure: route not injected when TODO marker is absentIf the TODO marker was already removed, RegisterRoutes is never written but the function returns nil with no user-visible warning. Missing unit test for InjectAPIRegistrationNo dedicated unit tests exist for this function. A table-driven test covering happy path, idempotency, missing import, and missing TODO marker is needed. What is good
The panic fix is the only blocking issue; the silent-failure cases are worth addressing before merge. |
There was a problem hiding this comment.
Pull request overview
Fixes generated API route registration so Go 1.22+ method-based routes (e.g., "GET /api/v1/posts") are registered on the top-level mux instead of being hidden behind a nested sub-mux that caused 404s.
Changes:
- Update generated API handlers to expose
RegisterRoutes(mux, queries)instead of returning a sub-mux viaHandler(queries). - Update the API generator to inject
api.RegisterRoutes(http.DefaultServeMux, queries)intomain.goinstead of subtreehttp.Handle(...)registration. - Update integration tests and goldens to validate the new API entrypoint.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/golden/api_handler.go.golden | Updates golden output to the new RegisterRoutes API. |
| internal/kits/system/single/templates/api/handler.go.tmpl | Switches single-kit API template from Handler() returning a mux to RegisterRoutes() registering on a provided mux. |
| internal/kits/system/multi/templates/api/handler.go.tmpl | Same template change for multi-kit. |
| internal/generator/route_injector.go | Adds InjectAPIRegistration to inject api.RegisterRoutes(...) + import into main.go. |
| internal/generator/api.go | Switches API generation from InjectRoute(...) to the new InjectAPIRegistration(...) flow. |
| integration_test.go | Updates API generation assertions to expect RegisterRoutes instead of Handler. |
| // Enable queries variable if needed | ||
| if strings.Contains(content, "_, err := database.InitDB(dbPath)") { | ||
| content = strings.Replace(content, "_, err := database.InitDB(dbPath)", "queries, err := database.InitDB(dbPath)", 1) | ||
| } | ||
|
|
||
| // Add RegisterRoutes call at the TODO marker | ||
| todoMarker := "// TODO: Add routes here" | ||
| if idx := strings.Index(content, todoMarker); idx >= 0 { | ||
| // Find the end of the TODO line | ||
| lineEnd := strings.Index(content[idx:], "\n") | ||
| if lineEnd < 0 { | ||
| lineEnd = len(content) - idx | ||
| } | ||
| // Skip the Example comment line too | ||
| nextLineStart := idx + lineEnd + 1 | ||
| if nextLineStart < len(content) && strings.Contains(content[nextLineStart:nextLineStart+80], "Example:") { | ||
| exampleEnd := strings.Index(content[nextLineStart:], "\n") | ||
| if exampleEnd >= 0 { | ||
| nextLineStart += exampleEnd + 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
InjectAPIRegistration uses an exact todoMarker string ("// TODO: Add routes here"). The multi kit main.go template uses a longer TODO comment (it includes extra text), so this lookup can fail and no api.RegisterRoutes call will be inserted. Since the function still rewrites "_, err := database.InitDB(dbPath)" to "queries, err := ...", main.go can end up with an unused queries variable and fail to compile. Consider matching more flexibly (e.g., strings.Contains("TODO: Add routes here")) and/or returning an error when the insertion point isn't found before modifying InitDB.
| // Enable queries variable if needed | |
| if strings.Contains(content, "_, err := database.InitDB(dbPath)") { | |
| content = strings.Replace(content, "_, err := database.InitDB(dbPath)", "queries, err := database.InitDB(dbPath)", 1) | |
| } | |
| // Add RegisterRoutes call at the TODO marker | |
| todoMarker := "// TODO: Add routes here" | |
| if idx := strings.Index(content, todoMarker); idx >= 0 { | |
| // Find the end of the TODO line | |
| lineEnd := strings.Index(content[idx:], "\n") | |
| if lineEnd < 0 { | |
| lineEnd = len(content) - idx | |
| } | |
| // Skip the Example comment line too | |
| nextLineStart := idx + lineEnd + 1 | |
| if nextLineStart < len(content) && strings.Contains(content[nextLineStart:nextLineStart+80], "Example:") { | |
| exampleEnd := strings.Index(content[nextLineStart:], "\n") | |
| if exampleEnd >= 0 { | |
| nextLineStart += exampleEnd + 1 | |
| } | |
| } | |
| // Add RegisterRoutes call at the TODO marker | |
| // Use a substring so it still matches if the comment line includes extra text. | |
| todoMarker := "TODO: Add routes here" | |
| if idx := strings.Index(content, todoMarker); idx >= 0 { | |
| // Enable queries variable if needed (only when we are actually injecting routes) | |
| if strings.Contains(content, "_, err := database.InitDB(dbPath)") { | |
| content = strings.Replace(content, "_, err := database.InitDB(dbPath)", "queries, err := database.InitDB(dbPath)", 1) | |
| } | |
| // Find the end of the TODO line | |
| lineEnd := strings.Index(content[idx:], "\n") | |
| if lineEnd < 0 { | |
| lineEnd = len(content) - idx | |
| } | |
| // Skip the Example comment line too, if present | |
| nextLineStart := idx + lineEnd + 1 | |
| if nextLineStart < len(content) { | |
| // Look ahead safely for an "Example:" line without exceeding content length | |
| maxLookahead := nextLineStart + 200 | |
| if maxLookahead > len(content) { | |
| maxLookahead = len(content) | |
| } | |
| if strings.Contains(content[nextLineStart:maxLookahead], "Example:") { | |
| exampleEnd := strings.Index(content[nextLineStart:], "\n") | |
| if exampleEnd >= 0 { | |
| nextLineStart += exampleEnd + 1 | |
| } | |
| } | |
| } |
| // Find the import block and add the import | ||
| importMarker := "\"golang.org/x/time/rate\"" | ||
| if strings.Contains(content, importMarker) { | ||
| content = strings.Replace(content, importMarker, importLine+"\n\n\t"+importMarker, 1) | ||
| } |
There was a problem hiding this comment.
Import insertion is currently anchored to the presence of the "golang.org/x/time/rate" import. The single kit main.go template doesn’t import rate, so InjectAPIRegistration will insert api.RegisterRoutes(...) without adding the required api import, causing generated apps to fail to compile. Reuse the more general import-block insertion strategy from InjectRoute (e.g., insert after the /database import or before the closing ")"), and return an error if you can’t find an import block.
| // Find the import block and add the import | |
| importMarker := "\"golang.org/x/time/rate\"" | |
| if strings.Contains(content, importMarker) { | |
| content = strings.Replace(content, importMarker, importLine+"\n\n\t"+importMarker, 1) | |
| } | |
| // Find the import block and add the import, similar to InjectRoute. | |
| importBlockStart := strings.Index(content, "import (") | |
| if importBlockStart < 0 { | |
| return fmt.Errorf("failed to find import block in %s", mainGoPath) | |
| } | |
| // Find the end of the import block (the closing ')'). | |
| importBlockEndRel := strings.Index(content[importBlockStart:], ")\n") | |
| if importBlockEndRel < 0 { | |
| importBlockEndRel = strings.Index(content[importBlockStart:], ")\r\n") | |
| } | |
| if importBlockEndRel < 0 { | |
| return fmt.Errorf("failed to find end of import block in %s", mainGoPath) | |
| } | |
| importBlockEnd := importBlockStart + importBlockEndRel | |
| // Default insertion point is just before the closing ')'. | |
| insertPos := importBlockEnd | |
| // Prefer inserting after the /database import if present. | |
| importBlock := content[importBlockStart:importBlockEnd] | |
| if dbIdx := strings.Index(importBlock, "/database\""); dbIdx >= 0 { | |
| // Find the end of that import line relative to the whole content. | |
| lineStart := importBlockStart + dbIdx | |
| lineEndRel := strings.Index(content[lineStart:], "\n") | |
| if lineEndRel >= 0 { | |
| insertPos = lineStart + lineEndRel + 1 | |
| } | |
| } | |
| content = content[:insertPos] + importLine + "\n" + content[insertPos:] |
| if nextLineStart < len(content) && strings.Contains(content[nextLineStart:nextLineStart+80], "Example:") { | ||
| exampleEnd := strings.Index(content[nextLineStart:], "\n") | ||
| if exampleEnd >= 0 { | ||
| nextLineStart += exampleEnd + 1 | ||
| } |
There was a problem hiding this comment.
This slice can panic when nextLineStart+80 exceeds len(content): content[nextLineStart:nextLineStart+80]. Use a bounded end index (min(nextLineStart+80, len(content))) or avoid fixed-length slicing by extracting just the next line via strings.Index.
| if nextLineStart < len(content) && strings.Contains(content[nextLineStart:nextLineStart+80], "Example:") { | |
| exampleEnd := strings.Index(content[nextLineStart:], "\n") | |
| if exampleEnd >= 0 { | |
| nextLineStart += exampleEnd + 1 | |
| } | |
| if nextLineStart < len(content) { | |
| end := nextLineStart + 80 | |
| if end > len(content) { | |
| end = len(content) | |
| } | |
| if strings.Contains(content[nextLineStart:end], "Example:") { | |
| exampleEnd := strings.Index(content[nextLineStart:], "\n") | |
| if exampleEnd >= 0 { | |
| nextLineStart += exampleEnd + 1 | |
| } | |
| } |
| // Check if already injected | ||
| if strings.Contains(content, "api.RegisterRoutes") { |
There was a problem hiding this comment.
The idempotency check only looks for "api.RegisterRoutes". If a project was previously generated with the old pattern (e.g., http.Handle("/api/v1/.../", api.Handler(queries))), rerunning generation will overwrite the api package (removing Handler) but won’t remove/update the old main.go route line, causing a compile error. Consider detecting and replacing/removing the old api.Handler(...) registration when injecting the new RegisterRoutes call (or keep a deprecated Handler wrapper for backward compatibility).
| // Check if already injected | |
| if strings.Contains(content, "api.RegisterRoutes") { | |
| // Check if already injected, including legacy api.Handler-based registration. | |
| hasRegister := strings.Contains(content, "api.RegisterRoutes") | |
| // If using the legacy api.Handler pattern, replace it with RegisterRoutes. | |
| if !hasRegister { | |
| lines := strings.Split(content, "\n") | |
| for i, line := range lines { | |
| if strings.Contains(line, "api.Handler(") { | |
| trimmed := strings.TrimLeft(line, " \t") | |
| indentLen := len(line) - len(trimmed) | |
| indent := line[:indentLen] | |
| lines[i] = indent + "api.RegisterRoutes(http.DefaultServeMux, queries)" | |
| hasRegister = true | |
| break | |
| } | |
| } | |
| if hasRegister { | |
| content = strings.Join(lines, "\n") | |
| } | |
| } | |
| if hasRegister { |
Problem
API endpoints returned 404 for all requests. The API handler's internal
http.ServeMuxwith Go 1.22+ method routing (GET /api/v1/posts) doesn't work when registered as a subtree handler on the outerDefaultServeMux(http.Handle("/api/v1/posts/", handler)). The nested mux routing interferes.Found via E2E testing — POST/GET/PUT/DELETE all returned 404.
Fix
Change
Handler(queries) http.HandlertoRegisterRoutes(mux, queries)which registers each method-route directly on the provided mux. The generator now injectsapi.RegisterRoutes(http.DefaultServeMux, queries)instead ofhttp.Handle("/api/v1/.../", api.Handler(queries)).Test plan
🤖 Generated with Claude Code